diff --git a/wallet/src/account/mod.rs b/wallet/src/account/mod.rs index 59b3ca04d..3765dc369 100644 --- a/wallet/src/account/mod.rs +++ b/wallet/src/account/mod.rs @@ -84,7 +84,7 @@ use wallet_types::{ use crate::{ SendRequest, WalletError, WalletResult, - account::utxo_selector::{OutputGroup, select_coins}, + account::utxo_selector::{OutputGroup, select_coins_preferring_confirmed}, destination_getters::{HtlcSpendingCondition, get_tx_output_destination}, key_chain::{AccountKeyChains, KeyChainError, VRFAccountKeyChains}, send_request::{ @@ -112,7 +112,7 @@ pub use self::{ utxo_selector::{CoinSelectionAlgo, UtxoSelectorError}, }; -#[derive(Debug)] +#[derive(Debug, Clone, Copy)] pub struct CurrentFeeRate { pub current_fee_rate: FeeRate, pub consolidate_fee_rate: FeeRate, @@ -277,39 +277,58 @@ impl Account { Ok(()) })?; - let (utxos, selection_algo) = if input_utxos.is_empty() { + let (utxos, confirmed_utxos, selection_algo) = if input_utxos.is_empty() { + let utxo_types = UtxoType::Transfer | UtxoType::LockThenTransfer | UtxoType::IssueNft; + let utxos = self.get_utxos( + utxo_types, + median_time, + UtxoState::Confirmed | UtxoState::InMempool | UtxoState::Inactive, + WithLocked::Unlocked, + ); + // Prefer confirmed utxos during selection: spending an unconfirmed utxo forms a + // mempool cluster, which is size limited, so building on top of confirmed utxos + // reduces the chance of producing a tx that the mempool would reject (see issue + // #2066). The confirmed-only set is tried first, with a fallback to the full set. + let confirmed_utxos = self.get_utxos( + utxo_types, + median_time, + UtxoState::Confirmed.into(), + WithLocked::Unlocked, + ); ( - self.get_utxos( - UtxoType::Transfer | UtxoType::LockThenTransfer | UtxoType::IssueNft, - median_time, - UtxoState::Confirmed | UtxoState::InMempool | UtxoState::Inactive, - WithLocked::Unlocked, - ), + utxos, + Some(confirmed_utxos), selection_algo.unwrap_or(CoinSelectionAlgo::Randomize), ) } else { let selection_algo = selection_algo.unwrap_or(CoinSelectionAlgo::UsePreselected); - match input_utxos { + let utxos = match input_utxos { SelectedInputs::Utxos(input_utxos) => { let current_block_info = BlockInfo { height: self.account_info.best_block_height(), timestamp: median_time, }; - ( - self.output_cache.find_utxos(current_block_info, input_utxos)?, - selection_algo, - ) + self.output_cache.find_utxos(current_block_info, input_utxos)? } - SelectedInputs::Inputs(ref inputs) => ( - inputs.iter().map(|(outpoint, utxo)| (outpoint.clone(), utxo)).collect(), - selection_algo, - ), - } + SelectedInputs::Inputs(ref inputs) => { + inputs.iter().map(|(outpoint, utxo)| (outpoint.clone(), utxo)).collect() + } + }; + (utxos, None, selection_algo) }; let current_fee_rate = fee_rates.current_fee_rate; let mut utxos_by_currency = self.utxo_output_groups_by_currency(fee_rates, &pay_fee_with_currency, utxos)?; + let mut confirmed_utxos_by_currency = confirmed_utxos + .map(|confirmed_utxos| { + self.utxo_output_groups_by_currency( + fee_rates, + &pay_fee_with_currency, + confirmed_utxos, + ) + }) + .transpose()?; let amount_to_be_paid_in_currency_with_fees = output_currency_amounts.remove(&pay_fee_with_currency).unwrap_or(Amount::ZERO); @@ -342,6 +361,9 @@ impl Account { .iter() .map(|(currency, output_amount)| -> WalletResult<_> { let utxos = utxos_by_currency.remove(currency).unwrap_or(vec![]); + let confirmed_utxos = confirmed_utxos_by_currency + .as_mut() + .map(|by_currency| by_currency.remove(currency).unwrap_or_default()); let preselected_amount = preselected_inputs .amounts .remove(currency) @@ -354,7 +376,8 @@ impl Account { change_address.clone(), )?; - let selection_result = select_coins( + let selection_result = select_coins_preferring_confirmed( + confirmed_utxos, utxos, output_amount.sub(preselected_amount).unwrap_or(Amount::ZERO), PayFee::DoNotPayFeeWithThisCurrency, @@ -393,6 +416,9 @@ impl Account { .try_collect()?; let utxos = utxos_by_currency.remove(&pay_fee_with_currency).unwrap_or(vec![]); + let confirmed_utxos = confirmed_utxos_by_currency + .as_mut() + .map(|by_currency| by_currency.remove(&pay_fee_with_currency).unwrap_or_default()); let num_inputs = selected_inputs .values() @@ -438,7 +464,8 @@ impl Account { change_address.clone(), )?; - let selection_result = select_coins( + let selection_result = select_coins_preferring_confirmed( + confirmed_utxos, utxos, (amount_to_be_paid_in_currency_with_fees - preselected_amount).unwrap_or(Amount::ZERO), PayFee::PayFeeWithThisCurrency, diff --git a/wallet/src/account/utxo_selector/mod.rs b/wallet/src/account/utxo_selector/mod.rs index dc03b8b84..c38c8f142 100644 --- a/wallet/src/account/utxo_selector/mod.rs +++ b/wallet/src/account/utxo_selector/mod.rs @@ -734,6 +734,49 @@ pub fn select_coins( } } +/// Select coins, preferring confirmed utxos. +/// +/// A transaction that spends an unconfirmed (in-mempool) utxo forms a cluster with the +/// transaction that produced it, and the mempool limits how big such a cluster may be. To +/// reduce the chance of building a transaction that the mempool would reject, this first tries +/// to reach the target using `confirmed_pool` alone. Only if that doesn't work (for example +/// because the confirmed utxos aren't enough) does it fall back to `full_pool`, which also +/// contains the unconfirmed utxos. +/// +/// `confirmed_pool` is `None` when there is no preference to apply, for example when the caller +/// selected the inputs explicitly; in that case `full_pool` is used directly. +pub fn select_coins_preferring_confirmed( + confirmed_pool: Option>, + full_pool: Vec, + selection_target: Amount, + pay_fees: PayFee, + cost_of_change: Amount, + coin_selection_algo: CoinSelectionAlgo, + max_tx_weight: usize, +) -> Result { + if let Some(confirmed_pool) = confirmed_pool + && let Ok(result) = select_coins( + confirmed_pool, + selection_target, + pay_fees, + cost_of_change, + coin_selection_algo, + max_tx_weight, + ) + { + return Ok(result); + } + + select_coins( + full_pool, + selection_target, + pay_fees, + cost_of_change, + coin_selection_algo, + max_tx_weight, + ) +} + fn select_random_coins( mut utxo_pool: Vec, selection_target: Amount, diff --git a/wallet/src/account/utxo_selector/tests.rs b/wallet/src/account/utxo_selector/tests.rs index 10b6f3ecd..2784cd05e 100644 --- a/wallet/src/account/utxo_selector/tests.rs +++ b/wallet/src/account/utxo_selector/tests.rs @@ -450,3 +450,70 @@ fn test_srd_solver_max_weight(#[case] seed: Seed) { assert_eq!(result.unwrap_err(), UtxoSelectorError::MaxWeightExceeded); } + +#[test] +fn test_prefer_confirmed_uses_confirmed_when_sufficient() { + // The confirmed pool alone can cover the target, so it must be used and the full pool + // (deliberately given a different, larger value) must be left untouched. + let mut confirmed = vec![]; + add_output(Amount::from_atoms(60), &mut confirmed); + let mut full = vec![]; + add_output(Amount::from_atoms(1000), &mut full); + + let result = select_coins_preferring_confirmed( + Some(confirmed), + full, + Amount::from_atoms(50), + PayFee::PayFeeWithThisCurrency, + Amount::ZERO, + CoinSelectionAlgo::Randomize, + 100, + ) + .unwrap(); + + assert_eq!(result.effective_value, Amount::from_atoms(60)); +} + +#[test] +fn test_prefer_confirmed_falls_back_when_confirmed_insufficient() { + // The confirmed pool can't reach the target on its own, so selection must fall back to the + // full pool, which additionally contains a larger (unconfirmed) utxo. + let mut confirmed = vec![]; + add_output(Amount::from_atoms(10), &mut confirmed); + let mut full = vec![]; + add_output(Amount::from_atoms(10), &mut full); + add_output(Amount::from_atoms(90), &mut full); + + let result = select_coins_preferring_confirmed( + Some(confirmed), + full, + Amount::from_atoms(50), + PayFee::PayFeeWithThisCurrency, + Amount::ZERO, + CoinSelectionAlgo::Randomize, + 100, + ) + .unwrap(); + + assert!(result.effective_value >= Amount::from_atoms(50)); +} + +#[test] +fn test_prefer_confirmed_without_preference_uses_full_pool() { + // With no confirmed pool the full pool is used directly. + let mut full = vec![]; + add_output(Amount::from_atoms(70), &mut full); + + let result = select_coins_preferring_confirmed( + None, + full, + Amount::from_atoms(50), + PayFee::PayFeeWithThisCurrency, + Amount::ZERO, + CoinSelectionAlgo::Randomize, + 100, + ) + .unwrap(); + + assert_eq!(result.effective_value, Amount::from_atoms(70)); +}