commands: include unconfirmed change as candidates

doc/API.md

@@ -148,7 +148,8 @@ A coin may have one of the following four statuses:
 Create a transaction spending one or more of our coins. All coins must exist and not be spent.
 
 If no coins are specified in `outpoints`, they will be selected automatically from the set of
-confirmed coins (see [`listcoins`](#listcoins) for coin status definitions).
+confirmed coins together with any unconfirmed coins that are change outputs
+(see [`listcoins`](#listcoins) for coin status definitions).
 
 Will error if the given coins are not sufficient to cover the transaction cost at 90% (or more) of
 the given feerate. If on the contrary the transaction is more than sufficiently funded, it will

src/commands/mod.rs

@@ -9,7 +9,7 @@ use crate::{
     database::{Coin, DatabaseConnection, DatabaseInterface},
     descriptors,
     spend::{
-        create_spend, AddrInfo, CandidateCoin, CreateSpendRes, SpendCreationError,
+        create_spend, AddrInfo, AncestorInfo, CandidateCoin, CreateSpendRes, SpendCreationError,
         SpendOutputAddress, SpendTxFees, TxGetter,
     },
     DaemonControl, VERSION,
@@ -180,6 +180,7 @@ fn coin_to_candidate(
     coin: &Coin,
     must_select: bool,
     sequence: Option<bitcoin::Sequence>,
+    ancestor_info: Option<AncestorInfo>,
 ) -> CandidateCoin {
     CandidateCoin {
         outpoint: coin.outpoint,
@@ -188,6 +189,7 @@ fn coin_to_candidate(
         is_change: coin.is_change,
         must_select,
         sequence,
+        ancestor_info,
     }
 }
 
@@ -460,13 +462,35 @@ impl DaemonControl {
         // the spend from a set of optional candidates.
         // Otherwise, only the specified coins will be used, all as mandatory candidates.
         let candidate_coins: Vec<CandidateCoin> = if coins_outpoints.is_empty() {
-            // We only select confirmed coins for now. Including unconfirmed ones as well would
-            // introduce a whole bunch of additional complexity.
+            // From our unconfirmed coins, we only include those that are change outputs
+            // since unconfirmed external deposits are more at risk of being dropped
+            // unexpectedly from the mempool as they are beyond the user's control.
             db_conn
-                .coins(&[CoinStatus::Confirmed], &[])
-                .into_values()
-                .map(|c| {
-                    coin_to_candidate(&c, /*must_select=*/ false, /*sequence=*/ None)
+                .coins(&[CoinStatus::Unconfirmed, CoinStatus::Confirmed], &[])
+                .into_iter()
+                .filter_map(|(op, c)| {
+                    if c.block_info.is_some() {
+                        Some((c, None)) // confirmed coins have no ancestor info
+                    } else if c.is_change {
+                        // In case the mempool_entry is None, the coin will be included without
+                        // any ancestor info.
+                        Some((
+                            c,
+                            self.bitcoin
+                                .mempool_entry(&op.txid)
+                                .map(AncestorInfo::from_mempool_entry),
+                        ))
+                    } else {
+                        None
+                    }
+                })
+                .map(|(c, ancestor_info)| {
+                    coin_to_candidate(
+                        &c,
+                        /*must_select=*/ false,
+                        /*sequence=*/ None,
+                        ancestor_info,
+                    )
                 })
                 .collect()
         } else {
@@ -483,7 +507,12 @@ impl DaemonControl {
             }
             coins
                 .into_values()
-                .map(|c| coin_to_candidate(&c, /*must_select=*/ true, /*sequence=*/ None))
+                .map(|c| {
+                    coin_to_candidate(
+                        &c, /*must_select=*/ true, /*sequence=*/ None,
+                        /*ancestor_info=*/ None,
+                    )
+                })
                 .collect()
         };
 
@@ -779,7 +808,14 @@ impl DaemonControl {
         let mut candidate_coins: Vec<CandidateCoin> = prev_coins
             .values()
             .map(|c| {
-                coin_to_candidate(c, /*must_select=*/ !is_cancel, /*sequence=*/ None)
+                // In case any previous coins are unconfirmed, we don't include their ancestor info
+                // in the candidate as the replacement fee and feerate will be higher and any
+                // additional fee to pay for ancestors should already have been taken into account
+                // when including these coins in the previous transaction.
+                coin_to_candidate(
+                    c, /*must_select=*/ !is_cancel, /*sequence=*/ None,
+                    /*ancestor_info=*/ None,
+                )
             })
             .collect();
         let confirmed_cands: Vec<CandidateCoin> = db_conn
@@ -791,6 +827,7 @@ impl DaemonControl {
                 if !prev_coins.contains_key(&c.outpoint) {
                     Some(coin_to_candidate(
                         &c, /*must_select=*/ false, /*sequence=*/ None,
+                        /*ancestor_info=*/ None,
                     ))
                 } else {
                     None
@@ -974,6 +1011,7 @@ impl DaemonControl {
                         &c,
                         /*must_select=*/ true,
                         /*sequence=*/ Some(bitcoin::Sequence::from_height(timelock)),
+                        /*ancestor_info=*/ None,
                     ))
                 } else {
                     None
@@ -1353,8 +1391,8 @@ mod tests {
             spend_txid: None,
             spend_block: None,
         }]);
-        // If we try to use coin selection, the unconfirmed coin will not be used as a candidate
-        // and so we get a coin selection error due to insufficient funds.
+        // If we try to use coin selection, the unconfirmed non-change coin will not be used
+        // as a candidate and so we get a coin selection error due to insufficient funds.
         assert!(matches!(
             control.create_spend(&destinations, &[], 1, None),
             Err(CommandError::SpendCreation(
@@ -1488,21 +1526,18 @@ mod tests {
             )))
         );
 
-        // Add a confirmed unspent coin to be used for coin selection.
+        // Add an unconfirmed change coin to be used for coin selection.
         let confirmed_op_1 = bitcoin::OutPoint {
             txid: dummy_op.txid,
             vout: dummy_op.vout + 100,
         };
         db_conn.new_unspent_coins(&[Coin {
             outpoint: confirmed_op_1,
             is_immature: false,
-            block_info: Some(BlockInfo {
-                height: 174500,
-                time: 174500,
-            }),
+            block_info: None,
             amount: bitcoin::Amount::from_sat(80_000),
             derivation_index: bip32::ChildNumber::from(42),
-            is_change: false,
+            is_change: true,
             spend_txid: None,
             spend_block: None,
         }]);

src/spend.rs

@@ -1,4 +1,4 @@
-use crate::descriptors;
+use crate::{bitcoin::MempoolEntry, descriptors};
 
 use std::{collections::BTreeMap, convert::TryInto, fmt};
 
@@ -11,6 +11,7 @@ use miniscript::bitcoin::{
     self,
     absolute::{Height, LockTime},
     bip32,
+    constants::WITNESS_SCALE_FACTOR,
     psbt::{Input as PsbtIn, Output as PsbtOut, Psbt},
     secp256k1,
 };
@@ -155,6 +156,29 @@ fn sanity_check_psbt(
     Ok(())
 }
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+pub struct AncestorInfo {
+    pub vsize: u32,
+    pub fee: u32,
+}
+
+impl AncestorInfo {
+    pub fn from_mempool_entry(entry: MempoolEntry) -> Self {
+        Self {
+            vsize: entry
+                .ancestor_vsize
+                .try_into()
+                .expect("vsize must fit in a u32"),
+            fee: entry
+                .fees
+                .ancestor
+                .to_sat()
+                .try_into()
+                .expect("fee in sats should fit in a u32"),
+        }
+    }
+}
+
 /// A candidate for coin selection when creating a transaction.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub struct CandidateCoin {
@@ -170,6 +194,8 @@ pub struct CandidateCoin {
     pub must_select: bool,
     /// The nSequence field to set for an input spending this coin.
     pub sequence: Option<bitcoin::Sequence>,
+    /// Information about in-mempool ancestors of the coin.
+    pub ancestor_info: Option<AncestorInfo>,
 }
 
 /// Metric based on [`LowestFee`] that aims to minimize transaction fees
@@ -268,12 +294,41 @@ fn select_coins_for_spend(
         .try_into()
         .expect("Transaction weight must fit in u32");
     let max_input_weight = TXIN_BASE_WEIGHT + max_sat_weight;
+    // Get feerate as u32 for calculation relating to ancestor below.
+    // We expect `feerate_vb` to be a positive integer, but take ceil()
+    // just in case to be sure we pay enough for ancestors.
+    let feerate_vb_u32 = feerate_vb.ceil() as u32;
+    let witness_factor: u32 = WITNESS_SCALE_FACTOR
+        .try_into()
+        .expect("scale factor must fit in u32");
     let candidates: Vec<Candidate> = candidate_coins
         .iter()
         .map(|cand| Candidate {
             input_count: 1,
             value: cand.amount.to_sat(),
-            weight: max_input_weight,
+            weight: {
+                let mut weight = max_input_weight;
+                if let Some(info) = cand.ancestor_info {
+                    // The implied ancestor vsize if the fee had been paid at our target feerate.
+                    let ancestor_vsize_at_feerate: u32 = info
+                        .fee
+                        .checked_div(feerate_vb_u32)
+                        .expect("feerate is greater than zero");
+                    // If the actual ancestor vsize is bigger than the implied vsize, we will need to
+                    // pay the difference in order for the combined feerate to be at the target value.
+                    // We multiply the vsize by 4 to get the ancestor weight, which is an upper bound
+                    // on its true weight (vsize*4 - 3 <= weight <= vsize*4), to ensure we pay enough.
+                    // Note that if candidates share ancestors, we may add this difference more than
+                    // once in the resulting transaction.
+                    if info.vsize > ancestor_vsize_at_feerate {
+                        let difference: u32 = (info.vsize - ancestor_vsize_at_feerate)
+                            .checked_mul(witness_factor)
+                            .expect("weight difference must fit in u32");
+                        weight += difference;
+                    }
+                }
+                weight
+            },
             is_segwit: true, // We only support receiving on Segwit scripts.
         })
         .collect();