chore: bump rust-dashcore to the merged sync fixes, minus the sweep-event chain - #4459
chore: bump rust-dashcore to the merged sync fixes, minus the sweep-event chain#4459romchornyi wants to merge 1 commit into
Conversation
…vent chain Six merged dash-spv fixes for stalled and slow sync have been sitting in `dev` unreachable from the app, because the pin does not follow `dev` — it is moved onto side branches cut for one fix at a time. The last bump to a `dev` revision was #4394 on 08-13; #4455 on 08-22 moved it to `a062ccb9`, a cherry-pick of the mnemonic fix on top of an older base. What that leaves out, and what this brings in: | | | |---|---| | #964 | extend the CFHeaders queue from the tick — the queue was never re-extended when block-header storage advanced without an event, and filter sync stopped for good while headers and ChainLocks carried on | | #960 | promote finished header segments from the tick, not only on a message | | #955 | resume on the invariant `start_download` asserts | | #947 | recover masternode sync from a rejected QRInfo | | #946 | release clean storage segments below the committed height during long scans | | #866 | rescan committed filter ranges for newly derived scripts | Plus #945, #928, #963, #965, #967, #970, and #980 — the multilingual BIP-39 fix the current pin carries, so nothing regresses there. **Why a curated branch rather than `dev` HEAD.** `dev` also contains #961, which adds `WalletEvent::TransactionsSwept`, and #962/#966/#969 on top of it. That variant makes four `match` sites in `rs-platform-wallet` non-exhaustive, so pinning `dev` does not compile here — handling it is the substance of #4406 and is not something this bump should duplicate. The branch is `dev` with that chain left out: base `55afccfc` (#945) plus nine cherry-picks, every one applying without conflict. The exclusion has a cost worth stating: #961 stops a never-broadcast transaction from crediting money that does not exist, so a wallet can still read a balance that is too high in that case, and #966/#969 remove quadratic work from the sweep walk. Those arrive with #4406. This trades that for six stall fixes that are blocking users today. Ref: dashpay/rust-dashcore branch `chore/sync-fixes-without-swept`.
|
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 ignored due to path filters (1)
📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe Cargo workspace updates all eight ChangesRust-Dashcore Revision Update
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This change only updates dependency revisions, with workspace checking and targeted SPV tests reported clean. No actionable merge-blocking risk remains; normal validation should complete before merge. 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 |
|
⛔ Blockers found — Opus deferred (commit 9924c10) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 9924c10, the dependency pin correctly updates all eight rust-dashcore crates, but it also introduces #866's unbounded whole-history match accumulation. Because the accepted compact-filter chain is peer-controlled and the resulting matches are queued together, an eclipsing peer can turn a full-history wallet rescan into a chain-length memory allocation followed by millions of block requests; this must be bounded before the revision is shipped.
Source: reviewer backend gpt-5.6-sol (general and security-auditor lanes); final verifier backend gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `Cargo.toml`:
- [BLOCKING] Cargo.toml:58: Historical filter rescan permits peer-driven memory and bandwidth exhaustion
The newly pinned revision includes rust-dashcore commit 2dc97c57 (#866), which was absent from the previous pin. Its `rescan_committed_range` loads history in 5,000-filter chunks but inserts every match from the wallet birth height through `batch_start - 1` into one `BTreeMap`; it does not drain matches until the entire range has been scanned. `queue_new_script_matches` then duplicates that chain-length state into the tracker and a `BlocksNeeded` event, and `BlocksManager` queues every missing block for download. This is reachable for Platform recovery wallets because `birth_height_override = Some(0)` explicitly enables scans from genesis. The filter-header manager accepts peer-provided CFHeaders, computes their self-consistent chain, and stores it without an independently authenticated checkpoint, while filters are only checked against that same peer-provided chain. An eclipsing compact-filter peer can therefore construct historical filters that avoid the current watch set but match a receive script that a later valid payment causes gap-limit maintenance to derive. At the current multi-million-block chain length, the backward sweep can exhaust mobile memory before emitting its event; if it survives, it schedules millions of full-block reads or downloads. Preserve the historical rescan cursor and process a bounded number of matches per iteration, with an explicit cap or recovery policy for implausibly high match rates, or repin without #866 until that upstream fix is available.
| dashcore-rpc = { git = "https://github.com/dashpay/rust-dashcore", rev = "a97b32c617c8b1fef5185bb806500b66faf8e8c4" } | ||
| dashcore = { git = "https://github.com/dashpay/rust-dashcore", rev = "1322ef7252fe00cbe941a574c78fe94d755c27cd" } | ||
| dash-network-seeds = { git = "https://github.com/dashpay/rust-dashcore", rev = "1322ef7252fe00cbe941a574c78fe94d755c27cd" } | ||
| dash-spv = { git = "https://github.com/dashpay/rust-dashcore", rev = "1322ef7252fe00cbe941a574c78fe94d755c27cd" } |
There was a problem hiding this comment.
🔴 Blocking: Historical filter rescan permits peer-driven memory and bandwidth exhaustion
The newly pinned revision includes rust-dashcore commit 2dc97c57 (#866), which was absent from the previous pin. Its rescan_committed_range loads history in 5,000-filter chunks but inserts every match from the wallet birth height through batch_start - 1 into one BTreeMap; it does not drain matches until the entire range has been scanned. queue_new_script_matches then duplicates that chain-length state into the tracker and a BlocksNeeded event, and BlocksManager queues every missing block for download. This is reachable for Platform recovery wallets because birth_height_override = Some(0) explicitly enables scans from genesis. The filter-header manager accepts peer-provided CFHeaders, computes their self-consistent chain, and stores it without an independently authenticated checkpoint, while filters are only checked against that same peer-provided chain. An eclipsing compact-filter peer can therefore construct historical filters that avoid the current watch set but match a receive script that a later valid payment causes gap-limit maintenance to derive. At the current multi-million-block chain length, the backward sweep can exhaust mobile memory before emitting its event; if it survives, it schedules millions of full-block reads or downloads. Preserve the historical rescan cursor and process a bounded number of matches per iteration, with an explicit cap or recovery policy for implausibly high match rates, or repin without #866 until that upstream fix is available.
source: ['codex']
3 participants
Issue being fixed or feature implemented
Six merged
dash-spvfixes for stalled and slow sync are unreachable from the app. The pin does not followdev— it is moved onto side branches cut for one fix at a time. The last bump to adevrevision was #4394 on 08-13; #4455 on 08-22 moved it toa062ccb9, a cherry-pick of the mnemonic fix on top of an older base, so everything merged intodevsince 08-13 is still not shipping.Reported symptom this addresses: sync freezes partway — one user is stuck at 71.3% on a build carrying the current pin — while headers and ChainLocks keep arriving, so the client looks alive.
What this brings in:
start_downloadassertsPlus #945, #928, #963, #965, #967, #970 and #980 — the multilingual BIP-39 fix the current pin already carries, so nothing regresses there.
What was done?
Cargo.tomlrepins all eight rust-dashcore crates,Cargo.lockregenerated. No other file changes.Why a curated branch rather than
devHEAD.devalso contains #961, which introducesWalletEvent::TransactionsSwept, plus #962/#966/#969 on top of it. That variant makes fourmatchsites inrs-platform-walletnon-exhaustive —changeset/core_bridge.rs:607,wallet/core/balance_handler.rs:44,wallet/identity/network/payment_handler.rs:249and:270— so pinningdevdoes not compile here. Handling that event is the substance of #4406 and this bump should not duplicate it.The pinned branch is
devwith that chain left out: base55afccfc(#945) plus nine cherry-picks, every one applying without conflict. Branch:dashpay/rust-dashcore@chore/sync-fixes-without-swept.The exclusion has a cost, stated plainly. #961 stops a never-broadcast transaction from crediting money that does not exist, so a wallet can still read a balance that is too high in that case; #966 and #969 remove quadratic work from the sweep walk. Those arrive with #4406. This trades them for six stall fixes that are blocking users now, and #4406 supersedes this pin when it lands.
How Has This Been Tested?
cargo check --workspaceagainst the new pin — clean. This was the point of the exclusion, and it was verified by first tryingdevHEAD and watching it fail on the four non-exhaustive matches above.cargo test -p dash-spv --libon the pinned branch — 563 passed, 0 failed, 2 ignored.a062ccb9= chore: v0.24-dev backports part 2 #980,a97b32c6= fix(dashmate): empty masternode status while syncing #970) have equivalents in the pinned branch.Not yet exercised end-to-end against the reported 71.3% freeze — that needs the reporter's
dash_spvlog or a rebuild on their device. #964 is the closest match to the symptom, but #947, #960 and #946 are also plausible from the description alone, and I would not claim which without the log.Breaking Changes
None. Dependency revision only.
Checklist
For repository code-owners and collaborators only
Summary by CodeRabbit