Prefer confirmed utxos during coin selection

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<K: AccountKeyChains> Account<K> {
                 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<K: AccountKeyChains> Account<K> {
             .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<K: AccountKeyChains> Account<K> {
                     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<K: AccountKeyChains> Account<K> {
             .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<K: AccountKeyChains> Account<K> {
             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,

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<Vec<OutputGroup>>,
+    full_pool: Vec<OutputGroup>,
+    selection_target: Amount,
+    pay_fees: PayFee,
+    cost_of_change: Amount,
+    coin_selection_algo: CoinSelectionAlgo,
+    max_tx_weight: usize,
+) -> Result<SelectionResult, UtxoSelectorError> {
+    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<OutputGroup>,
     selection_target: Amount,

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));
+}