Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(avm): re-enable sha256 in bulk test, fix bug in AVM SHL/SHR #9496

Merged
merged 6 commits into from
Oct 29, 2024
Merged

fix(avm): re-enable sha256 in bulk test, fix bug in AVM SHL/SHR #9496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
45f7d03
Revert "fix(avm): disable sha256 in bulk test until we debug it (#9482)"
dbanks12 Oct 29, 2024
8b5fec8
fix shl/shr
dbanks12 Oct 29, 2024
252481f
todo
dbanks12 Oct 29, 2024
266169a
uncomment nr test
dbanks12 Oct 29, 2024
815a1b6
Merge branch 'master' into revert-9482-fc/avm-fix-yarn
jeanmon Oct 29, 2024
6af41f8
Merge branch 'master' into revert-9482-fc/avm-fix-yarn
dbanks12 Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions barretenberg/cpp/src/barretenberg/vm/avm/tests/bitwise.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ void common_validate_shift_op(std::vector<Row> const& trace,
FF const& b,
FF const& c,
FF const& addr_a,
FF const& addr_b,
[[maybe_unused]] FF const& addr_b,
FF const& addr_c,
avm_trace::AvmMemoryTag const tag,
bool shr)
Expand All @@ -118,10 +118,11 @@ void common_validate_shift_op(std::vector<Row> const& trace,
EXPECT_EQ(row->main_rwa, FF(0));

// Check that ib register is correctly set with memory load operations.
EXPECT_EQ(row->main_ib, b);
EXPECT_EQ(row->main_mem_addr_b, addr_b);
EXPECT_EQ(row->main_sel_mem_op_b, FF(1));
EXPECT_EQ(row->main_rwb, FF(0));
// TODO(8603): once instructions can have multiple different tags for reads, constrain b's read & tag
// EXPECT_EQ(row->main_ib, b);
// EXPECT_EQ(row->main_mem_addr_b, addr_b);
// EXPECT_EQ(row->main_sel_mem_op_b, FF(1));
// EXPECT_EQ(row->main_rwb, FF(0));

// Check the instruction tags
EXPECT_EQ(row->main_r_in_tag, FF(static_cast<uint32_t>(tag)));
Expand Down
2 changes: 2 additions & 0 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/alu_trace.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,7 @@ FF AvmAluTraceBuilder::op_not(FF const& a, AvmMemoryTag in_tag, uint32_t const c
*/
FF AvmAluTraceBuilder::op_shl(FF const& a, FF const& b, AvmMemoryTag in_tag, uint32_t clk)
{
// TODO(9497): this should raise error flag in main trace, not assert
ASSERT(in_tag != AvmMemoryTag::FF);
// Check that the shifted amount is an 8-bit integer
ASSERT(uint256_t(b) < 256);
Expand Down Expand Up @@ -512,6 +513,7 @@ FF AvmAluTraceBuilder::op_shl(FF const& a, FF const& b, AvmMemoryTag in_tag, uin
*/
FF AvmAluTraceBuilder::op_shr(FF const& a, FF const& b, AvmMemoryTag in_tag, uint32_t clk)
{
// TODO(9497): this should raise error flag in main trace, not assert
ASSERT(in_tag != AvmMemoryTag::FF);
// Check that the shifted amount is an 8-bit integer
ASSERT(uint256_t(b) < 256);
Expand Down
63 changes: 38 additions & 25 deletions barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1064,12 +1064,16 @@ void AvmTraceBuilder::op_shl(uint8_t indirect, uint32_t a_offset, uint32_t b_off
// We get our representative memory tag from the resolved_a memory address.
AvmMemoryTag in_tag = unconstrained_get_memory_tag(resolved_a);
// Reading from memory and loading into ia resp. ib.
// TODO(9497): if simulator fails tag check here, witgen will not. Raise error flag if in_tag is FF!
auto read_a = constrained_read_from_memory(call_ptr, clk, resolved_a, in_tag, in_tag, IntermRegister::IA);
auto read_b = constrained_read_from_memory(call_ptr, clk, resolved_b, in_tag, in_tag, IntermRegister::IB);
bool tag_match = read_a.tag_match && read_b.tag_match;
// TODO(8603): once instructions can have multiple different tags for reads, constrain b's read & tag
// auto read_b = constrained_read_from_memory(call_ptr, clk, resolved_b, AvmMemoryTag::U8, AvmMemoryTag::U8,
// IntermRegister::IB); bool tag_match = read_a.tag_match && read_b.tag_match;
auto read_b = unconstrained_read_from_memory(resolved_b); // should be tagged as U8
bool tag_match = read_a.tag_match;

FF a = tag_match ? read_a.val : FF(0);
FF b = tag_match ? read_b.val : FF(0);
FF b = tag_match ? read_b : FF(0);

FF c = tag_match ? alu_trace_builder.op_shl(a, b, in_tag, clk) : FF(0);

Expand All @@ -1083,24 +1087,24 @@ void AvmTraceBuilder::op_shl(uint8_t indirect, uint32_t a_offset, uint32_t b_off
.main_alu_in_tag = FF(static_cast<uint32_t>(in_tag)),
.main_call_ptr = call_ptr,
.main_ia = read_a.val,
.main_ib = read_b.val,
.main_ib = read_b,
.main_ic = write_c.val,
.main_ind_addr_a = FF(read_a.indirect_address),
.main_ind_addr_b = FF(read_b.indirect_address),
//.main_ind_addr_b = FF(read_b.indirect_address),
.main_ind_addr_c = FF(write_c.indirect_address),
.main_internal_return_ptr = FF(internal_return_ptr),
.main_mem_addr_a = FF(read_a.direct_address),
.main_mem_addr_b = FF(read_b.direct_address),
//.main_mem_addr_b = FF(read_b.direct_address),
.main_mem_addr_c = FF(write_c.direct_address),
.main_pc = FF(pc++),
.main_r_in_tag = FF(static_cast<uint32_t>(in_tag)),
.main_rwc = FF(1),
.main_sel_mem_op_a = FF(1),
.main_sel_mem_op_b = FF(1),
//.main_sel_mem_op_b = FF(1),
.main_sel_mem_op_c = FF(1),
.main_sel_op_shl = FF(1),
.main_sel_resolve_ind_addr_a = FF(static_cast<uint32_t>(read_a.is_indirect)),
.main_sel_resolve_ind_addr_b = FF(static_cast<uint32_t>(read_b.is_indirect)),
//.main_sel_resolve_ind_addr_b = FF(static_cast<uint32_t>(read_b.is_indirect)),
.main_sel_resolve_ind_addr_c = FF(static_cast<uint32_t>(write_c.is_indirect)),
.main_tag_err = FF(static_cast<uint32_t>(!tag_match)),
.main_w_in_tag = FF(static_cast<uint32_t>(in_tag)),
Expand All @@ -1117,12 +1121,16 @@ void AvmTraceBuilder::op_shr(uint8_t indirect, uint32_t a_offset, uint32_t b_off
// We get our representative memory tag from the resolved_a memory address.
AvmMemoryTag in_tag = unconstrained_get_memory_tag(resolved_a);
// Reading from memory and loading into ia resp. ib.
// TODO(9497): if simulator fails tag check here, witgen will not. Raise error flag if in_tag is FF!
auto read_a = constrained_read_from_memory(call_ptr, clk, resolved_a, in_tag, in_tag, IntermRegister::IA);
auto read_b = constrained_read_from_memory(call_ptr, clk, resolved_b, in_tag, in_tag, IntermRegister::IB);
bool tag_match = read_a.tag_match && read_b.tag_match;
// TODO(8603): once instructions can have multiple different tags for reads, constrain b's read & tag
// auto read_b = constrained_read_from_memory(call_ptr, clk, resolved_b, AvmMemoryTag::U8, AvmMemoryTag::U8,
// IntermRegister::IB); bool tag_match = read_a.tag_match && read_b.tag_match;
auto read_b = unconstrained_read_from_memory(resolved_b); // should be tagged as U8
bool tag_match = read_a.tag_match;

FF a = tag_match ? read_a.val : FF(0);
FF b = tag_match ? read_b.val : FF(0);
FF b = tag_match ? read_b : FF(0);

FF c = tag_match ? alu_trace_builder.op_shr(a, b, in_tag, clk) : FF(0);

Expand All @@ -1136,24 +1144,28 @@ void AvmTraceBuilder::op_shr(uint8_t indirect, uint32_t a_offset, uint32_t b_off
.main_alu_in_tag = FF(static_cast<uint32_t>(in_tag)),
.main_call_ptr = call_ptr,
.main_ia = read_a.val,
.main_ib = read_b.val,
.main_ib = read_b,
.main_ic = write_c.val,
.main_ind_addr_a = FF(read_a.indirect_address),
.main_ind_addr_b = FF(read_b.indirect_address),
// TODO(8603): uncomment
//.main_ind_addr_b = FF(read_b.indirect_address),
.main_ind_addr_c = FF(write_c.indirect_address),
.main_internal_return_ptr = FF(internal_return_ptr),
.main_mem_addr_a = FF(read_a.direct_address),
.main_mem_addr_b = FF(read_b.direct_address),
// TODO(8603): uncomment
//.main_mem_addr_b = FF(read_b.direct_address),
.main_mem_addr_c = FF(write_c.direct_address),
.main_pc = FF(pc++),
.main_r_in_tag = FF(static_cast<uint32_t>(in_tag)),
.main_rwc = FF(1),
.main_sel_mem_op_a = FF(1),
.main_sel_mem_op_b = FF(1),
// TODO(8603): uncomment
//.main_sel_mem_op_b = FF(1),
.main_sel_mem_op_c = FF(1),
.main_sel_op_shr = FF(1),
.main_sel_resolve_ind_addr_a = FF(static_cast<uint32_t>(read_a.is_indirect)),
.main_sel_resolve_ind_addr_b = FF(static_cast<uint32_t>(read_b.is_indirect)),
// TODO(8603): uncomment
//.main_sel_resolve_ind_addr_b = FF(static_cast<uint32_t>(read_b.is_indirect)),
.main_sel_resolve_ind_addr_c = FF(static_cast<uint32_t>(write_c.is_indirect)),
.main_tag_err = FF(static_cast<uint32_t>(!tag_match)),
.main_w_in_tag = FF(static_cast<uint32_t>(in_tag)),
Expand Down Expand Up @@ -2429,7 +2441,7 @@ void AvmTraceBuilder::op_get_contract_instance(
break;
}

// TODO:(8603): once instructions can have multiple different tags for writes, write dst as FF and exists as U1
// TODO(8603): once instructions can have multiple different tags for writes, write dst as FF and exists as U1
// auto write_dst = constrained_write_to_memory(call_ptr, clk, resolved_dst_offset, member_value,
// AvmMemoryTag::FF, AvmMemoryTag::FF, IntermRegister::IC); auto write_exists =
// constrained_write_to_memory(call_ptr, clk, resolved_exists_offset, instance.instance_found_in_address,
Expand All @@ -2439,7 +2451,7 @@ void AvmTraceBuilder::op_get_contract_instance(
.main_clk = clk,
.main_call_ptr = call_ptr,
.main_ia = read_address.val,
// TODO:(8603): uncomment this and below blocks once instructions can have multiple different tags for
// TODO(8603): uncomment this and below blocks once instructions can have multiple different tags for
// writes
//.main_ic = write_dst.val,
//.main_id = write_exists.val,
Expand All @@ -2462,7 +2474,7 @@ void AvmTraceBuilder::op_get_contract_instance(
.main_tag_err = FF(static_cast<uint32_t>(!tag_match)),
});

// TODO:(8603): once instructions can have multiple different tags for writes, remove this and do a constrained
// TODO(8603): once instructions can have multiple different tags for writes, remove this and do a constrained
// writes
write_to_memory(resolved_dst_offset, member_value, AvmMemoryTag::FF);
write_to_memory(resolved_exists_offset, FF(static_cast<uint32_t>(instance.exists)), AvmMemoryTag::U1);
Expand Down Expand Up @@ -3305,13 +3317,14 @@ void AvmTraceBuilder::op_to_radix_le(uint8_t indirect,

auto read_src = constrained_read_from_memory(
call_ptr, clk, resolved_src_offset, AvmMemoryTag::FF, w_in_tag, IntermRegister::IA);
// TODO:(8603): once instructions can have multiple different tags for reads, constrain the radix's read
// TODO(8603): once instructions can have multiple different tags for reads, constrain the radix's read
// TODO(9497): if simulator fails tag check here, witgen will not. Raise error flag!
// auto read_radix = constrained_read_from_memory(
// call_ptr, clk, resolved_radix_offset, AvmMemoryTag::U32, AvmMemoryTag::U32, IntermRegister::IB);
auto read_radix = unconstrained_read_from_memory(resolved_radix_offset);

FF input = read_src.val;
// TODO:(8603): uncomment
// TODO(8603): uncomment
// uint32_t radix = static_cast<uint32_t>(read_radix.val);
uint32_t radix = static_cast<uint32_t>(read_radix);

Expand All @@ -3337,21 +3350,21 @@ void AvmTraceBuilder::op_to_radix_le(uint8_t indirect,
.main_ic = num_limbs,
.main_id = output_bits,
.main_ind_addr_a = read_src.indirect_address,
// TODO:(8603): uncomment
// TODO(8603): uncomment
//.main_ind_addr_b = read_radix.indirect_address,
.main_internal_return_ptr = FF(internal_return_ptr),
.main_mem_addr_a = read_src.direct_address,
// TODO:(8603): uncomment
// TODO(8603): uncomment
//.main_mem_addr_b = read_radix.direct_address,
.main_op_err = error ? FF(1) : FF(0),
.main_pc = FF(pc++),
.main_r_in_tag = FF(static_cast<uint32_t>(AvmMemoryTag::FF)),
.main_sel_mem_op_a = FF(1),
// TODO:(8603): uncomment
// TODO(8603): uncomment
//.main_sel_mem_op_b = FF(1),
.main_sel_op_radix_le = FF(1),
.main_sel_resolve_ind_addr_a = FF(static_cast<uint32_t>(read_src.is_indirect)),
// TODO:(8603): uncomment
// TODO(8603): uncomment
//.main_sel_resolve_ind_addr_b = FF(static_cast<uint32_t>(read_radix.is_indirect)),
.main_w_in_tag = FF(static_cast<uint32_t>(w_in_tag)),
});
Expand Down
4 changes: 2 additions & 2 deletions noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,8 +569,8 @@ contract AvmTest {
let _ = read_storage_map(context.this_address());
dep::aztec::oracle::debug_log::debug_log("keccak_hash");
let _ = keccak_hash(args_u8);
// dep::aztec::oracle::debug_log::debug_log("sha256_hash");
// let _ = sha256_hash(args_u8);
dep::aztec::oracle::debug_log::debug_log("sha256_hash");
let _ = sha256_hash(args_u8);
dep::aztec::oracle::debug_log::debug_log("poseidon2_hash");
let _ = poseidon2_hash(args_field);
dep::aztec::oracle::debug_log::debug_log("pedersen_hash");
Expand Down
Loading