fix(key-wallet): out-of-order UTXO spend causes history divergence (#649)#851
Closed
lklimek wants to merge 30 commits into
Closed
fix(key-wallet): out-of-order UTXO spend causes history divergence (#649)#851lklimek wants to merge 30 commits into
lklimek wants to merge 30 commits into
Conversation
…nd-before-funding defect Forensically traced (not guessed) from a real backend-e2e failure log against live Dash testnet: /data/tmp/backend-e2e-tc004-coldstart-JyD9Vy.log shows a real spend at height 1,474,746 processed at 08:45:45.155707Z with the spending transaction logged as `sent=0 DASH` (the wallet did not yet recognize the input as its own), and the transaction that created that same UTXO, at the earlier height 1,474,688, processed 0.87 seconds LATER at 08:45:46.028186Z — and inserted as a fresh, spendable UTXO despite its own spend having already been observed. That UTXO was later selected as require_final_inputs-eligible asset-lock funding and rejected by the real network (FinalityTimeout) because it was already spent. This test reproduces the exact same order — process the spending block, then the funding block — synthetically and deterministically (no mnemonic, no network, ~0.07s), via key-wallet-manager's public WalletInterface::process_block_for_wallets, matching the style of the existing spv_integration_tests.rs in this crate. Result: RED. `update_utxos`'s own is_outpoint_spent guard (managed_core_funds_account.rs) — which exists specifically to handle "out-of-order block processing during rescan" per its own comment — does not prevent the funding UTXO from being (re-)inserted as tracked/spendable once its spend has already been observed. Relationship to the CoinJoin gap-limit precedent already on this branch (coinjoin_gap_discovery_tests.rs): related in theme (rescan reordering breaking wallet-state invariants) but a different specific mechanism — that precedent's RED case is a CROSS-batch defect (a committed batch's funding output is never rescanned once its address is derived later); this is an INTRA-batch ordering defect (funding and spend land in the SAME committed batch, 1474001-1479000 per the real log, but get processed in the wrong order and the guard meant to compensate does not hold). cargo check / clippy --all-features -- -D warnings / +nightly fmt --check all clean. Actually run and confirmed red twice (before and after fmt). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… UTXO leak (#649) An out-of-order rescan can deliver a spend before the transaction that funds the coin it spends. The old `is_outpoint_spent` guard lived per-account inside `update_utxos` and only fired when the spend was attributable to the owning account — so a spend seen first (funding not yet inserted), or a spend routed away from the owning account by transaction-type narrowing, left the funding UTXO permanently tracked and selectable, which the real network then rejected as already-spent (FinalityTimeout). Fix: a wallet-level, classification-independent, persisted record of every outpoint observed spent in a processed block (`observed_spent_outpoints: BTreeMap<OutPoint, CoreBlockHeight>` on `ManagedWalletInfo`). In `check_core_transaction`, for block contexts only (`InBlock` / `InChainLockedBlock`): - record every input un-gated by relevance or classification (insert-only, so a matched spend still builds its `input_details` from the live funding UTXO); - on the unattributed short-circuit path, drop any coin the spend consumes from whichever funding account holds it (covers the mirror / cross-account-type routing case); - after processing matched accounts, reconcile freshly-inserted outputs against the observed set, dropping any coin whose spend was seen earlier at a higher height (the spend-first ordering). Mempool / InstantSend-only spends are never recorded and self-heal on confirmation. The set is permanent and one-way (no removal/rollback path) — the accepted mirror-image trade documented on the field. Persisted via a `(OutPoint, height)`-pair serde adapter because `OutPoint` is not a valid JSON map key, with `#[serde(default)]` for backward-compatible loads. Turns the committed RED repro green and adds a full suite covering all 19 QA scenarios (persistence, bounded growth, cross-account-type routing, dual bookkeeping, multi-wallet isolation, idempotent re-delivery, permanent-by-design reorg shape). The two existing regression guards and record-detail tests stay green; `input_details` are preserved by the insert-only recording order. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…649 guard QA verification (Marvin): the existing many_orphaned_spend_blocks_stay_bounded_and_linear stress test spreads growth across 2,000 separate block deliveries (3 inputs each). process_block_for_wallets iterates every tx in block.txdata unconditionally, so the real worst case is transactions-per-matched-block, not block count. This test builds one Block with 5,000 unrelated 3-input transactions plus one wallet-relevant spend and confirms: (a) the spend is still correctly reconciled when buried in a large noisy block, and (b) observed_spent_outpoints grows by the block's total input count (15,002 entries from ONE block in ~20ms debug / ~3ms release), not just the relevant tx's inputs — the true growth driver the dev plan's "single-digit MB" estimate did not measure directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Vq23cSxwwL4zSimo491Dr
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Vq23cSxwwL4zSimo491Dr
…649 guard removals; polish Follow-up polish on the #649 out-of-order guard. MUST-FIX (history/balance divergence): when the guard removes a UTXO whose spend was observed but never recorded (the spend looked irrelevant, `sent = 0`), the funding transaction's `TransactionRecord` still credited `+funding_value` while the recomputed balance was zeroed — so `transaction_history()` showed a phantom "received" against a zero balance. Both orderings were affected: the spend-first reconcile path and the funding-first un-attributed-spend path (a spend routed away from the owning account by tx-type narrowing). Fix: `compensate_removed_utxo` reverses the removed value from the funding record and drops it when the tx is left with no net effect and no surviving wallet output; the caller re-syncs the emitted `new_records` from the post-reconciliation account state. Invariant now tested both ways: `transaction_history()` net == `balance.total()`. Also in this pass: - SEC-003: cap `observed_spent_outpoints` deserialize at MAX_OBSERVED_SPENT_OUTPOINTS via a streaming visitor, so a corrupted/hostile wallet file cannot force an unbounded allocation on load. - Correct the serde-adapter rationale: `OutPoint` *is* a valid JSON map key; the pair-form adapter exists for format-agnosticism (bincode/`platform_value` encode `OutPoint` as a struct, unusable as a map key). Fixed in both the production doc and the test helper doc. - Hoist `all_funding_accounts_mut()` out of the per-input/per-output loops in `remove_spent_from_accounts` and `reconcile_inserts_with_observed_spends`. - Document (and test) that reconciliation is intentionally NOT gated to block context: the observed set only holds block-confirmed spends, so a mempool funding must still be reconciled away. - Fix the reconcile doc: it is a plain set-membership lookup, not a height comparison. - Reword the growth/pruning note (driver is transactions-per-matched-block, a documented no-pruning limitation) and drop the overclaimed "survives restart" wording in favor of order-independent correctness within one processing run. TDD: the two history-vs-balance tests were confirmed RED without the compensation. key-wallet 540 + observed-guard module 20 tests green; clippy (`-D warnings`) and `cargo +nightly fmt` clean; default and no-serde builds compile. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ng (QA-001/QA-002) Two regressions in the compensation logic from the previous commit, both rooted in the guard removals never fully mimicking the (unrecorded) spend they stand in for. QA-001 (HIGH) — reprocessing double-compensates, drives net_amount negative: `confirm_transaction` re-runs `update_utxos` on every reprocessing (rescan / duplicate block delivery, exercised by this repo's own rescan integration test). The guard's `account.utxos.remove` never registered the outpoint in the account-local `spent_outpoints` set that `update_utxos`' `is_outpoint_spent` guard checks, so the invalidated output was resurrected and reconciliation subtracted its value a second time (e.g. 500,000 -> -1,500,000 for a two-output funding with one output spent). Fix: `mark_outpoint_spent` registers the removed outpoint locally so `update_utxos` will not re-insert it; and compensation is now idempotent — an output's `output_details` entry marks "not yet compensated", so a re-removal (including across a serialize/deserialize, where the derived local set is rebuilt from recorded transactions and omits the unrecorded spend) is a no-op for the record. Fully-compensated records are now KEPT at net 0 rather than deleted: deleting one made a reprocessing look `is_new`, which — with the coin now marked spent so `update_utxos` won't re-insert it — re-recorded a positive net with no coin to reconcile it back, reopening the divergence. A kept net-0 record is both history-consistent and reprocess-safe. QA-002 (MEDIUM) — output_details desynced from net_amount: after a partial compensation `net_amount` reflected only the surviving output but `output_details` still listed the removed output as Received at full value. Compensation now drops the removed output's `output_details` entry too, so per-output line items agree with the aggregate. `compensate_removed_utxo` renamed to `finalize_guard_removed_utxo` to reflect the broadened responsibility (mark-spent + idempotent record/output compensation). TDD: QA-001 (`-1,500,000`) and QA-002 reproduced RED first, plus a fully-compensated-reprocess case. key-wallet 543 tests green (observed-guard module 23); clippy (`-D warnings`) and `cargo +nightly fmt` clean; default and no-serde builds compile. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Independently verifies 0185922's idempotent-compensation fix and adds a regression test for a newly found divergence: - reprocessing_partially_compensated_funding_stays_idempotent_across_many_replays: idempotency holds across 4 replays, not just one retry. - all_outputs_independently_observed_spent_reaches_net_zero_and_stays_consistent: a multi-output tx where every output is independently observed spent converges to net 0 and stays stable on reprocess. - compensation_survives_simulated_reload_across_two_restart_cycles (+ ManagedCoreFundsAccount::simulate_reload_rebuild_spent_outpoints, #[cfg(test)]): confirms the output_details compensation marker survives a simulated save/reload (account-local spent_outpoints rebuilt the way Deserialize does), across two restart cycles — the persisted wallet-level observed_spent_outpoints, not the transient local mark, is what makes this hold. - spend_first_ordering_with_chainlocked_first_sighting_diverges_history_from_balance (FAILING, default features): when a funding tx's first sighting is already InChainLockedBlock (the normal case for a full historical rescan), record_transaction drops the record to finalized_txids before reconcile_inserts_with_observed_spends can compensate it. Compensation silently no-ops on the missing record, so transaction_history() omits the transaction entirely while balance.total() still reflects the surviving output — history_net() != balance.total(), the exact invariant this whole #649 fix chain exists to protect. Reproducible only under default features; --all-features masks it by force-enabling keep-finalized-transactions, which is off by default in every shipping build (key-wallet-ffi, dash-spv-ffi's production dependency). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Vq23cSxwwL4zSimo491Dr
…ed records (#649 B1) Per the #649 restructure architecture decision: history_net == balance.total() is not a system invariant under default features (keep-finalized-transactions = OFF) for ANY chainlocked-and-pruned funding, independent of #649. Adds a documenting test for the default-feature divergence and a mirror test proving the equality DOES hold when keep-finalized-transactions keeps records live — so a future reviewer does not re-file the pruning-driven divergence as a regression. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…txos (#649 B2) Plumbing-only step of the #649 restructure: `ManagedCoreFundsAccount:: update_utxos`/`record_transaction`/`confirm_transaction` and the `ManagedAccountRefMut` dispatch now take the wallet-level `observed_spent_outpoints` view as a read-only parameter from `check_core_transaction`, which already owns it. No behavior change — `update_utxos` does not yet consult its parameter. Prepares the check-before-insert change (B3). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Spend-first ordering is now dissolved at the source instead of patched after the fact: `update_utxos` skips inserting a UTXO whose outpoint is already in `observed_spent_outpoints`, and `record_transaction` (via the new `TransactionRecord::compensate_for_observed_spends`) drops such outputs from `output_details`/`net_amount` before the record is ever inserted. The record and UTXO set are born correct, so the post-insert `reconcile_inserts_with_observed_spends` reconciliation pass — and its result-resync in `check_core_transaction` — has nothing left to do and is deleted. `compensate_for_observed_spends` recomputes declaratively (from current output_details + input_details) rather than subtracting incrementally, so it is naturally idempotent; `finalize_guard_removed_utxo` (funding- first ordering) still uses the old incremental form and is updated to the same declarative helper in the next commit (B4). All 544 previously-passing key-wallet/key-wallet-manager tests remain green under default features — the born-correct value matches the old after-the-fact compensated value bit-for-bit for every existing scenario. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…#649 B4) Replaces `finalize_guard_removed_utxo`'s incremental net_amount subtract and output_details-presence idempotency marker with a call to `TransactionRecord::compensate_for_observed_spends` (added in B3): the funding-first ordering (an already-live record whose coin is later guard-removed by an unattributed spend) now recomputes output_details/ net_amount declaratively from the current record state every time, instead of subtracting a delta once. Idempotent by construction, so repeated or replayed guard removals can never double-compensate or drift — no marker needed. A no-op when the record has been pruned (default `keep-finalized-transactions = OFF`): β is undefined for a pruned record regardless of this guard. `remove_spent_from_accounts` threads the wallet-level `observed_spent_outpoints` view through to the recompute. Both `key-wallet`/`key-wallet-manager` test suites stay green (544 passed, 1 known pre-existing failure fixed in the next commit) and the #649 repro (`out_of_order_spend_repro_test.rs`) stays green throughout. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ords (#649 B5) The previously-failing `spend_first_ordering_with_chainlocked_first_sighting_ diverges_history_from_balance` asserted history_net == balance.total() after a chainlocked-first-sighting funding — an equality that was never a system invariant for pruned records (see B1). Split into two feature-scoped tests: - Under default features: the record is born correct (B3) then pruned to finalized_txids; history_net is 0 while balance reflects the live coin. Documented as the general pruning non-invariant, not a #649 defect. - Under keep-finalized-transactions: the record stays live, was born correct at construction, and β holds — confirming the default-feature divergence is purely a pruning artifact, not an unclosed #649 gap. key-wallet: 545 passed / 0 failed (default), 543 passed / 0 failed (keep-finalized-transactions). key-wallet-manager: all green including the #649 repro. clippy clean under both configs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…#649 B6) Self-review pass: record_transaction's doc comment described history ("consulted starting in the check-before-insert change that follows this plumbing commit") instead of present behavior. Points at compensate_for_observed_spends directly, which is called a few lines below. Final B6 verification: key-wallet + key-wallet-manager green under default features (545 passed) and --features keep-finalized-transactions (543 passed); clippy --all-features --all-targets -D warnings clean; cargo +nightly fmt --check clean; downstream dash-spv and key-wallet-ffi build clean against the changed (still-unreleased) ManagedAccountRefMut API. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… sum (RUST-001/SEC-02) QA (Adams RUST-001, converged with Smythe SEC-02): compensate_for_observed_spends recomputed net_amount unconditionally as sum(surviving Received/Change output_details) - sum(input_details), silently replacing the authoritative account_match.received - account_match.sent basis for EVERY record, not just #649-affected ones. Two concrete divergences: an output matched by script (contains_script_pub_key) but unresolved to a tracked address is counted by account_match.received but invisible to output_details; account_match.sent can be nonzero while input_details is empty (partial rescan, per the existing comment at managed_core_funds_account.rs). Fix: compensate_for_observed_spends now only ever subtracts a delta — the value of output_details entries newly found in observed_spent — from whatever net_amount currently is, and never re-derives the base from output_details/input_details sums. Since record_transaction seeds net_amount from account_match.received - account_match.sent before calling this helper, the account_match-anchored value survives untouched whenever nothing is excluded (no threading of account_match through the helper needed). Still idempotent by construction: retain() permanently removes a compensated entry, so a repeat call for the same output finds nothing left to subtract. Explicitly did NOT apply an `if observed_spent.is_empty() { return }` guard: observed_spent_outpoints is permanent/monotonic (no reorg-removal path anywhere in key-wallet/key-wallet-manager), so after a wallet's first observed spend that guard would never fire again for the wallet's lifetime. Adds two regression tests (observed_spent empty, no #649 spend at all): (a) an output matching by script but failing address resolution (forced via direct AddressPool field manipulation, isolated from the separate prune_unused desync bug), (b) a partial-rescan case with account_match.sent > 0 and empty input_details (constructed via a direct record_transaction call with a stale account_match, mirroring the existing backfill-test pattern). Both assert net_amount == account_match.received - account_match.sent. 547 passed / 0 failed (default features), 545 passed / 0 failed (keep-finalized-transactions); clippy --all-features --all-targets -D warnings clean; +nightly fmt clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…+ idempotency Independently reconstructs decision-doc gap #5 (async apply_chain_lock prune racing an out-of-order unattributed spend) as a new integration test — passes, confirming the restructure's "recognized as β-undefined for pruned records, not a defect" closure claim rather than assuming it. Adds a white-box unit test directly exercising TransactionRecord::compensate_for_observed_spends twice (plus a third call with an expanded observed-spent set) to prove the "declarative, not incremental" idempotency claim in code, not just by trusting the doc comment. QA verification only — no production code changed.
…aming (DOC-001..006) Adams' docs review of the #649 restructure: several touched comments sat on private/pub(crate) items well over the internal ≤3-line budget, and narrated the *removed* incremental-compensation approach by contrast instead of describing present behavior — both against coding-best-practices. Applies her proposed rewrites: - update_utxos (DOC-001): 6 lines -> 2, drops "instead of needing after-the-fact compensation" contrast. - compensate_for_observed_spends (DOC-002): re-tightened for the RUST-001 fix's new delta-based implementation (already rewritten in the prior commit; trimmed further here). - record_transaction's #649 inline comment (DOC-003): 5 lines -> 2. - wallet_checker.rs's #649 block comment (DOC-004): 11 lines -> 3, drops "no longer needs a post-insert reconciliation pass" history framing. - gap-4 test docs (DOC-005): drop the "(B3)" plan-step label (meaningless outside the uncommitted decision doc) from both the default-feature and keep-finalized-transactions test doc comments. - finalize_guard_removed_utxo / remove_spent_from_accounts (DOC-006): trim ~20/~16 lines to ~10/~3, drop the "not an incremental subtract" contrast. Also corrects three comments left stale by the RUST-001 fix (not part of Adams' review, since it predates that fix): the cherry-picked idempotency test's and gap-5 test's doc comments described compensate_for_observed_spends as "recomputing" from output_details/input_details, which the RUST-001 fix replaced with a delta-subtract from the current net_amount. No behavior change. 549 passed / 0 failed (default), 546 / 0 (keep-finalized-transactions); clippy --all-features --all-targets -D warnings clean; +nightly fmt clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… prune_unused Marvin's LOW finding (discovered chasing SEC-02): AddressPool::prune_unused removed a pruned address from address_index but left its scriptPubkey behind in script_pubkey_index — a stale script->index mapping that would make contains_script_pub_key keep matching an address the pool otherwise no longer resolves. prune_unused has zero production callers today, so this was dormant, but it is a pub fn (part of the crate's API contract) and a one-line fix, worth landing before anything is ever wired up to call it. No behavior change to any current caller (there are none); no existing test exercised prune_unused. 549 passed / 0 failed (default), 546 / 0 (keep-finalized-transactions); clippy --all-features --all-targets -D warnings clean; +nightly fmt clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…1 — investigated, no live defect Marvin's exhaustive empirical follow-up (Addendum 2, /tmp/claudius-649-restructure-marvin-report.md) tested all three of Adams's RUST-001 divergence scenarios directly rather than theorizing about them, and found no reachable trigger for the concern the prior commit (c2184f3) fixed: - Bare P2PK (scenario 1): unreachable by two independent mechanisms — AddressType has no P2PK variant so it's never registered into script_pubkey_index, and a funding tx to a real P2PK script isn't even recognized as account-relevant (check_transaction_for_match returns None), so it never reaches record_transaction. - Partial-rescan sent/input_details mismatch (scenario 2): unreachable — sent's one assignment site and input_details' loop use the identical predicate over the same account state with no mutation in between; they cannot diverge as the (stale/defensive) code comment implies. - Funds vs. keys formula difference (scenario 3): true but deliberate — keys accounts are documented thin markers with permanently-empty input/output details; #649's premise doesn't apply to them. The only demonstrated live trigger for the whole RUST-001/SEC-02 concern is the dormant AddressPool::prune_unused desync (fixed separately, one-liner). Once that lands, there is no evidence of a reachable path where net_amount diverges from account_match. Touching the core accounting formula to guard against an unreached path is exactly the kind of change that should earn its risk rather than get it for free — so it's reverted rather than kept "just in case." Restores the original declarative-recompute compensate_for_observed_spends (net_amount recomputed from surviving output_details/input_details, never a delta) and removes the now-superseded net_amount_source_of_truth_tests.rs. Also reverts the three comments the prior commits had updated to describe the delta-subtract mechanism (the idempotency test's and gap-5 test's doc comments, and finalize_guard_removed_utxo's doc) back to accurately describing recompute, and re-applies Adams's exact DOC-002 proposed text (dropped in the interim by the now-reverted refactor). RUST-001 is closed as "investigated, no live defect found" (pinned by Marvin's ported regression suite in the next commit), not "fixed" — a valid QA outcome per team-lead. 547 passed / 0 failed (default features); clippy --all-features --all-targets -D warnings clean; +nightly fmt clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…urce-of-truth shift Adds 4 tests requested by Smythe's security pass on the #649 restructure: - 3 regression tests pinning net_amount == account_match.received - account_match.sent (the pre-restructure formula) when observed_spent is empty, across incoming/outgoing/self-transfer flows — confirms B3's unconditional compensate_for_observed_spends call doesn't silently change the common-case value. - 1 adversarial test constructing Smythe's hypothesized divergence condition directly: forces an AddressPool desync via prune_unused (which clears address_index but leaves script_pubkey_index stale — a pre-existing, #649-unrelated gap, reachable via prune_unused's public API even though it currently has no production callers), funds an output to the desynced address alongside a normal one, and confirms: the OLD account_match-derived formula would have over-counted a value the wallet never actually held as a spendable UTXO, while the NEW recompute agrees with balance. The restructure doesn't introduce a new spendable-fund miscount here — it happens to resolve a pre-existing mismatch instead of preserving it. QA verification only — no production code changed.
…unreachable Follow-up to Adams's independent RUST-001 finding (net_amount source-of-truth shift, same root cause as Smythe's SEC-02). Adds 2 tests attempting Adams's two concrete scenarios with observed_spent empty, isolating from #649 entirely: - scenario1_bare_p2pk_is_structurally_unreachable: builds a real P2PK script from the account's own derived pubkey and confirms contains_script_pub_key never matches it (AddressType has no P2PK variant; the pool only ever registers P2PKH scripts) — the transaction isn't even recognized as relevant, so it never reaches record_transaction. The underlying general class Adams illustrates with P2PK is real, just already demonstrated via a different, genuinely constructible trigger (see pruned_address_script_desync_does_not_under_report_relative_to_balance). - scenario2_sent_and_input_details_cannot_diverge_in_current_code: confirms account_match.sent (account_checker.rs, the single assignment site) and record_transaction's input_details both derive from the identical self.utxos.get(&input.previous_output) predicate on the same account instance with no intervening mutation — so sent > 0 cannot occur with empty input_details in the current code. The cited code comment's rationale doesn't correspond to a live divergence today. QA verification only — no production code changed. Fixed one clippy unnecessary-get-then-check flagged in my own test before committing.
…ect field removal Marvin's cherry-picked pruned_address_script_desync_does_not_under_report_ relative_to_balance (fbcbdf32) constructed its script/address desync by calling AddressPool::prune_unused — but this branch's earlier prune_unused fix (6217f1f, already landed when this test was cherry-picked) closed exactly that gap, so prune_unused no longer leaves the desync behind and the test's own setup assertion started failing immediately after the cherry-pick. Ports the test's intent (not its now-invalid setup mechanism): constructs the same address_index/script_pubkey_index desync via direct field removal (`external_pool.address_index.remove(...)`), the same technique already used elsewhere on this branch for the equivalent construction. All downstream assertions (account_match divergence, balance, net_amount, the three-way comparison) are unchanged — only the now-defunct prune_unused-based setup and its doc comment's description of the (now-fixed) mechanism are updated. 553 passed / 0 failed (default features), 550 / 0 (keep-finalized-transactions); clippy --all-features --all-targets -D warnings clean; +nightly fmt clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #851 +/- ##
==========================================
+ Coverage 74.54% 74.61% +0.06%
==========================================
Files 327 327
Lines 75032 75332 +300
==========================================
+ Hits 55936 56209 +273
- Misses 19096 19123 +27
|
`mis-processed` tokenized to a standalone `mis`, which the typos dictionary flags as a likely misspelling of `miss`/`mist`. Reword to avoid the hyphenated prefix; no behavior or test change. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Cleanup, hygiene, and API-surface fixes on top of the out-of-order UTXO spend reconciliation (#649). Core reconciliation logic and its assertions are unchanged. - PROJ-001: demote `ManagedAccountRefMut::{record_transaction, confirm_transaction}` to `pub(crate)` — both gained a required parameter and have no external callers (verified workspace-wide), so this closes the unintended semver break. - CODE-001: rename `sec02_*`/`gap5_*` test files, and rewrite review-session narrative (finding IDs, reviewer personas, machine-local log paths, cross-repo doc refs) into present-tense comments citing #649. - CODE-002: extract shared `rebuild_spent_outpoints` helper (Deserialize + test reload) and consolidate duplicated test helpers into `key-wallet/src/test_utils/blocks.rs` and `key-wallet-manager/tests/common`. - RUST-002/PROJ-002: make the observed-spent-set growth/pruning and `#[serde(default)]` backward-compat docs honest. - RUST-003: rewrite the `has_inputs` comment to state the actual invariant. - RUST-004: release build reservations in the guard-removal path (`finalize_guard_removed_utxo`), matching `update_utxos`; add a regression test proving immediate release (not via TTL). - RUST-005: convert CI-flaky wall-clock timing asserts into `eprintln!` diagnostics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…y pruning Phase 1 of the RUST-002 design: replace the insert-only, permanent-forever `observed_spent_outpoints` set with bounded permanence. Entries are evicted only at or below the finality boundary `min(last_applied_chain_lock height, synced_height)` — the point at which the spend is chain-locked (never reorged out) and any funding transaction has been delivered and finalized, so no redelivery path can resurrect the coin in either `keep-finalized-transactions` configuration or across a reload (#649). Eviction is event-driven (chainlock application, sync-checkpoint commit), never age- or recency-based: a naive age/LRU eviction is unsafe because it would drop the cold entries whose funding tx may still arrive out of order. - `ManagedWalletInfo::prune_finalized_observed_spends`: O(n) `retain` above the boundary; no-op until a chainlock defines the boundary. - Hook it into `apply_chain_lock` (after account promotion + metadata advance) and `update_synced_height`. - Rewrite the field doc: bounded-permanence invariant replaces the "not pruned" limitation. - Replace the permanence pin test with `observed_spend_removal_only_via_ finality_boundary`; add 6 regression tests (redelivery, across-reload, guard-removed survives-reload-then-prunes, no-prune-above-boundary, event-driven-not-age-based, self-heal-on-replay). Phase 2 (reorg retraction of the provisional tier) is out of scope — blocked on a dash-spv reorg pipeline and a WalletInterface disconnect event. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
coin Marvin/QA adversarial regression test for commit 5d7737b's finality-boundary pruning (#649 follow-up). Prunes the wallet-level observed_spent_outpoints entry via chainlock + synced_height advance, THEN adds a brand-new managed account, THEN delivers the funding tx to that account for the first time (no replay of the spend block in between). FAILS as expected: the coin resurrects. Before 5d7737b this scenario was safe because the guard map was permanent; the finality-boundary pruning now evicts the only protection available to an account that didn't exist at prune time, and nothing guarantees the spend block is replayed to re-guard it. This is a real regression, not the documented "self-heals on same-rescan replay" residual from the design doc — this test constructs exactly that residual's shape and shows it does NOT self-heal without an explicit rescan trigger. Intentionally left failing (red) to pin the bug for the developer; does not modify any existing test. See QA report for full findings: /tmp/claudius-q2TJyL/marvin-pr851-rust002-verify.md Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
QA-001: finality-boundary pruning (5d7737b) was unsound across account addition. `synced_height` certifies "every filter at or below this height was matched against the wallet's scripts" — but only for the account set present when those filters were scanned. `add_managed_*` grew the script set without invalidating the certificate, so `prune_finalized_observed_spends` consumed a stale certificate and could evict the only guard a newly-added account had for a coin spent before it existed, reopening #649 (#649). Fix the certificate, not the rule (Nagatha design §7): Part 1 (key-wallet): `ManagedWalletInfo::rewind_sync_checkpoint_for_new_account` rewinds `synced_height` to `birth_height - 1` on the success path of all six `add_managed_*` variants. The prune boundary collapses immediately and the sync layer backfills the new account from birth through the existing per-wallet rescan machinery. `last_processed_height` and `last_applied_chain_lock` are left untouched (chain facts, not coverage facts; keeping the chainlock makes the replayed history arrive born-chainlocked). Part 2 (dash-spv): commit-time contiguity guard in `try_commit_batches` — a batch advances a wallet's checkpoint only if the wallet was already certified up to the batch's start (`synced_height + 1 >= batch_start`). Closes the race where a batch scanned before the rewind commits afterward and clobbers the rewound checkpoint forward, permanently cancelling the rescan. Transparent to normal ascending commits. Tests: re-scope Marvin's pinning test (its literal assertion specified the deferred per-account-floor option, not this fix) into `late_account_finality_boundary_test.rs` — rewind pin, forward-replay convergence (both feature configs), interrupted-replay non-prunability, and an all-variants/no-op/no-underflow pin. Extend the boundary-invariant test to cover account addition. Add dash-spv contiguity-guard tests (mid-flight rewind not clobbered; guard transparent in normal advance). Rider: rename the stale `orphaned_spend_never_expires_no_pruning` test to state the current invariant. The accepted residual is a transient backfill window (funding replayed before its spend), same shape as the other documented residuals — the durable resurrection is eliminated. Per-account spendability gating (zero window) and reorg retraction remain deferred follow-ups. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two additions probing edge cases the task explicitly asked to attack: - dash-spv: contiguity_guard_is_per_wallet_not_batch_wide — two wallets in the same batch, only one rewound mid-flight (simulating an account add). Proves the try_commit_batches contiguity guard has no cross-wallet leak: the rewound wallet stays blocked, the untouched wallet still advances in the same commit call. - key-wallet: back_to_back_account_adds_rewind_idempotently — two accounts added with no intervening update_synced_height between them. Proves the rewind floor (wallet-level birth_height) makes repeated rewinds idempotent regardless of sequencing. Both pass. Full writeup and two new findings (QA-005, QA-006) in /tmp/claudius-q2TJyL/marvin-pr851-qa001-final-verify.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Two findings from field-testing this branch (merged with #866)#866's committed-range rescan re-applies old funding blocks after their spends were already processed, which makes out-of-order application routine — a good stress test for this PR. Tested via the The core fix works: without this PR the out-of-order re-application left four denominated coins falsely unspent; with it the final balance is exactly correct. 1. Infinite restart loop when wallet birth is below the checkpoint anchorThe commit-time contiguity guard ( 2. Out-of-order funding transactions are dropped from history entirelyA funding tx recovered after its spends were observed — all of its wallet-relevant outputs already in the observed-spent set — currently produces no 🤖 Generated with Claude Code |
Contributor
|
Supportive on-device datapoint (Android, testnet, bundled with #866) This PR was included in both device runs described in the #866 comment (CoinJoin-heavy reference wallet, 1,049 txs, first scan at gap 30 and gap 100). In both runs the balance and UTXO set came out byte-exact against the dashj oracle even where history records were missing — no history/balance divergence of the kind this PR addresses was observed. Bounded honestly: the runs bundled this PR with #866, so we can't isolate its individual effect, but the out-of-order-spend behavior it targets showed no regressions under a wallet class that exercises it heavily. 🤖 Generated with Claude Code |
Contributor
Author
|
Note this PR is 100% vibe-coded, I hope @xdustinface will find some time and take it over, as I don't have enough understanding to push it forward. |
6 tasks
This was referenced Jul 16, 2026
Closed
Contributor
Finding 2 becomes release-blocking once #866 or #873 merges: transactions will go missing from historyBoth open #846 fixes (#866 and #873) work by re-downloading and re-applying old blocks whose addresses were derived late. By construction, every transaction they recover is applied spend-before-funding — the forward scan already processed the spends. That is exactly the path where this PR's spend-first rule ("never insert already-spent value") currently causes the whole transaction to be treated as not-relevant: no So the combination math is:
Measured, same reference wallet: #866 alone = 1,047/1,047 records; #866 + this PR = 1,039/1,047 on desktop and 909/1,049 on Android at gap 30 (fresh network syncs apply blocks far more out-of-order, so the skip fires more often). #873 uses the identical The suggested change is unchanged from finding 2, but its priority rises with either merge: when all of a funding transaction's outputs are already in the observed-spent set, record the transaction anyway — outputs marked spent, zero net, no UTXOs inserted — instead of skipping it. That keeps this PR's balance guarantee intact while making the history complete, and unblocks shipping whichever #846 fix the maintainers choose. 🤖 Generated with Claude Code |
Contributor
|
Recommending we close this in favor of #909, which adopts this PR's core design (
The mobile teams are shipping against this fix now, so getting #909 reviewed/merged unblocks re-pinning from upstream. Happy to fold anything from the review back here if preferred. |
Contributor
Author
Sounds good, this one was fully-vibecoded, I didn't expect it to be the correct solution for the problem, more like proof-of-concept. |
Contributor
Author
|
Superseded by #909 |
bfoss765
added a commit
to bfoss765/rust-dashcore
that referenced
this pull request
Jul 23, 2026
… 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>
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
4 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.
Why this PR exists
key-walletcould record a transaction's historynet_amountincorrectly when a UTXO's spend was observed out of order relative to the funding transaction being recorded — the wallet's transaction history and its balance could permanently diverge for affected coins.What was done
Restructured how
key-walletreconciles a transaction record'snet_amountagainst observed spends, replacing an after-the-fact compensation mechanism (which mutated aTransactionRecordthat an orthogonal finalization/pruning subsystem could remove out from under it) with two structurally sound orderings:record_transaction/update_utxosnow check the wallet-level observed-spent set at construction time and never insert already-spent value in the first place.net_amount/output_detailsare recomputed declaratively (idempotent, safe to re-run), and left alone (no-op) if the record has already been pruned/finalized — that state is out of scope for history by design, identical to any other finalized coin.Also pins, with regression tests, that
history_net == balance.total()is not a system invariant for pruned/finalized records under the default (keep-finalized-transactions = OFF) feature configuration — only balance is guaranteed correct there.Fixes a related latent bug found during investigation:
AddressPool::prune_unusedfailed to clear the correspondingscript_pubkey_indexentry when pruning an address (currently unreachable in production — no callers — but fixed before anything wires it up).Follow-up work from review (post-initial-submission, addressing findings from an independent review pass):
ManagedAccountRefMut::record_transaction/confirm_transactiontopub(crate)— these are internal reconciliation hooks, not part of the crate's public surface.key-wallet/src/test_utils/blocks.rsandkey-wallet-manager/tests/common/mod.rs.ManagedWalletInfo::observed_spent_outpoints, which the original design left as permanent/one-way (an unbounded-growth risk raised in review). Added finality-boundary pruning: an entry is safe to evict once its height is<= min(last_applied_chain_lock.block_height, synced_height), since ChainLock finalization durably short-circuits every resurrection path at that point. Naive LRU/age-based eviction was considered and rejected — it could silently reopen bug: out-of-order block processing causes SPV wallet to miss UTXO spends #649 for evicted entries; the finality boundary is the only eviction rule proven safe against the fix this PR makes.synced_heightacts as a certificate that "every filter ≤ h was matched against this wallet's script set," but that certificate is relative to the account set at scan time — adding an account after the fact silently grows the set without invalidating the certificate, letting a previously-pruned coin resurrect. Fixed by rewindingsynced_heighttobirth_height - 1on every account-addition path, plus adash-spvcommit-time contiguity guard so an in-flight filter batch can't clobber the rewind. Verified against multi-wallet, back-to-back-add, and boundary-exact adversarial scenarios.Testing
cargo test -p key-wallet -p key-wallet-manager -p dash-spv(default features): all passing, including new regression coverage for finality-boundary pruning and the account-addition rewind/contiguity fix.cargo test -p key-wallet -p key-wallet-manager --features key-wallet-manager/keep-finalized-transactions: passing.cargo clippy -p key-wallet -p key-wallet-manager -p dash-spv --all-features --all-targets -- -D warnings: clean.cargo +nightly fmt --check: clean.dash-spv,key-wallet-ffi) build clean against the changed internal signatures.Breaking changes
None — internal-only changes to
key-wallet/key-wallet-manager/dash-spv; no public API signature changes reach downstream crates.Follow-ups tracked separately (out of scope for this PR)
WalletManager::create_accountnever registers the account withManagedWalletInfo(pre-existing gap, found during QA; this PR's protections only engage for accounts added viaadd_managed_*)watched_outpointsin compact-filter matching as defense-in-depth for bug: out-of-order block processing causes SPV wallet to miss UTXO spends #649-class bugsChecklist
cargo fmt/cargo clippycleankeep-finalized-transactionsfeature configurations both covered locallyAttribution
🤖 Co-authored by Claudius the Magnificent AI Agent