Uh oh!
There was an error while loading. Please reload this page.
revert: "fix: batched merkle tree zeroing" - #2058
Conversation
This reverts commit 435e679.
WalkthroughThe pull request removes error handling from the root-zeroing operation in the batched Merkle tree implementation. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
program-libs/batched-merkle-tree/src/merkle_tree.rs (2)
827-840: Major: Semantic mismatch between call site and function parameter.Line 840 passes
root_indexto a function whose parameter is namedfirst_safe_root_index(line 750). This semantic mismatch suggests:
- Either the parameter should be renamed to reflect what's actually passed, or
- The wrong value is being passed
The previous implementation likely calculated a distinct
first_safe_root_indexvalue. Passingroot_indexdirectly may not correctly identify which root is "safe" to keep, potentially affecting the correctness of the zeroing logic.Either rename the function parameter to
root_indexfor clarity, or verify thatroot_indexandfirst_safe_root_indexare semantically equivalent in this context:-fn zero_out_roots(&mut self, sequence_number: u64, first_safe_root_index: u32) {+fn zero_out_roots(&mut self, sequence_number: u64, root_index: u32) {
1148-1508: Minor: Debug println statements left in test code.Multiple
println!statements are scattered throughout the test code (lines 1167, 1220-1223, 1227, 1288, 1302, 1322, 1404, 1471, 1480, 1503), suggesting debugging artifacts that should be removed before merging.Remove debug println statements or replace them with proper test logging if the output is needed:
- println!("root_index: {}", root_index);- println!(- "currently inserted elements: {:?}",- account.queue_batches.batches[1].get_num_inserted_elements()- );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (11)
Cargo.lockis excluded by!**/*.lockand included by noneprogram-tests/batched-merkle-tree-test/Cargo.tomlis excluded by none and included by noneprogram-tests/batched-merkle-tree-test/tests/e2e.rsis excluded by none and included by noneprogram-tests/batched-merkle-tree-test/tests/e2e_tests/address.rsis excluded by none and included by noneprogram-tests/batched-merkle-tree-test/tests/e2e_tests/e2e.rsis excluded by none and included by noneprogram-tests/batched-merkle-tree-test/tests/e2e_tests/mod.rsis excluded by none and included by noneprogram-tests/batched-merkle-tree-test/tests/e2e_tests/shared.rsis excluded by none and included by noneprogram-tests/batched-merkle-tree-test/tests/e2e_tests/simulate_txs.rsis excluded by none and included by noneprogram-tests/batched-merkle-tree-test/tests/e2e_tests/state.rsis excluded by none and included by noneprogram-tests/batched-merkle-tree-test/tests/merkle_tree.rsis excluded by none and included by noneprogram-tests/registry-test/tests/tests.rsis 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(11 hunks)
💤 Files with no reviewable changes (1)
- program-libs/batched-merkle-tree/src/errors.rs
🧰 Additional context used
🧬 Code graph analysis (1)
program-libs/batched-merkle-tree/src/merkle_tree.rs (1)
program-libs/batched-merkle-tree/src/batch.rs (1)
get_num_inserted_elements(258-260)
⏰ 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). (9)
- 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 (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-system-prog...
- GitHub Check: programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
- GitHub Check: Forester e2e test
- GitHub Check: lint
- GitHub Check: Test program-libs-slow
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-fast
🔇 Additional comments (1)
program-libs/batched-merkle-tree/src/merkle_tree.rs (1)
750-774: Critical: Off-by-one error in security-critical root zeroing logic.The loop change from
0..num_remaining_rootsto1..num_remaining_roots(line 763) zeros one fewer root than before. According to the extensive security documentation (lines 700-749), all overlapping roots must be zeroed to prevent proving inclusion of nullified values after bloom filters are cleared.The comment "Skip one iteration we don't need to zero out the first safe root" (line 761-762) appears inconsistent with the assertion at line 769-772, which verifies that
oldest_root_indexadvances to exactly matchfirst_safe_root_index. If we skip zeroing the first root but still expect to reachfirst_safe_root_index, the invariant may be violated.This off-by-one change could leave an exploitable root in the history that allows proving inclusion of values that should have been invalidated—a critical security vulnerability in a Merkle tree implementation.
Verify this change doesn't violate the security invariant by checking:
- Whether the assertion at line 769 can still pass with the reduced loop iterations
- Whether any root that should be zeroed is left non-zero
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Nsthjuhh
commented
Nov 18, 2025
|
1 similar comment
Nsthjuhh
commented
Nov 18, 2025
|
Reverts #2045
Summary by CodeRabbit