feat(dash-spv): storage truncation primitives for reorg support#790
feat(dash-spv): storage truncation primitives for reorg support#790xdustinface wants to merge 3 commits into
Conversation
Add `truncate_above(height)` to `BlockHeaderStorage`, `FilterHeaderStorage`, `FilterStorage`, and `BlockStorage`, backed by a new `SegmentCache::truncate_above` that drops segments above the target and resets the boundary segment's tail slots to the persistable sentinel. Dropped segment files are queued for deletion on the next `persist`, and `PersistentBlockHeaderStorage` also prunes its `header_hash_index` so orphaned hashes no longer resolve to a height. This is the foundational primitive for upcoming fork/reorg handling in storage. No callers are wired up yet.
|
Warning Review limit reached
More reviews will be available in 48 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a cross-storage ChangesStorage truncate_above implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #790 +/- ##
==========================================
+ Coverage 72.45% 72.59% +0.13%
==========================================
Files 322 322
Lines 70898 71242 +344
==========================================
+ Hits 51368 51715 +347
+ Misses 19530 19527 -3
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
dash-spv/src/storage/segments.rs (1)
422-451:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPropagate persist failures instead of only logging them.
truncate_abovenow documents durability in terms of the next successfulpersist, but this method cannot report failure. Ifremove_fileorsegments.persist()fails, callers continue with an in-memory truncated tip while stale segment files remain on disk, and there is no way to retry or abort based on that failure.As per coding guidelines,
**/*.rs: Use proper error types (thiserror) and propagate errors appropriately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dash-spv/src/storage/segments.rs` around lines 422 - 451, The persist method currently swallows IO errors (from tokio::fs::remove_file) and segment.persist() failures, leaving on-disk state inconsistent; change persist(&mut self, segments_dir: impl Into<PathBuf>) to return Result<(), PersistError> (define a small thiserror enum PersistError with variants for Io { source: std::io::Error, path: PathBuf } and SegmentPersist { id: SegmentId, source: SegmentError } or similar), propagate and return the first/aggregated error instead of only logging, ensure failed deletions keep their ids in self.to_delete (preserve the existing failed.insert and extend behavior) and stop clearing self.evicted/self.segments when persist returns Err so callers can retry; update callers (e.g. truncate_above) to handle the Result and react (retry/abort) accordingly.
🧹 Nitpick comments (4)
dash-spv/src/storage/mod.rs (1)
587-606: ⚡ Quick winTest validates critical truncation behavior.
The test correctly verifies:
- Tip height updates to the truncation target
- Orphaned header hashes are pruned from the index
- Retained header hashes remain accessible
The explicit trait qualification on line 599 is necessary and correct given that
DiskStorageManagerimplements multiple traits withtruncate_above.💡 Optional enhancement: verify persistence of truncation
Consider adding a reload cycle to verify that truncation survives a persist/reopen:
let kept_hash = headers[3].block_hash(); assert_eq!(mgr.get_header_height_by_hash(&kept_hash).await.unwrap(), Some(3)); + + // Verify truncation persists across reload + mgr.shutdown().await; + drop(mgr); + let mgr = DiskStorageManager::new(&config).await.unwrap(); + assert_eq!(mgr.get_tip_height().await, Some(5)); + assert_eq!(mgr.get_header_height_by_hash(&orphaned_hash).await.unwrap(), None); + assert_eq!(mgr.get_header_height_by_hash(&kept_hash).await.unwrap(), Some(3)); }This would confirm that segment file deletion and index updates are durable, though the underlying
SegmentCachetests may already cover this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dash-spv/src/storage/mod.rs` around lines 587 - 606, The explicit trait-qualified call <DiskStorageManager as BlockHeaderStorage>::truncate_above(&mut mgr, 5).await.unwrap() is required and should be kept as-is; ensure you do not change it to an unqualified method call because DiskStorageManager implements multiple traits with truncate_above. Optionally, to further harden the test, add a reload/persist cycle after truncation that reopens DiskStorageManager and asserts the tip height and header-index state still reflect the truncation.dash-spv/src/storage/filters.rs (1)
96-151: 🏗️ Heavy liftExercise the error side of
truncate_aboveas well.Current coverage only validates successful truncation. Please add one regression for
truncate_abovebelowstart_heightand one forload_filterscrossing into a segment queued for deletion after truncation; the trait docs now advertise both as observable error cases.As per coding guidelines,
dash-spv/**/{src,tests}/**/*.rs: Implement comprehensive unit tests in-module for individual components using#[cfg(test)]and integration tests in thetests/directory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dash-spv/src/storage/filters.rs` around lines 96 - 151, Add unit tests that cover the error paths of PersistentFilterStorage::truncate_above and PersistentFilterStorage::load_filters: add one test that calls truncate_above with a start_height less than the current stored tip (e.g., truncate_above with a value below 0 or otherwise invalid) and asserts it returns the expected Err variant, and add another test that calls load_filters for a range that spans into a segment that was truncated/queued for deletion and asserts load_filters returns the documented error; locate tests near the existing tests (e.g., add functions named test_truncate_above_error_below_start and test_load_filters_error_after_truncate) and use the same TempDir + PersistentFilterStorage::open, store_filter, truncate_above, persist/open patterns to reproduce the queued-for-deletion state before calling load_filters.dash-spv/src/storage/block_headers.rs (1)
292-347: ⚡ Quick winCover the rejected truncation path too.
These tests only exercise success and no-op flows. Please add a case where the stored range starts above zero and
truncate_above(start_height - 1)returnsStorageError::InvalidArgument, then assertget_tip_heightandget_header_height_by_hashare unchanged. That protects the extraheader_hash_indexpruning logic from regressing on an error path.As per coding guidelines,
dash-spv/**/{src,tests}/**/*.rs: Implement comprehensive unit tests in-module for individual components using#[cfg(test)]and integration tests in thetests/directory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dash-spv/src/storage/block_headers.rs` around lines 292 - 347, Add a unit test in the same module that stores a non-empty range via PersistentBlockHeaderStorage::store_headers (e.g. BlockHeader::dummy_batch(5..10)), then call PersistentBlockHeaderStorage::truncate_above with start_height - 1 (e.g. 4) and assert it returns StorageError::InvalidArgument; after the failed call assert that get_tip_height and get_header_height_by_hash for an existing header (from the stored batch) are unchanged to ensure the header_hash_index pruning logic does not run on error. Use the same async #[tokio::test] pattern and reference the methods truncate_above, get_tip_height, get_header_height_by_hash, store_headers, and BlockHeader::dummy_batch so the test mirrors the existing style and verifies the rejected truncation path.dash-spv/src/storage/filter_headers.rs (1)
130-183: 🏗️ Heavy liftAdd coverage for the documented error contracts.
The new tests hit the happy path, but not the two failure modes now described by the trait docs:
truncate_above(start_height - 1)returningStorageError::InvalidArgument, andload_filter_headersreturningInvalidArgumentwhen a range crosses into a segment queued for deletion beforepersist. Both are part of the API surface now.As per coding guidelines,
dash-spv/**/{src,tests}/**/*.rs: Implement comprehensive unit tests in-module for individual components using#[cfg(test)]and integration tests in thetests/directory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dash-spv/src/storage/filter_headers.rs` around lines 130 - 183, Add unit tests in this module (filter_headers.rs) to cover the two documented error contracts: assert that PersistentFilterHeaderStorage::truncate_above(start_height - 1) returns StorageError::InvalidArgument, and assert that load_filter_headers(range) returns StorageError::InvalidArgument when the requested range crosses into a segment that was truncated (queued for deletion) before calling persist; use the existing TempDir setup and methods (PersistentFilterHeaderStorage::open, store_filter_headers, truncate_above, load_filter_headers, persist) to create the state that triggers each error and verify the exact StorageError::InvalidArgument variant is returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dash-spv/src/storage/segments.rs`:
- Around line 233-241: The get_items implementation currently only rejects
ranges for segments in to_delete but does not reject ranges that overrun a
partially truncated segment (after truncate_above), causing panics or sentinel
data; update get_items to validate that the requested end offset does not exceed
the segment's current tip/last_valid_offset (same guard logic used around
get_segment_mut/get_item) and return StorageError::InvalidArgument with a clear
message when the range end > last_valid_offset or overall tip; add the same
validation in the analogous code path referenced (the block around the other
guard at 274-282) and add a regression unit test that calls truncate_above
within a segment then calls get_items with an end that lies beyond the truncated
tip to assert it fails fast with the expected error.
---
Outside diff comments:
In `@dash-spv/src/storage/segments.rs`:
- Around line 422-451: The persist method currently swallows IO errors (from
tokio::fs::remove_file) and segment.persist() failures, leaving on-disk state
inconsistent; change persist(&mut self, segments_dir: impl Into<PathBuf>) to
return Result<(), PersistError> (define a small thiserror enum PersistError with
variants for Io { source: std::io::Error, path: PathBuf } and SegmentPersist {
id: SegmentId, source: SegmentError } or similar), propagate and return the
first/aggregated error instead of only logging, ensure failed deletions keep
their ids in self.to_delete (preserve the existing failed.insert and extend
behavior) and stop clearing self.evicted/self.segments when persist returns Err
so callers can retry; update callers (e.g. truncate_above) to handle the Result
and react (retry/abort) accordingly.
---
Nitpick comments:
In `@dash-spv/src/storage/block_headers.rs`:
- Around line 292-347: Add a unit test in the same module that stores a
non-empty range via PersistentBlockHeaderStorage::store_headers (e.g.
BlockHeader::dummy_batch(5..10)), then call
PersistentBlockHeaderStorage::truncate_above with start_height - 1 (e.g. 4) and
assert it returns StorageError::InvalidArgument; after the failed call assert
that get_tip_height and get_header_height_by_hash for an existing header (from
the stored batch) are unchanged to ensure the header_hash_index pruning logic
does not run on error. Use the same async #[tokio::test] pattern and reference
the methods truncate_above, get_tip_height, get_header_height_by_hash,
store_headers, and BlockHeader::dummy_batch so the test mirrors the existing
style and verifies the rejected truncation path.
In `@dash-spv/src/storage/filter_headers.rs`:
- Around line 130-183: Add unit tests in this module (filter_headers.rs) to
cover the two documented error contracts: assert that
PersistentFilterHeaderStorage::truncate_above(start_height - 1) returns
StorageError::InvalidArgument, and assert that load_filter_headers(range)
returns StorageError::InvalidArgument when the requested range crosses into a
segment that was truncated (queued for deletion) before calling persist; use the
existing TempDir setup and methods (PersistentFilterHeaderStorage::open,
store_filter_headers, truncate_above, load_filter_headers, persist) to create
the state that triggers each error and verify the exact
StorageError::InvalidArgument variant is returned.
In `@dash-spv/src/storage/filters.rs`:
- Around line 96-151: Add unit tests that cover the error paths of
PersistentFilterStorage::truncate_above and
PersistentFilterStorage::load_filters: add one test that calls truncate_above
with a start_height less than the current stored tip (e.g., truncate_above with
a value below 0 or otherwise invalid) and asserts it returns the expected Err
variant, and add another test that calls load_filters for a range that spans
into a segment that was truncated/queued for deletion and asserts load_filters
returns the documented error; locate tests near the existing tests (e.g., add
functions named test_truncate_above_error_below_start and
test_load_filters_error_after_truncate) and use the same TempDir +
PersistentFilterStorage::open, store_filter, truncate_above, persist/open
patterns to reproduce the queued-for-deletion state before calling load_filters.
In `@dash-spv/src/storage/mod.rs`:
- Around line 587-606: The explicit trait-qualified call <DiskStorageManager as
BlockHeaderStorage>::truncate_above(&mut mgr, 5).await.unwrap() is required and
should be kept as-is; ensure you do not change it to an unqualified method call
because DiskStorageManager implements multiple traits with truncate_above.
Optionally, to further harden the test, add a reload/persist cycle after
truncation that reopens DiskStorageManager and asserts the tip height and
header-index state still reflect the truncation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da3cce8f-fea2-4826-976c-2384101520cd
📒 Files selected for processing (8)
dash-spv/src/error.rsdash-spv/src/storage/block_headers.rsdash-spv/src/storage/blocks.rsdash-spv/src/storage/filter_headers.rsdash-spv/src/storage/filters.rsdash-spv/src/storage/mod.rsdash-spv/src/storage/segments.rsdash-spv/src/sync/masternodes/sync_manager.rs
After a within-segment `truncate_above`, the boundary segment is not in `to_delete`, so the loop guard in `get_items` does not catch overruns into its sentinel tail. In debug builds this panicked on the `last_valid_offset` assertion, in release builds it returned sentinel-filled data. Add an entry guard that fails fast when the requested end exceeds `next_height()`, plus a within-segment regression test alongside the existing cross-boundary one. Addresses CodeRabbit review comment on PR #790 #790 (comment)
Merge two noop edge cases in segments.rs into a single test. Fold the block_headers noop assertion into the index-prune test. Collapse the blocks, filter_headers, and filters wrappers to one smoke test each since the underlying SegmentCache::truncate_above engine is already exhaustively covered in segments.rs.
1 participant
Add
truncate_above(height)toBlockHeaderStorage,FilterHeaderStorage,FilterStorage, andBlockStorage, backed by a newSegmentCache::truncate_abovethat drops segments above the target and resets the boundary segment's tail slots to the persistable sentinel. Dropped segment files are queued for deletion on the nextpersist, andPersistentBlockHeaderStoragealso prunes itsheader_hash_indexso orphaned hashes no longer resolve to a height.This is the foundational primitive for upcoming fork/reorg handling in storage. No callers are wired up yet.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests