Revert "feat: PrivateSet::pop_notes(...) (#7834)"

This reverts commit 4348654.

docs/docs/migration_notes.md

@@ -19,42 +19,6 @@ The name of the canonical Gas contract has changed to Fee Juice. Update noir cod
 
 Additionally, `NativePaymentMethod` and `NativePaymentMethodWithClaim` have been renamed to `FeeJuicePaymentMethod` and `FeeJuicePaymentMethodWithClaim`.
 
-### PrivateSet::pop_notes(...)
-
-The most common flow when working with notes is obtaining them from a `PrivateSet` via `get_notes(...)` and then removing them via `PrivateSet::remove(...)`.
-This is cumbersome and it results in unnecessary constraints due to a redundant note read request checks in the remove function.
-
-For this reason we've implemented `pop_notes(...)` which gets the notes, removes them from the set and returns them.
-This tight coupling of getting notes and removing them allowed us to safely remove the redundant read request check.
-
-Token contract diff:
-
-```diff
--let options = NoteGetterOptions::with_filter(filter_notes_min_sum, target_amount).set_limit(max_notes);
--let notes = self.map.at(owner).get_notes(options);
--let mut subtracted = U128::from_integer(0);
--for i in 0..options.limit {
--    if i < notes.len() {
--        let note = notes.get_unchecked(i);
--        self.map.at(owner).remove(note);
--        subtracted = subtracted + note.get_amount();
--    }
--}
--assert(minuend >= subtrahend, "Balance too low");
-+let options = NoteGetterOptions::with_filter(filter_notes_min_sum, target_amount).set_limit(max_notes);
-+let notes = self.map.at(owner).pop_notes(options);
-+let mut subtracted = U128::from_integer(0);
-+for i in 0..options.limit {
-+    if i < notes.len() {
-+        let note = notes.get_unchecked(i);
-+        subtracted = subtracted + note.get_amount();
-+    }
-+}
-+assert(minuend >= subtrahend, "Balance too low");
-```
-
-Note that `pop_notes` may not have obtained and removed any notes! The caller must place checks on the returned notes, e.g. in the example above by checking a sum of balances, or by checking the number of returned notes (`assert_eq(notes.len(), expected_num_notes)`).
-
 ## 0.47.0
 
 # [Aztec sandbox] TXE deployment changes

docs/docs/reference/developer_references/smart_contract_reference/storage/private_state.md

@@ -212,28 +212,27 @@ The usage is similar to using the `insert` method with the difference that this
 
 #include_code insert_from_public /noir-projects/noir-contracts/contracts/token_contract/src/main.nr rust
 
-### `pop_notes`
-
-This function pops (gets, removes and returns) the notes the account has access to based on the provided filter.
+### `remove`
 
-The kernel circuits are constrained to a maximum number of notes this function can return at a time. Check [here (GitHub link)](https://github.com/AztecProtocol/aztec-packages/blob/#include_aztec_version/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr) and look for `MAX_NOTE_HASH_READ_REQUESTS_PER_CALL` for the up-to-date number.
+Will remove a note from the `PrivateSet` if it previously has been read from storage, e.g. you have fetched it through a `get_notes` call. This is useful when you want to remove a note that you have previously read from storage and do not have to read it again.
 
-Because of this limit, we should always consider using the second argument `NoteGetterOptions` to limit the number of notes we need to read and constrain in our programs. This is quite important as every extra call increases the time used to prove the program and we don't want to spend more time than necessary.
+Nullifiers are emitted when reading values to make sure that they are up to date.
 
-An example of such options is using the [filter_notes_min_sum (GitHub link)](https://github.com/AztecProtocol/aztec-packages/blob/#include_aztec_version/noir-projects/aztec-nr/value-note/src/filter.nr) to get "enough" notes to cover a given value. Essentially, this function will return just enough notes to cover the amount specified such that we don't need to read all our notes. For users with a lot of notes, this becomes increasingly important.
+An example of how to use this operation is visible in the `easy_private_state`:
 
-#include_code pop_notes /noir-projects/aztec-nr/easy-private-state/src/easy_private_uint.nr rust
+#include_code remove /noir-projects/aztec-nr/easy-private-state/src/easy_private_uint.nr rust
 
 ### `get_notes`
 
-This function has the same behavior as `pop_notes` above but it does not delete the notes.
+This function returns the notes the account has access to.
 
+The kernel circuits are constrained to a maximum number of notes this function can return at a time. Check [here (GitHub link)](https://github.com/AztecProtocol/aztec-packages/blob/#include_aztec_version/noir-projects/noir-protocol-circuits/crates/types/src/constants.nr) and look for `MAX_NOTE_HASH_READ_REQUESTS_PER_CALL` for the up-to-date number.
 
-### `remove`
+Because of this limit, we should always consider using the second argument `NoteGetterOptions` to limit the number of notes we need to read and constrain in our programs. This is quite important as every extra call increases the time used to prove the program and we don't want to spend more time than necessary.
 
-Will remove a note from the `PrivateSet` if it previously has been read from storage, e.g. you have fetched it through a `get_notes` call. This is useful when you want to remove a note that you have previously read from storage and do not have to read it again.
+An example of such options is using the [filter_notes_min_sum (GitHub link)](https://github.com/AztecProtocol/aztec-packages/blob/#include_aztec_version/noir-projects/aztec-nr/value-note/src/filter.nr) to get "enough" notes to cover a given value. Essentially, this function will return just enough notes to cover the amount specified such that we don't need to read all our notes. For users with a lot of notes, this becomes increasingly important.
 
-Note that if you obtained the note you are about to remove via `get_notes` it's much better to use `pop_notes` as `pop_notes` results in significantly fewer constraints since it doesn't need to check that the note has been previously read, as it reads and deletes at once.
+#include_code get_notes /noir-projects/aztec-nr/easy-private-state/src/easy_private_uint.nr rust
 
 ### `view_notes`
 

noir-projects/aztec-nr/aztec/src/state_vars/private_set.nr

@@ -41,44 +41,24 @@ impl<Note, let N: u32, let M: u32> PrivateSet<Note, &mut PrivateContext> where N
     }
     // docs:end:insert
 
-    pub fn pop_notes<FILTER_ARGS>(
-        self,
-        options: NoteGetterOptions<Note, N, M, FILTER_ARGS>
-    ) -> BoundedVec<Note, MAX_NOTE_HASH_READ_REQUESTS_PER_CALL> {
-        let notes = get_notes(self.context, self.storage_slot, options);
-        // We iterate in a range 0..options.limit instead of 0..notes.len() because options.limit is known at compile
-        // time and hence will result in less constraints when set to a lower value than
-        // MAX_NOTE_HASH_READ_REQUESTS_PER_CALL.
-        for i in 0..options.limit {
-            if i < notes.len() {
-                let note = notes.get_unchecked(i);
-                // We immediately destroy the note without doing any of the read request checks `remove` typically
-                // performs because we know that the `get_notes` call has already placed those constraints.
-                destroy_note(self.context, note);
-            }
-        }
-
-        notes
-    }
-
-    /// Note that if you obtained the note via `get_notes` it's much better to use `pop_notes` as `pop_notes` results
-    /// in significantly less constrains due to avoiding an extra hash and read request check.
+    // docs:start:remove
     pub fn remove(self, note: Note) {
         let note_hash = compute_note_hash_for_read_request(note);
         let has_been_read = self.context.note_hash_read_requests.any(|r: ReadRequest| r.value == note_hash);
         assert(has_been_read, "Can only remove a note that has been read from the set.");
 
         destroy_note(self.context, note);
     }
+    // docs:end:remove
 
-    /// Note that if you later on remove the note it's much better to use `pop_notes` as `pop_notes` results
-    /// in significantly less constrains due to avoiding 1 read request check.
+    // docs:start:get_notes
     pub fn get_notes<FILTER_ARGS>(
         self,
         options: NoteGetterOptions<Note, N, M, FILTER_ARGS>
     ) -> BoundedVec<Note, MAX_NOTE_HASH_READ_REQUESTS_PER_CALL> {
         get_notes(self.context, self.storage_slot, options)
     }
+    // docs:end:get_notes
 }
 
 impl<Note, let N: u32, let M: u32> PrivateSet<Note, UnconstrainedContext> where Note: NoteInterface<N, M> {

noir-projects/aztec-nr/easy-private-state/src/easy_private_uint.nr

@@ -39,16 +39,21 @@ impl<Context> EasyPrivateUint<&mut PrivateContext> {
         let header = self.context.get_header();
         let owner_npk_m_hash = header.get_npk_m_hash(self.context, owner);
 
-        // docs:start:pop_notes
+        // docs:start:get_notes
         let options = NoteGetterOptions::with_filter(filter_notes_min_sum, subtrahend as Field);
-        let notes = self.set.pop_notes(options);
-        // docs:end:pop_notes
+        let notes = self.set.get_notes(options);
+        // docs:end:get_notes
 
         let mut minuend: u64 = 0;
         for i in 0..options.limit {
             if i < notes.len() {
                 let note = notes.get_unchecked(i);
 
+                // Removes the note from the owner's set of notes.
+                // docs:start:remove
+                self.set.remove(note);
+                // docs:end:remove
+
                 minuend += note.value as u64;
             }
         }

noir-projects/aztec-nr/value-note/src/utils.nr

@@ -55,13 +55,14 @@ pub fn decrement_by_at_most(
     outgoing_viewer: AztecAddress
 ) -> Field {
     let options = create_note_getter_options_for_decreasing_balance(max_amount);
-    let notes = balance.pop_notes(options);
+    let notes = balance.get_notes(options);
 
     let mut decremented = 0;
     for i in 0..options.limit {
         if i < notes.len() {
             let note = notes.get_unchecked(i);
-            decremented += note.value;
+
+            decremented += destroy_note(balance, note);
         }
     }
 
@@ -75,3 +76,11 @@ pub fn decrement_by_at_most(
 
     decremented
 }
+
+// Removes the note from the owner's set of notes.
+// Returns the value of the destroyed note.
+pub fn destroy_note(balance: PrivateSet<ValueNote, &mut PrivateContext>, note: ValueNote) -> Field {
+    balance.remove(note);
+
+    note.value
+}

noir-projects/noir-contracts/contracts/benchmarking_contract/src/main.nr

@@ -31,8 +31,9 @@ contract Benchmarking {
     fn recreate_note(owner: AztecAddress, outgoing_viewer: AztecAddress, index: u32) {
         let owner_notes = storage.notes.at(owner);
         let mut getter_options = NoteGetterOptions::new();
-        let notes = owner_notes.pop_notes(getter_options.set_limit(1).set_offset(index));
+        let notes = owner_notes.get_notes(getter_options.set_limit(1).set_offset(index));
         let note = notes.get(0);
+        owner_notes.remove(note);
         increment(owner_notes, note.value, owner, outgoing_viewer);
     }
 

noir-projects/noir-contracts/contracts/card_game_contract/src/cards.nr

@@ -120,9 +120,9 @@ impl Deck<&mut PrivateContext> {
         inserted_cards
     }
 
-    pub fn remove_cards<N>(&mut self, cards: [Card; N]) {
+    pub fn get_cards<N>(&mut self, cards: [Card; N]) -> [CardNote; N] {
         let options = NoteGetterOptions::with_filter(filter_cards, cards);
-        let notes = self.set.pop_notes(options);
+        let notes = self.set.get_notes(options);
 
         // This array will hold the notes that correspond to each of the requested cards. It begins empty (with all the
         // options being none) and we gradually fill it up as we find the matching notes.
@@ -144,8 +144,18 @@ impl Deck<&mut PrivateContext> {
         }
 
         // And then we assert that we did indeed find all cards, since found_cards and cards have the same length.
-        for i in 0..found_cards.len() {
-            assert(found_cards[i].is_some(), "Card not found");
+        found_cards.map(
+            |card_note: Option<CardNote>| {
+            assert(card_note.is_some(), "Card not found");
+            card_note.unwrap_unchecked()
+        }
+        )
+    }
+
+    pub fn remove_cards<N>(&mut self, cards: [Card; N]) {
+        let card_notes = self.get_cards(cards);
+        for card_note in card_notes {
+            self.set.remove(card_note.note);
         }
     }
 }

noir-projects/noir-contracts/contracts/inclusion_proofs_contract/src/main.nr

@@ -149,8 +149,10 @@ contract InclusionProofs {
         let private_values = storage.private_values.at(owner);
         let mut options = NoteGetterOptions::new();
         options = options.set_limit(1);
-        let notes = private_values.pop_notes(options);
-        assert(notes.len() == 1, "note not popped");
+        let notes = private_values.get_notes(options);
+        let note = notes.get(0);
+
+        private_values.remove(note);
     }
     // docs:end:nullify_note
 

noir-projects/noir-contracts/contracts/pending_note_hashes_contract/src/main.nr

@@ -39,12 +39,14 @@ contract PendingNoteHashes {
 
         let options = NoteGetterOptions::with_filter(filter_notes_min_sum, amount);
         // get note inserted above
-        let notes = owner_balance.pop_notes(options);
+        let notes = owner_balance.get_notes(options);
 
         let note0 = notes.get(0);
         assert(note.value == note0.value);
         assert(notes.len() == 1);
 
+        owner_balance.remove(note0);
+
         note0.value
     }
 
@@ -135,9 +137,12 @@ contract PendingNoteHashes {
 
         let mut options = NoteGetterOptions::new();
         options = options.set_limit(1);
-        let note = owner_balance.pop_notes(options).get(0);
+        let note = owner_balance.get_notes(options).get(0);
 
         assert(expected_value == note.value);
+
+        owner_balance.remove(note);
+
         expected_value
     }
 
@@ -373,9 +378,12 @@ contract PendingNoteHashes {
     #[contract_library_method]
     fn destroy_max_notes(owner: AztecAddress, storage: Storage<&mut PrivateContext>) {
         let owner_balance = storage.balances.at(owner);
-        // Note that we're relying on PXE actually returning the notes, we're not constraining that any specific
-        // numer of notes are deleted.
-        let _ = owner_balance.pop_notes(NoteGetterOptions::new());
+        let notes = owner_balance.get_notes(NoteGetterOptions::new());
+
+        for i in 0..max_notes_per_call() {
+            let note = notes.get(i);
+            owner_balance.remove(note);
+        }
     }
 
     #[contract_library_method]

noir-projects/noir-contracts/contracts/test_contract/src/main.nr

@@ -474,8 +474,9 @@ contract Test {
         let secret_hash = compute_secret_hash(secret);
         let mut options = NoteGetterOptions::new();
         options = options.select(TestNote::properties().value, secret_hash, Option::none()).set_limit(1);
-        let notes = notes_set.pop_notes(options);
-        assert(notes.len() == 1, "note not popped");
+        let notes = notes_set.get_notes(options);
+        let note = notes.get(0);
+        notes_set.remove(note);
     }
 
     unconstrained fn get_constant() -> pub Field {

noir-projects/noir-contracts/contracts/token_blacklist_contract/src/main.nr

@@ -162,19 +162,20 @@ contract TokenBlacklist {
         let to_roles = storage.roles.at(to).get_current_value_in_private();
         assert(!to_roles.is_blacklisted, "Blacklisted: Recipient");
 
+        let pending_shields = storage.pending_shields;
         let secret_hash = compute_secret_hash(secret);
-
-        // Pop 1 note (set_limit(1)) which has an amount stored in a field with index 0 (select(0, amount)) and
-        // a secret_hash stored in a field with index 1 (select(1, secret_hash)).
+        // Get 1 note (set_limit(1)) which has amount stored in field with index 0 (select(0, amount)) and secret_hash
+        // stored in field with index 1 (select(1, secret_hash)).
         let mut options = NoteGetterOptions::new();
         options = options.select(TransparentNote::properties().amount, amount, Option::none()).select(
             TransparentNote::properties().secret_hash,
             secret_hash,
             Option::none()
         ).set_limit(1);
-
-        let notes = storage.pending_shields.pop_notes(options);
-        assert(notes.len() == 1, "note not popped");
+        let notes = pending_shields.get_notes(options);
+        let note = notes.get(0);
+        // Remove the note from the pending shields set
+        pending_shields.remove(note);
 
         // Add the token note to user's balances set
         let caller = context.msg_sender();

noir-projects/noir-contracts/contracts/token_blacklist_contract/src/types/balances_map.nr

@@ -81,13 +81,24 @@ impl<T> BalancesMap<T, &mut PrivateContext> {
         owner: AztecAddress,
         subtrahend: U128
     ) -> OuterNoteEmission<T> where T: NoteInterface<T_SERIALIZED_LEN, T_SERIALIZED_BYTES_LEN> + OwnedNote + Eq {
+        // docs:start:get_notes
         let options = NoteGetterOptions::with_filter(filter_notes_min_sum, subtrahend);
-        let notes = self.map.at(owner).pop_notes(options);
+        let notes = self.map.at(owner).get_notes(options);
+        // docs:end:get_notes
 
         let mut minuend: U128 = U128::from_integer(0);
         for i in 0..options.limit {
             if i < notes.len() {
-                let note: T = notes.get_unchecked(i);
+                let note = notes.get_unchecked(i);
+
+                // Removes the note from the owner's set of notes.
+                // This will call the `compute_nullifer` function of the `token_note`
+                // which require knowledge of the secret key (currently the users encryption key).
+                // The contract logic must ensure that the spending key is used as well.
+                // docs:start:remove
+                self.map.at(owner).remove(note);
+                // docs:end:remove
+
                 minuend = minuend + note.get_amount();
             }
         }

noir-projects/noir-contracts/contracts/token_contract/src/main.nr

@@ -293,19 +293,20 @@ contract Token {
     // docs:start:redeem_shield
     #[aztec(private)]
     fn redeem_shield(to: AztecAddress, amount: Field, secret: Field) {
+        let pending_shields = storage.pending_shields;
         let secret_hash = compute_secret_hash(secret);
-
-        // Pop 1 note (set_limit(1)) which has an amount stored in a field with index 0 (select(0, amount)) and
-        // a secret_hash stored in a field with index 1 (select(1, secret_hash)).
+        // Get 1 note (set_limit(1)) which has amount stored in field with index 0 (select(0, amount)) and secret_hash
+        // stored in field with index 1 (select(1, secret_hash)).
         let mut options = NoteGetterOptions::new();
         options = options.select(TransparentNote::properties().amount, amount, Option::none()).select(
             TransparentNote::properties().secret_hash,
             secret_hash,
             Option::none()
         ).set_limit(1);
-
-        let notes = storage.pending_shields.pop_notes(options);
-        assert(notes.len() == 1, "note not popped");
+        let notes = pending_shields.get_notes(options);
+        let note = notes.get(0);
+        // Remove the note from the pending shields set
+        pending_shields.remove(note);
 
         // Add the token note to user's balances set
         // Note: Using context.msg_sender() as a sender below makes this incompatible with escrows because we send

noir-projects/noir-contracts/contracts/token_contract/src/test/minting.nr

@@ -88,7 +88,7 @@ unconstrained fn mint_private_success() {
     utils::check_private_balance(token_contract_address, owner, mint_amount);
 }
 
-#[test(should_fail_with="note not popped")]
+#[test(should_fail_with="Attempted to read past end of BoundedVec")]
 unconstrained fn mint_private_failure_double_spend() {
     // Setup without account contracts. We are not using authwits here, so dummy accounts are enough
     let (env, token_contract_address, owner, recipient) = utils::setup(/* with_account_contracts */ false);