From 84d2e1faf9a0bbee670cdf13992b21d9e58871b3 Mon Sep 17 00:00:00 2001 From: David Banks <47112877+dbanks12@users.noreply.github.com> Date: Wed, 15 May 2024 17:45:19 -0400 Subject: [PATCH] fix(avm-simulator): pending storage and nullifiers should be accessible in grandchild nested calls (#6428) --- .../end-to-end/src/shared/uniswap_l1_l2.ts | 2 +- .../simulator/src/avm/journal/journal.ts | 6 ++- .../src/avm/journal/nullifiers.test.ts | 15 ++++++ .../simulator/src/avm/journal/nullifiers.ts | 41 +++++++++++------ .../src/avm/journal/public_storage.test.ts | 15 ++++++ .../src/avm/journal/public_storage.ts | 46 ++++++++++++------- 6 files changed, 92 insertions(+), 33 deletions(-) diff --git a/yarn-project/end-to-end/src/shared/uniswap_l1_l2.ts b/yarn-project/end-to-end/src/shared/uniswap_l1_l2.ts index 7b434878eb9..b56ed6e5ae5 100644 --- a/yarn-project/end-to-end/src/shared/uniswap_l1_l2.ts +++ b/yarn-project/end-to-end/src/shared/uniswap_l1_l2.ts @@ -940,7 +940,7 @@ export const uniswapL1L2TestSuite = ( uniswapPortal.simulate.swapPublic(swapArgs, { account: ownerEthAddress.toString(), } as any), - ).rejects.toThrow('The contract function "swapPublic" reverted.'); + ).rejects.toThrow('The contract function "swapPublic" reverted'); }); it("can't call swap_private on L1 if called swap_public on L2", async () => { diff --git a/yarn-project/simulator/src/avm/journal/journal.ts b/yarn-project/simulator/src/avm/journal/journal.ts index 9a4c85bdae7..94f764409bd 100644 --- a/yarn-project/simulator/src/avm/journal/journal.ts +++ b/yarn-project/simulator/src/avm/journal/journal.ts @@ -141,7 +141,7 @@ export class AvmPersistableStateManager { * @param value - the value being written to the slot */ public writeStorage(storageAddress: Fr, slot: Fr, value: Fr) { - this.log.debug(`storage(${storageAddress})@${slot} <- ${value}`); + this.log.debug(`Storage write (address=${storageAddress}, slot=${slot}): value=${value}`); // Cache storage writes for later reference/reads this.publicStorage.write(storageAddress, slot, value); @@ -172,7 +172,9 @@ export class AvmPersistableStateManager { */ public async readStorage(storageAddress: Fr, slot: Fr): Promise { const { value, exists, cached } = await this.publicStorage.read(storageAddress, slot); - this.log.debug(`storage(${storageAddress})@${slot} ?? value: ${value}, exists: ${exists}, cached: ${cached}.`); + this.log.debug( + `Storage read (address=${storageAddress}, slot=${slot}): value=${value}, exists=${exists}, cached=${cached}`, + ); // TRANSITIONAL: This should be removed once the kernel handles and entire enqueued call per circuit // The current info to the kernel kernel does not consider cached reads. diff --git a/yarn-project/simulator/src/avm/journal/nullifiers.test.ts b/yarn-project/simulator/src/avm/journal/nullifiers.test.ts index 16f4d359a53..f8cec85bd92 100644 --- a/yarn-project/simulator/src/avm/journal/nullifiers.test.ts +++ b/yarn-project/simulator/src/avm/journal/nullifiers.test.ts @@ -64,6 +64,21 @@ describe('avm nullifier caching', () => { expect(isPending).toEqual(true); expect(gotIndex).toEqual(Fr.ZERO); }); + it('Existence check works on fallback to grandparent (gets value, exists, is pending)', async () => { + const contractAddress = new Fr(1); + const nullifier = new Fr(2); + const childNullifiers = new Nullifiers(commitmentsDb, nullifiers); + const grandChildNullifiers = new Nullifiers(commitmentsDb, childNullifiers); + + // Write to parent cache + await nullifiers.append(contractAddress, nullifier); + // Get from child cache + const [exists, isPending, gotIndex] = await grandChildNullifiers.checkExists(contractAddress, nullifier); + // exists (in parent), isPending, index is zero (not in tree) + expect(exists).toEqual(true); + expect(isPending).toEqual(true); + expect(gotIndex).toEqual(Fr.ZERO); + }); }); describe('Nullifier collision failures', () => { diff --git a/yarn-project/simulator/src/avm/journal/nullifiers.ts b/yarn-project/simulator/src/avm/journal/nullifiers.ts index 99d12757e60..e580c1a885c 100644 --- a/yarn-project/simulator/src/avm/journal/nullifiers.ts +++ b/yarn-project/simulator/src/avm/journal/nullifiers.ts @@ -12,21 +12,38 @@ import type { CommitmentsDB } from '../../index.js'; export class Nullifiers { /** Cached nullifiers. */ public cache: NullifierCache; - /** Parent's nullifier cache. Checked on cache-miss. */ - private readonly parentCache: NullifierCache | undefined; - /** Reference to node storage. Checked on parent cache-miss. */ - private readonly hostNullifiers: CommitmentsDB; - constructor(hostNullifiers: CommitmentsDB, parent?: Nullifiers) { - this.hostNullifiers = hostNullifiers; - this.parentCache = parent?.cache; + constructor( + /** Reference to node storage. Checked on parent cache-miss. */ + private readonly hostNullifiers: CommitmentsDB, + /** Parent's nullifiers. Checked on this' cache-miss. */ + private readonly parent?: Nullifiers | undefined, + ) { this.cache = new NullifierCache(); } + /** + * Get a nullifier's existence in this' cache or parent's (recursively). + * DOES NOT CHECK HOST STORAGE! + * @param storageAddress - the address of the contract whose storage is being read from + * @param nullifier - the nullifier to check for + * @returns exists: whether the nullifier exists in cache here or in parent's + */ + private checkExistsHereOrParent(storageAddress: Fr, nullifier: Fr): boolean { + // First check this cache + let existsAsPending = this.cache.exists(storageAddress, nullifier); + // Then try parent's nullifier cache + if (!existsAsPending && this.parent) { + // Note: this will recurse to grandparent/etc until a cache-hit is encountered. + existsAsPending = this.parent.checkExistsHereOrParent(storageAddress, nullifier); + } + return existsAsPending; + } + /** * Get a nullifier's existence status. * 1. Check cache. - * 2. Check parent's cache. + * 2. Check parent cache. * 3. Fall back to the host state. * 4. Not found! Nullifier does not exist. * @@ -40,12 +57,8 @@ export class Nullifiers { storageAddress: Fr, nullifier: Fr, ): Promise<[/*exists=*/ boolean, /*isPending=*/ boolean, /*leafIndex=*/ Fr]> { - // First check this cache - let existsAsPending = this.cache.exists(storageAddress, nullifier); - // Then check parent's cache - if (!existsAsPending && this.parentCache) { - existsAsPending = this.parentCache?.exists(storageAddress, nullifier); - } + // Check this cache and parent's (recursively) + const existsAsPending = this.checkExistsHereOrParent(storageAddress, nullifier); // Finally try the host's Aztec state (a trip to the database) // If the value is found in the database, it will be associated with a leaf index! let leafIndex: bigint | undefined = undefined; diff --git a/yarn-project/simulator/src/avm/journal/public_storage.test.ts b/yarn-project/simulator/src/avm/journal/public_storage.test.ts index 54633e40f96..1d6359caef9 100644 --- a/yarn-project/simulator/src/avm/journal/public_storage.test.ts +++ b/yarn-project/simulator/src/avm/journal/public_storage.test.ts @@ -67,6 +67,21 @@ describe('avm public storage', () => { expect(cached).toEqual(true); }); + it('Reading works on fallback to grandparent (gets value & exists)', async () => { + const contractAddress = new Fr(1); + const slot = new Fr(2); + const value = new Fr(3); + const childStorage = new PublicStorage(publicDb, publicStorage); + const grandChildStorage = new PublicStorage(publicDb, childStorage); + + publicStorage.write(contractAddress, slot, value); + const { exists, value: gotValue, cached } = await grandChildStorage.read(contractAddress, slot); + // exists because it was previously written! + expect(exists).toEqual(true); + expect(gotValue).toEqual(value); + expect(cached).toEqual(true); + }); + it('When reading from storage, should check cache, then parent, then host', async () => { // Store a different value in storage vs the cache, and make sure the cache is returned const contractAddress = new Fr(1); diff --git a/yarn-project/simulator/src/avm/journal/public_storage.ts b/yarn-project/simulator/src/avm/journal/public_storage.ts index 30c59e421b3..6019934c201 100644 --- a/yarn-project/simulator/src/avm/journal/public_storage.ts +++ b/yarn-project/simulator/src/avm/journal/public_storage.ts @@ -17,14 +17,13 @@ type PublicStorageReadResult = { export class PublicStorage { /** Cached storage writes. */ private cache: PublicStorageCache; - /** Parent's storage cache. Checked on cache-miss. */ - private readonly parentCache: PublicStorageCache | undefined; - /** Reference to node storage. Checked on parent cache-miss. */ - private readonly hostPublicStorage: PublicStateDB; - constructor(hostPublicStorage: PublicStateDB, parent?: PublicStorage) { - this.hostPublicStorage = hostPublicStorage; - this.parentCache = parent?.cache; + constructor( + /** Reference to node storage. Checked on parent cache-miss. */ + private readonly hostPublicStorage: PublicStateDB, + /** Parent's storage. Checked on this' cache-miss. */ + private readonly parent?: PublicStorage, + ) { this.cache = new PublicStorageCache(); } @@ -35,10 +34,29 @@ export class PublicStorage { return this.cache; } + /** + * Read a storage value from this' cache or parent's (recursively). + * DOES NOT CHECK HOST STORAGE! + * + * @param storageAddress - the address of the contract whose storage is being read from + * @param slot - the slot in the contract's storage being read from + * @returns value: the latest value written according to this cache or the parent's. undefined on cache miss. + */ + public readHereOrParent(storageAddress: Fr, slot: Fr): Fr | undefined { + // First try check this storage cache + let value = this.cache.read(storageAddress, slot); + // Then try parent's storage cache + if (!value && this.parent) { + // Note: this will recurse to grandparent/etc until a cache-hit is encountered. + value = this.parent.readHereOrParent(storageAddress, slot); + } + return value; + } + /** * Read a value from storage. * 1. Check cache. - * 2. Check parent's cache. + * 2. Check parent cache. * 3. Fall back to the host state. * 4. Not found! Value has never been written to before. Flag it as non-existent and return value zero. * @@ -48,12 +66,8 @@ export class PublicStorage { */ public async read(storageAddress: Fr, slot: Fr): Promise { let cached = false; - // First try check this storage cache - let value = this.cache.read(storageAddress, slot); - // Then try parent's storage cache (if it exists / written to earlier in this TX) - if (!value && this.parentCache) { - value = this.parentCache?.read(storageAddress, slot); - } + // Check this cache and parent's (recursively) + let value = this.readHereOrParent(storageAddress, slot); // Finally try the host's Aztec state (a trip to the database) if (!value) { value = await this.hostPublicStorage.storageRead(storageAddress, slot); @@ -73,8 +87,8 @@ export class PublicStorage { * @param slot - the slot in the contract's storage being written to * @param value - the value being written to the slot */ - public write(storageAddress: Fr, key: Fr, value: Fr) { - this.cache.write(storageAddress, key, value); + public write(storageAddress: Fr, slot: Fr, value: Fr) { + this.cache.write(storageAddress, slot, value); } /**