Skip to content

Commit

Permalink
fix(avm): reenable tag error sload (#7153)
Browse files Browse the repository at this point in the history 
Please read [contributing guidelines](CONTRIBUTING.md) and remove this line.
  • Loading branch information
@IlyasRidhuan
IlyasRidhuan authored Jun 24, 2024
1 parent 33ccf1b commit fd92d46
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 55 deletions.
80 changes: 36 additions & 44 deletions barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_trace.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1572,60 +1572,51 @@ void AvmTraceBuilder::op_sload(uint8_t indirect, uint32_t slot_offset, uint32_t
auto clk = static_cast<uint32_t>(main_trace.size()) + 1;

auto [resolved_slot, resolved_dest] = unpack_indirects<2>(indirect, { slot_offset, dest_offset });
auto read_dest_value = constrained_read_from_memory(
call_ptr, clk, resolved_dest, AvmMemoryTag::FF, AvmMemoryTag::FF, IntermRegister::IA);
auto read_slot = constrained_read_from_memory(
call_ptr, clk, resolved_slot, AvmMemoryTag::FF, AvmMemoryTag::FF, IntermRegister::IB);
// TODO: Reenable tag_match
// bool tag_match = read_dest_value.tag_match && read_slot.tag_match;
call_ptr, clk, resolved_slot, AvmMemoryTag::FF, AvmMemoryTag::U0, IntermRegister::IA);

// Read the slot value that we will write hints to in a row
main_trace.push_back(Row{
.main_clk = clk,
.main_ia = read_dest_value.val,
.main_ib = read_slot.val,
.main_ind_addr_a = FF(read_dest_value.indirect_address),
.main_ind_addr_b = FF(read_slot.indirect_address),
.main_ia = read_slot.val,
.main_ind_addr_a = FF(read_slot.indirect_address),
.main_internal_return_ptr = FF(internal_return_ptr),
.main_mem_addr_a = FF(read_dest_value.direct_address),
.main_mem_addr_b = FF(read_slot.direct_address),
.main_mem_addr_a = FF(read_slot.direct_address),
.main_pc = pc, // No PC increment here since this is the same opcode as the rows created below
.main_r_in_tag = FF(static_cast<uint32_t>(AvmMemoryTag::FF)),
.main_sel_mem_op_a = FF(1),
.main_sel_mem_op_b = FF(1),
.main_sel_resolve_ind_addr_a = FF(static_cast<uint32_t>(read_dest_value.is_indirect)),
.main_sel_resolve_ind_addr_b = FF(static_cast<uint32_t>(read_slot.is_indirect)),
// .main_tag_err = FF(static_cast<uint32_t>(tag_match)),
.main_w_in_tag = FF(static_cast<uint32_t>(AvmMemoryTag::FF)),
.main_sel_resolve_ind_addr_a = FF(static_cast<uint32_t>(read_slot.is_indirect)),
.main_tag_err = FF(static_cast<uint32_t>(!read_slot.tag_match)),
});
clk++;

AddressWithMode write_dst = resolved_dest;
// Loop over the size and write the hints to memory
for (uint32_t i = 0; i < size; i++) {
FF value = execution_hints.get_side_effect_hints().at(side_effect_counter);

mem_trace_builder.write_into_memory(call_ptr,
clk,
IntermRegister::IA,
read_dest_value.direct_address + i,
value,
AvmMemoryTag::FF,
AvmMemoryTag::FF);
auto write_a = constrained_write_to_memory(
call_ptr, clk, write_dst, value, AvmMemoryTag::U0, AvmMemoryTag::FF, IntermRegister::IA);

auto row = Row{
.main_clk = clk,
.main_ia = value,
.main_ib = read_slot.val + i, // slot increments each time
.main_ind_addr_a = write_a.indirect_address,
.main_internal_return_ptr = internal_return_ptr,
.main_mem_addr_a = read_dest_value.direct_address + i,
.main_mem_addr_a = write_a.direct_address, // direct address incremented at end of the loop
.main_pc = pc, // No PC increment here since this is the same opcode for all loop iterations
.main_r_in_tag = static_cast<uint32_t>(AvmMemoryTag::FF),
.main_rwa = 1,
.main_sel_mem_op_a = 1,
.main_sel_op_sload = FF(1),
.main_sel_q_kernel_output_lookup = 1,
.main_sel_resolve_ind_addr_a = FF(static_cast<uint32_t>(write_a.is_indirect)),
.main_tag_err = FF(static_cast<uint32_t>(!write_a.tag_match)),
.main_w_in_tag = static_cast<uint32_t>(AvmMemoryTag::FF),
};

// Output storage read to kernel outputs (performs lookup)
// Tuples of (slot, value) in the kernel lookup
kernel_trace_builder.op_sload(clk, side_effect_counter, row.main_ib, row.main_ia);

// Constrain gas cost
Expand All @@ -1634,6 +1625,9 @@ void AvmTraceBuilder::op_sload(uint8_t indirect, uint32_t slot_offset, uint32_t
main_trace.push_back(row);
side_effect_counter++;
clk++;

// After the first loop, all future write destinations are direct, increment the direct address
write_dst = AddressWithMode{ AddressingMode::DIRECT, write_a.direct_address + 1 };
}
pc++;
}
Expand All @@ -1644,48 +1638,44 @@ void AvmTraceBuilder::op_sstore(uint8_t indirect, uint32_t src_offset, uint32_t

auto [resolved_src, resolved_slot] = unpack_indirects<2>(indirect, { src_offset, slot_offset });

auto read_src_value = constrained_read_from_memory(
call_ptr, clk, resolved_src, AvmMemoryTag::FF, AvmMemoryTag::FF, IntermRegister::IA);
auto read_slot = constrained_read_from_memory(
call_ptr, clk, resolved_slot, AvmMemoryTag::FF, AvmMemoryTag::FF, IntermRegister::IB);
// TODO: Reenable tag_match
// bool tag_match = read_src_value.tag_match && read_slot.tag_match;
call_ptr, clk, resolved_slot, AvmMemoryTag::FF, AvmMemoryTag::FF, IntermRegister::IA);

main_trace.push_back(Row{
.main_clk = clk,
.main_ia = read_src_value.val,
.main_ib = read_slot.val,
.main_ind_addr_a = FF(read_src_value.indirect_address),
.main_ind_addr_b = FF(read_slot.indirect_address),
.main_ia = read_slot.val,
.main_ind_addr_a = FF(read_slot.indirect_address),
.main_internal_return_ptr = FF(internal_return_ptr),
.main_mem_addr_a = FF(read_src_value.direct_address),
.main_mem_addr_b = FF(read_slot.direct_address),
.main_mem_addr_a = FF(read_slot.direct_address),
.main_pc = pc, // No PC increment here since this is the same opcode as the rows created below
.main_r_in_tag = FF(static_cast<uint32_t>(AvmMemoryTag::FF)),
.main_sel_mem_op_a = FF(1),
.main_sel_mem_op_b = FF(1),
.main_sel_resolve_ind_addr_a = FF(static_cast<uint32_t>(read_src_value.is_indirect)),
.main_sel_resolve_ind_addr_b = FF(static_cast<uint32_t>(read_slot.is_indirect)),
// TODO: Reenable when tag_match is wokring sstore
// .main_tag_err = FF(static_cast<uint32_t>(tag_match)),
.main_sel_resolve_ind_addr_a = FF(static_cast<uint32_t>(read_slot.is_indirect)),
.main_tag_err = FF(static_cast<uint32_t>(!read_slot.tag_match)),
.main_w_in_tag = FF(static_cast<uint32_t>(AvmMemoryTag::FF)),
});
clk++;

AddressWithMode read_src = resolved_src;

// This loop reads a _size_ number of elements from memory and places them into a tuple of (ele, slot)
// in the kernel lookup.
for (uint32_t i = 0; i < size; i++) {
auto read_a = mem_trace_builder.read_and_load_from_memory(
call_ptr, clk, IntermRegister::IA, read_src_value.direct_address + i, AvmMemoryTag::FF, AvmMemoryTag::U0);
auto read_a = constrained_read_from_memory(
call_ptr, clk, read_src, AvmMemoryTag::FF, AvmMemoryTag::U0, IntermRegister::IA);

Row row = Row{
.main_clk = clk,
.main_ia = read_a.val,
.main_ib = read_slot.val + i, // slot increments each time
.main_ind_addr_a = read_a.indirect_address,
.main_internal_return_ptr = internal_return_ptr,
.main_mem_addr_a = read_src_value.direct_address + i,
.main_mem_addr_a = read_a.direct_address, // direct address incremented at end of the loop
.main_pc = pc,
.main_r_in_tag = static_cast<uint32_t>(AvmMemoryTag::FF),
.main_sel_mem_op_a = 1,
.main_sel_q_kernel_output_lookup = 1,
.main_sel_resolve_ind_addr_a = FF(static_cast<uint32_t>(read_a.is_indirect)),
.main_tag_err = FF(static_cast<uint32_t>(!read_a.tag_match)),
};
row.main_sel_op_sstore = FF(1);
Expand All @@ -1697,6 +1687,8 @@ void AvmTraceBuilder::op_sstore(uint8_t indirect, uint32_t src_offset, uint32_t
main_trace.push_back(row);
side_effect_counter++;
clk++;
// All future reads are direct, increment the direct address
read_src = AddressWithMode{ AddressingMode::DIRECT, read_a.direct_address + 1 };
}
pc++;
}
Expand Down
17 changes: 8 additions & 9 deletions barretenberg/cpp/src/barretenberg/vm/tests/avm_execution.test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1723,21 +1723,20 @@ TEST_F(AvmExecutionTests, kernelOutputEmitOpcodes)
TEST_F(AvmExecutionTests, kernelOutputStorageLoadOpcodeSimple)
{
// Sload from a value that has not previously been written to will require a hint to process
std::string bytecode_hex = to_hex(OpCode::SET) + // opcode SET
"00" // Indirect flag
"03" // U32
"00000009" // value 9
"00000001" // dst_offset 1
// Cast set to field
+ to_hex(OpCode::CAST) + // opcode CAST
std::string bytecode_hex = to_hex(OpCode::SET) + // opcode SET
"00" // Indirect flag
"03" // U32
"00000009" // value 9
"00000001" // dst_offset 1
+ to_hex(OpCode::CAST) + // opcode CAST (Cast set to field)
"00" // Indirect flag
"06" // tag field
"00000001" // dst 1
"00000001" // dst 1
+ to_hex(OpCode::SLOAD) + // opcode SLOAD
"00" // Indirect flag
"00000001" // slot offset 1
"00000001" // slot offset 1
"00000001" // slot size 1
"00000002" // write storage value to offset 2
+ to_hex(OpCode::RETURN) + // opcode RETURN
"00" // Indirect flag
Expand Down Expand Up @@ -1794,7 +1793,7 @@ TEST_F(AvmExecutionTests, kernelOutputStorageLoadOpcodeComplex)
+ to_hex(OpCode::SLOAD) + // opcode SLOAD
"00" // Indirect flag (second operand indirect - dest offset)
"00000001" // slot offset 1
"00000002" // slot offset 2
"00000002" // slot size 2
"00000002" // write storage value to offset 2
+ to_hex(OpCode::RETURN) + // opcode RETURN
"00" // Indirect flag
Expand Down
4 changes: 2 additions & 2 deletions barretenberg/cpp/src/barretenberg/vm/tests/avm_kernel.test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1084,7 +1084,7 @@ TEST_F(AvmKernelOutputPositiveTests, kernelSload)
/*ib=*/slot,
/*mem_addr_b=*/0,
/*ind_b=*/false,
/*r_in_tag=*/AvmMemoryTag::FF,
/*r_in_tag=*/AvmMemoryTag::U0, // Kernel Sload is writing to memory
/*side_effect_counter=*/0,
/*rwa=*/1,
/*no_b=*/true);
Expand Down Expand Up @@ -1126,7 +1126,7 @@ TEST_F(AvmKernelOutputPositiveTests, kernelSstore)
/*ib=*/slot,
/*mem_addr_b=*/0,
/*ind_b*/ false,
/*w_in_tag=*/AvmMemoryTag::FF,
/*r_in_tag=*/AvmMemoryTag::FF,
/*side_effect_counter=*/0,
/*rwa=*/0,
/*no_b=*/true);
Expand Down

0 comments on commit fd92d46

Please sign in to comment.