[storage/mmr] change pruning API to be location instead of position based#3309
Open
roberto-bayardo wants to merge 2 commits intomainfrom
Open
[storage/mmr] change pruning API to be location instead of position based#3309roberto-bayardo wants to merge 2 commits intomainfrom
roberto-bayardo wants to merge 2 commits intomainfrom
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
commonware-mcp | 8d605a9 | Mar 07 2026, 04:09 AM |
roberto-bayardo
force-pushed
the
prune-api
branch
from
a452c75 to
7a655af
Compare
Deploying monorepo with
|
| Latest commit: |
8d605a9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f508ffd0.monorepo-eu0.pages.dev |
| Branch Preview URL: | https://prune-api.monorepo-eu0.pages.dev |
roberto-bayardo
requested review from
Copilot and
danlaine
and removed request for
danlaine
roberto-bayardo
added
breaking-format
roberto-bayardo
force-pushed
the
prune-api
branch
2 times, most recently
from
ac621cf to
2466678
Compare
roberto-bayardo
force-pushed
the
prune-api
branch
2 times, most recently
from
64385ba to
a3fe75c
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Collaborator
Author
|
bugbot run |
roberto-bayardo
force-pushed
the
prune-api
branch
from
a3fe75c to
31aee02
Compare
roberto-bayardo
force-pushed
the
prune-api
branch
4 times, most recently
from
50c06b8 to
4dfe3b0
Compare
roberto-bayardo
force-pushed
the
prune-api
branch
from
4dfe3b0 to
8d605a9
Compare
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #3309 +/- ##
=======================================
Coverage 93.25% 93.26%
=======================================
Files 423 423
Lines 146470 146499 +29
Branches 3455 3458 +3
=======================================
+ Hits 136597 136626 +29
Misses 8779 8779
Partials 1094 1094
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| let config = Config { | ||
| nodes, | ||
| pruned_to_pos: Position::new(input.pruned_to_pos), | ||
| pruned_to: Location::new(input.pruned_to_pos), |
Contributor
There was a problem hiding this comment.
I think we need to change the name of of pruned_to_pos?
| hasher, | ||
| )?; | ||
| let prune_pos = Position::new(effective_prune_pos); | ||
| let prune_pos = effective_prune_pos; |
Contributor
There was a problem hiding this comment.
does this need to be a new var?
Contributor
There was a problem hiding this comment.
LLM claims an issue:
• The patch mostly updates pruning APIs from positions to locations, but it introduces one data-loss path in init_sync() for non-leaf boundaries and weakens mem::Mmr::prune()'s documented error handling for invalid locations. Those issues make the patch unsafe to consider fully correct.
Full review comments:
- [P2] Validate sync boundary before clearing persisted data — /Users/patrickogrady/code/monorepo-pr-3309/storage/src/mmr/journaled.rs:409-410
If init_sync() is called with a non-leaf SyncConfig.range.start and the existing journal is empty or still behind that boundary, this branch calls clear_to_size(*cfg.range.start) before the later Location::try_from(cfg.range.start) validation runs. The function then returns an error
after it has already truncated/reinitialized the persisted journal, so a caller using a node-position boundary (which the public Range<Position> API still permits) can lose on-disk data on a failed sync attempt.
- [P3] Check prune input validity before returning LeafOutOfBounds — /Users/patrickogrady/code/monorepo-pr-3309/storage/src/mmr/mem.rs:331-334
If a caller passes Location::new(u64::MAX) or any other value above MAX_LOCATION, this branch returns LeafOutOfBounds as soon as the tree has fewer leaves than that value, because the Position::try_from(loc) validation now happens afterwards. That breaks the documented LocationOverflow
contract of Mmr::prune() and makes malformed locations indistinguishable from an ordinary out-of-range prune request.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes the MMR pruning API to align the pruning boundary to a leaf (Location), and to return Error on bad input instead of panicking (since it's pub). This is more consistent with how proving works (over locations). Also changes bounds() to be location based since begin/end are now always leaf aligned.