fix(key-wallet): promote confirmed spend-only transactions - #998
Conversation
When mempool processing consumes a wallet UTXO, a later block replay can no longer rediscover a spend-only transaction from live relevance evidence. Promote the existing routed record by txid, preserve monotonic finality and retention behavior, and emit the normal update path. Run key-wallet-manager in both all-features and default-feature CI configurations so both finalized-record retention branches execute. Test would have caught this in CI: FAIL before fix, PASS after fix.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #998 +/- ##
==========================================
+ Coverage 77.12% 77.17% +0.04%
==========================================
Files 329 329
Lines 83483 83511 +28
==========================================
+ Hits 64388 64448 +60
+ Misses 19095 19063 -32
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe wallet checker now promotes stored mempool records when confirmed contexts arrive for irrelevant transactions. New tests cover AssetLock block promotion, chainlocks, replay protection, irrelevant transactions, and idempotence. CI adds a feature-disabled test run for ChangesSpend-only transaction promotion
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to This change promotes known spend-only transactions from Mempool to confirmed context, preventing affected wallet rows from remaining stuck. It is narrowly scoped and tested, but maintainers should ensure callers authenticate the supplied block or chainlock context before relying on this promotion path. 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}
|
Adversarial cross-model review (Codex gpt-5.6, xhigh; ground truth = the diagnostic report that proved the defect) — no merge-blocking findings. Verified at file:line:
The red→green diagnostic regression matches the field defect exactly (record stuck at |
HashEngineering
left a comment
HashEngineering
left a comment
There was a problem hiding this comment.
Approving — the diagnosis matches what we've measured downstream, and the fix is scoped tightly enough that I don't see a false-positive risk.
The relevance-starvation mechanism is right, and the decision to gate promotion on an existing unconfirmed record for the exact txid in a routed account is the part that makes this safe: compact-filter false positives can't manufacture a record, so the promotion path can only ever fire on transactions the wallet already recorded itself. The existing_txids / BlockProcessed.updated idempotency on confirmed replay is the right call too — downstream persistence upserts on those signals, so a duplicate emission would have been visible as churn.
The three transaction shapes we care about downstream, all of which are "spends wallet UTXOs, pays no wallet-owned output":
-
Asset-lock (type 8) credit top-ups. The case this PR reproduces. Downstream symptom is a balance that re-inflates on every app relaunch, because the consumed output is handed back as spendable — dashpay/platform#4336 documents a 0.1 DASH wallet reading 0.21.
-
CoinJoin collateral / fee transactions with an
OP_RETURN-only output. Zero value out, nothing owned on the output side. On a large testnet CoinJoin wallet (~7,320 transactions) we have four of these — e.g.f2fe8ad5aad30d6d0859a55db01b2c9240ee028eaff792516051f32c795377b7at height 1153176 — whose input coins remain unspent, overstating the balance by 0.00048773 on every launch. -
Ordinary sends with no change back to us — a drain, or an exact-amount payment. Same shape, no CoinJoin or platform involvement required. We haven't isolated one on this wallet, but nothing about the mechanism excludes it, and it's the case most likely to show up on a user wallet with a small UTXO set.
Where this PR lands for each. It fixes all three on the live path, where a mempool sighting created the record first. It does not reach them when the wallet meets the transaction through a restore or rescan — there was never a mempool sighting, so there is no record to promote and the transaction is dropped at the relevance gate without being recorded at all. Our four burns are exactly that: mined 2019 at heights 1153140–1194323, first seen by a restored wallet. Filed separately as #992 so it isn't confused with this PR's scope, and the PR body's "rows already stuck downstream still require the separate platform healing path" reads consistently with that.
One thing worth noting for whoever picks up the follow-up: platform#4336 is currently load-bearing for shape 1 only, because every repair it makes keys on the tracked asset lock's own status (InstantSendLocked / Consumed). Shapes 2 and 3 have no equivalent signal, so platform can't generalize its side — which is an argument for widening the engine-side fallback from "an existing record for this txid" to spent-coin history (#897 Half 1) rather than adding a second platform special case per transaction shape. Not a blocker for this PR.
3 participants
The actual issue
A wallet's own spend-only transaction — a Dash type-8 asset lock (and any tx that
spends wallet UTXOs while paying no wallet-owned output, e.g. a drain with no change)
— is recorded from its mempool sighting but never advances to InBlock, forever.
Downstream (dashpay/platform mobile wallets) every "spent" signal keys on in-block
context, so the consumed outputs keep reading as unspent: balances over-report and
re-inflate on every app relaunch, and the stale outputs are handed back as spendable
(field reports in dashpay/platform#4336).
Root cause — the transaction is starved of relevance by its own mempool sighting
(reproduced at the platform-pinned rev 4db5c36 with a real DIP-2 type-8
AssetLockPayload driven through the production path):
filters include every spent coin's scriptPubKey (dashpay/dash
src/blockfilter.cpp, BasicFilterElements; also verified empirically against a live
testnet filter). The filter/scan layer is NOT the problem.
(key-wallet/src/transaction_checking/wallet_checker.rs:54-61 ->
account_checker.rs:645-672). Outputs pay nothing owned, and the UTXO is already
gone — deleted in step 1. Fresh relevance is false, and processing early-returns
before ever looking up the existing txid record.
Relevance is true exactly once, and the first sighting consumes it — hence
mempool-yes / block-no on every affected transaction.
Summary
Own spend-only transactions now advance from mempool to block or chainlocked context even after the mempool sighting consumed their only live UTXO evidence. The fallback trusts only an existing txid record in router-selected accounts, so compact-filter false positives remain irrelevant and the normal new-transaction path stays unchanged.
This prevents new downstream wallet rows from remaining in
Mempoolforever. Rows already stuck downstream still require the separate platform healing path.Design
existing_txidsandBlockProcessed.updatedsignals once; confirmed replays are idempotent.key-wallet-managerCI with both all features and default features so both retention branches execute.Validation
test_block_promotes_known_mempool_asset_lock_spending_wallet_utxofailed before the engine change (existing_txidswas empty and the record stayedMempool) and passes after it.cargo test -p key-wallet-manager --all-features --quiet- pass.cargo test -p key-wallet-manager --quiet- pass.cargo test -p key-wallet --all-features --quiet- pass.cargo clippy -p key-wallet -p key-wallet-manager -p dash-spv --all-features --all-targets -- -D warnings- pass.cargo fmt --all --checkandgit diff --check- pass.key-wallet-managerwith all features and then default features; coverage remains all-features-only.cargo test -p dash-spv --all-features- Rust unit suites passed (570 + 7 tests; 2 project-marked ignored). The 10 real-dashdintegration cases could not start locally because macOS killed the downloaded unsigneddashdbinary with exit 137 before startup.Post-Deploy Monitoring & Validation
existing_txidsandBlockProcessed.updatedwhen its block is processed, withInBlockorInChainLockedBlockcontext.Mempool, a confirmed replay emits duplicate updates, or an unknown irrelevant transaction is promoted.devintegration run and the first affected wallet replay.Related
Related: dashpay/platform#4336
Related: #948 (separate CoinJoin scan-query pruning path; out of scope)
Summary by CodeRabbit
Bug Fixes
Tests