940 - getNotes filter nullified notes (#1186)

# Description Resolves #940 # Checklist: - [x] I have reviewed my diff in github, line by line. - [x] Every change is related to the PR description. - [x] I have [linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue) this pull request to the issue(s) that it resolves. - [x] There are no unexpected formatting changes, superfluous debug logs, or commented-out code. - [x] The branch has been merged or rebased against the head of its merge target. - [x] I'm happy for the PR to be merged at the reviewer's next convenience.
AztecProtocol · Jul 27, 2023 · 12830b0 · 12830b0
1 parent 3d2299d
commit 12830b0
Show file tree

Hide file tree

Showing 19 changed files with 194 additions and 55 deletions.
diff --git a/yarn-project/acir-simulator/src/client/client_execution_context.ts b/yarn-project/acir-simulator/src/client/client_execution_context.ts
@@ -11,7 +11,7 @@ import {
   toAcvmL1ToL2MessageLoadOracleInputs,
 } from '../acvm/index.js';
 import { PackedArgsCache } from '../packed_args_cache.js';
-import { DBOracle, NoteData } from './db_oracle.js';
+import { DBOracle, PendingNoteData } from './db_oracle.js';
 import { pickNotes } from './pick_notes.js';
 
 /**
@@ -34,8 +34,12 @@ export class ClientTxExecutionContext {
     public historicRoots: PrivateHistoricTreeRoots,
     /** The cache of packed arguments */
     public packedArgsCache: PackedArgsCache,
-    /** Pending commitments created (and not nullified) up to current point in execution **/
-    private pendingNotes: NoteData[] = [],
+    /** Pending notes created (and not nullified) up to current point in execution.
+     *  If a nullifier for a note in this list is emitted, the note will be REMOVED. */
+    private pendingNotes: PendingNoteData[] = [],
+    /** The list of nullifiers created in this transaction. The commitment/note which is nullified
+     *  might be pending or not (i.e., was generated in a previous transaction) */
+    private pendingNullifiers: Set<Fr> = new Set<Fr>(),
   ) {}
 
   /**
@@ -50,6 +54,7 @@ export class ClientTxExecutionContext {
       this.historicRoots,
       this.packedArgsCache,
       this.pendingNotes,
+      this.pendingNullifiers,
     );
   }
 
@@ -114,14 +119,17 @@ export class ClientTxExecutionContext {
   ): Promise<ACVMField[]> {
     const storageSlotField = fromACVMField(storageSlot);
 
-    // TODO(https://github.com/AztecProtocol/aztec-packages/issues/920): don't 'get' notes nullified in pendingNullifiers
     const pendingNotes = this.pendingNotes.filter(
       n => n.contractAddress.equals(contractAddress) && n.storageSlot.equals(storageSlotField),
     );
 
     const dbNotes = await this.db.getNotes(contractAddress, storageSlotField);
 
-    const notes = pickNotes([...pendingNotes, ...dbNotes], {
+    // Remove notes which were already nullified during this transaction.
+    const dbNotesFiltered = dbNotes.filter(n => !this.pendingNullifiers.has(n.nullifier as Fr));
+
+    // Nullified pending notes are already removed from the list.
+    const notes = pickNotes([...dbNotesFiltered, ...pendingNotes], {
       sortBy: sortBy.map(field => +field),
       sortOrder: sortOrder.map(field => +field),
       limit,
@@ -171,15 +179,56 @@ export class ClientTxExecutionContext {
    * @param contractAddress - The contract address.
    * @param storageSlot - The storage slot.
    * @param preimage - new note preimage.
+   * @param nullifier - note nullifier
+   * @param innerNoteHash - inner note hash
    */
-  public async pushNewNote(contractAddress: AztecAddress, storageSlot: ACVMField, preimage: ACVMField[]) {
+  public async pushNewNote(contractAddress: AztecAddress, storageSlot: Fr, preimage: Fr[], innerNoteHash: Fr) {
     const wasm = await CircuitsWasm.get();
     const nonce = computeCommitmentNonce(wasm, this.txNullifier, this.pendingNotes.length);
     this.pendingNotes.push({
       contractAddress,
-      storageSlot: fromACVMField(storageSlot),
+      storageSlot: storageSlot,
       nonce,
-      preimage: preimage.map(f => fromACVMField(f)),
+      preimage,
+      innerNoteHash,
     });
   }
+
+  /**
+   * Adding a nullifier into the current set of all pending nullifiers created
+   * within the current transaction/execution.
+   * @param nullifier - The pending nullifier to add in the list.
+   */
+  public pushPendingNullifier(nullifier: Fr) {
+    this.pendingNullifiers.add(nullifier);
+  }
+
+  /**
+   * Update the list of pending notes by chopping a note which is nullified.
+   * The identifier used to determine matching is the inner note hash value.
+   * However, we adopt a defensive approach and ensure that the contract address
+   * and storage slot do match.
+   * Note that this method might be called with an innerNoteHash referring to
+   * a note created in a previous transaction which will result in this array
+   * of pending notes left unchanged.
+   * @param innerNoteHash - The inner note hash value to which note will be chopped.
+   * @param contractAddress - The contract address
+   * @param storageSlot - The storage slot as a Field Fr element
+   */
+  public nullifyPendingNotes(innerNoteHash: Fr, contractAddress: AztecAddress, storageSlot: Fr) {
+    // IMPORTANT: We do need an in-place array mutation of this.pendingNotes as this array is shared
+    // by reference among the nested calls. That is the main reason for 'splice' usage below.
+    this.pendingNotes.splice(
+      0,
+      this.pendingNotes.length,
+      ...this.pendingNotes.filter(
+        n =>
+          !(
+            n.innerNoteHash.equals(innerNoteHash) &&
+            n.contractAddress.equals(contractAddress) &&
+            n.storageSlot.equals(storageSlot)
+          ),
+      ),
+    );
+  }
 }
diff --git a/yarn-project/acir-simulator/src/client/db_oracle.ts b/yarn-project/acir-simulator/src/client/db_oracle.ts
@@ -18,10 +18,20 @@ export interface NoteData {
   nonce: Fr;
   /** The preimage of the note */
   preimage: Fr[];
+  /** The corresponding nullifier of the note */
+  nullifier?: Fr;
   /** The note's leaf index in the private data tree. Undefined for pending notes. */
   index?: bigint;
 }
 
+/**
+ * Information about a note needed during execution.
+ */
+export interface PendingNoteData extends NoteData {
+  /** The inner note hash (used as a nullified commitment). */
+  innerNoteHash: Fr;
+}
+
 /**
  * The format that noir uses to get L1 to L2 Messages.
  */

diff --git a/yarn-project/acir-simulator/src/client/private_execution.test.ts b/yarn-project/acir-simulator/src/client/private_execution.test.ts
@@ -174,7 +174,7 @@ describe('Private Execution test suite', () => {
     const buildNote = (amount: bigint, owner: AztecAddress, storageSlot = Fr.random()) => {
       const nonce = new Fr(currentNoteIndex);
       const preimage = [new Fr(amount), owner.toField(), Fr.random(), new Fr(1n)];
-      return { contractAddress, storageSlot, index: currentNoteIndex++, nonce, preimage };
+      return { contractAddress, storageSlot, index: currentNoteIndex++, nonce, nullifier: new Fr(0), preimage };
     };
 
     beforeEach(async () => {
@@ -359,6 +359,7 @@ describe('Private Execution test suite', () => {
           storageSlot,
           nonce,
           preimage: [new Fr(amount), secret],
+          nullifier: new Fr(0),
           index: 1n,
         },
       ]);
@@ -624,13 +625,15 @@ describe('Private Execution test suite', () => {
       )!;
       const insertAbi = PendingCommitmentsContractAbi.functions.find(f => f.name === 'insert_note')!;
       const getThenNullifyAbi = PendingCommitmentsContractAbi.functions.find(f => f.name === 'get_then_nullify_note')!;
+      const getZeroAbi = PendingCommitmentsContractAbi.functions.find(f => f.name === 'get_note_zero_balance')!;
 
       const insertFnSelector = generateFunctionSelector(insertAbi.name, insertAbi.parameters);
       const getThenNullifyFnSelector = generateFunctionSelector(getThenNullifyAbi.name, getThenNullifyAbi.parameters);
+      const getZeroFnSelector = generateFunctionSelector(getZeroAbi.name, getZeroAbi.parameters);
 
       oracle.getPortalContractAddress.mockImplementation(() => Promise.resolve(EthAddress.ZERO));
 
-      const args = [amountToTransfer, owner, insertFnSelector, getThenNullifyFnSelector];
+      const args = [amountToTransfer, owner, insertFnSelector, getThenNullifyFnSelector, getZeroFnSelector];
       const result = await runSimulator({
         args: args,
         abi: abi,
@@ -640,6 +643,7 @@ describe('Private Execution test suite', () => {
 
       const execInsert = result.nestedExecutions[0];
       const execGetThenNullify = result.nestedExecutions[1];
+      const getNotesAfterNullify = result.nestedExecutions[2];
 
       expect(execInsert.preimages.newNotes).toHaveLength(1);
       const note = execInsert.preimages.newNotes[0];
@@ -668,6 +672,10 @@ describe('Private Execution test suite', () => {
       expect(nullifier).toEqual(
         await acirSimulator.computeNullifier(contractAddress, nonce, note.storageSlot, note.preimage),
       );
+
+      // check that the last get_notes call return no note
+      const afterNullifyingNoteValue = getNotesAfterNullify.callStackItem.publicInputs.returnValues[0].value;
+      expect(afterNullifyingNoteValue).toEqual(0n);
     });
 
     it('cant read a commitment that is inserted later in same call', async () => {

diff --git a/yarn-project/acir-simulator/src/client/private_execution.ts b/yarn-project/acir-simulator/src/client/private_execution.ts
@@ -76,8 +76,13 @@ export class PrivateFunctionExecution {
       getNotes: ([slot], sortBy, sortOrder, [limit], [offset], [returnSize]) =>
         this.context.getNotes(this.contractAddress, slot, sortBy, sortOrder, +limit, +offset, +returnSize),
       getRandomField: () => Promise.resolve(toACVMField(Fr.random())),
-      notifyCreatedNote: async ([storageSlot], preimage) => {
-        await this.context.pushNewNote(this.contractAddress, storageSlot, preimage);
+      notifyCreatedNote: async ([storageSlot], preimage, [innerNoteHash]) => {
+        await this.context.pushNewNote(
+          this.contractAddress,
+          fromACVMField(storageSlot),
+          preimage.map(f => fromACVMField(f)),
+          fromACVMField(innerNoteHash),
+        );
 
         // TODO(https://github.com/AztecProtocol/aztec-packages/issues/1040): remove newNotePreimages
         // as it is redundant with pendingNoteData. Consider renaming pendingNoteData->pendingNotePreimages.
@@ -87,13 +92,14 @@ export class PrivateFunctionExecution {
         });
         return ZERO_ACVM_FIELD;
       },
-      notifyNullifiedNote: ([slot], [nullifier], acvmPreimage) => {
-        // TODO(https://github.com/AztecProtocol/aztec-packages/issues/920): track list of pendingNullifiers similar to pendingNotes
+      notifyNullifiedNote: ([slot], [nullifier], acvmPreimage, [innerNoteHash]) => {
         newNullifiers.push({
           preimage: acvmPreimage.map(f => fromACVMField(f)),
           storageSlot: fromACVMField(slot),
           nullifier: fromACVMField(nullifier),
         });
+        this.context.pushPendingNullifier(fromACVMField(nullifier));
+        this.context.nullifyPendingNotes(fromACVMField(innerNoteHash), this.contractAddress, fromACVMField(slot));
         return Promise.resolve(ZERO_ACVM_FIELD);
       },
       callPrivateFunction: async ([acvmContractAddress], [acvmFunctionSelector], [acvmArgsHash]) => {

diff --git a/yarn-project/acir-simulator/src/client/unconstrained_execution.test.ts b/yarn-project/acir-simulator/src/client/unconstrained_execution.test.ts
@@ -68,6 +68,7 @@ describe('Unconstrained Execution test suite', () => {
           storageSlot: Fr.random(),
           nonce: Fr.random(),
           preimage,
+          nullifier: Fr.random(),
           index: BigInt(index),
         })),
       );

diff --git a/yarn-project/aztec-rpc/src/simulator_oracle/index.ts b/yarn-project/aztec-rpc/src/simulator_oracle/index.ts
@@ -67,11 +67,12 @@ export class SimulatorOracle implements DBOracle {
    */
   async getNotes(contractAddress: AztecAddress, storageSlot: Fr) {
     const noteDaos = await this.db.getNoteSpendingInfo(contractAddress, storageSlot);
-    return noteDaos.map(({ contractAddress, storageSlot, nonce, notePreimage, index }) => ({
+    return noteDaos.map(({ contractAddress, storageSlot, nonce, notePreimage, nullifier, index }) => ({
       contractAddress,
       storageSlot,
       nonce,
       preimage: notePreimage.items,
+      nullifier,
       // RPC Client can use this index to get full MembershipWitness
       index,
     }));

diff --git a/yarn-project/end-to-end/src/e2e_pending_commitments_contract.test.ts b/yarn-project/end-to-end/src/e2e_pending_commitments_contract.test.ts
@@ -65,6 +65,7 @@ describe('e2e_pending_commitments_contract', () => {
         owner,
         Fr.fromBuffer(deployedContract.methods.insert_note.selector),
         Fr.fromBuffer(deployedContract.methods.get_then_nullify_note.selector),
+        Fr.fromBuffer(deployedContract.methods.get_note_zero_balance.selector),
       )
       .send({ origin: owner });
 
@@ -77,4 +78,6 @@ describe('e2e_pending_commitments_contract', () => {
   // TODO(https://github.com/AztecProtocol/aztec-packages/issues/892): test expected kernel failures if transient reads (or their hints) don't match
   // TODO(https://github.com/AztecProtocol/aztec-packages/issues/836): test expected kernel failures if nullifiers (or their hints) don't match
   // TODO(https://github.com/AztecProtocol/aztec-packages/issues/839): test creation, getting, nullifying of multiple notes
+  // TODO(https://github.com/AztecProtocol/aztec-packages/issues/1242): test nullifying a note created in a previous transaction and
+  //                                                                    get_notes in the same transaction should not return it.
 });