From 54c1e3dea82c431c73e7e22e02d67129030abfbb Mon Sep 17 00:00:00 2001 From: IlyasRidhuan Date: Fri, 25 Oct 2024 14:15:21 +0000 Subject: [PATCH] fix(avm): address bytecode hashing comments --- .../vm/avm/trace/bytecode_trace.cpp | 19 +++++++++++++------ .../vm/avm/trace/bytecode_trace.hpp | 3 ++- .../bb-prover/src/avm_proving.test.ts | 16 +++------------- .../circuits.js/src/structs/avm/avm.ts | 4 ++-- .../src/avm_integration.test.ts | 17 ++++------------- .../simulator/src/avm/avm_simulator.ts | 5 +---- .../simulator/src/avm/journal/journal.ts | 3 +-- 7 files changed, 26 insertions(+), 41 deletions(-) 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 54617f82983..5956a653481 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/bytecode_trace.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/bytecode_trace.cpp @@ -118,22 +118,30 @@ void AvmBytecodeTraceBuilder::build_bytecode_hash_columns() bytecode_hash_trace.push_back(BytecodeHashTraceEntry{ .field_encoded_bytecode = field_encoded_bytecode[i], .running_hash = running_hash, - .bytecode_length_remaining = static_cast(field_encoded_bytecode.size() - i), + .bytecode_field_length_remaining = static_cast(field_encoded_bytecode.size() - i), }); + // We pair-wise hash the i-th bytecode field with the running hash (which is the output of previous i-1 + // round). I.e. + // initially running_hash = 0, + // the first round is running_hash = hash(bytecode[0], running_hash), + // the second round is running_hash = hash(bytecode[1],running_hash), and so on. running_hash = poseidon2::hash({ field_encoded_bytecode[i], running_hash }); } // Now running_hash actually contains the bytecode hash BytecodeHashTraceEntry last_entry; - last_entry.bytecode_length_remaining = 0; + last_entry.bytecode_field_length_remaining = 0; last_entry.running_hash = running_hash; + // Assert that the computed bytecode hash is the same as what we received as the hint ASSERT(running_hash == contract_bytecode.contract_class_id_preimage.public_bytecode_commitment); last_entry.class_id = compute_contract_class_id(contract_bytecode.contract_class_id_preimage.artifact_hash, contract_bytecode.contract_class_id_preimage.private_fn_root, running_hash); + // Assert that the computed class id is the same as what we received as the hint ASSERT(last_entry.class_id == contract_bytecode.contract_instance.contract_class_id); last_entry.contract_address = compute_address_from_instance(contract_bytecode.contract_instance); + // Assert that the computed contract address is the same as what we received as the hint ASSERT(last_entry.contract_address == contract_bytecode.contract_instance.address); } } @@ -144,9 +152,9 @@ void AvmBytecodeTraceBuilder::finalize(std::vector>& main_trace) auto const& src = bytecode_hash_trace.at(i); auto& dest = main_trace.at(i); dest.bytecode_running_hash = src.running_hash; - dest.bytecode_length_remaining = src.bytecode_length_remaining; + dest.bytecode_length_remaining = src.bytecode_field_length_remaining; dest.bytecode_as_fields = src.field_encoded_bytecode; - dest.bytecode_end_latch = src.bytecode_length_remaining == 0 ? FF::one() : FF::zero(); + dest.bytecode_end_latch = src.bytecode_field_length_remaining == 0 ? FF::one() : FF::zero(); } // We should probably combine this step with the previous one @@ -157,9 +165,8 @@ void AvmBytecodeTraceBuilder::finalize(std::vector>& main_trace) for (auto& byte : bytecode.bytecode) { main_trace.at(row_index).bytecode_bytes = byte; main_trace.at(row_index).bytecode_bytes_pc = byte_index; + row_index++; } - // The row index will match up with the clk - row_index += bytecode.bytecode.size(); } } 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 c7f9daddf10..72288082dab 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/bytecode_trace.hpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm/trace/bytecode_trace.hpp @@ -4,6 +4,7 @@ #include "barretenberg/vm/avm/trace/common.hpp" #include "barretenberg/vm/avm/trace/execution_hints.hpp" + namespace bb::avm_trace { class AvmBytecodeTraceBuilder { @@ -18,7 +19,7 @@ class AvmBytecodeTraceBuilder { // Calculate the bytecode hash FF running_hash{}; // This is the length in fields, not bytes - max 3000 fields - uint16_t bytecode_length_remaining = 0; + uint16_t bytecode_field_length_remaining = 0; // Derive the class Id FF arifact_hash{}; diff --git a/yarn-project/bb-prover/src/avm_proving.test.ts b/yarn-project/bb-prover/src/avm_proving.test.ts index 392a5f34171..72b5f3ce510 100644 --- a/yarn-project/bb-prover/src/avm_proving.test.ts +++ b/yarn-project/bb-prover/src/avm_proving.test.ts @@ -73,12 +73,6 @@ const proveAndVerifyAvmTestContract = async ( const contractClass = makeContractClassPublic(0, publicFn); const contractInstance = makeContractInstanceFromClassId(contractClass.id); - const nestedCallBytecode = getAvmTestContractBytecode('public_dispatch'); - const nestedCallFnSelector = getAvmTestContractFunctionSelector('public_dispatch'); - const nestedCallPublicFn: PublicFunction = { bytecode: nestedCallBytecode, selector: nestedCallFnSelector }; - const nestedCallContractClass = makeContractClassPublic(0, nestedCallPublicFn); - const nestedCallContractInstance = makeContractInstanceFromClassId(nestedCallContractClass.id); - const instanceGet = new SerializableContractInstance({ version: 1, salt: new Fr(0x123), @@ -96,9 +90,8 @@ const proveAndVerifyAvmTestContract = async ( worldStateDB.getContractInstance .mockResolvedValueOnce(contractInstance) .mockResolvedValueOnce(instanceGet) - .mockResolvedValueOnce(nestedCallContractInstance) - .mockResolvedValueOnce(nestedCallContractInstance); - worldStateDB.getContractClass.mockResolvedValueOnce(contractClass).mockResolvedValue(nestedCallContractClass); + .mockResolvedValue(contractInstance); + worldStateDB.getContractClass.mockResolvedValue(contractClass); const storageValue = new Fr(5); worldStateDB.storageRead.mockResolvedValue(Promise.resolve(storageValue)); @@ -113,10 +106,7 @@ const proveAndVerifyAvmTestContract = async ( }); const context = initContext({ env: environment, persistableState }); - worldStateDB.getBytecode - .mockResolvedValueOnce(bytecode) - .mockResolvedValue(nestedCallBytecode) - .mockResolvedValue(nestedCallBytecode); + worldStateDB.getBytecode.mockResolvedValue(bytecode); const startGas = new Gas(context.machineState.gasLeft.daGas, context.machineState.gasLeft.l2Gas); diff --git a/yarn-project/circuits.js/src/structs/avm/avm.ts b/yarn-project/circuits.js/src/structs/avm/avm.ts index fbc25367be1..97278f0c53a 100644 --- a/yarn-project/circuits.js/src/structs/avm/avm.ts +++ b/yarn-project/circuits.js/src/structs/avm/avm.ts @@ -273,7 +273,7 @@ export class AvmContractInstanceHint { export class AvmContractBytecodeHints { /* - * @param bytecode currently the bytecode of the nested call function, will be changed to the contract bytecode (via the dispatch function) of the nested call + * @param bytecode the contract bytecode * @param contractInstance the contract instance of the nested call, used to derive the contract address * @param contractClassPreimage the contract class preimage of the nested call, used to derive the class id * */ @@ -321,7 +321,7 @@ export class AvmContractBytecodeHints { * @returns An array of fields. */ static getFields(fields: FieldsOf) { - // Buffers aren't serialised the same way as they are read (lenth prefixed), so we need to do this manually. + // Buffers aren't serialised the same way as they are read (length prefixed), so we need to do this manually. const lengthPrefixedBytecode = Buffer.alloc(fields.bytecode.length + 4); // Add a 4-byte length prefix to the bytecode. lengthPrefixedBytecode.writeUInt32BE(fields.bytecode.length); diff --git a/yarn-project/ivc-integration/src/avm_integration.test.ts b/yarn-project/ivc-integration/src/avm_integration.test.ts index 503e5427868..9aa3016ded5 100644 --- a/yarn-project/ivc-integration/src/avm_integration.test.ts +++ b/yarn-project/ivc-integration/src/avm_integration.test.ts @@ -167,12 +167,6 @@ const proveAvmTestContract = async ( const contractClass = makeContractClassPublic(0, publicFn); const contractInstance = makeContractInstanceFromClassId(contractClass.id); - const nestedCallBytecode = getAvmTestContractBytecode('public_dispatch'); - const nestedCallFnSelector = getAvmTestContractFunctionSelector('public_dispatch'); - const nestedCallPublicFn: PublicFunction = { bytecode: nestedCallBytecode, selector: nestedCallFnSelector }; - const nestedCallContractClass = makeContractClassPublic(0, nestedCallPublicFn); - const nestedCallContractInstance = makeContractInstanceFromClassId(nestedCallContractClass.id); - const instanceGet = new SerializableContractInstance({ version: 1, salt: new Fr(0x123), @@ -190,9 +184,9 @@ const proveAvmTestContract = async ( worldStateDB.getContractInstance .mockResolvedValueOnce(contractInstance) .mockResolvedValueOnce(instanceGet) - .mockResolvedValueOnce(nestedCallContractInstance) - .mockResolvedValueOnce(nestedCallContractInstance); - worldStateDB.getContractClass.mockResolvedValueOnce(contractClass).mockResolvedValue(nestedCallContractClass); + .mockResolvedValue(contractInstance); + + worldStateDB.getContractClass.mockResolvedValue(contractClass); const storageValue = new Fr(5); worldStateDB.storageRead.mockResolvedValue(storageValue); @@ -207,10 +201,7 @@ const proveAvmTestContract = async ( }); const context = initContext({ env: environment, persistableState }); - worldStateDB.getBytecode - .mockResolvedValueOnce(bytecode) - .mockResolvedValue(nestedCallBytecode) - .mockResolvedValue(nestedCallBytecode); + worldStateDB.getBytecode.mockResolvedValue(bytecode); const startGas = new Gas(context.machineState.gasLeft.daGas, context.machineState.gasLeft.l2Gas); diff --git a/yarn-project/simulator/src/avm/avm_simulator.ts b/yarn-project/simulator/src/avm/avm_simulator.ts index 96c905cc58b..9a6012ef566 100644 --- a/yarn-project/simulator/src/avm/avm_simulator.ts +++ b/yarn-project/simulator/src/avm/avm_simulator.ts @@ -33,10 +33,7 @@ export class AvmSimulator { * Fetch the bytecode and execute it in the current context. */ public async execute(): Promise { - const bytecode = await this.context.persistableState.getBytecode( - this.context.environment.address, - this.context.environment.functionSelector, - ); + const bytecode = await this.context.persistableState.getBytecode(this.context.environment.address); // This assumes that we will not be able to send messages to accounts without code // Pending classes and instances impl details diff --git a/yarn-project/simulator/src/avm/journal/journal.ts b/yarn-project/simulator/src/avm/journal/journal.ts index 5a399ab9c61..4a19de59e7a 100644 --- a/yarn-project/simulator/src/avm/journal/journal.ts +++ b/yarn-project/simulator/src/avm/journal/journal.ts @@ -1,6 +1,5 @@ import { AztecAddress, - type FunctionSelector, type Gas, SerializableContractInstance, computePublicBytecodeCommitment, @@ -247,7 +246,7 @@ export class AvmPersistableStateManager { /** * Get a contract's bytecode from the contracts DB, also trace the contract class and instance */ - public async getBytecode(contractAddress: AztecAddress, _selector: FunctionSelector): Promise { + public async getBytecode(contractAddress: AztecAddress): Promise { let exists = true; let contractInstance = await this.worldStateDB.getContractInstance(contractAddress); // If the contract instance is not found, we assume it has not been deployed.