From 1314a05c7317f266ba2d1ac669558e33b27c2063 Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Wed, 20 Nov 2024 02:25:53 +0000 Subject: [PATCH] feat: calls to non-existent contracts in the AVM return failure --- .../contracts/avm_test_contract/src/main.nr | 19 ++++++++++ .../end-to-end/src/e2e_avm_simulator.test.ts | 14 ++++++++ .../contract_class_registration.test.ts | 16 ++++++--- .../pxe/src/pxe_service/pxe_service.ts | 6 +++- .../simulator/src/avm/avm_simulator.test.ts | 36 ++++++++++++++++--- .../simulator/src/avm/avm_simulator.ts | 22 +++++++++--- .../src/avm/opcodes/external_calls.test.ts | 35 ++++++++++++++++++ .../src/avm/opcodes/external_calls.ts | 3 +- .../simulator/src/avm/opcodes/instruction.ts | 2 +- 9 files changed, 137 insertions(+), 16 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 bf1382bc773..cb2bd989254 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 @@ -30,6 +30,7 @@ contract AvmTest { // Libs use dep::aztec::context::gas::GasOpts; + use dep::aztec::context::public_context::call; use dep::aztec::macros::{functions::{private, public}, storage::storage}; use dep::aztec::oracle::get_contract_instance::{ get_contract_instance_class_id_avm, get_contract_instance_deployer_avm, @@ -515,6 +516,23 @@ contract AvmTest { /************************************************************************ * Nested calls ************************************************************************/ + #[public] + fn nested_call_to_nothing() { + let garbageAddress = AztecAddress::from_field(42); + AvmTest::at(garbageAddress).nested_call_to_nothing().call(&mut context) + } + + #[public] + fn nested_call_to_nothing_recovers() { + let garbageAddress = AztecAddress::from_field(42); + let gas = [1, 1]; + let success = call(gas, garbageAddress, &[]); + assert( + !success, + "Nested CALL instruction should return failure if target contract does not exist", + ); + } + #[public] fn nested_call_to_add_with_gas( arg_a: Field, @@ -644,5 +662,6 @@ contract AvmTest { //let _ = nested_call_to_add(1, 2); //dep::aztec::oracle::debug_log::debug_log("nested_static_call_to_add"); //let _ = nested_static_call_to_add(1, 2); + //let _ = nested_call_to_nothing_recovers(); } } 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 00a1c15d585..449a14eaf1b 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 @@ -155,6 +155,20 @@ describe('e2e_avm_simulator', () => { }); describe('Nested calls', () => { + it('Top-level call to non-existent contract reverts', async () => { + // The nested call reverts (returns failure), but the caller doesn't HAVE to rethrow. + const tx = await avmContract.methods.nested_call_to_nothing_recovers().send().wait(); + expect(tx.status).toEqual(TxStatus.SUCCESS); + }); + it('Nested call to non-existent contract reverts & rethrows by default', async () => { + // The nested call reverts and by default caller rethrows + await expect(avmContract.methods.nested_call_to_nothing().send().wait()).rejects.toThrow(/No bytecode/); + }); + it('Nested CALL instruction to non-existent contract returns failure, but caller can recover', async () => { + // The nested call reverts (returns failure), but the caller doesn't HAVE to rethrow. + const tx = await avmContract.methods.nested_call_to_nothing_recovers().send().wait(); + expect(tx.status).toEqual(TxStatus.SUCCESS); + }); it('Should NOT be able to emit the same unsiloed nullifier from the same contract', async () => { const nullifier = new Fr(1); await expect( diff --git a/yarn-project/end-to-end/src/e2e_deploy_contract/contract_class_registration.test.ts b/yarn-project/end-to-end/src/e2e_deploy_contract/contract_class_registration.test.ts index 9908dfecbec..b4e3340c074 100644 --- a/yarn-project/end-to-end/src/e2e_deploy_contract/contract_class_registration.test.ts +++ b/yarn-project/end-to-end/src/e2e_deploy_contract/contract_class_registration.test.ts @@ -257,13 +257,21 @@ describe('e2e_deploy_contract contract class registration', () => { }); describe('error scenarios in deployment', () => { - it('refuses to call a public function on an undeployed contract', async () => { + it('app logic call to an undeployed contract reverts, but can be included is not dropped', async () => { const whom = wallet.getAddress(); const outgoingViewer = whom; const instance = await t.registerContract(wallet, StatefulTestContract, { initArgs: [whom, outgoingViewer, 42] }); - await expect( - instance.methods.increment_public_value_no_init_check(whom, 10).send({ skipPublicSimulation: true }).wait(), - ).rejects.toThrow(/dropped/); + // Confirm that the tx reverts with the expected message + await expect(instance.methods.increment_public_value_no_init_check(whom, 10).send().wait()).rejects.toThrow( + /No bytecode/, + ); + // This time, don't throw on revert and confirm that the tx is included + // despite reverting in app logic because of the call to a non-existent contract + const tx = await instance.methods + .increment_public_value_no_init_check(whom, 10) + .send({ skipPublicSimulation: true }) + .wait({ dontThrowOnRevert: true }); + expect(tx.status).toEqual(TxStatus.APP_LOGIC_REVERTED); }); it('refuses to deploy an instance from a different deployer', () => { diff --git a/yarn-project/pxe/src/pxe_service/pxe_service.ts b/yarn-project/pxe/src/pxe_service/pxe_service.ts index 62e96081780..5c225053a2c 100644 --- a/yarn-project/pxe/src/pxe_service/pxe_service.ts +++ b/yarn-project/pxe/src/pxe_service/pxe_service.ts @@ -773,7 +773,11 @@ export class PXEService implements PXE { return result; } catch (err) { if (err instanceof SimulationError) { - await enrichPublicSimulationError(err, this.contractDataOracle, this.db, this.log); + try { + await enrichPublicSimulationError(err, this.contractDataOracle, this.db, this.log); + } catch (enrichErr) { + this.log.error(`Failed to enrich public simulation error: ${enrichErr}`); + } } throw err; } diff --git a/yarn-project/simulator/src/avm/avm_simulator.test.ts b/yarn-project/simulator/src/avm/avm_simulator.test.ts index 6697b235a65..4739107a393 100644 --- a/yarn-project/simulator/src/avm/avm_simulator.test.ts +++ b/yarn-project/simulator/src/avm/avm_simulator.test.ts @@ -188,6 +188,16 @@ describe('AVM simulator: transpiled Noir contracts', () => { expect(results.reverted).toBe(false); }); + + it('execution of a non-existent contract immediately reverts', async () => { + const context = initContext(); + const results = await new AvmSimulator(context).execute(); + + expect(results.reverted).toBe(true); + expect(results.output).toEqual([]); + expect(results.gasLeft).toEqual({ l2Gas: 0, daGas: 0 }); + }); + it('addition', async () => { const calldata: Fr[] = [new Fr(1), new Fr(2)]; const context = initContext({ env: initExecutionEnvironment({ calldata }) }); @@ -891,6 +901,7 @@ describe('AVM simulator: transpiled Noir contracts', () => { environment: AvmExecutionEnvironment, nestedTrace: PublicSideEffectTraceInterface, isStaticCall: boolean = false, + exists: boolean = true, ) => { expect(trace.traceNestedCall).toHaveBeenCalledTimes(1); expect(trace.traceNestedCall).toHaveBeenCalledWith( @@ -900,17 +911,34 @@ describe('AVM simulator: transpiled Noir contracts', () => { contractCallDepth: new Fr(1), // top call is depth 0, nested is depth 1 globals: environment.globals, // just confirming that nested env looks roughly right isStaticCall: isStaticCall, - // TODO(7121): can't check calldata like this since it is modified on environment construction - // with AvmContextInputs. These should eventually go away. - //calldata: expect.arrayContaining(environment.calldata), // top-level call forwards args + // top-level calls forward args in these tests, + // but nested calls in these tests use public_dispatch, so selector is inserted as first arg + calldata: expect.arrayContaining([/*selector=*/ expect.anything(), ...environment.calldata]), }), /*startGasLeft=*/ expect.anything(), - /*bytecode=*/ expect.anything(), + /*bytecode=*/ exists ? expect.anything() : undefined, /*avmCallResults=*/ expect.anything(), // we don't have the NESTED call's results to check /*functionName=*/ expect.anything(), ); }; + it(`Nested call to non-existent contract`, async () => { + const calldata = [value0, value1]; + const context = createContext(calldata); + const callBytecode = getAvmTestContractBytecode('nested_call_to_add'); + // We don't mock getBytecode for the nested contract, so it will not exist + // which should cause the nested call to immediately revert + + const nestedTrace = mock(); + mockTraceFork(trace, nestedTrace); + + const results = await new AvmSimulator(context).executeBytecode(callBytecode); + expect(results.reverted).toBe(true); + expect(results.output).toEqual([]); + + expectTracedNestedCall(context.environment, nestedTrace, /*isStaticCall=*/ false, /*exists=*/ false); + }); + it(`Nested call`, async () => { const calldata = [value0, value1]; const context = createContext(calldata); diff --git a/yarn-project/simulator/src/avm/avm_simulator.ts b/yarn-project/simulator/src/avm/avm_simulator.ts index 37c5e82feeb..24eb0dafe2a 100644 --- a/yarn-project/simulator/src/avm/avm_simulator.ts +++ b/yarn-project/simulator/src/avm/avm_simulator.ts @@ -18,8 +18,8 @@ import { AvmMachineState } from './avm_machine_state.js'; import { isAvmBytecode } from './bytecode_utils.js'; import { AvmExecutionError, + AvmRevertReason, InvalidProgramCounterError, - NoBytecodeForContractError, revertReasonFromExceptionalHalt, revertReasonFromExplicitRevert, } from './errors.js'; @@ -83,10 +83,24 @@ export class AvmSimulator { public async execute(): Promise { const bytecode = await this.context.persistableState.getBytecode(this.context.environment.address); - // This assumes that we will not be able to send messages to accounts without code - // Pending classes and instances impl details if (!bytecode) { - throw new NoBytecodeForContractError(this.context.environment.address); + // revert, consuming all gas + const message = `No bytecode found at: ${this.context.environment.address}. Reverting...`; + const revertReason = new AvmRevertReason( + message, + /*failingFunction=*/ { + contractAddress: this.context.environment.address, + functionSelector: this.context.environment.functionSelector, + }, + /*noirCallStack=*/ [], + ); + this.log.warn(message); + return new AvmContractCallResult( + /*reverted=*/ true, + /*output=*/ [], + /*gasLeft=*/ { l2Gas: 0, daGas: 0 }, + revertReason, + ); } return await this.executeBytecode(bytecode); 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 534a45102b7..24605b642da 100644 --- a/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts +++ b/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts @@ -58,6 +58,41 @@ describe('External Calls', () => { expect(inst.serialize()).toEqual(buf); }); + it('Call to non-existent bytecode returns failure', async () => { + const gasOffset = 0; + const l2Gas = 2e6; + const daGas = 3e6; + const addrOffset = 2; + const addr = new Fr(123456n); + const argsOffset = 3; + const args = [new Field(1), new Field(2), new Field(3)]; + const argsSize = args.length; + const argsSizeOffset = 20; + const successOffset = 6; + + const { l2GasLeft: initialL2Gas, daGasLeft: initialDaGas } = context.machineState; + + context.machineState.memory.set(0, new Field(l2Gas)); + context.machineState.memory.set(1, new Field(daGas)); + context.machineState.memory.set(2, new Field(addr)); + context.machineState.memory.set(argsSizeOffset, new Uint32(argsSize)); + context.machineState.memory.setSlice(3, args); + + const instruction = new Call(/*indirect=*/ 0, gasOffset, addrOffset, argsOffset, argsSizeOffset, successOffset); + await instruction.execute(context); + + const successValue = context.machineState.memory.get(successOffset); + expect(successValue).toEqual(new Uint1(0n)); // failure, contract non-existent! + + const retValue = context.machineState.nestedReturndata; + expect(retValue).toEqual([]); + + // should charge for the CALL instruction itself, and all allocated gas should be consumed + expect(context.machineState.l2GasLeft).toBeLessThan(initialL2Gas - l2Gas); + expect(context.machineState.daGasLeft).toEqual(initialDaGas - daGas); + expect(context.machineState.collectedRevertInfo?.recursiveRevertReason?.message).toMatch(/No bytecode found/); + }); + it('Should execute a call correctly', async () => { const gasOffset = 0; const l2Gas = 2e6; diff --git a/yarn-project/simulator/src/avm/opcodes/external_calls.ts b/yarn-project/simulator/src/avm/opcodes/external_calls.ts index 33b75009017..a5cb97ebc57 100644 --- a/yarn-project/simulator/src/avm/opcodes/external_calls.ts +++ b/yarn-project/simulator/src/avm/opcodes/external_calls.ts @@ -2,7 +2,6 @@ import { Fr, FunctionSelector, Gas, PUBLIC_DISPATCH_SELECTOR } from '@aztec/circ import type { AvmContext } from '../avm_context.js'; 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 { Opcode, OperandType } from '../serialization/instruction_serialization.js'; @@ -95,7 +94,7 @@ abstract class ExternalCall extends Instruction { memory.set(successOffset, new Uint1(success ? 1 : 0)); // Refund unused gas - context.machineState.refundGas(gasLeftToGas(nestedContext.machineState)); + context.machineState.refundGas(nestedCallResults.gasLeft); // Merge nested call's state and trace based on whether it succeeded. if (success) { diff --git a/yarn-project/simulator/src/avm/opcodes/instruction.ts b/yarn-project/simulator/src/avm/opcodes/instruction.ts index c7ae85a5d84..2b7a0e8f551 100644 --- a/yarn-project/simulator/src/avm/opcodes/instruction.ts +++ b/yarn-project/simulator/src/avm/opcodes/instruction.ts @@ -94,7 +94,7 @@ export abstract class Instruction { * Computes gas cost for the instruction based on its base cost and memory operations. * @returns Gas cost. */ - protected gasCost(dynMultiplier: number = 0): Gas { + public gasCost(dynMultiplier: number = 0): Gas { const baseGasCost = getBaseGasCost(this.opcode); const dynGasCost = mulGas(getDynamicGasCost(this.opcode), dynMultiplier); return sumGas(baseGasCost, dynGasCost);