revert: zeroing roots

[ananas-block]

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Improved root history management in batched merkle tree operations to prevent invalid error states.

• Refactor

  • Simplified error handling by removing an unused error variant.
  • Streamlined internal method signatures for enhanced code reliability.

[coderabbitai]

Walkthrough

Error variant CannotZeroCompleteRootHistory is removed, and the zero_out_roots helper method is converted from a fallible to infallible function. The method's iteration range is adjusted from 0..num_remaining_roots to 1..num_remaining_roots, and call sites are updated accordingly with an explicit consistency assertion added.

Changes

Cohort / File(s)	Summary
Error Enum Cleanup program-libs/batched-merkle-tree/src/errors.rs	Removed CannotZeroCompleteRootHistory variant from BatchedMerkleTreeError enum and its corresponding error code mapping (14313) in the From<BatchedMerkleTreeError> for u32 impl.
Merkle Tree Logic Refactoring program-libs/batched-merkle-tree/src/merkle_tree.rs	Converted zero_out_roots from a fallible method returning Result<(), BatchedMerkleTreeError> to an infallible method returning (). Updated iteration range from 0..num_remaining_roots to 1..num_remaining_roots. Replaced error-handling path with explicit assertion for internal consistency. Adjusted call sites to pass root_index directly.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Areas requiring extra attention:

• Iteration range change (0 → 1): Verify that skipping the first root in the clearing loop is intentional and doesn't inadvertently leave a root unzeroed that should be cleared.
• Removed error case: Confirm that the condition num_remaining_roots >= root_history.len() will never occur at runtime, justifying its removal.
• Call site consistency: Ensure all invocations of zero_out_roots correctly pass root_index and that the updated calling convention aligns with intended semantics.
• Assertion replacement: Validate that the new assert!(oldest_index == first_safe_root_index) provides sufficient runtime safety guarantees that the removed error was previously checking.

Suggested labels

ai-review

Suggested reviewers

• sergeytimoshin

Poem

🌿 One error banished, logic made lean,
Iteration shifts from zero to one—
Roots zeroed cleaner, assertions serene,
Simplification's gentle work is done.
No more fallible, the helper stands tall,
Silent assertions catch it all. ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'revert: zeroing roots' accurately summarizes the main change—reverting modifications to the root-zeroing logic by removing a related error variant and simplifying the zero_out_roots function signature.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 70.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch jorrit/revert-zeroing-roots

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f30d22b and f7ef60a.

⛔ Files ignored due to path filters (2)

• program-tests/batched-merkle-tree-test/tests/e2e_tests/shared.rs is excluded by none and included by none
• program-tests/batched-merkle-tree-test/tests/e2e_tests/state.rs is excluded by none and included by none

📒 Files selected for processing (2)

• program-libs/batched-merkle-tree/src/errors.rs (0 hunks)
• program-libs/batched-merkle-tree/src/merkle_tree.rs (4 hunks)

💤 Files with no reviewable changes (1)

• program-libs/batched-merkle-tree/src/errors.rs

🧰 Additional context used

🪛 GitHub Actions: rust

program-libs/batched-merkle-tree/src/merkle_tree.rs

[error] 1230-1230: Rust test failure in light-batched-merkle-tree: test_zero_out. Assertion 'left == right' failed. Command 'cargo test -p light-batched-merkle-tree --features test-only' exited with code 101.

⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_address", "c...
• GitHub Check: programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
• GitHub Check: programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test -- func...
• GitHub Check: programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
• GitHub Check: programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-system-prog...

🔇 Additional comments (3)

program-libs/batched-merkle-tree/src/merkle_tree.rs (3)

750-750: Function signature change looks correct.

Converting from fallible to infallible function is appropriate here since root zeroing is an internal operation with a defensive assertion to catch logic errors. The change aligns with the PR objective to revert previous complexity.

---

769-772: Defensive assertion is sound but depends on fixing the loop bug.

This assertion correctly validates the zeroing logic by ensuring oldest_root_index reaches first_safe_root_index after the loop completes. However, it will fail with the current off-by-one error at line 763. Once the loop range is corrected, this assertion provides good defensive verification.

---

811-840: Call site changes look correct.

The updates to zero_out_previous_batch_bloom_filter properly accommodate the new signature of zero_out_roots. The parameters passed at line 840 are appropriate:

• seq: sequence number marking when the batch was finalized
• root_index: the index of the first safe root (last root of the previous batch)

The whitespace and logic flow changes are clean.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Critical: Off-by-one error causes under-zeroing of roots and test failure.

The loop range change from 0..num_remaining_roots to 1..num_remaining_roots reduces iterations by one, leaving one overlapping root unzeroed. This creates a security vulnerability where proofs can be generated for values that should have been invalidated.

Evidence:

• Pipeline failure: test_zero_out assertion at line 1230
• Your own comment example (lines 728-748) shows 5 roots need zeroing when num_remaining_roots = 5
• New code only performs 4 iterations (1..5 means indices 1,2,3,4)
• The assertion at line 769-772 will fail because oldest_root_index will be one position before first_safe_root_index

The comment at lines 761-762 says "skip one iteration we don't need to zero out the first safe root," but the loop logic doesn't correctly implement this. The issue is that num_remaining_roots already represents the count of unsafe roots to zero, so reducing iterations by 1 under-counts.

Fix: Revert to the original loop range:

-            for _ in 1..num_remaining_roots {
+            for _ in 0..num_remaining_roots {
                 self.root_history[oldest_root_index] = [0u8; 32];
                 oldest_root_index += 1;
                 oldest_root_index %= self.root_history.len();
             }

If you truly need to skip zeroing the first safe root, then adjust the count calculation instead:

             let num_remaining_roots = sequence_number - self.sequence_number;
+            // Subtract 1 to skip zeroing the first safe root
+            let num_roots_to_zero = num_remaining_roots.saturating_sub(1);
             // 2.2. Zero out roots oldest to first safe root index.
             //      Skip one iteration we don't need to zero out
             //      the first safe root.
-            for _ in 1..num_remaining_roots {
+            for _ in 0..num_roots_to_zero {

But based on the comment block documentation and test expectations, the first approach (reverting to 0..num_remaining_roots) is correct.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	for _ in 1..num_remaining_roots {
	for _ in 0..num_remaining_roots {
	self.root_history[oldest_root_index] = [0u8; 32];
	oldest_root_index += 1;
	oldest_root_index %= self.root_history.len();
	}

🤖 Prompt for AI Agents

In program-libs/batched-merkle-tree/src/merkle_tree.rs around line 763, the
for-loop was changed from 0..num_remaining_roots to 1..num_remaining_roots
causing an off-by-one that leaves one unsafe root unzeroed; revert the loop
range back to 0..num_remaining_roots so it iterates exactly num_remaining_roots
times and zeroes all unsafe roots, or alternatively adjust the calculation of
num_remaining_roots (increment by one) if you intend to skip the first safe
root—prefer reverting to 0..num_remaining_roots to match the documented behavior
and tests.