fix(key-wallet): out-of-order UTXO spend recorded nowhere — phantom unspent balance (#649) - #909
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe wallet now records block-observed spends and prevents out-of-order processing from recreating spent UTXOs. Account additions rewind sync state. Filter batch commits validate account generations and contiguous coverage per wallet. ChangesObserved spend protection
Sync checkpoint consistency
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
key-wallet/src/transaction_checking/wallet_checker.rs (1)
63-93: 🚀 Performance & Scalability | 🔵 TrivialScalability note: per-transaction recording/removal is un-gated over the whole matched block.
Every transaction in a matched block (including thousands of wallet-irrelevant ones) runs
record_observed_spends(one insert per input) and, on this path,remove_spent_from_accounts, which allocates anall_funding_accounts_mut()Vecper call and scans accounts×inputs. During a large cold rescansynced_heightstays low, soprune_finalized_observed_spendsdefers, lettingobserved_spent_outpointsgrow with total block inputs across all matched blocks (bounded only by the 10M deser cap). Consider short-circuiting when there are no funding accounts/UTXOs to touch, or hoisting the account-handle fetch out of the per-tx loop, to keep busy-block/rescan cost bounded. Correctness is fine; this is about steady-state cost and memory during heavy rescans.🤖 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/transaction_checking/wallet_checker.rs` around lines 63 - 93, Bound the per-transaction work in the block-checking flow by skipping record_observed_spends and remove_spent_from_accounts when the wallet has no funding accounts or UTXOs that can be affected. Reuse or hoist the funding-account handles instead of allocating all_funding_accounts_mut() for every transaction, while preserving spend tracking and account removal whenever relevant funding exists.
🤖 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.
Nitpick comments:
In `@key-wallet/src/transaction_checking/wallet_checker.rs`:
- Around line 63-93: Bound the per-transaction work in the block-checking flow
by skipping record_observed_spends and remove_spent_from_accounts when the
wallet has no funding accounts or UTXOs that can be affected. Reuse or hoist the
funding-account handles instead of allocating all_funding_accounts_mut() for
every transaction, while preserving spend tracking and account removal whenever
relevant funding exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 274499ff-97d6-4dbb-b715-d929b750a8ff
📒 Files selected for processing (13)
dash-spv/src/sync/filters/manager.rskey-wallet-manager/tests/common/mod.rskey-wallet-manager/tests/observed_spent_large_block_stress_test.rskey-wallet-manager/tests/observed_spent_multi_wallet_test.rskey-wallet-manager/tests/out_of_order_spend_repro_test.rskey-wallet/src/managed_account/address_pool.rskey-wallet/src/managed_account/managed_account_ref.rskey-wallet/src/managed_account/managed_core_funds_account.rskey-wallet/src/managed_account/transaction_record.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/managed_accounts.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #909 +/- ##
==========================================
+ Coverage 74.75% 74.82% +0.07%
==========================================
Files 328 328
Lines 76700 77044 +344
==========================================
+ Hits 57337 57651 +314
- Misses 19363 19393 +30
|
Preempting the "block processing is order-enforced, so this shouldn't be possible" objectionThis came up on #649 back in April — @xdustinface noted that block processing is order-enforced and concluded the defect shouldn't be reachable, and @lklimek initially agreed — before @lklimek's 2026-07-07 sub-second deterministic repro on #649 reversed that read. It's worth addressing head-on, because the objection targets the wrong layer: the coin is lost during wallet-layer transaction classification, not during chain/header sequencing, and in-order block delivery does not close that gap. The root cause is classification, not ordering. The "already spent" guard in
This is not theoretical here. The PR's On coverage: the previously-uncovered branches of this machinery that Codecov flagged (the |
|
Re the wallet_checker.rs:63-93 scalability note: leaving this as-is; the cost is already bounded and the correctness-critical half can't be applied safely. |
…urface guard-rewritten records, restore public account API Addresses three external review findings on top of PR dashpay#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-dashpay#649 behavior); the checker uses new pub(crate) *_with_observed_spends variants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review fixes for dashpay#909: generation guard, surfaced record rewrites, restored public API
… history (dashpay#649/dashpay#846) HashEngineering reported (against dashpay#851/dashpay#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 dashpay#851"; dashpay#851 is superseded by dashpay#909. Investigation of dashpay#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>
|
@HashEngineering thanks for flagging the born-fully-spent case. I dug into it and this PR already records those transactions — the drop you're describing was specific to #851 (the earlier PoC), where the skip gated relevance. #909 reimplemented that path, so I don't think a behavior change is needed here; I've added regression tests to pin it instead. Details:
I traced the funding-after-spend ordering across The existing observed-spent tests only asserted UTXO/balance, so I added two that assert the history-record guarantee directly (just pushed): If you were seeing it drop in a specific scenario, happy to add that as a test case — let me know the ordering/context and I'll reproduce. |
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Rereviewed the current head (f76430d). I don't have any remaining findings.
The issues from my earlier passes are now addressed:
- The filter commit path snapshots each wallet's
account_generation, so an account added after a batch scan cannot have unscanned coverage certified—even when the checkpoint rewind lands inside the batch or does not move. - Funding records compensated by the classification-independent spend guard are returned as
updated_recordsand propagated byWalletManagerindependently ofis_relevant, preserving event-driven persistence consistency. - The existing public
ManagedAccountRefMut::record_transactionandconfirm_transactionentry points remain source-compatible, with wallet-aware observed-spend handling moved into crate-private helpers.
The latest Continuous integration, Sanitizer, and Pre-commit Checks workflow runs for this head are green.
github-actions
Bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
…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>
…ashpay#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 dashpay#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: dashpay#649 Adapted-from: dashpay#851 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…patch coverage Codecov flagged the dashpay#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: dashpay#649 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… 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-dashpay#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>
…urface guard-rewritten records, restore public account API Addresses three external review findings on top of PR dashpay#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-dashpay#649 behavior); the checker uses new pub(crate) *_with_observed_spends variants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… history (dashpay#649/dashpay#846) HashEngineering reported (against dashpay#851/dashpay#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 dashpay#851"; dashpay#851 is superseded by dashpay#909. Investigation of dashpay#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>
… job passes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
f76430d to
e8dd66f
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
github-actions
Bot
removed
merge-conflict
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-manager/tests/common/mod.rs`:
- Around line 58-70: Parameterize the spend_tx helper with a Network argument
and pass it to Address::dummy instead of hardcoding Network::Testnet. Update
every spend_tx call in the observed-spend tests to supply the intended network,
and execute the scenario for both mainnet and testnet configurations.
🪄 Autofix
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 Plus
Run ID: b561e970-0f3e-4f8d-8842-c0290b25dbb4
📒 Files selected for processing (21)
dash-spv/src/sync/filters/batch.rsdash-spv/src/sync/filters/manager.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/process_block.rskey-wallet-manager/src/test_utils/mock_wallet.rskey-wallet-manager/src/wallet_interface.rskey-wallet-manager/tests/common/mod.rskey-wallet-manager/tests/observed_spent_large_block_stress_test.rskey-wallet-manager/tests/observed_spent_multi_wallet_test.rskey-wallet-manager/tests/out_of_order_spend_repro_test.rskey-wallet/src/managed_account/address_pool.rskey-wallet/src/managed_account/managed_account_ref.rskey-wallet/src/managed_account/managed_core_funds_account.rskey-wallet/src/managed_account/transaction_record.rskey-wallet/src/tests/address_pool_tests.rskey-wallet/src/tests/mod.rskey-wallet/src/tests/observed_spent_outpoints_tests.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/managed_accounts.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
🚧 Files skipped from review as they are similar to previous changes (20)
- key-wallet/src/managed_account/address_pool.rs
- key-wallet/src/tests/address_pool_tests.rs
- key-wallet-manager/src/wallet_interface.rs
- key-wallet-manager/src/process_block.rs
- key-wallet/src/tests/mod.rs
- key-wallet-manager/src/test_utils/mock_wallet.rs
- key-wallet-manager/tests/observed_spent_multi_wallet_test.rs
- key-wallet/src/managed_account/managed_account_ref.rs
- key-wallet-manager/tests/out_of_order_spend_repro_test.rs
- key-wallet-manager/src/lib.rs
- key-wallet/src/wallet/managed_wallet_info/managed_accounts.rs
- key-wallet-manager/tests/observed_spent_large_block_stress_test.rs
- key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
- key-wallet/src/managed_account/transaction_record.rs
- dash-spv/src/sync/filters/batch.rs
- key-wallet/src/wallet/managed_wallet_info/mod.rs
- key-wallet/src/transaction_checking/wallet_checker.rs
- key-wallet/src/managed_account/managed_core_funds_account.rs
- key-wallet/src/tests/observed_spent_outpoints_tests.rs
- dash-spv/src/sync/filters/manager.rs
…elper 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>
ZocoLini
left a comment
ZocoLini
left a comment
There was a problem hiding this comment.
What this PR actually fixes
The #649 mechanism is two pieces, ~40 lines:
- wallet_checker.rs:63-82: record every input seen in a block-context tx into observed_spent_outpoints, independent of classification.
- managed_core_funds_account.rs:285-295: in update_utxos, skip inserting an output whose outpoint is already in that map.
I disabled everything else on this branch (both remove_spent_from_accounts call sites and compensate_for_observed_spends) and ran the suites: all three new integration tests pass, including the deterministic repro, and cargo test -p key-wallet --lib gives 611 passed / 3 failed: the 3 being tests of the extra machinery itself.
So: 2128 added lines for a ~40-line root-cause fix, bundling four independent bugs.
Suggested split
- #649 proper: observed_spent_outpoints + the update_utxos skip + serde adapter/cap + prune_finalized_observed_spends + the repro test. ~250 lines, mergeable as-is.
- Unattributable spend with funding already present: remove_spent_from_accounts, compensate_for_observed_spends, the *_with_observed_spends API pair, the updated_records change in key-wallet-manager. Real bug, different from #649.
- Filter coverage for late-added accounts: account_generation, the checkpoint rewind, wallet_account_generation, and the dash-spv commit guards. Real bug, unrelated to out-of-order delivery.
- AddressPool::prune_unused leaks script_pubkey_index entries: 2 lines plus its test, unrelated to #649.
Two blockers before 2 and 3 land. Note: This blockers where found by AI, I didn't review them myself since they are not related to 1
Group 3 breaks checkpoint-based sync. The contiguity guard (manager.rs:588-592) assumes synced_height + 1 == scan_start, but scan_start = birth_height.max(header_start_height)
(manager.rs:208-214). When headers sync from a checkpoint above the wallet's birth — the case handled at manager.rs:216-220 — that wallet's checkpoint never advances,
wallets_behind() keeps listing it, and the tick at sync_manager.rs:199-221 rescans forever. Repro in the manager's own test module: fresh wallet at synced_height = 0, first batch
at 1_000_000, matching generation → commits leave it at 0. This advances fine on dev.
Group 2 makes history order-dependent. compensate_for_observed_spends records a tx that genuinely received 2 DASH as a 0-value receive, and since the spend isn't attributed
there's no spend record either — the transaction disappears from history, where in-order delivery yields two records. Balance is right, history isn't. Option C from the issue
(retroactive detection at insert time) would record both correctly instead of erasing the receive.
| /// A transaction spending `outpoint` and paying `value` to an unrelated | ||
| /// external dummy address (`ext_id` selects a distinct address). | ||
| pub fn spend_tx(outpoint: OutPoint, value: u64, ext_id: usize) -> Transaction { | ||
| Transaction { |
There was a problem hiding this comment.
Check on dash/src/test_utils/*.rs, there are imps blocks to create dummy Transactions
| //! | ||
| //! Each test binary pulls this in via `mod common;` and uses a subset of the | ||
| //! helpers, so unused-item warnings are expected per binary and silenced here. | ||
| #![allow(dead_code)] |
There was a problem hiding this comment.
Isn't this unnecessary, as far as I know a symbols is not flagged as unused when a subset of tests use it
…d-spend mechanism Applies ZocoLini's requested simplification (PR dashpay#909 review): the dashpay#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>
|
@ZocoLini applied the core simplification — head is now Removed (group 2 — the unattributable-spend compensation machinery)
Net −417 lines in the #909 commit. Kept (the two-piece mechanism + its plumbing)
One rebuttal. You listed the Order-dependent historyRemoving Test-helper nits (
|
…iant 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 dashpay#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.
3 participants
Summary
Fixes #649. When a spending transaction is processed before its funding transaction (a normal ordering during compact-filter rescans and mempool-chained activity), the wallet does not yet own the spent input, classifies the spend as irrelevant, and records it nowhere. The funding tx is then inserted later as a fresh, spendable UTXO — a phantom balance that deterministically survives full from-seed rescans.
Basis: PR #851
The core mechanism here is adopted from @lklimek's #851 (a wallet-level
observed_spent_outpointsmap recording every block-observed spend independent of classification) — that design is correct and this PR builds directly on it. On top of the #851 core, this PR adds what our on-device validation showed was needed:out_of_order_spend_repro_test.rs): funds an address, then delivers the spend block before the funding block — fails on currentdev, passes with the fix. Plus two regression tests through the publicWalletManagerAPI (multi-wallet, large-block stress).AddressPool.script_pubkey_indexcleanup inprune_unused.prune_finalized_observed_spendscaps the set at the finality boundary.On-device validation (Android, testnet)
2febe5d7e8ad1dd0fb633004a82a24783d9b2e9095883541576e5f1344eb9975:0, spent per dashj, unspent per dash-spv) that survived a full wallet rebuild from seed — matching this root cause exactly.12.58712954DASH), ~8-minute scan.Tests
Relationship to #851
This PR supersedes #851 with the same core design plus the tests and hardening above; recommending #851 be closed in its favor (comment posted there). Design credit to @lklimek.
Summary by CodeRabbit
Bug Fixes
Reliability