fix(key-wallet): walk a sweep's descendant closure once instead of per generation - #969
Conversation
…r generation `drop_conflicted_transactions` rescanned every retained record once per generation of the unconfirmed descendant closure: each pass over the history collects only the next reachable generation, extends the loser set, and repeats until nothing is added. A depth-D chain therefore costs about D full scans of H retained records — O(D×H), quadratic when the chain dominates the history. Neither factor is bounded by anything the wallet controls: a peer can deliver a deep chain of wallet-relevant unconfirmed transactions and then a valid finalized replacement for the root's input, and the sweep runs while the account is held mutably and before persistence records anything. Build a parent-to-children index in one pass and follow a queue, so each record is visited once: O(records + edges). The carve-outs are unchanged — confirmed records and InstantSend-locked records are never followed, the winner and already-collected losers are never re-added — so exactly the same records are swept as before. Covered by a closure pin exercising the carve-outs and a diamond join, and by a 2000-deep chain asserting a linear visit bound through test-only instrumentation: the per-generation rescan counts five million visits there against a bound of seventy-five hundred.
|
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 (1)
Included review availability: Your plan provides up to 3 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughConflict sweeping now indexes parent-child relationships and traverses descendants once with a breadth-first queue. Tests cover conflict closure, carve-outs, diamond graphs, unrelated transactions, and linear visit complexity. ChangesConflict sweeping
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This PR changes only the traversal strategy while preserving swept records and carve-outs, with targeted tests and standard checks passing. The current title already uses a Conventional Commit prefix, so no actionable merge-blocking risk remains. Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #969 +/- ##
==========================================
+ Coverage 76.92% 76.97% +0.04%
==========================================
Files 329 329
Lines 82527 82676 +149
==========================================
+ Hits 63482 63637 +155
+ Misses 19045 19039 -6
|
There was a problem hiding this comment.
Two things I'd note, neither blocking:
-
The common path now pays for an index it never uses. Building the index lazily on the first non-empty generation would avoid it
-
The index is O(total inputs) rather than O(records): it keys on every
previous_output.txid, including txids the wallet does not hold (external
funding). Still linear, just worth knowing the memory tracks input count, not
record count
Neither changes the verdict, so go ahead with the squash merge. Worth a look if there's time
| #[inline] | ||
| fn note_descendant_walk_visit() { | ||
| #[cfg(test)] | ||
| descendant_walk_instrumentation::VISITS.with(|visits| visits.set(visits.get() + 1)); |
There was a problem hiding this comment.
I would rather prefer to see this inlined instead of hidden behind a function but its okay
dashpay/rust-dashcore#969 merged as 5877d15f, so the pin moves off the #966 merge commit onto it. The revision replaces the conflict sweep's per-generation rescan of the whole retained history with a parent-to-children index built once and a queue traversal that visits each record exactly once — O(records + edges) instead of O(depth × history), which a peer could drive with a deep chain of unconfirmed wallet-relevant transactions followed by a finalized replacement for the root input. Skip semantics are unchanged: confirmed and InstantSend-locked records are still never followed, the winner is never a candidate, and an IS-locked initial loser still has its descendants walked. All eight workspace pins and Cargo.lock move together; no API changed.
3 participants
Issue being fixed or feature implemented
drop_conflicted_transactionswalks the unconfirmed descendant closure of a sweep's losers with a fixed-point loop: every iteration rescans every retained record and collects only the next reachable generation, extends the loser set, and repeats until nothing is added. A depth-D chain therefore costs about D full scans of H retained records —O(D×H), quadratic when the chain dominates the history.Neither factor is bounded by anything the wallet controls. A peer can deliver a deep chain of wallet-relevant unconfirmed transactions (each spending the previous one's change) and then a valid finalized replacement for the root's input. The whole cost lands while the account is held mutably, before persistence records the sweep.
Reported during review of dashpay/platform#4406, which pins this crate. Same class as #966, different traversal.
What was done?
Build a parent→children index over the retained records in one pass, then follow a queue from the initial losers, visiting each record once:
O(records + edges).The carve-outs are unchanged and deliberately so — they are load-bearing:
Exactly the same records are swept as before. No behaviour change, only the shape of the walk.
How Has This Been Tested?
the_walk_sweeps_the_closure_and_respects_the_carve_outs: a rooted six-deep chain with a diamond join, a confirmed branch, an IS-locked branch (each with an unconfirmed child of its own), and an unrelated record — pins that the chain and the diamond are swept exactly once and every carve-out survivor is retained. This test also passes against the old loop, which is what pins behavioural equivalence.a_deep_chain_is_swept_with_linearly_many_visits: a 2000-deep chain against 2500 retained records, asserting a deterministic linear visit bound counted by test-only instrumentation (a#[cfg(test)]thread-local counter; the production call compiles to an empty inline function). This is not a timing assertion. Revert-tested: with the old per-generation loop restored, this test fails with 5,000,000 visits against a bound of 7,500 — exactly theD×Hshape.cargo test -p key-wallet— 663 passed, 0 failed.cargo clippy --all-features --all-targets -- -D warningsandcargo fmt --checkclean.Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit