refactor bytecode trace builder

AztecProtocol · Sep 25, 2024 · c093e9f · c093e9f
1 parent 5fb8c4f
commit c093e9f
Show file tree

Hide file tree

Showing 9 changed files with 66 additions and 54 deletions.
diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/tests/execution.test.cpp
@@ -36,12 +36,12 @@ class AvmExecutionTests : public ::testing::Test {
                                                     ExecutionHints execution_hints,
                                                     uint32_t side_effect_counter,
                                                     std::vector<FF> calldata,
-                                                    std::vector<uint8_t> contract_bytecode) {
+                                                    const std::vector<std::vector<uint8_t>>& all_contracts_bytecode) {
             return AvmTraceBuilder(std::move(public_inputs),
                                    std::move(execution_hints),
                                    side_effect_counter,
                                    std::move(calldata),
-                                   contract_bytecode)
+                                   all_contracts_bytecode)
                 .set_full_precomputed_tables(false)
                 .set_range_check_required(false);
         });

diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/bytecode_trace.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/bytecode_trace.cpp
@@ -4,41 +4,42 @@
 
 namespace bb::avm_trace {
 using poseidon2 = crypto::Poseidon2<crypto::Poseidon2Bn254ScalarFieldParams>;
-AvmBytecodeTraceBuilder::AvmBytecodeTraceBuilder(const std::vector<uint8_t>& contract_bytecode,
-                                                 const ExecutionHints& hints)
+AvmBytecodeTraceBuilder::AvmBytecodeTraceBuilder(const std::vector<std::vector<uint8_t>>& all_contracts_bytecode)
+    : all_contracts_bytecode(all_contracts_bytecode)
+{}
+
+std::vector<FF> AvmBytecodeTraceBuilder::pack_bytecode(const std::vector<uint8_t>& contract_bytes)
 {
-    // Do we need to pad contract_bytecode to be 31bytes?
-    std::vector<std::vector<uint8_t>> all_contracts_bytecode{ contract_bytecode };
-    for (const auto& hint : hints.externalcall_hints) {
-        all_contracts_bytecode.push_back(hint.bytecode);
-    }
-    for (auto& contract : all_contracts_bytecode) {
-        // To make from_buffer<uint256_t> work properly, we need to make sure the contract is a multiple of 31 bytes
-        // Otherwise we will end up over-reading the buffer
-        size_t padding = 31 - (contract.size() % 31);
-        contract.resize(contract.size() + padding, 0);
-        std::vector<FF> contract_bytecode_fields;
-        for (size_t i = 0; i < contract.size(); i += 31) {
-            uint256_t u256_elem = from_buffer<uint256_t>(contract, i);
-            // Drop the last byte
-            contract_bytecode_fields.emplace_back(u256_elem >> 8);
-        }
-        this->all_contracts_bytecode.push_back(contract_bytecode_fields);
+    // To make from_buffer<uint256_t> work properly, we need to make sure the contract is a multiple of 31 bytes
+    // Otherwise we will end up over-reading the buffer
+    size_t padding = 31 - (contract_bytes.size() % 31);
+    // We dont want to mutate the original contract bytes, since we will (probably) need them later in the trace
+    // unpadded
+    std::vector<uint8_t> contract_bytes_padded = contract_bytes;
+    contract_bytes_padded.resize(contract_bytes_padded.size() + padding, 0);
+    std::vector<FF> contract_bytecode_fields;
+    for (size_t i = 0; i < contract_bytes_padded.size(); i += 31) {
+        uint256_t u256_elem = from_buffer<uint256_t>(contract_bytes_padded, i);
+        // Drop the last byte
+        contract_bytecode_fields.emplace_back(u256_elem >> 8);
     }
+    return contract_bytecode_fields;
 }
 
 void AvmBytecodeTraceBuilder::build_bytecode_columns()
 {
+    // This is the main loop that will generate the bytecode trace
     for (auto& contract_bytecode : all_contracts_bytecode) {
         FF running_hash = FF::zero();
+        auto packed_bytecode = pack_bytecode(contract_bytecode);
         // This size is already based on the number of fields
-        for (size_t i = 0; i < contract_bytecode.size(); ++i) {
+        for (size_t i = 0; i < packed_bytecode.size(); ++i) {
             bytecode_trace.push_back(BytecodeTraceEntry{
-                .packed_bytecode = contract_bytecode[i],
+                .packed_bytecode = packed_bytecode[i],
                 .running_hash = running_hash,
-                .bytecode_length_remaining = static_cast<uint16_t>(contract_bytecode.size() - i),
+                .bytecode_length_remaining = static_cast<uint16_t>(packed_bytecode.size() - i),
             });
-            running_hash = poseidon2::hash({ contract_bytecode[i], running_hash });
+            running_hash = poseidon2::hash({ packed_bytecode[i], running_hash });
         }
         // Now running_hash actually contains the bytecode hash
         BytecodeTraceEntry last_entry;

diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/bytecode_trace.hpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/bytecode_trace.hpp
@@ -22,8 +22,9 @@ class AvmBytecodeTraceBuilder {
         // Derive the contract address
         FF contract_address{};
     };
+    AvmBytecodeTraceBuilder() = default;
     // These interfaces will change when we start feeding in more inputs and hints
-    AvmBytecodeTraceBuilder(const std::vector<uint8_t>& contract_bytecode, const ExecutionHints& hints);
+    AvmBytecodeTraceBuilder(const std::vector<std::vector<uint8_t>>& all_contracts_bytecode);
 
     size_t size() const { return bytecode_trace.size(); }
     void reset();
@@ -32,9 +33,12 @@ class AvmBytecodeTraceBuilder {
     void build_bytecode_columns();
 
   private:
+    // Converts the bytecode into field elements (chunks of 31 bytes)
+    static std::vector<FF> pack_bytecode(const std::vector<uint8_t>& contract_bytes);
+
     std::vector<BytecodeTraceEntry> bytecode_trace;
-    // This will contain the bytecode as field elements
-    std::vector<std::vector<FF>> all_contracts_bytecode;
+    // The first element is the main top-level contract, the rest are external calls
+    std::vector<std::vector<uint8_t>> all_contracts_bytecode;
     // TODO: Come back to this
     // VmPublicInputs public_inputs;
     // ExecutionHints hints;

diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.cpp
@@ -146,17 +146,18 @@ void show_trace_info(const auto& trace)
 } // namespace
 
 // Needed for dependency injection in tests.
-Execution::TraceBuilderConstructor Execution::trace_builder_constructor = [](VmPublicInputs public_inputs,
-                                                                             ExecutionHints execution_hints,
-                                                                             uint32_t side_effect_counter,
-                                                                             std::vector<FF> calldata,
-                                                                             std::vector<uint8_t> contract_bytecode) {
-    return AvmTraceBuilder(std::move(public_inputs),
-                           std::move(execution_hints),
-                           side_effect_counter,
-                           std::move(calldata),
-                           contract_bytecode);
-};
+Execution::TraceBuilderConstructor Execution::trace_builder_constructor =
+    [](VmPublicInputs public_inputs,
+       ExecutionHints execution_hints,
+       uint32_t side_effect_counter,
+       std::vector<FF> calldata,
+       std::vector<std::vector<uint8_t>> all_contract_bytecode) {
+        return AvmTraceBuilder(std::move(public_inputs),
+                               std::move(execution_hints),
+                               side_effect_counter,
+                               std::move(calldata),
+                               all_contract_bytecode);
+    };
 
 /**
  * @brief Temporary routine to generate default public inputs (gas values) until we get
@@ -434,9 +435,15 @@ std::vector<Row> Execution::gen_trace(std::vector<uint8_t> const& bytecode,
     uint32_t start_side_effect_counter =
         !public_inputs_vec.empty() ? static_cast<uint32_t>(public_inputs_vec[PCPI_START_SIDE_EFFECT_COUNTER_OFFSET])
                                    : 0;
-
+    std::vector<std::vector<uint8_t>> all_contract_bytecode;
+    all_contract_bytecode.reserve(execution_hints.externalcall_hints.size() + 1);
+    // Start with the main, top-level contract bytecode
+    all_contract_bytecode.push_back(bytecode);
+    for (const auto& externalcall_hint : execution_hints.externalcall_hints) {
+        all_contract_bytecode.emplace_back(externalcall_hint.bytecode);
+    }
     AvmTraceBuilder trace_builder = Execution::trace_builder_constructor(
-        public_inputs, execution_hints, start_side_effect_counter, calldata, bytecode);
+        public_inputs, execution_hints, start_side_effect_counter, calldata, all_contract_bytecode);
 
     // Copied version of pc maintained in trace builder. The value of pc is evolving based
     // on opcode logic and therefore is not maintained here. However, the next opcode in the execution

diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.hpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/execution.hpp
@@ -15,11 +15,12 @@ namespace bb::avm_trace {
 class Execution {
   public:
     static constexpr size_t SRS_SIZE = 1 << 22;
-    using TraceBuilderConstructor = std::function<AvmTraceBuilder(VmPublicInputs public_inputs,
-                                                                  ExecutionHints execution_hints,
-                                                                  uint32_t side_effect_counter,
-                                                                  std::vector<FF> calldata,
-                                                                  std::vector<uint8_t> contract_bytecode)>;
+    using TraceBuilderConstructor =
+        std::function<AvmTraceBuilder(VmPublicInputs public_inputs,
+                                      ExecutionHints execution_hints,
+                                      uint32_t side_effect_counter,
+                                      std::vector<FF> calldata,
+                                      const std::vector<std::vector<uint8_t>>& all_contract_bytecode)>;
 
     Execution() = default;
 

diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp
@@ -278,16 +278,16 @@ void AvmTraceBuilder::finalise_mem_trace_lookup_counts()
  *        underlying traces and initialize gas values.
  */
 AvmTraceBuilder::AvmTraceBuilder(VmPublicInputs public_inputs,
-                                 const ExecutionHints& execution_hints_,
+                                 ExecutionHints execution_hints_,
                                  uint32_t side_effect_counter,
                                  std::vector<FF> calldata,
-                                 const std::vector<uint8_t>& contract_bytecode)
+                                 const std::vector<std::vector<uint8_t>>& all_contract_bytecode)
     // NOTE: we initialise the environment builder here as it requires public inputs
     : calldata(std::move(calldata))
     , side_effect_counter(side_effect_counter)
     , execution_hints(std::move(execution_hints_))
     , kernel_trace_builder(side_effect_counter, public_inputs, execution_hints)
-    , bytecode_trace_builder(contract_bytecode, execution_hints)
+    , bytecode_trace_builder(all_contract_bytecode)
 {
     // TODO: think about cast
     gas_trace_builder.set_initial_gas(

diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.hpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.hpp
@@ -54,10 +54,10 @@ class AvmTraceBuilder {
 
   public:
     AvmTraceBuilder(VmPublicInputs public_inputs = {},
-                    const ExecutionHints& execution_hints = {},
+                    ExecutionHints execution_hints = {},
                     uint32_t side_effect_counter = 0,
                     std::vector<FF> calldata = {},
-                    const std::vector<uint8_t>& contract_bytecode = {});
+                    const std::vector<std::vector<uint8_t>>& all_contract_bytecode = {});
 
     uint32_t getPc() const { return pc; }
 

diff --git a/yarn-project/circuits.js/src/contract/contract_class_id.test.ts b/yarn-project/circuits.js/src/contract/contract_class_id.test.ts
@@ -25,7 +25,7 @@ describe('ContractClass', () => {
       };
 
       expect(computeContractClassId(contractClass).toString()).toMatchInlineSnapshot(
-        `"0x174bf0daff21f2b8b88f7d2b7ef7ed6a7b083c979a2996a4e78963ad4b84ff8d"`,
+        `"0x2d5c712c483891d42e5bca539e8516fc52b5b024568ac71e4fe47c0c0157f851"`,
       );
     });
   });

diff --git a/yarn-project/circuits.js/src/contract/events/contract_class_registered_event.test.ts b/yarn-project/circuits.js/src/contract/events/contract_class_registered_event.test.ts
@@ -11,9 +11,8 @@ describe('ContractClassRegisteredEvent', () => {
     );
     expect(event.artifactHash.toString()).toEqual('0x072dce903b1a299d6820eeed695480fe9ec46658b1101885816aed6dd86037f0');
     expect(event.packedPublicBytecode.length).toEqual(27090);
-    // TODO: #5860
     expect(computePublicBytecodeCommitment(event.packedPublicBytecode).toString()).toEqual(
-      '0x0000000000000000000000000000000000000000000000000000000000000005',
+      '0x0378491b34825cd67d1e13e140bbc80f2cd3a9b52171ea577f8f11620d4198ba',
     );
   });
 });