From 2cf11ac76805b8d648a4b32d7cd6446eb31b9a35 Mon Sep 17 00:00:00 2001 From: Facundo Date: Tue, 16 Apr 2024 13:55:42 +0100 Subject: [PATCH] feat(avm-simulator): plumb noir assertion messages (#5774) Enable recovering Noir assertion messages in the avm simulator. We had discussed propagating (re-throwing) nested call assertions to callers, to match the current Public behaviour, but I'm not doing it here so that I don't clash with David's work on nested calls. Moreover, the behaviour of reverting if a nested call reverts is currently happening (see `call_public` in AvmContext in noir). The only difference is really that the messsage propagated is different: in current public you get the assertion message from the nested call (I think but I should double check) and in the AVM simulator you'd get "Nested (static) call failed!". We can discuss if we want to change this. --- .../contracts/avm_test_contract/src/main.nr | 8 +++++--- .../simulator/src/avm/avm_machine_state.ts | 13 ++++++++++++- .../simulator/src/avm/avm_simulator.test.ts | 16 ++++++++++++++-- .../src/avm/opcodes/external_calls.test.ts | 9 ++++----- 4 files changed, 35 insertions(+), 11 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 918ee1abb27..2989e32a9f6 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 @@ -185,7 +185,7 @@ contract AvmTest { #[aztec(public-vm)] fn test_get_contract_instance() { let ci = get_contract_instance_avm(context.this_address()); - assert(ci.is_some()); + assert(ci.is_some(), "Contract instance not found!"); } /************************************************************************ @@ -258,7 +258,9 @@ contract AvmTest { #[aztec(public-vm)] fn check_selector() { - assert(context.selector() == FunctionSelector::from_signature("check_selector()")); + assert( + context.selector() == FunctionSelector::from_signature("check_selector()"), "Unexpected selector!" + ); } #[aztec(public-vm)] @@ -299,7 +301,7 @@ contract AvmTest { #[aztec(public-vm)] fn assert_nullifier_exists(nullifier: Field) { - assert(context.nullifier_exists(nullifier, context.this_address())); + assert(context.nullifier_exists(nullifier, context.this_address()), "Nullifier doesn't exist!"); } // Use the standard context interface to emit a new nullifier diff --git a/yarn-project/simulator/src/avm/avm_machine_state.ts b/yarn-project/simulator/src/avm/avm_machine_state.ts index 4c5c58fcc6a..8ebfa6b1e47 100644 --- a/yarn-project/simulator/src/avm/avm_machine_state.ts +++ b/yarn-project/simulator/src/avm/avm_machine_state.ts @@ -138,6 +138,17 @@ export class AvmMachineState { if (!this.halted) { throw new Error('Execution results are not ready! Execution is ongoing.'); } - return new AvmContractCallResults(this.reverted, this.output); + let revertReason = undefined; + if (this.reverted && this.output.length > 0) { + try { + // Try to interpret the output as a text string. + revertReason = new Error( + 'Reverted with output: ' + String.fromCharCode(...this.output.map(fr => fr.toNumber())), + ); + } catch (e) { + revertReason = new Error('Reverted with non-string output'); + } + } + return new AvmContractCallResults(this.reverted, this.output, revertReason); } } diff --git a/yarn-project/simulator/src/avm/avm_simulator.test.ts b/yarn-project/simulator/src/avm/avm_simulator.test.ts index d6026089f1c..58d451ab0b6 100644 --- a/yarn-project/simulator/src/avm/avm_simulator.test.ts +++ b/yarn-project/simulator/src/avm/avm_simulator.test.ts @@ -106,6 +106,18 @@ describe('AVM simulator: transpiled Noir contracts', () => { expect(results.output).toEqual([new Fr(4), new Fr(6)]); }); + it('Assertion message', async () => { + const calldata: Fr[] = [new Fr(20)]; + const context = initContext({ env: initExecutionEnvironment({ calldata }) }); + + const bytecode = getAvmTestContractBytecode('assert_nullifier_exists'); + const results = await new AvmSimulator(context).executeBytecode(bytecode); + + expect(results.reverted).toBe(true); + expect(results.revertReason?.message).toEqual("Reverted with output: Nullifier doesn't exist!"); + expect(results.output).toEqual([..."Nullifier doesn't exist!"].flatMap(c => new Fr(c.charCodeAt(0)))); + }); + describe.each([ ['set_opcode_u8', 8n], ['set_opcode_u32', 1n << 30n], @@ -454,6 +466,7 @@ describe('AVM simulator: transpiled Noir contracts', () => { const results = await new AvmSimulator(context).executeBytecode(bytecode); expect(results.reverted).toBe(true); + expect(results.revertReason?.message).toMatch(/Attempted to emit duplicate nullifier/); // Only the first nullifier should be in the trace, second one failed to add expect(context.persistableState.flush().newNullifiers).toEqual([ expect.objectContaining({ @@ -899,8 +912,7 @@ describe('AVM simulator: transpiled Noir contracts', () => { const results = await new AvmSimulator(context).executeBytecode(callBytecode); expect(results.reverted).toBe(true); // The outer call should revert. - // TODO(fcarreiro): revertReason lost in translation between results. - // expect(results.revertReason).toEqual(/StaticCallStorageAlterError/); + expect(results.revertReason?.message).toMatch(/Nested static call 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 e4dde5c335a..c9acf97b61e 100644 --- a/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts +++ b/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts @@ -304,11 +304,9 @@ describe('External Calls', () => { }); it('Should return data and revert from the revert opcode', async () => { - const returnData = [new Fr(1n), new Fr(2n), new Fr(3n)]; + const returnData = [...'assert message'].flatMap(c => new Field(c.charCodeAt(0))); - context.machineState.memory.set(0, new Field(1n)); - context.machineState.memory.set(1, new Field(2n)); - context.machineState.memory.set(2, new Field(3n)); + context.machineState.memory.setSlice(0, returnData); const instruction = new Revert(/*indirect=*/ 0, /*returnOffset=*/ 0, returnData.length); await instruction.execute(context); @@ -316,7 +314,8 @@ describe('External Calls', () => { expect(context.machineState.halted).toBe(true); expect(context.machineState.getResults()).toEqual({ reverted: true, - output: returnData, + revertReason: new Error('Reverted with output: assert message'), + output: returnData.map(f => f.toFr()), }); }); });