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

Reverts #9482
AztecProtocol · Oct 29, 2024 · 0fe64df · 0fe64df
1 parent 3351217
commit 0fe64df
Show file tree

Hide file tree

Showing 4 changed files with 48 additions and 32 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
@@ -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)
@@ -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)));

diff --git 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
@@ -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<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)),
@@ -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<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)),
@@ -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<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);
@@ -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);
 
@@ -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)),
     });

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
@@ -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");