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

@@ -107,6 +107,7 @@ export function resolveOpcodeLocations(
 }
 
 export function resolveAssertionMessage(errorPayload: RawAssertionPayload, abi: FunctionAbi): string | undefined {
+  console.log('resolveAssertionMessage', errorPayload, abi.errorTypes);
   const decoded = abiDecodeError(
     { parameters: [], error_types: abi.errorTypes, return_type: null }, // eslint-disable-line camelcase
     errorPayload,
@@ -119,6 +120,7 @@ export function resolveAssertionMessage(errorPayload: RawAssertionPayload, abi:
   }
 }
 
+// Move to common?
 export function resolveAssertionMessageFromRevertData(revertData: Fr[], abi: FunctionAbi): string | undefined {
   if (revertData.length == 0) {
     return undefined;

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,11 @@ export type InitialAvmMachineState = {
   daGasLeft: number;
 };
 
+type RevertInfo = {
+  revertDataRepresentative: Fr[];
+  recursiveRevertReason: AvmRevertReason;
+};
+
 type CallStackEntry = {
   callPc: number;
   returnPc: number;
@@ -30,6 +35,8 @@ export class AvmMachineState {
   public nextPc: number = 0;
   /** return/revertdata of the last nested call. */
   public nestedReturndata: Fr[] = [];
+  // DOCUMENT
+  public collectedRevertInfo: RevertInfo | 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

@@ -11,6 +11,7 @@ import { type Fieldable } from '@aztec/foundation/serialize';
 import { randomInt } from 'crypto';
 import { mock } from 'jest-mock-extended';
 
+import { resolveAssertionMessageFromRevertData } from '../acvm/acvm.js';
 import { PublicEnqueuedCallSideEffectTrace } from '../public/enqueued_call_side_effect_trace.js';
 import { type WorldStateDB } from '../public/public_db_sources.js';
 import { type PublicSideEffectTraceInterface } from '../public/side_effect_trace_interface.js';
@@ -20,6 +21,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 +323,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 +931,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,13 +949,10 @@ 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.output).toHaveLength(1);
+        expect(resolveAssertionMessageFromRevertData(results.output, artifact)).toMatch('Not enough L2GAS gas left');
 
         // Nested call should NOT have been made and therefore should not be traced
         expect(trace.traceNestedCall).toHaveBeenCalledTimes(0);

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

@@ -1,4 +1,4 @@
-import { MAX_L2_GAS_PER_ENQUEUED_CALL } from '@aztec/circuits.js';
+import { Fr, MAX_L2_GAS_PER_ENQUEUED_CALL } from '@aztec/circuits.js';
 import { type DebugLogger, createDebugLogger } from '@aztec/foundation/log';
 
 import { strict as assert } from 'assert';
@@ -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();

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

@@ -21,6 +21,13 @@ export class NoBytecodeForContractError extends AvmExecutionError {
   }
 }
 
+export class ArithmeticError extends AvmExecutionError {
+  constructor(message: string) {
+    super(message);
+    this.name = 'ArithmeticError';
+  }
+}
+
 /**
  * Error is thrown when the program counter goes to an invalid location.
  * There is no instruction at the provided pc
@@ -78,18 +85,6 @@ export class StaticCallAlterationError extends InstructionExecutionError {
   }
 }
 
-/**
- * Error thrown to propagate a nested call's revert.
- * @param message - the error's message
- * @param nestedError - the revert reason of the nested call
- */
-export class RethrownError extends AvmExecutionError {
-  constructor(message: string, public nestedError: AvmRevertReason, public revertData: Fr[]) {
-    super(message);
-    this.name = 'RethrownError';
-  }
-}
-
 /**
  * Meaningfully named alias for ExecutionError when used in the context of the AVM.
  * Maintains a recursive structure reflecting the AVM's external callstack/errorstack, where
@@ -106,9 +101,8 @@ export class AvmRevertReason extends ExecutionError {
  *
  * @param message - the error message
  * @param context - the context of the AVM execution used to extract the failingFunction and noirCallStack
- * @param nestedError - the error that caused this one (if this is not the root-cause itself)
  */
-function createRevertReason(message: string, context: AvmContext, nestedError?: AvmRevertReason): AvmRevertReason {
+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.
@@ -118,6 +112,17 @@ function createRevertReason(message: string, context: AvmContext, nestedError?:
   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;
+  const revertDataEquals = (a: Fr[], b: Fr[]) => a.length === b.length && a.every((v, i) => v.equals(b[i]));
+  if (
+    context.machineState.collectedRevertInfo &&
+    revertDataEquals(context.machineState.collectedRevertInfo.revertDataRepresentative, revertData)
+  ) {
+    nestedError = context.machineState.collectedRevertInfo.recursiveRevertReason;
+  }
+
   return new AvmRevertReason(
     message,
     /*failingFunction=*/ {
@@ -137,18 +142,7 @@ function createRevertReason(message: string, context: AvmContext, nestedError?:
  * @param context - the context of the AVM execution used to extract the failingFunction and noirCallStack
  */
 export function revertReasonFromExceptionalHalt(haltingError: AvmExecutionError, context: AvmContext): AvmRevertReason {
-  // A RethrownError has a nested/child AvmRevertReason
-  const nestedError = haltingError instanceof RethrownError ? haltingError.nestedError : undefined;
-  return createRevertReason(haltingError.message, context, nestedError);
-}
-
-/**
- * Extracts revert data from an exceptional halt. Currently this is only used to manually bubble up revertadata.
- * @param haltingError - the lower-level error causing the exceptional halt
- * @returns the revert data for the exceptional halt
- */
-export function revertDataFromExceptionalHalt(haltingError: AvmExecutionError): Fr[] {
-  return haltingError instanceof RethrownError ? haltingError.revertData : [];
+  return createRevertReason(haltingError.message, [], context);
 }
 
 /**
@@ -158,26 +152,5 @@ export function revertDataFromExceptionalHalt(haltingError: AvmExecutionError):
  * @param context - the context of the AVM execution used to extract the failingFunction and noirCallStack
  */
 export function revertReasonFromExplicitRevert(revertData: Fr[], context: AvmContext): AvmRevertReason {
-  const revertMessage = decodeRevertDataAsMessage(revertData);
-  return createRevertReason(revertMessage, context);
-}
-
-/**
- * Interpret revert data as a message string.
- *
- * @param revertData - output data of an explicit REVERT instruction
- */
-export function decodeRevertDataAsMessage(revertData: Fr[]): string {
-  if (revertData.length === 0) {
-    return 'Assertion failed';
-  } else {
-    try {
-      // We remove the first element which is the 'error selector'.
-      const revertOutput = revertData.slice(1);
-      // Try to interpret the output as a text string.
-      return 'Assertion failed: ' + String.fromCharCode(...revertOutput.map(fr => fr.toNumber()));
-    } catch (e) {
-      return 'Assertion failed: <cannot interpret as string>';
-    }
-  }
+  return createRevertReason('Assertion failed: ', revertData, context);
 }

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

@@ -1,6 +1,6 @@
 import { isNoirCallStackUnresolved } from '@aztec/circuit-types';
 import { GasFees, GlobalVariables, MAX_L2_GAS_PER_ENQUEUED_CALL } from '@aztec/circuits.js';
-import { FunctionSelector } from '@aztec/foundation/abi';
+import { type FunctionArtifact, FunctionSelector } from '@aztec/foundation/abi';
 import { AztecAddress } from '@aztec/foundation/aztec-address';
 import { EthAddress } from '@aztec/foundation/eth-address';
 import { Fr } from '@aztec/foundation/fields';
@@ -133,6 +133,15 @@ export function getAvmTestContractBytecode(functionName: string): Buffer {
   return artifact.bytecode;
 }
 
+export function getAvmTestContractArtifact(functionName: string): FunctionArtifact {
+  const artifact = AvmTestContractArtifact.functions.find(f => f.name === functionName)!;
+  assert(
+    !!artifact?.bytecode,
+    `No bytecode found for function ${functionName}. Try re-running bootstrap.sh on the repository root.`,
+  );
+  return artifact;
+}
+
 export function resolveAvmTestContractAssertionMessage(
   functionName: string,
   revertReason: AvmRevertReason,

yarn-project/simulator/src/avm/opcodes/arithmetic.ts

@@ -1,5 +1,6 @@
 import type { AvmContext } from '../avm_context.js';
 import { type Field, type MemoryValue } from '../avm_memory_types.js';
+import { ArithmeticError } from '../errors.js';
 import { Opcode } from '../serialization/instruction_serialization.js';
 import { Addressing } from './addressing_mode.js';
 import { ThreeOperandInstruction } from './instruction_impl.js';
@@ -58,11 +59,14 @@ export class Div extends ThreeOperandArithmeticInstruction {
   static readonly opcode = Opcode.DIV_8; // FIXME: needed for gas.
 
   protected compute(a: MemoryValue, b: MemoryValue): MemoryValue {
+    if (b.toBigInt() === 0n) {
+      throw new ArithmeticError('Division by zero');
+    }
+
     return a.div(b);
   }
 }
 
-// TODO: This class now temporarily has a tag, until all tags are removed.
 export class FieldDiv extends ThreeOperandArithmeticInstruction {
   static type: string = 'FDIV';
   static readonly opcode = Opcode.FDIV_8; // FIXME: needed for gas.