From 936d7e958567608a46e91c21699d611ca5eced1d Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Mon, 1 Apr 2024 17:21:48 +0100 Subject: [PATCH 1/3] spend: bump bdk_coin_select to 0.3.0 The `score` method of the `LowestFee` metric has been fixed and so our temporary fix is no longer required. The `min_fee` parameter of `select_coins_for_spend`, if positive, now ensures that RBF rule 4 is satisfied. `base_weight` is no longer stored in `CoinSelector` and instead the output weights are stored in `Target`. This means that the `output_weight` of `DrainWeights` no longer needs to take into account a potential change in output count varint. --- Cargo.lock | 21 +------ Cargo.toml | 2 +- src/spend.rs | 151 ++++++++++++++++++++++++--------------------------- 3 files changed, 75 insertions(+), 99 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1680010b..a0bd06a4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -64,9 +64,9 @@ checksum = "35636a1494ede3b646cc98f74f8e62c773a38a659ebc777a2cf26b9b74171df9" [[package]] name = "bdk_coin_select" -version = "0.1.1" +version = "0.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c0320167c3655e83f0415d52f39618902e449186ffc7dfb090f922f79675c316" +checksum = "3c084bf76f0f67546fc814ffa82044144be1bb4618183a15016c162f8b087ad4" [[package]] name = "bech32" @@ -110,27 +110,12 @@ dependencies = [ "serde", ] -[[package]] -name = "bitcoin-private" -version = "0.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "73290177011694f38ec25e165d0387ab7ea749a4b81cd4c80dae5988229f7a57" - [[package]] name = "bitcoin_hashes" version = "0.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "90064b8dee6815a6470d60bad07bbbaee885c0e12d04177138fa3291a01b7bc4" -[[package]] -name = "bitcoin_hashes" -version = "0.12.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5d7066118b13d4b20b23645932dfb3a81ce7e29f95726c2036fa33cd7b092501" -dependencies = [ - "bitcoin-private", -] - [[package]] name = "bitcoin_hashes" version = "0.13.0" @@ -485,7 +470,7 @@ version = "0.28.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2acea373acb8c21ecb5a23741452acd2593ed44ee3d343e72baaa143bc89d0d5" dependencies = [ - "bitcoin_hashes 0.12.0", + "bitcoin_hashes 0.13.0", "secp256k1-sys", "serde", ] diff --git a/Cargo.toml b/Cargo.toml index f6d50c16..82c6b304 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,7 +29,7 @@ nonblocking_shutdown = [] miniscript = { version = "11.0", features = ["serde", "compiler", "base64"] } # Coin selection algorithms for spend transaction creation. -bdk_coin_select = { version = "0.1.0" } +bdk_coin_select = "0.3" # Don't reinvent the wheel dirs = "5.0" diff --git a/src/spend.rs b/src/spend.rs index a90a0c67..3d2326c9 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -8,8 +8,8 @@ use std::{ pub use bdk_coin_select::InsufficientFunds; use bdk_coin_select::{ - change_policy, metrics::LowestFee, Candidate, CoinSelector, DrainWeights, FeeRate, Target, - TXIN_BASE_WEIGHT, + metrics::LowestFee, Candidate, ChangePolicy, CoinSelector, DrainWeights, FeeRate, Replace, + Target, TargetFee, TargetOutputs, TXIN_BASE_WEIGHT, }; use miniscript::bitcoin::{ self, @@ -223,47 +223,24 @@ pub struct CoinSelectionRes { /// /// Using this metric with `must_have_change: false` is equivalent to using /// [`LowestFee`]. -struct LowestFeeChangeCondition<'c, C> { +struct LowestFeeChangeCondition { /// The underlying [`LowestFee`] metric to use. - pub lowest_fee: LowestFee<'c, C>, + pub lowest_fee: LowestFee, /// 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); +impl bdk_coin_select::BnbMetric for LowestFeeChangeCondition { + fn score(&mut self, cs: &CoinSelector) -> Option { + let drain = cs.drain(self.lowest_fee.target, self.lowest_fee.change_policy); if drain.is_none() && self.must_have_change { None } else { - // This is a temporary partial fix for https://github.com/bitcoindevkit/coin-select/issues/6 - // until it has been fixed upstream. - // TODO: Revert this change once upstream fix has been made. - // When calculating the score, the excess should be added to changeless solutions instead of - // those with change. - // Given a solution has been found, this fix adds or removes the excess to its incorrectly - // calculated score as required so that two changeless solutions can be differentiated - // if one has higher excess (and therefore pays a higher fee). - // Note that the `bound` function is also affected by this bug, which could mean some branches - // are not considered when running BnB, but at least this fix will mean the score for those - // solutions that are found is correct. - self.lowest_fee.score(cs).map(|score| { - // See https://github.com/bitcoindevkit/coin-select/blob/29b187f5509a01ba125a0354f6711e317bb5522a/src/metrics/lowest_fee.rs#L35-L45 - assert!(cs.selected_value() >= self.lowest_fee.target.value); - let excess = (cs.selected_value() - self.lowest_fee.target.value) as f32; - bdk_coin_select::float::Ordf32(if drain.is_none() { - score.0 + excess - } else { - score.0 - excess - }) - }) + self.lowest_fee.score(cs) } } - fn bound(&mut self, cs: &CoinSelector<'_>) -> Option { + fn bound(&mut self, cs: &CoinSelector) -> Option { self.lowest_fee.bound(cs) } @@ -287,7 +264,10 @@ where /// and change may result in a slightly lower feerate than this as the underlying /// function instead uses a minimum feerate of `feerate_vb / 4.0` sats/wu. /// -/// `min_fee` is the minimum fee (in sats) that the selection must have. +/// If this is a replacement spend using RBF, then `min_fee` should be set to +/// the total fees (in sats) of the transaction(s) being replaced, including any +/// descendants, which will ensure that RBF rule 4 is satisfied. +/// Otherwise, it should be set to 0. /// /// `max_sat_weight` is the maximum weight difference of an input in the /// transaction before and after satisfaction. @@ -304,24 +284,21 @@ fn select_coins_for_spend( must_have_change: bool, ) -> 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 - // of the original ordering of candidates so we can select any mandatory candidates using their - // original indices. - let mut base_weight: u32 = base_tx - .weight() - .to_wu() - .try_into() - .expect("Transaction weight must fit in u32"); - // Starting with version 0.31, rust-bitcoin now accounts for the segwit marker when serializing - // transactions with no input. But BDK's coin selector does add the segwit marker cost to the - // transaction size upon selecting the first segwit coin. To avoid accounting twice for it, - // drop it from the base weight (but only when it was added). - // NOTE: make sure to reconsider this when updating rust-bitcoin!! Behaviour may change again - // who knows. - if base_tx.input.is_empty() { - base_weight = base_weight.saturating_sub(2); - } + let out_weight_nochange: u32 = { + let mut total: u32 = 0; + for output in &base_tx.output { + let weight: u32 = output + .weight() + .to_wu() + .try_into() + .expect("an output's weight must fit in u32"); + total = total + .checked_add(weight) + .expect("sum of transaction outputs' weights must fit in u32"); + } + total + }; + let n_outputs_nochange = base_tx.output.len(); 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() @@ -368,7 +345,7 @@ fn select_coins_for_spend( is_segwit: true, // We only support receiving on Segwit scripts. }) .collect(); - let mut selector = CoinSelector::new(&candidates, base_weight); + let mut selector = CoinSelector::new(&candidates); for (i, cand) in candidate_coins.iter().enumerate() { if cand.must_select { // It's fine because the index passed to `select` refers to the original candidates ordering @@ -378,42 +355,53 @@ fn select_coins_for_spend( } // Now set the change policy. We use a policy which ensures no change output is created with a - // lower value than our custom dust limit. NOTE: the change output weight must account for a - // potential difference in the size of the outputs count varint. This is why we take the whole - // change txo as argument and compute the weight difference below. + // lower value than our custom dust limit. NOTE: the change output weight must not account for + // a potential difference in the size of the outputs count varint. + let feerate = FeeRate::from_sat_per_vb(feerate_vb); let long_term_feerate = FeeRate::from_sat_per_vb(LONG_TERM_FEERATE_VB); + let change_output_weight: u32 = change_txo + .weight() + .to_wu() + .try_into() + .expect("output weight must fit in u32"); let drain_weights = DrainWeights { - output_weight: { - // We don't reuse the above base_weight.since 2 WU may have been substracted from it. - // See comment above for details. - let nochange_weight = base_tx.weight().to_wu(); - let mut tx_with_change = base_tx; - tx_with_change.output.push(change_txo); - tx_with_change - .weight() - .to_wu() - .checked_sub(nochange_weight) - .expect("base_weight can't be larger") - .try_into() - .expect("tx size must always fit in u32") - }, + output_weight: change_output_weight, spend_weight: max_input_weight, + n_outputs: 1, // we only want a single change output }; - let change_policy = - change_policy::min_value_and_waste(drain_weights, DUST_OUTPUT_SATS, long_term_feerate); + // As of bdk_coin_select v0.3.0, the min change value is exclusive so we must subtract 1. + let change_min_value = DUST_OUTPUT_SATS.saturating_sub(1); + let change_policy = ChangePolicy::min_value_and_waste( + drain_weights, + change_min_value, + feerate, + long_term_feerate, + ); // Finally, run the coin selection algorithm. We use an opportunistic BnB and if it couldn't // find any solution we fall back to selecting coins by descending value. - let feerate = FeeRate::from_sat_per_vb(feerate_vb); + let replace = if min_fee > 0 { + Some(Replace::new(min_fee)) + } else { + None + }; + let target_fee = TargetFee { + rate: feerate, + replace, + }; + let target_outputs = TargetOutputs { + value_sum: out_value_nochange, + weight_sum: out_weight_nochange, + n_outputs: n_outputs_nochange, + }; let target = Target { - value: out_value_nochange, - feerate, - min_fee, + fee: target_fee, + outputs: target_outputs, }; let lowest_fee = LowestFee { target, long_term_feerate, - change_policy: &change_policy, + change_policy, }; let lowest_fee_change_cond = LowestFeeChangeCondition { lowest_fee, @@ -436,8 +424,10 @@ fn select_coins_for_spend( selector.sort_candidates_by_descending_value_pwu(); // 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) { + let drain = selector.drain(target, change_policy); + if selector.is_target_met_with_drain(target, drain) + && (drain.is_some() || !must_have_change) + { break; } if !selector.select_next() { @@ -457,7 +447,7 @@ 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 drain = selector.drain(target, change_policy); let change_amount = bitcoin::Amount::from_sat(drain.value); // Max available change is given by the excess when adding a change output with zero value. let drain_novalue = bdk_coin_select::Drain { @@ -536,7 +526,8 @@ pub trait TxGetter { pub enum SpendTxFees { /// The target feerate in sats/vb for this transaction. Regular(u64), - /// The (target feerate, minimum absolute fees) for this transactions. Both in sats. + /// The (target feerate in sats/vb, total fees in sats of transaction(s) to be replaced + /// including descendants) for this transaction. Rbf(u64, u64), } From 6376909ea1f9cb172558f8ae470a709b18757e20 Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Mon, 1 Apr 2024 17:46:44 +0100 Subject: [PATCH 2/3] commands: remove rbf rule 4 logic RBF rule 4 is now enforced by coin selection. --- src/commands/mod.rs | 64 ++++++++++++++++++--------------------------- 1 file changed, 26 insertions(+), 38 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 94d7ea24..7bb8c595 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -897,18 +897,13 @@ impl DaemonControl { if !is_cancel { candidate_coins.extend(&confirmed_cands); } - // Try with increasing fee until fee paid by replacement transaction is high enough. - // Replacement fee must be at least: - // sum of fees paid by original transactions + incremental feerate * replacement size. - // Loop will continue until either we find a suitable replacement or we have insufficient funds. - let mut replacement_vsize = 0; - for incremental_feerate in 0.. { - let min_fee = descendant_fees.to_sat() + replacement_vsize * incremental_feerate; - let CreateSpendRes { - psbt: rbf_psbt, - has_change, - warnings, - } = match create_spend( + // The min fee is the fee of the transaction being replaced and its descendants. Coin selection + // will ensure that the replacement transaction additionally pays for its own weight as per + // RBF rule 4. + let min_fee = descendant_fees.to_sat(); + // This loop can have up to 2 iterations in the case of cancel and otherwise only 1. + loop { + match create_spend( &self.config.main_descriptor, &self.secp, &mut tx_getter, @@ -917,7 +912,25 @@ impl DaemonControl { SpendTxFees::Rbf(feerate_vb, min_fee), change_address.clone(), ) { - Ok(psbt) => psbt, + Ok(CreateSpendRes { + psbt, + has_change, + warnings, + }) => { + // In case of success, make sure to update our next derivation index if any address + // used in the transaction outputs was from the future. + for (addr, _) in destinations { + self.maybe_increase_next_deriv_index(&mut db_conn, &addr.info); + } + if has_change { + self.maybe_increase_next_deriv_index(&mut db_conn, &change_address.info); + } + + return Ok(CreateSpendResult::Success { + psbt, + warnings: warnings.iter().map(|w| w.to_string()).collect(), + }); + } Err(SpendCreationError::CoinSelection(e)) => { // If we get a coin selection error due to insufficient funds and we want to cancel the // transaction, then set all previous coins as mandatory and add confirmed coins as @@ -936,32 +949,7 @@ impl DaemonControl { return Err(e.into()); } }; - replacement_vsize = self - .config - .main_descriptor - .unsigned_tx_max_vbytes(&rbf_psbt.unsigned_tx); - - // Make sure it satisfies RBF rule 4. - if rbf_psbt.fee().expect("has already been sanity checked") - >= descendant_fees + bitcoin::Amount::from_sat(replacement_vsize) - { - // In case of success, make sure to update our next derivation index if any address - // used in the transaction outputs was from the future. - for (addr, _) in destinations { - self.maybe_increase_next_deriv_index(&mut db_conn, &addr.info); - } - if has_change { - self.maybe_increase_next_deriv_index(&mut db_conn, &change_address.info); - } - - return Ok(CreateSpendResult::Success { - psbt: rbf_psbt, - warnings: warnings.iter().map(|w| w.to_string()).collect(), - }); - } } - - unreachable!("We keep increasing the min fee until we run out of funds or satisfy rule 4.") } /// Trigger a rescan of the block chain for transactions involving our main descriptor between From 43ecd94a46a8abf82a6882e741aa1db2fd0abd7c Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Fri, 5 Apr 2024 21:26:09 +0100 Subject: [PATCH 3/3] spend: change parameter type for rbf The parameter has been renamed and changed to `Option` as it is only required when creating a replacement transaction using RBF. --- src/commands/mod.rs | 6 +++--- src/spend.rs | 20 ++++++++------------ 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 7bb8c595..f2102014 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -897,10 +897,10 @@ impl DaemonControl { if !is_cancel { candidate_coins.extend(&confirmed_cands); } - // The min fee is the fee of the transaction being replaced and its descendants. Coin selection + // The replaced fee is the fee of the transaction being replaced and its descendants. Coin selection // will ensure that the replacement transaction additionally pays for its own weight as per // RBF rule 4. - let min_fee = descendant_fees.to_sat(); + let replaced_fee = descendant_fees.to_sat(); // This loop can have up to 2 iterations in the case of cancel and otherwise only 1. loop { match create_spend( @@ -909,7 +909,7 @@ impl DaemonControl { &mut tx_getter, &destinations, &candidate_coins, - SpendTxFees::Rbf(feerate_vb, min_fee), + SpendTxFees::Rbf(feerate_vb, replaced_fee), change_address.clone(), ) { Ok(CreateSpendRes { diff --git a/src/spend.rs b/src/spend.rs index 3d2326c9..dc403e88 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -264,10 +264,10 @@ impl bdk_coin_select::BnbMetric for LowestFeeChangeCondition { /// and change may result in a slightly lower feerate than this as the underlying /// function instead uses a minimum feerate of `feerate_vb / 4.0` sats/wu. /// -/// If this is a replacement spend using RBF, then `min_fee` should be set to +/// If this is a replacement spend using RBF, then `replaced_fee` should be set to /// the total fees (in sats) of the transaction(s) being replaced, including any /// descendants, which will ensure that RBF rule 4 is satisfied. -/// Otherwise, it should be set to 0. +/// Otherwise, it should be `None`. /// /// `max_sat_weight` is the maximum weight difference of an input in the /// transaction before and after satisfaction. @@ -279,7 +279,7 @@ fn select_coins_for_spend( base_tx: bitcoin::Transaction, change_txo: bitcoin::TxOut, feerate_vb: f32, - min_fee: u64, + replaced_fee: Option, max_sat_weight: u32, must_have_change: bool, ) -> Result { @@ -380,11 +380,7 @@ fn select_coins_for_spend( // Finally, run the coin selection algorithm. We use an opportunistic BnB and if it couldn't // find any solution we fall back to selecting coins by descending value. - let replace = if min_fee > 0 { - Some(Replace::new(min_fee)) - } else { - None - }; + let replace = replaced_fee.map(Replace::new); let target_fee = TargetFee { rate: feerate, replace, @@ -617,9 +613,9 @@ pub fn create_spend( // 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), + let (feerate_vb, replaced_fee) = match fees { + SpendTxFees::Regular(feerate) => (feerate, None), + SpendTxFees::Rbf(feerate, fee) => (feerate, Some(fee)), }; let is_self_send = destinations.is_empty(); if feerate_vb < 1 { @@ -696,7 +692,7 @@ pub fn create_spend( tx.clone(), change_txo.clone(), feerate_vb, - min_fee, + replaced_fee, max_sat_wu, is_self_send, )