refactor: address rollup circuit audit comments (#20077)

- Fix incorrect error message
- Rename `copy_items_into_array` to `splice_at_count` and simplify it

Other changes not from the audit
- Use `splice_at_count` for merging non-revertible/revertible log arrays
- Remove the confusing and more expensive `array_merge`

Author: LeilaWang

noir-projects/noir-protocol-circuits/crates/rollup-lib/src/checkpoint_merge/utils/merge_checkpoint_rollups.nr

@@ -1,5 +1,5 @@
 use crate::abis::CheckpointRollupPublicInputs;
-use types::{constants::MAX_CHECKPOINTS_PER_EPOCH, utils::arrays::copy_items_into_array};
+use types::{constants::MAX_CHECKPOINTS_PER_EPOCH, utils::arrays::splice_at_count};
 
 pub fn merge_checkpoint_rollups(
     left: CheckpointRollupPublicInputs,
@@ -8,11 +8,8 @@ pub fn merge_checkpoint_rollups(
     let num_left_checkpoints = left.num_checkpoints() as u32;
     let num_right_checkpoints = right.num_checkpoints() as u32;
 
-    // Make sure that the total number of checkpoints does not exceed the maximum allowed in an epoch, to prevent the
-    // merged arrays (`checkpoint_header_hashes`, `out_hashes`, `fees`) from being truncated within the below calls to
-    // `copy_items_into_array`.
+    // Make sure that the total number of checkpoints does not exceed the maximum allowed in an epoch.
     let num_checkpoints = num_left_checkpoints + num_right_checkpoints;
-
     assert(
         num_checkpoints <= MAX_CHECKPOINTS_PER_EPOCH,
         "total number of checkpoints exceeds max allowed in an epoch",
@@ -24,27 +21,32 @@ pub fn merge_checkpoint_rollups(
     // (which is the only property that doesn't have empty values), for the number of fees.
     // Hence, we don't need to assert left.num_fees == num_left_checkpoints and right.num_fees == num_right_checkpoints.
 
-    // We use `copy_items_into_array` here because we know exactly how many items should be taken from each side when
-    // merging checkpoints.
-    // It is especially critical for `fees` to copy the exact amount of items from both checkpoints:
-    // The L1 contracts will process fees by index, assuming each entry corresponds to the checkpoint at the same
-    // position. However, a checkpoint may have zero fees and recipient as it is not prohibited by the circuit.
-    // Therefore, preserving empty values is important.
-    // Using this helper (instead of helper like `array_merge`) ensures those empty entries are copied over.
+    // `splice_at_count` uses the number of **left** checkpoints to decide exactly how many items to include from each
+    // side. This approach is more efficient than merging all non-empty items, and, importantly, it ensures the correct
+    // alignment of array items. It would be incorrect to simply merge non-empty items.
+    // For example, while header hashes are always present, the `fees` array can have empty (zeroed) entries that must
+    // be preserved at their original indices. The L1 contract expects the fee at each index to correspond to the
+    // checkpoint at the same index, even if that fee (or recipient) is zero. Therefore, we must always preserve all fee
+    // items during the merge. Merging by `num_checkpoints` guarantees that array indices in all merged arrays remain
+    // properly aligned.
 
-    let checkpoint_header_hashes = copy_items_into_array(
+    let checkpoint_header_hashes = splice_at_count(
         left.checkpoint_header_hashes,
         num_left_checkpoints,
         right.checkpoint_header_hashes,
-        num_right_checkpoints,
+    );
+    // Sanity check that the length of the array is defined correctly.
+    std::static_assert(
+        checkpoint_header_hashes.len() == MAX_CHECKPOINTS_PER_EPOCH,
+        "The length of the checkpoint header hashes array does not match the constant MAX_CHECKPOINTS_PER_EPOCH",
     );
 
     // We can't merge fees for the same recipient since the l1 contract iterates per checkpoint.
-    let fees = copy_items_into_array(
-        left.fees,
-        num_left_checkpoints,
-        right.fees,
-        num_right_checkpoints,
+    let fees = splice_at_count(left.fees, num_left_checkpoints, right.fees);
+    // Sanity check that the length of the array is defined correctly.
+    std::static_assert(
+        fees.len() == MAX_CHECKPOINTS_PER_EPOCH,
+        "The length of the fees array does not match the constant MAX_CHECKPOINTS_PER_EPOCH",
     );
 
     CheckpointRollupPublicInputs {

noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/log_array_length.nr

@@ -0,0 +1,27 @@
+use types::{
+    abis::{log_hash::LogHash, private_log::PrivateLog},
+    side_effect::Scoped,
+    utils::arrays::array_length_until,
+};
+
+// The logic in this file relies on the property that log arrays are "dense-trimmed": all logs with non-zero length
+// appear first, followed by logs with zero length (used as padding).
+// This structure is enforced by the Tail-to-Public kernel circuit and remains intact even after array concatenation in
+// the public_tx_effect_builder. As a result, we can safely use the index of the first zero-length log to determine the
+// logical length of the logs array.
+//
+// @dev: Unlike other smaller arrays (note hashes, nullifiers, and l2-to-l1 messages) that use `array_length`, it is
+// more efficient for log arrays to use `array_length_until` and iterate through all lengths (u32).
+// `array_length_until` is also more efficient than an optimized version of `array_length` that would call the
+// unconstrained `find_first_index` and then validate the result by checking just the lengths of two adjacent elements
+// (rather than verifying whether the entire log is empty).
+
+/// Returns the number of private logs with length > 0.
+pub fn get_private_log_array_length<let N: u32>(array: [PrivateLog; N]) -> u32 {
+    array_length_until(array, |log| log.length == 0)
+}
+
+/// Returns the number of contract class log hashes with length > 0.
+pub fn get_contract_class_log_hash_array_length<let N: u32>(array: [Scoped<LogHash>; N]) -> u32 {
+    array_length_until(array, |log| log.inner.length == 0)
+}

noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/mod.nr

@@ -1,4 +1,5 @@
 pub mod fees;
+pub mod log_array_length;
 pub mod private_tail_validator;
 pub mod private_tx_effect_builder;
 pub mod public_tx_effect_builder;

noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/private_tail_validator.nr

@@ -65,7 +65,7 @@ pub fn validate_tx_constant_data(
             },
             block_last_archive_root,
         ),
-        "Membership check failed: previous block hash not found in archive tree",
+        "Membership check failed: anchor block header hash not found in archive tree",
     );
 
     assert_eq(tx_chain_id, block_chain_id, "Mismatched chain_id between kernel and rollup");

noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/private_tx_effect_builder.nr

@@ -1,4 +1,7 @@
 use crate::tx_base::components::fees::compute_transaction_fee;
+use super::log_array_length::{
+    get_contract_class_log_hash_array_length, get_private_log_array_length,
+};
 use types::{
     abis::{
         block_constant_data::BlockConstantData, contract_class_log::ContractClassLog,
@@ -13,7 +16,7 @@ use types::{
     data::public_data_tree_leaf_preimage::PublicDataTreeLeafPreimage,
     hash::compute_l2_to_l1_message_hash,
     traits::{Empty, Hash},
-    utils::arrays::{array_length, array_length_until},
+    utils::arrays::array_length,
 };
 
 pub fn build_tx_effect(
@@ -80,23 +83,23 @@ pub fn build_tx_effect(
         contract_class_logs,
     };
 
-    // We can use `array_length_until` to check only the length, because the kernel circuits guarantee that logs with
-    // non-zero length are left-packed. In other words, there cannot be a log with length 0 before a log with non-zero
-    // length.
-    let private_logs_array_length =
-        array_length_until(accumulated_data.private_logs, |log| log.length == 0);
-    let contract_class_logs_array_length = array_length_until(
-        accumulated_data.contract_class_logs_hashes,
-        |log| log.inner.length == 0,
-    );
+    // The Tail kernel circuit guarantees that all accumulated data arrays are "dense-trimmed": all actual data emitted
+    // by the tx are non-empty and appear first, followed by empty padding elements. This means it is safe to use
+    // `array_length` to get the true logical length of these arrays.
+    // For log arrays, we use specialized functions (get_private_log_array_length,
+    // get_contract_class_log_hash_array_length) that efficiently determine the length by finding the first zero-length
+    // log. Because the kernel enforces that only actual logs have nonzero length and all padding logs have zero length,
+    // these functions safely provide the correct count, independent of the log contents themselves.
 
     let tx_effect_array_lengths = TxEffectArrayLengths {
         note_hashes: array_length(accumulated_data.note_hashes),
         nullifiers: array_length(accumulated_data.nullifiers),
         l2_to_l1_msgs: array_length(l2_to_l1_msgs),
         public_data_writes: 1, // No public functions were called. This is the fee_payer's FeeJuice balance decrementation.
-        private_logs: private_logs_array_length,
-        contract_class_logs: contract_class_logs_array_length,
+        private_logs: get_private_log_array_length(accumulated_data.private_logs),
+        contract_class_logs: get_contract_class_log_hash_array_length(
+            accumulated_data.contract_class_logs_hashes,
+        ),
     };
 
     (tx_effect, tx_effect_array_lengths)

noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/components/public_tx_effect_builder.nr

@@ -1,4 +1,7 @@
 use crate::tx_base::components::private_tail_validator;
+use super::log_array_length::{
+    get_contract_class_log_hash_array_length, get_private_log_array_length,
+};
 use types::{
     abis::{
         avm_circuit_public_inputs::AvmCircuitPublicInputs, contract_class_log::ContractClassLog,
@@ -8,7 +11,7 @@ use types::{
     constants::{CONTRACT_CLASS_LOG_SIZE_IN_FIELDS, MAX_CONTRACT_CLASS_LOGS_PER_TX},
     hash::compute_l2_to_l1_message_hash,
     traits::Hash,
-    utils::arrays::{array_length_until, array_merge},
+    utils::arrays::splice_at_count,
 };
 
 pub fn build_tx_effect(
@@ -49,13 +52,25 @@ pub fn build_tx_effect(
     // are conditionally discarded by the avm circuit. Private logs and contract class logs are
     // different, in that they're only emitted by private functions and so the avm doesn't care about them
     // or "see" them.
+    //
+    // Note: splice_at_count is safe to use here because both input arrays were originally split from a single
+    // dense-trimmed array [T; MAX_PER_TX] in the Tail-to-Public kernel circuit.
+    // (For general arrays, dense-trimmed means all non-empty entries appear before any empty entries; for logs, all
+    // logs with nonzero length appear before any zero-length logs.)
+    // The Tail-to-Public kernel ensures the 2 split arrays are also dense-trimmed.
+    // This guarantees that merging them will never produce more than MAX_PER_TX items and no actual values will be lost
+    // or truncated. And after concatenation, the resulting array remains dense-trimmed.
 
     // Conditionally discard the revertible private logs.
     let private_logs = if reverted {
         private_to_public.non_revertible_accumulated_data.private_logs
     } else {
-        array_merge(
+        let num_non_revertible_private_logs = get_private_log_array_length(
             private_to_public.non_revertible_accumulated_data.private_logs,
+        );
+        splice_at_count(
+            private_to_public.non_revertible_accumulated_data.private_logs,
+            num_non_revertible_private_logs,
             private_to_public.revertible_accumulated_data.private_logs,
         )
     };
@@ -64,8 +79,12 @@ pub fn build_tx_effect(
     let contract_class_log_hashes = if reverted {
         private_to_public.non_revertible_accumulated_data.contract_class_logs_hashes
     } else {
-        array_merge(
+        let num_non_revertible_contract_class_log_hashes = get_contract_class_log_hash_array_length(
+            private_to_public.non_revertible_accumulated_data.contract_class_logs_hashes,
+        );
+        splice_at_count(
             private_to_public.non_revertible_accumulated_data.contract_class_logs_hashes,
+            num_non_revertible_contract_class_log_hashes,
             private_to_public.revertible_accumulated_data.contract_class_logs_hashes,
         )
     };
@@ -102,12 +121,9 @@ pub fn build_tx_effect(
         contract_class_logs,
     };
 
-    // We can use `array_length_until` to check only the length, because the kernel circuits guarantee that logs with
-    // non-zero length are left-packed. In other words, there cannot be a log with length 0 before a log with non-zero
-    // length.
-    let private_logs_array_length = array_length_until(private_logs, |log| log.length == 0);
+    let private_logs_array_length = get_private_log_array_length(private_logs);
     let contract_class_logs_array_length =
-        array_length_until(contract_class_log_hashes, |log| log.inner.length == 0);
+        get_contract_class_log_hash_array_length(contract_class_log_hashes);
 
     let tx_effect_array_lengths = TxEffectArrayLengths {
         note_hashes: avm.accumulated_data_array_lengths.note_hashes,

noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/tests/private_tx_base/validate_private_tail_tests.nr

@@ -1,7 +1,7 @@
 use super::TestBuilder;
 use types::{constants::AVM_MAX_PROCESSABLE_L2_GAS, tests::fixture_builder::FixtureBuilder};
 
-#[test(should_fail_with = "Membership check failed: previous block hash not found in archive tree")]
+#[test(should_fail_with = "Membership check failed: anchor block header hash not found in archive tree")]
 unconstrained fn anchor_block_header_not_in_archive() {
     let mut builder = TestBuilder::new();
 

noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/tests/public_tx_base/end_blob_sponge_tests.nr

@@ -9,7 +9,7 @@ use types::{
     },
     tests::{FixtureBuilder, utils::pad_end},
     traits::{Hash, ToField},
-    utils::arrays::array_merge,
+    utils::arrays::subarray,
 };
 
 #[test]
@@ -100,10 +100,15 @@ unconstrained fn full_tx_effects() {
     }
     offset += MAX_TOTAL_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX * 2;
 
-    let private_logs = array_merge(
+    // Concatenate the 2 non-revertible and (MAX_PRIVATE_LOGS_PER_TX - 2) revertible private logs.
+    let private_logs = subarray::<_, _, 2>(
         builder.private_tail.non_revertible_accumulated_data.private_logs,
-        builder.private_tail.revertible_accumulated_data.private_logs,
-    );
+        0,
+    )
+        .concat(subarray::<_, _, MAX_PRIVATE_LOGS_PER_TX - 2>(
+            builder.private_tail.revertible_accumulated_data.private_logs,
+            0,
+        ));
     for i in 0..MAX_PRIVATE_LOGS_PER_TX {
         expected_blob_fields[offset] = PRIVATE_LOG_SIZE_IN_FIELDS as Field;
         offset += 1;

noir-projects/noir-protocol-circuits/crates/rollup-lib/src/tx_base/tests/public_tx_base/validate_private_tail_to_public_tests.nr

@@ -1,7 +1,7 @@
 use super::TestBuilder;
 use types::{constants::AVM_MAX_PROCESSABLE_L2_GAS, tests::fixture_builder::FixtureBuilder};
 
-#[test(should_fail_with = "Membership check failed: previous block hash not found in archive tree")]
+#[test(should_fail_with = "Membership check failed: anchor block header hash not found in archive tree")]
 unconstrained fn anchor_block_header_not_in_archive() {
     let mut builder = TestBuilder::new();