feat(avm): remove rethrowable reverts hack

noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr

@@ -229,6 +229,21 @@ contract AvmTest {
         helper_with_failed_assertion()
     }
 
+    #[public]
+    fn external_call_to_assertion_failure() {
+        AvmTest::at(context.this_address()).assertion_failure().call(&mut context);
+    }
+
+    #[public]
+    fn divide_by_zero() -> u8 {
+        1 / 0
+    }
+
+    #[public]
+    fn external_call_to_divide_by_zero() {
+        AvmTest::at(context.this_address()).divide_by_zero().call(&mut context);
+    }
+
     #[public]
     fn debug_logging() {
         dep::aztec::oracle::debug_log::debug_log("just text");

yarn-project/end-to-end/src/e2e_avm_simulator.test.ts

@@ -33,15 +33,32 @@ describe('e2e_avm_simulator', () => {
     });
 
     describe('Assertions', () => {
-      it('PXE processes failed assertions and fills in the error message with the expression', async () => {
-        await expect(avmContract.methods.assertion_failure().simulate()).rejects.toThrow(
-          "Assertion failed: This assertion should fail! 'not_true == true'",
-        );
-      });
-      it('PXE processes failed assertions and fills in the error message with the expression (even complex ones)', async () => {
-        await expect(avmContract.methods.assert_nullifier_exists(123).simulate()).rejects.toThrow(
-          "Assertion failed: Nullifier doesn't exist! 'context.nullifier_exists(nullifier, context.this_address())'",
-        );
+      describe('Not nested', () => {
+        it('PXE processes user code assertions and recovers message', async () => {
+          await expect(avmContract.methods.assertion_failure().simulate()).rejects.toThrow(
+            "Assertion failed: This assertion should fail! 'not_true == true'",
+          );
+        });
+        it('PXE processes user code assertions and recovers message (complex)', async () => {
+          await expect(avmContract.methods.assert_nullifier_exists(123).simulate()).rejects.toThrow(
+            "Assertion failed: Nullifier doesn't exist! 'context.nullifier_exists(nullifier, context.this_address())'",
+          );
+        });
+        it('PXE processes intrinsic assertions and recovers message', async () => {
+          await expect(avmContract.methods.divide_by_zero().simulate()).rejects.toThrow('Division by zero');
+        });
+      });
+      describe('Nested', () => {
+        it('PXE processes user code assertions and recovers message', async () => {
+          await expect(avmContract.methods.external_call_to_assertion_failure().simulate()).rejects.toThrow(
+            "Assertion failed: This assertion should fail! 'not_true == true'",
+          );
+        });
+        it('PXE processes intrinsic assertions and recovers message', async () => {
+          await expect(avmContract.methods.external_call_to_divide_by_zero().simulate()).rejects.toThrow(
+            'Division by zero',
+          );
+        });
       });
     });
 

yarn-project/simulator/src/acvm/acvm.ts

@@ -1,18 +1,15 @@
-import { type NoirCallStack, type SourceCodeLocation } from '@aztec/circuit-types';
-import { type Fr } from '@aztec/circuits.js';
-import type { BrilligFunctionId, FunctionAbi, FunctionDebugMetadata, OpcodeLocation } from '@aztec/foundation/abi';
+import { type NoirCallStack } from '@aztec/circuit-types';
+import type { FunctionDebugMetadata } from '@aztec/foundation/abi';
 import { createDebugLogger } from '@aztec/foundation/log';
 
 import {
   type ExecutionError,
   type ForeignCallInput,
   type ForeignCallOutput,
-  type RawAssertionPayload,
   executeCircuitWithReturnWitness,
 } from '@noir-lang/acvm_js';
-import { abiDecodeError } from '@noir-lang/noirc_abi';
 
-import { traverseCauseChain } from '../common/errors.js';
+import { resolveOpcodeLocations, traverseCauseChain } from '../common/errors.js';
 import { type ACVMWitness } from './acvm_types.js';
 import { type ORACLE_NAMES } from './oracle/index.js';
 
@@ -37,112 +34,6 @@ export interface ACIRExecutionResult {
   returnWitness: ACVMWitness;
 }
 
-/**
- * Extracts a brillig location from an opcode location.
- * @param opcodeLocation - The opcode location to extract from. It should be in the format `acirLocation.brilligLocation` or `acirLocation`.
- * @returns The brillig location if the opcode location contains one.
- */
-function extractBrilligLocation(opcodeLocation: string): string | undefined {
-  const splitted = opcodeLocation.split('.');
-  if (splitted.length === 2) {
-    return splitted[1];
-  }
-  return undefined;
-}
-
-/**
- * Extracts the call stack from the location of a failing opcode and the debug metadata.
- * One opcode can point to multiple calls due to inlining.
- */
-function getSourceCodeLocationsFromOpcodeLocation(
-  opcodeLocation: string,
-  debug: FunctionDebugMetadata,
-  brilligFunctionId?: BrilligFunctionId,
-): SourceCodeLocation[] {
-  const { debugSymbols, files } = debug;
-
-  let callStack = debugSymbols.locations[opcodeLocation] || [];
-  if (callStack.length === 0) {
-    const brilligLocation = extractBrilligLocation(opcodeLocation);
-    if (brilligFunctionId !== undefined && brilligLocation !== undefined) {
-      callStack = debugSymbols.brillig_locations[brilligFunctionId][brilligLocation] || [];
-    }
-  }
-  return callStack.map(call => {
-    const { file: fileId, span } = call;
-
-    const { path, source } = files[fileId];
-
-    const locationText = source.substring(span.start, span.end);
-    const precedingText = source.substring(0, span.start);
-    const previousLines = precedingText.split('\n');
-    // Lines and columns in stacks are one indexed.
-    const line = previousLines.length;
-    const column = previousLines[previousLines.length - 1].length + 1;
-
-    return {
-      filePath: path,
-      line,
-      column,
-      fileSource: source,
-      locationText,
-    };
-  });
-}
-
-/**
- * Extracts the source code locations for an array of opcode locations
- * @param opcodeLocations - The opcode locations that caused the error.
- * @param debug - The debug metadata of the function.
- * @returns The source code locations.
- */
-export function resolveOpcodeLocations(
-  opcodeLocations: OpcodeLocation[],
-  debug: FunctionDebugMetadata,
-  brilligFunctionId?: BrilligFunctionId,
-): SourceCodeLocation[] {
-  return opcodeLocations.flatMap(opcodeLocation =>
-    getSourceCodeLocationsFromOpcodeLocation(opcodeLocation, debug, brilligFunctionId),
-  );
-}
-
-export function resolveAssertionMessage(errorPayload: RawAssertionPayload, abi: FunctionAbi): string | undefined {
-  const decoded = abiDecodeError(
-    { parameters: [], error_types: abi.errorTypes, return_type: null }, // eslint-disable-line camelcase
-    errorPayload,
-  );
-
-  if (typeof decoded === 'string') {
-    return decoded;
-  } else {
-    return JSON.stringify(decoded);
-  }
-}
-
-export function resolveAssertionMessageFromRevertData(revertData: Fr[], abi: FunctionAbi): string | undefined {
-  if (revertData.length == 0) {
-    return undefined;
-  }
-
-  const [errorSelector, ...errorData] = revertData;
-
-  return resolveAssertionMessage(
-    {
-      selector: errorSelector.toBigInt().toString(),
-      data: errorData.map(f => f.toString()),
-    },
-    abi,
-  );
-}
-
-export function resolveAssertionMessageFromError(err: Error, abi: FunctionAbi): string {
-  if (typeof err === 'object' && err !== null && 'rawAssertionPayload' in err && err.rawAssertionPayload) {
-    return `Assertion failed: ${resolveAssertionMessage(err.rawAssertionPayload as RawAssertionPayload, abi)}`;
-  } else {
-    return err.message;
-  }
-}
-
 /**
  * The function call that executes an ACIR.
  */

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

@@ -2,7 +2,7 @@ import { type Fr } from '@aztec/circuits.js';
 
 import { GAS_DIMENSIONS, type Gas } from './avm_gas.js';
 import { TaggedMemory } from './avm_memory_types.js';
-import { OutOfGasError } from './errors.js';
+import { type AvmRevertReason, OutOfGasError } from './errors.js';
 
 /**
  * A few fields of machine state are initialized from AVM session inputs or call instruction arguments
@@ -12,6 +12,16 @@ export type InitialAvmMachineState = {
   daGasLeft: number;
 };
 
+/**
+ * Used to track the call stack and revert data of nested calls.
+ * This is used to provide a more detailed revert reason when a contract call reverts.
+ * It is only a heuristic and may not always provide the correct revert reason.
+ */
+type TrackedRevertInfo = {
+  revertDataRepresentative: Fr[];
+  recursiveRevertReason: AvmRevertReason;
+};
+
 type CallStackEntry = {
   callPc: number;
   returnPc: number;
@@ -30,6 +40,12 @@ export class AvmMachineState {
   public nextPc: number = 0;
   /** return/revertdata of the last nested call. */
   public nestedReturndata: Fr[] = [];
+  /**
+   * Used to track the call stack and revert data of nested calls.
+   * This is used to provide a more detailed revert reason when a contract call reverts.
+   * It is only a heuristic and may not always provide the correct revert reason.
+   */
+  public collectedRevertInfo: TrackedRevertInfo | undefined;
 
   /**
    * On INTERNALCALL, internal call stack is pushed to with the current pc and the return pc.

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

@@ -20,6 +20,7 @@ import { type MemoryValue, TypeTag, type Uint8, type Uint64 } from './avm_memory
 import { AvmSimulator } from './avm_simulator.js';
 import { isAvmBytecode, markBytecodeAsAvm } from './bytecode_utils.js';
 import {
+  getAvmTestContractArtifact,
   getAvmTestContractBytecode,
   initContext,
   initExecutionEnvironment,
@@ -321,10 +322,10 @@ describe('AVM simulator: transpiled Noir contracts', () => {
 
     expect(results.reverted).toBe(true);
     expect(results.revertReason).toBeDefined();
+    expect(results.output).toHaveLength(1); // Error selector for static string error
     expect(
       resolveAvmTestContractAssertionMessage('assert_nullifier_exists', results.revertReason!, results.output),
     ).toMatch("Nullifier doesn't exist!");
-    expect(results.output).toHaveLength(1); // Error selector for static string error
   });
 
   describe.each([
@@ -929,14 +930,16 @@ describe('AVM simulator: transpiled Noir contracts', () => {
 
       it(`Nested call with not enough gas (expect failure)`, async () => {
         const gas = [/*l2=*/ 5, /*da=*/ 10000].map(g => new Fr(g));
-        const calldata: Fr[] = [value0, value1, ...gas];
+        const targetFunctionSelector = FunctionSelector.fromSignature(
+          'nested_call_to_add_with_gas(Field,Field,Field,Field)',
+        );
+        const calldata: Fr[] = [targetFunctionSelector.toField(), value0, value1, ...gas];
         const context = createContext(calldata);
-        const callBytecode = getAvmTestContractBytecode('nested_call_to_add_with_gas');
-        const nestedBytecode = getAvmTestContractBytecode('public_dispatch');
-        mockGetBytecode(worldStateDB, nestedBytecode);
+        const artifact = getAvmTestContractArtifact('public_dispatch');
+        mockGetBytecode(worldStateDB, artifact.bytecode);
 
         const contractClass = makeContractClassPublic(0, {
-          bytecode: nestedBytecode,
+          bytecode: artifact.bytecode,
           selector: FunctionSelector.random(),
         });
         mockGetContractClass(worldStateDB, contractClass);
@@ -945,16 +948,12 @@ describe('AVM simulator: transpiled Noir contracts', () => {
 
         mockTraceFork(trace);
 
-        const results = await new AvmSimulator(context).executeBytecode(callBytecode);
-        // TODO(7141): change this once we don't force rethrowing of exceptions.
-        // Outer frame should not revert, but inner should, so the forwarded return value is 0
-        // expect(results.revertReason).toBeUndefined();
-        // expect(results.reverted).toBe(false);
+        const results = await new AvmSimulator(context).executeBytecode(artifact.bytecode);
         expect(results.reverted).toBe(true);
-        expect(results.revertReason?.message).toEqual('Not enough L2GAS gas left');
+        expect(results.revertReason?.message).toMatch('Not enough L2GAS gas left');
 
-        // Nested call should NOT have been made and therefore should not be traced
-        expect(trace.traceNestedCall).toHaveBeenCalledTimes(0);
+        // Nested call should have been made (and failed).
+        expect(trace.traceNestedCall).toHaveBeenCalledTimes(1);
       });
 
       it(`Nested static call which modifies storage (expect failure)`, async () => {
@@ -971,7 +970,8 @@ describe('AVM simulator: transpiled Noir contracts', () => {
         const contractInstance = makeContractInstanceFromClassId(contractClass.id);
         mockGetContractInstance(worldStateDB, contractInstance);
 
-        mockTraceFork(trace);
+        const nestedTrace = mock<PublicSideEffectTraceInterface>();
+        mockTraceFork(trace, nestedTrace);
 
         const results = await new AvmSimulator(context).executeBytecode(callBytecode);
 
@@ -980,9 +980,7 @@ describe('AVM simulator: transpiled Noir contracts', () => {
           'Static call cannot update the state, emit L2->L1 messages or generate logs',
         );
 
-        // TODO(7141): external call doesn't recover from nested exception until
-        // we support recoverability of reverts (here and in kernel)
-        //expectTracedNestedCall(context.environment, results, nestedTrace, /*isStaticCall=*/true);
+        expectTracedNestedCall(context.environment, nestedTrace, /*isStaticCall=*/ true);
 
         // Nested call should NOT have been able to write storage
         expect(trace.tracePublicStorageWrite).toHaveBeenCalledTimes(0);

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

@@ -12,7 +12,6 @@ import {
   AvmExecutionError,
   InvalidProgramCounterError,
   NoBytecodeForContractError,
-  revertDataFromExceptionalHalt,
   revertReasonFromExceptionalHalt,
   revertReasonFromExplicitRevert,
 } from './errors.js';
@@ -134,12 +133,8 @@ export class AvmSimulator {
       }
 
       const revertReason = revertReasonFromExceptionalHalt(err, this.context);
-      // Note: "exceptional halts" cannot return data, hence []
-      const results = new AvmContractCallResult(
-        /*reverted=*/ true,
-        /*output=*/ revertDataFromExceptionalHalt(err),
-        revertReason,
-      );
+      // Note: "exceptional halts" cannot return data, hence [].
+      const results = new AvmContractCallResult(/*reverted=*/ true, /*output=*/ [], revertReason);
       this.log.debug(`Context execution results: ${results.toString()}`);
 
       this.printOpcodeTallies();