Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: calls to non-existent contracts in the AVM simulator return failure #10051

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 41 additions & 4 deletions yarn-project/simulator/src/avm/avm_simulator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,27 @@ 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 startGas = { l2Gas: context.machineState.l2GasLeft, daGas: context.machineState.daGasLeft };
const results = await new AvmSimulator(context).execute();

expect(results.reverted).toBe(true);
expect(results.output).toEqual([]);
expect(results.gasLeft).toEqual(startGas);
});

it('execution of a non-existent contract immediately reverts', async () => {
const context = initContext();
const startGas = { l2Gas: context.machineState.l2GasLeft, daGas: context.machineState.daGasLeft };
const results = await new AvmSimulator(context).execute();

expect(results.reverted).toBe(true);
expect(results.output).toEqual([]);
expect(results.gasLeft).toEqual(startGas);
});

it('addition', async () => {
const calldata: Fr[] = [new Fr(1), new Fr(2)];
const context = initContext({ env: initExecutionEnvironment({ calldata }) });
Expand Down Expand Up @@ -891,6 +912,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(
Expand All @@ -900,17 +922,32 @@ 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');

const nestedTrace = mock<PublicSideEffectTraceInterface>();
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);
});

Comment on lines +936 to +950
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing non-existent contract by not mocking getBytecode

it(`Nested call`, async () => {
const calldata = [value0, value1];
const context = createContext(calldata);
Expand Down
22 changes: 18 additions & 4 deletions yarn-project/simulator/src/avm/avm_simulator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -83,10 +83,24 @@ export class AvmSimulator {
public async execute(): Promise<AvmContractCallResult> {
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);
Comment on lines 86 to -89
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we "throw" then we'd need to wrap calls to the AVM in try-catch. It would be fine for external calls, but breaks things for enqueued calls.

// revert without consuming any 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=*/ this.context.machineState.gasLeft,
revertReason,
);
}
Comment on lines -86 to 104
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it bad to hand-craft an AvmRevertReason like this?


return await this.executeBytecode(bytecode);
Expand Down
36 changes: 36 additions & 0 deletions yarn-project/simulator/src/avm/opcodes/external_calls.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,42 @@ 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 only charge gas for the call instruction itself
// (all gas allocated to the nested call should be refunded)
const callGasCost = instruction.gasCost(argsSize);
expect(context.machineState.l2GasLeft).toEqual(initialL2Gas - callGasCost.l2Gas);
expect(context.machineState.daGasLeft).toEqual(initialDaGas - callGasCost.daGas);
Comment on lines +90 to +94
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed instruction.gasCost from protected to public so that I can confirm here that the external call is charging no more than the cost of the call opcode itself. Is that an acceptable decision?

});

it('Should execute a call correctly', async () => {
const gasOffset = 0;
const l2Gas = 2e6;
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/simulator/src/avm/opcodes/instruction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading