From 44d79163a4ded1f24463a7cee8306b303f98d266 Mon Sep 17 00:00:00 2001 From: Facundo Date: Wed, 15 May 2024 12:44:36 +0100 Subject: [PATCH] fix(avm-simulator): nested calls should preserve static context (#6414) 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. --- .../simulator/src/avm/avm_simulator.test.ts | 4 +++- yarn-project/simulator/src/avm/errors.ts | 10 ++++++++++ .../src/avm/opcodes/accrued_substate.test.ts | 5 ++--- .../simulator/src/avm/opcodes/accrued_substate.ts | 11 +++++------ .../src/avm/opcodes/external_calls.test.ts | 4 +++- .../simulator/src/avm/opcodes/external_calls.ts | 8 +++++--- .../simulator/src/avm/opcodes/storage.test.ts | 5 +++-- yarn-project/simulator/src/avm/opcodes/storage.ts | 14 ++------------ 8 files changed, 33 insertions(+), 28 deletions(-) diff --git a/yarn-project/simulator/src/avm/avm_simulator.test.ts b/yarn-project/simulator/src/avm/avm_simulator.test.ts index 0d8fe997467..4190e3eaeae 100644 --- a/yarn-project/simulator/src/avm/avm_simulator.test.ts +++ b/yarn-project/simulator/src/avm/avm_simulator.test.ts @@ -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 () => { diff --git a/yarn-project/simulator/src/avm/errors.ts b/yarn-project/simulator/src/avm/errors.ts index 90117085d3c..46f759fc736 100644 --- a/yarn-project/simulator/src/avm/errors.ts +++ b/yarn-project/simulator/src/avm/errors.ts @@ -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 diff --git a/yarn-project/simulator/src/avm/opcodes/accrued_substate.test.ts b/yarn-project/simulator/src/avm/opcodes/accrued_substate.test.ts index 376f64a8cd9..7934a880ad6 100644 --- a/yarn-project/simulator/src/avm/opcodes/accrued_substate.test.ts +++ b/yarn-project/simulator/src/avm/opcodes/accrued_substate.test.ts @@ -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 { @@ -19,7 +19,6 @@ import { NullifierExists, SendL2ToL1Message, } from './accrued_substate.js'; -import { StaticCallStorageAlterError } from './storage.js'; describe('Accrued Substate', () => { let context: AvmContext; @@ -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); } }); }); diff --git a/yarn-project/simulator/src/avm/opcodes/accrued_substate.ts b/yarn-project/simulator/src/avm/opcodes/accrued_substate.ts index 7030fea54f6..820d937bd23 100644 --- a/yarn-project/simulator/src/avm/opcodes/accrued_substate.ts +++ b/yarn-project/simulator/src/avm/opcodes/accrued_substate.ts @@ -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'; @@ -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(); @@ -125,7 +124,7 @@ export class EmitNullifier extends Instruction { public async execute(context: AvmContext): Promise { if (context.environment.isStaticCall) { - throw new StaticCallStorageAlterError(); + throw new StaticCallAlterationError(); } const memoryOperations = { reads: 1, indirect: this.indirect }; @@ -210,7 +209,7 @@ export class EmitUnencryptedLog extends Instruction { public async execute(context: AvmContext): Promise { if (context.environment.isStaticCall) { - throw new StaticCallStorageAlterError(); + throw new StaticCallAlterationError(); } const memoryOperations = { reads: 1 + this.logSize, indirect: this.indirect }; @@ -244,7 +243,7 @@ export class SendL2ToL1Message extends Instruction { public async execute(context: AvmContext): Promise { if (context.environment.isStaticCall) { - throw new StaticCallStorageAlterError(); + throw new StaticCallAlterationError(); } const memoryOperations = { reads: 2, indirect: this.indirect }; 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 177bc29a890..01dcade9354 100644 --- a/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts +++ b/yarn-project/simulator/src/avm/opcodes/external_calls.test.ts @@ -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', + ); }); }); diff --git a/yarn-project/simulator/src/avm/opcodes/external_calls.ts b/yarn-project/simulator/src/avm/opcodes/external_calls.ts index 479d61e27bc..21d96882ed9 100644 --- a/yarn-project/simulator/src/avm/opcodes/external_calls.ts +++ b/yarn-project/simulator/src/avm/opcodes/external_calls.ts @@ -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(); } @@ -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(gasOffset + 1).toNumber(); - const functionSelector = memory.getAs(this.temporaryFunctionSelectorOffset).toFr(); + const functionSelector = memory.getAs(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 }; @@ -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; diff --git a/yarn-project/simulator/src/avm/opcodes/storage.test.ts b/yarn-project/simulator/src/avm/opcodes/storage.test.ts index 79b0bb3f0d7..2bd18ebc197 100644 --- a/yarn-project/simulator/src/avm/opcodes/storage.test.ts +++ b/yarn-project/simulator/src/avm/opcodes/storage.test.ts @@ -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; @@ -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); }); }); diff --git a/yarn-project/simulator/src/avm/opcodes/storage.ts b/yarn-project/simulator/src/avm/opcodes/storage.ts index 62c3d180b0f..1f2f63a48bd 100644 --- a/yarn-project/simulator/src/avm/opcodes/storage.ts +++ b/yarn-project/simulator/src/avm/opcodes/storage.ts @@ -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'; @@ -44,7 +44,7 @@ export class SStore extends BaseStorageInstruction { public async execute(context: AvmContext): Promise { if (context.environment.isStaticCall) { - throw new StaticCallStorageAlterError(); + throw new StaticCallAlterationError(); } const memoryOperations = { reads: this.size + 1, indirect: this.indirect }; @@ -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'; - } -}