fix(dash-spv): don't preload filters below the actually stored range on filter-download start#893
Conversation
…on filter-download start FiltersManager::start_download treated stored_filters_tip — a tip watermark — as proof that filter storage is contiguous from scan_start. When only tip-region filters were ever stored (e.g. headers previously synced from a checkpoint above the wallet's birth height), the preload called load_filters over never-populated segments: a debug-assertion abort in SegmentCache::get_items (crash-loop on every launch, since the restart resumes the same scan state) and silent sentinel filter data zipped against real headers in release builds. - FilterStorage gains filter_start_height() (lowest stored height, backed by the SegmentCache start it already tracks) and clear_filters() (drop all filters; files removed on next persist). - start_download only treats the stored range as usable when it reaches down to scan_start. Otherwise the unreachable tip-region filters are discarded and the download restarts from scan_start exactly as if no filters were stored — resuming from their tip would leave a permanent hole below them that the in-order store loop can never fill. Preload and the initial batch's mark_verified are gated on the same check, and progress.stored_height is reset so batch scan gating no longer trusts the stale watermark. - SegmentCache::get_items now returns a typed InvalidArgument error for ranges that begin below the lowest stored height, replacing the debug-only panic / release sentinel reads for that case. Fixes #892 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughFilter storage now reports its lowest stored height and supports clearing. Segment reads reject ranges below that height. Filter synchronization clears sparse, incompatible stored ranges and restarts downloads from ChangesFilter restart safety
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FiltersManager
participant DiskStorageManager
participant PersistentFilterStorage
participant SegmentCache
participant FilterPeer
FiltersManager->>DiskStorageManager: read stored tip and start
DiskStorageManager->>PersistentFilterStorage: delegate storage query
PersistentFilterStorage->>SegmentCache: return stored range
FiltersManager->>DiskStorageManager: clear filters when scan_start is uncovered
DiskStorageManager->>PersistentFilterStorage: delegate clear
FiltersManager->>FilterPeer: request filters from scan_start
FilterPeer-->>FiltersManager: return filter batch
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 137-149: Update parse_segment_id to validate the complete filename
rather than parsing a fixed four-byte slice: strip the exact segment prefix,
separator, and data-file extension, then parse the entire remaining ID component
as u32. Reject names with extra characters, missing separators, or malformed IDs
while preserving None for invalid filenames.
- Around line 458-480: Update Storage::clear to return StorageResult<()>,
propagate both read_dir failures and individual directory-entry errors, and
complete the entire scan before clearing or queuing any cache state. Then update
clear_filters and its callers to propagate the clear result while preserving the
existing reset behavior on success.
In `@dash-spv/src/sync/filters/manager.rs`:
- Around line 218-219: Update the stored-range check near stored_covers_scan to
use filter_start_height presence, rather than stored_filters_tip > 0, so a
stored range of 0..=0 is treated as non-empty and preloads height zero. Add a
restart unit test covering storage containing only filter height zero.
🪄 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: 12e47b48-9f16-4cd1-b92d-4a56ad7eab77
📒 Files selected for processing (4)
dash-spv/src/storage/filters.rsdash-spv/src/storage/mod.rsdash-spv/src/storage/segments.rsdash-spv/src/sync/filters/manager.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #893 +/- ##
==========================================
+ Coverage 74.39% 74.44% +0.05%
==========================================
Files 327 327
Lines 74779 75032 +253
==========================================
+ Hits 55629 55855 +226
- Misses 19150 19177 +27
|
Three follow-ups to the #892 fix: - parse_segment_id: parse the entire component between the `{prefix}_` and `.{ext}` fixtures via strip_prefix/strip_suffix instead of a fixed 4-byte slice. Trailing junk (`segment_0000junk.dat`) is now rejected and ids wider than the zero-padding (`segment_100000.dat`) are accepted rather than silently truncated. Adds a dedicated parse_segment_id test covering round-trips, junk tails, 5+ digit ids, and malformed names. - SegmentCache::clear now returns StorageResult<()>: the directory scan runs to completion before any cache state is mutated, io NotFound on read_dir is treated as empty-ok, and every other error is propagated. A failed scan no longer leaves the cache reporting empty while persisted segment files survive to resurrect the sparse-storage bug after restart. The Result is threaded through clear_filters and its callers. - start_download gates stored_covers_scan on the stored start height alone (`stored_filters_start.is_some_and(|s| s <= scan_start)`). filter_tip_height collapses "empty" and "only height 0 stored" to 0, so the previous `stored_filters_tip > 0` guard misread a genesis-only store as empty and re-downloaded height 0 instead of preloading it. The discard branch is made consistent via `is_some()`. Adds a genesis-only restart test asserting the lone filter is preloaded with no clear and no re-request. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Follow-up with the full root-cause timeline from the device where this reproduced (per-session How the sparse island formsThe state is manufactured by a mid-life client re-initialization from a checkpoint, not by plain sync + restart: So the mysterious Repro recipe: sync with filters stored from a low height → restart the client with a start-height request that initializes from a checkpoint below the wallet's current synced height → the filter re-download resumes from the wallet position, leaving the gap → restart once more → pre-fix crash (debug) / sentinel filter reads (release). What the discard does in practice (convergence)With this PR on the same corrupted device state: One discard total. After it, storage is dense from the scan floor, every later restart resumes normally, and the wallet completed the backfill scan to tip. It is a one-time self-heal that restores the dense-storage invariant, not a recurring reset. The remaining latent bug (separate issue)The island-manufacturer itself — the checkpoint re-init resetting filter storage but resuming the re-download from the wallet's synced height instead of the new floor — is a distinct bug; filing it separately. This PR makes the client self-heal from any such island regardless of origin (that session's mid-scan kill shapes can produce the same state), which is worth having even once the manufacturer is fixed. 🤖 Generated with Claude Code |
|
Good challenge on the timeline — the T1/T2 distinction is correct, my earlier comment conflated the checkpoint floor with the scan floor. Here are the exact lines that were asked for, plus the T2 evidence. All from the same device, same logs. T1 session (island forms), post-re-init and session end: Session ends cleanly at committed 2,504,884 — consistent state, exactly as predicted: T1 alone gives T3 (first crashing session, 70 seconds later): So T2 is real and sits between those two lines: the wallet's committed height (2,504,884) did not survive the restart. With committed effectively 0 and birth 0, T2 is conditional, and the condition looks like abrupt termination. Post-fix restarts on the same device: Every floor-collapse restart in the log history followed an abrupt kill (the pre-fix crash-loop itself, or forced terminations); the one restart that followed a healthy long-running session resumed at committed+1. That points at the sync-height persistence cadence in the embedding app/platform-wallet layer (not dash-spv) — likely only stamped on some pulse or graceful path that abrupt kills miss. Nasty interaction: the pre-fix crash-loop guaranteed abrupt terminations, so T2 re-armed itself on every relaunch. Refinement of #894: the island needs BOTH halves — T1 (checkpoint re-init resets filter storage, refill resumes from the wallet position instead of the floor) AND T2 (committed height regressing across an abrupt restart). I'll note this on #894; the T2 half belongs to the embedding-app layer and I'll chase it there. On discard cost — conceded. This island spanned ~990k filters; the discard re-fetched ~2.3M, so ~40% of prior work was thrown away rather than kept. "Cheap" was wrong for this shape. The claims that stand: it's one-time per island (convergence shown above), it's correctness-first (the release-mode alternative is silently wrong filter matches over the gap), and a backfill-below-the-island variant would be a legitimate future optimization on top of the same detection. 🤖 Generated with Claude Code |
|
Bullet-point reproduction recipes, as requested. Two levels: the minimal one reproduces the crash in the CLI today; the second is the exact path our device took. Minimal repro (CLI, no app needed — pre-#893 debug build)The crash needs only one condition: stored filter range starting above the wallet scan floor. Any way of creating it works:
So "importing a wallet at a lower height" — the scenario suspected early in this thread — is a real trigger of the same crash, independent of anything our app did. That's why I'd argue against merging-then-reverting: the guard protects a legitimate user flow that's about to ship, and the release-mode half (silent wrong matches) exists for every variant of the state. The path our device actually took (T1 + T2, both filed)
On "hiding the underlying issue"The discard path logs 🤖 Generated with Claude Code |
2 participants
Issue being fixed or feature implemented
Fixes #892.
FiltersManager::start_downloadtreatedstored_filters_tip(a tip watermark) as proof that filter storage is contiguous fromscan_start. When only tip-region filters exist and the wallet's scan floor is far below (birth height « tip), the preload read never-populated segments:SegmentCache::get_itemsdebug-aborts (permanent app crash-loop on relaunch, since auto-start resumes the same scan state) and release builds silently return sentinel filter data zipped against real headers.What was done?
FilterStoragegainsfilter_start_height()(the segment cache already tracked it, it was just never exposed) andclear_filters().start_downloadcomputesstored_covers_scan = stored_filters_tip > 0 && stored_start <= scan_start. When stored filters exist but don't reach down toscan_start, they are discarded (warn-logged) and the download restarts fromscan_start— merely skipping the preload isn't enough: resuming from the stored tip would leave a permanent hole below the island that the in-order store loop can never fill, and keeping the island through a backfill risks two disjoint islands plusSegment::insertnon-sentinel asserts. Preload and initial-batchmark_verifiedgate onstored_covers_scan.SegmentCache::get_itemsnow returns a typedInvalidArgumentfor ranges beginning below the lowest stored height (replacing debug-abort / release-sentinel behavior for that case);SegmentCache::clear()added;parse_segment_idhelper also fixes a latent slice-panic on short filenames inload_or_new.How Has This Been Tested?
test_start_download_discards_stored_filters_above_scan_start— the exact dash-spv: filter restart with scan_start far below stored tip loads never-populated segments — debug abort / release sentinel reads #892 shape (filters stored 900..=1000, scan_start 100); verified it fails against the old gate at exactly the bad preload.test_start_download_preloads_when_stored_filters_cover_scan_start— regression guard for the covers case (behavior unchanged).filter_start_height+clear_filtersround-trip incl. persist/reopen;get_itemsbelow-start typed error; segment-cache clear (persisted + unpersisted).cargo test -p dash-spv: 477 passed, 0 failed (thedashd_masternodebinary requiresDASHD_PATH, pre-existing environmental). New tests also pass in--release, where the debug assertions are compiled out. fmt/clippy/--all-features --tests/dash-spv-ffi --all-targetsclean.Breaking Changes
None.
FilterStorageimplementors gain two methods with default-free trait additions (in-repo implementors updated).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes