fix(dash-spv): release clean storage segments below the committed height during long scans - #946
fix(dash-spv): release clean storage segments below the committed height during long scans#946bfoss765 wants to merge 1 commit into
Conversation
…ght during long scans A long backfill pinned every item it ever downloaded. `SegmentCache` holds 50_000 items per segment and only evicts once more than `MAX_ACTIVE_SEGMENTS` (10) are resident, so a 350k-block scan — which spans just 7-8 segments — never tripped the LRU. Every compact filter and every full decoded block (GCS false positives, ~12k blocks at a 27k-script watch set) stayed in RAM until the process died. `persist` wrote segments to disk and marked them Clean but never freed them. Add a caller-declared committed-height watermark. After `persist`, any segment lying entirely at or below it is dropped from memory, leaving the on-disk file as the source of truth; the next read reloads it through the lazy path already used for any non-resident segment. This is safe because `Clean` is reachable only via `Segment::load` from an existing file or a successful `Segment::persist`, so a clean segment is byte-identical to its backing file and releasing it is invisible to readers. Dirty segments (contents exist only in memory), the frontier segment (still being written), and partially committed segments are always kept. The watermark is wired where each cache's owner already knows the answer: - blocks, from the in-order `take_next_ordered_block` drain, which applies blocks to every interested wallet before advancing; - filters, from batch commit, which happens only once every matched block in the batch has been downloaded and applied. It may move backwards — a wallet rescan rolls it back before re-reading lower heights — which only narrows the release window. `truncate_above` clamps it and `clear` drops it so it never outlives its data. Header caches are deliberately left alone: their random-access path is the separate `header_hash_index` map, which segment release cannot shrink, and their ranges are read at arbitrary depths during scanning. The mechanism is generic, so they can opt in later with one call. New trait methods carry default no-op bodies, so out-of-tree implementors are unaffected and the change is drop-in for the platform AAR build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 7 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 Plus Run ID: 📒 Files selected for processing (6)
Comment |
1 participant
Supersedes #932 — same change, recreated on a
dashpay/rust-dashcorebranch per repo policy (no more personal-fork PRs). Commits and authorship unchanged; full review history on #932.The problem
A long backfill pins every item it ever downloads.
SegmentCacheholds 50,000 items per segment and only evicts once more thanMAX_ACTIVE_SEGMENTS(10) segments are resident. A 350k-block scan spans just7-8 segments, so it never trips the LRU. Every compact filter and every full
decoded block (GCS false positives — roughly 12k blocks at a 27k-script watch
set) stays in RAM until the process dies.
persistwas already writing segments to disk and marking themClean, but itnever freed them. The data was durable and redundant in memory at the same time.
This is an out-of-memory class bug, not a speculative improvement.
Field evidence
A tester's mainnet wallet performs a forced rescan of roughly 345,000 filters
— a direct match for the case described above.
low-memory killer at 1.4–2.1 GB RSS, at least three times in a single day,
with some sessions lasting as little as one minute.
and fell back to ~1.0 GB once the scan completed — segments being released
— after which the process ran 5h57m uninterrupted with zero low-memory kills.
Confound, stated plainly: that build bundled several other fixes, so the
survival time is not attributable to this change in isolation. What is
attributable is the memory shape — a peak during the scan followed by a
release afterwards — which is precisely this fix's mechanism. There is no
dedicated eviction instrumentation in those logs, so the shape is the evidence,
not a counter.
The fix
Add a caller-declared committed-height watermark. After
persist, anysegment lying entirely at or below the watermark is dropped from memory, leaving
the on-disk file as the source of truth. The next read reloads it through the
lazy path already used for any non-resident segment.
This is safe because
Cleanis reachable only viaSegment::loadfrom anexisting file, or via a successful
Segment::persist. A clean segment istherefore byte-identical to its backing file, and releasing it is invisible to
readers.
Always kept:
The watermark may move backwards — a wallet rescan rolls it back before
re-reading lower heights — which only narrows the release window.
truncate_aboveclamps it and
cleardrops it, so it never outlives its data.Where the watermark comes from
It is wired where each cache's owner already knows the answer:
take_next_ordered_blockdrain, which appliesblocks to every interested wallet before advancing;
the batch has been downloaded and applied.
Deliberately out of scope
Header caches are left alone. Their random-access path is the separate
header_hash_indexmap, which segment release cannot shrink, and their rangesare read at arbitrary depths during scanning. The mechanism is generic, so they
can opt in later with a single call.
Compatibility
The new trait methods carry default no-op bodies, so out-of-tree implementors are
unaffected and the change is drop-in for the platform AAR build.
Tests
Six new tests in
dash-spv/src/storage/segments.rscover the release path andits guards:
test_committed_blocks_are_released_but_still_loadabletest_release_committed_segments_reloads_from_disktest_release_skips_dirty_frontier_and_partial_segmentstest_released_segments_serve_a_rescan_rereadtest_committed_height_follows_truncate_and_cleartest_store_into_released_segment_preserves_existing_itemscargo test -p dash-spvis green (587 passed, 0 failed, withSKIP_DASHD_TESTS=1for the targets that require a
dashdbinary).cargo fmtand strict clippy(
--all-features --all-targets -D warnings) are clean.Summary by CodeRabbit
New Features
Bug Fixes