Skip to content

fix(rotortree): read snapshot root at maximum depth - #51

Merged
rymnc merged 1 commit into
ethsystems:mainfrom
arunimshukla:fix/rotortree-snapshot-root-level
Oct 13, 2026
Merged

rymnc merged 1 commit into
ethsystems:mainfrom
arunimshukla:fix/rotortree-snapshot-root-level

Conversation

@arunimshukla

@arunimshukla arunimshukla commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Snapshot root-level accessors could panic after valid insertions reach MAX_DEPTH. The tree accepts that depth, but levels has only MAX_DEPTH slots; the root is also stored separately. Reading levels[depth] at maximum depth indexes beyond that array.

The fix serves root-level reads from the snapshot's existing root value. Empty trees still have zero nodes at level zero; non-empty roots have one. Invalid root indices return IndexOutOfRange. This includes maximum-depth trees before they reach full leaf capacity.

The diff remains limited to the two accessors and their tests.

Review changes

  • Clarified why root-level reads use self.root, including that the branch applies at every depth.
  • Removed proof generation checks from the accessor regression because proof generation does not call either accessor.
  • Combined the two accessor tests into one regression with shared assertions.
  • Removed duplicate above-root checks already covered by snapshot_get_node_and_level_len.
  • Added regression/control descriptions and given/when/then annotations.

Regression coverage

The consolidated regression checks root length, root hash, and invalid root indices through the public snapshot interface. It covers sequential and batch insertion for binary depths one and three and ternary depth two, including every leaf count in each configuration's maximum-depth range. A separate control preserves empty and single-leaf behaviour.

With the revised tests and the upstream implementation at e5061c8, cargo test -p rotortree --test tree snapshot_ reports 1 failed, 3 passed. The maximum-depth regression fails with an index-out-of-bounds panic. Restoring the patch produces 4 passed, 0 failed.

Validation

Rechecked the revised code using Rust 1.95.0:

Check Result
cargo test -p rotortree 107 passed
cargo test -p rotortree --all-features 180 passed
cargo test -p rotortree --no-default-features 107 passed
cargo clippy -p rotortree --all-features -- -D warnings Passed
cargo +nightly fmt --all --check Passed
git diff --check Passed

Counts include doctests where applicable. The test count drops by one because the accessor regressions are consolidated. These are local results; the repository's complete CI feature powerset is checked separately by CI. The updated upstream CI run currently requires maintainer approval to run the fork workflow.

The revised PR contains one commit with a GitHub-verified signature and a sign-off.

@rymnc rymnc self-assigned this Oct 13, 2026

@rymnc rymnc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for your contribution! please sign the commit so that we can merge after you address the comments :)

Comment thread crates/rotortree/src/tree.rs Resolved
Comment thread crates/rotortree/tests/tree.rs Resolved
Comment thread crates/rotortree/tests/tree.rs Resolved
Comment thread crates/rotortree/tests/tree.rs Resolved
Serve root-level snapshot reads from the stored root. Address all review feedback with a focused, shared accessor regression and empty/single-leaf controls.

Validation: default and no-default-feature builds each pass 107 tests; all features pass 180 tests. Clippy with warnings denied, nightly formatting, and diff whitespace checks pass.

Signed-off-by: Arunim Shukla <54760103+arunimshukla@users.noreply.github.com>
@arunimshukla
arunimshukla force-pushed the fix/rotortree-snapshot-root-level branch from bd59550 to 83826e9 Compare October 13, 2026 09:07
@rymnc
rymnc merged commit 1449c62 into ethsystems:main Oct 13, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants