Merge #873: commands: include unconfirmed change as coin selection ca…

…ndidates

c0d4320 spend: add warning about fee for ancestor (jp1ac4)
a38c173 spend: return additional fee paid for ancestors (jp1ac4)
62bb4ad commands: include unconfirmed change as candidates (jp1ac4)
b05b0f1 commands: add ancestor info for user-selected unconfirmed coins (jp1ac4)
94ef66c spend: increase candidate weight to pay for ancestor (jp1ac4)
5f00220 bitcoin: add mempool_entry to interface (jp1ac4)
edbf00f bitcoin: add ancestor size and fees to mempool entry (jp1ac4)
0450322 func test: use utils function (jp1ac4)

Pull request description:

  This is a first PR towards resolving #826 that adds unconfirmed change as coin selection candidates when creating a spend.

  As per #826 (comment), I haven't made any changes to the `rbfpsbt` command.

  We will also need to apply the same change in the GUI when selecting candidates for coin selection there (see #863 (comment)).

ACKs for top commit:
  darosior:
    ACK c0d4320

Tree-SHA512: 8c17f5f8c32913f1ffae3a93ca3e8ee52ac40ee86790e41d73def5ed0c057e110e101797f778715fcd5f6bded1cd170618209323b5114a4f69c02d0ce066a2f2

doc/API.md

@@ -149,7 +149,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/bitcoin/d/mod.rs

@@ -1473,6 +1473,7 @@ impl<'a> CachedTxGetter<'a> {
 #[derive(Debug, Clone)]
 pub struct MempoolEntry {
     pub vsize: u64,
+    pub ancestor_vsize: u64,
     pub fees: MempoolEntryFees,
 }
 
@@ -1482,19 +1483,28 @@ impl From<Json> for MempoolEntry {
             .get("vsize")
             .and_then(Json::as_u64)
             .expect("Must be present in bitcoind response");
+        let ancestor_vsize = json
+            .get("ancestorsize")
+            .and_then(Json::as_u64)
+            .expect("Must be present in bitcoind response");
         let fees = json
             .get("fees")
             .as_ref()
             .expect("Must be present in bitcoind response")
             .into();
 
-        MempoolEntry { vsize, fees }
+        MempoolEntry {
+            vsize,
+            ancestor_vsize,
+            fees,
+        }
     }
 }
 
 #[derive(Debug, Clone)]
 pub struct MempoolEntryFees {
     pub base: bitcoin::Amount,
+    pub ancestor: bitcoin::Amount,
     pub descendant: bitcoin::Amount,
 }
 
@@ -1506,11 +1516,20 @@ impl From<&&Json> for MempoolEntryFees {
             .and_then(Json::as_f64)
             .and_then(|a| bitcoin::Amount::from_btc(a).ok())
             .expect("Must be present and a valid amount");
+        let ancestor = json
+            .get("ancestor")
+            .and_then(Json::as_f64)
+            .and_then(|a| bitcoin::Amount::from_btc(a).ok())
+            .expect("Must be present and a valid amount");
         let descendant = json
             .get("descendant")
             .and_then(Json::as_f64)
             .and_then(|a| bitcoin::Amount::from_btc(a).ok())
             .expect("Must be present and a valid amount");
-        MempoolEntryFees { base, descendant }
+        MempoolEntryFees {
+            base,
+            ancestor,
+            descendant,
+        }
     }
 }

src/bitcoin/mod.rs

@@ -116,6 +116,11 @@ pub trait BitcoinInterface: Send {
 
     /// Get the details of unconfirmed transactions spending these outpoints, if any.
     fn mempool_spenders(&self, outpoints: &[bitcoin::OutPoint]) -> Vec<MempoolEntry>;
+
+    /// Get mempool data for the given transaction.
+    ///
+    /// Returns `None` if the transaction is not in the mempool.
+    fn mempool_entry(&self, txid: &bitcoin::Txid) -> Option<MempoolEntry>;
 }
 
 impl BitcoinInterface for d::BitcoinD {
@@ -366,6 +371,10 @@ impl BitcoinInterface for d::BitcoinD {
             .filter_map(|txid| self.mempool_entry(&txid))
             .collect()
     }
+
+    fn mempool_entry(&self, txid: &bitcoin::Txid) -> Option<MempoolEntry> {
+        self.mempool_entry(txid)
+    }
 }
 
 // FIXME: do we need to repeat the entire trait implemenation? Isn't there a nicer way?
@@ -456,6 +465,10 @@ impl BitcoinInterface for sync::Arc<sync::Mutex<dyn BitcoinInterface + 'static>>
     fn mempool_spenders(&self, outpoints: &[bitcoin::OutPoint]) -> Vec<MempoolEntry> {
         self.lock().unwrap().mempool_spenders(outpoints)
     }
+
+    fn mempool_entry(&self, txid: &bitcoin::Txid) -> Option<MempoolEntry> {
+        self.lock().unwrap().mempool_entry(txid)
+    }
 }
 
 // FIXME: We could avoid this type (and all the conversions entailing allocations) if bitcoind

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,
@@ -179,6 +179,7 @@ fn coin_to_candidate(
     coin: &Coin,
     must_select: bool,
     sequence: Option<bitcoin::Sequence>,
+    ancestor_info: Option<AncestorInfo>,
 ) -> CandidateCoin {
     CandidateCoin {
         outpoint: coin.outpoint,
@@ -187,6 +188,7 @@ fn coin_to_candidate(
         is_change: coin.is_change,
         must_select,
         sequence,
+        ancestor_info,
     }
 }
 
@@ -460,13 +462,33 @@ 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),
+                        ))
+                    } else {
+                        None
+                    }
+                })
+                .map(|(c, ancestor_info)| {
+                    coin_to_candidate(
+                        &c,
+                        /*must_select=*/ false,
+                        /*sequence=*/ None,
+                        ancestor_info,
+                    )
                 })
                 .collect()
         } else {
@@ -482,8 +504,23 @@ impl DaemonControl {
                 }
             }
             coins
-                .into_values()
-                .map(|c| coin_to_candidate(&c, /*must_select=*/ true, /*sequence=*/ None))
+                .into_iter()
+                .map(|(op, c)| {
+                    let ancestor_info = if c.block_info.is_none() {
+                        // We include any non-change coins here as they have been selected by the caller.
+                        // If the unconfirmed coin's transaction is no longer in the mempool, keep the
+                        // coin as a candidate but without any ancestor info (same as confirmed candidate).
+                        self.bitcoin.mempool_entry(&op.txid).map(AncestorInfo::from)
+                    } else {
+                        None
+                    };
+                    coin_to_candidate(
+                        &c,
+                        /*must_select=*/ true,
+                        /*sequence=*/ None,
+                        ancestor_info,
+                    )
+                })
                 .collect()
         };
 
@@ -795,7 +832,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
@@ -807,6 +851,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
@@ -996,6 +1041,7 @@ impl DaemonControl {
                         &c,
                         /*must_select=*/ true,
                         /*sequence=*/ Some(bitcoin::Sequence::from_height(timelock)),
+                        /*ancestor_info=*/ None,
                     ))
                 } else {
                     None
@@ -1384,8 +1430,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),
             Ok(CreateSpendResult::InsufficientFunds { .. }),
@@ -1616,21 +1662,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,
         }]);