12193: Addressing review feedback

jeanmon · jeanmon · commit 76e4ffd5b2b6 · 2025-03-06T10:57:22.000Z
diff --git a/barretenberg/cpp/src/barretenberg/vm2/common/instruction_spec.test.cpp b/barretenberg/cpp/src/barretenberg/vm2/common/instruction_spec.test.cpp
@@ -31,7 +31,7 @@ TEST(InstructionSpecTest, CheckAllInstructionSizes)
         const auto wire_opcode = static_cast<WireOpCode>(i);
         const auto computed_size = compute_instruction_size(wire_opcode, wire_format, operand_type_sizes);
         EXPECT_EQ(WIRE_INSTRUCTION_SPEC.at(wire_opcode).size_in_bytes, computed_size)
-            << format("Incorrect size_in_bytes field for ", wire_opcode, " in WIRE_INSTRUCTION_SPEC.");
+            << "Incorrect size_in_bytes field for " << wire_opcode << " in WIRE_INSTRUCTION_SPEC.";
     }
 }
 
diff --git a/barretenberg/cpp/src/barretenberg/vm2/common/memory_types.hpp b/barretenberg/cpp/src/barretenberg/vm2/common/memory_types.hpp
@@ -6,7 +6,7 @@
 
 namespace bb::avm2 {
 
-// Keep in sync with NUM_MEMORY_TAGS if we modify this enum.
+// Adapt NUM_MEMORY_TAGS in fixtures.cpp if this enum is modified.
 enum class MemoryTag {
     FF,
     U1,
@@ -17,8 +17,6 @@ enum class MemoryTag {
     U128,
 };
 
-constexpr uint8_t NUM_MEMORY_TAGS = static_cast<int>(MemoryTag::U128) + 1;
-
 using MemoryAddress = uint32_t;
 using MemoryValue = FF;
 constexpr auto MemoryAddressTag = MemoryTag::U32;
diff --git a/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/instr_fetching.test.cpp b/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/instr_fetching.test.cpp
@@ -28,6 +28,7 @@ using FF = AvmFlavorSettings::FF;
 using C = Column;
 using instr_fetching = bb::avm2::instr_fetching<FF>;
 using simulation::Instruction;
+using simulation::InstructionFetchingEvent;
 using simulation::Operand;
 using testing::random_bytes;
 
@@ -47,7 +48,7 @@ TEST(InstrFetchingConstrainingTest, Add8WithTraceGen)
         .operands = { Operand::u8(0x34), Operand::u8(0x35), Operand::u8(0x36) },
     };
 
-    std::vector<uint8_t> bytecode = add_8_instruction.encode();
+    std::vector<uint8_t> bytecode = add_8_instruction.serialize();
 
     builder.process_instruction_fetching({ { .bytecode_id = 1,
                                              .pc = 0,
@@ -77,7 +78,7 @@ TEST(InstrFetchingConstrainingTest, EcaddWithTraceGen)
                       Operand::u16(0x127f) },
     };
 
-    std::vector<uint8_t> bytecode = ecadd_instruction.encode();
+    std::vector<uint8_t> bytecode = ecadd_instruction.serialize();
     builder.process_instruction_fetching({ { .bytecode_id = 1,
                                              .pc = 0,
                                              .instruction = ecadd_instruction,
@@ -89,33 +90,33 @@ TEST(InstrFetchingConstrainingTest, EcaddWithTraceGen)
 }
 
 // Helper routine generating a vector of instruction fetching events for each
-// opcode. Note that operands of type TAG might fall outside of their valid range.
-std::vector<simulation::InstructionFetchingEvent> gen_instr_events_each_opcode()
+// opcode.
+std::vector<InstructionFetchingEvent> gen_instr_events_each_opcode()
 {
     std::vector<uint8_t> bytecode;
-    std::vector<uint32_t> pc_positions;
+    std::vector<Instruction> instructions;
     constexpr auto num_opcodes = static_cast<size_t>(WireOpCode::LAST_OPCODE_SENTINEL);
-    pc_positions.reserve(num_opcodes);
+    instructions.reserve(num_opcodes);
+    std::array<uint32_t, num_opcodes> pc_positions;
 
     for (size_t i = 0; i < num_opcodes; i++) {
-        pc_positions.emplace_back(static_cast<uint32_t>(bytecode.size()));
-        bytecode.emplace_back(i);
-        const auto instruction_bytes =
-            random_bytes(WIRE_INSTRUCTION_SPEC.at(static_cast<WireOpCode>(i)).size_in_bytes - 1);
+        pc_positions.at(i) = static_cast<uint32_t>(bytecode.size());
+        const auto instr = testing::random_instruction(static_cast<WireOpCode>(i));
+        instructions.emplace_back(instr);
+        const auto instruction_bytes = instr.serialize();
         bytecode.insert(bytecode.end(),
                         std::make_move_iterator(instruction_bytes.begin()),
                         std::make_move_iterator(instruction_bytes.end()));
     }
 
-    const auto bytecode_ptr = std::make_shared<std::vector<uint8_t>>(bytecode);
+    const auto bytecode_ptr = std::make_shared<std::vector<uint8_t>>(std::move(bytecode));
+    // Always use *bytecode_ptr from now on instead of bytecode as this one was moved.
 
-    std::vector<simulation::InstructionFetchingEvent> instr_events;
+    std::vector<InstructionFetchingEvent> instr_events;
     instr_events.reserve(num_opcodes);
-
     for (size_t i = 0; i < num_opcodes; i++) {
-        const auto instr = simulation::decode_instruction(bytecode, pc_positions.at(i));
-        instr_events.emplace_back(simulation::InstructionFetchingEvent{
-            .bytecode_id = 1, .pc = pc_positions.at(i), .instruction = instr, .bytecode = bytecode_ptr });
+        instr_events.emplace_back(InstructionFetchingEvent{
+            .bytecode_id = 1, .pc = pc_positions.at(i), .instruction = instructions.at(i), .bytecode = bytecode_ptr });
     }
     return instr_events;
 }
@@ -149,20 +150,20 @@ TEST(InstrFetchingConstrainingTest, NegativeWrongOperand)
         instr_fetching::SR_OP6_BYTES_DECOMPOSITION,      instr_fetching::SR_OP7_BYTES_DECOMPOSITION,
     };
 
-    const std::vector<C> operand_cols = {
+    constexpr std::array<C, 8> operand_cols = {
         C::instr_fetching_indirect, C::instr_fetching_op1, C::instr_fetching_op2, C::instr_fetching_op3,
         C::instr_fetching_op4,      C::instr_fetching_op5, C::instr_fetching_op6, C::instr_fetching_op7,
     };
 
     for (const auto& opcode : opcodes) {
         TestTraceContainer trace;
         const auto instr = testing::random_instruction(opcode);
-        builder.process_instruction_fetching({ simulation::InstructionFetchingEvent{
-                                                 .bytecode_id = 1,
-                                                 .pc = 0,
-                                                 .instruction = instr,
-                                                 .bytecode = std::make_shared<std::vector<uint8_t>>(instr.encode()) } },
-                                             trace);
+        builder.process_instruction_fetching(
+            { { .bytecode_id = 1,
+                .pc = 0,
+                .instruction = instr,
+                .bytecode = std::make_shared<std::vector<uint8_t>>(instr.serialize()) } },
+            trace);
         check_relation<instr_fetching>(trace);
 
         EXPECT_EQ(trace.get_num_rows(), 1);
@@ -210,7 +211,7 @@ TEST(InstrFetchingConstrainingTest, BcDecompositionInteractions)
 
     const auto instr_fetch_events = gen_instr_events_each_opcode();
     bytecode_builder.process_instruction_fetching(instr_fetch_events, trace);
-    bytecode_builder.process_decomposition({ simulation::BytecodeDecompositionEvent{
+    bytecode_builder.process_decomposition({ {
                                                .bytecode_id = instr_fetch_events.at(0).bytecode_id,
                                                .bytecode = instr_fetch_events.at(0).bytecode,
                                            } },
@@ -226,6 +227,7 @@ TEST(InstrFetchingConstrainingTest, BcDecompositionInteractions)
 TEST(InstrFetchingConstrainingTest, NegativeWrongWireInstructionSpecInteractions)
 {
     using wire_instr_spec_lookup = lookup_instr_fetching_wire_instruction_info_relation<FF>;
+    using tracegen::LookupIntoIndexedByClk;
 
     BytecodeTraceBuilder bytecode_builder;
     PrecomputedTraceBuilder precomputed_builder;
@@ -238,19 +240,20 @@ TEST(InstrFetchingConstrainingTest, NegativeWrongWireInstructionSpecInteractions
         TestTraceContainer trace;
         const auto instr = testing::random_instruction(opcode);
         bytecode_builder.process_instruction_fetching(
-            { simulation::InstructionFetchingEvent{ .bytecode_id = 1,
-                                                    .pc = 0,
-                                                    .instruction = instr,
-                                                    .bytecode =
-                                                        std::make_shared<std::vector<uint8_t>>(instr.encode()) } },
+            { { .bytecode_id = 1,
+                .pc = 0,
+                .instruction = instr,
+                .bytecode = std::make_shared<std::vector<uint8_t>>(instr.serialize()) } },
             trace);
         precomputed_builder.process_wire_instruction_spec(trace);
         precomputed_builder.process_misc(trace, trace.get_num_rows()); // Limit to the number of rows we need.
 
-        tracegen::LookupIntoIndexedByClk<wire_instr_spec_lookup::Settings>().process(trace);
+        LookupIntoIndexedByClk<wire_instr_spec_lookup::Settings>().process(trace);
+
+        ASSERT_EQ(trace.get(C::lookup_instr_fetching_wire_instruction_info_counts, static_cast<uint32_t>(opcode)), 1);
         check_interaction<wire_instr_spec_lookup>(trace);
 
-        const std::vector<C> mutated_cols = {
+        constexpr std::array<C, 20> mutated_cols = {
             C::instr_fetching_exec_opcode,  C::instr_fetching_instr_size_in_bytes, C::instr_fetching_sel_op_dc_0,
             C::instr_fetching_sel_op_dc_1,  C::instr_fetching_sel_op_dc_2,         C::instr_fetching_sel_op_dc_3,
             C::instr_fetching_sel_op_dc_4,  C::instr_fetching_sel_op_dc_5,         C::instr_fetching_sel_op_dc_6,
@@ -265,6 +268,11 @@ TEST(InstrFetchingConstrainingTest, NegativeWrongWireInstructionSpecInteractions
             auto mutated_trace = trace;
             const FF mutated_value = trace.get(col, 0) + 1; // Mutate to value + 1
             mutated_trace.set(col, 0, mutated_value);
+
+            // We do not need to re-run LookupIntoIndexedByClk<wire_instr_spec_lookup::Settings>().process(trace);
+            // because we never mutate the indexing column for this lookup (clk) and for this lookup
+            // find_in_dst only uses column C::instr_fetching_bd0 mapped to (clk). So, the counts are still valid.
+
             EXPECT_THROW_WITH_MESSAGE(check_interaction<wire_instr_spec_lookup>(mutated_trace),
                                       "Relation.*WIRE_INSTRUCTION_INFO.* ACCUMULATION.* is non-zero");
         }
@@ -275,6 +283,7 @@ TEST(InstrFetchingConstrainingTest, NegativeWrongWireInstructionSpecInteractions
 TEST(InstrFetchingConstrainingTest, NegativeWrongBcDecompositionInteractions)
 {
     using bc_decomposition_lookup = lookup_instr_fetching_bytes_from_bc_dec_relation<FF>;
+    using tracegen::LookupIntoDynamicTableSequential;
 
     TestTraceContainer trace;
     BytecodeTraceBuilder bytecode_builder;
@@ -286,24 +295,25 @@ TEST(InstrFetchingConstrainingTest, NegativeWrongBcDecompositionInteractions)
     for (const auto& opcode : opcodes) {
         TestTraceContainer trace;
         const auto instr = testing::random_instruction(opcode);
-        auto bytecode_ptr = std::make_shared<std::vector<uint8_t>>(instr.encode());
-        bytecode_builder.process_instruction_fetching({ simulation::InstructionFetchingEvent{
+        auto bytecode_ptr = std::make_shared<std::vector<uint8_t>>(instr.serialize());
+        bytecode_builder.process_instruction_fetching({ {
                                                           .bytecode_id = 1,
                                                           .pc = 0,
                                                           .instruction = instr,
                                                           .bytecode = bytecode_ptr,
                                                       } },
                                                       trace);
-        bytecode_builder.process_decomposition({ simulation::BytecodeDecompositionEvent{
+        bytecode_builder.process_decomposition({ {
                                                    .bytecode_id = 1,
                                                    .bytecode = bytecode_ptr,
                                                } },
                                                trace);
 
-        tracegen::LookupIntoDynamicTableSequential<bc_decomposition_lookup::Settings>().process(trace);
-        check_interaction<bc_decomposition_lookup>(trace);
+        auto valid_trace = trace; // Keep original trace before lookup processing
+        LookupIntoDynamicTableSequential<bc_decomposition_lookup::Settings>().process(valid_trace);
+        check_interaction<bc_decomposition_lookup>(valid_trace);
 
-        const std::vector<C> mutated_cols = {
+        constexpr std::array<C, 39> mutated_cols = {
             C::instr_fetching_pc,   C::instr_fetching_bytecode_id, C::instr_fetching_bd0,  C::instr_fetching_bd1,
             C::instr_fetching_bd2,  C::instr_fetching_bd3,         C::instr_fetching_bd4,  C::instr_fetching_bd5,
             C::instr_fetching_bd6,  C::instr_fetching_bd7,         C::instr_fetching_bd8,  C::instr_fetching_bd9,
@@ -321,6 +331,13 @@ TEST(InstrFetchingConstrainingTest, NegativeWrongBcDecompositionInteractions)
             auto mutated_trace = trace;
             const FF mutated_value = trace.get(col, 0) + 1; // Mutate to value + 1
             mutated_trace.set(col, 0, mutated_value);
+
+            // This sets the length of the inverse polynomial via SetDummyInverses, so we still need to call this even
+            // though we know it will fail.
+            EXPECT_THROW_WITH_MESSAGE(
+                LookupIntoDynamicTableSequential<bc_decomposition_lookup::Settings>().process(mutated_trace),
+                "Failed.*BYTES_FROM_BC_DEC. Could not find tuple in destination.");
+
             EXPECT_THROW_WITH_MESSAGE(check_interaction<bc_decomposition_lookup>(mutated_trace),
                                       "Relation.*BYTES_FROM_BC_DEC.* ACCUMULATION.* is non-zero");
         }
diff --git a/barretenberg/cpp/src/barretenberg/vm2/simulation/bytecode_manager.cpp b/barretenberg/cpp/src/barretenberg/vm2/simulation/bytecode_manager.cpp
@@ -57,7 +57,7 @@ Instruction TxBytecodeManager::read_instruction(BytecodeId bytecode_id, uint32_t
     auto bytecode_ptr = it->second;
     const auto& bytecode = *bytecode_ptr;
     // TODO: catch errors etc.
-    Instruction instruction = decode_instruction(bytecode, pc);
+    Instruction instruction = deserialize_instruction(bytecode, pc);
 
     // The event will be deduplicated internally.
     fetching_events.emit(
diff --git a/barretenberg/cpp/src/barretenberg/vm2/simulation/lib/serialization.cpp b/barretenberg/cpp/src/barretenberg/vm2/simulation/lib/serialization.cpp
@@ -227,6 +227,10 @@ Operand& Operand::operator=(const Operand& other)
 
 bool Operand::operator==(const Operand& other) const
 {
+    if (this == &other) {
+        return true;
+    }
+
     if (value.index() != other.value.index()) {
         return false;
     }
@@ -348,7 +352,7 @@ std::string Operand::to_string() const
     __builtin_unreachable();
 }
 
-Instruction decode_instruction(std::span<const uint8_t> bytecode, size_t pos)
+Instruction deserialize_instruction(std::span<const uint8_t> bytecode, size_t pos)
 {
     const auto bytecode_length = bytecode.size();
 
@@ -486,7 +490,7 @@ std::string Instruction::to_string() const
     return oss.str();
 }
 
-std::vector<uint8_t> Instruction::encode() const
+std::vector<uint8_t> Instruction::serialize() const
 {
     std::vector<uint8_t> output;
     output.reserve(WIRE_INSTRUCTION_SPEC.at(opcode).size_in_bytes);
diff --git a/barretenberg/cpp/src/barretenberg/vm2/simulation/lib/serialization.hpp b/barretenberg/cpp/src/barretenberg/vm2/simulation/lib/serialization.hpp
@@ -1,6 +1,5 @@
 #pragma once
 
-#include "barretenberg/common/serialize.hpp"
 #include "barretenberg/numeric/uint128/uint128.hpp"
 #include "barretenberg/vm2/common/field.hpp"
 #include "barretenberg/vm2/common/memory_types.hpp"
@@ -72,7 +71,8 @@ struct Instruction {
     std::vector<Operand> operands;
 
     std::string to_string() const;
-    std::vector<uint8_t> encode() const;
+    // Serialize the instruction according to the specification from OPCODE_WIRE_FORMAT.
+    std::vector<uint8_t> serialize() const;
 
     bool operator==(const Instruction& other) const = default;
 };
@@ -87,6 +87,6 @@ struct Instruction {
  * @throws runtime_error exception when the bytecode is invalid or pos is out-of-range
  * @return The instruction
  */
-Instruction decode_instruction(std::span<const uint8_t> bytecode, size_t pos);
+Instruction deserialize_instruction(std::span<const uint8_t> bytecode, size_t pos);
 
 } // namespace bb::avm2::simulation
diff --git a/barretenberg/cpp/src/barretenberg/vm2/simulation/lib/serialization.test.cpp b/barretenberg/cpp/src/barretenberg/vm2/simulation/lib/serialization.test.cpp
@@ -5,7 +5,7 @@
 
 namespace bb::avm2 {
 namespace {
-using simulation::decode_instruction;
+using simulation::deserialize_instruction;
 using simulation::Instruction;
 using simulation::Operand;
 
@@ -15,7 +15,7 @@ TEST(SerializationTest, Not8RoundTrip)
     const Instruction instr = { .opcode = WireOpCode::NOT_8,
                                 .indirect = 5,
                                 .operands = { Operand::u8(123), Operand::u8(45) } };
-    const auto decoded = decode_instruction(instr.encode(), 0);
+    const auto decoded = deserialize_instruction(instr.serialize(), 0);
     EXPECT_EQ(instr, decoded);
 }
 
@@ -25,7 +25,7 @@ TEST(SerializationTest, Add16RoundTrip)
     const Instruction instr = { .opcode = WireOpCode::ADD_16,
                                 .indirect = 3,
                                 .operands = { Operand::u16(1000), Operand::u16(1001), Operand::u16(1002) } };
-    const auto decoded = decode_instruction(instr.encode(), 0);
+    const auto decoded = deserialize_instruction(instr.serialize(), 0);
     EXPECT_EQ(instr, decoded);
 }
 
@@ -35,7 +35,7 @@ TEST(SerializationTest, Jumpi32RoundTrip)
     const Instruction instr = { .opcode = WireOpCode::JUMPI_32,
                                 .indirect = 7,
                                 .operands = { Operand::u16(12345), Operand::u32(678901234) } };
-    const auto decoded = decode_instruction(instr.encode(), 0);
+    const auto decoded = deserialize_instruction(instr.serialize(), 0);
     EXPECT_EQ(instr, decoded);
 }
 
@@ -49,7 +49,7 @@ TEST(SerializationTest, Set64RoundTrip)
         .indirect = 2,
         .operands = { Operand::u16(1002), Operand::u8(static_cast<uint8_t>(MemoryTag::U64)), Operand::u64(value_64) }
     };
-    const auto decoded = decode_instruction(instr.encode(), 0);
+    const auto decoded = deserialize_instruction(instr.serialize(), 0);
     EXPECT_EQ(instr, decoded);
 }
 
@@ -63,7 +63,7 @@ TEST(SerializationTest, Set128RoundTrip)
         .indirect = 2,
         .operands = { Operand::u16(1002), Operand::u8(static_cast<uint8_t>(MemoryTag::U128)), Operand::u128(value_128) }
     };
-    const auto decoded = decode_instruction(instr.encode(), 0);
+    const auto decoded = deserialize_instruction(instr.serialize(), 0);
     EXPECT_EQ(instr, decoded);
 }
 
@@ -77,7 +77,7 @@ TEST(SerializationTest, SetFFRoundTrip)
         .indirect = 2,
         .operands = { Operand::u16(1002), Operand::u8(static_cast<uint8_t>(MemoryTag::FF)), Operand::ff(large_ff) }
     };
-    const auto decoded = decode_instruction(instr.encode(), 0);
+    const auto decoded = deserialize_instruction(instr.serialize(), 0);
     EXPECT_EQ(instr, decoded);
 }
 
diff --git a/barretenberg/cpp/src/barretenberg/vm2/testing/fixtures.cpp b/barretenberg/cpp/src/barretenberg/vm2/testing/fixtures.cpp
diff --git a/barretenberg/cpp/src/barretenberg/vm2/testing/fixtures.hpp b/barretenberg/cpp/src/barretenberg/vm2/testing/fixtures.hpp
diff --git a/barretenberg/cpp/src/barretenberg/vm2/tracegen/bytecode_trace.test.cpp b/barretenberg/cpp/src/barretenberg/vm2/tracegen/bytecode_trace.test.cpp

Original file line number	Diff line number	Diff line change
`@@ -31,7 +31,7 @@ TEST(InstructionSpecTest, CheckAllInstructionSizes)`
`31`	`31`	`const auto wire_opcode = static_cast<WireOpCode>(i);`
`32`	`32`	`const auto computed_size = compute_instruction_size(wire_opcode, wire_format, operand_type_sizes);`
`33`	`33`	`EXPECT_EQ(WIRE_INSTRUCTION_SPEC.at(wire_opcode).size_in_bytes, computed_size)`
`34`		`- << format("Incorrect size_in_bytes field for ", wire_opcode, " in WIRE_INSTRUCTION_SPEC.");`
	`34`	`+ << "Incorrect size_in_bytes field for " << wire_opcode << " in WIRE_INSTRUCTION_SPEC.";`
`35`	`35`	`}`
`36`	`36`	`}`
`37`	`37`
Original file line number	Diff line number	Diff line change
`@@ -227,6 +227,10 @@ Operand& Operand::operator=(const Operand& other)`
`227`	`227`
`228`	`228`	`bool Operand::operator==(const Operand& other) const`
`229`	`229`	`{`
	`230`	`+ if (this == &other) {`
	`231`	`+ return true;`
	`232`	`+ }`
	`233`	`+`
`230`	`234`	`if (value.index() != other.value.index()) {`
`231`	`235`	`return false;`
`232`	`236`	`}`
`@@ -348,7 +352,7 @@ std::string Operand::to_string() const`
`348`	`352`	`__builtin_unreachable();`
`349`	`353`	`}`
`350`	`354`
`351`		`-Instruction decode_instruction(std::span<const uint8_t> bytecode, size_t pos)`
	`355`	`+Instruction deserialize_instruction(std::span<const uint8_t> bytecode, size_t pos)`
`352`	`356`	`{`
`353`	`357`	`const auto bytecode_length = bytecode.size();`
`354`	`358`
`@@ -486,7 +490,7 @@ std::string Instruction::to_string() const`
`486`	`490`	`return oss.str();`
`487`	`491`	`}`
`488`	`492`
`489`		`-std::vector<uint8_t> Instruction::encode() const`
	`493`	`+std::vector<uint8_t> Instruction::serialize() const`
`490`	`494`	`{`
`491`	`495`	`std::vector<uint8_t> output;`
`492`	`496`	`output.reserve(WIRE_INSTRUCTION_SPEC.at(opcode).size_in_bytes);`