diff --git a/Cargo.lock b/Cargo.lock index 4237332a4..56ebe194a 100644 --- a/Cargo.lock +++ b/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" diff --git a/liana/Cargo.toml b/liana/Cargo.toml index e25063f54..9779dc48c 100644 --- a/liana/Cargo.toml +++ b/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"] } diff --git a/liana/src/spend.rs b/liana/src/spend.rs index 3f9cb2244..c4f350cfc 100644 --- a/liana/src/spend.rs +++ b/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, - max_sat_weight: u32, + max_sat_weight: u64, must_have_change: bool, ) -> Result { let out_value_nochange = base_tx.output.iter().map(|o| o.value.to_sat()).sum(); - let out_weight_nochange: u32 = { - let mut total: u32 = 0; + let out_weight_nochange = { + let mut total: u64 = 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"); + let weight = output.weight().to_wu(); 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 witness_factor: u32 = WITNESS_SCALE_FACTOR + let feerate_vb_u64 = feerate_vb.ceil() as u64; + 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::::with_capacity(candidate_coins.len()); + let mut added_weights = HashMap::::with_capacity(candidate_coins.len()); let candidates: Vec = 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 - .fee - .checked_div(feerate_vb_u32) + let ancestor_vsize_at_feerate = >::into(info.fee) + .checked_div(feerate_vb_u64) .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 - .weight() - .to_wu() - .try_into() - .expect("output weight must fit in u32"); + let change_output_weight = change_txo.weight().to_wu(); 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(), diff --git a/lianad/src/commands/mod.rs b/lianad/src/commands/mod.rs index e36df88de..dec64a3a4 100644 --- a/lianad/src/commands/mod.rs +++ b/lianad/src/commands/mod.rs @@ -512,10 +512,7 @@ impl DaemonControl { self.bitcoin .mempool_entry(&op.txid) .map(|info| AncestorInfo { - vsize: info - .ancestor_vsize - .try_into() - .expect("vsize must fit in u32"), + vsize: info.ancestor_vsize, fee: info .fees .ancestor @@ -559,10 +556,7 @@ impl DaemonControl { self.bitcoin .mempool_entry(&op.txid) .map(|info| AncestorInfo { - vsize: info - .ancestor_vsize - .try_into() - .expect("vsize must fit in u32"), + vsize: info.ancestor_vsize, fee: info .fees .ancestor diff --git a/tests/test_rpc.py b/tests/test_rpc.py index 8ade015f2..4ba57726e 100644 --- a/tests/test_rpc.py +++ b/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). diff --git a/tests/test_spend.py b/tests/test_spend.py index 1c248325e..f53520b9c 100644 --- a/tests/test_spend.py +++ b/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 - # the change amount is determined using feerate in terms of sat/wu which, due - # 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). + # not be lower than the one provided 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.5 - else: - assert specified_feerate - 0.5 < spend_feerate < specified_feerate + 0.5 + assert specified_feerate <= spend_feerate < specified_feerate + 0.51 # 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 - 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." - ) + assert len(spend_res_2["warnings"]) == 0 # 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 - < ((mempool_txid_3["fees"]["base"] * COIN) - additional_fee) + feerate + <= ((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 - - # 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) + assert spend_fee == math.ceil(646.0 / 4.0) * feerate