fix(dash-spv): release clean storage segments below the committed height during long scans - #932
Open
bfoss765 wants to merge 1 commit into
Open
fix(dash-spv): release clean storage segments below the committed height during long scans#932bfoss765 wants to merge 1 commit into
bfoss765 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>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughChangesCommitted storage watermark
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BlocksManager
participant FiltersManager
participant DiskStorageManager
participant PersistentBlockStorage
participant PersistentFilterStorage
participant SegmentCache
BlocksManager->>DiskStorageManager: update committed block height
DiskStorageManager->>PersistentBlockStorage: forward block watermark
PersistentBlockStorage->>SegmentCache: set committed height
FiltersManager->>DiskStorageManager: rollback or advance filter watermark
DiskStorageManager->>PersistentFilterStorage: forward filter watermark
PersistentFilterStorage->>SegmentCache: set committed height
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #932 +/- ##
==========================================
+ Coverage 75.52% 75.57% +0.04%
==========================================
Files 328 328
Lines 78017 78212 +195
==========================================
+ Hits 58923 59108 +185
- Misses 19094 19104 +10
|
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
1 participant
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.
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