From 2ab33e7f741f51905542cc4f646f2451454a770b Mon Sep 17 00:00:00 2001 From: Facundo Date: Tue, 5 Nov 2024 16:54:39 +0000 Subject: [PATCH] feat(avm): remove rethrowable reverts hack (#9752) This PR removes the RethrowableError hack in the AVM simulator, and relies on the PublicContext's [rethrow](https://github.com/AztecProtocol/aztec-packages/blob/master/noir-projects/aztec-nr/aztec/src/context/public_context.nr#L88) to propagate the errors. There are two caveats. First, because currently Aztec-nr does not keep track of the cause chain, it would be impossible to have the call stack and original contract address available, so that the PXE can interpret the error and show debug information. Solidity has the same problem. I'm introducing a heuristic to keep track of the call stack for the simple case where the caller always rethrows: the simulator keeps a running trace in the machineState, and the caller uses this trace IF the revertData coincides. That is, if you are (re)throwing the same as what we were tracking. Second, while this all works well for errors in user code (e.g., `assert` in Noir), because they generate a revertData with an error selector and data, the "intrinsic" errors from the simulator (aka exceptional halts) do not work as well. E.g., "division by zero", "duplicated nullifier", "l1 to l2 blah blah". These errors are exceptions in typescript and do not have an associated error selector, and do not add to the revertdata. This _could_ be done with enshrined error selectors. That's easy in the simulator, but it's not easy in the circuit for several reasons that are beyond the scope of this description. The current solution is to propagate the error message (the user will see the right error) but you cannot "catch and process" the error in aztec.nr because there is no selector. This is not a limitation right now because there's no interface in the PublicContext that would let you catch errors. To be continued. Part of #9061. --- .../contracts/avm_test_contract/src/main.nr | 15 +++ .../end-to-end/src/e2e_avm_simulator.test.ts | 35 ++++-- yarn-project/simulator/src/acvm/acvm.ts | 115 +---------------- .../simulator/src/avm/avm_machine_state.ts | 18 ++- .../simulator/src/avm/avm_simulator.test.ts | 34 +++-- .../simulator/src/avm/avm_simulator.ts | 9 +- yarn-project/simulator/src/avm/errors.ts | 79 ++++-------- .../simulator/src/avm/fixtures/index.ts | 12 +- .../simulator/src/avm/opcodes/arithmetic.ts | 6 +- .../src/avm/opcodes/external_calls.test.ts | 4 +- .../src/avm/opcodes/external_calls.ts | 27 ++-- .../simulator/src/client/private_execution.ts | 4 +- .../src/client/unconstrained_execution.ts | 4 +- yarn-project/simulator/src/common/errors.ts | 117 +++++++++++++++++- 14 files changed, 250 insertions(+), 229 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 586ad030846..e80df65b8a3 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 @@ -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"); 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 c594663f8de..026a2e8602f 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 @@ -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', + ); + }); }); }); diff --git a/yarn-project/simulator/src/acvm/acvm.ts b/yarn-project/simulator/src/acvm/acvm.ts index bbd031eb315..dce850d220b 100644 --- a/yarn-project/simulator/src/acvm/acvm.ts +++ b/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. */ diff --git a/yarn-project/simulator/src/avm/avm_machine_state.ts b/yarn-project/simulator/src/avm/avm_machine_state.ts index 3c1aac84528..1ba4fa601b9 100644 --- a/yarn-project/simulator/src/avm/avm_machine_state.ts +++ b/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. diff --git a/yarn-project/simulator/src/avm/avm_simulator.test.ts b/yarn-project/simulator/src/avm/avm_simulator.test.ts index 70682ec440a..f4d84ead171 100644 --- a/yarn-project/simulator/src/avm/avm_simulator.test.ts +++ b/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(); + 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); diff --git a/yarn-project/simulator/src/avm/avm_simulator.ts b/yarn-project/simulator/src/avm/avm_simulator.ts index 788a3908b2b..7b6f7e26aef 100644 --- a/yarn-project/simulator/src/avm/avm_simulator.ts +++ b/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(); diff --git a/yarn-project/simulator/src/avm/errors.ts b/yarn-project/simulator/src/avm/errors.ts index 444d95ce28b..f9c5d819a77 100644 --- a/yarn-project/simulator/src/avm/errors.ts +++ b/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 @@ -101,14 +96,7 @@ export class AvmRevertReason extends ExecutionError { } } -/** - * 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 { +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 +106,18 @@ 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; + message = context.machineState.collectedRevertInfo.recursiveRevertReason.message; + } + return new AvmRevertReason( message, /*failingFunction=*/ { @@ -130,25 +130,13 @@ function createRevertReason(message: string, context: AvmContext, nestedError?: } /** - * Create a "revert reason" error for an exceptional halt, - * creating the recursive structure if the halt was a RethrownError. + * Create a "revert reason" error for an exceptional halt. * * @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); -} - -/** - * 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 +146,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: '; - } - } + return createRevertReason('Assertion failed: ', revertData, context); } diff --git a/yarn-project/simulator/src/avm/fixtures/index.ts b/yarn-project/simulator/src/avm/fixtures/index.ts index 4b511b31ace..271fca8618e 100644 --- a/yarn-project/simulator/src/avm/fixtures/index.ts +++ b/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'; @@ -124,12 +124,17 @@ export function getAvmTestContractFunctionSelector(functionName: string): Functi return FunctionSelector.fromNameAndParameters(artifact.name, params); } -export function getAvmTestContractBytecode(functionName: string): Buffer { +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 getAvmTestContractBytecode(functionName: string): Buffer { + const artifact = getAvmTestContractArtifact(functionName); return artifact.bytecode; } @@ -138,12 +143,11 @@ export function resolveAvmTestContractAssertionMessage( revertReason: AvmRevertReason, output: Fr[], ): string | undefined { - const functionArtifact = AvmTestContractArtifact.functions.find(f => f.name === functionName)!; - traverseCauseChain(revertReason, cause => { revertReason = cause as AvmRevertReason; }); + const functionArtifact = AvmTestContractArtifact.functions.find(f => f.name === functionName); if (!functionArtifact || !revertReason.noirCallStack || !isNoirCallStackUnresolved(revertReason.noirCallStack)) { return undefined; } diff --git a/yarn-project/simulator/src/avm/opcodes/arithmetic.ts b/yarn-project/simulator/src/avm/opcodes/arithmetic.ts index 571c1b64ed5..30a1ead0efa 100644 --- a/yarn-project/simulator/src/avm/opcodes/arithmetic.ts +++ b/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. 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 58dd8152391..4bc38e78d2c 100644 --- a/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts +++ b/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts @@ -231,7 +231,9 @@ describe('External Calls', () => { argsSizeOffset, successOffset, ); - await expect(() => instruction.execute(context)).rejects.toThrow( + await instruction.execute(context); + // Ideally we'd mock the nested call. + expect(context.machineState.collectedRevertInfo?.recursiveRevertReason.message).toMatch( 'Static call cannot update the state, emit L2->L1 messages or generate logs', ); }); diff --git a/yarn-project/simulator/src/avm/opcodes/external_calls.ts b/yarn-project/simulator/src/avm/opcodes/external_calls.ts index d1200cdc491..7bf31a24a45 100644 --- a/yarn-project/simulator/src/avm/opcodes/external_calls.ts +++ b/yarn-project/simulator/src/avm/opcodes/external_calls.ts @@ -5,7 +5,6 @@ import { type AvmContractCallResult } from '../avm_contract_call_result.js'; import { gasLeftToGas } from '../avm_gas.js'; import { type Field, TypeTag, Uint1 } from '../avm_memory_types.js'; import { AvmSimulator } from '../avm_simulator.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'; @@ -76,24 +75,22 @@ abstract class ExternalCall extends Instruction { const nestedCallResults: AvmContractCallResult = await simulator.execute(); 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) { - if (!nestedCallResults.revertReason) { - throw new Error('A reverted nested call should be assigned a revert reason in the AVM execution loop'); - } - // 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, - nestedCallResults.output, - ); - } - // Save return/revert data for later. const fullReturnData = nestedCallResults.output; context.machineState.nestedReturndata = fullReturnData; + // If the nested call reverted, we try to save the reason and the revert data. + // This will be used by the caller to try to reconstruct the call stack. + // This is only a heuristic and may not always work. It is intended to work + // for the case where a nested call reverts and the caller always rethrows + // (in Noir code). + if (!success) { + context.machineState.collectedRevertInfo = { + revertDataRepresentative: fullReturnData, + recursiveRevertReason: nestedCallResults.revertReason!, + }; + } + // Write our success flag into memory. memory.set(successOffset, new Uint1(success ? 1 : 0)); diff --git a/yarn-project/simulator/src/client/private_execution.ts b/yarn-project/simulator/src/client/private_execution.ts index d27db358c56..0d117501df2 100644 --- a/yarn-project/simulator/src/client/private_execution.ts +++ b/yarn-project/simulator/src/client/private_execution.ts @@ -12,8 +12,8 @@ import { createDebugLogger } from '@aztec/foundation/log'; import { Timer } from '@aztec/foundation/timer'; import { fromACVMField, witnessMapToFields } from '../acvm/deserialize.js'; -import { type ACVMWitness, Oracle, acvm, extractCallStack, resolveAssertionMessageFromError } from '../acvm/index.js'; -import { ExecutionError } from '../common/errors.js'; +import { type ACVMWitness, Oracle, acvm, extractCallStack } from '../acvm/index.js'; +import { ExecutionError, resolveAssertionMessageFromError } from '../common/errors.js'; import { type ClientExecutionContext } from './client_execution_context.js'; /** diff --git a/yarn-project/simulator/src/client/unconstrained_execution.ts b/yarn-project/simulator/src/client/unconstrained_execution.ts index f926b29bdca..70e488434db 100644 --- a/yarn-project/simulator/src/client/unconstrained_execution.ts +++ b/yarn-project/simulator/src/client/unconstrained_execution.ts @@ -4,8 +4,8 @@ import { type Fr } from '@aztec/foundation/fields'; import { createDebugLogger } from '@aztec/foundation/log'; import { witnessMapToFields } from '../acvm/deserialize.js'; -import { Oracle, acvm, extractCallStack, resolveAssertionMessageFromError, toACVMWitness } from '../acvm/index.js'; -import { ExecutionError } from '../common/errors.js'; +import { Oracle, acvm, extractCallStack, toACVMWitness } from '../acvm/index.js'; +import { ExecutionError, resolveAssertionMessageFromError } from '../common/errors.js'; import { type ViewDataOracle } from './view_data_oracle.js'; // docs:start:execute_unconstrained_function diff --git a/yarn-project/simulator/src/common/errors.ts b/yarn-project/simulator/src/common/errors.ts index c02ec606a8d..f51536ac457 100644 --- a/yarn-project/simulator/src/common/errors.ts +++ b/yarn-project/simulator/src/common/errors.ts @@ -1,5 +1,14 @@ -import { type FailingFunction, type NoirCallStack, SimulationError } from '@aztec/circuit-types'; +import { + type FailingFunction, + type NoirCallStack, + SimulationError, + 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 RawAssertionPayload } from '@noir-lang/acvm_js'; +import { abiDecodeError } from '@noir-lang/noirc_abi'; /** * An error that occurred during the execution of a function. @@ -65,3 +74,109 @@ export function createSimulationError(error: Error, revertData?: Fr[]): Simulati return new SimulationError(rootCause.message, aztecCallStack, revertData, noirCallStack, { cause: rootCause }); } + +/** + * 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; + } +}