fix(dash-spv): match masternode collateral outpoints in compact filters - #864
Merged
Conversation
Dash Core inserts a `ProRegTx`'s `collateralOutpoint` into the block's BIP158 compact filter serialized the consensus way — 32-byte txid followed by the 4-byte little-endian vout, 36 bytes total (see `ExtractSpecialTxFilterElements`). A wallet that owns the 1000-DASH collateral output but holds none of the masternode's owner/voting keys — for example a third party registered the masternode against the wallet's collateral — never matches that `ProRegTx` on the scriptPubKey path, so the block is skipped during compact-filter sync. This extends `ManagedWalletInfo::monitored_filter_elements` to append each watched UTXO's consensus-serialized outpoint after the existing owner/voting hashes. The matcher and manager already fold `monitored_filter_elements` into the shared `FilterQuery`, so no change is needed there. The outpoint elements are gated on the wallet holding provider accounts (`ManagedAccountCollection::has_provider_accounts`). The collateral element only helps a masternode owner — it discovers a `ProRegTx` that references one of the wallet's UTXOs as collateral — and the common paths are already covered without it (an owner-created `ProRegTx` is matched by its fee input's script, a key-holding owner by the owner/voting hashes). A funding-only wallet can never hit that case, so it skips these elements rather than pay the per-UTXO false-positive cost on every compact-filter query. Within a provider wallet every UTXO is watched, to avoid hardcoding network-specific collateral amounts. Step 2 of #861: collateral outpoint. `proTxHash` follows in the next stacked branch.
Contributor
📝 WalkthroughWalkthroughProvider-capable wallets now add consensus-serialized UTXO outpoints to monitored compact-filter elements. Provider detection and wallet test options were updated, with tests covering provider filtering and extra-element matching. ChangesProvider collateral compact-filter coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@key-wallet/src/managed_account/managed_account_collection.rs`:
- Around line 98-109: Move the stale “Check if a managed account type exists in
the collection” rustdoc block so it directly precedes
contains_managed_account_type, and ensure has_provider_accounts retains only
documentation describing provider account detection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db961ade-789f-4a72-b927-cfa75c84a791
📒 Files selected for processing (4)
key-wallet-manager/src/matching.rskey-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/test_utils/wallet.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
Comment on lines
98
to
+109
| /// Check if a managed account type exists in the collection | ||
| /// Whether the wallet holds any masternode provider account (owner, | ||
| /// voting, operator, or platform keys). | ||
| /// | ||
| /// A wallet with none of these can never own or operate a masternode, so it | ||
| /// has no reason to watch for masternode special transactions. | ||
| pub(crate) fn has_provider_accounts(&self) -> bool { | ||
| self.provider_owner_keys.is_some() | ||
| || self.provider_voting_keys.is_some() | ||
| || self.provider_operator_keys.is_some() | ||
| || self.provider_platform_keys.is_some() | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale doc comment on line 98 now attaches to has_provider_accounts.
Line 98 (/// Check if a managed account type exists in the collection) was the doc comment for contains_managed_account_type, but the new method was inserted between them. In Rust, consecutive /// lines form one doc block, so has_provider_accounts now carries an incorrect first line in its rustdoc.
📝 Proposed fix: move the stale comment to `contains_managed_account_type`
/// Check if a managed account type exists in the collection
+ pub fn contains_managed_account_type(&self, managed_type: &ManagedAccountType) -> bool {
+
+ /// Whether the wallet holds any masternode provider account (owner,
+ /// voting, operator, or platform keys).
+ ///
+ /// A wallet with none of these can never own or operate a masternode, so it
+ /// has no reason to watch for masternode special transactions.
+ pub(crate) fn has_provider_accounts(&self) -> bool {Reordering so contains_managed_account_type's doc stays with it:
+ /// Whether the wallet holds any masternode provider account (owner,
+ /// voting, operator, or platform keys).
+ ///
+ /// A wallet with none of these can never own or operate a masternode, so it
+ /// has no reason to watch for masternode special transactions.
+ pub(crate) fn has_provider_accounts(&self) -> bool {
+ self.provider_owner_keys.is_some()
+ || self.provider_voting_keys.is_some()
+ || self.provider_operator_keys.is_some()
+ || self.provider_platform_keys.is_some()
+ }
+
/// Check if a managed account type exists in the collection
pub fn contains_managed_account_type(&self, managed_type: &ManagedAccountType) -> bool {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@key-wallet/src/managed_account/managed_account_collection.rs` around lines 98
- 109, Move the stale “Check if a managed account type exists in the collection”
rustdoc block so it directly precedes contains_managed_account_type, and ensure
has_provider_accounts retains only documentation describing provider account
detection.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #864 +/- ##
==========================================
+ Coverage 73.62% 73.69% +0.07%
==========================================
Files 324 324
Lines 73374 73434 +60
==========================================
+ Hits 54018 54117 +99
+ Misses 19356 19317 -39
|
QuantumExplorer
approved these changes
Jul 11, 2026
This was referenced Jul 11, 2026
bfoss765
added a commit
to bfoss765/rust-dashcore
that referenced
this pull request
Aug 5, 2026
…efore-funding (dashpay#649) A spend processed BEFORE the transaction that funded the UTXO it spends (out-of-order block delivery during a cold rescan) leaves that UTXO permanently in the wallet's tracked set, producing phantom spendable balance. Device evidence (testnet): output 2febe5d7e8ad1dd0fb633004a82a24783d9b2e9095883541576e5f1344eb9975:0 (1,000,000 duffs) is counted unspent by the SDK while dashj has it spent, and the phantom +0.01 survives a full wallet rebuild from seed (fresh re-derivation + rescan reproduces it deterministically), proving the miss lives in the scan/processing path, not just live mempool ingestion. This test models that scenario at the WalletManager level: fund 1,000,000 duffs to a wallet address, then deliver the spending block (height 200) BEFORE the funding block (height 100). It asserts the funding outpoint is NOT still tracked afterward. FAILS on the current pin (which already contains dashpay#837/dashpay#864/dashpay#891/dashpay#893) — those do not address this defect. Refs: dashpay#649 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
added a commit
that referenced
this pull request
Aug 6, 2026
…nspent balance (#649) (#909) * test(key-wallet-manager): deterministic repro of out-of-order spend-before-funding (#649) A spend processed BEFORE the transaction that funded the UTXO it spends (out-of-order block delivery during a cold rescan) leaves that UTXO permanently in the wallet's tracked set, producing phantom spendable balance. Device evidence (testnet): output 2febe5d7e8ad1dd0fb633004a82a24783d9b2e9095883541576e5f1344eb9975:0 (1,000,000 duffs) is counted unspent by the SDK while dashj has it spent, and the phantom +0.01 survives a full wallet rebuild from seed (fresh re-derivation + rescan reproduces it deterministically), proving the miss lives in the scan/processing path, not just live mempool ingestion. This test models that scenario at the WalletManager level: fund 1,000,000 duffs to a wallet address, then deliver the spending block (height 200) BEFORE the funding block (height 100). It asserts the funding outpoint is NOT still tracked afterward. FAILS on the current pin (which already contains #837/#864/#891/#893) — those do not address this defect. Refs: #649 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(key-wallet): track wallet-level observed_spent_outpoints to fix #649 Root cause: the "already spent" guard in managed_core_funds_account.rs::update_utxos keys off the ACCOUNT-LOCAL `spent_outpoints` set, which is only populated when the account itself processes the spending transaction. When a spend is delivered before its funding tx (out-of-order rescan), the wallet does not yet own the input, so the spend is classified as irrelevant, update_utxos never runs for it, and nothing records the spend. When the funding tx is processed later, the output is (re-)inserted as a fresh, spendable UTXO -> phantom balance that survives a full from-seed rescan. Fix (adapted from #851): record every spend observed in a block into a new wallet-level `observed_spent_outpoints` map (ManagedWalletInfo), independent of the spending tx's classification or account attribution. update_utxos and record_transaction consult this map: - update_utxos skips any output already observed spent (spend-first ordering: funding arrives after the spend). - remove_spent_from_accounts drops a coin the matched-account path missed (funding-first ordering: spend routed to another account). - TransactionRecord::compensate_for_observed_spends keeps net_amount / output_details consistent with the observed spend (declarative, so it is idempotent across rescan replays). The set is bounded-permanent: entries are evicted by prune_finalized_observed_spends once the spend height is provably final (<= min(chainlock height, synced_height)); add-account rewinds the sync checkpoint so a late account gets filter coverage before pruning can run. A dash-spv commit-time contiguity guard keeps a mid-flight account-add rescan from being silently clobbered forward. Also fixes AddressPool::prune_unused to clear script_pubkey_index alongside address_index. Adds manager-level regression tests (multi-wallet, large-block stress) that exercise the fix through the public WalletManager API. The repro test from the previous commit now passes; key-wallet (549), key-wallet-manager (all) and dash-spv lib (482) suites are green. The pre-existing masternode-network integration failures (test_utils/masternode_network.rs:106) are unrelated and fail identically on the clean pin. Refs: #649 Adapted-from: #851 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(key-wallet): cover observed_spent guard branches for #649 patch coverage Codecov flagged the #649 fix's previously-uncovered branches: the wallet-level `observed_spent_outpoints` serde adapter, finality-boundary pruning, the funding-first removal guard, the account-add sync rewind, and the AddressPool `script_pubkey_index` prune fix. The manager-level integration tests only drive the spend-first ordering end-to-end, leaving these reachable only from the crate-internal `pub(crate)` surface. Add `key-wallet/src/tests/observed_spent_outpoints_tests.rs` (the sibling file already referenced by observed_spent_large_block_stress_test.rs) with five white-box tests, plus one AddressPool prune test: - observed_spent_outpoints_survive_serde_round_trip: exercises the (OutPoint, height) sequence serde adapter (serialize + deserialize visitor) and the empty-map / `#[serde(default)]` path, isolated on an account-less wallet so the populated-account `script_pubkey_index` JSON-key blocker does not apply. - prune_finalized_observed_spends_respects_finality_boundary: no-op without a chainlock; otherwise evicts exactly entries at/below min(chainlock height, synced_height), keeping the rescan case (chainlock above sync checkpoint) from over-pruning. - funding_first_guard_removes_held_coin_and_compensates_record: the un-gated remove_spent_from_accounts / finalize_guard_removed_utxo path — coin dropped, reservation released, funding record compensated to net 0; idempotent; coinbase skipped. - wallet_level_set_outlives_account_local_reload: the account-local spent_outpoints derived set (rebuilt from recorded txs via simulate_reload_rebuild_spent_outpoints) forgets an unrecorded spend, but the persisted wallet-level set still prevents resurrection on funding re-delivery. - adding_account_from_xpub_rewinds_sync_checkpoint: standalone-account add collapses synced_height to birth_height - 1; a still-behind checkpoint is left untouched. - prune_unused_clears_script_pubkey_index (address_pool_tests.rs): regression guard for the missing script_pubkey_index.remove in AddressPool::prune_unused. key-wallet lib (554) and key-wallet-manager (all) suites green. Refs: #649 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * review fixes: gate net_amount recompute, surface observed-spend state modification, tighten deser cap - compensate_for_observed_spends: only replace the match-derived net_amount when the compensation actually dropped an output detail, keeping the no-observed-spend path byte-identical to pre-#649 behavior; pinned by a new unit test. - record_observed_spends: report whether the persisted observed-spent map actually changed, and surface that as state_modified in check_core_transaction — a consumer persisting only on reported modifications must not lose a recorded spend across a restart; pinned by a new regression test (new spend reports, unchanged redelivery and mempool spends do not). - Replace a comment reference to a nonexistent test with the inline rationale for why input_details and account_match.sent populate together. - Tighten MAX_OBSERVED_SPENT_OUTPOINTS 10M -> 1M (load-time allocation cap from a few hundred MB to a few tens of MB), still far above any legitimate size. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * review fixes round 2: generation guard for mid-flight account adds, surface guard-rewritten records, restore public account API Addresses three external review findings on top of PR #909: 1. [P1] The commit-time contiguity guard could still certify unscanned coverage for a newly added account: a rewind landing INSIDE a scanned batch's range passed the height check, and an account add that moved no heights (checkpoint already at the birth floor) was undetectable by any height comparison. ManagedWalletInfo now carries an in-memory account_generation counter bumped on every account add (even height-invisible ones); filter scan snapshots it per wallet and commit refuses to advance a wallet whose generation changed since scan. Pinned by three new dash-spv tests including the mid-batch rewind repro (9000 -> scan [5000..9999] -> rewind 7499 -> commit keeps 7499) and the unmoved-checkpoint case. 2. [P2] The funding-first guard rewrote funding records without ever surfacing them: remove_spent_from_accounts now returns post-compensation clones of every rewritten record, the checker adds them to updated_records on both the relevant and irrelevant paths, and the manager propagates updated_records independent of is_relevant, so consumers persisting per-record updates see the rewrite. 3. [P2] ManagedAccountRefMut::record_transaction/confirm_transaction had silently gone pub -> pub(crate) with changed signatures. The public methods are restored with their original signatures (recording with no observed-spend context, the pre-#649 behavior); the checker uses new pub(crate) *_with_observed_spends variants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(key-wallet): pin born-fully-spent recovered transactions stay in history (#649/#846) HashEngineering reported (against #851/#866) that a funding transaction recovered after its spend was already observed -- every wallet-relevant output already in observed_spent_outpoints before the funding is applied -- was dropped from history entirely: the spend-first path made it come out not-relevant, so no TransactionRecord and no detection event were produced, while balance and UTXO set stayed exact. That record-loss "belongs in #851"; #851 is superseded by #909. Investigation of #909 shows its core commit (ebcd40a) already implements the suggested remedy, so no behavior change is needed: - relevance in check_transaction_for_match is address-membership based and is never gated on spent-status, so a fully-spent funding tx is still classified relevant; - ManagedCoreFundsAccount::record_transaction unconditionally inserts the record after TransactionRecord::compensate_for_observed_spends zeroes the already-spent outputs (net 0, no UTXO); - the "never insert already-spent value" guard lives in update_utxos (UTXO insertion only), not in recording. The QuantumExplorer "surface updated records independent of relevance" review fix covers the separate funding-first UPDATE case (remove_spent_from_accounts rewriting an existing funding record); the born-fully-spent NEW-record insertion is covered independently by the record_transaction compensate path. The existing observed-spent tests assert only UTXO/balance, leaving the history-record guarantee uncovered. This adds that coverage: two tests pin that a born-fully-spent recovered tx -- a plain funding tx, and a CoinJoin- style intermediate hop that spends a live coin -- is surfaced as a new record and recorded in the account's transaction history, while balance and UTXOs stay at zero. Verified across InBlock and InChainLockedBlock (chainlocked recovery) contexts and the WalletManager block path during investigation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(key-wallet): fix private intra-doc links so the Documentation CI job passes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(key-wallet-manager): parameterize network in the spend_tx test helper CodeRabbit: the shared `spend_tx` helper hardcoded `Address::dummy(Network::Testnet, ..)`, forcing every observed-spend test onto Testnet (coding guideline: never hardcode network parameters in a shared helper). Add a `network: Network` parameter and thread it into `Address::dummy`; each caller passes the network its manager uses. All key-wallet-manager tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(key-wallet): reduce #649 fix to the two-piece observed-spend mechanism Applies ZocoLini's requested simplification (PR #909 review): the #649 out-of-order-spend fix is the two-piece mechanism only — (a) record every input seen in a block-context tx into `observed_spent_outpoints`, independent of classification (`wallet_checker.rs`), and (b) in `update_utxos`, skip inserting an output whose outpoint is already in that map (`managed_core_funds_account.rs`). Removes the separate unattributable-spend compensation machinery that was bundled in, which also made transaction history order-dependent (a receive delivered before its spend was rewritten to a 0-value entry, erasing it from history — a spend delivered first leaves it intact): - `TransactionRecord::compensate_for_observed_spends` and its unit tests - `ManagedWalletInfo::remove_spent_from_accounts` (both call sites in `check_core_transaction`) and its `finalize_guard_removed_utxo` helper - the now-dead account-local helpers `mark_outpoint_spent`, `release_reservation_for`, and the test-only `simulate_reload_rebuild_spent_outpoints` - the `updated_records`-independent-of-relevance change in `WalletManager` (its only source was the removed funding-first guard) The `record_transaction_with_observed_spends` / `confirm_transaction_with_observed_spends` pair is kept: it is the plumbing that delivers the wallet-level observed map to `update_utxos`, i.e. piece (b) itself. A born-fully-spent funding tx is still recorded in history; its already-spent output is simply never (re-)tracked as a UTXO (balance/UTXO correctness comes from piece (b), not from rewriting the record). Tests updated to pin that the receive is preserved in history rather than erased. Test-helper cleanup (`key-wallet-manager/tests/common/mod.rs`): drop the superfluous `Network` parameter from `spend_tx` (every caller passed Testnet and the payee's network is irrelevant to observed-spend logic) and build the external payee script directly, so the helper needs no network at all. All three observed-spend integration tests (incl. the deterministic repro) and `cargo test -p key-wallet --lib` pass; workspace clippy (`-D warnings`, debug + release) and rustfmt are clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(key-wallet-manager): name the out-of-order repro after the invariant it pins The test asserted `!still_tracked` — the funding UTXO must NOT remain in the tracked set once its spend was observed first — but was named `..._leaves_utxo_permanently_tracked`, i.e. after the #649 bug rather than after the pinned behaviour. A failure therefore read as the expected outcome. Rename to `..._does_not_leave_utxo_tracked`; assertions and scenario are unchanged. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Quantum Explorer <quantum@dash.org>
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.
Dash Core inserts a
ProRegTx'scollateralOutpointinto the block's BIP158 compact filter serialized the consensus way — 32-byte txid followed by the 4-byte little-endian vout, 36 bytes total (seeExtractSpecialTxFilterElements). A wallet that owns the 1000-DASH collateral output but holds none of the masternode's owner/voting keys — for example a third party registered the masternode against the wallet's collateral — never matches thatProRegTxon the scriptPubKey path, so the block is skipped during compact-filter sync.This extends
ManagedWalletInfo::monitored_filter_elementsto append each watched UTXO's consensus-serialized outpoint after the existing owner/voting hashes. The matcher and manager already foldmonitored_filter_elementsinto the sharedFilterQuery, so no change is needed there.The outpoint elements are gated on the wallet holding provider accounts (
ManagedAccountCollection::has_provider_accounts). The collateral element only helps a masternode owner — it discovers aProRegTxthat references one of the wallet's UTXOs as collateral — and the common paths are already covered without it (an owner-createdProRegTxis matched by its fee input's script, a key-holding owner by the owner/voting hashes). A funding-only wallet can never hit that case, so it skips these elements rather than pay the per-UTXO false-positive cost on every compact-filter query. Within a provider wallet every UTXO is watched, to avoid hardcoding network-specific collateral amounts.Step 2 of #861: collateral outpoint.
proTxHashfollows in the next stacked branch.Summary by CodeRabbit
Bug Fixes
Tests