diff --git a/dash-spv-ffi/src/callbacks.rs b/dash-spv-ffi/src/callbacks.rs index 7a2abf355..bb01d1d05 100644 --- a/dash-spv-ffi/src/callbacks.rs +++ b/dash-spv-ffi/src/callbacks.rs @@ -797,6 +797,14 @@ impl FFIOutPoint { /// be wallet-relevant at all (it can spend our coin while paying only /// external addresses), so it may never appear in any other callback. Null /// with a zero count when the removal released nothing. +/// +/// The Rust `WalletEvent::TransactionsSwept` additionally carries +/// `winner_mined_height` (the winner's block height, `None` for an +/// InstantSend-locked winner not yet mined). It is deliberately not +/// forwarded here: this signature has no safely-degrading insertion point +/// for hand-declared C consumers (see the released-outpoints addition), +/// so growing it is a flagged breaking change deferred until a C consumer +/// needs the field. /// All pointer parameters are borrowed and only valid for the duration of the /// callback. `balance` is the wallet's balance *after* the removal; /// `account_balances` follows the same contract as on @@ -1091,6 +1099,16 @@ impl FFIWalletEventCallbacks { wallet_id, txids, superseded_by, + // Deliberately not forwarded across the C ABI. #962 + // established that `OnTransactionsSweptCallback` has no + // safely-degrading insertion point: a C consumer declaring + // the function pointer by hand keeps compiling against a + // grown parameter list and reads shifted arguments. Rust + // consumers of `WalletEvent` get the field directly; the C + // surface stays byte-for-byte identical until a consumer + // needs it there, at which point the addition must be a + // flagged breaking change like #962's. + winner_mined_height: _, released_outpoints, balance, account_balances, @@ -1398,6 +1416,7 @@ mod tests { wallet_id: [7u8; 32], txids: vec![Txid::from_byte_array([1u8; 32])], superseded_by: Txid::from_byte_array([2u8; 32]), + winner_mined_height: Some(1_000), released_outpoints: Vec::new(), balance: WalletCoreBalance::default(), account_balances: BTreeMap::new(), @@ -1445,6 +1464,7 @@ mod tests { wallet_id: [7u8; 32], txids: vec![Txid::from_byte_array([1u8; 32])], superseded_by: Txid::from_byte_array([2u8; 32]), + winner_mined_height: None, released_outpoints: vec![ dashcore::OutPoint { txid: parent, diff --git a/key-wallet-manager/src/event_tests.rs b/key-wallet-manager/src/event_tests.rs index 40628c567..915fe44d9 100644 --- a/key-wallet-manager/src/event_tests.rs +++ b/key-wallet-manager/src/event_tests.rs @@ -625,9 +625,10 @@ async fn test_block_winner_emits_swept_event_naming_the_released_outpoints() { wallet_id: wid, txids, superseded_by, + winner_mined_height, released_outpoints, .. - } => Some((wid, txids, superseded_by, released_outpoints)), + } => Some((wid, txids, superseded_by, winner_mined_height, released_outpoints)), _ => None, }) .unwrap_or_else(|| panic!("a sweep must be emitted, got {:?}", events)); @@ -635,7 +636,122 @@ async fn test_block_winner_emits_swept_event_naming_the_released_outpoints() { assert_eq!(swept.0, &wallet_id); assert_eq!(swept.1, &vec![loser.txid()], "the beaten transaction is named"); assert_eq!(swept.2, &winner.txid(), "attributed to the transaction that beat it"); - assert_eq!(swept.3, &vec![coin_b], "only the coin the winner did not take is released"); + assert_eq!( + swept.3, + &Some(101), + "a block-context sweep must carry the winner's mined height — the height of \ + the block whose processing triggered the sweep, not None and not any other \ + height the manager has seen" + ); + assert_eq!(swept.4, &vec![coin_b], "only the coin the winner did not take is released"); +} + +/// The mempool emission site's counterpart to the block test above, pinning +/// the other half of `winner_mined_height`'s contract: a sweep triggered by +/// an InstantSend-locked winner that has not been mined carries `None`. This +/// is the distinction the field exists to make — a consumer retiring durable +/// records of the swept spends can anchor a block-context sweep to the +/// winner's height, while an IS-locked winner has no mining deadline, so +/// conflating the two (a height where there is none, or vice versa) would +/// let those records be retired while the conflict is still unmined. +#[tokio::test] +async fn test_is_locked_mempool_winner_sweeps_with_no_mined_height() { + let (mut manager, wallet_id, addr) = setup_manager_with_wallet(); + + // Same shape as the block test: one funding transaction pays us twice so + // the loser spends a coin the winner does not. + let funding = Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: OutPoint { + txid: Txid::from_byte_array([0x6a; 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()], 0x6a, 2000); + let wallets = BTreeSet::from([wallet_id]); + manager + .process_block_for_wallets(&funding_block, funding_block.block_hash(), 100, &wallets) + .await; + + let coin_a = OutPoint { + txid: funding.txid(), + vout: 0, + }; + let coin_b = OutPoint { + txid: funding.txid(), + vout: 1, + }; + let spend = |inputs: Vec, 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, + }; + + let loser = spend(vec![coin_a, coin_b], 800_000); + manager.process_mempool_transaction(&loser, None).await; + + let mut rx = manager.subscribe_events(); + + // The winner arrives off-chain with an InstantSend lock — final enough + // to sweep the loser, but not mined anywhere. + let winner = spend(vec![coin_a], 400_000); + manager.process_mempool_transaction(&winner, Some(dummy_instant_lock(winner.txid()))).await; + + let events = drain_events(&mut rx); + let swept = events + .iter() + .find_map(|event| match event { + WalletEvent::TransactionsSwept { + wallet_id: wid, + txids, + superseded_by, + winner_mined_height, + released_outpoints, + .. + } => Some((wid, txids, superseded_by, winner_mined_height, released_outpoints)), + _ => None, + }) + .unwrap_or_else(|| panic!("a sweep must be emitted, got {:?}", events)); + + assert_eq!(swept.0, &wallet_id); + assert_eq!(swept.1, &vec![loser.txid()], "the beaten transaction is named"); + assert_eq!(swept.2, &winner.txid(), "attributed to the transaction that beat it"); + assert_eq!( + swept.3, &None, + "an IS-locked winner is not mined: fabricating a height here would give the \ + consumer a finality horizon the winner does not have" + ); + assert_eq!(swept.4, &vec![coin_b], "only the coin the winner did not take is released"); } #[tokio::test] diff --git a/key-wallet-manager/src/events.rs b/key-wallet-manager/src/events.rs index 8bb336ac9..e044f0796 100644 --- a/key-wallet-manager/src/events.rs +++ b/key-wallet-manager/src/events.rs @@ -238,6 +238,25 @@ pub enum WalletEvent { txids: Vec, /// The transaction whose arrival settled the inputs, for provenance. superseded_by: Txid, + /// Mined height of `superseded_by` when this sweep was triggered by + /// its arrival in a block; `None` when it was triggered by an + /// InstantSend-locked transaction still waiting to be mined (those + /// are the only two triggers — an unlocked mempool arrival never + /// sweeps, see `WalletTransactionChecker::check_core_transaction`). + /// + /// This is the winner's finality context, and only the emission site + /// has it: `superseded_by` need not be wallet-relevant (see + /// `released_outpoints` below), so a consumer cannot look the height + /// up in its own records — the winner may never appear anywhere else + /// in this wallet's event stream. A consumer durably mirroring the + /// removed spends (e.g. observed-spent rows kept so a restored + /// wallet does not re-credit the swept coins) needs it to retire + /// those rows soundly: a block-context sweep anchors the winner at a + /// height that chainlocks, giving the rows a finality horizon, while + /// an IS-locked winner has no mining deadline — aging its rows out + /// by wall clock would delete a genuine hold while the conflict is + /// still unmined. + winner_mined_height: Option, /// Outpoints the sweep released: inputs the removed transactions /// claimed to spend that no surviving record spends too (a loser /// spending A+B against a winner spending only A leaves A marked and @@ -446,15 +465,17 @@ impl fmt::Display for WalletEvent { WalletEvent::TransactionsSwept { txids, superseded_by, + winner_mined_height, released_outpoints, balance, account_balances, .. } => write!( f, - "TransactionsSwept(count={}, superseded_by={}, released={}, balance={}, account_balances={})", + "TransactionsSwept(count={}, superseded_by={}, winner_mined_height={:?}, released={}, balance={}, account_balances={})", txids.len(), superseded_by, + winner_mined_height, released_outpoints.len(), balance, format_account_balances(account_balances), @@ -506,6 +527,44 @@ impl fmt::Display for WalletEvent { } } +#[cfg(test)] +mod display_tests { + use super::*; + use std::collections::BTreeMap; + + fn swept(winner_mined_height: Option) -> WalletEvent { + WalletEvent::TransactionsSwept { + wallet_id: WalletId::from([7u8; 32]), + txids: vec![Txid::from_raw_hash(dashcore::hashes::Hash::from_byte_array([1u8; 32]))], + superseded_by: Txid::from_raw_hash(dashcore::hashes::Hash::from_byte_array([2u8; 32])), + winner_mined_height, + released_outpoints: Vec::new(), + balance: WalletCoreBalance::default(), + account_balances: BTreeMap::new(), + } + } + + /// The winner's finality context is the first thing a reader of these + /// logs needs when a swept coin misbehaves, so `Display` must + /// distinguish the two triggers rather than printing one shape for + /// both. Both legs are asserted together: a formatter that dropped the + /// field would satisfy neither. + #[test] + fn transactions_swept_display_reports_the_winners_finality_context() { + let mined = format!("{}", swept(Some(1_000))); + assert!( + mined.contains("winner_mined_height=Some(1000)"), + "a block-triggered sweep must report the winner's height: {mined}" + ); + + let unmined = format!("{}", swept(None)); + assert!( + unmined.contains("winner_mined_height=None"), + "an InstantSend-triggered sweep must report that no height exists yet: {unmined}" + ); + } +} + #[cfg(test)] mod project_derived_addresses_tests { use super::*; diff --git a/key-wallet-manager/src/process_block.rs b/key-wallet-manager/src/process_block.rs index 1520b60da..1e3322e47 100644 --- a/key-wallet-manager/src/process_block.rs +++ b/key-wallet-manager/src/process_block.rs @@ -101,6 +101,9 @@ impl WalletInterface for WalletM wallet_id, txids, superseded_by: tx.txid(), + // The winner is a transaction of this very block, so its + // mined height is the block's height. + winner_mined_height: Some(height), released_outpoints, balance: info.balance(), account_balances: BTreeMap::new(), @@ -231,6 +234,10 @@ impl WalletInterface for WalletM wallet_id, txids, superseded_by: tx.txid(), + // Off-chain arrival: only an InstantSend-locked winner sweeps + // from this path (an unlocked mempool arrival never does), + // and an IS-locked winner is by definition not mined yet. + winner_mined_height: None, released_outpoints, balance: info.balance(), account_balances: per_wallet_account_diff