nik/liana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge #1076: Bump bdk_coin_select to 0.3.0

Browse Source 
43ecd94a46a8abf82a6882e741aa1db2fd0abd7c spend: change parameter type for rbf (jp1ac4)
6376909ea1f9cb172558f8ae470a709b18757e20 commands: remove rbf rule 4 logic (jp1ac4)
936d7e958567608a46e91c21699d611ca5eced1d spend: bump bdk_coin_select to 0.3.0 (jp1ac4)

Pull request description:

  This is to resolve #923.

  The `score` method of the `LowestFee` metric has been fixed upstream and so our temporary partial fix from #867 is no longer required.

  The `min_fee` parameter of `select_coins_for_spend`, if positive, now ensures that RBF rule 4 is satisfied. Accordingly, it has been renamed to `replaced_fee` and made an `Option`. We could potentially have used the `SpendTxFees` enum as a parameter directly instead of `feerate_vb` and `replaced_fee`, but `feerate_vb` is currently `f32` rather than `u64` and so I kept them as separate parameters.

  Thanks to how the `replaced_fee` parameter works, the fee iteration approach used in `rbf_psbt` to ensure the replacement satisfies [RBF rule 4](https://github.com/bitcoin/bitcoin/blob/master/doc/policy/mempool-replacements.md#current-replace-by-fee-policy) is no longer required.

  `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 as this is now handled by bdk_coin_select.

  The min value for change no longer includes the threshold itself and so we have to subtract 1 from our change policy's min value (see https://github.com/bitcoindevkit/coin-select/pull/14#discussion_r1439665103). We already have a [test](https://github.com/wizardsardine/liana/blob/master/src/commands/mod.rs#L1653-#L1654) that fails without this subtraction as it expects a change output of 5000 sats.

ACKs for top commit:
  darosior:
    ACK 43ecd94a46a8abf82a6882e741aa1db2fd0abd7c

Tree-SHA512: a064bafef13abefcb8c4b640cfc4017eb288c62891a8b486add33c1499e7061bf262d6aadabbdd4c191ed9b81e3931b382c8c8445e6bb9c1b052380caf14b3f9
This commit is contained in:
Antoine Poinsot 2024-04-15 13:13:15 +02:00
commit 8c55eeca1d
No known key found for this signature in database
GPG Key ID: E13FC145CD3F4304
4 changed files with 103 additions and 143 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
Cargo.lock generated
View File

@ -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",
]

2
Cargo.toml
View File

@ -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"

66
src/commands/mod.rs
View File

@ -897,27 +897,40 @@ 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 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 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(
&self.config.main_descriptor,
&self.secp,
&mut tx_getter,
&destinations,
&candidate_coins,
SpendTxFees::Rbf(feerate_vb, min_fee),
SpendTxFees::Rbf(feerate_vb, replaced_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

157
src/spend.rs
View File

@ -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<bdk_coin_select::float::Ordf32> {
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<bdk_coin_select::float::Ordf32> {
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<bdk_coin_select::float::Ordf32> {
fn bound(&mut self, cs: &CoinSelector) -> Option<bdk_coin_select::float::Ordf32> {
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 `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 `None`.
///
/// `max_sat_weight` is the maximum weight difference of an input in the
/// transaction before and after satisfaction.
@ -299,29 +279,26 @@ fn select_coins_for_spend(
base_tx: bitcoin::Transaction,
change_txo: bitcoin::TxOut,
feerate_vb: f32,
min_fee: u64,
replaced_fee: Option<u64>,
max_sat_weight: u32,
must_have_change: bool,
) -> 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
// 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,49 @@ 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 = replaced_fee.map(Replace::new);
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 +420,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 +443,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 +522,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),
}
@ -626,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 {
@ -705,7 +692,7 @@ pub fn create_spend(
tx.clone(),
change_txo.clone(),
feerate_vb,
min_fee,
replaced_fee,
max_sat_wu,
is_self_send,
)