fix(key-wallet): emit the removals the islock and abandon paths already performed

dash-spv-ffi/src/callbacks.rs

@@ -776,18 +776,26 @@ impl FFIOutPoint {
 
 /// Callback for `WalletEvent::TransactionsSwept`.
 ///
-/// Fires when the wallet removes transactions that a later, final transaction
-/// provably beat to one of their inputs: they can never confirm, so their
-/// outputs are gone from the UTXO set and their records deleted.
+/// Fires when the wallet removes transactions that can never confirm — either
+/// because a later, final transaction provably beat them to one of their
+/// inputs, or because the root was explicitly abandoned as never having
+/// reached the network and the removal cascaded to everything built on its
+/// change. Their outputs are gone from the UTXO set and their records deleted.
 ///
 /// **The only removal-shaped wallet callback.** Every other one is additive,
 /// so a consumer mirroring wallet state to disk must act on this — delete the
 /// named transactions and any UTXO they created. Ignoring it leaves the dead
 /// rows in the mirror, which replays them on the next load and re-creates a
 /// balance the wallet has already corrected.
 ///
-/// `txids` points to `txids_count` consecutive 32-byte txids.
-/// `superseded_by` is the transaction whose arrival settled the inputs.
+/// `txids` points to `txids_count` consecutive 32-byte txids. On the abandon
+/// path this is what the abandon asked for, which may include a txid the
+/// wallet held no record for — a mirror can hold rows the load path never
+/// restored, so the delete is driven by the requested set.
+/// `superseded_by` is the transaction whose arrival settled the inputs. On the
+/// abandon path no such transaction exists and this repeats the abandoned
+/// root, which therefore also appears in `txids`; never assume it names a row
+/// that survives.
 /// `released_outpoints` points to `released_outpoints_count` outpoints freed
 /// by the removal: inputs the removed transactions claimed to spend that no
 /// surviving record spends too. Mark these coins spendable again. This is

key-wallet-manager/src/event_tests.rs

@@ -1163,6 +1163,222 @@ async fn test_instant_send_lock_event_does_not_carry_addresses_derived_field() {
     }
 }
 
+/// An InstantSend lock settles the locked transaction's inputs, so it beats a
+/// recorded competing spend exactly as a block does — and the removal has to
+/// reach consumers.
+///
+/// This is the live ordering for the ordinary case: dash-spv routes a lock
+/// arriving for an already-tracked mempool transaction straight to
+/// `process_instant_send_lock`, which is a different code path from the one a
+/// first sighting that already carries its lock takes. That path emitted only
+/// the additive `TransactionInstantLocked` while the sweep quietly deleted the
+/// loser's record and freed its coins, so a mirror kept the dead row, replayed
+/// it on the next load, and left the released coin marked spent forever.
+#[tokio::test]
+async fn test_instant_send_lock_emits_swept_event_before_the_lock_event() {
+    let (mut manager, wallet_id, addr) = setup_manager_with_wallet();
+
+    // One funding transaction pays us twice, so the loser can spend a coin
+    // the winner does not — that second coin is what gets released.
+    let funding = Transaction {
+        version: 2,
+        lock_time: 0,
+        input: vec![TxIn {
+            previous_output: OutPoint {
+                txid: Txid::from_byte_array([0x7a; 32]),
+                vout: 0,
+            },
+            script_sig: ScriptBuf::new(),
+            sequence: u32::MAX,
+            witness: Witness::default(),
+        }],
+        output: vec![
+            TxOut {
+                value: 500_000,
+                script_pubkey: addr.script_pubkey(),
+            },
+            TxOut {
+                value: 400_000,
+                script_pubkey: addr.script_pubkey(),
+            },
+        ],
+        special_transaction_payload: None,
+    };
+    let funding_block = make_block(vec![funding.clone()], 0x7a, 2000);
+    let wallets = BTreeSet::from([wallet_id]);
+    manager
+        .process_block_for_wallets(&funding_block, funding_block.block_hash(), 200, &wallets)
+        .await;
+
+    let coin_a = OutPoint {
+        txid: funding.txid(),
+        vout: 0,
+    };
+    let coin_b = OutPoint {
+        txid: funding.txid(),
+        vout: 1,
+    };
+    let spend = |inputs: Vec<OutPoint>, value: u64| Transaction {
+        version: 2,
+        lock_time: 0,
+        input: inputs
+            .into_iter()
+            .map(|previous_output| TxIn {
+                previous_output,
+                script_sig: ScriptBuf::new(),
+                sequence: u32::MAX,
+                witness: Witness::default(),
+            })
+            .collect(),
+        output: vec![TxOut {
+            value,
+            script_pubkey: addr.script_pubkey(),
+        }],
+        special_transaction_payload: None,
+    };
+
+    // Both competing spends sit in the mempool, neither final, so neither
+    // sweeps the other yet.
+    let loser = spend(vec![coin_a, coin_b], 880_000);
+    manager.process_mempool_transaction(&loser, None).await;
+    let winner = spend(vec![coin_a], 480_000);
+    manager.process_mempool_transaction(&winner, None).await;
+
+    // Subscribe only now: the setup above is not what this test is about.
+    let mut rx = manager.subscribe_events();
+
+    // The lock arrives for the already-tracked winner — the transition that
+    // goes through `process_instant_send_lock`, not through the checker.
+    let lock = InstantLock {
+        txid: winner.txid(),
+        cyclehash: CycleHash::from_byte_array([0x1a; 32]),
+        signature: BLSSignature::from([0x2b; 96]),
+        ..InstantLock::default()
+    };
+    manager.process_instant_send_lock(lock);
+
+    let events = drain_events(&mut rx);
+    let swept_index = events
+        .iter()
+        .position(|event| matches!(event, WalletEvent::TransactionsSwept { .. }))
+        .unwrap_or_else(|| {
+            panic!("the lock swept a conflict, so a removal must be emitted, got {:?}", events)
+        });
+    let locked_index = events
+        .iter()
+        .position(|event| matches!(event, WalletEvent::TransactionInstantLocked { .. }))
+        .unwrap_or_else(|| panic!("expected TransactionInstantLocked, got {:?}", events));
+    // Removal before the additive event, as on the block and mempool paths: a
+    // consumer applying them in order must not have a delete land on top of
+    // something that came after it.
+    assert!(
+        swept_index < locked_index,
+        "the removal must precede the lock event, got {:?}",
+        events
+    );
+
+    match &events[swept_index] {
+        WalletEvent::TransactionsSwept {
+            wallet_id: wid,
+            txids,
+            superseded_by,
+            released_outpoints,
+            ..
+        } => {
+            assert_eq!(*wid, wallet_id);
+            assert_eq!(txids, &vec![loser.txid()], "the beaten transaction is named");
+            assert_eq!(
+                *superseded_by,
+                winner.txid(),
+                "attributed to the transaction the lock settled"
+            );
+            assert_eq!(
+                released_outpoints,
+                &vec![coin_b],
+                "only the coin the winner did not take is released"
+            );
+        }
+        _ => unreachable!(),
+    }
+}
+
+/// Abandoning a transaction is a removal like any other, and consumers learn
+/// about removals from exactly one event. Without it a mirror keeps rows for
+/// transactions this wallet has decided never existed and replays them on the
+/// next load, re-creating the phantom balance the abandon just removed.
+#[tokio::test]
+async fn test_abandon_emits_swept_event_with_released_outpoints() {
+    let (mut manager, wallet_id, addr) = setup_manager_with_wallet();
+
+    // A real, confirmed coin.
+    let funding = create_tx_paying_to(&addr, 0x8a);
+    let funding_block = make_block(vec![funding.clone()], 0x8a, 2100);
+    let wallets = BTreeSet::from([wallet_id]);
+    manager
+        .process_block_for_wallets(&funding_block, funding_block.block_hash(), 300, &wallets)
+        .await;
+    let funding_outpoint = OutPoint {
+        txid: funding.txid(),
+        vout: 0,
+    };
+
+    // A spend of it that never reached the network. Its change is credited
+    // and, as a trusted self-send, counted confirmed and spendable.
+    let dead = Transaction {
+        version: 2,
+        lock_time: 0,
+        input: vec![TxIn {
+            previous_output: funding_outpoint,
+            script_sig: ScriptBuf::new(),
+            sequence: u32::MAX,
+            witness: Witness::default(),
+        }],
+        output: vec![TxOut {
+            value: TX_AMOUNT - 1_000,
+            script_pubkey: addr.script_pubkey(),
+        }],
+        special_transaction_payload: None,
+    };
+    manager.process_mempool_transaction(&dead, None).await;
+
+    let mut rx = manager.subscribe_events();
+    let outcome = manager
+        .abandon_transaction(&wallet_id, dead.txid(), &BTreeMap::new())
+        .expect("the wallet exists");
+    assert!(outcome.abandoned.contains(&dead.txid()));
+
+    let events = drain_events(&mut rx);
+    let swept = events
+        .iter()
+        .find_map(|event| match event {
+            WalletEvent::TransactionsSwept {
+                wallet_id: wid,
+                txids,
+                superseded_by,
+                released_outpoints,
+                balance,
+                ..
+            } => Some((wid, txids, superseded_by, released_outpoints, balance)),
+            _ => None,
+        })
+        .unwrap_or_else(|| panic!("an abandon must emit a removal, got {:?}", events));
+
+    assert_eq!(swept.0, &wallet_id);
+    assert_eq!(swept.1, &vec![dead.txid()], "the abandoned transaction is named");
+    // No competing transaction exists — that is what an abandon is — so the
+    // root stands in as its own provenance.
+    assert_eq!(swept.2, &dead.txid());
+    assert_eq!(
+        swept.3,
+        &vec![funding_outpoint],
+        "the coin the dead transaction claimed comes free"
+    );
+    // And the balance carried is the post-abandon one: the phantom change is
+    // gone, the real coin is back.
+    assert_eq!(swept.4.confirmed(), TX_AMOUNT);
+    assert_eq!(swept.4.spendable(), TX_AMOUNT);
+}
+
 // ---------------------------------------------------------------------------
 // ChainLock path
 // ---------------------------------------------------------------------------

key-wallet-manager/src/events.rs

@@ -222,10 +222,17 @@ pub enum WalletEvent {
         /// full balance after the change — not a delta.
         account_balances: BTreeMap<AccountType, WalletCoreBalance>,
     },
-    /// Transactions were removed from the wallet: each was a recorded spend
-    /// that a later, final transaction provably beat to one of its inputs, so
-    /// it can never confirm. Their outputs are gone from the UTXO set and
-    /// their records deleted.
+    /// Transactions were removed from the wallet because they can never
+    /// confirm. Their outputs are gone from the UTXO set and their records
+    /// deleted. Two paths produce this:
+    ///
+    /// * **Swept** — a later, final transaction (in a block, or InstantSend
+    ///   locked) provably beat them to one of their inputs.
+    /// * **Abandoned** — the caller asserted the root never reached the
+    ///   network, via [`crate::WalletManager::abandon_transaction`], and the
+    ///   removal cascaded to everything built on its change. There is no
+    ///   competing transaction in this case, by construction: it is the one
+    ///   the sweep cannot reach.
     ///
     /// The only removal-shaped event on this bus. A consumer mirroring wallet
     /// state to disk must act on it — every other variant is additive, so
@@ -235,8 +242,22 @@ pub enum WalletEvent {
         /// ID of the affected wallet.
         wallet_id: WalletId,
         /// Transactions removed. Delete these rows and any UTXO they created.
+        ///
+        /// On the abandon path this is everything the abandon *asked* for,
+        /// which can include a txid the in-memory wallet held no record for.
+        /// That is deliberate: a mirror can hold rows the load path never
+        /// restored — that asymmetry is exactly why
+        /// `abandon_transaction_with_spends` takes an external spend view —
+        /// so the delete has to be driven by the requested set, not by what
+        /// happened to be in memory.
         txids: Vec<Txid>,
         /// The transaction whose arrival settled the inputs, for provenance.
+        ///
+        /// On the abandon path there is no such transaction, and this carries
+        /// the abandoned root itself — which therefore also appears in
+        /// `txids`. Consumers must not assume it names a row that survives, or
+        /// indeed any row at all: on the sweep path it need not be
+        /// wallet-relevant either (see `released_outpoints`).
         superseded_by: Txid,
         /// Outpoints the sweep released: inputs the removed transactions
         /// claimed to spend that no surviving record spends too (a loser
@@ -271,6 +292,30 @@ pub enum WalletEvent {
         /// reported released though it is spent on chain. The inputs of a
         /// pruned record survive nowhere else, so this cannot be resolved at
         /// this layer.
+        ///
+        /// The wallet re-credits these coins to its own UTXO set as it emits
+        /// them, so "spendable again" means the same thing on both sides of
+        /// this event wherever the re-credit can be proven safe. Where it
+        /// cannot, the coin is named here but deliberately withheld from this
+        /// library's own coin selection until a rescan re-delivers its funding
+        /// transaction:
+        ///
+        /// * the funding transaction was pruned to its txid by a chainlock, so
+        ///   no `TxOut` survives to rebuild the coin from;
+        /// * the funding transaction can never confirm, because a block
+        ///   already spent one of *its* inputs — the credit path refuses such
+        ///   outputs too, and the re-credit must not disagree with it;
+        /// * the coin's own spent-status is no longer verifiable, because the
+        ///   observed-spent map that records block spends is pruned at the
+        ///   finality boundary and this coin's funding record sits at or below
+        ///   it. Absence from a pruned map is not evidence a coin is unspent.
+        ///
+        /// Consumers are unaffected — their own record of the coin is what
+        /// they restore, and this set is reported in full in every case — but
+        /// a caller reading this library's balance back will see the
+        /// difference. The divergence is one-directional by construction: core
+        /// holds at most what the event reports, never more. See
+        /// `ManagedCoreFundsAccount::recredit_released_outpoints`.
         released_outpoints: Vec<OutPoint>,
         /// Wallet balance after the removal.
         balance: WalletCoreBalance,

key-wallet-manager/src/lib.rs

@@ -718,25 +718,56 @@ impl WalletManager<ManagedWalletInfo> {
     /// are refused, but the judgement otherwise belongs to the caller that
     /// owns broadcast policy.
     ///
+    /// Emits [`WalletEvent::TransactionsSwept`] naming the abandoned
+    /// transactions and the coins their removal freed, so a consumer mirroring
+    /// wallet state deletes the same rows. That is the only removal-shaped
+    /// event on the bus; without it the mirror keeps records for transactions
+    /// this wallet has decided never existed and replays them on its next
+    /// load, re-creating the phantom balance the abandon just removed.
+    /// `superseded_by` carries `root` — an abandon has no competing
+    /// transaction to attribute the removal to, which is precisely why it
+    /// exists (see the field's documentation).
+    ///
     /// Returns `None` when the wallet is unknown.
     pub fn abandon_transaction(
         &mut self,
         wallet_id: &WalletId,
         root: Txid,
         external_spends: &BTreeMap<OutPoint, Txid>,
     ) -> Option<AbandonOutcome> {
+        // Snapshot before the abandon so the event can carry the diff: the
+        // cached per-account balances are still the pre-abandon ones here.
+        let prior = self.wallet_infos.get(wallet_id)?.account_balances();
+
         let info = self.get_wallet_info_mut(wallet_id)?;
         let outcome = info.abandon_transaction_with_spends(root, external_spends);
-        if !outcome.is_empty() {
-            info.update_balance();
-            tracing::info!(
-                %root,
-                abandoned = outcome.abandoned.len(),
-                records_removed = outcome.records_removed,
-                utxos_removed = outcome.utxos_removed,
-                "Abandoned a dead transaction and everything built on it"
-            );
+        if outcome.is_empty() {
+            return Some(outcome);
         }
+        info.update_balance();
+        tracing::info!(
+            %root,
+            abandoned = outcome.abandoned.len(),
+            records_removed = outcome.records_removed,
+            utxos_removed = outcome.utxos_removed,
+            released_outpoints = outcome.released_outpoints.len(),
+            "Abandoned a dead transaction and everything built on it"
+        );
+
+        // Read the post-abandon balances out while the borrow is still live,
+        // so the emit below needs no second lookup that could fail after the
+        // wallet has already been mutated.
+        let balance = info.balance();
+        let account_balances = events::diff_account_balances(&prior, &info.account_balances());
+        self.emit_event(WalletEvent::TransactionsSwept {
+            wallet_id: *wallet_id,
+            txids: outcome.abandoned.iter().copied().collect(),
+            superseded_by: root,
+            released_outpoints: outcome.released_outpoints.clone(),
+            balance,
+            account_balances,
+        });
+
         Some(outcome)
     }