From 81dd3de4e9ca746a8c3b3824f7bd16df24810879 Mon Sep 17 00:00:00 2001 From: IlyasRidhuan Date: Wed, 25 Sep 2024 11:32:37 +0000 Subject: [PATCH] refactor bytecode trace builder --- .../vm/avm/tests/execution.test.cpp | 4 +- .../vm/avm/trace/bytecode_trace.cpp | 47 ++++++++++--------- .../vm/avm/trace/bytecode_trace.hpp | 10 ++-- .../barretenberg/vm/avm/trace/execution.cpp | 37 +++++++++------ .../barretenberg/vm/avm/trace/execution.hpp | 11 +++-- .../src/barretenberg/vm/avm/trace/trace.cpp | 6 +-- .../src/barretenberg/vm/avm/trace/trace.hpp | 4 +- .../src/main.nr | 3 ++ .../src/contract/contract_class_id.test.ts | 2 +- .../contract_class_registered_event.test.ts | 3 +- 10 files changed, 71 insertions(+), 56 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 index c25e0aa20b4f..3de1b6d07015 100644 --- 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 calldata, - std::vector contract_bytecode) { + const std::vector>& 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 index 199ae2b15ce6..82041e02871f 100644 --- 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; -AvmBytecodeTraceBuilder::AvmBytecodeTraceBuilder(const std::vector& contract_bytecode, - const ExecutionHints& hints) +AvmBytecodeTraceBuilder::AvmBytecodeTraceBuilder(const std::vector>& all_contracts_bytecode) + : all_contracts_bytecode(all_contracts_bytecode) +{} + +std::vector AvmBytecodeTraceBuilder::pack_bytecode(const std::vector& contract_bytes) { - // Do we need to pad contract_bytecode to be 31bytes? - std::vector> 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 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 contract_bytecode_fields; - for (size_t i = 0; i < contract.size(); i += 31) { - uint256_t u256_elem = from_buffer(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 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 contract_bytes_padded = contract_bytes; + contract_bytes_padded.resize(contract_bytes_padded.size() + padding, 0); + std::vector contract_bytecode_fields; + for (size_t i = 0; i < contract_bytes_padded.size(); i += 31) { + uint256_t u256_elem = from_buffer(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(contract_bytecode.size() - i), + .bytecode_length_remaining = static_cast(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 index 10700807ae01..a62784fe59e5 100644 --- 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& contract_bytecode, const ExecutionHints& hints); + AvmBytecodeTraceBuilder(const std::vector>& 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 pack_bytecode(const std::vector& contract_bytes); + std::vector bytecode_trace; - // This will contain the bytecode as field elements - std::vector> all_contracts_bytecode; + // The first element is the main top-level contract, the rest are external calls + std::vector> 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 index 229cf49fc6e2..c29d75111175 100644 --- 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 calldata, - std::vector 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 calldata, + std::vector> 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 @@ -253,7 +254,7 @@ bool Execution::verify(AvmFlavor::VerificationKey vk, HonkProof const& proof) std::copy(returndata_offset, raw_proof_offset, std::back_inserter(returndata)); std::copy(raw_proof_offset, proof.end(), std::back_inserter(raw_proof)); - VmPublicInputs public_inputs = convert_public_inputs(public_inputs_vec); + VmPublicInputs public_inputs = avm_trace::convert_public_inputs(public_inputs_vec); std::vector> public_inputs_columns = copy_public_inputs_columns(public_inputs, calldata, returndata); return verifier.verify_proof(raw_proof, public_inputs_columns); @@ -279,13 +280,19 @@ std::vector Execution::gen_trace(std::vector const& bytecode, vinfo("------- GENERATING TRACE -------"); // TODO(https://github.com/AztecProtocol/aztec-packages/issues/6718): construction of the public input columns // should be done in the kernel - this is stubbed and underconstrained - VmPublicInputs public_inputs = convert_public_inputs(public_inputs_vec); + VmPublicInputs public_inputs = avm_trace::convert_public_inputs(public_inputs_vec); uint32_t start_side_effect_counter = !public_inputs_vec.empty() ? static_cast(public_inputs_vec[PCPI_START_SIDE_EFFECT_COUNTER_OFFSET]) : 0; - + std::vector> 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 index 6ad0d777249e..da222b87f672 100644 --- 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 calldata, - std::vector contract_bytecode)>; + using TraceBuilderConstructor = + std::function calldata, + const std::vector>& 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 index a9162d4750da..3765a2f79246 100644 --- 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 calldata, - const std::vector& contract_bytecode) + const std::vector>& 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 index 0ccb8fe03b3d..684d41082411 100644 --- 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 calldata = {}, - const std::vector& contract_bytecode = {}); + const std::vector>& all_contract_bytecode = {}); uint32_t getPc() const { return pc; } diff --git a/noir-projects/noir-contracts/contracts/contract_class_registerer_contract/src/main.nr b/noir-projects/noir-contracts/contracts/contract_class_registerer_contract/src/main.nr index 069f0829028d..90bf691d1f15 100644 --- a/noir-projects/noir-contracts/contracts/contract_class_registerer_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/contract_class_registerer_contract/src/main.nr @@ -54,6 +54,9 @@ contract ContractClassRegisterer { if (i < bytecode_length_in_fields) { // We skip the first element when hashing since it is the length computed_public_bytecode_commitment = std::hash::poseidon2::Poseidon2::hash([packed_public_bytecode[i + 1],computed_public_bytecode_commitment],2); + } else { + // Any bytes after the bytecode length must be 0 + assert_eq(packed_public_bytecode[i + 1], 0); } } assert_eq(computed_public_bytecode_commitment, public_bytecode_commitment); 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 index c9b607c28c69..83c797abe1b3 100644 --- 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 index 4fec2f49d4a6..77d9e760faef 100644 --- 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', ); }); });