From 25047dd67a26a1fe7288520c4190e857bf87a4e6 Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Thu, 9 May 2024 01:19:17 +0000 Subject: [PATCH] feat(avm-simulator): error stack tracking in AVM to match ACVM/ACIR-SIM --- .../contracts/avm_test_contract/src/main.nr | 37 +++++-- .../end-to-end/src/e2e_avm_simulator.test.ts | 9 +- .../simulator/src/avm/avm_machine_state.ts | 45 +++------ .../src/avm/avm_message_call_result.ts | 17 +--- .../simulator/src/avm/avm_simulator.test.ts | 2 +- .../simulator/src/avm/avm_simulator.ts | 34 ++++--- yarn-project/simulator/src/avm/errors.ts | 98 ++++++++++++++++++- .../src/avm/opcodes/external_calls.test.ts | 17 ++-- .../src/avm/opcodes/external_calls.ts | 14 ++- yarn-project/simulator/src/common/errors.ts | 5 + .../src/public/transitional_adaptors.ts | 36 +------ 11 files changed, 190 insertions(+), 124 deletions(-) diff --git a/noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr b/noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr index a2f148e4e23..cbff33dd760 100644 --- a/noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr @@ -140,6 +140,10 @@ contract AvmTest { a + b } + /************************************************************************ + * Misc + ************************************************************************/ + #[aztec(public-vm)] fn u128_addition_overflow() -> U128 { let max_u128: U128 = U128::from_hex("0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"); @@ -153,10 +157,24 @@ contract AvmTest { U128::from_integer(should_overflow) } - #[aztec(private)] - fn enqueue_public_from_private() { - AvmTest::at(context.this_address()).set_opcode_u8().static_enqueue(&mut context); - AvmTest::at(context.this_address()).set_read_storage_single(5).enqueue(&mut context); + #[aztec(public-vm)] + fn to_radix_le(input: Field) -> [u8; 10] { + let result: [u8] = input.to_le_radix(/*base=*/ 2, /*limbs=*/ 10); + result.as_array() + } + + // Helper functions to demonstrate an internal call stack in error messages + fn inner_helper_with_failed_assertion() { + let not_true = false; + assert(not_true == true, "This assertion should fail!"); + } + fn helper_with_failed_assertion() { + inner_helper_with_failed_assertion(); + } + + #[aztec(public-vm)] + fn assertion_failure() { + helper_with_failed_assertion() } /************************************************************************ @@ -341,9 +359,12 @@ contract AvmTest { context.message_portal(recipient, content) } - #[aztec(public-vm)] - fn to_radix_le(input: Field) -> [u8; 10] { - let result: [u8] = input.to_le_radix(/*base=*/ 2, /*limbs=*/ 10); - result.as_array() + /** + * Enqueue a public call from private + */ + #[aztec(private)] + fn enqueue_public_from_private() { + AvmTest::at(context.this_address()).set_opcode_u8().static_enqueue(&mut context); + AvmTest::at(context.this_address()).set_read_storage_single(5).enqueue(&mut context); } } diff --git a/yarn-project/end-to-end/src/e2e_avm_simulator.test.ts b/yarn-project/end-to-end/src/e2e_avm_simulator.test.ts index 4869cc90162..bd9a26ebba1 100644 --- a/yarn-project/end-to-end/src/e2e_avm_simulator.test.ts +++ b/yarn-project/end-to-end/src/e2e_avm_simulator.test.ts @@ -34,9 +34,14 @@ describe('e2e_avm_simulator', () => { }); describe('Assertions', () => { - it('Processes assertions in the PXE', async () => { + 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!", + "Assertion failed: Nullifier doesn't exist! 'context.nullifier_exists(nullifier, context.this_address())'", ); }); }); diff --git a/yarn-project/simulator/src/avm/avm_machine_state.ts b/yarn-project/simulator/src/avm/avm_machine_state.ts index 0af30ddefb3..740f607bafb 100644 --- a/yarn-project/simulator/src/avm/avm_machine_state.ts +++ b/yarn-project/simulator/src/avm/avm_machine_state.ts @@ -2,7 +2,6 @@ import { type Fr } from '@aztec/circuits.js'; import { type Gas, GasDimensions } from './avm_gas.js'; import { TaggedMemory } from './avm_memory_types.js'; -import { AvmContractCallResults } from './avm_message_call_result.js'; import { OutOfGasError } from './errors.js'; /** @@ -36,7 +35,7 @@ export class AvmMachineState { * Signals that execution should end. * AvmContext execution continues executing instructions until the machine state signals "halted" */ - public halted: boolean = false; + private halted: boolean = false; /** Signals that execution has reverted normally (this does not cover exceptional halts) */ private reverted: boolean = false; /** Output data must NOT be modified once it is set */ @@ -118,40 +117,24 @@ export class AvmMachineState { this.output = output; } + public getHalted(): boolean { + return this.halted; + } + + public getReverted(): boolean { + return this.reverted; + } + + public getOutput(): Fr[] { + return this.output; + } + /** * Flag an exceptional halt. Clears gas left and sets the reverted flag. No output data. */ - protected exceptionalHalt() { + private exceptionalHalt() { GasDimensions.forEach(dimension => (this[`${dimension}Left`] = 0)); this.reverted = true; this.halted = true; } - - /** - * Get a summary of execution results for a halted machine state - * @returns summary of execution results - */ - public getResults(): AvmContractCallResults { - if (!this.halted) { - throw new Error('Execution results are not ready! Execution is ongoing.'); - } - let revertReason = undefined; - if (this.reverted) { - if (this.output.length === 0) { - revertReason = new Error('Assertion failed.'); - } else { - try { - // We remove the first element which is the 'error selector'. - const revertOutput = this.output.slice(1); - // Try to interpret the output as a text string. - revertReason = new Error( - 'Assertion failed: ' + String.fromCharCode(...revertOutput.map(fr => fr.toNumber())), - ); - } catch (e) { - revertReason = new Error('Assertion failed: '); - } - } - } - return new AvmContractCallResults(this.reverted, this.output, revertReason); - } } diff --git a/yarn-project/simulator/src/avm/avm_message_call_result.ts b/yarn-project/simulator/src/avm/avm_message_call_result.ts index 9585ff75448..49e5355b3d2 100644 --- a/yarn-project/simulator/src/avm/avm_message_call_result.ts +++ b/yarn-project/simulator/src/avm/avm_message_call_result.ts @@ -1,24 +1,13 @@ import { type Fr } from '@aztec/foundation/fields'; +import { type AvmRevertReason } from './errors.js'; + /** * Results of an contract call's execution in the AVM. */ export class AvmContractCallResults { - public readonly reverted: boolean; - public readonly output: Fr[]; - - /** For exceptional halts */ - public readonly revertReason: Error | undefined; - - constructor(reverted: boolean, output: Fr[], revertReason?: Error) { - this.reverted = reverted; - this.output = output; - this.revertReason = revertReason; - } + constructor(public reverted: boolean, public output: Fr[], public revertReason?: AvmRevertReason) {} - /** - * Generate a string representation of call results. - */ toString(): string { let resultsStr = `reverted: ${this.reverted}, output: ${this.output}`; if (this.revertReason) { diff --git a/yarn-project/simulator/src/avm/avm_simulator.test.ts b/yarn-project/simulator/src/avm/avm_simulator.test.ts index 80ea7a60e29..3e180e51c57 100644 --- a/yarn-project/simulator/src/avm/avm_simulator.test.ts +++ b/yarn-project/simulator/src/avm/avm_simulator.test.ts @@ -62,7 +62,7 @@ describe('AVM simulator: injected bytecode', () => { const results = await new AvmSimulator(context).executeBytecode(bytecode); expect(results.reverted).toBe(true); expect(results.output).toEqual([]); - expect(results.revertReason?.name).toEqual('OutOfGasError'); + expect(results.revertReason?.message).toEqual('Not enough L2GAS gas left'); expect(context.machineState.l2GasLeft).toEqual(0); expect(context.machineState.daGasLeft).toEqual(0); }); diff --git a/yarn-project/simulator/src/avm/avm_simulator.ts b/yarn-project/simulator/src/avm/avm_simulator.ts index 4750ecdb23a..428dcb624d2 100644 --- a/yarn-project/simulator/src/avm/avm_simulator.ts +++ b/yarn-project/simulator/src/avm/avm_simulator.ts @@ -5,7 +5,13 @@ import { strict as assert } from 'assert'; import { isAvmBytecode } from '../public/transitional_adaptors.js'; import type { AvmContext } from './avm_context.js'; import { AvmContractCallResults } from './avm_message_call_result.js'; -import { AvmExecutionError, InvalidProgramCounterError, NoBytecodeForContractError } from './errors.js'; +import { + AvmExecutionError, + InvalidProgramCounterError, + NoBytecodeForContractError, + revertReasonFromExceptionalHalt, + revertReasonFromExplicitRevert, +} from './errors.js'; import type { Instruction } from './opcodes/index.js'; import { decodeFromBytecode } from './serialization/bytecode_serialization.js'; @@ -56,7 +62,7 @@ export class AvmSimulator { try { // Execute instruction pointed to by the current program counter // continuing until the machine state signifies a halt - while (!machineState.halted) { + while (!machineState.getHalted()) { const instruction = instructions[machineState.pc]; assert( !!instruction, @@ -76,21 +82,25 @@ export class AvmSimulator { } } - // Return results for processing by calling context - const results = machineState.getResults(); + const output = machineState.getOutput(); + const reverted = machineState.getReverted(); + const revertReason = reverted ? revertReasonFromExplicitRevert(output, this.context) : undefined; + const results = new AvmContractCallResults(reverted, output, revertReason); this.log.debug(`Context execution results: ${results.toString()}`); + // Return results for processing by calling context return results; - } catch (e) { - this.log.verbose('Exceptional halt'); - if (!(e instanceof AvmExecutionError)) { - this.log.verbose(`Unknown error thrown by avm: ${e}`); - throw e; + } catch (err: any) { + this.log.verbose('Exceptional halt (revert by something other than REVERT opcode)'); + if (!(err instanceof AvmExecutionError)) { + this.log.verbose(`Unknown error thrown by AVM: ${err}`); + throw err; } - // Return results for processing by calling context - // Note: "exceptional halts" cannot return data - const results = new AvmContractCallResults(/*reverted=*/ true, /*output=*/ [], /*revertReason=*/ e); + const revertReason = revertReasonFromExceptionalHalt(err, this.context); + // Note: "exceptional halts" cannot return data, hence [] + const results = new AvmContractCallResults(/*reverted=*/ true, /*output=*/ [], revertReason); this.log.debug(`Context execution results: ${results.toString()}`); + // Return results for processing by calling context return results; } } diff --git a/yarn-project/simulator/src/avm/errors.ts b/yarn-project/simulator/src/avm/errors.ts index 403853e7e02..90117085d3c 100644 --- a/yarn-project/simulator/src/avm/errors.ts +++ b/yarn-project/simulator/src/avm/errors.ts @@ -1,12 +1,16 @@ -import { type AztecAddress } from '@aztec/circuits.js'; +import { type FailingFunction, type NoirCallStack } from '@aztec/circuit-types'; +import { type AztecAddress, type Fr } from '@aztec/circuits.js'; + +import { ExecutionError } from '../common/errors.js'; +import { type AvmContext } from './avm_context.js'; /** * Avm-specific errors should derive from this */ export abstract class AvmExecutionError extends Error { - constructor(message: string, ...rest: any[]) { - super(message, ...rest); - this.name = 'AvmInterpreterError'; + constructor(message: string) { + super(message); + this.name = 'AvmExecutionError'; } } @@ -63,3 +67,89 @@ export class OutOfGasError extends AvmExecutionError { this.name = 'OutOfGasError'; } } + +/** + * 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) { + 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 + * options.cause is the error that caused this error (if this is not the root-cause itself). + */ +export class AvmRevertReason extends ExecutionError { + constructor(message: string, failingFunction: FailingFunction, noirCallStack: NoirCallStack, options?: ErrorOptions) { + super(message, failingFunction, noirCallStack, options); + } +} + +/** + * Helper to create a "revert reason" error optionally with a nested error cause. + * + * @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 { + return new AvmRevertReason( + message, + /*failingFunction=*/ { + contractAddress: context.environment.address, + functionSelector: context.environment.temporaryFunctionSelector, + }, + /*noirCallStack=*/ [...context.machineState.internalCallStack, context.machineState.pc].map(pc => `0.${pc}`), + /*options=*/ { cause: nestedError }, + ); +} + +/** + * Create a "revert reason" error for an exceptional halt, + * creating the recursive structure if the halt was a RethrownError. + * + * @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 { + // A RethrownError has a nested/child AvmRevertReason + const nestedError = haltingError instanceof RethrownError ? haltingError.nestedError : undefined; + return createRevertReason(haltingError.message, context, nestedError); +} + +/** + * Create a "revert reason" error for an explicit revert (a root cause). + * + * @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 { + 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: '; + } + } +} diff --git a/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts b/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts index d19f5b833e0..177bc29a890 100644 --- a/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts +++ b/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts @@ -271,11 +271,9 @@ describe('External Calls', () => { const instruction = new Return(/*indirect=*/ 0, /*returnOffset=*/ 0, returnData.length); await instruction.execute(context); - expect(context.machineState.halted).toBe(true); - expect(context.machineState.getResults()).toEqual({ - reverted: false, - output: returnData, - }); + expect(context.machineState.getHalted()).toBe(true); + expect(context.machineState.getReverted()).toBe(false); + expect(context.machineState.getOutput()).toEqual(returnData); }); }); @@ -302,12 +300,9 @@ describe('External Calls', () => { const instruction = new Revert(/*indirect=*/ 0, /*returnOffset=*/ 0, returnData.length); await instruction.execute(context); - expect(context.machineState.halted).toBe(true); - expect(context.machineState.getResults()).toEqual({ - reverted: true, - revertReason: new Error('Assertion failed: assert message'), - output: returnData.map(f => f.toFr()), - }); + expect(context.machineState.getHalted()).toBe(true); + expect(context.machineState.getReverted()).toBe(true); + expect(context.machineState.getOutput()).toEqual(returnData.map(f => f.toFr())); }); }); }); diff --git a/yarn-project/simulator/src/avm/opcodes/external_calls.ts b/yarn-project/simulator/src/avm/opcodes/external_calls.ts index 1cf06ce5fcb..479d61e27bc 100644 --- a/yarn-project/simulator/src/avm/opcodes/external_calls.ts +++ b/yarn-project/simulator/src/avm/opcodes/external_calls.ts @@ -7,7 +7,7 @@ import { gasLeftToGas, sumGas } from '../avm_gas.js'; import { Field, Uint8 } from '../avm_memory_types.js'; import { type AvmContractCallResults } from '../avm_message_call_result.js'; import { AvmSimulator } from '../avm_simulator.js'; -import { AvmExecutionError } from '../errors.js'; +import { RethrownError } from '../errors.js'; import { Opcode, OperandType } from '../serialization/instruction_serialization.js'; import { Addressing } from './addressing_mode.js'; import { Instruction } from './instruction.js'; @@ -101,15 +101,13 @@ abstract class ExternalCall extends Instruction { const success = !nestedCallResults.reverted; // TRANSITIONAL: We rethrow here so that the MESSAGE gets propagated. + // This means that for now, the caller cannot recover from errors. if (!success) { - class RethrownError extends AvmExecutionError { - constructor(message: string) { - super(message); - this.name = 'RethrownError'; - } + if (!nestedCallResults.revertReason) { + throw new Error('A reverted nested call should be assigned a revert reason in the AVM execution loop'); } - - throw new RethrownError(nestedCallResults.revertReason?.message || 'Unknown nested call error'); + // The nested call's revertReason will be used to track the stack of error causes down to the root. + throw new RethrownError(nestedCallResults.revertReason.message, nestedCallResults.revertReason); } // We only take as much data as was specified in the return size and pad with zeroes if the return data is smaller diff --git a/yarn-project/simulator/src/common/errors.ts b/yarn-project/simulator/src/common/errors.ts index 1d0bdf18402..b90a7714d20 100644 --- a/yarn-project/simulator/src/common/errors.ts +++ b/yarn-project/simulator/src/common/errors.ts @@ -2,6 +2,11 @@ import { type FailingFunction, type NoirCallStack, SimulationError } from '@azte /** * An error that occurred during the execution of a function. + * @param message - the error message + * @param failingFunction - the Aztec function that failed + * @param noirCallStack - the internal call stack of the function that failed (within the failing Aztec function) + * @param options - additional error options (an optional "cause" entry allows for a recursive error stack where + * an error's cause may be an ExecutionError itself) */ export class ExecutionError extends Error { constructor( diff --git a/yarn-project/simulator/src/public/transitional_adaptors.ts b/yarn-project/simulator/src/public/transitional_adaptors.ts index ae317f9006e..161b10091c8 100644 --- a/yarn-project/simulator/src/public/transitional_adaptors.ts +++ b/yarn-project/simulator/src/public/transitional_adaptors.ts @@ -1,10 +1,8 @@ // All code in this file needs to die once the public executor is phased out in favor of the AVM. -import { type SimulationError, UnencryptedFunctionL2Logs } from '@aztec/circuit-types'; +import { UnencryptedFunctionL2Logs } from '@aztec/circuit-types'; import { - type AztecAddress, CallContext, FunctionData, - type FunctionSelector, type Gas, type GasSettings, type GlobalVariables, @@ -12,12 +10,11 @@ import { } from '@aztec/circuits.js'; import { Fr } from '@aztec/foundation/fields'; -import { extractCallStack } from '../acvm/index.js'; import { type AvmContext } from '../avm/avm_context.js'; import { AvmExecutionEnvironment } from '../avm/avm_execution_environment.js'; import { type AvmContractCallResults } from '../avm/avm_message_call_result.js'; import { Mov } from '../avm/opcodes/memory.js'; -import { ExecutionError, createSimulationError } from '../common/errors.js'; +import { createSimulationError } from '../common/errors.js'; import { type PublicExecution, type PublicExecutionResult } from './execution.js'; /** @@ -75,29 +72,6 @@ export function createPublicExecution( return execution; } -export function processRevertReason( - revertReason: Error | undefined, - contractAddress: AztecAddress, - functionSelector: FunctionSelector, -): SimulationError | undefined { - if (!revertReason) { - return undefined; - } - if (revertReason instanceof Error) { - const ee = new ExecutionError( - revertReason.message, - { - contractAddress, - functionSelector, - }, - extractCallStack(revertReason), - { cause: revertReason }, - ); - - return createSimulationError(ee); - } -} - export function convertAvmResultsToPxResult( avmResult: AvmContractCallResults, startSideEffectCounter: number, @@ -119,11 +93,7 @@ export function convertAvmResultsToPxResult( endPersistableState.transitionalExecutionResult.allUnencryptedLogs, ), reverted: avmResult.reverted, - revertReason: processRevertReason( - avmResult.revertReason, - endAvmContext.environment.address, - fromPx.functionData.selector, - ), + revertReason: avmResult.revertReason ? createSimulationError(avmResult.revertReason) : undefined, startGasLeft: startGas, endGasLeft: endMachineState.gasLeft, transactionFee: endAvmContext.environment.transactionFee,