From f04ef7223e992b77eb7a557ffa732b89a0de7377 Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Mon, 30 Sep 2024 18:19:24 +0000 Subject: [PATCH 1/2] feat: enforce limits for each side effect type in AVM --- .../simulator/src/public/execution.ts | 2 - .../src/public/side_effect_errors.ts | 6 + .../src/public/side_effect_trace.test.ts | 147 +++++++++++++++++- .../simulator/src/public/side_effect_trace.ts | 97 ++++++++---- .../src/public/side_effect_trace_interface.ts | 1 + 5 files changed, 222 insertions(+), 31 deletions(-) create mode 100644 yarn-project/simulator/src/public/side_effect_errors.ts diff --git a/yarn-project/simulator/src/public/execution.ts b/yarn-project/simulator/src/public/execution.ts index 2312af9898f..216bd09eeca 100644 --- a/yarn-project/simulator/src/public/execution.ts +++ b/yarn-project/simulator/src/public/execution.ts @@ -86,8 +86,6 @@ export interface PublicExecutionResult { */ allUnencryptedLogs: UnencryptedFunctionL2Logs; - // TODO(dbanks12): add contract instance read requests - /** The requests to call public functions made by this call. */ publicCallRequests: PublicInnerCallRequest[]; /** The results of nested calls. */ diff --git a/yarn-project/simulator/src/public/side_effect_errors.ts b/yarn-project/simulator/src/public/side_effect_errors.ts new file mode 100644 index 00000000000..4953c72d2bc --- /dev/null +++ b/yarn-project/simulator/src/public/side_effect_errors.ts @@ -0,0 +1,6 @@ +export class SideEffectLimitReachedError extends Error { + constructor(sideEffectType: string, limit: number) { + super(`Reached the limit on number of '${sideEffectType}' side effects: ${limit}`); + this.name = 'SideEffectLimitReachedError'; + } +} diff --git a/yarn-project/simulator/src/public/side_effect_trace.test.ts b/yarn-project/simulator/src/public/side_effect_trace.test.ts index 5ee1469f161..ff5a9efcff1 100644 --- a/yarn-project/simulator/src/public/side_effect_trace.test.ts +++ b/yarn-project/simulator/src/public/side_effect_trace.test.ts @@ -1,5 +1,20 @@ import { UnencryptedL2Log } from '@aztec/circuit-types'; -import { AztecAddress, EthAddress, Gas, L2ToL1Message } from '@aztec/circuits.js'; +import { + AztecAddress, + EthAddress, + Gas, + L2ToL1Message, + MAX_L1_TO_L2_MSG_READ_REQUESTS_PER_TX, + MAX_L2_TO_L1_MSGS_PER_TX, + MAX_NOTE_HASHES_PER_TX, + MAX_NOTE_HASH_READ_REQUESTS_PER_TX, + MAX_NULLIFIERS_PER_TX, + MAX_NULLIFIER_NON_EXISTENT_READ_REQUESTS_PER_TX, + MAX_NULLIFIER_READ_REQUESTS_PER_TX, + MAX_PUBLIC_DATA_READS_PER_TX, + MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX, + MAX_UNENCRYPTED_LOGS_PER_TX, +} from '@aztec/circuits.js'; import { Fr } from '@aztec/foundation/fields'; import { SerializableContractInstance } from '@aztec/types/contracts'; @@ -7,6 +22,7 @@ import { randomBytes, randomInt } from 'crypto'; import { AvmContractCallResult } from '../avm/avm_contract_call_result.js'; import { initExecutionEnvironment } from '../avm/fixtures/index.js'; +import { SideEffectLimitReachedError } from './side_effect_errors.js'; import { PublicSideEffectTrace, type TracedContractInstance } from './side_effect_trace.js'; function randomTracedContractInstance(): TracedContractInstance { @@ -24,6 +40,7 @@ describe('Side Effect Trace', () => { const recipient = Fr.random(); const content = Fr.random(); const log = [Fr.random(), Fr.random(), Fr.random()]; + const contractInstance = SerializableContractInstance.empty().withAddress(new Fr(42)); const startGasLeft = Gas.fromFields([new Fr(randomInt(10000)), new Fr(randomInt(10000))]); const endGasLeft = Gas.fromFields([new Fr(randomInt(10000)), new Fr(randomInt(10000))]); @@ -221,8 +238,7 @@ describe('Side Effect Trace', () => { expect(trace.getCounter()).toBe(startCounterPlus1); const pxResult = toPxResult(trace); - // TODO(dbanks12): process contract instance read requests in public kernel - //expect(pxResult.gotContractInstances).toEqual([instance]); + // TODO(dbanks12): once this emits nullifier read, check here expect(pxResult.avmCircuitHints.contractInstances.items).toEqual([ { // hint omits "version" and has "exists" as an Fr @@ -231,6 +247,125 @@ describe('Side Effect Trace', () => { }, ]); }); + describe('Maximum accesses', () => { + it('Should enforce maximum number of public storage reads', () => { + for (let i = 0; i < MAX_PUBLIC_DATA_READS_PER_TX; i++) { + trace.tracePublicStorageRead(new Fr(i), new Fr(i), new Fr(i), true, true); + } + expect(() => trace.tracePublicStorageRead(new Fr(42), new Fr(42), new Fr(42), true, true)).toThrow( + SideEffectLimitReachedError, + ); + }); + + it('Should enforce maximum number of public storage writes', () => { + for (let i = 0; i < MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX; i++) { + trace.tracePublicStorageWrite(new Fr(i), new Fr(i), new Fr(i)); + } + expect(() => trace.tracePublicStorageWrite(new Fr(42), new Fr(42), new Fr(42))).toThrow( + SideEffectLimitReachedError, + ); + }); + + it('Should enforce maximum number of note hash checks', () => { + for (let i = 0; i < MAX_NOTE_HASH_READ_REQUESTS_PER_TX; i++) { + trace.traceNoteHashCheck(new Fr(i), new Fr(i), new Fr(i), true); + } + expect(() => trace.traceNoteHashCheck(new Fr(42), new Fr(42), new Fr(42), true)).toThrow( + SideEffectLimitReachedError, + ); + }); + + it('Should enforce maximum number of new note hashes', () => { + for (let i = 0; i < MAX_NOTE_HASHES_PER_TX; i++) { + trace.traceNewNoteHash(new Fr(i), new Fr(i)); + } + expect(() => trace.traceNewNoteHash(new Fr(42), new Fr(42))).toThrow(SideEffectLimitReachedError); + }); + + it('Should enforce maximum number of nullifier checks', () => { + for (let i = 0; i < MAX_NULLIFIER_READ_REQUESTS_PER_TX; i++) { + trace.traceNullifierCheck(new Fr(i), new Fr(i), new Fr(i), true, true); + } + expect(() => trace.traceNullifierCheck(new Fr(42), new Fr(42), new Fr(42), true, true)).toThrow( + SideEffectLimitReachedError, + ); + // NOTE: also cannot do a non-existent check once existent checks have filled up + expect(() => trace.traceNullifierCheck(new Fr(42), new Fr(42), new Fr(42), false, true)).toThrow( + SideEffectLimitReachedError, + ); + }); + + it('Should enforce maximum number of nullifier non-existent checks', () => { + for (let i = 0; i < MAX_NULLIFIER_NON_EXISTENT_READ_REQUESTS_PER_TX; i++) { + trace.traceNullifierCheck(new Fr(i), new Fr(i), new Fr(i), false, true); + } + expect(() => trace.traceNullifierCheck(new Fr(42), new Fr(42), new Fr(42), false, true)).toThrow( + SideEffectLimitReachedError, + ); + // NOTE: also cannot do a existent check once non-existent checks have filled up + expect(() => trace.traceNullifierCheck(new Fr(42), new Fr(42), new Fr(42), true, true)).toThrow( + SideEffectLimitReachedError, + ); + }); + + it('Should enforce maximum number of new nullifiers', () => { + for (let i = 0; i < MAX_NULLIFIERS_PER_TX; i++) { + trace.traceNewNullifier(new Fr(i), new Fr(i)); + } + expect(() => trace.traceNewNullifier(new Fr(42), new Fr(42))).toThrow(SideEffectLimitReachedError); + }); + + it('Should enforce maximum number of L1 to L2 message checks', () => { + for (let i = 0; i < MAX_L1_TO_L2_MSG_READ_REQUESTS_PER_TX; i++) { + trace.traceL1ToL2MessageCheck(new Fr(i), new Fr(i), new Fr(i), true); + } + expect(() => trace.traceL1ToL2MessageCheck(new Fr(42), new Fr(42), new Fr(42), true)).toThrow( + SideEffectLimitReachedError, + ); + }); + + it('Should enforce maximum number of new l2 to l1 messages', () => { + for (let i = 0; i < MAX_L2_TO_L1_MSGS_PER_TX; i++) { + trace.traceNewL2ToL1Message(new Fr(i), new Fr(i)); + } + expect(() => trace.traceNewL2ToL1Message(new Fr(42), new Fr(42))).toThrow(SideEffectLimitReachedError); + }); + + it('Should enforce maximum number of new logs hashes', () => { + for (let i = 0; i < MAX_UNENCRYPTED_LOGS_PER_TX; i++) { + trace.traceUnencryptedLog(new Fr(i), [new Fr(i), new Fr(i)]); + } + expect(() => trace.traceUnencryptedLog(new Fr(42), [new Fr(42), new Fr(42)])).toThrow( + SideEffectLimitReachedError, + ); + }); + + it('Should enforce maximum number of nullifier checks for GETCONTRACTINSTANCE', () => { + for (let i = 0; i < MAX_NULLIFIER_READ_REQUESTS_PER_TX; i++) { + trace.traceNullifierCheck(new Fr(i), new Fr(i), new Fr(i), true, true); + } + expect(() => trace.traceGetContractInstance({ ...contractInstance, exists: true })).toThrow( + SideEffectLimitReachedError, + ); + // NOTE: also cannot do a existent check once non-existent checks have filled up + expect(() => trace.traceGetContractInstance({ ...contractInstance, exists: false })).toThrow( + SideEffectLimitReachedError, + ); + }); + + it('Should enforce maximum number of nullifier non-existent checks for GETCONTRACTINSTANCE', () => { + for (let i = 0; i < MAX_NULLIFIER_NON_EXISTENT_READ_REQUESTS_PER_TX; i++) { + trace.traceNullifierCheck(new Fr(i), new Fr(i), new Fr(i), false, true); + } + expect(() => trace.traceGetContractInstance({ ...contractInstance, exists: false })).toThrow( + SideEffectLimitReachedError, + ); + // NOTE: also cannot do a existent check once non-existent checks have filled up + expect(() => trace.traceGetContractInstance({ ...contractInstance, exists: true })).toThrow( + SideEffectLimitReachedError, + ); + }); + }); it('Should trace nested calls', () => { const existsDefault = true; @@ -244,6 +379,7 @@ describe('Side Effect Trace', () => { nestedTrace.tracePublicStorageWrite(address, slot, value); testCounter++; nestedTrace.traceNoteHashCheck(address, utxo, leafIndex, existsDefault); + // counter does not increment for note hash checks nestedTrace.traceNewNoteHash(address, utxo); testCounter++; nestedTrace.traceNullifierCheck(address, utxo, leafIndex, /*exists=*/ true, isPending); @@ -253,10 +389,15 @@ describe('Side Effect Trace', () => { nestedTrace.traceNewNullifier(address, utxo); testCounter++; nestedTrace.traceL1ToL2MessageCheck(address, utxo, leafIndex, existsDefault); + // counter does not increment for l1tol2 message checks nestedTrace.traceNewL2ToL1Message(recipient, content); testCounter++; nestedTrace.traceUnencryptedLog(address, log); testCounter++; + nestedTrace.traceGetContractInstance({ ...contractInstance, exists: true }); + testCounter++; + nestedTrace.traceGetContractInstance({ ...contractInstance, exists: false }); + testCounter++; trace.traceNestedCall(nestedTrace, avmEnvironment, startGasLeft, endGasLeft, bytecode, avmCallResults); // parent trace adopts nested call's counter diff --git a/yarn-project/simulator/src/public/side_effect_trace.ts b/yarn-project/simulator/src/public/side_effect_trace.ts index 783e1c64ff6..c5a458788d0 100644 --- a/yarn-project/simulator/src/public/side_effect_trace.ts +++ b/yarn-project/simulator/src/public/side_effect_trace.ts @@ -12,6 +12,16 @@ import { Gas, L2ToL1Message, LogHash, + MAX_L1_TO_L2_MSG_READ_REQUESTS_PER_TX, + MAX_L2_TO_L1_MSGS_PER_TX, + MAX_NOTE_HASHES_PER_TX, + MAX_NOTE_HASH_READ_REQUESTS_PER_TX, + MAX_NULLIFIERS_PER_TX, + MAX_NULLIFIER_NON_EXISTENT_READ_REQUESTS_PER_TX, + MAX_NULLIFIER_READ_REQUESTS_PER_TX, + MAX_PUBLIC_DATA_READS_PER_TX, + MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX, + MAX_UNENCRYPTED_LOGS_PER_TX, NoteHash, Nullifier, type PublicInnerCallRequest, @@ -26,6 +36,7 @@ import { type AvmContractCallResult } from '../avm/avm_contract_call_result.js'; import { type AvmExecutionEnvironment } from '../avm/avm_execution_environment.js'; import { createSimulationError } from '../common/errors.js'; import { type PublicExecutionResult, resultToPublicCallRequest } from './execution.js'; +import { SideEffectLimitReachedError } from './side_effect_errors.js'; import { type PublicSideEffectTraceInterface } from './side_effect_trace_interface.js'; export type TracedContractInstance = { exists: boolean } & ContractInstanceWithAddress; @@ -55,8 +66,6 @@ export class PublicSideEffectTrace implements PublicSideEffectTraceInterface { private publicCallRequests: PublicInnerCallRequest[] = []; - private gotContractInstances: ContractInstanceWithAddress[] = []; - private nestedExecutions: PublicExecutionResult[] = []; private avmCircuitHints: AvmExecutionHints; @@ -81,10 +90,13 @@ export class PublicSideEffectTrace implements PublicSideEffectTraceInterface { this.sideEffectCounter++; } + // TODO(dbanks12): checks against tx-wide limit need access to parent trace's length + public tracePublicStorageRead(storageAddress: Fr, slot: Fr, value: Fr, _exists: boolean, _cached: boolean) { - // TODO(4805): check if some threshold is reached for max storage reads - // (need access to parent length, or trace needs to be initialized with parent's contents) // NOTE: exists and cached are unused for now but may be used for optimizations or kernel hints later + if (this.contractStorageReads.length >= MAX_PUBLIC_DATA_READS_PER_TX) { + throw new SideEffectLimitReachedError('contract storage read', MAX_PUBLIC_DATA_READS_PER_TX); + } this.contractStorageReads.push( new ContractStorageRead(slot, value, this.sideEffectCounter, AztecAddress.fromField(storageAddress)), ); @@ -96,8 +108,9 @@ export class PublicSideEffectTrace implements PublicSideEffectTraceInterface { } public tracePublicStorageWrite(storageAddress: Fr, slot: Fr, value: Fr) { - // TODO(4805): check if some threshold is reached for max storage writes - // (need access to parent length, or trace needs to be initialized with parent's contents) + if (this.contractStorageUpdateRequests.length >= MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX) { + throw new SideEffectLimitReachedError('contract storage write', MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX); + } this.contractStorageUpdateRequests.push( new ContractStorageUpdateRequest(slot, value, this.sideEffectCounter, storageAddress), ); @@ -107,32 +120,32 @@ export class PublicSideEffectTrace implements PublicSideEffectTraceInterface { // TODO(8287): _exists can be removed once we have the vm properly handling the equality check public traceNoteHashCheck(_storageAddress: Fr, noteHash: Fr, leafIndex: Fr, exists: boolean) { - // TODO(4805): check if some threshold is reached for max note hash checks // NOTE: storageAddress is unused but will be important when an AVM circuit processes an entire enqueued call - // TODO(dbanks12): leafIndex is unused for now but later must be used by kernel to constrain that the kernel - // is in fact checking the leaf indicated by the user + if (this.noteHashReadRequests.length >= MAX_NOTE_HASH_READ_REQUESTS_PER_TX) { + throw new SideEffectLimitReachedError('note hash read request', MAX_NOTE_HASH_READ_REQUESTS_PER_TX); + } this.noteHashReadRequests.push(new TreeLeafReadRequest(noteHash, leafIndex)); this.avmCircuitHints.noteHashExists.items.push( new AvmKeyValueHint(/*key=*/ new Fr(leafIndex), /*value=*/ exists ? Fr.ONE : Fr.ZERO), ); + // NOTE: counter does not increment for note hash checks (because it doesn't rely on pending note hashes) } public traceNewNoteHash(_storageAddress: Fr, noteHash: Fr) { - // TODO(4805): check if some threshold is reached for max new note hash - // NOTE: storageAddress is unused but will be important when an AVM circuit processes an entire enqueued call - // TODO(dbanks12): non-existent note hashes should emit a read request of the note hash that actually - // IS there, and the AVM circuit should accept THAT noteHash as a hint. The circuit will then compare - // the noteHash against the one provided by the user code to determine what to return to the user (exists or not), - // and will then propagate the actually-present noteHash to its public inputs. + if (this.noteHashes.length >= MAX_NOTE_HASHES_PER_TX) { + throw new SideEffectLimitReachedError('note hash', MAX_NOTE_HASHES_PER_TX); + } this.noteHashes.push(new NoteHash(noteHash, this.sideEffectCounter)); this.logger.debug(`NEW_NOTE_HASH cnt: ${this.sideEffectCounter}`); this.incrementSideEffectCounter(); } public traceNullifierCheck(_storageAddress: Fr, nullifier: Fr, _leafIndex: Fr, exists: boolean, _isPending: boolean) { - // TODO(4805): check if some threshold is reached for max new nullifier // NOTE: storageAddress is unused but will be important when an AVM circuit processes an entire enqueued call // NOTE: isPending and leafIndex are unused for now but may be used for optimizations or kernel hints later + + this.enforceLimitOnNullifierChecks(); + const readRequest = new ReadRequest(nullifier, this.sideEffectCounter); if (exists) { this.nullifierReadRequests.push(readRequest); @@ -147,8 +160,10 @@ export class PublicSideEffectTrace implements PublicSideEffectTraceInterface { } public traceNewNullifier(_storageAddress: Fr, nullifier: Fr) { - // TODO(4805): check if some threshold is reached for max new nullifier // NOTE: storageAddress is unused but will be important when an AVM circuit processes an entire enqueued call + if (this.nullifiers.length >= MAX_NULLIFIERS_PER_TX) { + throw new SideEffectLimitReachedError('nullifier', MAX_NULLIFIERS_PER_TX); + } this.nullifiers.push(new Nullifier(nullifier, this.sideEffectCounter, /*noteHash=*/ Fr.ZERO)); this.logger.debug(`NEW_NULLIFIER cnt: ${this.sideEffectCounter}`); this.incrementSideEffectCounter(); @@ -156,18 +171,21 @@ export class PublicSideEffectTrace implements PublicSideEffectTraceInterface { // TODO(8287): _exists can be removed once we have the vm properly handling the equality check public traceL1ToL2MessageCheck(_contractAddress: Fr, msgHash: Fr, msgLeafIndex: Fr, exists: boolean) { - // TODO(4805): check if some threshold is reached for max message reads // NOTE: contractAddress is unused but will be important when an AVM circuit processes an entire enqueued call - // TODO(dbanks12): leafIndex is unused for now but later must be used by kernel to constrain that the kernel - // is in fact checking the leaf indicated by the user + if (this.l1ToL2MsgReadRequests.length >= MAX_L1_TO_L2_MSG_READ_REQUESTS_PER_TX) { + throw new SideEffectLimitReachedError('l1 to l2 message read request', MAX_L1_TO_L2_MSG_READ_REQUESTS_PER_TX); + } this.l1ToL2MsgReadRequests.push(new TreeLeafReadRequest(msgHash, msgLeafIndex)); this.avmCircuitHints.l1ToL2MessageExists.items.push( new AvmKeyValueHint(/*key=*/ new Fr(msgLeafIndex), /*value=*/ exists ? Fr.ONE : Fr.ZERO), ); + // NOTE: counter does not increment for l1tol2 message checks (because it doesn't rely on pending messages) } public traceNewL2ToL1Message(recipient: Fr, content: Fr) { - // TODO(4805): check if some threshold is reached for max messages + if (this.newL2ToL1Messages.length >= MAX_L2_TO_L1_MSGS_PER_TX) { + throw new SideEffectLimitReachedError('l2 to l1 message', MAX_L2_TO_L1_MSGS_PER_TX); + } const recipientAddress = EthAddress.fromField(recipient); this.newL2ToL1Messages.push(new L2ToL1Message(recipientAddress, content, this.sideEffectCounter)); this.logger.debug(`NEW_L2_TO_L1_MSG cnt: ${this.sideEffectCounter}`); @@ -175,7 +193,9 @@ export class PublicSideEffectTrace implements PublicSideEffectTraceInterface { } public traceUnencryptedLog(contractAddress: Fr, log: Fr[]) { - // TODO(4805): check if some threshold is reached for max logs + if (this.unencryptedLogs.length >= MAX_UNENCRYPTED_LOGS_PER_TX) { + throw new SideEffectLimitReachedError('unencrypted log', MAX_UNENCRYPTED_LOGS_PER_TX); + } const ulog = new UnencryptedL2Log( AztecAddress.fromField(contractAddress), Buffer.concat(log.map(f => f.toBuffer())), @@ -191,8 +211,9 @@ export class PublicSideEffectTrace implements PublicSideEffectTraceInterface { } public traceGetContractInstance(instance: TracedContractInstance) { - // TODO(4805): check if some threshold is reached for max contract instance retrievals - this.gotContractInstances.push(instance); + this.enforceLimitOnNullifierChecks('(contract address nullifier from GETCONTRACTINSTANCE)'); + // TODO(dbanks12): should emit a nullifier read request + this.avmCircuitHints.contractInstances.items.push( new AvmContractInstanceHint( instance.address, @@ -228,6 +249,11 @@ export class PublicSideEffectTrace implements PublicSideEffectTraceInterface { /** Function name for logging */ functionName: string = 'unknown', ) { + // TODO(4805): check if some threshold is reached for max nested calls (to unique contracts?) + // TODO(dbanks12): should emit a nullifier read request. There should be two thresholds. + // one for max unique contract calls, and another based on max nullifier reads. + // Since this trace function happens _after_ a nested call, such threshold limits must take + // place in another trace function that occurs _before_ a nested call. const result = nestedCallTrace.toPublicExecutionResult( nestedEnvironment, startGasLeft, @@ -305,8 +331,6 @@ export class PublicSideEffectTrace implements PublicSideEffectTraceInterface { unencryptedLogs: new UnencryptedFunctionL2Logs(this.unencryptedLogs), allUnencryptedLogs: new UnencryptedFunctionL2Logs(this.allUnencryptedLogs), unencryptedLogsHashes: this.unencryptedLogsHashes, - // TODO(dbanks12): process contract instance read requests in public kernel - //gotContractInstances: this.gotContractInstances, publicCallRequests: this.publicCallRequests, nestedExecutions: this.nestedExecutions, @@ -316,6 +340,27 @@ export class PublicSideEffectTrace implements PublicSideEffectTraceInterface { functionName, }; } + + private enforceLimitOnNullifierChecks(errorMsgOrigin: string = '') { + // NOTE: Why error if _either_ limit was reached? If user code emits either an existent or non-existent + // nullifier read request (NULLIFIEREXISTS, GETCONTRACTINSTANCE, *CALL), and one of the limits has been + // reached (MAX_NULLIFIER_NON_EXISTENT_RRS vs MAX_NULLIFIER_RRS), but not the other, we must prevent the + // sequencer from lying and saying "this nullifier exists, but MAX_NULLIFIER_RRS has been reached, so I'm + // going to skip the read request and just revert instead" when the nullifier actually doesn't exist + // (or vice versa). So, if either maximum has been reached, any nullifier-reading operation must error. + if (this.nullifierReadRequests.length >= MAX_NULLIFIER_READ_REQUESTS_PER_TX) { + throw new SideEffectLimitReachedError( + `nullifier read request ${errorMsgOrigin}`, + MAX_NULLIFIER_READ_REQUESTS_PER_TX, + ); + } + if (this.nullifierNonExistentReadRequests.length >= MAX_NULLIFIER_NON_EXISTENT_READ_REQUESTS_PER_TX) { + throw new SideEffectLimitReachedError( + `nullifier non-existent read request ${errorMsgOrigin}`, + MAX_NULLIFIER_NON_EXISTENT_READ_REQUESTS_PER_TX, + ); + } + } } /** diff --git a/yarn-project/simulator/src/public/side_effect_trace_interface.ts b/yarn-project/simulator/src/public/side_effect_trace_interface.ts index 91326fb021b..b5a6c397b00 100644 --- a/yarn-project/simulator/src/public/side_effect_trace_interface.ts +++ b/yarn-project/simulator/src/public/side_effect_trace_interface.ts @@ -8,6 +8,7 @@ import { type TracedContractInstance } from './side_effect_trace.js'; export interface PublicSideEffectTraceInterface { fork(): PublicSideEffectTraceInterface; getCounter(): number; + // all "trace*" functions can throw SideEffectLimitReachedError tracePublicStorageRead(storageAddress: Fr, slot: Fr, value: Fr, exists: boolean, cached: boolean): void; tracePublicStorageWrite(storageAddress: Fr, slot: Fr, value: Fr): void; traceNoteHashCheck(storageAddress: Fr, noteHash: Fr, leafIndex: Fr, exists: boolean): void; From b7f624ee28861c83fa8c8d6860523717854f5c22 Mon Sep 17 00:00:00 2001 From: David Banks <47112877+dbanks12@users.noreply.github.com> Date: Tue, 1 Oct 2024 21:12:04 -0400 Subject: [PATCH 2/2] chore: add l2 contract address to public side effect tracing for l2tol1messages (#8917) --- yarn-project/simulator/src/avm/journal/journal.test.ts | 4 ++-- yarn-project/simulator/src/avm/journal/journal.ts | 7 ++++--- .../simulator/src/avm/opcodes/accrued_substate.test.ts | 2 +- .../simulator/src/avm/opcodes/accrued_substate.ts | 2 +- .../simulator/src/public/side_effect_trace.test.ts | 10 ++++++---- yarn-project/simulator/src/public/side_effect_trace.ts | 2 +- .../src/public/side_effect_trace_interface.ts | 3 +-- 7 files changed, 16 insertions(+), 14 deletions(-) diff --git a/yarn-project/simulator/src/avm/journal/journal.test.ts b/yarn-project/simulator/src/avm/journal/journal.test.ts index 07efb46a647..09e368f0100 100644 --- a/yarn-project/simulator/src/avm/journal/journal.test.ts +++ b/yarn-project/simulator/src/avm/journal/journal.test.ts @@ -139,9 +139,9 @@ describe('journal', () => { it('Should maintain l1 messages', () => { const recipient = new Fr(1); - persistableState.writeL2ToL1Message(recipient, utxo); + persistableState.writeL2ToL1Message(address, recipient, utxo); expect(trace.traceNewL2ToL1Message).toHaveBeenCalledTimes(1); - expect(trace.traceNewL2ToL1Message).toHaveBeenCalledWith(recipient, utxo); + expect(trace.traceNewL2ToL1Message).toHaveBeenCalledWith(address, recipient, utxo); }); }); diff --git a/yarn-project/simulator/src/avm/journal/journal.ts b/yarn-project/simulator/src/avm/journal/journal.ts index 843b6ed90ae..b26c4041838 100644 --- a/yarn-project/simulator/src/avm/journal/journal.ts +++ b/yarn-project/simulator/src/avm/journal/journal.ts @@ -187,12 +187,13 @@ export class AvmPersistableStateManager { /** * Write an L2 to L1 message. + * @param contractAddress - L2 contract address that created this message * @param recipient - L1 contract address to send the message to. * @param content - Message content. */ - public writeL2ToL1Message(recipient: Fr, content: Fr) { - this.log.debug(`L1Messages(${recipient}) += ${content}.`); - this.trace.traceNewL2ToL1Message(recipient, content); + public writeL2ToL1Message(contractAddress: Fr, recipient: Fr, content: Fr) { + this.log.debug(`L2ToL1Messages(${contractAddress}) += (recipient: ${recipient}, content: ${content}).`); + this.trace.traceNewL2ToL1Message(contractAddress, recipient, content); } /** 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 442bb985ba3..14841c01950 100644 --- a/yarn-project/simulator/src/avm/opcodes/accrued_substate.test.ts +++ b/yarn-project/simulator/src/avm/opcodes/accrued_substate.test.ts @@ -362,7 +362,7 @@ describe('Accrued Substate', () => { /*contentOffset=*/ value1Offset, ).execute(context); expect(trace.traceNewL2ToL1Message).toHaveBeenCalledTimes(1); - expect(trace.traceNewL2ToL1Message).toHaveBeenCalledWith(/*recipient=*/ value0, /*content=*/ value1); + expect(trace.traceNewL2ToL1Message).toHaveBeenCalledWith(address, /*recipient=*/ value0, /*content=*/ value1); }); }); diff --git a/yarn-project/simulator/src/avm/opcodes/accrued_substate.ts b/yarn-project/simulator/src/avm/opcodes/accrued_substate.ts index e7b16a6b4f0..e5c1b7c3b0b 100644 --- a/yarn-project/simulator/src/avm/opcodes/accrued_substate.ts +++ b/yarn-project/simulator/src/avm/opcodes/accrued_substate.ts @@ -268,7 +268,7 @@ export class SendL2ToL1Message extends Instruction { const recipient = memory.get(recipientOffset).toFr(); const content = memory.get(contentOffset).toFr(); - context.persistableState.writeL2ToL1Message(recipient, content); + context.persistableState.writeL2ToL1Message(context.environment.address, recipient, content); memory.assert({ reads: 2, addressing }); context.machineState.incrementPc(); diff --git a/yarn-project/simulator/src/public/side_effect_trace.test.ts b/yarn-project/simulator/src/public/side_effect_trace.test.ts index ff5a9efcff1..a0b4f16f709 100644 --- a/yarn-project/simulator/src/public/side_effect_trace.test.ts +++ b/yarn-project/simulator/src/public/side_effect_trace.test.ts @@ -207,7 +207,7 @@ describe('Side Effect Trace', () => { }); it('Should trace new L2ToL1 messages', () => { - trace.traceNewL2ToL1Message(recipient, content); + trace.traceNewL2ToL1Message(address, recipient, content); expect(trace.getCounter()).toBe(startCounterPlus1); const pxResult = toPxResult(trace); @@ -326,9 +326,11 @@ describe('Side Effect Trace', () => { it('Should enforce maximum number of new l2 to l1 messages', () => { for (let i = 0; i < MAX_L2_TO_L1_MSGS_PER_TX; i++) { - trace.traceNewL2ToL1Message(new Fr(i), new Fr(i)); + trace.traceNewL2ToL1Message(new Fr(i), new Fr(i), new Fr(i)); } - expect(() => trace.traceNewL2ToL1Message(new Fr(42), new Fr(42))).toThrow(SideEffectLimitReachedError); + expect(() => trace.traceNewL2ToL1Message(new Fr(42), new Fr(42), new Fr(42))).toThrow( + SideEffectLimitReachedError, + ); }); it('Should enforce maximum number of new logs hashes', () => { @@ -390,7 +392,7 @@ describe('Side Effect Trace', () => { testCounter++; nestedTrace.traceL1ToL2MessageCheck(address, utxo, leafIndex, existsDefault); // counter does not increment for l1tol2 message checks - nestedTrace.traceNewL2ToL1Message(recipient, content); + nestedTrace.traceNewL2ToL1Message(address, recipient, content); testCounter++; nestedTrace.traceUnencryptedLog(address, log); testCounter++; diff --git a/yarn-project/simulator/src/public/side_effect_trace.ts b/yarn-project/simulator/src/public/side_effect_trace.ts index c5a458788d0..5e0f8d51cf7 100644 --- a/yarn-project/simulator/src/public/side_effect_trace.ts +++ b/yarn-project/simulator/src/public/side_effect_trace.ts @@ -182,7 +182,7 @@ export class PublicSideEffectTrace implements PublicSideEffectTraceInterface { // NOTE: counter does not increment for l1tol2 message checks (because it doesn't rely on pending messages) } - public traceNewL2ToL1Message(recipient: Fr, content: Fr) { + public traceNewL2ToL1Message(_contractAddress: Fr, recipient: Fr, content: Fr) { if (this.newL2ToL1Messages.length >= MAX_L2_TO_L1_MSGS_PER_TX) { throw new SideEffectLimitReachedError('l2 to l1 message', MAX_L2_TO_L1_MSGS_PER_TX); } diff --git a/yarn-project/simulator/src/public/side_effect_trace_interface.ts b/yarn-project/simulator/src/public/side_effect_trace_interface.ts index b5a6c397b00..2d229d7f7b4 100644 --- a/yarn-project/simulator/src/public/side_effect_trace_interface.ts +++ b/yarn-project/simulator/src/public/side_effect_trace_interface.ts @@ -16,8 +16,7 @@ export interface PublicSideEffectTraceInterface { traceNullifierCheck(storageAddress: Fr, nullifier: Fr, leafIndex: Fr, exists: boolean, isPending: boolean): void; traceNewNullifier(storageAddress: Fr, nullifier: Fr): void; traceL1ToL2MessageCheck(contractAddress: Fr, msgHash: Fr, msgLeafIndex: Fr, exists: boolean): void; - // TODO(dbanks12): should new message accept contract address as arg? - traceNewL2ToL1Message(recipient: Fr, content: Fr): void; + traceNewL2ToL1Message(contractAddress: Fr, recipient: Fr, content: Fr): void; traceUnencryptedLog(contractAddress: Fr, log: Fr[]): void; // TODO(dbanks12): odd that getContractInstance is a one-off in that it accepts an entire object instead of components traceGetContractInstance(instance: TracedContractInstance): void;