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 <[email protected]>

Author: nventuro

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<Fr>;
-        }
-      })
-      .filter(nullifier => nullifier !== undefined) as InBlock<Fr>[];
-
-    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<Fr>;
+          }
+        })
+        .filter(nullifier => nullifier !== undefined) as InBlock<Fr>[];
+
+      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(),
+        });
       });
-    });
+    }
   }
 }

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);
   });
 });

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);
   }
 }

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<void>;
+
+  /**
+   * Removes all of a contract's notes that have been nullified from the note database.
+   */
+  removeNullifiedNotes(contractAddress: AztecAddress): Promise<void>;
 }

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);
   }
 }

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();
   }