refactor(dash-spv): drop committed range sweep - #1016
Merged
Merged
Conversation
Contributor
|
Warning Review limit reachedNext included review available in 18 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: dashpay/rust-dashcore/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1016 +/- ##
==========================================
- Coverage 77.16% 77.09% -0.08%
==========================================
Files 329 329
Lines 83847 83577 -270
==========================================
- Hits 64699 64430 -269
+ Misses 19148 19147 -1
|
ZocoLini
force-pushed
the
refactor/drop-committed-range-sweep
branch
2 times, most recently
from
September 22, 2026 16:04
5156e07 to
53c8f98
Compare
ZocoLini
added a commit
that referenced
this pull request
Sep 22, 2026
With the committed-height release gone, the segment cache itself has to bound the resident set. Ten 50 000-item segments per cache kept most of a restore in memory; two keep the peak below what the release achieved, without costing time. Mainnet restore, mainnet.100mbi.100ms, #1015/#1016/#1014 applied, jemalloc heap profiling, wallet identical in every run (14114383 sat, 7112 records, 13389 addresses): segments peak RSS time segment loads from disk (headers/filter headers/filters/blocks) 10 + #946 982 MiB 8.2 min - 10 1490 MiB 8.8 min 84 / 2 / 28 / 0 2 930 MiB 8.8 min 212 / 103 / 74 / 0 1 1000 MiB 7.7 min 427 / 742 / 156 / 33 Each cache served ~2.34M requests from memory in every run. One segment starts reloading block segments of up to 87 MB from disk, which raises the peak again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017DruChNTWXwJoWPartZwCf
ZocoLini
added a commit
that referenced
this pull request
Sep 22, 2026
A dirty segment leaving the resident set used to wait in `evicted`, in memory, until the next 5 s storage tick wrote it. The resident limit was therefore not a limit: every segment evicted between two ticks stayed in memory. Now eviction writes the segment first and only then drops it; if the write fails the segment stays resident and the error is returned. The `evicted` map is gone, and the tick only persists the resident segments. Mainnet restore, mainnet.100mbi.100ms, #1015/#1016/#1014 applied, two resident segments: 7.7 and 7.9 min, peak RSS 943 and 948 MiB, wallet identical (14114383 sat, 7112 records, 13389 addresses). No cache misses served from `evicted` any more; header, filter header and filter segments are reloaded from disk about as often as they used to come back from `evicted` (~270 / ~150 / ~90), block segments never. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017DruChNTWXwJoWPartZwCf
ZocoLini
added a commit
that referenced
this pull request
Sep 22, 2026
… one buffer `Segment::persist` encoded the whole segment into a `Vec` before handing it to `atomic_write`. For a block segment near the tip (up to 87 MB on disk) that `Vec` grew to 128 MiB, and since dirty segments are now written on eviction this happened in the middle of the sync. `atomic_write_items` encodes one item at a time through a 1 MiB `BufWriter` into the temporary file, then syncs and renames it as before. `atomic_write` keeps its behaviour and shares the temporary file and rename logic. Mainnet restore, mainnet.100mbi.100ms, #1015/#1016/#1014 applied, two resident segments, wallet identical in every run (14114383 sat, 7112 records, 13389 addresses): whole-segment buffer peak RSS 943 / 948 MiB 7.7 / 7.9 min streamed peak RSS 845 / 855 / 856 MiB 9.4 / 7.7 / 6.5 min The persist buffer, 68–133 MiB in the earlier heap snapshots, no longer shows up. The 9.4 min run lost time to peers in the header and filter header phases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017DruChNTWXwJoWPartZwCf
ZocoLini
added a commit
that referenced
this pull request
Sep 22, 2026
Every segment cache held 50 000 items, whatever an item costs. Near the tip a block segment holds tens of MB of decoded blocks, while a header segment spanning the same heights is 5.6 MB and a filter header one 1.6 MB. `Persistable::ITEMS_PER_SEGMENT` lets each type choose: headers 10 000 (~1.1 MB per segment), filter headers 50 000 (~1.6 MB), filters 2 000 (~2 MB near the tip) and blocks 1 000. This changes the on-disk layout of the header, filter and block segments. Storage written with 50 000-item segments is misread by this layout and has to be deleted until a migration or a versioned folder lands. Mainnet restore, mainnet.100mbi.100ms, #1015/#1016/#1014 applied, two resident segments, jemalloc heap profiling, wallet identical in every run (14114383 sat, 7112 records, 13389 addresses): segment items time peak RSS segment caches 50 000 for every type 7.0 min 818 MiB 332 MiB 5 000 for every type 7.3-8.3 min 554-687 MiB 23-106 MiB 1 000 for every type 9.2-12.2 min 453-577 MiB 2-66 MiB per type (this commit) 7.5 min 538 MiB 38 MiB With 1 000 items everywhere, the header and filter header phases paid an fsync per evicted segment (105-141 s and 254-332 s instead of ~60 s and ~150 s). Blocks at 500 items saved ~18 MiB of cache but reloaded 50 % more block segments. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017DruChNTWXwJoWPartZwCf
Contributor
github-actions
Bot
added
the
merge-conflict
Contributor
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
ZocoLini
force-pushed
the
refactor/drop-committed-range-sweep
branch
from
September 23, 2026 18:04
53c8f98 to
1b6d1c8
Compare
github-actions
Bot
removed
the
merge-conflict
ZocoLini
changed the title
…scripts rescan The #846 sweep re-tested every late-derived script against the whole stored filter history, from the wallet's birth height. On a mainnet restore of the bench wallet that was one 63 s walk over 200 000..tip matching 41 687 blocks, overwhelmingly compact-filter false positives, then about 17.7 min downloading and applying them, out of a 21–27 min sync. The notification-driven path (`collect_new_scripts`, `FiltersBatch::collected_scripts`, the current-and-later batch rescan at commit) duplicated `reconcile_untested_scripts`, which asks the wallet what it watches, a superset of any `new_scripts`, and re-tests whatever the batch never matched. `rescan_complete` was set immediately before the batch left `active_batches`, so it was never read as true. Trade-off: the #846 case, a CoinJoin output paying an index derived only after its batch committed, is no longer recovered. Its repro, `coinjoin_gap_limit_stall_across_committed_batch`, fails without the sweep and is removed with it, as is the sweep-cost test. The bench wallet does not hit that case. Validated on top of the spend re-application fix: 5 consecutive mainnet restores at 100 Mbit / 100 ms, then 100/50 Mbit x 100/500 ms, all ending at 7112 records, 14 114 383 sat and 13 389 addresses, in 6.0–17.2 min. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017DruChNTWXwJoWPartZwCf
ZocoLini
force-pushed
the
refactor/drop-committed-range-sweep
branch
2 times, most recently
from
September 23, 2026 18:22
1b6d1c8 to
1674cd2
Compare
ZocoLini
marked this pull request as ready for review
September 23, 2026 18:23
github-actions
Bot
added
the
waiting-bots
Collaborator
Author
|
@coderabbitai approve |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
✅ Action performedComments resolved and changes approved. |
Contributor
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
13 tasks
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.
Drops de final sweep, syncs goes from 21–27 min → 6–9 min, gives up the #846 case. This can be done now, because all (known) bugs that were losing funds are solved and it doesn't discover anything new
PR Hygiene ·
1674cd2/self-revieweddash-spv(dash-spv/src/sync/filters/batch.rs,dash-spv/src/sync/filters/block_match_tracker.rs,dash-spv/src/sync/filters/coinjoin_gap_discovery_tests.rsand 2 more) — QuantumExplorer or xdustinfaceWhen every box is checked the
PR Hygienecheck passes and this can merge.