chore(avm): Remove function selector from AvmExecutionEnvironment (#1…

…0532)

yarn-project/circuit-types/src/simulation_error.ts

@@ -19,7 +19,7 @@ export interface FailingFunction {
   /**
    * The selector of the function that failed.
    */
-  functionSelector: FunctionSelector;
+  functionSelector?: FunctionSelector;
   /**
    * The name of the function that failed.
    */
@@ -160,6 +160,7 @@ export class SimulationError extends Error {
     this.functionErrorStack.forEach(failingFunction => {
       if (
         failingFunction.contractAddress.equals(contractAddress) &&
+        !!failingFunction.functionSelector &&
         failingFunction.functionSelector.equals(functionSelector)
       ) {
         failingFunction.functionName = functionName;
@@ -175,7 +176,7 @@ export class SimulationError extends Error {
     const stackLines: string[] = [
       ...functionCallStack.map(failingFunction => {
         return `at ${failingFunction.contractName ?? failingFunction.contractAddress.toString()}.${
-          failingFunction.functionName ?? failingFunction.functionSelector.toString()
+          failingFunction.functionName ?? failingFunction.functionSelector?.toString() ?? 'unknown'
         }`;
       }),
       ...noirCallStack.map(errorLocation =>

yarn-project/pxe/src/pxe_service/error_enriching.ts

@@ -20,7 +20,7 @@ export async function enrichSimulationError(err: SimulationError, db: PxeDatabas
     if (!mentionedFunctions.has(contractAddress.toString())) {
       mentionedFunctions.set(contractAddress.toString(), new Set());
     }
-    mentionedFunctions.get(contractAddress.toString())!.add(functionSelector.toString());
+    mentionedFunctions.get(contractAddress.toString())!.add(functionSelector?.toString() ?? '');
   });
 
   await Promise.all(
@@ -89,7 +89,9 @@ export async function enrichPublicSimulationError(
         err.setNoirCallStack(parsedCallStack);
       } catch (err) {
         logger.warn(
-          `Could not resolve noir call stack for ${originalFailingFunction.contractAddress.toString()}:${originalFailingFunction.functionSelector.toString()}: ${err}`,
+          `Could not resolve noir call stack for ${originalFailingFunction.contractAddress.toString()}:${
+            originalFailingFunction.functionName?.toString() ?? ''
+          }: ${err}`,
         );
       }
     }

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

@@ -1,4 +1,4 @@
-import { type AztecAddress, FunctionSelector } from '@aztec/circuits.js';
+import { type AztecAddress } from '@aztec/circuits.js';
 import { type Fr } from '@aztec/foundation/fields';
 
 import { type AvmExecutionEnvironment } from './avm_execution_environment.js';
@@ -43,13 +43,12 @@ export class AvmContext {
     calldata: Fr[],
     allocatedGas: Gas,
     callType: 'CALL' | 'STATICCALL',
-    functionSelector: FunctionSelector = FunctionSelector.empty(),
   ): AvmContext {
     const deriveFn =
       callType === 'CALL'
         ? this.environment.deriveEnvironmentForNestedCall
         : this.environment.deriveEnvironmentForNestedStaticCall;
-    const newExecutionEnvironment = deriveFn.call(this.environment, address, calldata, functionSelector);
+    const newExecutionEnvironment = deriveFn.call(this.environment, address, calldata);
     const forkedWorldState = this.persistableState.fork();
     const machineState = AvmMachineState.fromState(gasToGasLeft(allocatedGas));
     return new AvmContext(forkedWorldState, newExecutionEnvironment, machineState);

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

@@ -1,16 +1,15 @@
-import { AztecAddress, FunctionSelector } from '@aztec/circuits.js';
+import { AztecAddress } from '@aztec/circuits.js';
 import { Fr } from '@aztec/foundation/fields';
 
 import { allSameExcept, initExecutionEnvironment } from './fixtures/index.js';
 
 describe('Execution Environment', () => {
   const newAddress = AztecAddress.fromNumber(123456);
   const calldata = [new Fr(1n), new Fr(2n), new Fr(3n)];
-  const selector = FunctionSelector.empty();
 
   it('New call should fork execution environment correctly', () => {
     const executionEnvironment = initExecutionEnvironment();
-    const newExecutionEnvironment = executionEnvironment.deriveEnvironmentForNestedCall(newAddress, calldata, selector);
+    const newExecutionEnvironment = executionEnvironment.deriveEnvironmentForNestedCall(newAddress, calldata);
 
     expect(newExecutionEnvironment).toEqual(
       allSameExcept(executionEnvironment, {
@@ -21,13 +20,9 @@ describe('Execution Environment', () => {
     );
   });
 
-  it('New static call call should fork execution environment correctly', () => {
+  it('New static call should fork execution environment correctly', () => {
     const executionEnvironment = initExecutionEnvironment();
-    const newExecutionEnvironment = executionEnvironment.deriveEnvironmentForNestedStaticCall(
-      newAddress,
-      calldata,
-      selector,
-    );
+    const newExecutionEnvironment = executionEnvironment.deriveEnvironmentForNestedStaticCall(newAddress, calldata);
 
     expect(newExecutionEnvironment).toEqual(
       allSameExcept(executionEnvironment, {

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

@@ -1,4 +1,4 @@
-import { FunctionSelector, type GlobalVariables } from '@aztec/circuits.js';
+import { type GlobalVariables } from '@aztec/circuits.js';
 import { type AztecAddress } from '@aztec/foundation/aztec-address';
 import { Fr } from '@aztec/foundation/fields';
 
@@ -10,24 +10,17 @@ export class AvmExecutionEnvironment {
   constructor(
     public readonly address: AztecAddress,
     public readonly sender: AztecAddress,
-    public readonly functionSelector: FunctionSelector, // may be temporary (#7224)
     public readonly contractCallDepth: Fr,
     public readonly transactionFee: Fr,
     public readonly globals: GlobalVariables,
     public readonly isStaticCall: boolean,
     public readonly calldata: Fr[],
   ) {}
 
-  private deriveEnvironmentForNestedCallInternal(
-    targetAddress: AztecAddress,
-    calldata: Fr[],
-    functionSelector: FunctionSelector,
-    isStaticCall: boolean,
-  ) {
+  private deriveEnvironmentForNestedCallInternal(targetAddress: AztecAddress, calldata: Fr[], isStaticCall: boolean) {
     return new AvmExecutionEnvironment(
       /*address=*/ targetAddress,
       /*sender=*/ this.address,
-      functionSelector,
       this.contractCallDepth.add(Fr.ONE),
       this.transactionFee,
       this.globals,
@@ -36,29 +29,11 @@ export class AvmExecutionEnvironment {
     );
   }
 
-  public deriveEnvironmentForNestedCall(
-    targetAddress: AztecAddress,
-    calldata: Fr[],
-    functionSelector: FunctionSelector = FunctionSelector.empty(),
-  ): AvmExecutionEnvironment {
-    return this.deriveEnvironmentForNestedCallInternal(
-      targetAddress,
-      calldata,
-      functionSelector,
-      /*isStaticCall=*/ false,
-    );
+  public deriveEnvironmentForNestedCall(targetAddress: AztecAddress, calldata: Fr[]): AvmExecutionEnvironment {
+    return this.deriveEnvironmentForNestedCallInternal(targetAddress, calldata, /*isStaticCall=*/ false);
   }
 
-  public deriveEnvironmentForNestedStaticCall(
-    targetAddress: AztecAddress,
-    calldata: Fr[],
-    functionSelector: FunctionSelector,
-  ): AvmExecutionEnvironment {
-    return this.deriveEnvironmentForNestedCallInternal(
-      targetAddress,
-      calldata,
-      functionSelector,
-      /*isStaticCall=*/ true,
-    );
+  public deriveEnvironmentForNestedStaticCall(targetAddress: AztecAddress, calldata: Fr[]): AvmExecutionEnvironment {
+    return this.deriveEnvironmentForNestedCallInternal(targetAddress, calldata, /*isStaticCall=*/ true);
   }
 }

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

@@ -160,7 +160,6 @@ describe('AVM simulator: transpiled Noir contracts', () => {
     const ephemeralTrees = await AvmEphemeralForest.create(worldStateDB.getMerkleInterface());
     const persistableState = initPersistableStateManager({ worldStateDB, trace, merkleTrees: ephemeralTrees });
     const environment = initExecutionEnvironment({
-      functionSelector,
       calldata,
       globals,
       address: contractInstance.address,
@@ -434,7 +433,6 @@ describe('AVM simulator: transpiled Noir contracts', () => {
   describe('Environment getters', () => {
     const address = AztecAddress.random();
     const sender = AztecAddress.random();
-    const functionSelector = FunctionSelector.random();
     const transactionFee = Fr.random();
     const chainId = Fr.random();
     const version = Fr.random();
@@ -453,7 +451,6 @@ describe('AVM simulator: transpiled Noir contracts', () => {
     const env = initExecutionEnvironment({
       address,
       sender,
-      functionSelector,
       transactionFee,
       globals,
     });

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

@@ -1,10 +1,4 @@
-import {
-  type AztecAddress,
-  Fr,
-  type FunctionSelector,
-  type GlobalVariables,
-  MAX_L2_GAS_PER_ENQUEUED_CALL,
-} from '@aztec/circuits.js';
+import { type AztecAddress, Fr, type GlobalVariables, MAX_L2_GAS_PER_ENQUEUED_CALL } from '@aztec/circuits.js';
 import { type Logger, createLogger } from '@aztec/foundation/log';
 
 import { strict as assert } from 'assert';
@@ -49,24 +43,35 @@ export class AvmSimulator {
   private tallyPrintFunction = () => {};
   private tallyInstructionFunction = (_a: number, _b: string, _c: Gas) => {};
 
+  // Test Purposes only: Logger will not have the proper function name. Use this constructor for testing purposes
+  // only. Otherwise, use build() below.
   constructor(private context: AvmContext, private instructionSet: InstructionSet = INSTRUCTION_SET()) {
     assert(
       context.machineState.gasLeft.l2Gas <= MAX_L2_GAS_PER_ENQUEUED_CALL,
       `Cannot allocate more than ${MAX_L2_GAS_PER_ENQUEUED_CALL} to the AVM for execution of an enqueued call`,
     );
-    this.log = createLogger(`simulator:avm:core(f:${context.environment.functionSelector.toString()})`);
+    this.log = createLogger(`simulator:avm(calldata[0]: ${context.environment.calldata[0]})`);
     // TODO(palla/log): Should tallies be printed on debug, or only on trace?
     if (this.log.isLevelEnabled('debug')) {
       this.tallyPrintFunction = this.printOpcodeTallies;
       this.tallyInstructionFunction = this.tallyInstruction;
     }
   }
 
-  public static create(
+  // Factory to have a proper function name in the logger. Retrieving the name is asynchronous and
+  // cannot be done as part of the constructor.
+  public static async build(context: AvmContext): Promise<AvmSimulator> {
+    const simulator = new AvmSimulator(context);
+    const fnName = await context.persistableState.getPublicFunctionDebugName(context.environment);
+    simulator.log = createLogger(`simulator:avm(f:${fnName})`);
+
+    return simulator;
+  }
+
+  public static async create(
     stateManager: AvmPersistableStateManager,
     address: AztecAddress,
     sender: AztecAddress,
-    functionSelector: FunctionSelector, // may be temporary (#7224)
     transactionFee: Fr,
     globals: GlobalVariables,
     isStaticCall: boolean,
@@ -76,7 +81,6 @@ export class AvmSimulator {
     const avmExecutionEnv = new AvmExecutionEnvironment(
       address,
       sender,
-      functionSelector,
       /*contractCallDepth=*/ Fr.zero(),
       transactionFee,
       globals,
@@ -86,8 +90,7 @@ export class AvmSimulator {
 
     const avmMachineState = new AvmMachineState(allocatedGas);
     const avmContext = new AvmContext(stateManager, avmExecutionEnv, avmMachineState);
-    const instructionSet = INSTRUCTION_SET();
-    return new AvmSimulator(avmContext, instructionSet);
+    return await AvmSimulator.build(avmContext);
   }
 
   /**
@@ -98,11 +101,12 @@ export class AvmSimulator {
     if (!bytecode) {
       // revert, consuming all gas
       const message = `No bytecode found at: ${this.context.environment.address}. Reverting...`;
+      const fnName = await this.context.persistableState.getPublicFunctionDebugName(this.context.environment);
       const revertReason = new AvmRevertReason(
         message,
         /*failingFunction=*/ {
           contractAddress: this.context.environment.address,
-          functionSelector: this.context.environment.functionSelector,
+          functionName: fnName,
         },
         /*noirCallStack=*/ [],
       );
@@ -176,7 +180,7 @@ export class AvmSimulator {
 
       const output = machineState.getOutput();
       const reverted = machineState.getReverted();
-      const revertReason = reverted ? revertReasonFromExplicitRevert(output, this.context) : undefined;
+      const revertReason = reverted ? await revertReasonFromExplicitRevert(output, this.context) : undefined;
       const results = new AvmContractCallResult(reverted, output, machineState.gasLeft, revertReason);
       this.log.debug(`Context execution results: ${results.toString()}`);
 
@@ -190,7 +194,7 @@ export class AvmSimulator {
         throw err;
       }
 
-      const revertReason = revertReasonFromExceptionalHalt(err, this.context);
+      const revertReason = await revertReasonFromExceptionalHalt(err, this.context);
       // Note: "exceptional halts" cannot return data, hence [].
       const results = new AvmContractCallResult(/*reverted=*/ true, /*output=*/ [], machineState.gasLeft, revertReason);
       this.log.debug(`Context execution results: ${results.toString()}`);

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

@@ -1,5 +1,5 @@
 import { type FailingFunction, type NoirCallStack } from '@aztec/circuit-types';
-import { type AztecAddress, Fr, FunctionSelector, PUBLIC_DISPATCH_SELECTOR } from '@aztec/circuits.js';
+import { type AztecAddress, type Fr } from '@aztec/circuits.js';
 
 import { ExecutionError } from '../common/errors.js';
 import { type AvmContext } from './avm_context.js';
@@ -138,16 +138,9 @@ export class AvmRevertReason extends ExecutionError {
   }
 }
 
-function createRevertReason(message: string, revertData: Fr[], context: AvmContext): AvmRevertReason {
-  // TODO(https://github.com/AztecProtocol/aztec-packages/issues/8985): Properly fix this.
-  // If the function selector is the public dispatch selector, we need to extract the actual function selector from the calldata.
-  // We should remove this because the AVM (or public protocol) shouldn't be aware of the public dispatch calling convention.
-  let functionSelector = context.environment.functionSelector;
+async function createRevertReason(message: string, revertData: Fr[], context: AvmContext): Promise<AvmRevertReason> {
   // We drop the returnPc information.
   const internalCallStack = context.machineState.internalCallStack.map(entry => entry.callPc);
-  if (functionSelector.toField().equals(new Fr(PUBLIC_DISPATCH_SELECTOR)) && context.environment.calldata.length > 0) {
-    functionSelector = FunctionSelector.fromField(context.environment.calldata[0]);
-  }
 
   // If we are reverting due to the same error that we have been tracking, we use the nested error as the cause.
   let nestedError = undefined;
@@ -160,11 +153,13 @@ function createRevertReason(message: string, revertData: Fr[], context: AvmConte
     message = context.machineState.collectedRevertInfo.recursiveRevertReason.message;
   }
 
+  const fnName = await context.persistableState.getPublicFunctionDebugName(context.environment);
+
   return new AvmRevertReason(
     message,
     /*failingFunction=*/ {
       contractAddress: context.environment.address,
-      functionSelector: functionSelector,
+      functionName: fnName,
     },
     /*noirCallStack=*/ [...internalCallStack, context.machineState.pc].map(pc => `0.${pc}`),
     /*options=*/ { cause: nestedError },
@@ -177,8 +172,11 @@ function createRevertReason(message: string, revertData: Fr[], context: AvmConte
  * @param haltingError - the lower-level error causing the exceptional halt
  * @param context - the context of the AVM execution used to extract the failingFunction and noirCallStack
  */
-export function revertReasonFromExceptionalHalt(haltingError: AvmExecutionError, context: AvmContext): AvmRevertReason {
-  return createRevertReason(haltingError.message, [], context);
+export async function revertReasonFromExceptionalHalt(
+  haltingError: AvmExecutionError,
+  context: AvmContext,
+): Promise<AvmRevertReason> {
+  return await createRevertReason(haltingError.message, [], context);
 }
 
 /**
@@ -187,6 +185,6 @@ export function revertReasonFromExceptionalHalt(haltingError: AvmExecutionError,
  * @param revertData - output data of the explicit REVERT instruction
  * @param context - the context of the AVM execution used to extract the failingFunction and noirCallStack
  */
-export function revertReasonFromExplicitRevert(revertData: Fr[], context: AvmContext): AvmRevertReason {
-  return createRevertReason('Assertion failed: ', revertData, context);
+export async function revertReasonFromExplicitRevert(revertData: Fr[], context: AvmContext): Promise<AvmRevertReason> {
+  return await createRevertReason('Assertion failed: ', revertData, context);
 }

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

@@ -64,7 +64,6 @@ export function initExecutionEnvironment(overrides?: Partial<AvmExecutionEnviron
   return new AvmExecutionEnvironment(
     overrides?.address ?? AztecAddress.zero(),
     overrides?.sender ?? AztecAddress.zero(),
-    overrides?.functionSelector ?? FunctionSelector.empty(),
     overrides?.contractCallDepth ?? Fr.zero(),
     overrides?.transactionFee ?? Fr.zero(),
     overrides?.globals ?? GlobalVariables.empty(),

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

@@ -687,7 +687,6 @@ export class AvmPersistableStateManager {
     const functionName = await getPublicFunctionDebugName(
       this.worldStateDB,
       nestedEnvironment.address,
-      nestedEnvironment.functionSelector,
       nestedEnvironment.calldata,
     );
 
@@ -706,6 +705,10 @@ export class AvmPersistableStateManager {
   public traceEnqueuedCall(publicCallRequest: PublicCallRequest, calldata: Fr[], reverted: boolean) {
     this.trace.traceEnqueuedCall(publicCallRequest, calldata, reverted);
   }
+
+  public async getPublicFunctionDebugName(avmEnvironment: AvmExecutionEnvironment): Promise<string> {
+    return await getPublicFunctionDebugName(this.worldStateDB, avmEnvironment.address, avmEnvironment.calldata);
+  }
 }
 
 function contractAddressIsCanonical(contractAddress: AztecAddress): boolean {