From 5e4b46d577ebf63114a5a5a1c5b6d2947d3b2567 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 16 Dec 2024 13:02:22 -0300 Subject: [PATCH] fix: always remove nullified notes (#10722) We used to remove nullified notes as we process tagged logs, but this is incomplete: it may happen that a recipient got no new logs but they still need to nullify things. I imagine we never caught this because either our tests are not comprehensive enough, of because all in all scenarios we tried there was always a new note (e.g. a transfer for less than the full balance resulted in a change note). I changed things so that the note syncing process also triggers a full search of all nullifiers for all recipients always, instead of only doing it for those that got notes. This is perhaps not ideal long-term, but it is a simple fix for the issue we currently have. --------- Co-authored-by: thunkar --- .../pxe/src/simulator_oracle/index.ts | 45 ++++++++++--------- .../simulator_oracle/simulator_oracle.test.ts | 39 ++++++---------- .../src/client/client_execution_context.ts | 2 + .../simulator/src/client/db_oracle.ts | 5 +++ .../simulator/src/client/view_data_oracle.ts | 3 ++ yarn-project/txe/src/oracle/txe_oracle.ts | 2 + 6 files changed, 50 insertions(+), 46 deletions(-) diff --git a/yarn-project/pxe/src/simulator_oracle/index.ts b/yarn-project/pxe/src/simulator_oracle/index.ts index 160947e25c1..fdd3058ab1d 100644 --- a/yarn-project/pxe/src/simulator_oracle/index.ts +++ b/yarn-project/pxe/src/simulator_oracle/index.ts @@ -624,27 +624,30 @@ export class SimulatorOracle implements DBOracle { }); }); } - const nullifiedNotes: IncomingNoteDao[] = []; - const currentNotesForRecipient = await this.db.getIncomingNotes({ owner: recipient }); - const nullifiersToCheck = currentNotesForRecipient.map(note => note.siloedNullifier); - const currentBlockNumber = await this.getBlockNumber(); - const nullifierIndexes = await this.aztecNode.findNullifiersIndexesWithBlock(currentBlockNumber, nullifiersToCheck); - - const foundNullifiers = nullifiersToCheck - .map((nullifier, i) => { - if (nullifierIndexes[i] !== undefined) { - return { ...nullifierIndexes[i], ...{ data: nullifier } } as InBlock; - } - }) - .filter(nullifier => nullifier !== undefined) as InBlock[]; - - await this.db.removeNullifiedNotes(foundNullifiers, recipient.toAddressPoint()); - nullifiedNotes.forEach(noteDao => { - this.log.verbose(`Removed note for contract ${noteDao.contractAddress} at slot ${noteDao.storageSlot}`, { - contract: noteDao.contractAddress, - slot: noteDao.storageSlot, - nullifier: noteDao.siloedNullifier.toString(), + } + + public async removeNullifiedNotes(contractAddress: AztecAddress) { + for (const recipient of await this.keyStore.getAccounts()) { + const currentNotesForRecipient = await this.db.getIncomingNotes({ contractAddress, owner: recipient }); + const nullifiersToCheck = currentNotesForRecipient.map(note => note.siloedNullifier); + const nullifierIndexes = await this.aztecNode.findNullifiersIndexesWithBlock('latest', nullifiersToCheck); + + const foundNullifiers = nullifiersToCheck + .map((nullifier, i) => { + if (nullifierIndexes[i] !== undefined) { + return { ...nullifierIndexes[i], ...{ data: nullifier } } as InBlock; + } + }) + .filter(nullifier => nullifier !== undefined) as InBlock[]; + + const nullifiedNotes = await this.db.removeNullifiedNotes(foundNullifiers, recipient.toAddressPoint()); + nullifiedNotes.forEach(noteDao => { + this.log.verbose(`Removed note for contract ${noteDao.contractAddress} at slot ${noteDao.storageSlot}`, { + contract: noteDao.contractAddress, + slot: noteDao.storageSlot, + nullifier: noteDao.siloedNullifier.toString(), + }); }); - }); + } } } diff --git a/yarn-project/pxe/src/simulator_oracle/simulator_oracle.test.ts b/yarn-project/pxe/src/simulator_oracle/simulator_oracle.test.ts index 1e88eaab363..b291362943c 100644 --- a/yarn-project/pxe/src/simulator_oracle/simulator_oracle.test.ts +++ b/yarn-project/pxe/src/simulator_oracle/simulator_oracle.test.ts @@ -2,11 +2,13 @@ import { type AztecNode, EncryptedLogPayload, L1NotePayload, + L2Block, Note, type TxEffect, TxHash, TxScopedL2Log, randomInBlock, + wrapInBlock, } from '@aztec/circuit-types'; import { AztecAddress, @@ -654,40 +656,27 @@ describe('Simulator oracle', () => { } }); - it('should not store nullified notes', async () => { + it('should remove nullified notes', async () => { const requests = [ new MockNoteRequest(getRandomNoteLogPayload(Fr.random(), contractAddress), 1, 1, 1, recipient.address), new MockNoteRequest(getRandomNoteLogPayload(Fr.random(), contractAddress), 6, 3, 2, recipient.address), new MockNoteRequest(getRandomNoteLogPayload(Fr.random(), contractAddress), 12, 3, 2, recipient.address), ]; - const taggedLogs = mockTaggedLogs(requests, 2); - - getIncomingNotesSpy.mockResolvedValueOnce(Promise.resolve(requests.map(request => request.snippetOfNoteDao))); - - await simulatorOracle.processTaggedLogs(taggedLogs, recipient.address, simulator); - - expect(addNotesSpy).toHaveBeenCalledTimes(1); - expect(addNotesSpy).toHaveBeenCalledWith( - // Incoming should contain notes from requests 0, 1, 2 because in those requests we set owner address point. - [ - expect.objectContaining({ - ...requests[0].snippetOfNoteDao, - index: requests[0].indexWithinNoteHashTree, - }), - expect.objectContaining({ - ...requests[1].snippetOfNoteDao, - index: requests[1].indexWithinNoteHashTree, - }), - expect.objectContaining({ - ...requests[2].snippetOfNoteDao, - index: requests[2].indexWithinNoteHashTree, - }), - ], - recipient.address, + getIncomingNotesSpy.mockResolvedValueOnce( + Promise.resolve(requests.map(request => ({ siloedNullifier: Fr.random(), ...request.snippetOfNoteDao }))), ); + let requestedNullifier; + aztecNode.findNullifiersIndexesWithBlock.mockImplementationOnce((_blockNumber, nullifiers) => { + const block = L2Block.random(2); + requestedNullifier = wrapInBlock(nullifiers[0], block); + return Promise.resolve([wrapInBlock(1n, L2Block.random(2)), undefined, undefined]); + }); + + await simulatorOracle.removeNullifiedNotes(contractAddress); expect(removeNullifiedNotesSpy).toHaveBeenCalledTimes(1); + expect(removeNullifiedNotesSpy).toHaveBeenCalledWith([requestedNullifier], recipient.address.toAddressPoint()); }, 30_000); }); }); diff --git a/yarn-project/simulator/src/client/client_execution_context.ts b/yarn-project/simulator/src/client/client_execution_context.ts index 6a9a8eff781..12e8c814a2a 100644 --- a/yarn-project/simulator/src/client/client_execution_context.ts +++ b/yarn-project/simulator/src/client/client_execution_context.ts @@ -565,5 +565,7 @@ export class ClientExecutionContext extends ViewDataOracle { for (const [recipient, taggedLogs] of taggedLogsByRecipient.entries()) { await this.db.processTaggedLogs(taggedLogs, AztecAddress.fromString(recipient)); } + + await this.db.removeNullifiedNotes(this.contractAddress); } } diff --git a/yarn-project/simulator/src/client/db_oracle.ts b/yarn-project/simulator/src/client/db_oracle.ts index 9472148257a..d53022e49e2 100644 --- a/yarn-project/simulator/src/client/db_oracle.ts +++ b/yarn-project/simulator/src/client/db_oracle.ts @@ -242,4 +242,9 @@ export interface DBOracle extends CommitmentsDB { * @param recipient - The recipient of the logs. */ processTaggedLogs(logs: TxScopedL2Log[], recipient: AztecAddress): Promise; + + /** + * Removes all of a contract's notes that have been nullified from the note database. + */ + removeNullifiedNotes(contractAddress: AztecAddress): Promise; } diff --git a/yarn-project/simulator/src/client/view_data_oracle.ts b/yarn-project/simulator/src/client/view_data_oracle.ts index bfd870760b1..d80b2d91182 100644 --- a/yarn-project/simulator/src/client/view_data_oracle.ts +++ b/yarn-project/simulator/src/client/view_data_oracle.ts @@ -314,8 +314,11 @@ export class ViewDataOracle extends TypedOracle { await this.aztecNode.getBlockNumber(), this.scopes, ); + for (const [recipient, taggedLogs] of taggedLogsByRecipient.entries()) { await this.db.processTaggedLogs(taggedLogs, AztecAddress.fromString(recipient)); } + + await this.db.removeNullifiedNotes(this.contractAddress); } } diff --git a/yarn-project/txe/src/oracle/txe_oracle.ts b/yarn-project/txe/src/oracle/txe_oracle.ts index 03928a58a30..ac5792a5b7c 100644 --- a/yarn-project/txe/src/oracle/txe_oracle.ts +++ b/yarn-project/txe/src/oracle/txe_oracle.ts @@ -921,6 +921,8 @@ export class TXE implements TypedOracle { await this.simulatorOracle.processTaggedLogs(taggedLogs, AztecAddress.fromString(recipient)); } + await this.simulatorOracle.removeNullifiedNotes(this.contractAddress); + return Promise.resolve(); }