feat!: change for_each iteration order in CapsuleArray, EphemeralArray, TransientArray

docs/docs-developers/docs/resources/migration_notes.md

@@ -9,6 +9,43 @@ Aztec is in active development. Each version may introduce breaking changes that
 
 ## TBD
 
+### [Aztec.nr] `for_each` visits elements in order; removing during iteration no longer supported
+
+`CapsuleArray::for_each` and `EphemeralArray::for_each` previously iterated backwards (from the last element to the first) so that the callback could safely remove the current element. They now visit elements in order, from first to last, as is usually expected in other languages. Structurally mutating the array (e.g. via `push` or `remove`) from inside the callback is no longer supported.
+
+For `EphemeralArray`, replace remove-during-iteration with `filter`:
+
+```diff
+- array.for_each(|index, value| {
+-     if should_remove(value) {
+-         array.remove(index);
+-     }
+- });
++ let kept = array.filter(|value| !should_remove(value));
+```
+
+`filter` collects the kept elements into a fresh array at a new slot. If the original slot matters (e.g. a `TransientArray` slot shared with other call frames), rebuild it from the filtered result:
+
+```noir
+let kept = array.filter(|value| !should_remove(value));
+let _ = array.clear();
+kept.for_each(|_index, value| array.push(value));
+```
+
+`EphemeralArray`'s are cheap and by nature not persistent though, so in most cases you probably can just work with the new copy instead of going through this hassle. 
+
+`CapsuleArray` has no `filter`, so iterate manually, backwards. Removing the current element is safe in a backward loop because it only shifts elements at higher indices:
+
+```noir
+let mut i = array.len();
+while i > 0 {
+    i -= 1;
+    if should_remove(array.get(i)) {
+        array.remove(i);
+    }
+}
+```
+
 ### [Aztec.js] Prefunded local network test accounts are now initializerless
 
 The genesis-funded test accounts in the local network (sandbox), returned by `getInitialTestAccountsData()`, are now initializerless Schnorr accounts (`schnorr_initializerless`). An initializerless account has no onchain deployment transaction: its address commits to the signing public key (through `immutables_hash`) and its contract state is materialized locally in the PXE, so these accounts are usable right away.

noir-projects/aztec-nr/aztec/src/capsules/mod.nr

@@ -103,40 +103,17 @@ impl<T> CapsuleArray<T> {
 
     /// Calls a function on each element of the array.
     ///
-    /// The function `f` is called once with each array value and its corresponding index. The order in which values
-    /// are processed is arbitrary.
+    /// The function `f` is called once with each array value and its corresponding index, in order (from the first
+    /// element to the last).
     ///
-    /// ## Array Mutation
-    ///
-    /// It is safe to delete the current element (and only the current element) from inside the callback via `remove`:
-    /// ```noir
-    /// array.for_each(|index, value| {
-    ///   if some_condition(value) {
-    ///     array.remove(index); // safe only for this index
-    ///   }
-    /// }
-    /// ```
-    ///
-    /// If all elements in the array need to iterated over and then removed, then using `for_each` results in optimal
-    /// efficiency.
-    ///
-    /// It is **not** safe to push new elements into the array from inside the callback.
+    /// Structurally mutating the array from inside the callback (e.g. via `push` or `remove`) is **not** supported:
+    /// it can cause elements to be skipped, visited more than once, or read out of bounds.
     pub unconstrained fn for_each<Env>(self, f: unconstrained fn[Env](u32, T) -> ())
     where
         T: Deserialize,
     {
-        // Iterating over all elements is simple, but we want to do it in such a way that a) deleting the current
-        // element is safe to do, and b) deleting *all* elements is optimally efficient. This is because CapsuleArrays
-        // are typically used to hold pending tasks, so iterating them while clearing completed tasks (sometimes
-        // unconditionally, resulting in a full clear) is a very common access pattern.
-        //
-        // The way we achieve this is by iterating backwards: each element can always be deleted since it won't change
-        // any preceding (lower) indices, and if every element is deleted then every element will (in turn) be the last
-        // element. This results in an optimal full clear since `remove` will be able to skip the `capsules::copy` call
-        // to shift any elements past the deleted one (because there will be none).
-        let mut i = self.len();
-        while i > 0 {
-            i -= 1;
+        let n = self.len();
+        for i in 0..n {
             f(i, self.get(i));
         }
     }
@@ -271,7 +248,7 @@ mod test {
     }
 
     #[test]
-    unconstrained fn for_each_called_with_all_elements() {
+    unconstrained fn for_each_called_with_all_elements_in_order() {
         let mut env = TestEnvironment::new();
         let scope = env.create_light_account();
         env.private_context(|context| {
@@ -282,79 +259,13 @@ mod test {
             array.push(5);
             array.push(6);
 
-            // We store all values that we were called with and check that all (value, index) tuples are present. Note
-            // that we do not care about the order in which each tuple was passed to the closure.
             let called_with = &mut BoundedVec::<(u32, Field), 3>::new();
             array.for_each(|index, value| { called_with.push((index, value)); });
 
             assert_eq(called_with.len(), 3);
-            assert(called_with.any(|(index, value)| (index == 0) & (value == 4)));
-            assert(called_with.any(|(index, value)| (index == 1) & (value == 5)));
-            assert(called_with.any(|(index, value)| (index == 2) & (value == 6)));
-        });
-    }
-
-    #[test]
-    unconstrained fn for_each_remove_some() {
-        let mut env = TestEnvironment::new();
-        let scope = env.create_light_account();
-        env.private_context(|context| {
-            let contract_address = context.this_address();
-            let array = CapsuleArray::at(contract_address, SLOT, scope);
-
-            array.push(4);
-            array.push(5);
-            array.push(6);
-
-            array.for_each(|index, _| {
-                if index == 1 {
-                    array.remove(index);
-                }
-            });
-
-            assert_eq(array.len(), 2);
-            assert_eq(array.get(0), 4);
-            assert_eq(array.get(1), 6);
-        });
-    }
-
-    #[test]
-    unconstrained fn for_each_remove_all() {
-        let mut env = TestEnvironment::new();
-        let scope = env.create_light_account();
-        env.private_context(|context| {
-            let contract_address = context.this_address();
-            let array = CapsuleArray::at(contract_address, SLOT, scope);
-
-            array.push(4);
-            array.push(5);
-            array.push(6);
-
-            array.for_each(|index, _| { array.remove(index); });
-
-            assert_eq(array.len(), 0);
-        });
-    }
-
-    #[test]
-    unconstrained fn for_each_remove_all_no_copy() {
-        let mut env = TestEnvironment::new();
-        let scope = env.create_light_account();
-        env.private_context(|context| {
-            let contract_address = context.this_address();
-            let array = CapsuleArray::at(contract_address, SLOT, scope);
-
-            array.push(4);
-            array.push(5);
-            array.push(6);
-
-            // We test that the aztec_utl_copyCapsule was never called, which is the expensive operation we want to
-            // avoid.
-            let mock = std::test::OracleMock::mock("aztec_utl_copyCapsule");
-
-            array.for_each(|index, _| { array.remove(index); });
-
-            assert_eq(mock.times_called(), 0);
+            assert_eq(called_with.get(0), (0, 4));
+            assert_eq(called_with.get(1), (1, 5));
+            assert_eq(called_with.get(2), (2, 6));
         });
     }
 

noir-projects/aztec-nr/aztec/src/messages/discovery/partial_notes.nr

@@ -95,7 +95,13 @@ pub(crate) unconstrained fn fetch_and_process_partial_note_completion_logs(
     // index. Each inner array contains all matching LogRetrievalResponses.
     assert_eq(completion_logs.len(), pending_partial_notes.len());
 
-    completion_logs.for_each(|i, logs_for_tag: EphemeralArray<LogRetrievalResponse>| {
+    // Note: the loop runs backwards because it removes completed entries from `pending_partial_notes`, which is
+    // index-aligned with `completion_logs`. Going from last to first keeps the indices of not-yet-visited entries
+    // stable.
+    let mut i = completion_logs.len();
+    while i > 0 {
+        i -= 1;
+        let logs_for_tag: EphemeralArray<LogRetrievalResponse> = completion_logs.get(i);
         let pending_partial_note = pending_partial_notes.get(i);
         let num_logs = logs_for_tag.len();
 
@@ -172,5 +178,5 @@ pub(crate) unconstrained fn fetch_and_process_partial_note_completion_logs(
             // delete the pending work entry, regardless of whether it was actually completed or not.
             pending_partial_notes.remove(i);
         }
-    });
+    }
 }

noir-projects/aztec-nr/aztec/src/messages/processing/mod.nr

@@ -155,23 +155,15 @@ pub(crate) unconstrained fn get_pending_partial_notes_completion_logs(
 ) -> EphemeralArray<EphemeralArray<LogRetrievalResponse>> {
     let log_retrieval_requests = EphemeralArray::at(LOG_RETRIEVAL_REQUESTS_ARRAY_BASE_SLOT);
 
-    // We create a LogRetrievalRequest for each PendingPartialNote in the EphemeralArray. Because we need the indices
-    // in the request array to match the indices in the partial note array, we can't use EphemeralArray::for_each, as
-    // that function has arbitrary iteration order. Instead, we manually iterate the array from the beginning and push
-    // into the requests array, which we expect to be empty.
-    let mut i = 0;
-    let pending_partial_notes_count = pending_partial_notes.len();
-    while i < pending_partial_notes_count {
-        let pending_partial_note = pending_partial_notes.get(i);
+    pending_partial_notes.for_each(|_i, pending_partial_note| {
         // Partial note completion logs are emitted with a domain-separated tag. To find matching logs, we apply the
         // same domain separation to the stored raw tag.
         let log_tag = compute_log_tag(
             pending_partial_note.note_completion_log_tag,
             DOM_SEP__NOTE_COMPLETION_LOG_TAG,
         );
         log_retrieval_requests.push(LogRetrievalRequest::new(contract_address, log_tag));
-        i += 1;
-    }
+    });
 
     let responses = message_processing::get_logs_by_tag(log_retrieval_requests);
 

noir-projects/aztec-nr/aztec/src/unconstrained_array/mod.nr

@@ -130,18 +130,17 @@ where
 
     /// Calls a function on each element of the array.
     ///
-    /// The function `f` is called once with each array value and its corresponding index. Iteration proceeds
-    /// backwards so that it is safe to remove the current element (and only the current element) inside the
-    /// callback.
+    /// The function `f` is called once with each array value and its corresponding index, in order (from the first
+    /// element to the last).
     ///
-    /// It is **not** safe to push new elements from inside the callback.
+    /// Structurally mutating the array from inside the callback (e.g. via `push`, `pop`, `remove` or `clear`) is
+    /// **not** supported: it can cause elements to be skipped, visited more than once, or read out of bounds.
     pub unconstrained fn for_each<Env>(self, f: unconstrained fn[Env](u32, T) -> ())
     where
         T: Deserialize,
     {
-        let mut i = self.len();
-        while i > 0 {
-            i -= 1;
+        let n = self.len();
+        for i in 0..n {
             f(i, self.get(i));
         }
     }

noir-projects/aztec-nr/aztec/src/unconstrained_array/test_helpers.nr

@@ -163,7 +163,7 @@ where
     });
 }
 
-pub(crate) unconstrained fn for_each_called_with_all_elements<Oracle>()
+pub(crate) unconstrained fn for_each_called_with_all_elements_in_order<Oracle>()
 where
     Oracle: ArrayOracles,
 {
@@ -179,51 +179,9 @@ where
         array.for_each(|index, value| { called_with.push((index, value)); });
 
         assert_eq(called_with.len(), 3);
-        assert(called_with.any(|(index, value)| (index == 0) & (value == 4)));
-        assert(called_with.any(|(index, value)| (index == 1) & (value == 5)));
-        assert(called_with.any(|(index, value)| (index == 2) & (value == 6)));
-    });
-}
-
-pub(crate) unconstrained fn for_each_remove_some<Oracle>()
-where
-    Oracle: ArrayOracles,
-{
-    let env = TestEnvironment::new();
-    env.utility_context(|_| {
-        let array: UnconstrainedArray<Field, Oracle> = UnconstrainedArray::at(SLOT);
-
-        array.push(4);
-        array.push(5);
-        array.push(6);
-
-        array.for_each(|index, _| {
-            if index == 1 {
-                array.remove(index);
-            }
-        });
-
-        assert_eq(array.len(), 2);
-        assert_eq(array.get(0), 4);
-        assert_eq(array.get(1), 6);
-    });
-}
-
-pub(crate) unconstrained fn for_each_remove_all<Oracle>()
-where
-    Oracle: ArrayOracles,
-{
-    let env = TestEnvironment::new();
-    env.utility_context(|_| {
-        let array: UnconstrainedArray<Field, Oracle> = UnconstrainedArray::at(SLOT);
-
-        array.push(4);
-        array.push(5);
-        array.push(6);
-
-        array.for_each(|index, _| { array.remove(index); });
-
-        assert_eq(array.len(), 0);
+        assert_eq(called_with.get(0), (0, 4));
+        assert_eq(called_with.get(1), (1, 5));
+        assert_eq(called_with.get(2), (2, 6));
     });
 }
 

noir-projects/contract-snapshots/tests/snapshots/expand/token_contract/snapshots__expanded.snap

@@ -4035,6 +4035,18 @@ mod test {
             utils::check_private_balance(env, token_contract_address, owner, 0_u128);
         }
 
+        /// Completes multiple partial notes in a single message-discovery sync: the recipient does not sync until both
+        /// completion logs have been emitted, so both pending partial notes are processed together.
+        #[test]
+        unconstrained fn transfer_to_private_external_orchestration_multiple_notes() {
+            let (env, token_contract_address, owner, recipient, amount): (aztec::test::helpers::test_environment::TestEnvironment, aztec::protocol::address::AztecAddress, aztec::protocol::address::AztecAddress, aztec::protocol::address::AztecAddress, u128) = utils::setup_and_mint_to_public(false);
+            let partial_uint_note_a: PartialUintNote = env.call_private(owner, Token::at(token_contract_address).prepare_private_balance_increase(recipient));
+            let partial_uint_note_b: PartialUintNote = env.call_private(owner, Token::at(token_contract_address).prepare_private_balance_increase(recipient));
+            env.call_public(owner, Token::at(token_contract_address).finalize_transfer_to_private(amount - 3_u128, partial_uint_note_a));
+            env.call_public(owner, Token::at(token_contract_address).finalize_transfer_to_private(3_u128, partial_uint_note_b));
+            utils::check_private_balance(env, token_contract_address, recipient, amount);
+        }
+
         #[test(should_fail_with = "Invalid partial note or completer")]
         unconstrained fn transfer_to_private_transfer_not_prepared() {
             let (env, token_contract_address, owner, _, amount): (aztec::test::helpers::test_environment::TestEnvironment, aztec::protocol::address::AztecAddress, aztec::protocol::address::AztecAddress, aztec::protocol::address::AztecAddress, u128) = utils::setup_and_mint_to_public(false);

noir-projects/noir-contracts/contracts/app/token_contract/src/test/transfer_to_private.nr

@@ -59,6 +59,29 @@ unconstrained fn transfer_to_private_from_private_external_orchestration() {
     utils::check_private_balance(env, token_contract_address, owner, 0);
 }
 
+/// Completes multiple partial notes in a single message-discovery sync: the recipient does not sync until both
+/// completion logs have been emitted, so both pending partial notes are processed together.
+#[test]
+unconstrained fn transfer_to_private_external_orchestration_multiple_notes() {
+    // Setup without account contracts. We are not using authwits here, so dummy accounts are enough
+    let (env, token_contract_address, owner, recipient, amount) =
+        utils::setup_and_mint_to_public(/* with_account_contracts */ false);
+
+    let partial_uint_note_a =
+        env.call_private(owner, Token::at(token_contract_address).prepare_private_balance_increase(recipient));
+    let partial_uint_note_b =
+        env.call_private(owner, Token::at(token_contract_address).prepare_private_balance_increase(recipient));
+
+    env.call_public(
+        owner,
+        Token::at(token_contract_address).finalize_transfer_to_private(amount - 3, partial_uint_note_a),
+    );
+    env.call_public(owner, Token::at(token_contract_address).finalize_transfer_to_private(3, partial_uint_note_b));
+
+    // Recipient's private balance should be equal to the sum of both finalized amounts
+    utils::check_private_balance(env, token_contract_address, recipient, amount);
+}
+
 #[test(should_fail_with = "Invalid partial note or completer")]
 unconstrained fn transfer_to_private_transfer_not_prepared() {
     // Setup without account contracts. We are not using authwits here, so dummy accounts are enough