diff --git a/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs b/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs index 24d54bb0776..c96cfcf5851 100644 --- a/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs +++ b/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs @@ -70,17 +70,32 @@ pub unsafe extern "C" fn core_wallet_send_to_addresses( check_ptr!(out_tx_bytes); check_ptr!(out_tx_len); - let mut outputs = Vec::with_capacity(count); - let addr_ptrs = std::slice::from_raw_parts(addresses, count); - let amount_slice = std::slice::from_raw_parts(amounts, count); + // `std::slice::from_raw_parts` requires a non-null, properly + // aligned pointer even for `len == 0`. Swift's empty + // `Array.withUnsafeBufferPointer.baseAddress` returns `nil`, so + // the `count == 0` path is allowed to ship null `addresses` / + // `amounts` — guard against constructing the slice in that case. + let outputs: Vec<(dashcore::Address, u64)> = if count == 0 { + Vec::new() + } else { + let addr_ptrs = std::slice::from_raw_parts(addresses, count); + let amount_slice = std::slice::from_raw_parts(amounts, count); - for i in 0..count { - let c_str = unwrap_result_or_return!(std::ffi::CStr::from_ptr(addr_ptrs[i]).to_str()); - - let addr = unwrap_result_or_return!(dashcore::Address::from_str(c_str)).assume_checked(); - - outputs.push((addr, amount_slice[i])); - } + let mut outputs = Vec::with_capacity(count); + for i in 0..count { + if addr_ptrs[i].is_null() { + return PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorNullPointer, + format!("null address pointer at index {i}"), + ); + } + let c_str = unwrap_result_or_return!(std::ffi::CStr::from_ptr(addr_ptrs[i]).to_str()); + let addr = + unwrap_result_or_return!(dashcore::Address::from_str(c_str)).assume_checked(); + outputs.push((addr, amount_slice[i])); + } + outputs + }; use key_wallet::account::account_type::StandardAccountType; let std_account_type = match account_type { @@ -134,3 +149,93 @@ pub unsafe extern "C" fn core_wallet_free_tx_bytes(bytes: *mut u8, len: usize) { let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut(bytes, len)); } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::handle::NULL_HANDLE; + + /// `count == 0` MUST NOT touch `addresses` / `amounts` — Swift's + /// empty `Array.withUnsafeBufferPointer.baseAddress` gives `nil`, + /// and `slice::from_raw_parts` is UB on a null pointer regardless + /// of length. Pass `NULL_HANDLE` so the storage lookup short- + /// circuits to `NotFound` before any wallet code runs — we only + /// care that input marshalling did not blow up. + #[test] + fn send_to_addresses_zero_count_null_pointers_is_safe() { + // Use a non-null but fake signer pointer; the closure that + // would dereference it is never entered because `NULL_HANDLE` + // makes `with_item` return `None`. + let fake_signer = std::ptr::dangling_mut::(); + let mut out_tx: *mut u8 = std::ptr::null_mut(); + let mut out_len: usize = 0; + + let result = unsafe { + core_wallet_send_to_addresses( + NULL_HANDLE, + 0, // BIP44Account + 0, + std::ptr::null(), // null addresses — allowed because count == 0 + std::ptr::null(), // null amounts — allowed because count == 0 + 0, + fake_signer, + &mut out_tx, + &mut out_len, + ) + }; + assert_eq!(result.code, PlatformWalletFFIResultCode::NotFound); + } + + /// Non-null `addresses` array with `count == 0` is also fine. + #[test] + fn send_to_addresses_zero_count_nonnull_pointers_is_safe() { + let dummy_addr: *const c_char = std::ptr::null(); + let dummy_amount: u64 = 0; + let addrs: [*const c_char; 1] = [dummy_addr]; + let amts: [u64; 1] = [dummy_amount]; + let fake_signer = std::ptr::dangling_mut::(); + let mut out_tx: *mut u8 = std::ptr::null_mut(); + let mut out_len: usize = 0; + + let result = unsafe { + core_wallet_send_to_addresses( + NULL_HANDLE, + 0, + 0, + addrs.as_ptr(), + amts.as_ptr(), + 0, // count = 0 → array contents ignored + fake_signer, + &mut out_tx, + &mut out_len, + ) + }; + assert_eq!(result.code, PlatformWalletFFIResultCode::NotFound); + } + + /// A null entry inside the address array must surface as + /// `ErrorNullPointer`, not UB. + #[test] + fn send_to_addresses_null_element_is_rejected() { + let addrs: [*const c_char; 2] = [std::ptr::null(), std::ptr::null()]; + let amts: [u64; 2] = [1, 2]; + let fake_signer = std::ptr::dangling_mut::(); + let mut out_tx: *mut u8 = std::ptr::null_mut(); + let mut out_len: usize = 0; + + let result = unsafe { + core_wallet_send_to_addresses( + NULL_HANDLE, + 0, + 0, + addrs.as_ptr(), + amts.as_ptr(), + 2, + fake_signer, + &mut out_tx, + &mut out_len, + ) + }; + assert_eq!(result.code, PlatformWalletFFIResultCode::ErrorNullPointer); + } +} diff --git a/packages/rs-platform-wallet-ffi/src/error.rs b/packages/rs-platform-wallet-ffi/src/error.rs index de1a6cb9441..1304221c370 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -80,12 +80,14 @@ pub enum PlatformWalletFFIResultCode { /// no in-tree producer today. Holding the slot here keeps language-mirror /// enums (Swift, Kotlin) numerically aligned with the eventual producer. ErrorArithmeticOverflow = 13, - /// Auto-select had no candidate inputs. Covers all three "can't-select-inputs" - /// wallet variants: `NoSpendableInputs` (account has nothing spendable), - /// `OnlyOutputAddressesFunded` (every funded address is also a destination), - /// and `OnlyDustInputs` (every funded address is below `min_input_amount`). - /// The typed Display rendering survives via the result message so callers - /// can distinguish the underlying cause. Caller must rotate to a fresh + /// Auto-select had no candidate inputs. Umbrella for every + /// "can't-pick-UTXOs" wallet variant: `NoSpendableInputs` (account + /// has nothing spendable, including the race-loser path), + /// `OnlyOutputAddressesFunded` (every funded address is also a + /// destination), and `OnlyDustInputs` (every funded address is + /// below `min_input_amount`). The typed Display rendering survives + /// via the result message so callers can distinguish the underlying + /// cause. Caller must wait for sync / new UTXOs, rotate to a fresh /// receive address, consolidate sub-min balances, or fall back to /// `InputSelection::Explicit`. ErrorNoSelectableInputs = 14, @@ -125,6 +127,15 @@ pub enum PlatformWalletFFIResultCode { /// and could double-send if the original spend landed. ErrorShieldedSpendUnconfirmed = 18, + /// Transaction builder selected an outpoint that another in-flight + /// build had already reserved — retryable. The originating + /// [`PlatformWalletError::ConcurrentSpendConflict`] carries a + /// `Vec` of the colliding inputs; that payload is + /// stringified into the `message` field for now (TODO: propagate + /// the structured outpoint list across the FFI when a generic + /// payload sidecar exists). + ErrorConcurrentSpendConflict = 31, + NotFound = 98, // Used exclusively for all the Option that are retuned as errors ErrorUnknown = 99, } @@ -204,17 +215,27 @@ impl From> for PlatformWalletFFIResult { impl From for PlatformWalletFFIResult { fn from(error: PlatformWalletError) -> Self { - // Map the typed wallet error variants explicitly so they - // don't flatten to ErrorUnknown at the FFI boundary. The - // catch-all ErrorUnknown remains for variants the FFI hasn't - // assigned a dedicated code yet — those still carry the - // typed Display rendering as the message. + // Map the typed wallet error variants explicitly so they don't + // flatten to ErrorUnknown at the FFI boundary. The catch-all + // ErrorUnknown remains for variants the FFI hasn't assigned a + // dedicated code yet — those still carry the typed Display + // rendering as the message. + // + // All three "can't-select-inputs" wallet variants + // (`NoSpendableInputs`, `OnlyOutputAddressesFunded`, + // `OnlyDustInputs`) collapse onto the umbrella + // `ErrorNoSelectableInputs` code; the typed Display rendering + // in the message lets callers distinguish the underlying cause + // (including the race-loser breadcrumb on `NoSpendableInputs`). let code = match &error { PlatformWalletError::NoSpendableInputs { .. } | PlatformWalletError::OnlyOutputAddressesFunded { .. } | PlatformWalletError::OnlyDustInputs { .. } => { PlatformWalletFFIResultCode::ErrorNoSelectableInputs } + PlatformWalletError::ConcurrentSpendConflict { .. } => { + PlatformWalletFFIResultCode::ErrorConcurrentSpendConflict + } PlatformWalletError::WalletAlreadyExists(..) => { PlatformWalletFFIResultCode::ErrorWalletAlreadyExists } @@ -466,14 +487,13 @@ mod tests { assert!(!r.message.is_null()); } - /// The three "can't-select-inputs" wallet variants (`NoSpendableInputs`, - /// `OnlyOutputAddressesFunded`, `OnlyDustInputs`) all map to the dedicated - /// `ErrorNoSelectableInputs` FFI code rather than flattening to - /// `ErrorUnknown`, and the typed Display rendering survives across the - /// boundary so callers can distinguish the underlying cause from the - /// message string. + /// All three "can't-select-inputs" wallet variants collapse onto the + /// umbrella `ErrorNoSelectableInputs` (14) FFI code, and the typed + /// Display rendering survives across the boundary so callers can + /// distinguish the underlying cause (including the race-loser + /// breadcrumb on `NoSpendableInputs`) from the message string. #[test] - fn no_selectable_inputs_maps_to_dedicated_code() { + fn no_selectable_inputs_maps_to_umbrella_code() { use dpp::address_funds::PlatformAddress; use key_wallet::account::StandardAccountType; @@ -502,7 +522,7 @@ mod tests { assert_eq!( result.code, PlatformWalletFFIResultCode::ErrorNoSelectableInputs, - "variant should map to ErrorNoSelectableInputs (rendered: {rendered})" + "variant must collapse onto the umbrella code (rendered: {rendered})" ); assert!(!result.message.is_null()); let msg = unsafe { std::ffi::CStr::from_ptr(result.message) } @@ -515,6 +535,28 @@ mod tests { } } + /// `ConcurrentSpendConflict` keeps its dedicated FFI code (31) — + /// race-loser breadcrumb on the typed `NoSpendableInputs` Display + /// is not the same thing as the builder picking an already-reserved + /// outpoint, and downstream callers (retry logic) must be able to + /// tell them apart by code. + #[test] + fn concurrent_spend_conflict_keeps_dedicated_code() { + let err = PlatformWalletError::ConcurrentSpendConflict { + selected: Vec::new(), + }; + let rendered = err.to_string(); + let result: PlatformWalletFFIResult = err.into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorConcurrentSpendConflict, + ); + let msg = unsafe { std::ffi::CStr::from_ptr(result.message) } + .to_string_lossy() + .into_owned(); + assert_eq!(msg, rendered); + } + /// `WalletAlreadyExists` maps to the dedicated /// `ErrorWalletAlreadyExists` FFI code rather than flattening to /// `ErrorUnknown`, so multi-network wallet create/enable callers can diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index 189772d10e0..337208834c0 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -1,3 +1,4 @@ +use dashcore::OutPoint; use dpp::address_funds::PlatformAddress; use dpp::fee::Credits; use dpp::identifier::Identifier; @@ -63,6 +64,12 @@ pub enum PlatformWalletError { #[error("Transaction building failed: {0}")] TransactionBuild(String), + #[error( + "Transaction builder selected an unavailable UTXO (concurrent spend); retry. \ + Selected outpoints: {selected:?}" + )] + ConcurrentSpendConflict { selected: Vec }, + #[error("no spendable inputs available on {account_type} account {account_index}: {context}")] NoSpendableInputs { account_type: StandardAccountType, diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 4609d1fb6d2..5f478ffc11f 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -1,11 +1,179 @@ -use dashcore::{Address as DashAddress, Transaction}; +use std::collections::BTreeSet; +use std::sync::Arc; + +use dashcore::{Address as DashAddress, OutPoint, Transaction, Txid}; use key_wallet::account::account_type::StandardAccountType; use key_wallet::managed_account::managed_account_trait::ManagedAccountTrait; use key_wallet::signer::Signer; +use key_wallet::transaction_checking::{TransactionContext, WalletTransactionChecker}; +use key_wallet::wallet::managed_wallet_info::coin_selection::SelectionError; +use key_wallet::wallet::managed_wallet_info::transaction_builder::BuilderError; +use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; +use key_wallet_manager::WalletManager; +use tokio::sync::RwLock; +use super::reservations::OutpointReservationGuard; use crate::broadcaster::TransactionBroadcaster; +use crate::wallet::platform_wallet::{PlatformWalletInfo, WalletId}; use crate::{CoreWallet, PlatformWalletError}; +/// Map a key-wallet [`BuilderError`] from `TransactionBuilder::build_signed` into a +/// [`PlatformWalletError`], distinguishing depleted-UTXO conditions (retryable +/// [`PlatformWalletError::NoSpendableInputs`]) from every other build failure +/// (fatal [`PlatformWalletError::TransactionBuild`]). +/// +/// The variants treated as "no spendable inputs" are: +/// +/// * [`BuilderError::InsufficientFunds`] — the builder's own pre-selection +/// shortfall check. +/// * [`BuilderError::CoinSelection`] wrapping either +/// [`SelectionError::NoUtxosAvailable`] or +/// [`SelectionError::InsufficientFunds`] — the coin-selector's two +/// depleted-UTXO outcomes. +/// +/// `account_type` and `account_index` are threaded through unchanged so the +/// resulting `NoSpendableInputs` carries the same operator-facing context the +/// upstream call sites already populate. +pub(crate) fn classify_build_error( + err: BuilderError, + account_type: StandardAccountType, + account_index: u32, +) -> PlatformWalletError { + let context = err.to_string(); + match err { + BuilderError::InsufficientFunds { .. } + | BuilderError::CoinSelection(SelectionError::NoUtxosAvailable) + | BuilderError::CoinSelection(SelectionError::InsufficientFunds { .. }) => { + PlatformWalletError::NoSpendableInputs { + account_type, + account_index, + context, + } + } + _ => PlatformWalletError::TransactionBuild(context), + } +} + +/// Post-`build_signed` defense-in-depth: re-snapshot the funding account's +/// spendable UTXOs and confirm every outpoint the builder `selected` is +/// still present. Returns [`PlatformWalletError::ConcurrentSpendConflict`] +/// listing the missing outpoints otherwise. +/// +/// Today every UTXO mutator goes through the wallet write lock the send +/// flow holds across build, so this is unreachable — but a future mutator +/// running outside that lock (mempool listener, chain reorg, etc.) would +/// slip through the pre-build spendable snapshot; this fresh re-fetch +/// catches it before broadcast. The reservations guard remains the primary +/// in-process race defense; this is the cross-process / cross-subsystem net. +/// +/// Both `selected` and `fresh_spendable` are caller-computed outpoint sets +/// so this stays a pure set check, independent of the managed-account type. +pub(crate) fn assert_selected_still_spendable( + selected: &BTreeSet, + fresh_spendable: &BTreeSet, +) -> Result<(), PlatformWalletError> { + if selected.is_subset(fresh_spendable) { + return Ok(()); + } + let missing: Vec = selected.difference(fresh_spendable).copied().collect(); + Err(PlatformWalletError::ConcurrentSpendConflict { selected: missing }) +} + +/// Shared send-flow tail used by [`CoreWallet::send_to_addresses`] and +/// `IdentityWallet::send_payment`. +/// +/// Broadcasts `tx`, then under a single write lock acquisition reconciles +/// wallet state and either releases or leaks the reservation guard +/// according to the post-broadcast invariant: +/// +/// * On broadcast failure the reservation is implicitly dropped — the +/// guard is moved into this function and unwinds via `Drop` when the +/// early `?` returns, releasing the outpoints so the caller can retry. +/// * On broadcast success, `check_core_transaction` is run inside the +/// write lock. If `is_relevant == true` the reservation is released via +/// [`OutpointReservationGuard::release_after_commit`]; otherwise it is +/// leaked via [`OutpointReservationGuard::leak_until_sync`] to prevent a +/// concurrent caller from re-selecting an already-broadcast outpoint. +/// * `on_reconcile` is invoked inside the write lock *after* +/// `check_core_transaction` so call-site-specific bookkeeping (e.g. +/// `IdentityWallet::send_payment` recording a `PaymentEntry`) sees the +/// same locked wallet state and runs in the same critical section. +/// +/// Returns the broadcasted [`Txid`]. +pub(crate) async fn broadcast_and_reconcile( + wallet_manager: &Arc>>, + wallet_id: WalletId, + broadcaster: &Arc, + tx: &Transaction, + reservation: OutpointReservationGuard, + on_reconcile: F, +) -> Result +where + B: TransactionBroadcaster + ?Sized, + F: FnOnce(&mut PlatformWalletInfo, &Txid, bool), +{ + // Broadcast first — on error the `reservation` guard is dropped via + // `?` unwind, releasing the outpoints so the caller can retry. If the + // network accepted but the call errored (ambiguous outcome), a retry + // will be rejected as a duplicate spend rather than us marking UTXOs + // spent prematurely. + let txid = broadcaster.broadcast(tx).await?; + + // Mark inputs spent under the write lock, transitioning them from + // "reserved" to "spent" before the reservation guard drops. The guard + // is consumed below by either `release_after_commit` (success) or + // `leak_until_sync` (post-broadcast reconcile failure). + let mut reconciled = false; + { + let mut wm = wallet_manager.write().await; + if let Some((wallet, info)) = wm.get_wallet_mut_and_info_mut(&wallet_id) { + let check_result = info + .check_core_transaction(tx, TransactionContext::Mempool, wallet, true, true) + .await; + reconciled = check_result.is_relevant; + if !reconciled { + tracing::error!( + target: "platform_wallet::broadcast", + event = "post_broadcast_unrelated_to_own_wallet", + txid = %txid, + wallet_id = %hex::encode(wallet_id), + "Internal invariant violation: own-built broadcast not recognized by post-broadcast check" + ); + } + on_reconcile(info, &txid, reconciled); + } else { + tracing::warn!( + target: "platform_wallet::broadcast", + event = "post_broadcast_wallet_missing", + wallet_id = %hex::encode(wallet_id), + %txid, + "wallet missing during post-broadcast transaction registration" + ); + } + } + + if reconciled { + // Inputs are now marked spent; safe to release reservation. + reservation.release_after_commit(); + } else { + // Broadcast succeeded but state could not be reconciled. Releasing + // the reservation now risks a concurrent send re-selecting the + // same UTXO and producing a double-spend the network would + // reject. Keep it held until process restart — see + // `OutpointReservationGuard::leak_until_sync`. + tracing::warn!( + target: "platform_wallet::broadcast", + event = "post_broadcast_reservation_leaked_until_sync", + %txid, + wallet_id = %hex::encode(wallet_id), + "leaking outpoint reservation: post-broadcast reconciliation failed" + ); + reservation.leak_until_sync(); + } + + Ok(txid) +} + impl CoreWallet { /// Broadcast a signed transaction to the network. /// @@ -31,21 +199,19 @@ impl CoreWallet { /// for UTXO selection, fee estimation, and signing. Change is sent to /// the next internal address of the specified account. /// + /// Concurrent calls on the same wallet handle are race-safe via the + /// reservation set in [`super::reservations`]: the second caller + /// short-circuits with [`PlatformWalletError::NoSpendableInputs`] + /// before touching the network if all UTXOs are reserved by an + /// in-flight broadcast. + /// /// Signing is delegated to the caller-supplied /// [`Signer`](key_wallet::signer::Signer) via the /// `impl TransactionSigner for S` blanket in /// `key-wallet`'s `transaction_builder.rs`. For Swift wallets this - /// is typically a - /// [`MnemonicResolverCoreSigner`](crate::wallet::asset_lock::build) - /// from `platform-wallet-ffi`, backed by the Keychain-resolver - /// vtable so private keys never cross the FFI boundary. - /// - /// **Note (smell):** the body of this method is a near-duplicate of - /// `ManagedWalletInfo::build_and_sign_transaction` in `key-wallet` - /// (`wallet/managed_wallet_info/transaction_building.rs`). - /// It's reimplemented here because the upstream helper is BIP-44-only, - /// parametrizing upstream on `AccountTypePreference` so it picks - /// `standard_bip{32,44}_accounts` would be a trivial change + /// is typically a `MnemonicResolverCoreSigner` from + /// `platform-wallet-ffi`, backed by the Keychain-resolver vtable so + /// private keys never cross the FFI boundary. pub async fn send_to_addresses( &self, account_type: StandardAccountType, @@ -55,7 +221,6 @@ impl CoreWallet { ) -> Result { use key_wallet::wallet::managed_wallet_info::coin_selection::SelectionStrategy; use key_wallet::wallet::managed_wallet_info::transaction_builder::TransactionBuilder; - use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; if outputs.is_empty() { return Err(PlatformWalletError::TransactionBuild( @@ -63,7 +228,7 @@ impl CoreWallet { )); } - let tx = { + let (tx, _reservation) = { let mut wm = self.wallet_manager.write().await; let (wallet, info) = wm.get_wallet_and_info_mut(&self.wallet_id).ok_or_else(|| { crate::error::PlatformWalletError::WalletNotFound( @@ -120,15 +285,66 @@ impl CoreWallet { ), }; - // The blanket `impl TransactionSigner for S` in - // `key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs:482` - // makes the signer drop-in for the previously `Wallet`-backed - // path; the funds-derived `address_derivation_path` lookup is - // unchanged. + let xpub = account.account_xpub; + + // Snapshot spendable UTXOs minus any in-flight reservations from + // a concurrent `send_to_addresses` on this handle. Single lock + // acquisition for the whole filter pass. + let reserved = self.reservations.snapshot(); + let spendable: Vec<_> = managed_account + .spendable_utxos(current_height) + .into_iter() + .filter(|utxo| !reserved.contains(&utxo.outpoint)) + .cloned() + .collect(); + + if spendable.is_empty() { + return Err(PlatformWalletError::NoSpendableInputs { + account_index, + account_type, + context: "all UTXOs used or reserved by in-flight transactions".to_string(), + }); + } + + // Pick the next change address. Peek (advance=false) first; if the + // peeked address is already pending from a concurrent in-flight + // send, advance the derivation index and peek again until we find + // one that is not pending. The final chosen address is committed + // (advance=true) inside this same write lock and inserted into the + // reservation set so a concurrent caller can never select the same + // change address. Advancing burns at most one index per concurrent + // in-flight send — a bounded, acceptable cost for privacy. + // + // TODO(#3770): switch change-address selection to the multi-pool + // address_reserve / OutpointReservations bridge (stacked + // follow-up) instead of this peek-and-advance loop. + let pending_change = self.reservations.pending_change_snapshot(); + let change_addr = loop { + let peeked = managed_account + .next_change_address(Some(&xpub), false) + .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; + if !pending_change.contains(&peeked) { + // Commit the advance now (under the same write lock as + // the outpoint reservation below). On broadcast failure + // a single index is burned — acceptable; on success the + // pending-change entry is released when the guard drops + // post-`check_core_transaction`. + let _ = managed_account + .next_change_address(Some(&xpub), true) + .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; + break peeked; + } + // Burn this index by advancing past it and try again. + let _ = managed_account + .next_change_address(Some(&xpub), true) + .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; + }; + let mut builder = TransactionBuilder::new() .set_current_height(current_height) .set_selection_strategy(SelectionStrategy::LargestFirst) - .set_funding(managed_account, account); + .set_change_address(change_addr.clone()) + .add_inputs(spendable.iter().cloned()); for (addr, amount) in &outputs { builder = builder.add_output(addr, *amount); } @@ -138,11 +354,990 @@ impl CoreWallet { managed_account.address_derivation_path(&addr) }) .await - .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; - tx + .map_err(|e| classify_build_error(e, account_type, account_index))?; + + // Defense-in-depth: unreachable under normal builder contract but guards against + // a future regression where coin selection picks an outpoint outside `spendable`. + let selected: BTreeSet = + tx.input.iter().map(|txin| txin.previous_output).collect(); + let spendable_outpoints: BTreeSet = + spendable.iter().map(|utxo| utxo.outpoint).collect(); + if !selected.is_subset(&spendable_outpoints) { + // Typed retryable variant: forward-compatible with cross-process + // concurrent-spend surfacing; today only a builder regression hits it. + return Err(PlatformWalletError::ConcurrentSpendConflict { + selected: selected.into_iter().collect(), + }); + } + + // Defense-in-depth: re-snapshot spendable UTXOs after + // `build_signed` and confirm every selected outpoint is still + // present. See [`assert_selected_still_spendable`] for why. + let fresh_spendable_outpoints: BTreeSet = managed_account + .spendable_utxos(current_height) + .into_iter() + .map(|utxo| utxo.outpoint) + .collect(); + assert_selected_still_spendable(&selected, &fresh_spendable_outpoints)?; + + // Reserve before releasing the lock so the next caller sees these outpoints + // filtered out *and* skips the peeked change address. Guard held until + // `check_core_transaction` marks them spent (success) or the error + // unwinds (failure → outpoints released for retry). + // + // Atomic reject-overlap: `reserve` returns `None` if any selected + // outpoint or the change address was grabbed by a concurrent caller + // between our prefilter and here. Unreachable today (both filters run + // under this same write lock), but handled defensively as the retryable + // concurrent-spend conflict the prefilter would otherwise surface. + let reservation = self + .reservations + .reserve( + selected.iter().copied().collect(), + Some(change_addr.clone()), + ) + .ok_or_else(|| PlatformWalletError::ConcurrentSpendConflict { + selected: selected.iter().copied().collect(), + })?; + + (tx, reservation) }; - self.broadcast_transaction(&tx).await?; + broadcast_and_reconcile( + &self.wallet_manager, + self.wallet_id, + &self.broadcaster, + &tx, + _reservation, + |_info, _txid, _reconciled| { + // No path-specific post-reconcile bookkeeping for the + // raw send-to-addresses flow. + }, + ) + .await?; + Ok(tx) } } + +#[cfg(test)] +mod tests { + //! Broadcast and `send_to_addresses` contracts. + //! + //! Pins: + //! - `broadcast_transaction` forwards the broadcaster's `Ok`/`Err` unchanged. + //! - Concurrent `send_to_addresses` on the same wallet handle resolves via + //! the reservation set: the loser short-circuits with `NoSpendableInputs` + //! before reaching the broadcaster. + //! - A broadcast failure releases the reservation so a retry sees the same + //! UTXO as spendable again. + //! - An empty spendable snapshot (e.g. all UTXOs reserved) maps to + //! `NoSpendableInputs` via the early-exit guard. + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + + use async_trait::async_trait; + use dashcore::consensus::deserialize; + use dashcore::{Transaction, Txid}; + use tokio::sync::RwLock; + + use crate::broadcaster::TransactionBroadcaster; + use crate::wallet::core::balance::WalletBalance; + use crate::wallet::core::CoreWallet; + use crate::PlatformWalletError; + use key_wallet::Network; + use key_wallet_manager::WalletManager; + + /// Records every call and returns a canned outcome. + struct MockBroadcaster { + outcome: BroadcastOutcome, + calls: AtomicUsize, + } + + enum BroadcastOutcome { + Ok(Txid), + Err(String), + } + + impl MockBroadcaster { + fn new(outcome: BroadcastOutcome) -> Self { + Self { + outcome, + calls: AtomicUsize::new(0), + } + } + + fn call_count(&self) -> usize { + self.calls.load(Ordering::SeqCst) + } + } + + #[async_trait] + impl TransactionBroadcaster for MockBroadcaster { + async fn broadcast(&self, _transaction: &Transaction) -> Result { + self.calls.fetch_add(1, Ordering::SeqCst); + match &self.outcome { + BroadcastOutcome::Ok(txid) => Ok(*txid), + BroadcastOutcome::Err(msg) => { + Err(PlatformWalletError::TransactionBroadcast(msg.clone())) + } + } + } + } + + /// Minimal serialized tx (1 input, 1 output, 0 value) — only the + /// broadcaster's Err/Ok branch matters here, not the shape. + fn dummy_transaction() -> Transaction { + let bytes = hex::decode( + "010000000100000000000000000000000000000000000000000000000000000000000000\ + 00ffffffff00ffffffff0100000000000000000000000000", + ) + .expect("valid hex"); + deserialize(&bytes).expect("deserializable tx") + } + + fn make_core_wallet(broadcaster: Arc) -> CoreWallet { + let sdk = Arc::new( + dash_sdk::SdkBuilder::new_mock() + .build() + .expect("mock sdk build"), + ); + let wallet_manager = Arc::new(RwLock::new(WalletManager::new(Network::Testnet))); + CoreWallet::new( + sdk, + wallet_manager, + [0u8; 32], + broadcaster, + Arc::new(WalletBalance::new()), + ) + } + + /// `broadcast_transaction` forwards a broadcaster `Err` to the caller + /// without transformation. + #[tokio::test] + async fn broadcast_transaction_passes_through_err_unchanged() { + let broadcaster = Arc::new(MockBroadcaster::new(BroadcastOutcome::Err( + "simulated network rejection".to_string(), + ))); + let wallet = make_core_wallet(Arc::clone(&broadcaster)); + let tx = dummy_transaction(); + + let result = wallet.broadcast_transaction(&tx).await; + + assert!( + matches!(result, Err(PlatformWalletError::TransactionBroadcast(_))), + "expected broadcast Err to propagate, got {:?}", + result + ); + assert_eq!( + broadcaster.call_count(), + 1, + "broadcaster must be called exactly once on a failed broadcast" + ); + } + + /// `broadcast_transaction` forwards the broadcaster's `Txid` to the + /// caller without transformation. + #[tokio::test] + async fn broadcast_transaction_passes_through_ok_unchanged() { + let expected_txid = dummy_transaction().txid(); + let broadcaster = Arc::new(MockBroadcaster::new(BroadcastOutcome::Ok(expected_txid))); + let wallet = make_core_wallet(Arc::clone(&broadcaster)); + let tx = dummy_transaction(); + + let result = wallet.broadcast_transaction(&tx).await; + + assert_eq!( + result.expect("broadcast Ok"), + expected_txid, + "broadcast_transaction must pass the broadcaster's Txid through unchanged" + ); + assert_eq!( + broadcaster.call_count(), + 1, + "broadcaster must be called exactly once on a successful broadcast" + ); + } + + // Race-closing tests: same-UTXO concurrent `send_to_addresses`. + // B must short-circuit with `NoSpendableInputs` before the network — a `TransactionBroadcast` + // failure from B would mean the bug is still open. + + use std::collections::BTreeMap; + + use dashcore::hashes::Hash; + use dashcore::{Address as DashAddress, OutPoint, TxOut}; + use key_wallet::wallet::initialization::WalletAccountCreationOptions; + use key_wallet::wallet::Wallet; + use key_wallet::Utxo; + use tokio::sync::Notify; + + use crate::wallet::platform_wallet::PlatformWalletInfo; + use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; + + /// In-memory `Signer` for tests — mirrors the one in key-wallet's own + /// transaction-building tests. Backed by the mnemonic wallet's root + /// extended private key, so derivation paths from the funded account + /// resolve to the keys that actually own the seeded UTXO. + struct InMemorySigner { + root: key_wallet::wallet::root_extended_keys::RootExtendedPrivKey, + network: Network, + } + + const IN_MEMORY_METHODS: &[key_wallet::signer::SignerMethod] = + &[key_wallet::signer::SignerMethod::Digest]; + + #[async_trait] + impl key_wallet::signer::Signer for InMemorySigner { + type Error = String; + + fn supported_methods(&self) -> &[key_wallet::signer::SignerMethod] { + IN_MEMORY_METHODS + } + + async fn sign_ecdsa( + &self, + path: &key_wallet::bip32::DerivationPath, + sighash: [u8; 32], + ) -> Result< + ( + dashcore::secp256k1::ecdsa::Signature, + dashcore::secp256k1::PublicKey, + ), + Self::Error, + > { + let secp = dashcore::secp256k1::Secp256k1::new(); + let xpriv = self + .root + .to_extended_priv_key(self.network) + .derive_priv(&secp, path) + .map_err(|e| e.to_string())?; + let msg = dashcore::secp256k1::Message::from_digest(sighash); + let sig = secp.sign_ecdsa(&msg, &xpriv.private_key); + let pk = dashcore::secp256k1::PublicKey::from_secret_key(&secp, &xpriv.private_key); + Ok((sig, pk)) + } + + async fn public_key( + &self, + path: &key_wallet::bip32::DerivationPath, + ) -> Result { + let secp = dashcore::secp256k1::Secp256k1::new(); + let xpriv = self + .root + .to_extended_priv_key(self.network) + .derive_priv(&secp, path) + .map_err(|e| e.to_string())?; + Ok(dashcore::secp256k1::PublicKey::from_secret_key( + &secp, + &xpriv.private_key, + )) + } + } + + fn root_from( + wallet: &key_wallet::wallet::Wallet, + ) -> key_wallet::wallet::root_extended_keys::RootExtendedPrivKey { + match &wallet.wallet_type { + key_wallet::wallet::WalletType::Mnemonic { + root_extended_private_key, + .. + } => root_extended_private_key.clone(), + _ => unreachable!("test wallets are mnemonic"), + } + } + + /// Mock broadcaster that gates the broadcast on an external `Notify`. + /// `entered` fires the moment `broadcast()` is awaited — by then the + /// caller has reserved its outpoints and dropped the wallet write lock. + struct GatedBroadcaster { + gate: Arc, + entered: Arc, + calls: AtomicUsize, + succeed: bool, + } + + #[async_trait] + impl TransactionBroadcaster for GatedBroadcaster { + async fn broadcast(&self, transaction: &Transaction) -> Result { + self.calls.fetch_add(1, Ordering::SeqCst); + self.entered.notify_one(); + self.gate.notified().await; + if self.succeed { + Ok(transaction.txid()) + } else { + Err(PlatformWalletError::TransactionBroadcast( + "mock failure".to_string(), + )) + } + } + } + + /// Always-failing mock broadcaster — used to assert that a failed + /// broadcast releases the reservation so a retry can pick up the + /// same UTXO. + struct FailingBroadcaster; + + #[async_trait] + impl TransactionBroadcaster for FailingBroadcaster { + async fn broadcast(&self, _transaction: &Transaction) -> Result { + Err(PlatformWalletError::TransactionBroadcast( + "always fails".to_string(), + )) + } + } + + /// Build a single-wallet `WalletManager` containing one BIP-44 + /// account (index 0) funded with one large UTXO at the account's + /// first receive address. Returns the wallet manager handle, the + /// wallet id, a recipient address (a separate derived address in the + /// same account — funding/sending to the same address is not the + /// property under test), and a signer backed by the wallet's + /// mnemonic root so `build_signed` can sign the seeded UTXO. + fn build_funded_wallet_manager( + utxo_value: u64, + ) -> ( + Arc>>, + crate::wallet::platform_wallet::WalletId, + DashAddress, + InMemorySigner, + ) { + let wallet = Wallet::new_random(Network::Testnet, WalletAccountCreationOptions::Default) + .expect("test wallet"); + let signer = InMemorySigner { + root: root_from(&wallet), + network: Network::Testnet, + }; + + let xpub = wallet + .accounts + .standard_bip44_accounts + .get(&0) + .expect("bip44 account 0") + .account_xpub; + let mut wallet_info = ManagedWalletInfo::from_wallet(&wallet, 0); + + // Height must be well past UTXO height: `select_coins_with_size` enforces + // `min_confirmations >= 1`, which requires synced_height > utxo_height. + use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface as _; + wallet_info.update_synced_height(100); + + let funding_address = wallet_info + .accounts + .standard_bip44_accounts + .get_mut(&0) + .expect("managed bip44 account 0") + .next_receive_address(Some(&xpub), true) + .expect("derive receive address"); + + let outpoint = OutPoint::new(Txid::from_byte_array([7u8; 32]), 0); + let mut utxo = Utxo::new( + outpoint, + TxOut { + value: utxo_value, + script_pubkey: funding_address.script_pubkey(), + }, + funding_address, + 1, + false, + ); + utxo.is_confirmed = true; + wallet_info + .accounts + .standard_bip44_accounts + .get_mut(&0) + .expect("managed bip44 account 0") + .utxos + .insert(outpoint, utxo); + + let info = PlatformWalletInfo { + core_wallet: wallet_info, + balance: Arc::new(WalletBalance::new()), + identity_manager: crate::wallet::identity::IdentityManager::new(), + tracked_asset_locks: BTreeMap::new(), + }; + + let mut wm: WalletManager = WalletManager::new(Network::Testnet); + let wallet_id = wm.insert_wallet(wallet, info).expect("insert"); + + // Recipient — use the second receive address as a stable target. + let recipient = { + let info = wm.get_wallet_info_mut(&wallet_id).expect("info"); + info.core_wallet + .accounts + .standard_bip44_accounts + .get_mut(&0) + .expect("acc") + .next_receive_address(Some(&xpub), true) + .expect("derive recipient") + }; + + (Arc::new(RwLock::new(wm)), wallet_id, recipient, signer) + } + + fn make_core_wallet_for_manager( + wm: Arc>>, + wallet_id: crate::wallet::platform_wallet::WalletId, + broadcaster: Arc, + ) -> CoreWallet { + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + CoreWallet::new( + sdk, + wm, + wallet_id, + broadcaster, + Arc::new(WalletBalance::new()), + ) + } + + /// Two concurrent `send_to_addresses` calls on one wallet with one UTXO must yield exactly + /// one broadcast. The loser must get [`PlatformWalletError::NoSpendableInputs`] — never + /// `TransactionBroadcast` (that would mean it reached the network, which is the bug closed). + #[tokio::test] + async fn concurrent_same_utxo_sends_resolve_via_reservation_set() { + use key_wallet::account::account_type::StandardAccountType; + + let (wm, wallet_id, recipient, signer) = build_funded_wallet_manager(2_000_000); + let signer = Arc::new(signer); + let gate = Arc::new(Notify::new()); + let entered = Arc::new(Notify::new()); + let broadcaster = Arc::new(GatedBroadcaster { + gate: Arc::clone(&gate), + entered: Arc::clone(&entered), + calls: AtomicUsize::new(0), + succeed: true, + }); + let core = make_core_wallet_for_manager( + wm, + wallet_id, + Arc::clone(&broadcaster) as Arc, + ); + + let send_value = 100_000; + let outputs_a = vec![(recipient.clone(), send_value)]; + let outputs_b = vec![(recipient.clone(), send_value)]; + + // Spawn caller A. It will reserve the only spendable outpoint + // under the wallet write lock, drop the lock, and block on the + // broadcast `Notify`. + let core_a = core.clone(); + let signer_a = Arc::clone(&signer); + let a_handle = tokio::spawn(async move { + core_a + .send_to_addresses( + StandardAccountType::BIP44Account, + 0, + outputs_a, + signer_a.as_ref(), + ) + .await + }); + + // Deterministic handshake: wait until A has reached the broadcast gate. + // By that point A has reserved the outpoint and dropped the wallet write lock. + entered.notified().await; + + // Caller B starts now. The wallet's only UTXO is reserved by A, + // so B's spendable snapshot is empty → `NoSpendableInputs`. + let b_result = core + .send_to_addresses( + StandardAccountType::BIP44Account, + 0, + outputs_b, + signer.as_ref(), + ) + .await; + + match &b_result { + Err(PlatformWalletError::NoSpendableInputs { context, .. }) => { + assert!( + context.contains("reserved") + || context.contains("Insufficient") + || context.contains("No UTXOs"), + "B's NoSpendableInputs context should mention reservation \ + or insufficient/no-utxos; got: {context}" + ); + } + other => panic!( + "B must short-circuit with NoSpendableInputs (the race-loser \ + must not reach the broadcaster); got: {other:?}" + ), + } + + // Now release A's broadcast. + gate.notify_one(); + + let a_result = a_handle.await.expect("a task panicked"); + assert!( + a_result.is_ok(), + "A must succeed once its broadcast gate fires; got: {a_result:?}" + ); + + // Pin "loser never reached the network" directly: only A invoked the broadcaster. + assert_eq!( + broadcaster.calls.load(Ordering::SeqCst), + 1, + "broadcaster must be called exactly once across both concurrent senders" + ); + } + + /// On broadcast failure, the reservation must be released so the + /// caller can retry. This is the regression-tripwire for the + /// reservation guard's Drop semantics. + #[tokio::test] + async fn broadcast_failure_releases_reservation_for_retry() { + use key_wallet::account::account_type::StandardAccountType; + + let (wm, wallet_id, recipient, signer) = build_funded_wallet_manager(2_000_000); + let broadcaster: Arc = Arc::new(FailingBroadcaster); + let core = make_core_wallet_for_manager(wm, wallet_id, broadcaster); + + let outputs = vec![(recipient.clone(), 100_000)]; + + // First call fails at the broadcast step → guard drops → + // reservation released. The change-address index is also rolled + // back by virtue of #3585's peek-then-commit pattern. + let first = core + .send_to_addresses( + StandardAccountType::BIP44Account, + 0, + outputs.clone(), + &signer, + ) + .await; + assert!( + matches!(first, Err(PlatformWalletError::TransactionBroadcast(_))), + "first call must surface broadcast failure; got: {first:?}" + ); + + // Reservation released: the second call must reach the broadcaster (same UTXO visible), + // not short-circuit with `NoSpendableInputs` (which would indicate a leaked reservation). + let second = core + .send_to_addresses(StandardAccountType::BIP44Account, 0, outputs, &signer) + .await; + match second { + Err(PlatformWalletError::TransactionBroadcast(_)) => { + // Expected — reservation released, coin selection + // succeeded, broadcaster rejected as designed. + } + Err(PlatformWalletError::NoSpendableInputs { .. }) => { + panic!( + "reservation leaked after broadcast failure — second \ + call should have selected the released UTXO" + ); + } + other => panic!("unexpected second call result: {other:?}"), + } + } + + /// If `check_core_transaction` returns `is_relevant = false` + /// after a successful broadcast (an internal invariant violation but a + /// real-world possibility on a corrupted/stale wallet state), the + /// reservation must stay held — releasing it could let a concurrent + /// caller select the same already-broadcast outpoint and produce a + /// double-spend the network would reject. + /// + /// We force `is_relevant = false` by clearing the funding account's + /// address-pool entries between the broadcast and the reconcile step; + /// the post-broadcast `check_core_transaction` then matches nothing. + #[tokio::test] + async fn unreconciled_broadcast_keeps_reservation_held() { + use key_wallet::account::account_type::StandardAccountType; + + let (wm, wallet_id, recipient, signer) = build_funded_wallet_manager(2_000_000); + let signer = Arc::new(signer); + + let gate = Arc::new(Notify::new()); + let entered = Arc::new(Notify::new()); + let broadcaster = Arc::new(GatedBroadcaster { + gate: Arc::clone(&gate), + entered: Arc::clone(&entered), + calls: AtomicUsize::new(0), + succeed: true, + }); + + let core = make_core_wallet_for_manager( + Arc::clone(&wm), + wallet_id, + Arc::clone(&broadcaster) as Arc, + ); + + let outputs = vec![(recipient.clone(), 100_000)]; + + let core_send = core.clone(); + let signer_send = Arc::clone(&signer); + let handle = tokio::spawn(async move { + core_send + .send_to_addresses( + StandardAccountType::BIP44Account, + 0, + outputs, + signer_send.as_ref(), + ) + .await + }); + + // Wait until the broadcast call has been entered — by then the + // outpoint is reserved, the change index is committed, and the + // wallet write lock has been released. + entered.notified().await; + + // Sabotage the wallet so `check_core_transaction` cannot match + // anything. Clearing the account-collection map entirely is the + // surest way: with no accounts the checker walks zero entries. + { + let mut wm_guard = wm.write().await; + let info = wm_guard.get_wallet_info_mut(&wallet_id).expect("info"); + info.core_wallet.accounts.standard_bip44_accounts.clear(); + } + + // Capture the reservation's outpoint *before* releasing the gate + // so the assertion target is stable. + let funding_outpoint = OutPoint::new(Txid::from_byte_array([7u8; 32]), 0); + + // Release the broadcast — the post-broadcast reconcile sees + // `is_relevant=false` and leaks the reservation. + gate.notify_one(); + + let result = handle.await.expect("task panicked"); + assert!( + result.is_ok(), + "send_to_addresses must succeed when broadcast succeeds; got: {result:?}" + ); + + assert!( + core.reservations.contains(&funding_outpoint), + "reservation must remain held when post-broadcast reconcile fails (is_relevant=false), \ + otherwise a concurrent caller could re-select the same already-broadcast outpoint" + ); + } + + /// Pins the early-exit guard: when the spendable snapshot is empty + /// (e.g. all UTXOs reserved by in-flight broadcasts), `send_to_addresses` + /// surfaces `NoSpendableInputs` without invoking the builder. + /// + /// Note: the upstream coin-selection string-match in `send_to_addresses` + /// is not exercised here — that path is currently unpinned. + #[tokio::test] + async fn builder_error_text_contract_for_no_inputs() { + use key_wallet::account::account_type::StandardAccountType; + + let (wm, wallet_id, recipient, signer) = build_funded_wallet_manager(2_000_000); + let broadcaster: Arc = Arc::new(FailingBroadcaster); + let core = make_core_wallet_for_manager(wm, wallet_id, broadcaster); + + let outputs = vec![(recipient.clone(), 100_000)]; + + // Reserve the wallet's only outpoint so the spendable snapshot is + // empty for the next caller, exercising the early-exit guard. + let outpoint = OutPoint::new(Txid::from_byte_array([7u8; 32]), 0); + let _guard = core + .reservations + .reserve(vec![outpoint], None) + .expect("reserve"); + + let result = core + .send_to_addresses(StandardAccountType::BIP44Account, 0, outputs, &signer) + .await; + + assert!( + matches!(result, Err(PlatformWalletError::NoSpendableInputs { .. })), + "send_to_addresses must map a fully-reserved wallet to NoSpendableInputs; got: {result:?}" + ); + } + + // ---- classify_build_error: typed BuilderError → PlatformWalletError ---- + // + // These tests pin the typed contract directly so a future rename or + // rewording of the upstream `Display` impl cannot silently downgrade + // `NoSpendableInputs` back to `TransactionBuild`. + + use super::classify_build_error; + use key_wallet::account::account_type::StandardAccountType; + use key_wallet::wallet::managed_wallet_info::coin_selection::SelectionError; + use key_wallet::wallet::managed_wallet_info::transaction_builder::BuilderError; + + #[test] + fn classify_builder_insufficient_funds_maps_to_no_spendable_inputs() { + let err = BuilderError::InsufficientFunds { + available: 1_000, + required: 10_000, + }; + let mapped = classify_build_error(err, StandardAccountType::BIP44Account, 7); + match mapped { + PlatformWalletError::NoSpendableInputs { + account_type, + account_index, + context, + } => { + assert!(matches!(account_type, StandardAccountType::BIP44Account)); + assert_eq!(account_index, 7); + assert!( + context.contains("Insufficient funds"), + "context should preserve the upstream Display; got: {context}" + ); + } + other => panic!("expected NoSpendableInputs, got: {other:?}"), + } + } + + #[test] + fn classify_coin_selection_no_utxos_available_maps_to_no_spendable_inputs() { + let err = BuilderError::CoinSelection(SelectionError::NoUtxosAvailable); + let mapped = classify_build_error(err, StandardAccountType::BIP32Account, 3); + match mapped { + PlatformWalletError::NoSpendableInputs { + account_type, + account_index, + .. + } => { + assert!(matches!(account_type, StandardAccountType::BIP32Account)); + assert_eq!(account_index, 3); + } + other => panic!("expected NoSpendableInputs, got: {other:?}"), + } + } + + #[test] + fn classify_coin_selection_insufficient_funds_maps_to_no_spendable_inputs() { + let err = BuilderError::CoinSelection(SelectionError::InsufficientFunds { + available: 5, + required: 100, + }); + let mapped = classify_build_error(err, StandardAccountType::BIP44Account, 0); + assert!( + matches!(mapped, PlatformWalletError::NoSpendableInputs { .. }), + "CoinSelection(InsufficientFunds) must map to NoSpendableInputs; got: {mapped:?}" + ); + } + + #[test] + fn classify_no_inputs_falls_through_to_transaction_build() { + let err = BuilderError::NoInputs; + let mapped = classify_build_error(err, StandardAccountType::BIP44Account, 0); + assert!( + matches!(mapped, PlatformWalletError::TransactionBuild(_)), + "non-depleted BuilderError must fall through to TransactionBuild; got: {mapped:?}" + ); + } + + #[test] + fn classify_signing_failed_falls_through_to_transaction_build() { + let err = BuilderError::SigningFailed("bad signer".to_string()); + let mapped = classify_build_error(err, StandardAccountType::BIP44Account, 0); + match mapped { + PlatformWalletError::TransactionBuild(msg) => { + assert!( + msg.contains("bad signer"), + "context should preserve message" + ); + } + other => panic!("expected TransactionBuild, got: {other:?}"), + } + } + + #[test] + fn classify_coin_selection_failed_falls_through_to_transaction_build() { + let err = + BuilderError::CoinSelection(SelectionError::SelectionFailed("wraparound".to_string())); + let mapped = classify_build_error(err, StandardAccountType::BIP44Account, 0); + assert!( + matches!(mapped, PlatformWalletError::TransactionBuild(_)), + "CoinSelection(SelectionFailed) is not a depleted-UTXO outcome; \ + must fall through to TransactionBuild; got: {mapped:?}" + ); + } + + // ---- assert_selected_still_spendable: post-build fresh-spendable net ---- + + use std::collections::BTreeSet; + + use super::assert_selected_still_spendable; + + fn op(byte: u8, vout: u32) -> OutPoint { + OutPoint::new(Txid::from_byte_array([byte; 32]), vout) + } + + #[test] + fn selected_subset_of_fresh_spendable_is_ok() { + let selected: BTreeSet = [op(1, 0), op(2, 0)].into_iter().collect(); + let fresh: BTreeSet = [op(1, 0), op(2, 0), op(3, 0)].into_iter().collect(); + assert!( + assert_selected_still_spendable(&selected, &fresh).is_ok(), + "a selected set fully contained in the fresh spendable set must pass" + ); + } + + #[test] + fn selected_missing_from_fresh_spendable_reports_concurrent_spend_conflict() { + let gone = op(2, 0); + let selected: BTreeSet = [op(1, 0), gone].into_iter().collect(); + // `gone` disappeared between build and the post-build re-snapshot. + let fresh: BTreeSet = [op(1, 0), op(3, 0)].into_iter().collect(); + + match assert_selected_still_spendable(&selected, &fresh) { + Err(PlatformWalletError::ConcurrentSpendConflict { selected: missing }) => { + assert_eq!( + missing, + vec![gone], + "the conflict must list exactly the outpoint that vanished from the fresh set" + ); + } + other => panic!( + "expected ConcurrentSpendConflict listing the missing outpoint; got: {other:?}" + ), + } + } + + // ---- broadcast_and_reconcile: shared post-broadcast helper ---- + // + // The helper centralises the broadcast → reconcile → release-or-leak + // decision tree used by both `CoreWallet::send_to_addresses` and + // `IdentityWallet::send_payment`. These tests pin the helper's three + // invariants directly so the shared path stays honest: + // + // 1. Broadcast failure unwinds the reservation (caller can retry). + // 2. The `on_reconcile` hook is only invoked when the wallet lookup + // succeeded, and is passed the same `is_relevant` flag the helper + // used to decide release-vs-leak. + // 3. When the wallet is missing post-broadcast, the hook is NOT + // invoked and the reservation is leaked (no double-spend risk). + + use std::sync::atomic::AtomicBool; + + use super::broadcast_and_reconcile; + + /// On broadcast failure, the reservation guard moved into the helper + /// must be dropped via early-return, releasing the outpoints so the + /// caller can retry. The `on_reconcile` hook must NOT fire. + #[tokio::test] + async fn broadcast_and_reconcile_drops_reservation_on_broadcast_failure() { + use crate::wallet::core::reservations::OutpointReservations; + use dashcore::hashes::Hash; + use dashcore::{OutPoint, Txid}; + + let reservations = OutpointReservations::new(); + let outpoint = OutPoint::new(Txid::from_byte_array([42u8; 32]), 0); + let guard = reservations.reserve(vec![outpoint], None).expect("reserve"); + assert!(reservations.contains(&outpoint)); + + let broadcaster: Arc = Arc::new(FailingBroadcaster); + let wm = Arc::new(RwLock::new(WalletManager::< + crate::wallet::platform_wallet::PlatformWalletInfo, + >::new(Network::Testnet))); + let tx = dummy_transaction(); + + let hook_called = Arc::new(AtomicBool::new(false)); + let hook_called_inner = Arc::clone(&hook_called); + + let result = broadcast_and_reconcile( + &wm, + [0u8; 32], + &broadcaster, + &tx, + guard, + move |_info, _txid, _reconciled| { + hook_called_inner.store(true, Ordering::SeqCst); + }, + ) + .await; + + assert!( + matches!(result, Err(PlatformWalletError::TransactionBroadcast(_))), + "broadcast failure must propagate to the caller; got: {result:?}" + ); + assert!( + !hook_called.load(Ordering::SeqCst), + "on_reconcile must NOT be invoked when the broadcast itself fails" + ); + assert!( + !reservations.contains(&outpoint), + "broadcast failure must release the reservation so a retry can succeed" + ); + } + + /// When the wallet is absent from the manager post-broadcast (stale + /// handle), the hook must NOT fire and the reservation must be leaked + /// (held until process restart) so a concurrent caller cannot + /// re-select the same already-broadcast outpoint. + #[tokio::test] + async fn broadcast_and_reconcile_leaks_when_wallet_missing() { + use crate::wallet::core::reservations::OutpointReservations; + use dashcore::hashes::Hash; + use dashcore::{OutPoint, Txid}; + + let reservations = OutpointReservations::new(); + let outpoint = OutPoint::new(Txid::from_byte_array([99u8; 32]), 0); + let guard = reservations.reserve(vec![outpoint], None).expect("reserve"); + + // OK-broadcasting mock so we reach the post-broadcast write lock. + let broadcaster: Arc = Arc::new(MockBroadcaster::new( + BroadcastOutcome::Ok(dummy_transaction().txid()), + )); + // Empty wallet manager — `get_wallet_mut_and_info_mut` returns `None`. + let wm = Arc::new(RwLock::new(WalletManager::< + crate::wallet::platform_wallet::PlatformWalletInfo, + >::new(Network::Testnet))); + let tx = dummy_transaction(); + + let hook_called = Arc::new(AtomicBool::new(false)); + let hook_called_inner = Arc::clone(&hook_called); + + let result = broadcast_and_reconcile( + &wm, + [0u8; 32], + &broadcaster, + &tx, + guard, + move |_info, _txid, _reconciled| { + hook_called_inner.store(true, Ordering::SeqCst); + }, + ) + .await + .expect("broadcast succeeded; missing wallet is a log-only branch"); + assert_eq!( + result, + tx.txid(), + "helper must surface the broadcaster's Txid" + ); + assert!( + !hook_called.load(Ordering::SeqCst), + "hook must not fire when the wallet handle is stale" + ); + assert!( + reservations.contains(&outpoint), + "missing-wallet post-broadcast must leak the reservation \ + (concurrent re-selection of an already-broadcast outpoint \ + would race the network)" + ); + } + + /// End-to-end through `send_to_addresses` with the full funded + /// fixture: `is_relevant = true` post-broadcast → hook receives + /// `true` and the reservation is released. Pins the success path of + /// the shared helper as exercised by the core call site. + #[tokio::test] + async fn send_to_addresses_success_invokes_hook_and_releases_reservation() { + let (wm, wallet_id, recipient, signer) = build_funded_wallet_manager(2_000_000); + let broadcaster: Arc = Arc::new(MockBroadcaster::new( + BroadcastOutcome::Ok(dummy_transaction().txid()), + )); + let core = make_core_wallet_for_manager(wm, wallet_id, broadcaster); + + let outpoint = OutPoint::new(Txid::from_byte_array([7u8; 32]), 0); + let outputs = vec![(recipient.clone(), 100_000)]; + + let result = core + .send_to_addresses(StandardAccountType::BIP44Account, 0, outputs, &signer) + .await; + assert!( + result.is_ok(), + "happy-path send must succeed; got: {result:?}" + ); + + assert!( + !core.reservations.contains(&outpoint), + "successful send + relevant tx must release the reservation" + ); + } +} diff --git a/packages/rs-platform-wallet/src/wallet/core/mod.rs b/packages/rs-platform-wallet/src/wallet/core/mod.rs index 106a4108c22..48fcfbfd8d8 100644 --- a/packages/rs-platform-wallet/src/wallet/core/mod.rs +++ b/packages/rs-platform-wallet/src/wallet/core/mod.rs @@ -1,6 +1,7 @@ pub mod balance; pub mod balance_handler; -mod broadcast; +pub(crate) mod broadcast; +pub(crate) mod reservations; pub mod wallet; pub use balance::WalletBalance; diff --git a/packages/rs-platform-wallet/src/wallet/core/reservations.rs b/packages/rs-platform-wallet/src/wallet/core/reservations.rs new file mode 100644 index 00000000000..2cf0586b735 --- /dev/null +++ b/packages/rs-platform-wallet/src/wallet/core/reservations.rs @@ -0,0 +1,488 @@ +//! Generic key reservation set with an RAII guard, plus the +//! [`OutpointReservations`] wrapper used by the platform-wallet +//! UTXO race-protection path. +//! +//! [`OutpointReservations`] closes the same-UTXO concurrent-selection +//! race in [`CoreWallet::send_to_addresses`](super::broadcast). It +//! composes a generic `Reservations` with a parallel +//! `Reservations
` tracking change addresses peeked but not yet +//! committed to a confirmed broadcast. +//! +//! Both inner sets share the [`Reservations`] core: a single +//! `Mutex`-protected `HashSet` with an RAII [`ReservationGuard`] +//! that releases keys on drop, with `release_after_commit` and +//! `leak_until_sync` escape hatches mirroring the broadcast-success / +//! unrecoverable-leak distinction. + +use std::collections::HashSet; +use std::hash::Hash; +use std::sync::{Arc, Mutex}; + +use dashcore::{Address, OutPoint}; + +/// Inner shared state for [`Reservations`]. Held behind a `Mutex` so +/// reserve / release commit atomically. +#[derive(Debug)] +struct ReservationsInner { + keys: HashSet, +} + +impl Default for ReservationsInner { + fn default() -> Self { + Self { + keys: HashSet::new(), + } + } +} + +/// Generic per-wallet set of keys reserved by an in-flight operation but +/// not yet committed to durable wallet state. Cheaply cloneable: holds +/// an `Arc>`. All clones share the same set. +/// +/// `K` must be hashable and cloneable (the guard stores its own copy of +/// the reserved keys to release on drop) and `Debug` so the guard prints +/// usefully in `tracing` events. +#[derive(Debug)] +pub(crate) struct Reservations { + inner: Arc>>, +} + +impl Default for Reservations { + fn default() -> Self { + Self { + inner: Arc::new(Mutex::new(ReservationsInner::default())), + } + } +} + +impl Clone for Reservations { + fn clone(&self) -> Self { + Self { + inner: Arc::clone(&self.inner), + } + } +} + +impl Reservations { + /// Test whether `key` is currently reserved. Only used by tests and + /// the [`OutpointReservations`] wrapper's `#[cfg(test)]` accessors — + /// production paths use [`snapshot`](Self::snapshot) to filter many + /// candidates under one lock acquisition. + #[cfg(test)] + pub(crate) fn contains(&self, key: &K) -> bool { + let guard = self + .inner + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + guard.keys.contains(key) + } + + /// Clone the current reservation set under a single lock acquisition. + /// Callers filter spendable candidates against the returned snapshot + /// to avoid one mutex lock per candidate. + pub(crate) fn snapshot(&self) -> HashSet { + let guard = self + .inner + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + guard.keys.clone() + } + + /// Reserve `keys` atomically, returning an RAII guard that releases + /// them on drop. The guard must be held until the operation outcome + /// has been reconciled into durable wallet state. + /// + /// All-or-nothing: if **any** requested key is already reserved by a + /// live guard, nothing is inserted and `None` is returned. This is the + /// exclusivity invariant of the race-guard — two guards can never cover + /// the same key, so the first to release cannot momentarily report a + /// still-owned key as free. + pub(crate) fn reserve(&self, keys: Vec) -> Option> { + { + let mut guard = self + .inner + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + if keys.iter().any(|k| guard.keys.contains(k)) { + return None; + } + for k in &keys { + guard.keys.insert(k.clone()); + } + } + Some(ReservationGuard { + reservations: Arc::clone(&self.inner), + keys, + released: false, + }) + } +} + +/// RAII guard releasing reservations on drop. +/// +/// Drop is infallible and panic-safe — the underlying `Mutex` is +/// recovered from poisoning so a panicking caller still releases its +/// keys. +#[must_use = "dropping the guard immediately releases the reservation"] +pub(crate) struct ReservationGuard { + reservations: Arc>>, + keys: Vec, + /// Set after a successful `release_after_commit` so `Drop` is a no-op. + released: bool, +} + +impl ReservationGuard { + /// Release the reserved keys now, marking the guard inert so its + /// `Drop` is a no-op. Called by the caller path after the wallet + /// state has transitioned the keys from "reserved" to "committed" + /// (e.g., an outpoint marked spent, an address marked used). + /// + /// The deliberate release point exists so the same code path that + /// *succeeded* the operation also relinquishes the reservation — + /// separating it from the panic/drop path keeps post-failure + /// handling on the implicit `Drop` branch. + pub(crate) fn release_after_commit(mut self) { + self.do_release(); + self.released = true; + } + + /// Keep the reservation held until the process exits. + /// + /// Use this when the operation succeeded but wallet state could not + /// be reconciled, OR when the keys must remain reserved across an + /// asynchronous follow-up event (chain sync, balance flip) that + /// will eventually commit them out-of-band. + /// + /// **No in-process reclaim path exists today.** Keys stay pinned in + /// [`Reservations`] for the lifetime of the holding manager; only + /// a process restart (which drops the whole reservations set) + /// releases them. + /// + /// TODO: a periodic-sync-driven + /// `Reservations::reclaim_committed(&[K])` API would let the next + /// confirmation / balance-sync pass reconcile leaked reservations + /// without a restart. Tracked as a follow-up in #3770. + pub(crate) fn leak_until_sync(self) { + // `mem::forget` skips `Drop`, leaving the reservation entries + // in the shared set. The guard's only owned heap allocation is + // the `Vec`, which is small and never freed for the lifetime + // of the process — acceptable given this is a rare error / async + // commit branch. + std::mem::forget(self); + } + + fn do_release(&mut self) { + let mut inner = self + .reservations + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + for k in &self.keys { + inner.keys.remove(k); + } + } +} + +impl Drop for ReservationGuard { + fn drop(&mut self) { + if self.released { + return; + } + self.do_release(); + } +} + +// ===================================================================== +// Domain wrapper +// ===================================================================== + +/// Per-wallet set of outpoints reserved by an in-flight +/// `send_to_addresses` plus any change addresses peeked but not yet +/// reconciled with a confirmed broadcast. Cheaply cloneable. +/// +/// Composes a [`Reservations`] (outpoint slot) with a +/// [`Reservations
`] (pending change address). Two distinct +/// generic instances are used rather than one keyed by an enum because +/// the call-sites filter on each independently (`snapshot` returns just +/// the outpoints, `pending_change_snapshot` just the addresses) and the +/// two sets never overlap. Holding them under separate mutexes is safe +/// — `reserve` always takes them in `(outpoints, change)` order, the +/// only acquisition order in the module. +#[derive(Debug, Default, Clone)] +pub(crate) struct OutpointReservations { + outpoints: Reservations, + pending_change: Reservations
, +} + +impl OutpointReservations { + pub(crate) fn new() -> Self { + Self::default() + } + + /// Test whether `outpoint` is currently reserved. + #[cfg(test)] + pub(crate) fn contains(&self, outpoint: &OutPoint) -> bool { + self.outpoints.contains(outpoint) + } + + /// Test whether a change address is currently pending. + #[cfg(test)] + pub(crate) fn change_address_pending(&self, addr: &Address) -> bool { + self.pending_change.contains(addr) + } + + /// Clone the current outpoint reservation set under a single lock + /// acquisition. + pub(crate) fn snapshot(&self) -> HashSet { + self.outpoints.snapshot() + } + + /// Clone the current pending-change-address set so callers can skip + /// past in-flight peeks without holding the reservation mutex. + pub(crate) fn pending_change_snapshot(&self) -> HashSet
{ + self.pending_change.snapshot() + } + + /// Reserve `outpoints` and (optionally) a chosen change address + /// atomically across both sub-sets, returning an RAII guard that + /// releases both on drop. The guard must be held until the broadcast + /// outcome is reconciled into wallet state. + /// + /// All-or-nothing across both sub-sets: returns `None` if any outpoint + /// is already reserved, or if the change address is already pending. On + /// the change-address rejection path the already-acquired outpoint + /// reservation is rolled back (released) before returning, so no + /// partial state ever leaks. + pub(crate) fn reserve( + &self, + outpoints: Vec, + change_address: Option
, + ) -> Option { + let outpoint_guard = self.outpoints.reserve(outpoints)?; + let change_guard = match change_address { + Some(addr) => match self.pending_change.reserve(vec![addr]) { + Some(g) => Some(g), + // Roll back the outpoint reservation: dropping `outpoint_guard` + // here releases it before we return, so neither sub-set keeps + // partial state. + None => return None, + }, + None => None, + }; + Some(OutpointReservationGuard { + outpoint_guard, + change_guard, + }) + } +} + +/// RAII guard releasing outpoint + (optional) change-address +/// reservations together. Mirrors the `release_after_commit` / +/// `leak_until_sync` API of the generic guard but composes two inner +/// guards under one type so callers don't have to thread both. +#[must_use = "dropping the guard immediately releases the reservation"] +pub(crate) struct OutpointReservationGuard { + outpoint_guard: ReservationGuard, + change_guard: Option>, +} + +impl OutpointReservationGuard { + /// Release outpoints and any pending change index now, marking the + /// guard inert so its `Drop` is a no-op. Called by the broadcast + /// path after `check_core_transaction` has transitioned the inputs + /// from "reserved" to "spent" and the change index has been + /// committed via `next_change_address(..., true)`. + pub(crate) fn release_after_commit(self) { + self.outpoint_guard.release_after_commit(); + if let Some(g) = self.change_guard { + g.release_after_commit(); + } + } + + /// Keep the reservation held until the process exits. See + /// [`ReservationGuard::leak_until_sync`] for the rationale. + pub(crate) fn leak_until_sync(self) { + self.outpoint_guard.leak_until_sync(); + if let Some(g) = self.change_guard { + g.leak_until_sync(); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use dashcore::hashes::Hash; + use dashcore::Txid; + + fn op(n: u32) -> OutPoint { + OutPoint::new(Txid::all_zeros(), n) + } + + fn addr(byte: u8) -> Address { + use dashcore::secp256k1::{PublicKey, Secp256k1, SecretKey}; + let secp = Secp256k1::new(); + let sk = SecretKey::from_slice(&[byte; 32]).expect("valid sk"); + let pk = PublicKey::from_secret_key(&secp, &sk); + let cpk = dashcore::PublicKey::new(pk); + Address::p2pkh(&cpk, dashcore::Network::Testnet) + } + + #[test] + fn reserve_then_drop_releases() { + let res = OutpointReservations::new(); + let a = op(1); + { + let _g = res.reserve(vec![a], None).expect("disjoint reserve"); + assert!(res.contains(&a)); + } + assert!(!res.contains(&a)); + } + + #[test] + fn second_reservation_is_disjoint() { + let res = OutpointReservations::new(); + let a = op(1); + let b = op(2); + let _g1 = res.reserve(vec![a], None).expect("first reserve"); + let _g2 = res.reserve(vec![b], None).expect("disjoint reserve"); + assert!(res.contains(&a)); + assert!(res.contains(&b)); + } + + #[test] + fn poisoned_mutex_still_releases() { + let res = OutpointReservations::new(); + let a = op(7); + let res_clone = res.clone(); + let _ = std::thread::spawn(move || { + let _g = res_clone.reserve(vec![a], None).expect("reserve"); + panic!("intentional"); + }) + .join(); + // Guard dropped during unwind — outpoint must be released even + // though the mutex was poisoned. + assert!(!res.contains(&a)); + } + + #[test] + fn change_address_reserved_and_released_on_drop() { + let res = OutpointReservations::new(); + let ch = addr(0x42); + { + let _g = res + .reserve(vec![op(1)], Some(ch.clone())) + .expect("reserve with change"); + assert!(res.change_address_pending(&ch)); + } + assert!(!res.change_address_pending(&ch)); + } + + #[test] + fn pending_change_snapshot_reflects_reservations() { + let res = OutpointReservations::new(); + let ch1 = addr(0x11); + let ch2 = addr(0x22); + let _g1 = res + .reserve(vec![op(1)], Some(ch1.clone())) + .expect("first reserve"); + let _g2 = res + .reserve(vec![op(2)], Some(ch2.clone())) + .expect("second reserve"); + let snap = res.pending_change_snapshot(); + assert!(snap.contains(&ch1)); + assert!(snap.contains(&ch2)); + } + + #[test] + fn release_after_commit_is_drop_noop() { + let res = OutpointReservations::new(); + let a = op(11); + let ch = addr(0x55); + let g = res + .reserve(vec![a], Some(ch.clone())) + .expect("reserve with change"); + assert!(res.contains(&a)); + assert!(res.change_address_pending(&ch)); + g.release_after_commit(); + assert!(!res.contains(&a)); + assert!(!res.change_address_pending(&ch)); + } + + #[test] + fn leak_until_sync_keeps_reservation_held() { + let res = OutpointReservations::new(); + let a = op(13); + let g = res.reserve(vec![a], None).expect("reserve"); + g.leak_until_sync(); + assert!( + res.contains(&a), + "leak_until_sync must keep the outpoint reserved until process restart" + ); + } + + #[test] + fn reserve_rejects_overlapping_key_and_inserts_nothing() { + // A live guard owns `a`. A second reserve of {a, b} must reject + // atomically: `b` must NOT be inserted, leaving the set unchanged. + let res: Reservations = Reservations::default(); + let a = op(1); + let b = op(2); + let _held = res.reserve(vec![a]).expect("first reserve"); + + assert!( + res.reserve(vec![a, b]).is_none(), + "reserve must reject when any key overlaps a live guard" + ); + assert!(res.contains(&a), "the originally held key stays reserved"); + assert!( + !res.contains(&b), + "the non-overlapping key must NOT leak in on a rejected reserve" + ); + } + + #[test] + fn reserve_disjoint_succeeds_and_releases_all_on_drop() { + let res: Reservations = Reservations::default(); + let a = op(1); + let b = op(2); + { + let _g = res.reserve(vec![a, b]).expect("disjoint reserve"); + assert!(res.contains(&a)); + assert!(res.contains(&b)); + } + assert!(!res.contains(&a), "drop releases every reserved key"); + assert!(!res.contains(&b), "drop releases every reserved key"); + } + + #[test] + fn composed_reserve_rolls_back_outpoints_when_change_is_taken() { + // Change address `ch` is already pending under a live guard. A + // composed reserve that wants fresh outpoints PLUS `ch` must reject + // the whole call AND roll back the outpoint reservation it took + // first — no partial state may leak. + let res = OutpointReservations::new(); + let ch = addr(0x42); + let _holds_change = res + .reserve(vec![op(99)], Some(ch.clone())) + .expect("seed the pending change address"); + + let a = op(1); + let b = op(2); + assert!( + res.reserve(vec![a, b], Some(ch.clone())).is_none(), + "composed reserve must reject when the change address is taken" + ); + assert!( + !res.contains(&a), + "rollback: outpoint `a` must be released after change-address rejection" + ); + assert!( + !res.contains(&b), + "rollback: outpoint `b` must be released after change-address rejection" + ); + assert!( + res.change_address_pending(&ch), + "the originally pending change address stays held by its own guard" + ); + } +} diff --git a/packages/rs-platform-wallet/src/wallet/core/wallet.rs b/packages/rs-platform-wallet/src/wallet/core/wallet.rs index 8e83fd6b947..1476f0ac45b 100644 --- a/packages/rs-platform-wallet/src/wallet/core/wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/core/wallet.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use super::balance::WalletBalance; +use super::reservations::OutpointReservations; use dashcore::Address as DashAddress; use tokio::sync::RwLock; @@ -31,6 +32,10 @@ pub struct CoreWallet { pub(crate) broadcaster: Arc, /// Lock-free balance for UI reads. balance: Arc, + /// Outpoints currently reserved by an in-flight `send_to_addresses` + /// call on this handle. Closes the same-UTXO concurrent-selection + /// race — see [`super::reservations`]. + pub(crate) reservations: OutpointReservations, } impl CoreWallet { @@ -47,6 +52,7 @@ impl CoreWallet { wallet_id, broadcaster, balance, + reservations: OutpointReservations::new(), } } @@ -252,6 +258,7 @@ impl Clone for CoreWallet { wallet_id: self.wallet_id, broadcaster: Arc::clone(&self.broadcaster), balance: Arc::clone(&self.balance), + reservations: self.reservations.clone(), } } } diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs b/packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs index 881a9a73890..953955c7c23 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs @@ -315,6 +315,13 @@ pub struct IdentityWallet { /// `SpvBroadcaster`-pinned, while this one picks the broadcaster /// used by `send_payment` (static dispatch per call). pub(crate) broadcaster: Arc, + /// Shared outpoint reservation set (cloned from the sibling + /// [`crate::CoreWallet`]). DashPay `send_payment` and core + /// `send_to_addresses` both fund from the same BIP-44 account 0 + /// UTXOs, so they must consult the same reservation set to avoid + /// the same-UTXO concurrent-selection race. See + /// [`crate::wallet::core::reservations`]. + pub(crate) reservations: crate::wallet::core::reservations::OutpointReservations, } // Manual `Debug`: the derive would require `B: Debug`, which is not part @@ -337,6 +344,7 @@ impl Clone for IdentityWallet { asset_locks: Arc::clone(&self.asset_locks), persister: self.persister.clone(), broadcaster: Arc::clone(&self.broadcaster), + reservations: self.reservations.clone(), } } } diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs index c135e04e6fc..2b398c9035b 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -103,6 +103,9 @@ impl IdentityWallet { ), PlatformWalletError, > { + use std::collections::BTreeSet; + + use dashcore::OutPoint; use key_wallet::account::account_collection::DashpayAccountKey; use key_wallet::wallet::managed_wallet_info::coin_selection::SelectionStrategy; use key_wallet::wallet::managed_wallet_info::transaction_builder::TransactionBuilder; @@ -110,16 +113,14 @@ impl IdentityWallet { let account_index: u32 = 0; - let (payment_address, tx) = { + // Build, sign, reserve — all under one write-lock acquisition. Mirrors + // `CoreWallet::send_to_addresses` so concurrent calls between this and + // core `send_to_addresses` cannot select the same UTXO (#3585). + let (payment_address, tx, _reservation) = { let mut wm = self.wallet_manager.write().await; // Resolve the external account's xpub so we can derive addresses. let contact_xpub = { - // Look up the external account in the *immutable* AccountCollection on - // `Wallet`. The ManagedAccountCollection only stores the managed state; - // the xpub lives on the immutable Account in `wallet.accounts`. - // For a watch-only external account we stored the contact's xpub directly - // as `account_xpub` on the Account struct — look it up via DashpayAccountKey. let wallet = wm.get_wallet(&self.wallet_id).ok_or_else(|| { PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)) })?; @@ -145,7 +146,74 @@ impl IdentityWallet { .get_wallet_and_info_mut(&self.wallet_id) .ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))?; - // Derive the next unused address from the external account's address pool. + // Resolve the funding-account xpub up front so we can advance the + // change-address derivation under the same lock. + let funding_xpub = wallet + .accounts + .standard_bip44_accounts + .get(&0) + .map(|a| a.account_xpub) + .ok_or_else(|| { + PlatformWalletError::TransactionBuild( + "BIP-44 account 0 not found in wallet".to_string(), + ) + })?; + + let current_height = info.core_wallet.synced_height(); + + let managed_account = info + .core_wallet + .accounts + .standard_bip44_accounts + .get_mut(&0) + .ok_or_else(|| { + PlatformWalletError::TransactionBuild( + "BIP-44 managed account 0 not found".to_string(), + ) + })?; + + // Snapshot spendable UTXOs minus any in-flight reservations from + // a concurrent `send_to_addresses`/`send_payment` on this wallet. + let reserved = self.reservations.snapshot(); + let spendable: Vec<_> = managed_account + .spendable_utxos(current_height) + .into_iter() + .filter(|utxo| !reserved.contains(&utxo.outpoint)) + .cloned() + .collect(); + if spendable.is_empty() { + return Err(PlatformWalletError::NoSpendableInputs { + account_index, + account_type: + key_wallet::account::account_type::StandardAccountType::BIP44Account, + context: "all UTXOs used or reserved by in-flight transactions".to_string(), + }); + } + + // Pick a change address that no concurrent send has already + // peeked. Commit the advance inside this write lock so the + // privacy property holds even if broadcast fails later (one + // burned index per failure is acceptable; reuse is not). + let pending_change = self.reservations.pending_change_snapshot(); + let change_addr = loop { + let peeked = managed_account + .next_change_address(Some(&funding_xpub), false) + .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; + if !pending_change.contains(&peeked) { + let _ = managed_account + .next_change_address(Some(&funding_xpub), true) + .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; + break peeked; + } + let _ = managed_account + .next_change_address(Some(&funding_xpub), true) + .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; + }; + + // Derive the recipient's payment address from the external pool. + // Done *after* the change-address pick so a derivation failure + // doesn't leave a committed funding-side change advance dangling + // without a matching outpoint reservation. let key = DashpayAccountKey { index: account_index, user_identity_id: from_identity_id.to_buffer(), @@ -162,13 +230,12 @@ impl IdentityWallet { to_contact_id )) })?; - let payment_address = external_account .next_address(Some(&contact_xpub), true) .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; - let current_height = info.core_wallet.synced_height(); - + // Re-borrow the managed funding account for the builder (the + // external_account borrow above ended at `payment_address`). let managed_account = info .core_wallet .accounts @@ -179,20 +246,12 @@ impl IdentityWallet { "BIP-44 managed account 0 not found".to_string(), ) })?; - let account = wallet - .accounts - .standard_bip44_accounts - .get(&0) - .ok_or_else(|| { - PlatformWalletError::TransactionBuild( - "BIP-44 account 0 not found in wallet".to_string(), - ) - })?; let builder = TransactionBuilder::new() .set_current_height(current_height) .set_selection_strategy(SelectionStrategy::LargestFirst) - .set_funding(managed_account, account) + .set_change_address(change_addr.clone()) + .add_inputs(spendable.iter().cloned()) .add_output(&payment_address, amount_duffs); let (tx, _fee) = builder @@ -200,17 +259,84 @@ impl IdentityWallet { managed_account.address_derivation_path(&addr) }) .await - .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; + .map_err(|e| { + crate::wallet::core::broadcast::classify_build_error( + e, + key_wallet::account::account_type::StandardAccountType::BIP44Account, + account_index, + ) + })?; + + // Defense-in-depth: confirm the builder picked only outpoints + // from our pre-filtered spendable snapshot. + let selected: BTreeSet = + tx.input.iter().map(|txin| txin.previous_output).collect(); + let spendable_outpoints: BTreeSet = + spendable.iter().map(|utxo| utxo.outpoint).collect(); + if !selected.is_subset(&spendable_outpoints) { + return Err(PlatformWalletError::ConcurrentSpendConflict { + selected: selected.into_iter().collect(), + }); + } - (payment_address, tx) + // Second defense-in-depth net, in parity with + // `CoreWallet::send_to_addresses`: re-snapshot the funding + // account's spendable UTXOs after `build_signed` and confirm + // every selected outpoint is still present. See + // [`crate::wallet::core::broadcast::assert_selected_still_spendable`]. + let fresh_spendable_outpoints: BTreeSet = managed_account + .spendable_utxos(current_height) + .into_iter() + .map(|utxo| utxo.outpoint) + .collect(); + crate::wallet::core::broadcast::assert_selected_still_spendable( + &selected, + &fresh_spendable_outpoints, + )?; + + // Atomic reject-overlap: `reserve` returns `None` if a concurrent + // caller grabbed any selected outpoint or the change address between + // our prefilter and here. Unreachable today (both filters run under + // this same write lock), but handled defensively as the retryable + // concurrent-spend conflict the prefilter would otherwise surface. + let reservation = self + .reservations + .reserve(selected.iter().copied().collect(), Some(change_addr)) + .ok_or_else(|| PlatformWalletError::ConcurrentSpendConflict { + selected: selected.iter().copied().collect(), + })?; + + (payment_address, tx, reservation) }; - // --- 3. Broadcast the transaction. --- - let txid = self - .broadcaster - .broadcast(&tx) - .await - .map_err(|e| PlatformWalletError::TransactionBroadcast(e.to_string()))?; + // Broadcast + reconcile via the shared post-broadcast helper. + // The hook records the outgoing `PaymentEntry` on the sender's + // `ManagedIdentity` inside the same write lock the reconcile uses + // — keeping the entry recording in the same critical section + // ensures it cannot race a concurrent state mutation between + // `check_core_transaction` and `record_dashpay_payment`. + let entry = crate::wallet::identity::types::dashpay::payment::PaymentEntry::new_sent( + *to_contact_id, + amount_duffs, + memo, + ); + let entry_for_hook = entry.clone(); + let persister = self.persister.clone(); + let from_identity = *from_identity_id; + + let txid = crate::wallet::core::broadcast::broadcast_and_reconcile( + &self.wallet_manager, + self.wallet_id, + &self.broadcaster, + &tx, + _reservation, + move |info, txid, _reconciled| { + if let Some(managed) = info.identity_manager.managed_identity_mut(&from_identity) { + managed.record_dashpay_payment(txid.to_string(), entry_for_hook, &persister); + } + }, + ) + .await?; tracing::info!( from_identity = %from_identity_id, @@ -221,26 +347,440 @@ impl IdentityWallet { "DashPay payment broadcast" ); - // --- 4. Record the outgoing payment on the sender's ManagedIdentity. --- - let entry = crate::wallet::identity::types::dashpay::payment::PaymentEntry::new_sent( - *to_contact_id, - amount_duffs, - memo, + Ok((txid, entry)) + } +} + +#[cfg(test)] +mod tests { + use std::collections::BTreeMap; + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + + use async_trait::async_trait; + use dashcore::hashes::Hash; + use dashcore::{OutPoint, Transaction, Txid}; + use dpp::identity::v0::IdentityV0; + use dpp::identity::Identity; + use dpp::prelude::Identifier; + use key_wallet::account::account_type::AccountType; + use key_wallet::wallet::initialization::WalletAccountCreationOptions; + use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface as _; + use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; + use key_wallet::wallet::Wallet; + use key_wallet::{Network, Utxo}; + use key_wallet_manager::WalletManager; + use tokio::sync::{Notify, RwLock}; + + use super::*; + use crate::broadcaster::TransactionBroadcaster; + use crate::error::PlatformWalletError; + use crate::wallet::core::balance::WalletBalance; + use crate::wallet::core::reservations::OutpointReservations; + use crate::wallet::identity::types::dashpay::payment::PaymentDirection; + use crate::wallet::identity::IdentityManager; + use crate::wallet::persister::{NoPlatformPersistence, WalletPersister}; + use crate::wallet::platform_wallet::{PlatformWalletInfo, WalletId}; + + /// The single seeded UTXO's outpoint — fixed so assertions can name it. + fn funding_outpoint() -> OutPoint { + OutPoint::new(Txid::from_byte_array([7u8; 32]), 0) + } + + /// In-memory signer backed by the test wallet's mnemonic root, so + /// `build_signed` can sign the seeded UTXO. + struct InMemorySigner { + root: key_wallet::wallet::root_extended_keys::RootExtendedPrivKey, + network: Network, + } + + const IN_MEMORY_METHODS: &[key_wallet::signer::SignerMethod] = + &[key_wallet::signer::SignerMethod::Digest]; + + #[async_trait] + impl key_wallet::signer::Signer for InMemorySigner { + type Error = String; + + fn supported_methods(&self) -> &[key_wallet::signer::SignerMethod] { + IN_MEMORY_METHODS + } + + async fn sign_ecdsa( + &self, + path: &key_wallet::bip32::DerivationPath, + sighash: [u8; 32], + ) -> Result< + ( + dashcore::secp256k1::ecdsa::Signature, + dashcore::secp256k1::PublicKey, + ), + Self::Error, + > { + let secp = dashcore::secp256k1::Secp256k1::new(); + let xpriv = self + .root + .to_extended_priv_key(self.network) + .derive_priv(&secp, path) + .map_err(|e| e.to_string())?; + let msg = dashcore::secp256k1::Message::from_digest(sighash); + let sig = secp.sign_ecdsa(&msg, &xpriv.private_key); + let pk = dashcore::secp256k1::PublicKey::from_secret_key(&secp, &xpriv.private_key); + Ok((sig, pk)) + } + + async fn public_key( + &self, + path: &key_wallet::bip32::DerivationPath, + ) -> Result { + let secp = dashcore::secp256k1::Secp256k1::new(); + let xpriv = self + .root + .to_extended_priv_key(self.network) + .derive_priv(&secp, path) + .map_err(|e| e.to_string())?; + Ok(dashcore::secp256k1::PublicKey::from_secret_key( + &secp, + &xpriv.private_key, + )) + } + } + + fn root_from(wallet: &Wallet) -> key_wallet::wallet::root_extended_keys::RootExtendedPrivKey { + match &wallet.wallet_type { + key_wallet::wallet::WalletType::Mnemonic { + root_extended_private_key, + .. + } => root_extended_private_key.clone(), + _ => unreachable!("test wallets are mnemonic"), + } + } + + /// Mock broadcaster that gates `broadcast()` on an external `Notify`. + /// `entered` fires the instant `broadcast()` is awaited — by then the + /// caller has reserved its outpoint and dropped the wallet write lock. + struct GatedBroadcaster { + gate: Arc, + entered: Arc, + calls: AtomicUsize, + } + + #[async_trait] + impl TransactionBroadcaster for GatedBroadcaster { + async fn broadcast(&self, transaction: &Transaction) -> Result { + self.calls.fetch_add(1, Ordering::SeqCst); + self.entered.notify_one(); + self.gate.notified().await; + Ok(transaction.txid()) + } + } + + /// Build a single-wallet manager with one funded BIP-44 account 0, + /// a registered watch-only `DashpayExternalAccount` for the + /// `(from_identity, to_contact)` pair, and a sending `ManagedIdentity`. + /// Returns the manager handle, wallet id, the two identity ids, and a + /// signer that owns the seeded UTXO. + fn build_dashpay_wallet( + utxo_value: u64, + ) -> ( + Arc>>, + WalletId, + Identifier, + Identifier, + InMemorySigner, + ) { + let from_identity = Identifier::from([0xA1; 32]); + let to_contact = Identifier::from([0xB2; 32]); + + let wallet = Wallet::new_random(Network::Testnet, WalletAccountCreationOptions::Default) + .expect("test wallet"); + let signer = InMemorySigner { + root: root_from(&wallet), + network: Network::Testnet, + }; + + // Reuse the funding account's xpub as the contact's watch-only + // xpub: `send_payment` only derives a payment address from it, and + // the test asserts plumbing, not contact-key provenance. + let contact_xpub = wallet + .accounts + .standard_bip44_accounts + .get(&0) + .expect("bip44 account 0") + .account_xpub; + + let mut wallet = wallet; + let dashpay_account_type = AccountType::DashpayExternalAccount { + index: 0, + user_identity_id: from_identity.to_buffer(), + friend_identity_id: to_contact.to_buffer(), + }; + wallet + .add_account(dashpay_account_type, Some(contact_xpub)) + .expect("register watch-only dashpay external account"); + + let xpub = wallet + .accounts + .standard_bip44_accounts + .get(&0) + .expect("bip44 account 0") + .account_xpub; + + let mut wallet_info = ManagedWalletInfo::from_wallet(&wallet, 0); + wallet_info.update_synced_height(100); + + // Mirror the watch-only managed account so the external pool is + // available for address derivation, exactly as + // `register_external_contact_account` does. + let dashpay_account = key_wallet::Account { + parent_wallet_id: None, + account_type: dashpay_account_type, + network: Network::Testnet, + account_xpub: contact_xpub, + is_watch_only: true, + }; + wallet_info + .accounts + .insert_funds_bearing_account( + key_wallet::managed_account::ManagedCoreFundsAccount::from_account( + &dashpay_account, + ), + ) + .expect("insert watch-only managed dashpay account"); + + let funding_address = wallet_info + .accounts + .standard_bip44_accounts + .get_mut(&0) + .expect("managed bip44 account 0") + .next_receive_address(Some(&xpub), true) + .expect("derive funding address"); + + let outpoint = funding_outpoint(); + let mut utxo = Utxo::new( + outpoint, + dashcore::TxOut { + value: utxo_value, + script_pubkey: funding_address.script_pubkey(), + }, + funding_address, + 1, + false, ); + utxo.is_confirmed = true; + wallet_info + .accounts + .standard_bip44_accounts + .get_mut(&0) + .expect("managed bip44 account 0") + .utxos + .insert(outpoint, utxo); + + let mut identity_manager = IdentityManager::new(); + + let info = PlatformWalletInfo { + core_wallet: wallet_info, + balance: Arc::new(WalletBalance::new()), + identity_manager: IdentityManager::new(), + tracked_asset_locks: BTreeMap::new(), + }; + + let mut wm: WalletManager = WalletManager::new(Network::Testnet); + let wallet_id = wm.insert_wallet(wallet, info).expect("insert wallet"); + + // Add the sending identity through the same persister the wallet + // uses, so `record_dashpay_payment` finds it post-broadcast. + let persister = WalletPersister::new(wallet_id, Arc::new(NoPlatformPersistence)); + identity_manager + .add_identity(make_test_identity(from_identity), 0, wallet_id, &persister) + .expect("add sending identity"); + wm.get_wallet_info_mut(&wallet_id) + .expect("info") + .identity_manager = identity_manager; + + ( + Arc::new(RwLock::new(wm)), + wallet_id, + from_identity, + to_contact, + signer, + ) + } + + fn make_test_identity(id: Identifier) -> Identity { + Identity::V0(IdentityV0 { + id, + public_keys: BTreeMap::new(), + balance: 0, + revision: 0, + }) + } + + /// Construct an `IdentityWallet` wired to a mock broadcaster and a + /// shared reservation set, mirroring how `PlatformWallet::new` wires + /// the identity handle to its sibling `CoreWallet`'s reservations. + fn make_identity_wallet( + wm: Arc>>, + wallet_id: WalletId, + broadcaster: Arc, + reservations: OutpointReservations, + ) -> IdentityWallet { + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let persister = WalletPersister::new(wallet_id, Arc::new(NoPlatformPersistence)); + let event_manager = Arc::new(crate::events::PlatformEventManager::new(vec![])); + let spv = Arc::new(crate::spv::SpvRuntime::new(Arc::clone(&wm), event_manager)); + // `asset_locks` is pinned to `SpvBroadcaster` but is never exercised + // by `send_payment` — the never-started runtime only satisfies the + // type. + let asset_locks = Arc::new(crate::wallet::asset_lock::manager::AssetLockManager::new( + Arc::clone(&sdk), + Arc::clone(&wm), + wallet_id, + Arc::new(Notify::new()), + Arc::new(crate::broadcaster::SpvBroadcaster::new(spv)), + persister.clone(), + )); + + IdentityWallet { + sdk, + wallet_manager: wm, + wallet_id, + asset_locks, + persister, + broadcaster, + reservations, + } + } + + // The post-build fresh-spendable re-snapshot check + // (`assert_selected_still_spendable`, called from `send_payment`) is + // covered as a pure set check by unit tests in `core::broadcast`. A + // path-level test that trips it through `send_payment` is not added + // here: `build_signed` and the re-snapshot run under one held write + // lock, so no in-process task can drop a selected UTXO between them + // without a production-side mutation hook. That's the very condition + // the check is a forward-compat net for. + // TODO: add a send_payment-path trip test once a UTXO mutator can run + // outside the send-flow write lock (the change that makes the check + // reachable). + + /// Happy path: `send_payment` records a Sent `PaymentEntry` on the + /// sending identity, and the outpoint reserved during the build is + /// released once the broadcast reconciles successfully. + #[tokio::test] + async fn send_payment_records_entry_and_releases_reservation() { + let (wm, wallet_id, from_identity, to_contact, _signer) = build_dashpay_wallet(2_000_000); + + let gate = Arc::new(Notify::new()); + let entered = Arc::new(Notify::new()); + let broadcaster = Arc::new(GatedBroadcaster { + gate: Arc::clone(&gate), + entered: Arc::clone(&entered), + calls: AtomicUsize::new(0), + }); + let reservations = OutpointReservations::new(); + let identity = make_identity_wallet( + Arc::clone(&wm), + wallet_id, + Arc::clone(&broadcaster) as Arc, + reservations.clone(), + ); + + let send = identity.clone(); + let handle = tokio::spawn(async move { + send.send_payment(&from_identity, &to_contact, 100_000, Some("lunch".into())) + .await + }); + + // Once the broadcast is entered, the outpoint is reserved and the + // wallet write lock is dropped — observe the reservation held. + entered.notified().await; + assert!( + reservations.contains(&funding_outpoint()), + "the selected outpoint must be reserved while the broadcast is in flight" + ); + + // Let the broadcast complete; the post-broadcast reconcile then + // releases the reservation and records the entry. + gate.notify_one(); + let (_txid, entry) = handle + .await + .expect("task panicked") + .expect("send_payment ok"); + + assert_eq!(entry.direction, PaymentDirection::Sent); + assert_eq!(entry.counterparty_id, to_contact); + assert_eq!(entry.amount_duffs, 100_000); + assert_eq!(entry.memo.as_deref(), Some("lunch")); + + // (a) Entry recorded on the sending identity. { - let mut wm = self.wallet_manager.write().await; - if let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) { - if let Some(managed) = info.identity_manager.managed_identity_mut(from_identity_id) - { - managed.record_dashpay_payment( - txid.to_string(), - entry.clone(), - &self.persister, - ); - } - } + let wm_guard = wm.read().await; + let managed = wm_guard + .get_wallet_info(&wallet_id) + .expect("info") + .identity_manager + .managed_identity(&from_identity) + .expect("sending identity present"); + assert_eq!( + managed.dashpay_payments.len(), + 1, + "exactly one outgoing payment must be recorded on the sender" + ); + let recorded = managed.dashpay_payments.values().next().expect("entry"); + assert_eq!(recorded.direction, PaymentDirection::Sent); + assert_eq!(recorded.counterparty_id, to_contact); } - Ok((txid, entry)) + // (b) Reservation released after a successful reconcile. + assert!( + !reservations.contains(&funding_outpoint()), + "the outpoint reservation must be released after a successful reconcile" + ); + assert_eq!( + broadcaster.calls.load(Ordering::SeqCst), + 1, + "the broadcaster must be called exactly once" + ); + } + + /// (c) When the funding outpoint is already reserved by a concurrent + /// in-flight send (core `send_to_addresses` or another `send_payment`), + /// `send_payment` short-circuits with `NoSpendableInputs` — the + /// race-loser never reaches the broadcaster. + #[tokio::test] + async fn send_payment_loses_utxo_race_with_no_spendable_inputs() { + let (wm, wallet_id, from_identity, to_contact, _signer) = build_dashpay_wallet(2_000_000); + + let broadcaster = Arc::new(GatedBroadcaster { + gate: Arc::new(Notify::new()), + entered: Arc::new(Notify::new()), + calls: AtomicUsize::new(0), + }); + let reservations = OutpointReservations::new(); + let identity = make_identity_wallet( + Arc::clone(&wm), + wallet_id, + Arc::clone(&broadcaster) as Arc, + reservations.clone(), + ); + + // Simulate a concurrent send holding the wallet's only UTXO. + let _guard = reservations + .reserve(vec![funding_outpoint()], None) + .expect("reserve"); + + let result = identity + .send_payment(&from_identity, &to_contact, 100_000, None) + .await; + + assert!( + matches!(result, Err(PlatformWalletError::NoSpendableInputs { .. })), + "the race-loser must short-circuit with NoSpendableInputs; got: {result:?}" + ); + assert_eq!( + broadcaster.calls.load(Ordering::SeqCst), + 0, + "the race-loser must never reach the broadcaster" + ); } } diff --git a/packages/rs-platform-wallet/src/wallet/platform_wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_wallet.rs index 8a95c25d210..bdfbd758651 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_wallet.rs @@ -294,6 +294,9 @@ impl PlatformWallet { asset_locks: Arc::clone(&asset_locks), persister: wallet_persister.clone(), broadcaster: dashpay_broadcaster, + // Shared with `CoreWallet::send_to_addresses` so DashPay + // payments do not race the same UTXO. See #3585. + reservations: core.reservations.clone(), }; let platform = PlatformAddressWallet::new( diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift index 2c311f91e9d..758d10aa8fb 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift @@ -39,6 +39,7 @@ public enum PlatformWalletResultCode: Int32, Sendable { /// outcome. Do NOT auto-retry — a retry would rebuild the bundle and /// could double-execute if the original landed. case errorShieldedSpendUnconfirmed = 18 + case errorConcurrentSpendConflict = 31 case notFound = 98 case errorUnknown = 99 @@ -82,6 +83,8 @@ public enum PlatformWalletResultCode: Int32, Sendable { self = .errorShieldedBroadcastUnconfirmed case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_SHIELDED_SPEND_UNCONFIRMED: self = .errorShieldedSpendUnconfirmed + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_CONCURRENT_SPEND_CONFLICT: + self = .errorConcurrentSpendConflict case PLATFORM_WALLET_FFI_RESULT_CODE_NOT_FOUND: self = .notFound case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_UNKNOWN: @@ -158,6 +161,13 @@ public enum PlatformWalletError: LocalizedError { case deserialization(String) case memoryAllocation(String) case arithmeticOverflow(String) + /// Umbrella for every "can't-select-inputs" wallet variant + /// (`NoSpendableInputs`, `OnlyOutputAddressesFunded`, + /// `OnlyDustInputs`). Mirrors the Rust `ErrorNoSelectableInputs` + /// FFI code; the originating Rust variant's Display rendering is + /// preserved in the associated message so callers can distinguish + /// the underlying cause (including the race-loser breadcrumb on + /// `NoSpendableInputs`). case noSelectableInputs(String) case walletAlreadyExists(String) /// Definitive shielded-broadcast failure: the shielded transition @@ -177,6 +187,11 @@ public enum PlatformWalletError: LocalizedError { /// notes reserved wallet-side (a shield reserves nothing) until the /// next sync reconciles the outcome. Do NOT auto-retry. case shieldedSpendUnconfirmed(String) + /// Transaction builder picked an outpoint another concurrent + /// build had already selected. Retry — the underlying reservation + /// will have cleared. Mirrors the Rust + /// `PlatformWalletError::ConcurrentSpendConflict` variant. + case concurrentSpendConflict(String) case notFound(String) case unknown(String) @@ -192,6 +207,7 @@ public enum PlatformWalletError: LocalizedError { .arithmeticOverflow(let m), .noSelectableInputs(let m), .walletAlreadyExists(let m), .shieldedBroadcastFailed(let m), .shieldedBroadcastUnconfirmed(let m), .shieldedSpendUnconfirmed(let m), + .concurrentSpendConflict(let m), .notFound(let m), .unknown(let m): return m } @@ -222,6 +238,8 @@ public enum PlatformWalletError: LocalizedError { case .errorShieldedBroadcastFailed: self = .shieldedBroadcastFailed(detail) case .errorShieldedBroadcastUnconfirmed: self = .shieldedBroadcastUnconfirmed(detail) case .errorShieldedSpendUnconfirmed: self = .shieldedSpendUnconfirmed(detail) + case .errorConcurrentSpendConflict: + self = .concurrentSpendConflict(detail) case .notFound: self = .notFound(detail) case .errorUnknown: self = .unknown(detail) }