Skip to content

Commit

Permalink
fix(avm-simulator): nested calls should preserve static context (#6414)
Browse files Browse the repository at this point in the history
Also change error messages to conform to the current ones. Makes all
`e2e_static_calls.test.ts` tests pass under the AVM.

Thanks @Thunkar for the heads up and fix.

Closes #6370.
  • Loading branch information
fcarreiro authored May 15, 2024
1 parent 116eef0 commit 44d7916
Show file tree
Hide file tree
Showing 8 changed files with 33 additions and 28 deletions.
4 changes: 3 additions & 1 deletion yarn-project/simulator/src/avm/avm_simulator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -880,7 +880,9 @@ 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).toEqual('Static calls cannot alter storage');
expect(results.revertReason?.message).toEqual(
'Static call cannot update the state, emit L2->L1 messages or generate logs',
);
});

it(`Nested calls rethrow exceptions`, async () => {
Expand Down
10 changes: 10 additions & 0 deletions yarn-project/simulator/src/avm/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,16 @@ export class OutOfGasError extends AvmExecutionError {
}
}

/**
* Error is thrown when a static call attempts to alter some state
*/
export class StaticCallAlterationError extends InstructionExecutionError {
constructor() {
super('Static call cannot update the state, emit L2->L1 messages or generate logs');
this.name = 'StaticCallAlterationError';
}
}

/**
* Error thrown to propagate a nested call's revert.
* @param message - the error's message
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { mock } from 'jest-mock-extended';
import { type CommitmentsDB } from '../../index.js';
import { type AvmContext } from '../avm_context.js';
import { Field, Uint8 } from '../avm_memory_types.js';
import { InstructionExecutionError } from '../errors.js';
import { InstructionExecutionError, StaticCallAlterationError } from '../errors.js';
import { initContext, initExecutionEnvironment, initHostStorage } from '../fixtures/index.js';
import { AvmPersistableStateManager } from '../journal/journal.js';
import {
Expand All @@ -19,7 +19,6 @@ import {
NullifierExists,
SendL2ToL1Message,
} from './accrued_substate.js';
import { StaticCallStorageAlterError } from './storage.js';

describe('Accrued Substate', () => {
let context: AvmContext;
Expand Down Expand Up @@ -482,7 +481,7 @@ describe('Accrued Substate', () => {
];

for (const instruction of instructions) {
await expect(instruction.execute(context)).rejects.toThrow(StaticCallStorageAlterError);
await expect(instruction.execute(context)).rejects.toThrow(StaticCallAlterationError);
}
});
});
11 changes: 5 additions & 6 deletions yarn-project/simulator/src/avm/opcodes/accrued_substate.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
import type { AvmContext } from '../avm_context.js';
import { Uint8 } from '../avm_memory_types.js';
import { InstructionExecutionError } from '../errors.js';
import { InstructionExecutionError, StaticCallAlterationError } from '../errors.js';
import { NullifierCollisionError } from '../journal/nullifiers.js';
import { Opcode, OperandType } from '../serialization/instruction_serialization.js';
import { Addressing } from './addressing_mode.js';
import { Instruction } from './instruction.js';
import { StaticCallStorageAlterError } from './storage.js';

export class NoteHashExists extends Instruction {
static type: string = 'NOTEHASHEXISTS';
Expand Down Expand Up @@ -65,7 +64,7 @@ export class EmitNoteHash extends Instruction {
context.machineState.consumeGas(this.gasCost(memoryOperations));

if (context.environment.isStaticCall) {
throw new StaticCallStorageAlterError();
throw new StaticCallAlterationError();
}

const noteHash = memory.get(this.noteHashOffset).toFr();
Expand Down Expand Up @@ -125,7 +124,7 @@ export class EmitNullifier extends Instruction {

public async execute(context: AvmContext): Promise<void> {
if (context.environment.isStaticCall) {
throw new StaticCallStorageAlterError();
throw new StaticCallAlterationError();
}

const memoryOperations = { reads: 1, indirect: this.indirect };
Expand Down Expand Up @@ -210,7 +209,7 @@ export class EmitUnencryptedLog extends Instruction {

public async execute(context: AvmContext): Promise<void> {
if (context.environment.isStaticCall) {
throw new StaticCallStorageAlterError();
throw new StaticCallAlterationError();
}

const memoryOperations = { reads: 1 + this.logSize, indirect: this.indirect };
Expand Down Expand Up @@ -244,7 +243,7 @@ export class SendL2ToL1Message extends Instruction {

public async execute(context: AvmContext): Promise<void> {
if (context.environment.isStaticCall) {
throw new StaticCallStorageAlterError();
throw new StaticCallAlterationError();
}

const memoryOperations = { reads: 2, indirect: this.indirect };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,9 @@ describe('External Calls', () => {
successOffset,
/*temporaryFunctionSelectorOffset=*/ 0,
);
await expect(() => instruction.execute(context)).rejects.toThrow(/Static calls cannot alter storage/);
await expect(() => instruction.execute(context)).rejects.toThrow(
'Static call cannot update the state, emit L2->L1 messages or generate logs',
);
});
});

Expand Down
8 changes: 5 additions & 3 deletions yarn-project/simulator/src/avm/opcodes/external_calls.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ abstract class ExternalCall extends Instruction {
// Function selector is temporary since eventually public contract bytecode will be one blob
// containing all functions, and function selector will become an application-level mechanism
// (e.g. first few bytes of calldata + compiler-generated jump table)
private temporaryFunctionSelectorOffset: number,
private functionSelectorOffset: number,
) {
super();
}
Expand All @@ -59,7 +59,9 @@ abstract class ExternalCall extends Instruction {
const calldata = memory.getSlice(argsOffset, calldataSize).map(f => f.toFr());
const l2Gas = memory.get(gasOffset).toNumber();
const daGas = memory.getAs<Field>(gasOffset + 1).toNumber();
const functionSelector = memory.getAs<Field>(this.temporaryFunctionSelectorOffset).toFr();
const functionSelector = memory.getAs<Field>(this.functionSelectorOffset).toFr();
// If we are already in a static call, we propagate the environment.
const callType = context.environment.isStaticCall ? 'STATICCALL' : this.type;

const allocatedGas = { l2Gas, daGas };
const memoryOperations = { reads: calldataSize + 5, writes: 1 + this.retSize, indirect: this.indirect };
Expand All @@ -71,7 +73,7 @@ abstract class ExternalCall extends Instruction {
callAddress.toFr(),
calldata,
allocatedGas,
this.type,
callType,
FunctionSelector.fromField(functionSelector),
);
const startSideEffectCounter = nestedContext.persistableState.trace.accessCounter;
Expand Down
5 changes: 3 additions & 2 deletions yarn-project/simulator/src/avm/opcodes/storage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@ import { type MockProxy, mock } from 'jest-mock-extended';

import { type AvmContext } from '../avm_context.js';
import { Field } from '../avm_memory_types.js';
import { StaticCallAlterationError } from '../errors.js';
import { initContext, initExecutionEnvironment } from '../fixtures/index.js';
import { type AvmPersistableStateManager } from '../journal/journal.js';
import { SLoad, SStore, StaticCallStorageAlterError } from './storage.js';
import { SLoad, SStore } from './storage.js';

describe('Storage Instructions', () => {
let context: AvmContext;
Expand Down Expand Up @@ -68,7 +69,7 @@ describe('Storage Instructions', () => {

const instruction = () =>
new SStore(/*indirect=*/ 0, /*srcOffset=*/ 0, /*size=*/ 1, /*slotOffset=*/ 1).execute(context);
await expect(instruction()).rejects.toThrow(StaticCallStorageAlterError);
await expect(instruction()).rejects.toThrow(StaticCallAlterationError);
});
});

Expand Down
14 changes: 2 additions & 12 deletions yarn-project/simulator/src/avm/opcodes/storage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { Fr } from '@aztec/foundation/fields';
import type { AvmContext } from '../avm_context.js';
import { type Gas, getBaseGasCost, getMemoryGasCost, mulGas, sumGas } from '../avm_gas.js';
import { Field, type MemoryOperations } from '../avm_memory_types.js';
import { InstructionExecutionError } from '../errors.js';
import { StaticCallAlterationError } from '../errors.js';
import { Opcode, OperandType } from '../serialization/instruction_serialization.js';
import { Addressing } from './addressing_mode.js';
import { Instruction } from './instruction.js';
Expand Down Expand Up @@ -44,7 +44,7 @@ export class SStore extends BaseStorageInstruction {

public async execute(context: AvmContext): Promise<void> {
if (context.environment.isStaticCall) {
throw new StaticCallStorageAlterError();
throw new StaticCallAlterationError();
}

const memoryOperations = { reads: this.size + 1, indirect: this.indirect };
Expand Down Expand Up @@ -100,13 +100,3 @@ export class SLoad extends BaseStorageInstruction {
memory.assert(memoryOperations);
}
}

/**
* Error is thrown when a static call attempts to alter storage
*/
export class StaticCallStorageAlterError extends InstructionExecutionError {
constructor() {
super('Static calls cannot alter storage');
this.name = 'StaticCallStorageAlterError';
}
}

0 comments on commit 44d7916

Please sign in to comment.