Skip to content

Commit

Permalink
feat(avm): remove rethrowable reverts hack (#9752)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@fcarreiro
fcarreiro authored Nov 5, 2024
1 parent 5c5be5c commit 2ab33e7
Show file tree
Hide file tree
Showing 14 changed files with 250 additions and 229 deletions.
15 changes: 15 additions & 0 deletions noir-projects/noir-contracts/contracts/avm_test_contract/src/main.nr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
35 changes: 26 additions & 9 deletions yarn-project/end-to-end/src/e2e_avm_simulator.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
);
});
});
});

Expand Down
115 changes: 3 additions & 112 deletions yarn-project/simulator/src/acvm/acvm.ts
View file
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -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.
*/
Expand Down
18 changes: 17 additions & 1 deletion yarn-project/simulator/src/avm/avm_machine_state.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand All @@ -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.
Expand Down
34 changes: 16 additions & 18 deletions yarn-project/simulator/src/avm/avm_simulator.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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([
Expand Down Expand Up @@ -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);
Expand All @@ -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 () => {
Expand All @@ -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);

Expand All @@ -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);
Expand Down
9 changes: 2 additions & 7 deletions yarn-project/simulator/src/avm/avm_simulator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {
AvmExecutionError,
InvalidProgramCounterError,
NoBytecodeForContractError,
revertDataFromExceptionalHalt,
revertReasonFromExceptionalHalt,
revertReasonFromExplicitRevert,
} from './errors.js';
Expand Down Expand Up @@ -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();
Expand Down
Loading

0 comments on commit 2ab33e7

Please sign in to comment.