From 2baaeb78eca08ff794eaeeaf9cfc45d9961f50d5 Mon Sep 17 00:00:00 2001 From: Thomas Coratger <60488569+tcoratger@users.noreply.github.com> Date: Mon, 4 Dec 2023 18:02:32 +0100 Subject: [PATCH] fix: modify `get` method in `Memory` to return `null` instead of error (#207) * modify get method in memory to return null instead of error * fix tests --- src/vm/builtins/bitwise/bitwise.zig | 4 +- src/vm/builtins/builtin_runner/keccak.zig | 8 +- .../builtins/builtin_runner/range_check.zig | 4 +- src/vm/core.zig | 12 +- src/vm/core_test.zig | 26 ++-- src/vm/memory/memory.zig | 115 +++++++++--------- src/vm/memory/segments.zig | 12 +- 7 files changed, 86 insertions(+), 95 deletions(-) diff --git a/src/vm/builtins/bitwise/bitwise.zig b/src/vm/builtins/bitwise/bitwise.zig index f7994a84..32dec068 100644 --- a/src/vm/builtins/bitwise/bitwise.zig +++ b/src/vm/builtins/bitwise/bitwise.zig @@ -35,9 +35,7 @@ const BITWISE_INPUT_CELLS_PER_INSTANCE = 2; /// # Returns /// The felt as an integer. fn getValue(address: Relocatable, memory: *Memory) BitwiseError!u256 { - const value = memory.get(address) catch { - return BitwiseError.InvalidAddressForBitwise; - }; + const value = memory.get(address); if (value) |v| { var felt = v.tryIntoFelt() catch { diff --git a/src/vm/builtins/builtin_runner/keccak.zig b/src/vm/builtins/builtin_runner/keccak.zig index c13ba320..e6e5bfb1 100644 --- a/src/vm/builtins/builtin_runner/keccak.zig +++ b/src/vm/builtins/builtin_runner/keccak.zig @@ -346,9 +346,7 @@ pub const KeccakBuiltinRunner = struct { const stop_pointer_addr = pointer.subUint( @intCast(1), ) catch return RunnerError.NoStopPointer; - const stop_pointer = try ((segments.memory.get( - stop_pointer_addr, - ) catch return RunnerError.NoStopPointer) orelse return RunnerError.NoStopPointer).tryIntoRelocatable(); + const stop_pointer = try (segments.memory.get(stop_pointer_addr) orelse return RunnerError.NoStopPointer).tryIntoRelocatable(); if (@as( isize, @intCast(self.base), @@ -430,9 +428,7 @@ pub const KeccakBuiltinRunner = struct { usize, @intCast(self.n_input_cells), )) |i| { - const num = ((memory.get(try first_input_addr.addUint(@intCast(i))) catch { - return null; - }) orelse return null).tryIntoFelt() catch { + const num = (memory.get(try first_input_addr.addUint(@intCast(i))) orelse return null).tryIntoFelt() catch { return RunnerError.BuiltinExpectedInteger; }; diff --git a/src/vm/builtins/builtin_runner/range_check.zig b/src/vm/builtins/builtin_runner/range_check.zig index 6980205c..0b6b2580 100644 --- a/src/vm/builtins/builtin_runner/range_check.zig +++ b/src/vm/builtins/builtin_runner/range_check.zig @@ -192,9 +192,7 @@ pub const RangeCheckBuiltinRunner = struct { const stop_pointer_addr = pointer.subUint( @intCast(1), ) catch return RunnerError.NoStopPointer; - const stop_pointer = try (segments.memory.get( - stop_pointer_addr, - ) catch return RunnerError.NoStopPointer).tryIntoRelocatable(); + const stop_pointer = try (segments.memory.get(stop_pointer_addr)).tryIntoRelocatable(); if (@as( isize, @intCast(self.base), diff --git a/src/vm/core.zig b/src/vm/core.zig index 1cf3dc59..101299e0 100644 --- a/src/vm/core.zig +++ b/src/vm/core.zig @@ -142,7 +142,7 @@ pub const CairoVM = struct { pub fn getRelocatable( self: *Self, address: Relocatable, - ) error{MemoryOutOfBounds}!?MaybeRelocatable { + ) ?MaybeRelocatable { return self.segments.memory.get(address); } @@ -232,9 +232,7 @@ pub const CairoVM = struct { // * FETCH * // ************************************************************ - const encoded_instruction = self.segments.memory.get(self.run_context.pc.*) catch { - return CairoVMError.InstructionFetchingFailed; - }; + const encoded_instruction = self.segments.memory.get(self.run_context.pc.*); // ************************************************************ // * DECODE * @@ -326,7 +324,7 @@ pub const CairoVM = struct { op_res.res = null; op_res.dst_addr = try self.run_context.computeDstAddr(instruction); - const dst_op = try self.segments.memory.get(op_res.dst_addr); + const dst_op = self.segments.memory.get(op_res.dst_addr); op_res.op_0_addr = try self.run_context.computeOp0Addr(instruction); @@ -334,11 +332,11 @@ pub const CairoVM = struct { instruction, op_res.op_0, ); - const op_1_op = try self.segments.memory.get(op_res.op_1_addr); + const op_1_op = self.segments.memory.get(op_res.op_1_addr); // Deduce the operands if they haven't been successfully retrieved from memory. - if (self.segments.memory.get(op_res.op_0_addr) catch null) |op_0| { + if (self.segments.memory.get(op_res.op_0_addr)) |op_0| { op_res.op_0 = op_0; } else { op_res.setOp0(true); diff --git a/src/vm/core_test.zig b/src/vm/core_test.zig index 6053e100..a45d6733 100644 --- a/src/vm/core_test.zig +++ b/src/vm/core_test.zig @@ -887,7 +887,7 @@ test "set get value in vm memory" { // Test checks // Verify the value is correctly set to 42. - const actual_value = try vm.segments.memory.get(address); + const actual_value = vm.segments.memory.get(address); const expected_value = value; try expectEqual( expected_value, @@ -1548,8 +1548,8 @@ test "CairoVM: getRelocatable without value raises error" { defer vm.deinit(); // Test check - try expectError( - error.MemoryOutOfBounds, + try expectEqual( + @as(?MaybeRelocatable, null), vm.getRelocatable(Relocatable.new(0, 0)), ); } @@ -1571,7 +1571,7 @@ test "CairoVM: getRelocatable with value should return a MaybeRelocatable" { // Test check try expectEqual( MaybeRelocatable.fromU256(5), - (try vm.getRelocatable(Relocatable.new(34, 12))).?, + (vm.getRelocatable(Relocatable.new(34, 12))).?, ); } @@ -1913,15 +1913,15 @@ test "CairoVM: InserDeducedOperands should insert operands if set as deduced" { // Test checks try expectEqual( - try vm.segments.memory.get(Relocatable.new(1, 0)), + vm.segments.memory.get(Relocatable.new(1, 0)), dst_val, ); try expectEqual( - try vm.segments.memory.get(Relocatable.new(1, 1)), + vm.segments.memory.get(Relocatable.new(1, 1)), op0_val, ); try expectEqual( - try vm.segments.memory.get(Relocatable.new(1, 2)), + vm.segments.memory.get(Relocatable.new(1, 2)), op1_val, ); } @@ -1967,16 +1967,16 @@ test "CairoVM: InserDeducedOperands insert operands should not be inserted if no try vm.insertDeducedOperands(allocator, test_operands); // Test checks - try expectError( - error.MemoryOutOfBounds, + try expectEqual( + @as(?MaybeRelocatable, null), vm.segments.memory.get(Relocatable.new(1, 0)), ); - try expectError( - error.MemoryOutOfBounds, + try expectEqual( + @as(?MaybeRelocatable, null), vm.segments.memory.get(Relocatable.new(1, 1)), ); - try expectError( - error.MemoryOutOfBounds, + try expectEqual( + @as(?MaybeRelocatable, null), vm.segments.memory.get(Relocatable.new(1, 2)), ); } diff --git a/src/vm/memory/memory.zig b/src/vm/memory/memory.zig index 53de5c64..8b91fc2b 100644 --- a/src/vm/memory/memory.zig +++ b/src/vm/memory/memory.zig @@ -382,24 +382,20 @@ pub const Memory = struct { data_segment.items[address.offset] = MemoryCell.new(value); } - // Get some value from the memory at the given address. - // # Arguments - // - `address` - The address to get the value from. - // # Returns - // The value at the given address. - pub fn get( - self: *Self, - address: Relocatable, - ) error{MemoryOutOfBounds}!?MaybeRelocatable { + /// Retrieves data at a specified address within a relocatable data structure. + /// + /// - `self`: The instance of the data structure. + /// - `address`: The target address to retrieve data from. + /// Returns: + /// A `MaybeRelocatable` value if the address is valid; otherwise, returns `null`. + pub fn get(self: *Self, address: Relocatable) ?MaybeRelocatable { const data = if (address.segment_index < 0) &self.temp_data else &self.data; const segment_index: usize = @intCast(if (address.segment_index < 0) -(address.segment_index + 1) else address.segment_index); const isSegmentIndexValid = address.segment_index < data.items.len; const isOffsetValid = isSegmentIndexValid and (address.offset < data.items[segment_index].items.len); - if (!isSegmentIndexValid or !isOffsetValid) { - return CairoVMError.MemoryOutOfBounds; - } + if (!isSegmentIndexValid or !isOffsetValid) return null; if (data.items[segment_index].items[@intCast(address.offset)]) |val| { return val.maybe_relocatable; @@ -423,8 +419,8 @@ pub const Memory = struct { pub fn getFelt( self: *Self, address: Relocatable, - ) error{ MemoryOutOfBounds, ExpectedInteger }!Felt252 { - if (try self.get(address)) |m| { + ) error{ ExpectedInteger, MemoryOutOfBounds }!Felt252 { + if (self.get(address)) |m| { return switch (m) { .felt => |fe| fe, else => error.ExpectedInteger, @@ -449,8 +445,8 @@ pub const Memory = struct { pub fn getRelocatable( self: *Self, address: Relocatable, - ) error{ MemoryOutOfBounds, ExpectedRelocatable }!Relocatable { - if (try self.get(address)) |m| { + ) error{ExpectedRelocatable}!Relocatable { + if (self.get(address)) |m| { return switch (m) { .relocatable => |rel| rel, else => error.ExpectedRelocatable, @@ -697,7 +693,7 @@ pub const Memory = struct { ) !std.ArrayList(?MaybeRelocatable) { var values = std.ArrayList(?MaybeRelocatable).init(allocator); for (0..size) |i| { - try values.append(try self.get(try address.addUint(@intCast(i)))); + try values.append(self.get(try address.addUint(@intCast(i)))); } return values; } @@ -752,7 +748,7 @@ pub const Memory = struct { ); errdefer values.deinit(); for (0..size) |i| { - if (try self.get(try address.addUint(@intCast(i)))) |elem| { + if (self.get(try address.addUint(@intCast(i)))) |elem| { try values.append(elem); } else { return MemoryError.GetRangeMemoryGap; @@ -974,10 +970,8 @@ test "memory inner for testing test" { ); } -test "memory get without value raises error" { - // ************************************************************ - // * SETUP TEST CONTEXT * - // ************************************************************ +test "Memory: get method without segment should return null" { + // Test setup // Initialize an allocator. const allocator = std.testing.allocator; @@ -985,20 +979,16 @@ test "memory get without value raises error" { var memory = try Memory.init(allocator); defer memory.deinit(); - // ************************************************************ - // * TEST CHECKS * - // ************************************************************ + // Test checks // Get a value from the memory at an address that doesn't exist. - try expectError( - error.MemoryOutOfBounds, + try expectEqual( + @as(?MaybeRelocatable, null), memory.get(Relocatable.new(0, 0)), ); } -test "memory set and get" { - // ************************************************************ - // * SETUP TEST CONTEXT * - // ************************************************************ +test "Memory: get method wit segment but non allocated memory should return null" { + // Test setup // Initialize an allocator. const allocator = std.testing.allocator; @@ -1006,20 +996,33 @@ test "memory set and get" { var memory = try Memory.init(allocator); defer memory.deinit(); - // ************************************************************ - // * TEST BODY * - // ************************************************************ - const address_1 = Relocatable.new( - 0, - 0, + // Set a value into the memory. + try setUpMemory( + memory, + std.testing.allocator, + .{ + .{ .{ 0, 0 }, .{1} }, + .{ .{ 0, 10 }, .{1} }, + }, ); - const value_1 = MaybeRelocatable.fromFelt(starknet_felt.Felt252.one()); + defer memory.deinitData(std.testing.allocator); - const address_2 = Relocatable.new( - -1, - 0, + // Test checks + // Get a value from the memory at an address that doesn't exist. + try expectEqual( + @as(?MaybeRelocatable, null), + memory.get(Relocatable.new(0, 15)), ); - const value_2 = MaybeRelocatable.fromFelt(starknet_felt.Felt252.one()); +} + +test "Memory: set and get for both segments and temporary segments should return proper MaybeRelocatable values" { + // Test setup + // Initialize an allocator. + const allocator = std.testing.allocator; + + // Initialize a memory instance. + var memory = try Memory.init(allocator); + defer memory.deinit(); // Set a value into the memory. try setUpMemory( @@ -1030,19 +1033,17 @@ test "memory set and get" { .{ .{ -1, 0 }, .{1} }, }, ); - defer memory.deinitData(std.testing.allocator); - // Get the value from the memory. - const maybe_value_1 = try memory.get(address_1); - const maybe_value_2 = try memory.get(address_2); - - // ************************************************************ - // * TEST CHECKS * - // ************************************************************ - // Assert that the value is the expected value. - try expect(maybe_value_1.?.eq(value_1)); - try expect(maybe_value_2.?.eq(value_2)); + // Test checks + try expectEqual( + @as(?MaybeRelocatable, MaybeRelocatable.fromU256(1)), + memory.get(Relocatable.new(0, 0)), + ); + try expectEqual( + @as(?MaybeRelocatable, MaybeRelocatable.fromU256(1)), + memory.get(Relocatable.new(-1, 0)), + ); } test "Memory: get inside a segment without value but inbout should return null" { @@ -1069,7 +1070,7 @@ test "Memory: get inside a segment without value but inbout should return null" // Test check try expectEqual( @as(?MaybeRelocatable, null), - try memory.get(Relocatable.new(1, 3)), + memory.get(Relocatable.new(1, 3)), ); } @@ -1125,7 +1126,7 @@ test "validate existing memory for range check within bound" { defer memory.deinitData(std.testing.allocator); // Get the value from the memory. - const maybe_value_1 = try memory.get(address_1); + const maybe_value_1 = memory.get(address_1); // ************************************************************ // * TEST CHECKS * @@ -1184,14 +1185,14 @@ test "Memory: getFelt should return ExpectedInteger error if Relocatable instead ); } -test "Memory: getRelocatable should return MemoryOutOfBounds error if no value at the given address" { +test "Memory: getRelocatable should return ExpectedRelocatable error if no value at the given address" { // Test setup var memory = try Memory.init(std.testing.allocator); defer memory.deinit(); // Test checks try expectError( - error.MemoryOutOfBounds, + error.ExpectedRelocatable, memory.getRelocatable(Relocatable.new(0, 0)), ); } diff --git a/src/vm/memory/segments.zig b/src/vm/memory/segments.zig index 29f06417..5a582fd4 100644 --- a/src/vm/memory/segments.zig +++ b/src/vm/memory/segments.zig @@ -442,9 +442,9 @@ test "set get integer value in segment memory" { // ************************************************************ // * TEST CHECKS * // ************************************************************ - const actual_value_1 = try memory_segment_manager.memory.get(address_1); + const actual_value_1 = memory_segment_manager.memory.get(address_1); const expected_value_1 = value_1; - const actual_value_2 = try memory_segment_manager.memory.get(address_2); + const actual_value_2 = memory_segment_manager.memory.get(address_2); const expected_value_2 = value_2; try expect(expected_value_1.eq(actual_value_1.?)); @@ -908,7 +908,7 @@ test "MemorySegmentManager: loadData with one element" { try expectEqual(Relocatable.new(0, 1), actual); try expectEqual( MaybeRelocatable.fromU256(4), - (try memory_segment_manager.memory.get(Relocatable.new(0, 0))).?, + (memory_segment_manager.memory.get(Relocatable.new(0, 0))).?, ); } @@ -936,15 +936,15 @@ test "MemorySegmentManager: loadData with three elements" { try expectEqual(Relocatable.new(0, 3), actual); try expectEqual( MaybeRelocatable.fromU256(4), - (try memory_segment_manager.memory.get(Relocatable.new(0, 0))).?, + (memory_segment_manager.memory.get(Relocatable.new(0, 0))).?, ); try expectEqual( MaybeRelocatable.fromU256(5), - (try memory_segment_manager.memory.get(Relocatable.new(0, 1))).?, + (memory_segment_manager.memory.get(Relocatable.new(0, 1))).?, ); try expectEqual( MaybeRelocatable.fromU256(6), - (try memory_segment_manager.memory.get(Relocatable.new(0, 2))).?, + (memory_segment_manager.memory.get(Relocatable.new(0, 2))).?, ); }