fix(spv): apply InstantSend locks to self-broadcast transactions#815
Conversation
Self-broadcast transactions bypass the MempoolManager (fed directly to WalletManager via notify_wallet_after_broadcast). When the IS lock arrives from the network, the MempoolManager doesn't know about the tx, so it stores the lock as "pending" — never matched, never applied. Result: the tx stays unconfirmed and balance.spendable() returns 0. Fix: in SpvEventHandler::on_sync_event(InstantLockReceived), apply the IS lock directly on the WalletManager via process_instant_send_lock(). For MempoolManager-tracked txs this is a harmless no-op — the WalletManager deduplicates via its instant_send_locks HashSet. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ 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 |
Both notify_wallet_after_broadcast and the EventHandler IS lock workaround exist because upstream broadcast doesn't call handle_tx on the MempoolManager. Added TODO linking them so they can be removed together when the upstream fix lands. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a gap in SPV balance finality handling where InstantSend (IS) locks weren’t being applied to self-broadcast transactions (which bypass the MempoolManager via notify_wallet_after_broadcast()), causing outgoing funds to remain “unconfirmed” until mined.
Changes:
- Add a
WalletManagerreference toSpvEventHandler. - On
SyncEvent::InstantLockReceived, apply the IS lock directly to theWalletManagervia an async task.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let wallet = Arc::clone(&self.wallet); | ||
| tokio::spawn(async move { | ||
| let mut wm = wallet.write().await; | ||
| wm.process_instant_send_lock(txid); | ||
| }); |
There was a problem hiding this comment.
The InstantLockReceived handler applies the lock in a detached task, but the reconcile signal for this event is sent immediately afterward (outside the spawned task). If the wallet write lock is contended, reconcile_spv_wallets() can run before process_instant_send_lock() executes, leaving balances unchanged until some later unrelated reconcile trigger. Consider sending an additional reconcile signal after the lock is applied (e.g., clone reconcile_tx into the task and try_send once process_instant_send_lock returns), or otherwise ensure reconcile cannot run before the lock application completes.
| let wallet = Arc::clone(&self.wallet); | |
| tokio::spawn(async move { | |
| let mut wm = wallet.write().await; | |
| wm.process_instant_send_lock(txid); | |
| }); | |
| let mut wm = self.wallet.write().await; | |
| wm.process_instant_send_lock(txid); |
| // TODO(workaround): Remove once dashpay/rust-dashcore#487 is fixed. | ||
| // | ||
| // Apply InstantSend locks directly on the WalletManager. | ||
| // | ||
| // Self-broadcast transactions bypass the MempoolManager (they are fed | ||
| // directly to WalletManager via notify_wallet_after_broadcast — see | ||
| // the other workaround in spawn_request_handler). When the IS lock | ||
| // arrives from the network, the MempoolManager doesn't know about | ||
| // the tx and stores it as a "pending IS lock" that is never matched. | ||
| // Applying the lock here ensures self-broadcast txs transition from | ||
| // unconfirmed to spendable. | ||
| // | ||
| // Once upstream broadcast calls handle_tx() on the MempoolManager, | ||
| // both workarounds (notify_wallet_after_broadcast and this) can be | ||
| // removed — the normal MempoolManager pipeline will handle everything. | ||
| // | ||
| // For MempoolManager-tracked txs this is a harmless no-op — the | ||
| // WalletManager deduplicates via its instant_send_locks HashSet. | ||
| if let SyncEvent::InstantLockReceived { instant_lock, .. } = event { | ||
| let txid = instant_lock.txid; | ||
| let wallet = Arc::clone(&self.wallet); | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
This adds important new behavior (applying InstantSend locks to self-broadcast transactions) but there’s no regression test coverage in src/spv/tests.rs for InstantLockReceived or for the notify_wallet_after_broadcast path. Adding a focused test that simulates a broadcast-notified tx and then an InstantLockReceived event (asserting spendable/unconfirmed transitions) would help prevent this from silently regressing.
With 14+ orphaned wallets from previous runs, the 10s per-wallet spendable balance wait added 2+ minutes to test startup. Most orphaned wallets have 0 spendable balance anyway (IS locks never arrived), so the wait is wasted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Testing Progress & AnalysisIS Lock Fix ValidationThe EventHandler IS lock path works correctly. Trace logs confirm:
Test Results (parallel run with trace logging)
Root Cause of Remaining FailuresThe testnet only produced IS locks for 2 of 6 broadcast transactions. The other 4 never received
Related Issues Found During Investigation
Additional Fix in This Branch
Resumption NotesTo continue work on this PR:
🤖 Co-authored by Claudius the Magnificent AI Agent |
The init sequence waited 180s for spendable balance BEFORE waiting for SPV to reach Running state. Wallet balances are only available after compact filter sync completes, so the balance check always timed out on the first attempt, wasting 3+ minutes per retry. Swapped the order: wait for SPV Running first (up to 300s), then check spendable balance (30s — should be near-instant after sync). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
send_wallet_payment_via_spv() built and signed the transaction under the WalletManager write lock, then dropped the lock before broadcasting. Concurrent callers could select the same UTXOs, creating double-spend transactions that the network rejects (no IS lock issued for the conflicting tx). Now calls process_mempool_transaction() while still holding the write lock, so spent UTXOs are immediately marked and unavailable to concurrent callers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update: 7/8 E2E tests passing, 4x fasterAfter all fixes in this PR, parallel E2E test results improved dramatically:
Fixes in this PR
Remaining failure
🤖 Co-authored by Claudius the Magnificent AI Agent |
tx_is_ours test sends from wallet A to wallet B, but B's bloom filter may not have propagated to peers yet. Peers don't relay the tx back through B's filter, so B never sees it. Adding a 2s delay after wallet creation gives the bloom filter time to reach peers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
10-Run Stability AnalysisRan E2E tests 10 times sequentially (run 1 with wiped SPV data, runs 2-10 reusing existing data). Results
FindingsStable (runs 2-5): 7/8 consistently. Only
Runtime degradation: 309s → 366s → 453s → 554s → 928s across runs 2-6. Caused by orphaned test wallets accumulating in the persistent DB. Each run adds ~6 wallets that cleanup can't sweep (0 spendable balance). By run 6, ~30 orphaned wallets are loaded into SPV on every init, each with ~185 monitored addresses. The reconciliation loop clones Fresh SPV wipe (run 1): 3 extra failures from cold compact filter sync. Expected — initial sync takes longer, some tests time out. Fixes in this PR (cumulative)
Open issues
Resumption notes
🤖 Co-authored by Claudius the Magnificent AI Agent |
Orphaned test wallets with 0 total balance were skipped during cleanup and accumulated across runs (~10MB + 185 monitored addresses each). By run 6, ~30 orphaned wallets caused reconciliation to saturate all 12 CPU cores (987% CPU, 928s runtime). Now deletes wallets with 0 total balance via remove_wallet(). Only wallets with unconfirmed-but-unspendable funds are kept for future cleanup attempts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Final Validation: 2 Runs with Clean SlateWiped SPV data + DB, ran tests twice (run 1 fresh sync, run 2 reusing SPV data). Results
What's fixed
What's still flaky
All commits in this PR
🤖 Co-authored by Claudius the Magnificent AI Agent |
Review GateCommit:
|
Summary
Imagine you send Dash from within the app. The transaction broadcasts successfully and your balance shows the outgoing amount as "unconfirmed". On the Dash network, InstantSend locks this transaction within seconds — but the app never notices. Your funds stay stuck as "unconfirmed" indefinitely, and the receiving wallet can't spend them until a block is mined (~2.5 minutes).
Now IS locks are applied immediately, making funds spendable within seconds of broadcast.
Root cause
Self-broadcast transactions follow a different path than transactions received from peers:
The
notify_wallet_after_broadcast()workaround (for upstream rust-dashcore#487) feeds the tx directly toWalletManager::process_mempool_transaction(), bypassing theMempoolManager. When the IS lock arrives, theMempoolManagerdoesn't know about the tx, so the lock is stored as "pending" and never applied.Evidence from trace logs: broadcast txids
a867b813,4363a2d0,55344098,f03aec1awere notified to the wallet but never appeared as "Marked mempool tx" — their IS locks were silently dropped.Fix
Add a
walletreference toSpvEventHandlerand apply IS locks directly on theWalletManagerwhenInstantLockReceivedfires:For
MempoolManager-tracked txs this is a harmless no-op — theWalletManagerdeduplicates via itsinstant_send_locksHashSet.Test plan
cargo clippy --all-features --all-targets -- -D warningspasseswait_for_spendable_balancesucceeds without waiting for block confirmationspendable()reflects locked amount within seconds🤖 Co-authored by Claudius the Magnificent AI Agent