feat(key-wallet-manager): carry the sweep winner's mined height on TransactionsSwept - #975
Open
romchornyi wants to merge 2 commits into
Open
feat(key-wallet-manager): carry the sweep winner's mined height on TransactionsSwept#975romchornyi wants to merge 2 commits into
romchornyi wants to merge 2 commits into
Conversation
…ansactionsSwept WalletEvent::TransactionsSwept names the winner (superseded_by) but not its finality context, and the winner need not be wallet-relevant, so a consumer durably mirroring the swept spends cannot look its height up anywhere: it cannot tell a sweep triggered by a mined block from one triggered by an InstantSend-locked transaction still waiting to be mined, and so has no sound horizon for retiring those records. Add winner_mined_height: Option<CoreBlockHeight> to the event. The block emission site in process_block.rs passes Some(height) — the winner is a transaction of that very block — and the mempool site passes None, which is exhaustive there: only an IS-locked arrival sweeps off-chain, and an IS-locked winner is by definition unmined. The C ABI is deliberately unchanged: OnTransactionsSweptCallback has no safely-degrading insertion point for hand-declared C consumers (#962), so the FFI dispatch acknowledges the field without forwarding it and the generated header is byte-for-byte identical.
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 (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesSwept transaction height
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR adds the sweep winner’s mined-height context without changing sweep behavior, and the supplied checks are green; no actionable merge-blocking risk remains beyond normal review. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant process_block
participant WalletEvent_TransactionsSwept
participant dash_spv_ffi_callbacks
alt Block sweep
process_block->>WalletEvent_TransactionsSwept: set winner_mined_height to block height
else Mempool sweep
process_block->>WalletEvent_TransactionsSwept: set winner_mined_height to None
end
WalletEvent_TransactionsSwept->>dash_spv_ffi_callbacks: dispatch existing callback arguments
Note over dash_spv_ffi_callbacks: ignore winner_mined_height to preserve the C ABI
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![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 #975 +/- ##
==========================================
- Coverage 76.96% 76.83% -0.13%
==========================================
Files 329 329
Lines 82676 82698 +22
==========================================
- Hits 63631 63544 -87
- Misses 19045 19154 +109
|
…ent's Display The `Display` arm was the one line the new field reached that no test exercised. It is not decoration: when a swept coin misbehaves on a consumer, whether the winner was mined is the first thing the log has to answer, and a formatter that silently dropped the field would send a reader looking in the wrong place. Both legs are asserted in one test because either alone is satisfiable by a formatter that prints a constant.
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.
Summary
WalletEvent::TransactionsSweptnames the winner (superseded_by) but carries nothing about its finality context, and the winner need not be wallet-relevant — it can spend our coin while paying only external addresses, so it may never appear anywhere else in the wallet's event stream (the same asymmetry that motivatedreleased_outpointsin #962). A consumer durably mirroring the swept spends therefore cannot tell a sweep triggered by a transaction mined in a block from one triggered by an InstantSend-locked transaction still waiting to be mined, and cannot recover the distinction from its own records.That distinction is load-bearing downstream. dashpay/platform#4406 keeps durable observed-spent rows for the coins a sweep consumed, so a restored wallet does not re-credit them. Retiring those rows needs a finality horizon: a block-context sweep anchors the winner at a height that chainlocks, while an IS-locked winner has no mining deadline — aging its rows out by wall clock would delete a genuine hold while the conflict is still unmined.
key-walletalready applies exactly this doctrine internally (wallet_checker.rs: mempool/IS-lock spends are deliberately never recorded inobserved_spent_outpoints;prune_finalized_observed_spendsevicts onspend_height <= min(chainlock_height, synced_height)). The emission sites already know the winner's context — the event just did not forward it.winner_mined_height: Option<CoreBlockHeight>toWalletEvent::TransactionsSwept.key-wallet-manager/src/process_block.rs(process_block_for_wallets) passesSome(height)— the winner is a transaction of that very block, so the block's height is its mined height. This includes the late-blockInChainLockedBlockpath, where the height is the same.process_mempool_transaction) passesNone, which is exhaustive there: sweeps fire only forcontext.confirmed() || context.is_instant_send()(wallet_checker.rs), so the only off-chain trigger is an IS-locked winner, which is by definition unmined. These are the only two construction sites of the event.Displayfor the event now includeswinner_mined_height.Not the same thing as #968
#968 asks the sweep to attest wallet ownership of the loser's held inputs, which the sweep site cannot decide today (no ownership information for unclassified-ours coins) and which carries a semantics trade-off flagged open in that issue. This PR carries the winner's context, which the emission site trivially has, and decides nothing about ownership. The two are orthogonal; landing this does not implement #968. It does change #968's cost/benefit — the placeholder rows #968 exists to avoid become retirable once a block-context sweep (or a later
BlockProcessed/chainlock advance) gives them a finality horizon, so the population is no longer permanent — which may be worth weighing when deciding whether #968 is still needed.C ABI: deliberately unchanged
WalletEventis Rust-only (Debug + Clone, no serde), so the field addition is a compile-time-visible Rust change with no wire format. Indash-spv-ffi, the dispatch's exhaustive destructure acknowledges the field (winner_mined_height: _) without forwarding it: #962 established thatOnTransactionsSweptCallbackhas no safely-degrading insertion point — a C consumer declaring the function pointer by hand keeps compiling against a grown parameter list and reads shifted arguments. The regenerated header was diffed against the pre-change one: the callback signature is byte-for-byte identical (the field's only appearance is in the copied doc comment, which now records that the Rust event carries more than the C surface and why). If a C consumer ever needs the height, adding it must be a flagged breaking change like #962's.Test plan
key-wallet-manager/src/event_tests.rs::test_block_winner_emits_swept_event_naming_the_released_outpoints— extended to assert the block-context sweep carries exactlySome(101)(the processed block's height), notNoneand not any other height the manager has seen.test_is_locked_mempool_winner_sweeps_with_no_mined_height— an IS-locked winner arriving viaprocess_mempool_transactionsweeps a recorded loser and the event carriesNone; the pair fails if the two contexts are conflated in either direction.dash-spv-ffidispatch tests updated for the new field; both marshalling tests still pass, confirming the C-side parameter layout is untouched.cargo test -p key-wallet— 663 passed.cargo test -p key-wallet-manager— 56 passed (all suites green).cargo test -p dash-spv-ffi --lib— 49 passed.cargo clippy --all-features --all-targets -- -D warnings— clean.cargo fmt --check— clean.Summary by CodeRabbit
New Features
Bug Fixes