From 45f7d032b3659ca58cb368c4afe10d74167988bd Mon Sep 17 00:00:00 2001 From: David Banks <47112877+dbanks12@users.noreply.github.com> Date: Mon, 28 Oct 2024 20:40:28 -0400 Subject: [PATCH 1/4] Revert "fix(avm): disable sha256 in bulk test until we debug it (#9482)" This reverts commit 078c318f9671566500c472553d88990076a8c32a. --- .../contracts/avm_test_contract/src/main.nr | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr b/noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr index bc3d85ae1689..5d39516e7936 100644 --- a/noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr @@ -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"); @@ -617,9 +617,9 @@ contract AvmTest { let _ = l1_to_l2_msg_exists(1, 2); dep::aztec::oracle::debug_log::debug_log("send_l2_to_l1_msg"); let _ = send_l2_to_l1_msg(EthAddress::from_field(0x2020), 1); - dep::aztec::oracle::debug_log::debug_log("nested_call_to_add"); - let _ = nested_call_to_add(1, 2); - dep::aztec::oracle::debug_log::debug_log("nested_static_call_to_add"); - let _ = nested_static_call_to_add(1, 2); + //dep::aztec::oracle::debug_log::debug_log("nested_call_to_add"); + //let _ = nested_call_to_add(1, 2); + //dep::aztec::oracle::debug_log::debug_log("nested_static_call_to_add"); + //let _ = nested_static_call_to_add(1, 2); } } From 8b5fec8e4104b3fdcb7825bb1f1eaa6a7bebbc97 Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Tue, 29 Oct 2024 01:00:05 +0000 Subject: [PATCH 2/4] fix shl/shr --- .../vm/avm/tests/bitwise.test.cpp | 10 +-- .../barretenberg/vm/avm/trace/alu_trace.cpp | 2 + .../src/barretenberg/vm/avm/trace/trace.cpp | 63 +++++++++++-------- 3 files changed, 45 insertions(+), 30 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/tests/bitwise.test.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/tests/bitwise.test.cpp index 0e4b9316ca9b..18b0e316a139 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/tests/bitwise.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/tests/bitwise.test.cpp @@ -93,7 +93,7 @@ void common_validate_shift_op(std::vector 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) @@ -118,10 +118,10 @@ void common_validate_shift_op(std::vector 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)); + // 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(tag))); diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/alu_trace.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/alu_trace.cpp index 0d9f7b4caf02..25d1b54ebc6d 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/alu_trace.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/alu_trace.cpp @@ -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); @@ -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); diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp index d5400f5c8f5b..7b4ee622f76e 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp @@ -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); @@ -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(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(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(read_a.is_indirect)), - .main_sel_resolve_ind_addr_b = FF(static_cast(read_b.is_indirect)), + //.main_sel_resolve_ind_addr_b = FF(static_cast(read_b.is_indirect)), .main_sel_resolve_ind_addr_c = FF(static_cast(write_c.is_indirect)), .main_tag_err = FF(static_cast(!tag_match)), .main_w_in_tag = FF(static_cast(in_tag)), @@ -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); @@ -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(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(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(read_a.is_indirect)), - .main_sel_resolve_ind_addr_b = FF(static_cast(read_b.is_indirect)), + // TODO(8603): uncomment + //.main_sel_resolve_ind_addr_b = FF(static_cast(read_b.is_indirect)), .main_sel_resolve_ind_addr_c = FF(static_cast(write_c.is_indirect)), .main_tag_err = FF(static_cast(!tag_match)), .main_w_in_tag = FF(static_cast(in_tag)), @@ -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, @@ -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, @@ -2462,7 +2474,7 @@ void AvmTraceBuilder::op_get_contract_instance( .main_tag_err = FF(static_cast(!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(instance.exists)), AvmMemoryTag::U1); @@ -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(read_radix.val); uint32_t radix = static_cast(read_radix); @@ -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(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(read_src.is_indirect)), - // TODO:(8603): uncomment + // TODO(8603): uncomment //.main_sel_resolve_ind_addr_b = FF(static_cast(read_radix.is_indirect)), .main_w_in_tag = FF(static_cast(w_in_tag)), }); From 252481f77e2c1e534a28213bcb6cd72c50bf7361 Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Tue, 29 Oct 2024 01:21:47 +0000 Subject: [PATCH 3/4] todo --- barretenberg/cpp/src/barretenberg/vm/avm/tests/bitwise.test.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/tests/bitwise.test.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/tests/bitwise.test.cpp index 18b0e316a139..9a3b1b8b369d 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/tests/bitwise.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/tests/bitwise.test.cpp @@ -118,6 +118,7 @@ void common_validate_shift_op(std::vector const& trace, EXPECT_EQ(row->main_rwa, FF(0)); // Check that ib register is correctly set with memory load operations. + // 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)); From 266169aa59ea525475dd4be98b171bb8745d014d Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Tue, 29 Oct 2024 01:27:21 +0000 Subject: [PATCH 4/4] uncomment nr test --- .../contracts/avm_test_contract/src/main.nr | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr b/noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr index 5d39516e7936..d564c046037f 100644 --- a/noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr @@ -617,9 +617,9 @@ contract AvmTest { let _ = l1_to_l2_msg_exists(1, 2); dep::aztec::oracle::debug_log::debug_log("send_l2_to_l1_msg"); let _ = send_l2_to_l1_msg(EthAddress::from_field(0x2020), 1); - //dep::aztec::oracle::debug_log::debug_log("nested_call_to_add"); - //let _ = nested_call_to_add(1, 2); - //dep::aztec::oracle::debug_log::debug_log("nested_static_call_to_add"); - //let _ = nested_static_call_to_add(1, 2); + dep::aztec::oracle::debug_log::debug_log("nested_call_to_add"); + let _ = nested_call_to_add(1, 2); + dep::aztec::oracle::debug_log::debug_log("nested_static_call_to_add"); + let _ = nested_static_call_to_add(1, 2); } }