From b8fd97fc83a18acb8f1f6b20216fd048955e909d Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Wed, 22 Nov 2023 19:38:53 +0000 Subject: [PATCH 1/2] commands: require change for self-send This adds a new `LowestFeeChangeCondition` metric for use in coin selection. It's the same as `LowestFee` with the option to only find solutions with change. This option is then used for self-sends to ensure only a solution with change will be returned. --- src/commands/mod.rs | 19 ++++------ src/commands/utils.rs | 81 ++++++++++++++++++++++++++++++++++++------- tests/test_spend.py | 2 +- 3 files changed, 76 insertions(+), 26 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 8fbb363fb..72c3121ce 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -550,10 +550,13 @@ impl DaemonControl { feerate_vb, 0, // We only constrain the feerate. max_sat_wu, + is_self_send, // Must have change if self-send. ) .map_err(CommandError::CoinSelectionError)? }; // If necessary, add a change output. + // For a self-send, coin selection will only find solutions with change and will otherwise + // return an error. In any case, the PSBT sanity check will catch a transaction with no outputs. if change_amount.to_sat() > 0 { // Don't forget to update our next change index! let next_index = change_index @@ -569,12 +572,6 @@ impl DaemonControl { bip32_derivation: change_desc.bip32_derivations(), ..PsbtOut::default() }); - } else if is_self_send { - return Err(CommandError::InsufficientFunds( - selected_coins.iter().map(|c| c.amount).sum(), - None, - feerate_vb, - )); } // Iterate through selected coins and add necessary information to the PSBT inputs. @@ -1492,14 +1489,10 @@ mod tests { spend_block: None, }]); let empty_dest = &HashMap::, u64>::new(); - assert_eq!( + assert!(matches!( control.create_spend(empty_dest, &[confirmed_op_3], 5), - Err(CommandError::InsufficientFunds( - bitcoin::Amount::from_sat(5_250), - None, - 5 - )) - ); + Err(CommandError::CoinSelectionError(..)) + )); // If we use a lower fee, the self-send will succeed. let res = control .create_spend(empty_dest, &[confirmed_op_3], 1) diff --git a/src/commands/utils.rs b/src/commands/utils.rs index d12b242a0..8437b649a 100644 --- a/src/commands/utils.rs +++ b/src/commands/utils.rs @@ -72,6 +72,40 @@ where consensus::deserialize(&s).map_err(de::Error::custom) } +/// Metric based on [`LowestFee`] that aims to minimize transaction fees +/// with the additional option to only find solutions with a change output. +/// +/// Using this metric with `must_have_change: false` is equivalent to using +/// [`LowestFee`]. +pub struct LowestFeeChangeCondition<'c, C> { + /// The underlying [`LowestFee`] metric to use. + pub lowest_fee: LowestFee<'c, C>, + /// If `true`, only solutions with change will be found. + pub must_have_change: bool, +} + +impl<'c, C> bdk_coin_select::BnbMetric for LowestFeeChangeCondition<'c, C> +where + for<'a, 'b> C: Fn(&'b CoinSelector<'a>, Target) -> bdk_coin_select::Drain, +{ + fn score(&mut self, cs: &CoinSelector<'_>) -> Option { + let drain = (self.lowest_fee.change_policy)(cs, self.lowest_fee.target); + if drain.is_none() && self.must_have_change { + None + } else { + self.lowest_fee.score(cs) + } + } + + fn bound(&mut self, cs: &CoinSelector<'_>) -> Option { + self.lowest_fee.bound(cs) + } + + fn requires_ordering_by_descending_value_pwu(&self) -> bool { + self.lowest_fee.requires_ordering_by_descending_value_pwu() + } +} + /// Select coins for spend. /// /// Returns the selected coins and the change amount, which could be zero. @@ -91,6 +125,9 @@ where /// /// `max_sat_weight` is the maximum size difference (in vb) of /// an input in the transaction before and after satisfaction. +/// +/// `must_have_change` indicates whether the transaction must have a change output. +/// If `true`, the returned change amount will be positive. pub fn select_coins_for_spend( candidate_coins: &[CandidateCoin], base_tx: bitcoin::Transaction, @@ -98,6 +135,7 @@ pub fn select_coins_for_spend( feerate_vb: f32, min_fee: u64, max_sat_weight: u32, + must_have_change: bool, ) -> Result<(Vec, bitcoin::Amount), InsufficientFunds> { let out_value_nochange = base_tx.output.iter().map(|o| o.value).sum(); @@ -157,23 +195,42 @@ pub fn select_coins_for_spend( feerate: FeeRate::from_sat_per_vb(feerate_vb), min_fee, }; - if let Err(e) = selector.run_bnb( - LowestFee { - target, - long_term_feerate, - change_policy: &change_policy, - }, - 100_000, - ) { + let lowest_fee = LowestFee { + target, + long_term_feerate, + change_policy: &change_policy, + }; + let lowest_fee_change_cond = LowestFeeChangeCondition { + lowest_fee, + must_have_change, + }; + if let Err(e) = selector.run_bnb(lowest_fee_change_cond, 100_000) { warn!( "Coin selection error: '{}'. Selecting coins by descending value per weight unit...", e.to_string() ); selector.sort_candidates_by_descending_value_pwu(); - // If more coins still need to be selected to meet target, then `change_policy(&selector, target)` - // will give `Drain::none()`, i.e. no change, and this will simply select more coins until - // they cover the target. - selector.select_until_target_met(target, change_policy(&selector, target))?; + // Select more coins until target is met and change condition satisfied. + loop { + let drain = change_policy(&selector, target); + if selector.is_target_met(target, drain) && (drain.is_some() || !must_have_change) { + break; + } + if !selector.select_next() { + // If the solution must have change, we calculate how much is missing from the current + // selection in order for there to be a change output with the smallest possible value. + let drain = if must_have_change { + bdk_coin_select::Drain { + weights: drain_weights, + value: DUST_OUTPUT_SATS, + } + } else { + drain + }; + let missing = selector.excess(target, drain).unsigned_abs(); + return Err(InsufficientFunds { missing }); + } + } } // By now, selection is complete and we can check how much change to give according to our policy. let drain = change_policy(&selector, target); diff --git a/tests/test_spend.py b/tests/test_spend.py index 8ad184935..de396cf12 100644 --- a/tests/test_spend.py +++ b/tests/test_spend.py @@ -195,7 +195,7 @@ def test_send_to_self(lianad, bitcoind): # Note they may ask for an impossible send-to-self. In this case we'll error cleanly. with pytest.raises( RpcError, - match="Not enough fund to create a 40500 sat/vb transaction with input value 0.12 BTC", + match="Insufficient funds. Missing \\d+ sats", ): lianad.rpc.createspend({}, outpoints, 40500) From 6123b87a024c04906338aa68c6afbcf57d57fc0b Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Fri, 24 Nov 2023 10:05:43 +0000 Subject: [PATCH 2/2] commands: fix documentation --- src/commands/utils.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/commands/utils.rs b/src/commands/utils.rs index 8437b649a..ed2301d4c 100644 --- a/src/commands/utils.rs +++ b/src/commands/utils.rs @@ -123,8 +123,8 @@ where /// /// `min_fee` is the minimum fee (in sats) that the selection must have. /// -/// `max_sat_weight` is the maximum size difference (in vb) of -/// an input in the transaction before and after satisfaction. +/// `max_sat_weight` is the maximum weight difference of an input in the +/// transaction before and after satisfaction. /// /// `must_have_change` indicates whether the transaction must have a change output. /// If `true`, the returned change amount will be positive.