Skip to content

Commit

Permalink
feat: Initial trazability of ACIR (#1701)
Browse files Browse the repository at this point in the history
Working towards
#1224

This PR adds trazability of errors for ACIR opcodes when the debug data
is available. Future work will enable this in brillig, and also will add
assertion messages embedded in the bytecode itself.
# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [ ] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [x] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [x] Every change is related to the PR description.
- [x] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).
  • Loading branch information
sirasistant authored Aug 22, 2023
1 parent 79b4585 commit 89e4e1a
Show file tree
Hide file tree
Showing 30 changed files with 67,302 additions and 263 deletions.
2 changes: 1 addition & 1 deletion yarn-project/acir-simulator/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
"@aztec/circuits.js": "workspace:^",
"@aztec/foundation": "workspace:^",
"@aztec/types": "workspace:^",
"acvm_js": "github:noir-lang/acvm-js-wasm#db/init-sim-backend",
"acvm_js": "github:noir-lang/acvm-js-wasm#arv/0.22+init-pedersen",
"levelup": "^5.1.1",
"memdown": "^6.1.1",
"tslib": "^2.4.0"
Expand Down
102 changes: 98 additions & 4 deletions yarn-project/acir-simulator/src/acvm/acvm.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { FunctionDebugMetadata } from '@aztec/foundation/abi';
import { AztecAddress } from '@aztec/foundation/aztec-address';
import { EthAddress } from '@aztec/foundation/eth-address';
import { Fr } from '@aztec/foundation/fields';
Expand Down Expand Up @@ -71,6 +72,74 @@ export interface ACIRExecutionResult {
partialWitness: ACVMWitness;
}

/**
* Extracts the opcode location from an ACVM error string.
*/
function extractOpcodeLocationFromError(err: string): string | undefined {
const match = err.match(/^Cannot satisfy constraint (?<opcodeLocation>[0-9]+(?:\.[0-9]+)?)/);
return match?.groups?.opcodeLocation;
}

/**
* The data for a call in the call stack.
*/
interface SourceCodeLocation {
/**
* The path to the source file.
*/
filePath: string;
/**
* The line number of the call.
*/
line: number;
/**
* The source code of the file.
*/
fileSource: string;
/**
* The source code text of the failed constraint.
*/
assertionText: string;
}

/**
* Extracts the call stack from the location of a failing opcode and the debug metadata.
*/
function getCallStackFromOpcodeLocation(opcodeLocation: string, debug: FunctionDebugMetadata): SourceCodeLocation[] {
const { debugSymbols, files } = debug;

const callStack = debugSymbols.locations[opcodeLocation] || [];
return callStack.map(call => {
const { file: fileId, span } = call;

const { path, source } = files[fileId];

const assertionText = source.substring(span.start, span.end + 1);
const precedingText = source.substring(0, span.start);
const line = precedingText.split('\n').length;

return {
filePath: path,
line,
fileSource: source,
assertionText,
};
});
}

/**
* Creates a formatted string for an error stack
* @param callStack - The error stack
* @returns - The formatted string
*/
function printErrorStack(callStack: SourceCodeLocation[]): string {
// TODO experiment with formats of reporting this for better error reporting
return [
'Error: Assertion failed',
callStack.map(call => ` at ${call.filePath}:${call.line} '${call.assertionText}'`),
].join('\n');
}

/**
* The function call that executes an ACIR.
*/
Expand All @@ -79,8 +148,13 @@ export async function acvm(
acir: Buffer,
initialWitness: ACVMWitness,
callback: ACIRCallback,
debug?: FunctionDebugMetadata,
): Promise<ACIRExecutionResult> {
const logger = createDebugLogger('aztec:simulator:acvm');
// This is a workaround to avoid the ACVM removing the information about the underlying error.
// We should probably update the ACVM to let proper errors through.
let oracleError: Error | undefined = undefined;

const partialWitness = await executeCircuitWithBlackBoxSolver(
solver,
acir,
Expand All @@ -95,12 +169,32 @@ export async function acvm(

const result = await oracleFunction.call(callback, ...args);
return [result];
} catch (err: any) {
logger.error(`Error in oracle callback ${name}: ${err.message ?? err ?? 'Unknown'}`);
throw err;
} catch (err) {
let typedError: Error;
if (err instanceof Error) {
typedError = err;
} else {
typedError = new Error(`Error in oracle callback ${err}`);
}
oracleError = typedError;
logger.error(`Error in oracle callback ${name}: ${typedError.message}`);
throw typedError;
}
},
);
).catch((acvmError: string) => {
if (oracleError) {
throw oracleError;
}
const opcodeLocation = extractOpcodeLocationFromError(acvmError);
if (!opcodeLocation || !debug) {
throw new Error(acvmError);
}

const callStack = getCallStackFromOpcodeLocation(opcodeLocation, debug);
logger(printErrorStack(callStack));
throw new Error(`Assertion failed: '${callStack.pop()?.assertionText ?? 'Unknown'}'`);
});

return Promise.resolve({ partialWitness });
}

Expand Down
14 changes: 12 additions & 2 deletions yarn-project/acir-simulator/src/client/db_oracle.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { CompleteAddress, HistoricBlockData, PrivateKey, PublicKey } from '@aztec/circuits.js';
import { FunctionAbi } from '@aztec/foundation/abi';
import { FunctionAbi, FunctionDebugMetadata } from '@aztec/foundation/abi';
import { AztecAddress } from '@aztec/foundation/aztec-address';
import { EthAddress } from '@aztec/foundation/eth-address';
import { Fr } from '@aztec/foundation/fields';
Expand Down Expand Up @@ -67,6 +67,16 @@ export interface CommitmentDataOracleInputs {
index: bigint;
}

/**
* A function ABI with optional debug metadata
*/
export interface FunctionAbiWithDebugMetadata extends FunctionAbi {
/**
* Debug metadata for the function.
*/
debug?: FunctionDebugMetadata;
}

/**
* The database oracle interface.
*/
Expand Down Expand Up @@ -109,7 +119,7 @@ export interface DBOracle extends CommitmentsDB {
* @param functionSelector - The Buffer containing the function selector bytes.
* @returns A Promise that resolves to a FunctionAbi object containing the ABI information of the target function.
*/
getFunctionABI(contractAddress: AztecAddress, functionSelector: Buffer): Promise<FunctionAbi>;
getFunctionABI(contractAddress: AztecAddress, functionSelector: Buffer): Promise<FunctionAbiWithDebugMetadata>;

/**
* Retrieves the portal contract address associated with the given contract address.
Expand Down
40 changes: 20 additions & 20 deletions yarn-project/acir-simulator/src/client/private_execution.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ import { MockProxy, mock } from 'jest-mock-extended';
import { default as levelup } from 'levelup';
import { type MemDown, default as memdown } from 'memdown';

import { buildL1ToL2Message } from '../test/utils.js';
import { buildL1ToL2Message, getFunctionAbi } from '../test/utils.js';
import { computeSlotForMapping } from '../utils.js';
import { DBOracle } from './db_oracle.js';
import { AcirSimulator } from './simulator.js';
Expand Down Expand Up @@ -154,7 +154,7 @@ describe('Private Execution test suite', () => {

describe('empty constructor', () => {
it('should run the empty constructor', async () => {
const abi = TestContractAbi.functions[0];
const abi = getFunctionAbi(TestContractAbi, 'constructor');
const contractDeploymentData = makeContractDeploymentData(100);
const txContext = { isContractDeploymentTx: true, contractDeploymentData };
const result = await runSimulator({ abi, txContext });
Expand Down Expand Up @@ -251,7 +251,7 @@ describe('Private Execution test suite', () => {
});

it('should a constructor with arguments that inserts notes', async () => {
const abi = PrivateTokenAirdropContractAbi.functions.find(f => f.name === 'constructor')!;
const abi = getFunctionAbi(PrivateTokenAirdropContractAbi, 'constructor');

const result = await runSimulator({ args: [140, owner], abi });

Expand All @@ -269,7 +269,7 @@ describe('Private Execution test suite', () => {
});

it('should run the mint function', async () => {
const abi = PrivateTokenAirdropContractAbi.functions.find(f => f.name === 'mint')!;
const abi = getFunctionAbi(PrivateTokenAirdropContractAbi, 'mint');

const result = await runSimulator({ args: [140, owner], abi });

Expand All @@ -288,7 +288,7 @@ describe('Private Execution test suite', () => {

it('should run the transfer function', async () => {
const amountToTransfer = 100n;
const abi = PrivateTokenAirdropContractAbi.functions.find(f => f.name === 'transfer')!;
const abi = getFunctionAbi(PrivateTokenAirdropContractAbi, 'transfer');

const storageSlot = computeSlotForMapping(new Fr(1n), owner.toField(), circuitsWasm);
const recipientStorageSlot = computeSlotForMapping(new Fr(1n), recipient.toField(), circuitsWasm);
Expand Down Expand Up @@ -333,7 +333,7 @@ describe('Private Execution test suite', () => {
it('should be able to transfer with dummy notes', async () => {
const amountToTransfer = 100n;
const balance = 160n;
const abi = PrivateTokenAirdropContractAbi.functions.find(f => f.name === 'transfer')!;
const abi = getFunctionAbi(PrivateTokenAirdropContractAbi, 'transfer');

const storageSlot = computeSlotForMapping(new Fr(1n), owner.toField(), circuitsWasm);

Expand All @@ -360,7 +360,7 @@ describe('Private Execution test suite', () => {
it('Should be able to claim a note by providing the correct secret', async () => {
const amount = 100n;
const secret = Fr.random();
const abi = PrivateTokenAirdropContractAbi.functions.find(f => f.name === 'claim')!;
const abi = getFunctionAbi(PrivateTokenAirdropContractAbi, 'claim');
const storageSlot = new Fr(2n);
// choose nonzero nonce otherwise reads will be interpreted as transient (inner note hash instead of unique+siloed)
const nonce = new Fr(1n);
Expand Down Expand Up @@ -479,7 +479,7 @@ describe('Private Execution test suite', () => {
});

it('should a constructor with arguments that inserts notes', async () => {
const abi = PrivateTokenContractAbi.functions.find(f => f.name === 'constructor')!;
const abi = getFunctionAbi(PrivateTokenContractAbi, 'constructor');

const result = await runSimulator({ args: [140, owner], abi });

Expand All @@ -497,7 +497,7 @@ describe('Private Execution test suite', () => {
});

it('should run the mint function', async () => {
const abi = PrivateTokenContractAbi.functions.find(f => f.name === 'mint')!;
const abi = getFunctionAbi(PrivateTokenContractAbi, 'mint');

const result = await runSimulator({ args: [140, owner], abi });

Expand All @@ -516,7 +516,7 @@ describe('Private Execution test suite', () => {

it('should run the transfer function', async () => {
const amountToTransfer = 100n;
const abi = PrivateTokenContractAbi.functions.find(f => f.name === 'transfer')!;
const abi = getFunctionAbi(PrivateTokenContractAbi, 'transfer');

const storageSlot = computeSlotForMapping(new Fr(1n), owner.toField(), circuitsWasm);
const recipientStorageSlot = computeSlotForMapping(new Fr(1n), recipient.toField(), circuitsWasm);
Expand Down Expand Up @@ -561,7 +561,7 @@ describe('Private Execution test suite', () => {
it('should be able to transfer with dummy notes', async () => {
const amountToTransfer = 100n;
const balance = 160n;
const abi = PrivateTokenContractAbi.functions.find(f => f.name === 'transfer')!;
const abi = getFunctionAbi(PrivateTokenContractAbi, 'transfer');

const storageSlot = computeSlotForMapping(new Fr(1n), owner.toField(), circuitsWasm);

Expand Down Expand Up @@ -591,15 +591,15 @@ describe('Private Execution test suite', () => {

it('child function should be callable', async () => {
const initialValue = 100n;
const abi = ChildContractAbi.functions.find(f => f.name === 'value')!;
const abi = getFunctionAbi(ChildContractAbi, 'value');
const result = await runSimulator({ args: [initialValue], abi });

expect(result.callStackItem.publicInputs.returnValues[0]).toEqual(new Fr(initialValue + privateIncrement));
});

it('parent should call child', async () => {
const childAbi = ChildContractAbi.functions.find(f => f.name === 'value')!;
const parentAbi = ParentContractAbi.functions.find(f => f.name === 'entryPoint')!;
const childAbi = getFunctionAbi(ChildContractAbi, 'value');
const parentAbi = getFunctionAbi(ParentContractAbi, 'entryPoint');
const parentAddress = AztecAddress.random();
const childAddress = AztecAddress.random();
const childSelector = generateFunctionSelector(childAbi.name, childAbi.parameters);
Expand Down Expand Up @@ -686,7 +686,7 @@ describe('Private Execution test suite', () => {

it('Should be able to consume a dummy cross chain message', async () => {
const bridgedAmount = 100n;
const abi = NonNativeTokenContractAbi.functions.find(f => f.name === 'mint')!;
const abi = getFunctionAbi(NonNativeTokenContractAbi, 'mint');

const secret = new Fr(1n);
const canceller = EthAddress.random();
Expand Down Expand Up @@ -720,7 +720,7 @@ describe('Private Execution test suite', () => {

it('Should be able to consume a dummy public to private message', async () => {
const amount = 100n;
const abi = NonNativeTokenContractAbi.functions.find(f => f.name === 'redeemShield')!;
const abi = getFunctionAbi(NonNativeTokenContractAbi, 'redeemShield');

const wasm = await CircuitsWasm.get();
const secret = new Fr(1n);
Expand Down Expand Up @@ -998,7 +998,7 @@ describe('Private Execution test suite', () => {
describe('get public key', () => {
it('gets the public key for an address', async () => {
// Tweak the contract ABI so we can extract return values
const abi = TestContractAbi.functions.find(f => f.name === 'getPublicKey')!;
const abi = getFunctionAbi(TestContractAbi, 'getPublicKey');
abi.returnTypes = [{ kind: 'array', length: 2, type: { kind: 'field' } }];

// Generate a partial address, pubkey, and resulting address
Expand All @@ -1018,7 +1018,7 @@ describe('Private Execution test suite', () => {
const aztecAddressToQuery = AztecAddress.random();

// Tweak the contract ABI so we can extract return values
const abi = TestContractAbi.functions.find(f => f.name === 'getPortalContractAddress')!;
const abi = getFunctionAbi(TestContractAbi, 'getPortalContractAddress');
abi.returnTypes = [{ kind: 'field' }];

const args = [aztecAddressToQuery.toField()];
Expand All @@ -1033,7 +1033,7 @@ describe('Private Execution test suite', () => {
const contractAddress = AztecAddress.random();

// Tweak the contract ABI so we can extract return values
const abi = TestContractAbi.functions.find(f => f.name === 'getThisAddress')!;
const abi = getFunctionAbi(TestContractAbi, 'getThisAddress');
abi.returnTypes = [{ kind: 'field' }];

// Overwrite the oracle return value
Expand All @@ -1045,7 +1045,7 @@ describe('Private Execution test suite', () => {
const portalContractAddress = EthAddress.random();

// Tweak the contract ABI so we can extract return values
const abi = TestContractAbi.functions.find(f => f.name === 'getThisPortalAddress')!;
const abi = getFunctionAbi(TestContractAbi, 'getThisPortalAddress');
abi.returnTypes = [{ kind: 'field' }];

// Overwrite the oracle return value
Expand Down
Loading

0 comments on commit 89e4e1a

Please sign in to comment.