From e4d8330f34cf82e43bd7d1dc54e56ebf4cfe0baf Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Tue, 19 Dec 2023 01:06:13 +0000 Subject: [PATCH 1/4] spend: use debug log level in coin selection This function is now called many times from the GUI via `create_spend` and we expect this log message to be generated multiple times. --- src/spend.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/spend.rs b/src/spend.rs index c139271ff..5b15a372e 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -344,7 +344,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() ); From 8d84f0de863c8fea97d7402651482b908b6d5b87 Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Fri, 29 Dec 2023 17:46:32 +0000 Subject: [PATCH 2/4] spend: return max possible change from coin selection --- src/spend.rs | 44 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/src/spend.rs b/src/spend.rs index 5b15a372e..a121c0c3c 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -170,6 +170,21 @@ pub struct CandidateCoin { pub sequence: Option, } +/// 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, + /// 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 +269,7 @@ fn select_coins_for_spend( min_fee: u64, max_sat_weight: u32, must_have_change: bool, -) -> Result<(Vec, bitcoin::Amount), InsufficientFunds> { +) -> Result { 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 @@ -374,14 +389,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 @@ -535,7 +563,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, + .. + } = { // 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()); @@ -593,8 +625,8 @@ pub fn create_spend( } // 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); From da474ad6ce1a7553bdc16a586565ef509d74f8ee Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Thu, 4 Jan 2024 21:17:22 +0000 Subject: [PATCH 3/4] spend: add warning when adding change to fee --- src/commands/mod.rs | 9 +++++++-- src/spend.rs | 34 ++++++++++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 9eb224497..f300787c1 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -490,7 +490,9 @@ 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, .. + } = create_spend( &self.config.main_descriptor, &self.secp, &mut tx_getter, @@ -810,6 +812,7 @@ impl DaemonControl { let CreateSpendRes { psbt: rbf_psbt, has_change, + .. } = match create_spend( &self.config.main_descriptor, &self.secp, @@ -985,7 +988,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, diff --git a/src/spend.rs b/src/spend.rs index a121c0c3c..4b9435d7c 100644 --- a/src/spend.rs +++ b/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. @@ -455,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, } /// Create a PSBT for a transaction spending some, or all, of `candidate_coins` to `destinations`. @@ -508,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), @@ -566,7 +588,7 @@ pub fn create_spend( 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. @@ -622,6 +644,10 @@ 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. @@ -668,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, + }) } From 5a15c744e796c9ebe4f6c1eb3010b824189f7855 Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Thu, 4 Jan 2024 21:27:36 +0000 Subject: [PATCH 4/4] commands: return warnings from spend creation --- doc/API.md | 7 ++-- src/commands/mod.rs | 79 ++++++++++++++++++++++++++++++++++++++++++--- tests/test_spend.py | 41 +++++++++++++++++------ 3 files changed, 110 insertions(+), 17 deletions(-) diff --git a/doc/API.md b/doc/API.md index 35a9b3b93..d629bc7c0 100644 --- a/doc/API.md +++ b/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` diff --git a/src/commands/mod.rs b/src/commands/mod.rs index f300787c1..751726fa3 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -491,7 +491,9 @@ impl DaemonControl { // future. let change_info = change_address.info; let CreateSpendRes { - psbt, has_change, .. + psbt, + has_change, + warnings, } = create_spend( &self.config.main_descriptor, &self.secp, @@ -508,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> { @@ -812,7 +817,7 @@ impl DaemonControl { let CreateSpendRes { psbt: rbf_psbt, has_change, - .. + warnings, } = match create_spend( &self.config.main_descriptor, &self.secp, @@ -857,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(), + }); } } @@ -1103,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, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -1141,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}, @@ -1374,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() @@ -1450,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. diff --git a/tests/test_spend.py b/tests/test_spend.py index 94ba1a592..82ac0f58d 100644 --- a/tests/test_spend.py +++ b/tests/test_spend.py @@ -25,6 +25,8 @@ def test_spend_change(lianad, bitcoind): spend_psbt = PSBT.from_base64(res["psbt"]) assert len(spend_psbt.o) == 3 assert len(spend_psbt.tx.vout) == 3 + # Since the transaction contains a change output there is no warning. + assert len(res["warnings"]) == 0 # Sign and broadcast this first Spend transaction. signed_psbt = lianad.signer.sign_psbt(spend_psbt) @@ -46,6 +48,8 @@ def test_spend_change(lianad, bitcoind): } res = lianad.rpc.createspend(destinations, outpoints, 2) spend_psbt = PSBT.from_base64(res["psbt"]) + assert len(spend_psbt.o) == 2 + assert len(res["warnings"]) == 0 # We can sign and broadcast it. signed_psbt = lianad.signer.sign_psbt(spend_psbt) @@ -110,6 +114,8 @@ def sign_and_broadcast(psbt): res = lianad.rpc.createspend(destinations, [outpoint], 6) psbt = PSBT.from_base64(res["psbt"]) sign_and_broadcast(psbt) + assert len(psbt.o) == 2 + assert len(res["warnings"]) == 0 # Spend the second coin without a change output outpoint = next( @@ -123,30 +129,43 @@ def sign_and_broadcast(psbt): res = lianad.rpc.createspend(destinations, [outpoint], 1) psbt = PSBT.from_base64(res["psbt"]) sign_and_broadcast(psbt) + assert len(psbt.o) == 1 + assert len(res["warnings"]) == 1 + assert ( + res["warnings"][0] + == "Change amount of 830 sats added to fee as it was too small to create a transaction output." + ) # Spend the third coin to an address of ours, no change - outpoints = [ - c["outpoint"] - for c in lianad.rpc.listcoins()["coins"] - if deposit_c in c["outpoint"] - ] + coins_c = [c for c in lianad.rpc.listcoins()["coins"] if deposit_c in c["outpoint"]] destinations = { lianad.rpc.getnewaddress()["address"]: int(0.03 * COIN) - 1_000, } - res = lianad.rpc.createspend(destinations, [outpoints[0]], 1) + outpoint_3 = [c["outpoint"] for c in coins_c if c["amount"] == 0.03 * COIN][0] + res = lianad.rpc.createspend(destinations, [outpoint_3], 1) psbt = PSBT.from_base64(res["psbt"]) sign_and_broadcast(psbt) + assert len(psbt.o) == 1 + assert len(res["warnings"]) == 1 + assert ( + res["warnings"][0] + == "Change amount of 818 sats added to fee as it was too small to create a transaction output." + ) # Spend the fourth coin to an address of ours, with change + outpoint_4 = [c["outpoint"] for c in coins_c if c["amount"] == 0.04 * COIN][0] destinations = { lianad.rpc.getnewaddress()["address"]: int(0.04 * COIN / 2), } - res = lianad.rpc.createspend(destinations, [outpoints[1]], 18) + res = lianad.rpc.createspend(destinations, [outpoint_4], 18) psbt = PSBT.from_base64(res["psbt"]) sign_and_broadcast(psbt) + assert len(psbt.o) == 2 + assert len(res["warnings"]) == 0 - # Batch spend the fourth and fifth coins - outpoint = next( + # Batch spend the fifth and sixth coins + outpoint_5 = [c["outpoint"] for c in coins_c if c["amount"] == 0.05 * COIN][0] + outpoint_6 = next( c["outpoint"] for c in lianad.rpc.listcoins()["coins"] if deposit_d in c["outpoint"] @@ -156,9 +175,11 @@ def sign_and_broadcast(psbt): lianad.rpc.getnewaddress()["address"]: int(0.01 * COIN), bitcoind.rpc.getnewaddress(): int(0.01 * COIN), } - res = lianad.rpc.createspend(destinations, [outpoints[2], outpoint], 2) + res = lianad.rpc.createspend(destinations, [outpoint_5, outpoint_6], 2) psbt = PSBT.from_base64(res["psbt"]) sign_and_broadcast(psbt) + assert len(psbt.o) == 4 + assert len(res["warnings"]) == 0 # All the spent coins must have been detected as such all_deposits = (deposit_a, deposit_b, deposit_c, deposit_d)