Merge #1559: bump bdk_coin_select to 0.4

1859a2d5f0fe67b22a850d0f7daac43b06bce163 bump bdk_coin_select to 0.4 (Michael Mallan)

Pull request description:

  Weights are now stored in the crate as `u64` instead of `u32` and so we can keep our weights and vsizes as `u64`.

  The tests have been updated to reflect that coin selection now calculates feerates using a transaction's size in rounded-up vbytes, which fixes #1132.

ACKs for top commit:
  edouardparis:
    ACK 1859a2d5f0fe67b22a850d0f7daac43b06bce163

Tree-SHA512: 703edd66db85fff0bef34eee282493c371ad7f3ff512f4c0b7c39ee7fc8b2878647c19b81ab31f9d04cbd7d8770455dfcf8929587aa3e98025ce9ccd31027350

Cargo.lock

@@ -515,9 +515,9 @@ dependencies = [
 
 [[package]]
 name = "bdk_coin_select"
-version = "0.3.0"
+version = "0.4.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "3c084bf76f0f67546fc814ffa82044144be1bb4618183a15016c162f8b087ad4"
+checksum = "d2e57e4db8b917d554a0eb142be5267bbc88d787c3346c8dc0590fbe76be68bd"
 
 [[package]]
 name = "bdk_electrum"

liana/Cargo.toml

@@ -13,7 +13,7 @@ description = "Liana development kit"
 miniscript = { version = "12.0", features = ["serde", "compiler", "base64"] }
 
 # Coin selection algorithms for spend transaction creation.
-bdk_coin_select = "0.3"
+bdk_coin_select = "0.4"
 # We use TOML for the config, and JSON for RPC
 serde = { version = "1.0", features = ["derive"] }
 

liana/src/spend.rs

@@ -167,7 +167,7 @@ fn sanity_check_psbt(
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub struct AncestorInfo {
-    pub vsize: u32,
+    pub vsize: u64,
     pub fee: u32,
 }
 
@@ -269,35 +269,31 @@ fn select_coins_for_spend(
     change_txo: bitcoin::TxOut,
     feerate_vb: f32,
     replaced_fee: Option<u64>,
-    max_sat_weight: u32,
+    max_sat_weight: u64,
     must_have_change: bool,
 ) -> Result<CoinSelectionRes, InsufficientFunds> {
     let out_value_nochange = base_tx.output.iter().map(|o| o.value.to_sat()).sum();
-    let out_weight_nochange: u32 = {
+    let out_weight_nochange = {
-        let mut total: u32 = 0;
+        let mut total: u64 = 0;
         for output in &base_tx.output {
-            let weight: u32 = output
+            let weight = output.weight().to_wu();
-                .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");
+                .expect("sum of transaction outputs' weights must fit in u64");
         }
         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.
+    // Get feerate as u64 for calculation relating to ancestor below.
     // We expect `feerate_vb` to be a positive integer, but take ceil()
     // just in case to be sure we pay enough for ancestors.
-    let feerate_vb_u32 = feerate_vb.ceil() as u32;
+    let feerate_vb_u64 = feerate_vb.ceil() as u64;
-    let witness_factor: u32 = WITNESS_SCALE_FACTOR
+    let witness_factor: u64 = WITNESS_SCALE_FACTOR
         .try_into()
-        .expect("scale factor must fit in u32");
+        .expect("scale factor must fit in u64");
     // This will be used to store any extra weight added to candidates.
-    let mut added_weights = HashMap::<bitcoin::OutPoint, u32>::with_capacity(candidate_coins.len());
+    let mut added_weights = HashMap::<bitcoin::OutPoint, u64>::with_capacity(candidate_coins.len());
     let candidates: Vec<Candidate> = candidate_coins
         .iter()
         .map(|cand| Candidate {
@@ -308,9 +304,8 @@ fn select_coins_for_spend(
                     .ancestor_info
                     .map(|info| {
                         // The implied ancestor vsize if the fee had been paid at our target feerate.
-                        let ancestor_vsize_at_feerate: u32 = info
+                        let ancestor_vsize_at_feerate = <u32 as Into<u64>>::into(info.fee)
-                            .fee
+                            .checked_div(feerate_vb_u64)
-                            .checked_div(feerate_vb_u32)
                             .expect("feerate is greater than zero");
                         // If the actual ancestor vsize is bigger than the implied vsize, we will need to
                         // pay the difference in order for the combined feerate to be at the target value.
@@ -321,7 +316,7 @@ fn select_coins_for_spend(
                         info.vsize
                             .saturating_sub(ancestor_vsize_at_feerate)
                             .checked_mul(witness_factor)
-                            .expect("weight difference must fit in u32")
+                            .expect("weight difference must fit in u64")
                     })
                     .unwrap_or(0);
                 // Store the extra weight for this candidate for use later on.
@@ -329,7 +324,7 @@ fn select_coins_for_spend(
                 assert!(added_weights.insert(cand.outpoint, extra).is_none());
                 max_input_weight
                     .checked_add(extra)
-                    .expect("effective weight must fit in u32")
+                    .expect("effective weight must fit in u64")
             },
             is_segwit: true, // We only support receiving on Segwit scripts.
         })
@@ -348,11 +343,7 @@ fn select_coins_for_spend(
     // 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
+    let change_output_weight = change_txo.weight().to_wu();
-        .weight()
-        .to_wu()
-        .try_into()
-        .expect("output weight must fit in u32");
     let drain_weights = DrainWeights {
         output_weight: change_output_weight,
         spend_weight: max_input_weight,
@@ -446,7 +437,7 @@ fn select_coins_for_spend(
             .try_into()
             .expect("value is non-negative"),
     );
-    let mut total_added_weight: u32 = 0;
+    let mut total_added_weight: u64 = 0;
     let selected = selector
         .selected_indices()
         .iter()
@@ -458,7 +449,7 @@ fn select_coins_for_spend(
                         .get(&cand.outpoint)
                         .expect("contains added weight for all candidates"),
                 )
-                .expect("should fit in u32")
+                .expect("should fit in u64")
         })
         .collect();
     // Calculate added fee based on the feerate in sats/wu, which is the feerate used for coin selection.
@@ -719,7 +710,7 @@ pub fn create_spend(
         let max_sat_wu = main_descriptor
             .max_sat_weight(use_primary_path)
             .try_into()
-            .expect("Weight must fit in a u32");
+            .expect("Weight must fit in a u64");
         select_coins_for_spend(
             candidate_coins,
             tx.clone(),

lianad/src/commands/mod.rs

@@ -512,10 +512,7 @@ impl DaemonControl {
                             self.bitcoin
                                 .mempool_entry(&op.txid)
                                 .map(|info| AncestorInfo {
-                                    vsize: info
+                                    vsize: info.ancestor_vsize,
-                                        .ancestor_vsize
-                                        .try_into()
-                                        .expect("vsize must fit in u32"),
                                     fee: info
                                         .fees
                                         .ancestor
@@ -559,10 +556,7 @@ impl DaemonControl {
                         self.bitcoin
                             .mempool_entry(&op.txid)
                             .map(|info| AncestorInfo {
-                                vsize: info
+                                vsize: info.ancestor_vsize,
-                                    .ancestor_vsize
-                                    .try_into()
-                                    .expect("vsize must fit in u32"),
                                 fee: info
                                     .fees
                                     .ancestor

tests/test_rpc.py

@@ -1187,7 +1187,7 @@ def test_rbfpsbt_bump_fee(lianad, bitcoind):
     rbf_1_feerate = (
         mempool_rbf_1["fees"]["ancestor"] * COIN / mempool_rbf_1["ancestorsize"]
     )
-    assert 9.75 < rbf_1_feerate < 10.25
+    assert 10 <= rbf_1_feerate < 10.25
     # If we try to RBF the first transaction again, it will not be possible as we
     # deleted the PSBT above and the tx is no longer part of our wallet's
     # spending txs (even though it's saved in the DB).

tests/test_spend.py

@@ -254,17 +254,11 @@ def test_send_to_self(lianad, bitcoind):
     lianad.rpc.broadcastspend(spend_txid)
 
     # The only output is the change output so the feerate of the transaction must
-    # not be much lower than the one provided (it could be slightly lower since
+    # not be lower than the one provided and only possibly slightly higher
-    # the change amount is determined using feerate in terms of sat/wu which, due
+    # (since we slightly overestimate the satisfaction size).
-    # to rounding, can lead to a slightly lower feerate in terms of sat/vb,
-    # especially when the number of inputs increases), and only possibly slightly
-    # higher (since we slightly overestimate the satisfaction size).
     res = bitcoind.rpc.getmempoolentry(spend_txid)
     spend_feerate = res["fees"]["base"] * COIN / res["vsize"]  # keep as decimal
-    if USE_TAPROOT:
+    assert specified_feerate <= spend_feerate < specified_feerate + 0.51
-        assert specified_feerate <= spend_feerate < specified_feerate + 0.5
-    else:
-        assert specified_feerate - 0.5 < spend_feerate < specified_feerate + 0.5
 
     # We should by now only have one coin.
     bitcoind.generate_block(1, wait_for_mempool=spend_txid)
@@ -334,7 +328,7 @@ def test_coin_selection(lianad, bitcoind):
     )
     # txid_1's feerate is approx 2 sat/vb as required.
     txid_1_feerate = anc_fees / anc_vsize
-    assert 1.99 < txid_1_feerate < 2.01
+    assert 2 <= txid_1_feerate < 2.01
     wait_for(lambda: len(lianad.rpc.listcoins()["coins"]) == 2)
     # Check that change output is unconfirmed.
     assert len(lianad.rpc.listcoins(["unconfirmed"])["coins"]) == 1
@@ -391,21 +385,7 @@ def test_coin_selection(lianad, bitcoind):
     assert "psbt" in spend_res_2
     spend_psbt_2 = PSBT.from_base64(spend_res_2["psbt"])
     spend_txid_2 = spend_psbt_2.tx.txid().hex()
-    if USE_TAPROOT:
+    assert len(spend_res_2["warnings"]) == 0
-        assert len(spend_res_2["warnings"]) == 0
-    else:
-        # We still get a warning in the non-taproot case.
-        assert len(spend_res_2["warnings"]) == 1
-        additional_fee_at_2satvb = additional_fees(anc_vsize, anc_fees, feerate)
-        assert additional_fee_at_3satvb > additional_fee_at_2satvb
-        assert len(spend_res_2["warnings"]) == 1
-        assert (
-            spend_res_2["warnings"][0]
-            == "CPFP: an unconfirmed input was selected. The current transaction fee "
-            f"was increased by {additional_fee_at_2satvb} sats to make the average "
-            "feerate of both the input and current transaction equal to the selected "
-            "feerate."
-        )
 
     # The spend is using the unconfirmed change.
     assert spend_psbt_2.tx.vin[0].prevout.hash == uint256_from_str(
@@ -478,8 +458,8 @@ def test_coin_selection(lianad, bitcoind):
     # If we subtract the extra that pays for the ancestor, the feerate is approximately
     # at the target value.
     assert (
-        feerate - 0.5
+        feerate
-        < ((mempool_txid_3["fees"]["base"] * COIN) - additional_fee)
+        <= ((mempool_txid_3["fees"]["base"] * COIN) - additional_fee)
         / mempool_txid_3["vsize"]
         < feerate + 0.5
     )
@@ -634,7 +614,4 @@ def test_tr_multisig_2_of_2_feerate_is_met(feerate, lianad_multisig_2_of_2, bitc
     spend_fee = res["fees"]["base"] * COIN
     spend_weight = res["weight"]
     assert spend_weight == 646
-
+    assert spend_fee == math.ceil(646.0 / 4.0) * feerate
-    # Note that due to https://github.com/wizardsardine/liana/issues/1132
-    # we do not round up vbytes before multiplying by feerate.
-    assert spend_fee == math.ceil((646.0 / 4.0) * feerate)