From 12a94360237eb6a56994a013afed84caee167826 Mon Sep 17 00:00:00 2001 From: fcarreiro Date: Wed, 8 May 2024 15:36:54 +0000 Subject: [PATCH 1/2] fix(avm-simulator): rethrow nested assertions --- .../src/main.nr | 6 ++++ .../simulator/src/avm/avm_simulator.test.ts | 18 ++++++++++- .../src/avm/opcodes/external_calls.test.ts | 32 +++++++------------ .../src/avm/opcodes/external_calls.ts | 13 ++++++++ 4 files changed, 47 insertions(+), 22 deletions(-) diff --git a/noir-projects/noir-contracts/contracts/avm_nested_calls_test_contract/src/main.nr b/noir-projects/noir-contracts/contracts/avm_nested_calls_test_contract/src/main.nr index 1ebc736cc8d..a21791aaee4 100644 --- a/noir-projects/noir-contracts/contracts/avm_nested_calls_test_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/avm_nested_calls_test_contract/src/main.nr @@ -26,6 +26,12 @@ contract AvmNestedCallsTest { arg_a + arg_b } + #[aztec(public-vm)] + fn assert_same(arg_a: Field, arg_b: Field) -> pub Field { + assert(arg_a == arg_b, "Values are not equal"); + 1 + } + // Use the standard context interface to emit a new nullifier #[aztec(public-vm)] fn new_nullifier(nullifier: Field) { diff --git a/yarn-project/simulator/src/avm/avm_simulator.test.ts b/yarn-project/simulator/src/avm/avm_simulator.test.ts index 73afe6c07ed..9e21712d017 100644 --- a/yarn-project/simulator/src/avm/avm_simulator.test.ts +++ b/yarn-project/simulator/src/avm/avm_simulator.test.ts @@ -842,7 +842,23 @@ describe('AVM simulator: transpiled Noir contracts', () => { const results = await new AvmSimulator(context).executeBytecode(callBytecode); expect(results.reverted).toBe(true); // The outer call should revert. - expect(results.revertReason?.message).toMatch(/Nested static call failed/); + expect(results.revertReason?.message).toEqual('Static calls cannot alter storage'); + }); + + it(`Nested calls rethrow exceptions`, async () => { + const calldata: Fr[] = [new Fr(1), new Fr(2)]; + const callBytecode = getAvmNestedCallsTestContractBytecode('nested_call_to_add'); + // We actually don't pass the function ADD, but it's ok because the signature is the same. + const nestedBytecode = getAvmNestedCallsTestContractBytecode('assert_same'); + const context = initContext({ env: initExecutionEnvironment({ calldata }) }); + jest + .spyOn(context.persistableState.hostStorage.contractsDb, 'getBytecode') + .mockReturnValue(Promise.resolve(nestedBytecode)); + + const results = await new AvmSimulator(context).executeBytecode(callBytecode); + + expect(results.reverted).toBe(true); // The outer call should revert. + expect(results.revertReason?.message).toEqual('Assertion failed: Values are not equal'); }); }); }); 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 bf052349410..d19f5b833e0 100644 --- a/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts +++ b/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts @@ -205,31 +205,25 @@ describe('External Calls', () => { it('Should fail if a static call attempts to touch storage', async () => { const gasOffset = 0; - const gas = new Field(0); - const addrOffset = 1; + const gas = [new Field(0n), new Field(0n), new Field(0n)]; + const addrOffset = 10; const addr = new Field(123456n); - const argsOffset = 2; + const argsOffset = 20; const args = [new Field(1n), new Field(2n), new Field(3n)]; const argsSize = args.length; - const argsSizeOffset = 20; - const retOffset = 8; + const argsSizeOffset = 40; + const retOffset = 80; const retSize = 2; - const successOffset = 7; + const successOffset = 70; - context.machineState.memory.set(0, gas); - context.machineState.memory.set(1, addr); + context.machineState.memory.setSlice(gasOffset, gas); + context.machineState.memory.set(addrOffset, addr); context.machineState.memory.set(argsSizeOffset, new Uint32(argsSize)); - context.machineState.memory.setSlice(2, args); + context.machineState.memory.setSlice(argsOffset, args); const otherContextInstructions: Instruction[] = [ - new CalldataCopy( - /*indirect=*/ 0, - /*csOffset=*/ adjustCalldataIndex(0), - /*copySize=*/ argsSize, - /*dstOffset=*/ 0, - ), - new SStore(/*indirect=*/ 0, /*srcOffset=*/ 1, /*size=*/ 1, /*slotOffset=*/ 0), + new SStore(/*indirect=*/ 0, /*srcOffset=*/ 0, /*size=*/ 0, /*slotOffset=*/ 0), ]; const otherContextInstructionsBytecode = markBytecodeAsAvm(encodeToBytecode(otherContextInstructions)); @@ -249,11 +243,7 @@ describe('External Calls', () => { successOffset, /*temporaryFunctionSelectorOffset=*/ 0, ); - await instruction.execute(context); - - // No revert has occurred, but the nested execution has failed - const successValue = context.machineState.memory.get(successOffset); - expect(successValue).toEqual(new Uint8(0n)); + await expect(() => instruction.execute(context)).rejects.toThrow(/Static calls cannot alter storage/); }); }); diff --git a/yarn-project/simulator/src/avm/opcodes/external_calls.ts b/yarn-project/simulator/src/avm/opcodes/external_calls.ts index 2fa2f02ddfc..aec6d12769b 100644 --- a/yarn-project/simulator/src/avm/opcodes/external_calls.ts +++ b/yarn-project/simulator/src/avm/opcodes/external_calls.ts @@ -7,6 +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 { Opcode, OperandType } from '../serialization/instruction_serialization.js'; import { Addressing } from './addressing_mode.js'; import { Instruction } from './instruction.js'; @@ -99,6 +100,18 @@ abstract class ExternalCall extends Instruction { const success = !nestedCallResults.reverted; + // TRANSITIONAL: We rethrow here so that the MESSAGE gets propagated. + if (!success) { + class RethrowedError extends AvmExecutionError { + constructor(message: string) { + super(message); + this.name = 'RethrowedError'; + } + } + + throw new RethrowedError(nestedCallResults.revertReason?.message || 'Unknown nested call error'); + } + // We only take as much data as was specified in the return size and pad with zeroes if the return data is smaller // than the specified size in order to prevent that memory to be left with garbage const returnData = nestedCallResults.output.slice(0, this.retSize); From bf844902eea45a7a2634f1295b723e54009eae6a Mon Sep 17 00:00:00 2001 From: fcarreiro Date: Wed, 8 May 2024 15:49:33 +0000 Subject: [PATCH 2/2] change name --- yarn-project/simulator/src/avm/opcodes/external_calls.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/yarn-project/simulator/src/avm/opcodes/external_calls.ts b/yarn-project/simulator/src/avm/opcodes/external_calls.ts index aec6d12769b..1cf06ce5fcb 100644 --- a/yarn-project/simulator/src/avm/opcodes/external_calls.ts +++ b/yarn-project/simulator/src/avm/opcodes/external_calls.ts @@ -102,14 +102,14 @@ abstract class ExternalCall extends Instruction { // TRANSITIONAL: We rethrow here so that the MESSAGE gets propagated. if (!success) { - class RethrowedError extends AvmExecutionError { + class RethrownError extends AvmExecutionError { constructor(message: string) { super(message); - this.name = 'RethrowedError'; + this.name = 'RethrownError'; } } - throw new RethrowedError(nestedCallResults.revertReason?.message || 'Unknown nested call error'); + throw new RethrownError(nestedCallResults.revertReason?.message || 'Unknown nested call error'); } // We only take as much data as was specified in the return size and pad with zeroes if the return data is smaller