Merge #905: spend: warn when overpaying fee

5a15c74 commands: return warnings from spend creation (jp1ac4)
da474ad spend: add warning when adding change to fee (jp1ac4)
8d84f0d spend: return max possible change from coin selection (jp1ac4)
e4d8330 spend: use debug log level in coin selection (jp1ac4)

Pull request description:

  This is a first step towards #811. The GUI will be updated in a follow-up PR.

  It adds a warning if there is any change/excess available that has been added to the fee, building on the assumption from bitcoindevkit/coin-select#14 that the decision whether to include change depends on whether the excess is over a threshold. The function `select_coins_for_spend()` now returns this threshold so that the caller can check if the possible change amount is above it.

  I've used an `enum` for the different warnings, but could use strings directly.

  If the approach looks fine, I'll add some tests.

ACKs for top commit:
  darosior:
    ACK 5a15c74

Tree-SHA512: 12716b1e299d3154c520667c66aeb7a176a770f4a1d53125a8334d7c5a7e7bb2ce1dcb795ad5998bffc1303d9cb34c651cf8d3ef3dee8210572d75069012bddd

doc/API.md

@@ -179,9 +179,10 @@ This command will refuse to create any output worth less than 5k sats.
 
 #### Response
 
-| Field          | Type      | Description                                          |
-| -------------- | --------- | ---------------------------------------------------- |
-| `psbt`         | string    | PSBT of the spending transaction, encoded as base64. |
+| Field          | Type              | Description                                          |
+| -------------- | ----------------- | ---------------------------------------------------- |
+| `psbt`         | string            | PSBT of the spending transaction, encoded as base64. |
+| `warnings`     | list of string    | Warnings, if any, generated during spend creation.   |
 
 
 ### `updatespend`

src/commands/mod.rs

@@ -490,7 +490,11 @@ impl DaemonControl {
         // derivation index in case any address in the transaction outputs was ours and from the
         // future.
         let change_info = change_address.info;
-        let CreateSpendRes { psbt, has_change } = create_spend(
+        let CreateSpendRes {
+            psbt,
+            has_change,
+            warnings,
+        } = create_spend(
             &self.config.main_descriptor,
             &self.secp,
             &mut tx_getter,
@@ -506,7 +510,10 @@ impl DaemonControl {
             self.maybe_increase_next_deriv_index(&mut db_conn, &change_info);
         }
 
-        Ok(CreateSpendResult { psbt })
+        Ok(CreateSpendResult {
+            psbt,
+            warnings: warnings.iter().map(|w| w.to_string()).collect(),
+        })
     }
 
     pub fn update_spend(&self, mut psbt: Psbt) -> Result<(), CommandError> {
@@ -810,6 +817,7 @@ impl DaemonControl {
             let CreateSpendRes {
                 psbt: rbf_psbt,
                 has_change,
+                warnings,
             } = match create_spend(
                 &self.config.main_descriptor,
                 &self.secp,
@@ -854,7 +862,10 @@ impl DaemonControl {
                     self.maybe_increase_next_deriv_index(&mut db_conn, &change_address.info);
                 }
 
-                return Ok(CreateSpendResult { psbt: rbf_psbt });
+                return Ok(CreateSpendResult {
+                    psbt: rbf_psbt,
+                    warnings: warnings.iter().map(|w| w.to_string()).collect(),
+                });
             }
         }
 
@@ -985,7 +996,9 @@ impl DaemonControl {
         }
 
         let sweep_addr_info = sweep_addr.info;
-        let CreateSpendRes { psbt, has_change } = create_spend(
+        let CreateSpendRes {
+            psbt, has_change, ..
+        } = create_spend(
             &self.config.main_descriptor,
             &self.secp,
             &mut tx_getter,
@@ -1098,6 +1111,7 @@ pub struct ListCoinsResult {
 pub struct CreateSpendResult {
     #[serde(serialize_with = "ser_to_string", deserialize_with = "deser_fromstr")]
     pub psbt: Psbt,
+    pub warnings: Vec<String>,
 }
 
 #[derive(Debug, Clone, Serialize, Deserialize)]
@@ -1136,6 +1150,7 @@ mod tests {
     use super::*;
     use crate::{bitcoin::Block, database::BlockInfo, spend::InsaneFeeInfo, testutils::*};
 
+    use bdk_coin_select::InsufficientFunds;
     use bitcoin::{
         bip32::{self, ChildNumber},
         blockdata::transaction::{TxIn, TxOut, Version as TxVersion},
@@ -1369,6 +1384,8 @@ mod tests {
         assert_eq!(tx.input.len(), 1);
         assert_eq!(tx.input[0].previous_output, dummy_op);
         assert_eq!(tx.output.len(), 2);
+        // It has change so no warnings expected.
+        assert!(res.warnings.is_empty());
         assert_eq!(
             tx.output[0].script_pubkey,
             dummy_addr.payload().script_pubkey()
@@ -1445,6 +1462,65 @@ mod tests {
             dummy_addr.payload().script_pubkey()
         );
         assert_eq!(tx.output[0].value.to_sat(), 95_000);
+        // change = 100_000 - 95_000 - /* fee without change */ 127 - /* extra fee for change output */ 43 = 4830
+        assert_eq!(res.warnings, vec!["Change amount of 4830 sats added to fee as it was too small to create a transaction output."]);
+
+        // Increase the target value by the change amount and the warning will disappear.
+        *destinations.get_mut(&dummy_addr).unwrap() = 95_000 + 4_830;
+        let res = control
+            .create_spend(&destinations, &[dummy_op], 1, None)
+            .unwrap();
+        let tx = res.psbt.unsigned_tx;
+        assert_eq!(tx.output.len(), 1);
+        assert!(res.warnings.is_empty());
+
+        // Now increase target also by the extra fee that was paying for change and we can still create the spend.
+        *destinations.get_mut(&dummy_addr).unwrap() =
+            95_000 + 4_830 + /* fee for change output */ 43;
+        let res = control
+            .create_spend(&destinations, &[dummy_op], 1, None)
+            .unwrap();
+        let tx = res.psbt.unsigned_tx;
+        assert_eq!(tx.output.len(), 1);
+        assert!(res.warnings.is_empty());
+
+        // Now increase the target by 1 more sat and we will have insufficient funds.
+        *destinations.get_mut(&dummy_addr).unwrap() =
+            95_000 + 4_830 + /* fee for change output */ 43 + 1;
+        assert_eq!(
+            control.create_spend(&destinations, &[dummy_op], 1, None),
+            Err(CommandError::SpendCreation(
+                SpendCreationError::CoinSelection(InsufficientFunds { missing: 1 })
+            ))
+        );
+
+        // Now decrease the target so that the lost change is just 1 sat.
+        *destinations.get_mut(&dummy_addr).unwrap() =
+            100_000 - /* fee without change */ 127 - /* extra fee for change output */ 43 - 1;
+        let res = control
+            .create_spend(&destinations, &[dummy_op], 1, None)
+            .unwrap();
+        // Message uses "sat" instead of "sats" when value is 1.
+        assert_eq!(res.warnings, vec!["Change amount of 1 sat added to fee as it was too small to create a transaction output."]);
+
+        // Now decrease the target value so that we have enough for a change output.
+        *destinations.get_mut(&dummy_addr).unwrap() =
+            95_000 - /* fee without change */ 127 - /* extra fee for change output */ 43;
+        let res = control
+            .create_spend(&destinations, &[dummy_op], 1, None)
+            .unwrap();
+        let tx = res.psbt.unsigned_tx;
+        assert_eq!(tx.output.len(), 2);
+        assert_eq!(tx.output[1].value.to_sat(), 5_000);
+        assert!(res.warnings.is_empty());
+
+        // Now increase the target by 1 and we'll get a warning again, this time for 1 less than the dust threshold.
+        *destinations.get_mut(&dummy_addr).unwrap() =
+            95_000 - /* fee without change */ 127 - /* extra fee for change output */ 43 + 1;
+        let res = control
+            .create_spend(&destinations, &[dummy_op], 1, None)
+            .unwrap();
+        assert_eq!(res.warnings, vec!["Change amount of 4999 sats added to fee as it was too small to create a transaction output."]);
 
         // Now if we mark the coin as spent, we won't create another Spend transaction containing
         // it.

src/spend.rs

@@ -14,6 +14,7 @@ use miniscript::bitcoin::{
     psbt::{Input as PsbtIn, Output as PsbtOut, Psbt},
     secp256k1,
 };
+use serde::{Deserialize, Serialize};
 
 /// We would never create a transaction with an output worth less than this.
 /// That's 1$ at 20_000$ per BTC.
@@ -170,6 +171,21 @@ pub struct CandidateCoin {
     pub sequence: Option<bitcoin::Sequence>,
 }
 
+/// A coin selection result.
+///
+/// A change output should only be added if `change_amount > 0`.
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub struct CoinSelectionRes {
+    /// Selected candidates.
+    pub selected: Vec<CandidateCoin>,
+    /// Change amount that should be included according to the change policy used
+    /// for selection.
+    pub change_amount: bitcoin::Amount,
+    /// Maximum change amount possible with the selection irrespective of any change
+    /// policy.
+    pub max_change_amount: bitcoin::Amount,
+}
+
 /// Metric based on [`LowestFee`] that aims to minimize transaction fees
 /// with the additional option to only find solutions with a change output.
 ///
@@ -254,7 +270,7 @@ fn select_coins_for_spend(
     min_fee: u64,
     max_sat_weight: u32,
     must_have_change: bool,
-) -> Result<(Vec<CandidateCoin>, bitcoin::Amount), InsufficientFunds> {
+) -> Result<CoinSelectionRes, InsufficientFunds> {
     let out_value_nochange = base_tx.output.iter().map(|o| o.value.to_sat()).sum();
 
     // Create the coin selector from the given candidates. NOTE: the coin selector keeps track
@@ -344,7 +360,7 @@ fn select_coins_for_spend(
     #[cfg(debug)]
     let bnb_rounds = bnb_rounds / 1_000;
     if let Err(e) = selector.run_bnb(lowest_fee_change_cond, bnb_rounds) {
-        log::warn!(
+        log::debug!(
             "Coin selection error: '{}'. Selecting coins by descending value per weight unit...",
             e.to_string()
         );
@@ -374,14 +390,27 @@ fn select_coins_for_spend(
     // By now, selection is complete and we can check how much change to give according to our policy.
     let drain = change_policy(&selector, target);
     let change_amount = bitcoin::Amount::from_sat(drain.value);
-    Ok((
+    // Max available change is given by the excess when adding a change output with zero value.
+    let drain_novalue = bdk_coin_select::Drain {
+        weights: drain_weights,
+        value: 0,
+    };
+    let max_change_amount = bitcoin::Amount::from_sat(
         selector
+            .excess(target, drain_novalue)
+            .max(0) // negative excess would mean insufficient funds to pay for change output
+            .try_into()
+            .expect("value is non-negative"),
+    );
+    Ok(CoinSelectionRes {
+        selected: selector
             .selected_indices()
             .iter()
             .map(|i| candidate_coins[*i])
             .collect(),
         change_amount,
-    ))
+        max_change_amount,
+    })
 }
 
 // Get the derived descriptor for this coin
@@ -427,11 +456,31 @@ pub enum SpendTxFees {
     Rbf(u64, u64),
 }
 
+#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
+pub enum CreateSpendWarning {
+    ChangeAddedToFee(u64),
+}
+
+impl fmt::Display for CreateSpendWarning {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            CreateSpendWarning::ChangeAddedToFee(amt) => write!(
+                f,
+                "Change amount of {} sat{} added to fee as it was too small to create a transaction output.",
+                amt,
+                if *amt > 1 {"s"} else {""},
+            ),
+        }
+    }
+}
+
 pub struct CreateSpendRes {
     /// The created PSBT.
     pub psbt: Psbt,
     /// Whether the created PSBT has a change output.
     pub has_change: bool,
+    /// Warnings relating to the PSBT.
+    pub warnings: Vec<CreateSpendWarning>,
 }
 
 /// Create a PSBT for a transaction spending some, or all, of `candidate_coins` to `destinations`.
@@ -480,6 +529,7 @@ pub fn create_spend(
     // 3. Add the selected coins as inputs to the transaction.
     // 4. Finalize the PSBT and sanity check it before returning it.
 
+    let mut warnings = Vec::new();
     let (feerate_vb, min_fee) = match fees {
         SpendTxFees::Regular(feerate) => (feerate, 0),
         SpendTxFees::Rbf(feerate, fee) => (feerate, fee),
@@ -535,7 +585,11 @@ pub fn create_spend(
     };
     // Now select the coins necessary using the provided candidates and determine whether
     // there is any leftover to create a change output.
-    let (selected_coins, change_amount) = {
+    let CoinSelectionRes {
+        selected,
+        change_amount,
+        max_change_amount,
+    } = {
         // At this point the transaction still has no input and no change output, as expected
         // by the coins selection helper function.
         assert!(tx.input.is_empty());
@@ -590,11 +644,15 @@ pub fn create_spend(
             bip32_derivation,
             ..PsbtOut::default()
         });
+    } else if max_change_amount.to_sat() > 0 {
+        warnings.push(CreateSpendWarning::ChangeAddedToFee(
+            max_change_amount.to_sat(),
+        ));
     }
 
     // Iterate through selected coins and add necessary information to the PSBT inputs.
-    let mut psbt_ins = Vec::with_capacity(selected_coins.len());
-    for cand in &selected_coins {
+    let mut psbt_ins = Vec::with_capacity(selected.len());
+    for cand in &selected {
         let sequence = cand
             .sequence
             .unwrap_or(bitcoin::Sequence::ENABLE_RBF_NO_LOCKTIME);
@@ -636,5 +694,9 @@ pub fn create_spend(
     sanity_check_psbt(main_descriptor, &psbt)?;
     // TODO: maybe check for common standardness rules (max size, ..)?
 
-    Ok(CreateSpendRes { psbt, has_change })
+    Ok(CreateSpendRes {
+        psbt,
+        has_change,
+        warnings,
+    })
 }