chore: misc cleanup in simulator (#7203)

CODEOWNERS

@@ -12,8 +12,8 @@
 /barretenberg/cpp/pil @Maddiaa0 @jeanmon @IlyasRidhuan @fcarreiro
 # on changes to PIL-generated C++
 /barretenberg/cpp/src/barretenberg/**/generated @jeanmon @IlyasRidhuan @fcarreiro
-# on changes to AVM trace (C++ witness generator)
-/barretenberg/cpp/src/barretenberg/vm/avm_trace @jeanmon @IlyasRidhuan @fcarreiro
+# on changes to AVM C++ code
+/barretenberg/cpp/src/barretenberg/vm @jeanmon @IlyasRidhuan @fcarreiro
 # on changes to public context in aztec-nr
 /noir-projects/aztec-nr/aztec/src/context/inputs/public_context_inputs.nr @fcarreiro @dbanks12
 /noir-projects/aztec-nr/aztec/src/context/public_context.nr @fcarreiro @dbanks12
@@ -24,7 +24,6 @@
 /yarn-project/simulator/src/public/side_effect_trace.test.ts @fcarreiro @dbanks12
 /yarn-project/simulator/src/public/side_effect_trace.ts @fcarreiro @dbanks12
 /yarn-project/simulator/src/public/side_effect_trace_interface.ts @fcarreiro @dbanks12
-/yarn-project/simulator/src/public/transitional_adaptors.ts @fcarreiro @dbanks12
 # on changes to the AVM transpiler
 /avm-transpiler/src @fcarreiro @dbanks12
 #####################################################

yarn-project/bb-prover/src/avm_proving.test.ts

@@ -285,9 +285,9 @@ const proveAndVerifyAvmTestContract = async (
 // TODO: pub somewhere more usable - copied from abstract phase manager
 const getPublicInputs = (result: PublicExecutionResult): PublicCircuitPublicInputs => {
   return PublicCircuitPublicInputs.from({
-    callContext: result.execution.callContext,
+    callContext: result.executionRequest.callContext,
     proverAddress: AztecAddress.ZERO,
-    argsHash: computeVarArgsHash(result.execution.args),
+    argsHash: computeVarArgsHash(result.executionRequest.args),
     newNoteHashes: padArrayEnd(result.newNoteHashes, NoteHash.empty(), MAX_NEW_NOTE_HASHES_PER_CALL),
     newNullifiers: padArrayEnd(result.newNullifiers, Nullifier.empty(), MAX_NEW_NULLIFIERS_PER_CALL),
     newL2ToL1Msgs: padArrayEnd(result.newL2ToL1Messages, L2ToL1Message.empty(), MAX_NEW_L2_TO_L1_MSGS_PER_CALL),

yarn-project/prover-client/src/mocks/test_context.ts

@@ -21,7 +21,7 @@ import { type DebugLogger } from '@aztec/foundation/log';
 import { openTmpStore } from '@aztec/kv-store/utils';
 import {
   type ContractsDataSourcePublicDB,
-  type PublicExecution,
+  type PublicExecutionRequest,
   type PublicExecutionResult,
   PublicExecutionResultBuilder,
   type PublicExecutor,
@@ -164,7 +164,7 @@ export class TestContext {
     txValidator?: TxValidator<ProcessedTx>,
   ) {
     const defaultExecutorImplementation = (
-      execution: PublicExecution,
+      execution: PublicExecutionRequest,
       _globalVariables: GlobalVariables,
       availableGas: Gas,
       _txContext: TxContext,
@@ -204,7 +204,7 @@ export class TestContext {
     blockProver?: BlockProver,
     txValidator?: TxValidator<ProcessedTx>,
     executorMock?: (
-      execution: PublicExecution,
+      execution: PublicExecutionRequest,
       globalVariables: GlobalVariables,
       availableGas: Gas,
       txContext: TxContext,

yarn-project/simulator/src/avm/avm_message_call_result.ts → yarn-project/simulator/src/avm/avm_contract_call_result.ts

@@ -5,7 +5,7 @@ import { type AvmRevertReason } from './errors.js';
 /**
  * Results of an contract call's execution in the AVM.
  */
-export class AvmContractCallResults {
+export class AvmContractCallResult {
   constructor(public reverted: boolean, public output: Fr[], public revertReason?: AvmRevertReason) {}
 
   toString(): string {

yarn-project/simulator/src/avm/avm_execution_environment.ts

@@ -17,7 +17,6 @@ export class AvmContextInputs {
  * Contains variables that remain constant during AVM execution
  * These variables are provided by the public kernel circuit
  */
-// TODO(https://github.com/AztecProtocol/aztec-packages/issues/3992): gas not implemented
 export class AvmExecutionEnvironment {
   private readonly calldataPrefixLength;
   constructor(
@@ -35,15 +34,15 @@ export class AvmExecutionEnvironment {
     public readonly gasSettings: GasSettings,
     public readonly transactionFee: Fr,
 
-    // Function selector is temporary since eventually public contract bytecode will be one blob
-    // containing all functions, and function selector will become an application-level mechanism
+    // Function selector may be temporary since eventually public contract bytecode will likely be one
+    // blob containing all functions, and function selector will become an application-level mechanism
     // (e.g. first few bytes of calldata + compiler-generated jump table)
-    public readonly temporaryFunctionSelector: FunctionSelector,
+    public readonly functionSelector: FunctionSelector,
   ) {
     // We encode some extra inputs (AvmContextInputs) in calldata.
     // This will have to go once we move away from one proof per call.
     const inputs = new AvmContextInputs(
-      temporaryFunctionSelector.toField(),
+      functionSelector.toField(),
       computeVarArgsHash(calldata),
       isStaticCall,
     ).toFields();

yarn-project/simulator/src/avm/avm_simulator.test.ts

@@ -9,11 +9,11 @@ import { type Fieldable } from '@aztec/foundation/serialize';
 import { mock } from 'jest-mock-extended';
 
 import { type PublicSideEffectTraceInterface } from '../public/side_effect_trace_interface.js';
-import { isAvmBytecode, markBytecodeAsAvm } from '../public/transitional_adaptors.js';
 import { type AvmExecutionEnvironment } from './avm_execution_environment.js';
 import { AvmMachineState } from './avm_machine_state.js';
 import { type MemoryValue, TypeTag, type Uint8 } from './avm_memory_types.js';
 import { AvmSimulator } from './avm_simulator.js';
+import { isAvmBytecode, markBytecodeAsAvm } from './bytecode_utils.js';
 import {
   adjustCalldataIndex,
   getAvmTestContractBytecode,
@@ -308,7 +308,7 @@ describe('AVM simulator: transpiled Noir contracts', () => {
     it('selector', async () => {
       const context = initContext({
         env: initExecutionEnvironment({
-          temporaryFunctionSelector: FunctionSelector.fromSignature('check_selector()'),
+          functionSelector: FunctionSelector.fromSignature('check_selector()'),
         }),
       });
       const bytecode = getAvmTestContractBytecode('check_selector');
@@ -345,8 +345,7 @@ describe('AVM simulator: transpiled Noir contracts', () => {
 
   describe('Side effects, world state, nested calls', () => {
     const address = new Fr(1);
-    // TODO(dbanks12): should be able to make address and storage address different
-    const storageAddress = new Fr(1);
+    const storageAddress = new Fr(2);
     const sender = new Fr(42);
     const leafIndex = new Fr(7);
     const slotNumber = 1; // must update Noir contract if changing this
@@ -411,7 +410,7 @@ describe('AVM simulator: transpiled Noir contracts', () => {
 
     describe.each([[/*exists=*/ false], [/*exists=*/ true]])('Nullifier checks', (exists: boolean) => {
       const existsStr = exists ? 'DOES exist' : 'does NOT exist';
-      it(`Should return ${exists} (and be traced) when noteHash ${existsStr}`, async () => {
+      it(`Should return ${exists} (and be traced) when nullifier ${existsStr}`, async () => {
         const calldata = [value0];
         const context = createContext(calldata);
         const bytecode = getAvmTestContractBytecode('nullifier_exists');
@@ -430,7 +429,7 @@ describe('AVM simulator: transpiled Noir contracts', () => {
         const tracedLeafIndex = exists && !isPending ? leafIndex : Fr.ZERO;
         expect(trace.traceNullifierCheck).toHaveBeenCalledWith(
           storageAddress,
-          value0,
+          /*nullifier=*/ value0,
           tracedLeafIndex,
           exists,
           isPending,
@@ -451,7 +450,7 @@ describe('AVM simulator: transpiled Noir contracts', () => {
         ? `at leafIndex=${mockAtLeafIndex.toNumber()} (exists at leafIndex=${leafIndex.toNumber()})`
         : '';
 
-      it(`Should return ${expectFound} (and be traced) when noteHash ${existsStr} ${foundAtStr}`, async () => {
+      it(`Should return ${expectFound} (and be traced) when message ${existsStr} ${foundAtStr}`, async () => {
         const calldata = [value0, leafIndex];
         const context = createContext(calldata);
         const bytecode = getAvmTestContractBytecode('l1_to_l2_msg_exists');
@@ -466,7 +465,7 @@ describe('AVM simulator: transpiled Noir contracts', () => {
         expect(trace.traceL1ToL2MessageCheck).toHaveBeenCalledTimes(1);
         expect(trace.traceL1ToL2MessageCheck).toHaveBeenCalledWith(
           address,
-          /*noteHash=*/ value0,
+          /*msgHash=*/ value0,
           leafIndex,
           /*exists=*/ expectFound,
         );
@@ -485,7 +484,7 @@ describe('AVM simulator: transpiled Noir contracts', () => {
       expect(trace.traceNewNoteHash).toHaveBeenCalledTimes(1);
       expect(trace.traceNewNoteHash).toHaveBeenCalledWith(
         expect.objectContaining(storageAddress),
-        /*nullifier=*/ value0,
+        /*noteHash=*/ value0,
       );
     });
 
@@ -525,7 +524,7 @@ describe('AVM simulator: transpiled Noir contracts', () => {
         // leafIndex is returned from DB call for nullifiers, so it is absent on DB miss
         expect(trace.traceNullifierCheck).toHaveBeenCalledWith(
           storageAddress,
-          value0,
+          /*nullifier=*/ value0,
           /*leafIndex=*/ Fr.ZERO,
           /*exists=*/ true,
           /*isPending=*/ true,
@@ -639,8 +638,8 @@ describe('AVM simulator: transpiled Noir contracts', () => {
         const results = await new AvmSimulator(context).executeBytecode(bytecode);
         expect(results.reverted).toBe(false);
 
-        expect(await context.persistableState.peekStorage(address, listSlot0)).toEqual(calldata[0]);
-        expect(await context.persistableState.peekStorage(address, listSlot1)).toEqual(calldata[1]);
+        expect(await context.persistableState.peekStorage(storageAddress, listSlot0)).toEqual(calldata[0]);
+        expect(await context.persistableState.peekStorage(storageAddress, listSlot1)).toEqual(calldata[1]);
 
         expect(trace.tracePublicStorageWrite).toHaveBeenCalledTimes(2);
         expect(trace.tracePublicStorageWrite).toHaveBeenCalledWith(storageAddress, listSlot0, value0);

yarn-project/simulator/src/avm/avm_simulator.ts

@@ -2,9 +2,9 @@ import { type DebugLogger, createDebugLogger } from '@aztec/foundation/log';
 
 import { strict as assert } from 'assert';
 
-import { decompressBytecodeIfCompressed, isAvmBytecode } from '../public/transitional_adaptors.js';
 import type { AvmContext } from './avm_context.js';
-import { AvmContractCallResults } from './avm_message_call_result.js';
+import { AvmContractCallResult } from './avm_contract_call_result.js';
+import { decompressBytecodeIfCompressed, isAvmBytecode } from './bytecode_utils.js';
 import {
   AvmExecutionError,
   InvalidProgramCounterError,
@@ -20,18 +20,16 @@ export class AvmSimulator {
   private bytecode: Buffer | undefined;
 
   constructor(private context: AvmContext) {
-    this.log = createDebugLogger(
-      `aztec:avm_simulator:core(f:${context.environment.temporaryFunctionSelector.toString()})`,
-    );
+    this.log = createDebugLogger(`aztec:avm_simulator:core(f:${context.environment.functionSelector.toString()})`);
   }
 
   /**
    * Fetch the bytecode and execute it in the current context.
    */
-  public async execute(): Promise<AvmContractCallResults> {
+  public async execute(): Promise<AvmContractCallResult> {
     const bytecode = await this.context.persistableState.getBytecode(
       this.context.environment.address,
-      this.context.environment.temporaryFunctionSelector,
+      this.context.environment.functionSelector,
     );
 
     // This assumes that we will not be able to send messages to accounts without code
@@ -54,7 +52,7 @@ export class AvmSimulator {
    * Executes the provided bytecode in the current context.
    * This method is useful for testing and debugging.
    */
-  public async executeBytecode(bytecode: Buffer): Promise<AvmContractCallResults> {
+  public async executeBytecode(bytecode: Buffer): Promise<AvmContractCallResult> {
     const decompressedBytecode = await decompressBytecodeIfCompressed(bytecode);
     assert(isAvmBytecode(decompressedBytecode), "AVM simulator can't execute non-AVM bytecode");
 
@@ -66,7 +64,7 @@ export class AvmSimulator {
    * Executes the provided instructions in the current context.
    * This method is useful for testing and debugging.
    */
-  public async executeInstructions(instructions: Instruction[]): Promise<AvmContractCallResults> {
+  public async executeInstructions(instructions: Instruction[]): Promise<AvmContractCallResult> {
     assert(instructions.length > 0);
     const { machineState } = this.context;
     try {
@@ -95,7 +93,7 @@ export class AvmSimulator {
       const output = machineState.getOutput();
       const reverted = machineState.getReverted();
       const revertReason = reverted ? revertReasonFromExplicitRevert(output, this.context) : undefined;
-      const results = new AvmContractCallResults(reverted, output, revertReason);
+      const results = new AvmContractCallResult(reverted, output, revertReason);
       this.log.debug(`Context execution results: ${results.toString()}`);
       // Return results for processing by calling context
       return results;
@@ -108,7 +106,7 @@ export class AvmSimulator {
 
       const revertReason = revertReasonFromExceptionalHalt(err, this.context);
       // Note: "exceptional halts" cannot return data, hence []
-      const results = new AvmContractCallResults(/*reverted=*/ true, /*output=*/ [], revertReason);
+      const results = new AvmContractCallResult(/*reverted=*/ true, /*output=*/ [], revertReason);
       this.log.debug(`Context execution results: ${results.toString()}`);
       // Return results for processing by calling context
       return results;

yarn-project/simulator/src/avm/bytecode_utils.ts

@@ -0,0 +1,32 @@
+import { promisify } from 'util';
+import { gunzip } from 'zlib';
+
+import { Mov } from '../avm/opcodes/memory.js';
+
+const AVM_MAGIC_SUFFIX = Buffer.from([
+  Mov.opcode, // opcode
+  0x00, // indirect
+  ...Buffer.from('000018ca', 'hex'), // srcOffset
+  ...Buffer.from('000018ca', 'hex'), // dstOffset
+]);
+
+export function markBytecodeAsAvm(bytecode: Buffer): Buffer {
+  return Buffer.concat([bytecode, AVM_MAGIC_SUFFIX]);
+}
+
+// This is just a helper function for the AVM simulator
+export async function decompressBytecodeIfCompressed(bytecode: Buffer): Promise<Buffer> {
+  try {
+    return await promisify(gunzip)(bytecode);
+  } catch {
+    // If the bytecode is not compressed, the gunzip call will throw an error
+    // In this case, we assume the bytecode is not compressed and continue.
+    return Promise.resolve(bytecode);
+  }
+}
+
+export async function isAvmBytecode(bytecode: Buffer): Promise<boolean> {
+  const decompressedBytecode = await decompressBytecodeIfCompressed(bytecode);
+  const magicSize = AVM_MAGIC_SUFFIX.length;
+  return decompressedBytecode.subarray(-magicSize).equals(AVM_MAGIC_SUFFIX);
+}

yarn-project/simulator/src/avm/errors.ts

@@ -113,7 +113,7 @@ function createRevertReason(message: string, context: AvmContext, nestedError?:
     message,
     /*failingFunction=*/ {
       contractAddress: context.environment.address,
-      functionSelector: context.environment.temporaryFunctionSelector,
+      functionSelector: context.environment.functionSelector,
     },
     /*noirCallStack=*/ [...context.machineState.internalCallStack, context.machineState.pc].map(pc => `0.${pc}`),
     /*options=*/ { cause: nestedError },

yarn-project/simulator/src/avm/fixtures/index.ts

@@ -82,7 +82,7 @@ export function initExecutionEnvironment(overrides?: Partial<AvmExecutionEnviron
     overrides?.calldata ?? [],
     overrides?.gasSettings ?? GasSettings.empty(),
     overrides?.transactionFee ?? Fr.ZERO,
-    overrides?.temporaryFunctionSelector ?? FunctionSelector.empty(),
+    overrides?.functionSelector ?? FunctionSelector.empty(),
   );
 }
 

yarn-project/simulator/src/avm/journal/journal.ts

@@ -5,8 +5,8 @@ import { SerializableContractInstance } from '@aztec/types/contracts';
 
 import { type TracedContractInstance } from '../../public/side_effect_trace.js';
 import { type PublicSideEffectTraceInterface } from '../../public/side_effect_trace_interface.js';
+import { type AvmContractCallResult } from '../avm_contract_call_result.js';
 import { type AvmExecutionEnvironment } from '../avm_execution_environment.js';
-import { type AvmContractCallResults } from '../avm_message_call_result.js';
 import { type HostStorage } from './host_storage.js';
 import { NullifierManager } from './nullifiers.js';
 import { PublicStorage } from './public_storage.js';
@@ -25,10 +25,11 @@ export class AvmPersistableStateManager {
 
   constructor(
     /** Reference to node storage */
-    private hostStorage: HostStorage,
+    private readonly hostStorage: HostStorage,
     /** Side effect trace */
-    private trace: PublicSideEffectTraceInterface,
+    private readonly trace: PublicSideEffectTraceInterface,
     /** Public storage, including cached writes */
+    // TODO(5818): make private once no longer accessed in executor
     public readonly publicStorage: PublicStorage,
     /** Nullifier set, including cached/recently-emitted nullifiers */
     private readonly nullifiers: NullifierManager,
@@ -243,22 +244,23 @@ export class AvmPersistableStateManager {
    */
   public async processNestedCall(
     nestedState: AvmPersistableStateManager,
-    success: boolean,
     nestedEnvironment: AvmExecutionEnvironment,
     startGasLeft: Gas,
     endGasLeft: Gas,
     bytecode: Buffer,
-    avmCallResults: AvmContractCallResults,
+    avmCallResults: AvmContractCallResult,
   ) {
-    if (success) {
+    if (!avmCallResults.reverted) {
       this.acceptNestedCallState(nestedState);
     }
     const functionName =
       (await nestedState.hostStorage.contractsDb.getDebugFunctionName(
         nestedEnvironment.address,
-        nestedEnvironment.temporaryFunctionSelector,
-      )) ?? `${nestedEnvironment.address}:${nestedEnvironment.temporaryFunctionSelector}`;
+        nestedEnvironment.functionSelector,
+      )) ?? `${nestedEnvironment.address}:${nestedEnvironment.functionSelector}`;
+
     this.log.verbose(`[AVM] Calling nested function ${functionName}`);
+
     this.trace.traceNestedCall(
       nestedState.trace,
       nestedEnvironment,

yarn-project/simulator/src/avm/journal/public_storage.ts

@@ -16,7 +16,7 @@ type PublicStorageReadResult = {
  */
 export class PublicStorage {
   /** Cached storage writes. */
-  private cache: PublicStorageCache;
+  private readonly cache: PublicStorageCache;
 
   constructor(
     /** Reference to node storage. Checked on parent cache-miss. */

yarn-project/simulator/src/avm/opcodes/accrued_substate.test.ts

@@ -180,7 +180,7 @@ describe('Accrued Substate', () => {
         const tracedLeafIndex = exists && !isPending ? leafIndex : Fr.ZERO;
         expect(trace.traceNullifierCheck).toHaveBeenCalledWith(
           storageAddress,
-          value0,
+          /*nullifier=*/ value0,
           tracedLeafIndex,
           exists,
           isPending,
@@ -292,7 +292,7 @@ describe('Accrued Substate', () => {
         expect(trace.traceL1ToL2MessageCheck).toHaveBeenCalledTimes(1);
         expect(trace.traceL1ToL2MessageCheck).toHaveBeenCalledWith(
           address,
-          /*noteHash=*/ value0,
+          /*msgHash=*/ value0,
           leafIndex,
           /*exists=*/ expectFound,
         );