From f4a5cc8dcb7206d980bd05b8f2e4ed87d5af0f09 Mon Sep 17 00:00:00 2001 From: Jonas <281324108+xn1xn1@users.noreply.github.com> Date: Sun, 3 May 2026 13:56:32 -0400 Subject: [PATCH 1/3] bitcoin-wallet: Consider unconfirmed parent fees during PSBT fee estimation --- bitcoin-wallet/src/wallet.rs | 367 ++++++++++++++++++++++++++--------- 1 file changed, 270 insertions(+), 97 deletions(-) diff --git a/bitcoin-wallet/src/wallet.rs b/bitcoin-wallet/src/wallet.rs index 6183aad3c..678b5988b 100644 --- a/bitcoin-wallet/src/wallet.rs +++ b/bitcoin-wallet/src/wallet.rs @@ -11,6 +11,7 @@ use bdk_wallet::bitcoin::FeeRate; use bdk_wallet::bitcoin::Network; use bdk_wallet::export::FullyNodedExport; use bdk_wallet::rusqlite::Connection; +use bdk_wallet::chain::ChainPosition; use bdk_wallet::template::{Bip84, DescriptorTemplate}; use bdk_wallet::{Balance, PersistedWallet}; use bitcoin::bip32::Xpriv; @@ -23,6 +24,7 @@ use rust_decimal::Decimal; use rust_decimal::prelude::*; use std::collections::BTreeMap; use std::collections::HashMap; +use std::collections::HashSet; use std::fmt::Debug; use std::path::Path; use std::path::PathBuf; @@ -1359,8 +1361,7 @@ where tx_builder.finish()? }; - let weight = psbt.unsigned_tx.weight(); - let fee = self.estimate_fee(weight, Some(amount)).await?; + let fee = self.estimate_fee_for_psbt(&psbt, Some(amount)).await?; self.send_to_address(address, amount, fee, change_override) .await @@ -1450,16 +1451,17 @@ where /// /// Returns a tuple of (max_giveable_amount, spending_fee). pub async fn max_giveable(&self, locking_script_size: usize) -> Result<(Amount, Amount)> { - let mut wallet = self.wallet.lock().await; + let (dummy_max_giveable, dummy_psbt) = { + let mut wallet = self.wallet.lock().await; - // Construct a dummy drain transaction - let dummy_script = ScriptBuf::from(vec![0u8; locking_script_size]); + // Construct a dummy drain transaction + let dummy_script = ScriptBuf::from(vec![0u8; locking_script_size]); - let mut tx_builder = wallet.build_tx(); + let mut tx_builder = wallet.build_tx(); - tx_builder.drain_to(dummy_script.clone()); - tx_builder.fee_absolute(Amount::ZERO); - tx_builder.drain_wallet(); + tx_builder.drain_to(dummy_script.clone()); + tx_builder.fee_absolute(Amount::ZERO); + tx_builder.drain_wallet(); // The weight WILL NOT change, even if we change the fee // because we are draining the wallet (using all inputs) and @@ -1475,90 +1477,89 @@ where bail!("Expected a single output in the dummy transaction"); } - let max_giveable = psbt.unsigned_tx.output.first().expect("Expected a single output in the dummy transaction").value; - let weight = psbt.unsigned_tx.weight(); - - Ok((Some(max_giveable), weight)) - }, - Err(bdk_wallet::error::CreateTxError::CoinSelection(_)) => { - // We don't have enough funds to create a transaction (below dust limit) - // - // We still want to to return a valid fee. - // Callers of this function might want to calculate *how* large - // the next UTXO needs to be such that we can spend any funds - // - // To be able to calculate an accurate fee, we need to figure out - // the weight our drain transaction if we received another UTXO - - // We create fake deposit UTXO - // Our dummy drain transaction will spend this deposit UTXO - let mut fake_deposit_input = bitcoin::psbt::Input::default(); - - let dummy_deposit_address = wallet.peek_address(KeychainKind::External, 0); - let fake_deposit_script = dummy_deposit_address.script_pubkey(); - let fake_deposit_txout = bitcoin::blockdata::transaction::TxOut { - // The exact deposit amount does not matter - // because we only care about the weight of the transaction - // which does not depend on the amount of the input - value: DUST_AMOUNT * 5, - script_pubkey: fake_deposit_script, - }; - let fake_deposit_tx = bitcoin::Transaction { - version: bitcoin::blockdata::transaction::Version::TWO, - lock_time: bitcoin::blockdata::locktime::absolute::LockTime::ZERO, - input: vec![bitcoin::TxIn { - previous_output: bitcoin::OutPoint::null(), // or some dummy outpoint - script_sig: Default::default(), - sequence: bitcoin::Sequence::ENABLE_RBF_NO_LOCKTIME, - witness: Default::default(), - }], - output: vec![fake_deposit_txout.clone()], - }; - - let fake_deposit_txid = fake_deposit_tx.compute_txid(); - - fake_deposit_input.witness_utxo = Some(fake_deposit_txout); - fake_deposit_input.non_witness_utxo = Some(fake_deposit_tx); - - // Create outpoint that points to our fake transaction's output 0 - let fake_deposit_outpoint = bitcoin::OutPoint { - txid: fake_deposit_txid, - vout: 0, - }; + let max_giveable = psbt.unsigned_tx.output.first().expect("Expected a single output in the dummy transaction").value; - // Worst-case witness weight for our script type. - const DUMMY_SATISFACTION_WEIGHT: Weight = Weight::from_wu(107 * 10); - - let mut tx_builder = wallet.build_tx(); - - tx_builder.drain_to(dummy_script.clone()); - tx_builder.fee_absolute(Amount::ZERO); - tx_builder.drain_wallet(); - - tx_builder - .add_foreign_utxo( - fake_deposit_outpoint, - fake_deposit_input, - DUMMY_SATISFACTION_WEIGHT, - ).context("Failed to add dummy foreign utxo to calculate fee for max_giveable if we had one more utxo")?; - - // Try building the dummy drain transaction with the new fake UTXO - // If we fail now, we propagate the error to the caller - let psbt = tx_builder.finish()?; - let weight = psbt.unsigned_tx.weight(); + Ok((Some(max_giveable), psbt)) + }, + Err(bdk_wallet::error::CreateTxError::CoinSelection(_)) => { + // We don't have enough funds to create a transaction (below dust limit) + // + // We still want to to return a valid fee. + // Callers of this function might want to calculate *how* large + // the next UTXO needs to be such that we can spend any funds + // + // To be able to calculate an accurate fee, we need to figure out + // the weight our drain transaction if we received another UTXO + + // We create fake deposit UTXO + // Our dummy drain transaction will spend this deposit UTXO + let mut fake_deposit_input = bitcoin::psbt::Input::default(); + + let dummy_deposit_address = wallet.peek_address(KeychainKind::External, 0); + let fake_deposit_script = dummy_deposit_address.script_pubkey(); + let fake_deposit_txout = bitcoin::blockdata::transaction::TxOut { + // The exact deposit amount does not matter + // because we only care about the weight of the transaction + // which does not depend on the amount of the input + value: DUST_AMOUNT * 5, + script_pubkey: fake_deposit_script, + }; + let fake_deposit_tx = bitcoin::Transaction { + version: bitcoin::blockdata::transaction::Version::TWO, + lock_time: bitcoin::blockdata::locktime::absolute::LockTime::ZERO, + input: vec![bitcoin::TxIn { + previous_output: bitcoin::OutPoint::null(), // or some dummy outpoint + script_sig: Default::default(), + sequence: bitcoin::Sequence::ENABLE_RBF_NO_LOCKTIME, + witness: Default::default(), + }], + output: vec![fake_deposit_txout.clone()], + }; + + let fake_deposit_txid = fake_deposit_tx.compute_txid(); + + fake_deposit_input.witness_utxo = Some(fake_deposit_txout); + fake_deposit_input.non_witness_utxo = Some(fake_deposit_tx); + + // Create outpoint that points to our fake transaction's output 0 + let fake_deposit_outpoint = bitcoin::OutPoint { + txid: fake_deposit_txid, + vout: 0, + }; + + // Worst-case witness weight for our script type. + const DUMMY_SATISFACTION_WEIGHT: Weight = Weight::from_wu(107 * 10); + + let mut tx_builder = wallet.build_tx(); + + tx_builder.drain_to(dummy_script.clone()); + tx_builder.fee_absolute(Amount::ZERO); + tx_builder.drain_wallet(); + + tx_builder + .add_foreign_utxo( + fake_deposit_outpoint, + fake_deposit_input, + DUMMY_SATISFACTION_WEIGHT, + ).context("Failed to add dummy foreign utxo to calculate fee for max_giveable if we had one more utxo")?; + + // Try building the dummy drain transaction with the new fake UTXO + // If we fail now, we propagate the error to the caller + let psbt = tx_builder.finish()?; - tracing::trace!( - weight = weight.to_wu(), - "Built dummy drain transaction with fake UTXO, max giveable is 0" - ); + tracing::trace!( + weight = psbt.unsigned_tx.weight().to_wu(), + "Built dummy drain transaction with fake UTXO, max giveable is 0" + ); - Ok((None, weight)) - } - Err(e) => Err(e) - }.context("Failed to build transaction to figure out max giveable")?; + Ok((None, psbt)) + } + Err(e) => Err(e) + }.context("Failed to build transaction to figure out max giveable")? + }; - // Estimate the fee rate using our real fee rate estimation - let fee = self.estimate_fee(dummy_weight, dummy_max_giveable).await?; + // Estimate fee, accounting for any unconfirmed parents (CPFP) + let fee = self.estimate_fee_for_psbt(&dummy_psbt, dummy_max_giveable).await?; Ok(match dummy_max_giveable { // If the max giveable is less than the dust amount, we return 0 @@ -1609,6 +1610,172 @@ where estimate_fee(weight, transfer_amount, fee_rate, min_relay_fee) } + + /// Estimates the fee for a transaction represented by a PSBT, taking unconfirmed parent transactions into account (CPFP package fee). + /// + /// Recursively collects all unconfirmed ancestors; if any exist, the child fee is increased so that the full package (parents + child) + /// reaches the target fee rate. Fee caps (relative and absolute) are applied with warnings. + async fn estimate_fee_for_psbt( + &self, + psbt: &Psbt, + transfer_amount: Option, + ) -> Result { + let fee_rate = self.combined_fee_rate().await?; + let min_relay_fee = self.combined_min_relay_fee().await?; + let target_fee_rate = compute_effective_fee_rate(fee_rate, min_relay_fee)?; + + let child_only_fee = estimate_fee( + psbt.unsigned_tx.weight(), + transfer_amount, + target_fee_rate, + min_relay_fee, + )?; + + let (ancestor_fee, ancestor_weight) = self.collect_unconfirmed_ancestors(psbt).await?; + + if ancestor_weight == Weight::from_wu(0) { + return Ok(child_only_fee); + } + + let total_weight = Weight::from_wu( + psbt.unsigned_tx + .weight() + .to_wu() + .saturating_add(ancestor_weight.to_wu()), + ); + + let package_fee = target_fee_rate + .checked_mul_by_weight(total_weight) + .context("Failed to compute package fee")?; + + let required_child_fee = Amount::from_sat( + package_fee + .to_sat() + .saturating_sub(ancestor_fee.to_sat()), + ); + + let fee = child_only_fee.max(required_child_fee); + Ok(Self::clamp_cpfp_fee(fee, transfer_amount)) + } + + /// Walks all unconfirmed ancestors of every input in `psbt` and returns the total fee already paid by those ancestors + their total weight. + /// Confirmed ancestors are ignored. If a parent's fee cannot be calculated, that ancestor is skipped entirely. + async fn collect_unconfirmed_ancestors( + &self, + psbt: &Psbt, + ) -> Result<(Amount, Weight)> { + const MAX_ANCESTOR_DEPTH: usize = 3; + + let mut seen = HashSet::::new(); + let mut stack: Vec<(Txid, usize)> = psbt + .unsigned_tx + .input + .iter() + .map(|input| (input.previous_output.txid, 0)) + .collect(); + + let mut ancestor_fee = Amount::ZERO; + let mut ancestor_weight = Weight::from_wu(0); + + while let Some((txid, depth)) = stack.pop() { + if depth >= MAX_ANCESTOR_DEPTH { + continue; + } + + if !seen.insert(txid) { + continue; + } + + let (parent_tx, parent_fee) = { + let wallet = self.wallet.lock().await; + + // Local wallet lookup only. This reads from the synced tx graph and doesn't hit the network. + let Some(wallet_tx) = wallet.get_tx(txid) else { + continue; + }; + + if matches!(wallet_tx.chain_position, ChainPosition::Confirmed { .. }) { + continue; + } + + let parent_tx = wallet_tx.tx_node.tx.clone(); + + let parent_fee = match wallet.calculate_fee(&parent_tx) { + Ok(fee) => fee, + Err(err) => { + tracing::debug!( + %txid, + error = %err, + "Cannot compute fee for unconfirmed ancestor; skipping it" + ); + continue; + } + }; + + (parent_tx, parent_fee) + }; + + ancestor_fee = Amount::from_sat( + ancestor_fee.to_sat().saturating_add(parent_fee.to_sat()), + ); + ancestor_weight = Weight::from_wu( + ancestor_weight.to_wu().saturating_add(parent_tx.weight().to_wu()), + ); + + stack.extend( + parent_tx + .input + .iter() + .map(|input| (input.previous_output.txid, depth + 1)), + ); + } + + Ok((ancestor_fee, ancestor_weight)) + } + + /// Clamps a CPFP fee to the allowed range (relative cap -> min absolute -> hard cap) + fn clamp_cpfp_fee(fee: Amount, transfer_amount: Option) -> Amount { + let mut final_fee = fee; + + if let Some(transfer_amount) = transfer_amount { + let relative_max = Amount::from_sat( + MAX_RELATIVE_TX_FEE + .saturating_mul(Decimal::from(transfer_amount.to_sat())) + .ceil() + .to_u64() + .expect("MAX_RELATIVE_TX_FEE * transfer_amount fits in u64"), + ); + if final_fee > relative_max { + tracing::warn!( + "CPFP fee {} exceeds relative cap {} ({}% of transfer amount). Clamping.", + final_fee.to_sat(), + relative_max.to_sat(), + MAX_RELATIVE_TX_FEE.saturating_mul(Decimal::from(100)).ceil().to_u64().unwrap(), + ); + final_fee = relative_max; + } + } + + if final_fee < MIN_ABSOLUTE_TX_FEE { + tracing::warn!( + "CPFP fee {} below absolute minimum {}. Setting to minimum.", + final_fee.to_sat(), + MIN_ABSOLUTE_TX_FEE.to_sat(), + ); + final_fee = MIN_ABSOLUTE_TX_FEE; + } + + if final_fee > MAX_ABSOLUTE_TX_FEE { + tracing::warn!( + "CPFP fee {} exceeds absolute hard cap {}. Capping.", + final_fee.to_sat(), + MAX_ABSOLUTE_TX_FEE.to_sat(), + ); + final_fee = MAX_ABSOLUTE_TX_FEE; + } + + final_fee + } } impl Client { @@ -2443,6 +2610,19 @@ pub fn trace_status_change( new } +fn compute_effective_fee_rate( + fee_rate: FeeRate, + min_relay_fee: FeeRate, +) -> Result { + FeeRate::from_sat_per_vb( + fee_rate + .to_sat_per_vb_ceil() + .max(min_relay_fee.to_sat_per_vb_ceil()) + .max(FeeRate::BROADCAST_MIN.to_sat_per_vb_ceil()), + ) + .context("Failed to compute effective fee rate") +} + /// Estimate the absolute fee for a transaction. /// /// This function takes the following parameters: @@ -2491,14 +2671,7 @@ pub fn estimate_fee( // 2. The minimum relay fee rate (comes from fee estimation source, might vary depending on mempool congestion) // 3. The broadcast minimum fee rate (hardcoded in the Bitcoin library) // We round up to the next sat/vbyte - let recommended_fee_rate = FeeRate::from_sat_per_vb( - fee_rate_estimation - .to_sat_per_vb_ceil() - .max(min_relay_fee_rate.to_sat_per_vb_ceil()) - .max(FeeRate::BROADCAST_MIN.to_sat_per_vb_ceil()), - ) - .context("Failed to compute recommended fee rate")?; - + let recommended_fee_rate = compute_effective_fee_rate(fee_rate_estimation, min_relay_fee_rate)?; if recommended_fee_rate > fee_rate_estimation { tracing::warn!( "Estimated fee was below the minimum relay fee rate. Falling back to: {} sats/vbyte", From 81c4375f209c0f8820f9ab7717f656781008b7e6 Mon Sep 17 00:00:00 2001 From: Jonas <281324108+xn1xn1@users.noreply.github.com> Date: Sun, 10 May 2026 10:38:27 -0400 Subject: [PATCH 2/3] swap(tests): add child_fee_accounts_for_unconfirmed_parent --- ...ild_fee_accounts_for_unconfirmed_parent.rs | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 swap/tests/child_fee_accounts_for_unconfirmed_parent.rs diff --git a/swap/tests/child_fee_accounts_for_unconfirmed_parent.rs b/swap/tests/child_fee_accounts_for_unconfirmed_parent.rs new file mode 100644 index 000000000..f6eb55c72 --- /dev/null +++ b/swap/tests/child_fee_accounts_for_unconfirmed_parent.rs @@ -0,0 +1,112 @@ +pub mod harness; + +use anyhow::{Context, Result}; +use bitcoin::Amount; +use harness::FastCancelConfig; + +fn psbt_fee_sats(psbt: &bitcoin::psbt::Psbt) -> u64 { + let input_value: u64 = psbt + .inputs + .iter() + .map(|input| { + input + .witness_utxo + .as_ref() + .expect("missing witness_utxo") + .value + .to_sat() + }) + .sum(); + + let output_value: u64 = psbt + .unsigned_tx + .output + .iter() + .map(|output| output.value.to_sat()) + .sum(); + + input_value.saturating_sub(output_value) +} + +#[tokio::test] +async fn child_fee_accounts_for_unconfirmed_parent() -> Result<()> { + harness::setup_test(FastCancelConfig, None, None, |mut ctx| async move { + let (bob_swap, bob_handle) = ctx.bob_swap().await; + let wallet = bob_swap.bitcoin_wallet.clone(); + + bob_handle.abort(); + + let balance = wallet.balance().await?; + let parent_fee = Amount::from_sat(200); + let parent_amount = balance + .checked_sub(parent_fee) + .context("balance too small for parent transaction")?; + + let parent_dest = wallet.new_address().await?; + let parent_psbt = wallet + .send_to_address(parent_dest, parent_amount, parent_fee, None) + .await?; + + let parent_tx = wallet.sign_and_finalize(parent_psbt).await?; + let parent_txid = parent_tx.compute_txid(); + + wallet + .ensure_broadcasted(parent_tx.clone(), "low-fee-parent") + .await?; + + wallet.sync().await?; + + let child_dest = wallet.new_address().await?; + let child_amount = Amount::from_sat(100_000); + let child_psbt = wallet + .send_to_address_dynamic_fee(child_dest, child_amount, None) + .await?; + + assert!( + child_psbt + .unsigned_tx + .input + .iter() + .any(|input| input.previous_output.txid == parent_txid), + "child tx did not spend the unconfirmed parent output" + ); + + let child_fee_sat = psbt_fee_sats(&child_psbt); + let child_vbytes = child_psbt.unsigned_tx.weight().to_vbytes_floor() as u64; + let parent_vbytes = parent_tx.weight().to_vbytes_floor() as u64; + + // Minimum child fee needed for the combined package + // (parent + child) to reach ~1 sat/vB relay feerate + let min_package_relay_child_fee = parent_vbytes + .saturating_add(child_vbytes) + .saturating_sub(parent_fee.to_sat()); + + assert!( + child_fee_sat >= min_package_relay_child_fee, + "CPFP fee too low: child_fee={} sat, expected at least {} sat (parent_vbytes={}, child_vbytes={}, parent_fee={} sat)", + child_fee_sat, + min_package_relay_child_fee, + parent_vbytes, + child_vbytes, + parent_fee.to_sat(), + ); + + let package_fee_sat = parent_fee.to_sat().saturating_add(child_fee_sat); + let package_vbytes = parent_vbytes.saturating_add(child_vbytes); + + let parent_feerate = parent_fee.to_sat() as f64 / parent_vbytes as f64; + let package_feerate = package_fee_sat as f64 / package_vbytes as f64; + + assert!( + package_feerate > parent_feerate, + "CPFP did not improve package feerate (parent={} sat/vB, package={} sat/vB)", + parent_feerate, + package_feerate, + ); + + Ok(()) + }) + .await; + + Ok(()) +} From 179628704daadc4c5437a84fe7eb14852416dfb5 Mon Sep 17 00:00:00 2001 From: xn1xn1 <281324108+xn1xn1@users.noreply.github.com> Date: Tue, 12 May 2026 08:54:14 -0400 Subject: [PATCH 3/3] use bitcoin-wallet harness for tests and address review comments --- bitcoin-wallet/src/wallet.rs | 190 +++++--- bitcoin-wallet/tests/cpfp.rs | 418 ++++++++++++++++++ bitcoin-wallet/tests/integration.rs | 4 +- ...ild_fee_accounts_for_unconfirmed_parent.rs | 112 ----- 4 files changed, 541 insertions(+), 183 deletions(-) create mode 100644 bitcoin-wallet/tests/cpfp.rs delete mode 100644 swap/tests/child_fee_accounts_for_unconfirmed_parent.rs diff --git a/bitcoin-wallet/src/wallet.rs b/bitcoin-wallet/src/wallet.rs index 678b5988b..a913334b5 100644 --- a/bitcoin-wallet/src/wallet.rs +++ b/bitcoin-wallet/src/wallet.rs @@ -1655,7 +1655,7 @@ where ); let fee = child_only_fee.max(required_child_fee); - Ok(Self::clamp_cpfp_fee(fee, transfer_amount)) + Ok(clamp_cpfp_fee(fee, transfer_amount)) } /// Walks all unconfirmed ancestors of every input in `psbt` and returns the total fee already paid by those ancestors + their total weight. @@ -1664,9 +1664,16 @@ where &self, psbt: &Psbt, ) -> Result<(Amount, Weight)> { - const MAX_ANCESTOR_DEPTH: usize = 3; + // Included: + // depth 0 = parent + // depth 1 = grandparent + // + // Excluded: + // depth 2 = great-grandparent and beyond + const MAX_ANCESTOR_DEPTH: usize = 1; let mut seen = HashSet::::new(); + let mut stack: Vec<(Txid, usize)> = psbt .unsigned_tx .input @@ -1674,11 +1681,13 @@ where .map(|input| (input.previous_output.txid, 0)) .collect(); - let mut ancestor_fee = Amount::ZERO; - let mut ancestor_weight = Weight::from_wu(0); + let mut total_fee = Amount::ZERO; + let mut total_weight = Weight::ZERO; + + let wallet = self.wallet.lock().await; while let Some((txid, depth)) = stack.pop() { - if depth >= MAX_ANCESTOR_DEPTH { + if depth > MAX_ANCESTOR_DEPTH { continue; } @@ -1686,96 +1695,96 @@ where continue; } - let (parent_tx, parent_fee) = { - let wallet = self.wallet.lock().await; + // // Local wallet lookup only. This reads from the synced tx graph and doesn't hit the network. + let Some(wallet_tx) = wallet.get_tx(txid) else { + continue; + }; - // Local wallet lookup only. This reads from the synced tx graph and doesn't hit the network. - let Some(wallet_tx) = wallet.get_tx(txid) else { - continue; - }; + if matches!(wallet_tx.chain_position, ChainPosition::Confirmed { .. }) { + continue; + } - if matches!(wallet_tx.chain_position, ChainPosition::Confirmed { .. }) { - continue; - } + let tx = wallet_tx.tx_node.tx.clone(); - let parent_tx = wallet_tx.tx_node.tx.clone(); - - let parent_fee = match wallet.calculate_fee(&parent_tx) { - Ok(fee) => fee, - Err(err) => { - tracing::debug!( - %txid, - error = %err, - "Cannot compute fee for unconfirmed ancestor; skipping it" - ); - continue; - } - }; + let fee = match wallet.calculate_fee(&tx) { + Ok(fee) => fee, + Err(err) => { + tracing::debug!( + %txid, + error = %err, + "Cannot compute fee for unconfirmed ancestor; skipping it." + ); - (parent_tx, parent_fee) + continue; + } }; - ancestor_fee = Amount::from_sat( - ancestor_fee.to_sat().saturating_add(parent_fee.to_sat()), + total_fee = Amount::from_sat( + total_fee.to_sat().saturating_add(fee.to_sat()), ); - ancestor_weight = Weight::from_wu( - ancestor_weight.to_wu().saturating_add(parent_tx.weight().to_wu()), + + total_weight = Weight::from_wu( + total_weight + .to_wu() + .saturating_add(tx.weight().to_wu()), ); stack.extend( - parent_tx - .input + tx.input .iter() .map(|input| (input.previous_output.txid, depth + 1)), ); } - Ok((ancestor_fee, ancestor_weight)) + Ok((total_fee, total_weight)) } +} - /// Clamps a CPFP fee to the allowed range (relative cap -> min absolute -> hard cap) - fn clamp_cpfp_fee(fee: Amount, transfer_amount: Option) -> Amount { - let mut final_fee = fee; +/// Clamps a CPFP fee to the allowed range (relative cap -> min absolute -> hard cap) +fn clamp_cpfp_fee(fee: Amount, transfer_amount: Option) -> Amount { + let mut final_fee = fee; - if let Some(transfer_amount) = transfer_amount { - let relative_max = Amount::from_sat( - MAX_RELATIVE_TX_FEE - .saturating_mul(Decimal::from(transfer_amount.to_sat())) - .ceil() - .to_u64() - .expect("MAX_RELATIVE_TX_FEE * transfer_amount fits in u64"), - ); - if final_fee > relative_max { - tracing::warn!( - "CPFP fee {} exceeds relative cap {} ({}% of transfer amount). Clamping.", - final_fee.to_sat(), - relative_max.to_sat(), - MAX_RELATIVE_TX_FEE.saturating_mul(Decimal::from(100)).ceil().to_u64().unwrap(), - ); - final_fee = relative_max; - } - } + if let Some(transfer_amount) = transfer_amount { + let relative_max = Amount::from_sat( + MAX_RELATIVE_TX_FEE + .saturating_mul(Decimal::from(transfer_amount.to_sat())) + .ceil() + .to_u64() + .expect("MAX_RELATIVE_TX_FEE * transfer_amount fits in u64"), + ); - if final_fee < MIN_ABSOLUTE_TX_FEE { + if final_fee > relative_max { tracing::warn!( - "CPFP fee {} below absolute minimum {}. Setting to minimum.", + "CPFP fee {} exceeds relative cap {}. Clamping.", final_fee.to_sat(), - MIN_ABSOLUTE_TX_FEE.to_sat(), + relative_max.to_sat(), ); - final_fee = MIN_ABSOLUTE_TX_FEE; - } - if final_fee > MAX_ABSOLUTE_TX_FEE { - tracing::warn!( - "CPFP fee {} exceeds absolute hard cap {}. Capping.", - final_fee.to_sat(), - MAX_ABSOLUTE_TX_FEE.to_sat(), - ); - final_fee = MAX_ABSOLUTE_TX_FEE; + final_fee = relative_max; } + } + + if final_fee < MIN_ABSOLUTE_TX_FEE { + tracing::warn!( + "CPFP fee {} below minimum {}. Raising.", + final_fee.to_sat(), + MIN_ABSOLUTE_TX_FEE.to_sat(), + ); + + final_fee = MIN_ABSOLUTE_TX_FEE; + } + + if final_fee > MAX_ABSOLUTE_TX_FEE { + tracing::warn!( + "CPFP fee {} exceeds absolute hard cap {}. Capping.", + final_fee.to_sat(), + MAX_ABSOLUTE_TX_FEE.to_sat(), + ); - final_fee + final_fee = MAX_ABSOLUTE_TX_FEE; } + + final_fee } impl Client { @@ -3188,3 +3197,46 @@ impl BitcoinWallet for Wallet { unimplemented!("stub method called erroneously") } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn cpfp_fee_clamps_to_absolute_max() { + let fee = Amount::from_sat(MAX_ABSOLUTE_TX_FEE.to_sat() * 100); + + let actual = clamp_cpfp_fee(fee, None); + + assert_eq!(actual, MAX_ABSOLUTE_TX_FEE); + } + + #[test] + fn cpfp_fee_clamps_to_absolute_minimum() { + let actual = clamp_cpfp_fee(Amount::from_sat(1), None); + + assert_eq!(actual, MIN_ABSOLUTE_TX_FEE); + } + + #[test] + fn cpfp_fee_enforces_minimum_even_when_fee_is_zero() { + let actual = clamp_cpfp_fee( + Amount::from_sat(0), + Some(Amount::from_sat(1_000)), + ); + + assert_eq!(actual, MIN_ABSOLUTE_TX_FEE); + } + + #[test] + fn cpfp_fee_is_capped() { + let absurd_fee = Amount::from_sat(u64::MAX / 2); + + let actual = clamp_cpfp_fee( + absurd_fee, + Some(Amount::from_sat(1_000_000)), + ); + + assert!(actual <= MAX_ABSOLUTE_TX_FEE); + } +} diff --git a/bitcoin-wallet/tests/cpfp.rs b/bitcoin-wallet/tests/cpfp.rs new file mode 100644 index 000000000..34abddf5e --- /dev/null +++ b/bitcoin-wallet/tests/cpfp.rs @@ -0,0 +1,418 @@ +mod harness; + +use anyhow::{Context, Result}; +use bitcoin::Amount; +use bitcoin_wallet::{PersisterConfig, WalletBuilder}; +use std::time::Duration; +use testcontainers::clients::Cli; + +fn feerate(fee_sat: u64, vbytes: u64) -> f64 { + fee_sat as f64 / vbytes as f64 +} + +fn tx_vbytes(tx: &bitcoin::Transaction) -> u64 { + tx.weight().to_vbytes_floor() as u64 +} + +fn psbt_vbytes(psbt: &bitcoin::psbt::Psbt) -> u64 { + psbt.unsigned_tx.weight().to_vbytes_floor() as u64 +} + +#[derive(Clone)] +struct BroadcastTx { + tx: bitcoin::Transaction, + fee_sat: u64, + vbytes: u64, +} + +impl BroadcastTx { + fn txid(&self) -> bitcoin::Txid { + self.tx.compute_txid() + } +} + +#[derive(Clone, Debug)] +struct TestSeed([u8; 64]); + +impl TestSeed { + fn new(byte: u8) -> Self { + Self([byte; 64]) + } +} + +impl Default for TestSeed { + fn default() -> Self { + Self::new(42) + } +} + +impl bitcoin_wallet::BitcoinWalletSeed for TestSeed { + fn derive_extended_private_key( + &self, + network: bitcoin::Network, + ) -> anyhow::Result { + #[allow(deprecated)] + { + Ok(bitcoin::bip32::Xpriv::new_master(network, &self.0)?) + } + } + + fn derive_extended_private_key_legacy( + &self, + network: bdk::bitcoin::Network, + ) -> anyhow::Result { + Ok(bdk::bitcoin::util::bip32::ExtendedPrivKey::new_master( + network, + &self.0, + )?) + } +} + +fn init_tracing() { + let _ = tracing_subscriber::fmt() + .with_env_filter("info,bitcoin_wallet=debug,electrum_pool=debug,testcontainers=info") + .with_test_writer() + .try_init(); +} + +async fn make_wallet( + env: &harness::TestEnv<'_>, + seed: TestSeed, +) -> Result { + let wallet = WalletBuilder::::default() + .seed(seed) + .network(bitcoin::Network::Regtest) + .electrum_rpc_urls(vec![env.electrum_url.clone()]) + .persister(PersisterConfig::InMemorySqlite) + .finality_confirmations(1u32) + .target_block(1u32) + .sync_interval(Duration::from_millis(0)) + .use_mempool_space_fee_estimation(false) + .build() + .await?; + + wallet.sync().await?; + Ok(wallet) +} + +async fn sync_until_balance( + wallet: &bitcoin_wallet::Wallet, + expected_at_least: Amount, +) -> Result<()> { + let deadline = tokio::time::Instant::now() + Duration::from_secs(30); + + loop { + wallet.sync().await?; + + if wallet.balance().await? >= expected_at_least { + return Ok(()); + } + + if tokio::time::Instant::now() >= deadline { + anyhow::bail!( + "timed out waiting for wallet balance to reach {} sats", + expected_at_least.to_sat() + ); + } + + tokio::time::sleep(Duration::from_millis(500)).await; + } +} + +async fn wait_until_tx_seen( + wallet: &bitcoin_wallet::Wallet, + txid: bitcoin::Txid, +) -> Result<()> { + let deadline = tokio::time::Instant::now() + Duration::from_secs(30); + + loop { + wallet.sync().await?; + + if wallet.get_raw_transaction(txid).await?.is_some() { + return Ok(()); + } + + if tokio::time::Instant::now() >= deadline { + anyhow::bail!("timed out waiting for tx {txid} to be indexed"); + } + + tokio::time::sleep(Duration::from_millis(500)).await; + } +} + +async fn funded_wallet( + env: &harness::TestEnv<'_>, + seed: TestSeed, + amount: Amount, +) -> Result { + let wallet = make_wallet(env, seed).await?; + + let receive_addr = wallet.new_address().await?; + + harness::fund_and_mine(&env.bitcoind, receive_addr, amount).await?; + + sync_until_balance(&wallet, amount).await?; + + Ok(wallet) +} + +async fn build_and_broadcast_tx( + wallet: &bitcoin_wallet::Wallet, + amount: Amount, + fee: Amount, + label: &str, +) -> Result { + let dest = wallet.new_address().await?; + + let psbt = wallet.send_to_address(dest, amount, fee, None).await?; + + let actual_fee = psbt + .fee() + .expect("PSBT fee must be computable"); + + assert_eq!( + actual_fee, + fee, + "constructed tx fee differs from expected fee", + ); + + let tx = wallet.sign_and_finalize(psbt).await?; + + let (txid, _sub) = wallet.broadcast(tx.clone(), label).await?; + + wait_until_tx_seen(wallet, txid).await?; + + Ok(BroadcastTx { + vbytes: tx_vbytes(&tx), + fee_sat: fee.to_sat(), + tx, + }) +} + +async fn build_low_fee_chain( + wallet: &bitcoin_wallet::Wallet, + chain_length: usize, + fee: Amount, +) -> Result> { + assert!(chain_length > 0); + + let mut txs = Vec::with_capacity(chain_length); + + for i in 0..chain_length { + let balance = wallet.balance().await?; + + let amount = balance + .checked_sub(fee) + .context("balance too small for low-fee tx")?; + + let tx = build_and_broadcast_tx( + wallet, + amount, + fee, + &format!("low-fee-chain-{i}"), + ) + .await?; + + txs.push(tx); + } + + Ok(txs) +} + +fn assert_cpfp_requirements( + ancestor_fee_sat: u64, + ancestor_vbytes: u64, + child_fee_sat: u64, + child_vbytes: u64, +) { + let ancestor_feerate = feerate(ancestor_fee_sat, ancestor_vbytes); + let child_feerate = feerate(child_fee_sat, child_vbytes); + + assert!( + child_feerate > ancestor_feerate, + "child feerate ({}) must exceed ancestor feerate ({})", + child_feerate, + ancestor_feerate, + ); + + let package_fee_sat = ancestor_fee_sat.saturating_add(child_fee_sat); + let package_vbytes = ancestor_vbytes.saturating_add(child_vbytes); + let package_feerate = feerate(package_fee_sat, package_vbytes); + + assert!( + package_feerate > ancestor_feerate, + "package feerate ({}) must exceed ancestor feerate ({})", + package_feerate, + ancestor_feerate, + ); + + assert!( + package_feerate >= 1.0, + "package feerate ({}) must satisfy minimum relay feerate", + package_feerate, + ); +} + +#[tokio::test] +async fn cpfp_accounts_for_direct_parent() -> Result<()> { + init_tracing(); + + let cli = Cli::default(); + let env = harness::setup(&cli).await?; + + let wallet = funded_wallet(&env, TestSeed::new(1), Amount::from_sat(1_000_000)).await?; + + let parent_fee = Amount::from_sat(200); + let chain = build_low_fee_chain(&wallet, 1, parent_fee).await?; + let parent = &chain[0]; + + let child_dest = wallet.new_address().await?; + let child_psbt = wallet + .send_to_address_dynamic_fee(child_dest, Amount::from_sat(100_000), None) + .await?; + + assert!( + child_psbt + .unsigned_tx + .input + .iter() + .any(|input| input.previous_output.txid == parent.txid()), + "child tx did not spend parent output", + ); + + let child_fee_sat = child_psbt.fee()?.to_sat(); + let child_vbytes = psbt_vbytes(&child_psbt); + + assert_cpfp_requirements( + parent.fee_sat, + parent.vbytes, + child_fee_sat, + child_vbytes, + ); + + Ok(()) +} + +#[tokio::test] +async fn cpfp_accounts_for_parent_and_grandparent() -> Result<()> { + init_tracing(); + + let cli = Cli::default(); + let env = harness::setup(&cli).await?; + + let wallet = funded_wallet(&env, TestSeed::new(2), Amount::from_sat(1_000_000)).await?; + + let fee = Amount::from_sat(200); + let chain = build_low_fee_chain(&wallet, 2, fee).await?; + + let grandparent = &chain[0]; + let parent = &chain[1]; + + assert!( + parent + .tx + .input + .iter() + .any(|input| input.previous_output.txid == grandparent.txid()), + "parent tx did not spend grandparent output", + ); + + let child_dest = wallet.new_address().await?; + let child_psbt = wallet + .send_to_address_dynamic_fee(child_dest, Amount::from_sat(100_000), None) + .await?; + + assert!( + child_psbt + .unsigned_tx + .input + .iter() + .any(|input| input.previous_output.txid == parent.txid()), + "child tx did not spend parent output", + ); + + let child_fee_sat = child_psbt.fee()?.to_sat(); + let child_vbytes = psbt_vbytes(&child_psbt); + + let included_fee_sat = grandparent.fee_sat + parent.fee_sat; + let included_vbytes = grandparent.vbytes + parent.vbytes; + + assert_cpfp_requirements( + included_fee_sat, + included_vbytes, + child_fee_sat, + child_vbytes, + ); + + Ok(()) +} + +#[tokio::test] +async fn cpfp_ignores_great_grandparent() -> Result<()> { + init_tracing(); + + let cli = Cli::default(); + let env = harness::setup(&cli).await?; + let low_fee = Amount::from_sat(200); + + let wallet_a = funded_wallet(&env, TestSeed::new(10), Amount::from_sat(1_000_000)).await?; + let chain_a = build_low_fee_chain(&wallet_a, 2, low_fee).await?; + let grandparent_a = &chain_a[0]; + let parent_a = &chain_a[1]; + + let child_a_psbt = wallet_a + .send_to_address_dynamic_fee( + wallet_a.new_address().await?, + Amount::from_sat(100_000), + None, + ) + .await?; + let child_a_fee_sat = child_a_psbt.fee().expect("PSBT fee must be computable").to_sat(); + let child_a_vbytes = psbt_vbytes(&child_a_psbt); + + let wallet_b = funded_wallet(&env, TestSeed::new(11), Amount::from_sat(1_000_000)).await?; + let chain_b = build_low_fee_chain(&wallet_b, 3, low_fee).await?; + + let grandparent_b = &chain_b[1]; + let parent_b = &chain_b[2]; + + let child_b_psbt = wallet_b + .send_to_address_dynamic_fee( + wallet_b.new_address().await?, + Amount::from_sat(100_000), + None, + ) + .await?; + let child_b_fee_sat = child_b_psbt.fee().expect("PSBT fee must be computable").to_sat(); + let child_b_vbytes = psbt_vbytes(&child_b_psbt); + + assert!( + child_b_psbt + .unsigned_tx + .input + .iter() + .any(|input| input.previous_output.txid == parent_b.txid()), + "child tx did not spend parent output" + ); + + assert_eq!( + child_a_fee_sat, + child_b_fee_sat, + "great-grandparent must be ignored: child fee with depth 3 must equal child fee with depth 2", + ); + + assert_cpfp_requirements( + grandparent_a.fee_sat + parent_a.fee_sat, + grandparent_a.vbytes + parent_a.vbytes, + child_a_fee_sat, + child_a_vbytes, + ); + assert_cpfp_requirements( + grandparent_b.fee_sat + parent_b.fee_sat, + grandparent_b.vbytes + parent_b.vbytes, + child_b_fee_sat, + child_b_vbytes, + ); + + Ok(()) +} diff --git a/bitcoin-wallet/tests/integration.rs b/bitcoin-wallet/tests/integration.rs index bd6846726..c6ab7ab46 100644 --- a/bitcoin-wallet/tests/integration.rs +++ b/bitcoin-wallet/tests/integration.rs @@ -42,10 +42,10 @@ impl bitcoin_wallet::BitcoinWalletSeed for TestSeed { fn derive_extended_private_key( &self, network: bitcoin::Network, - ) -> anyhow::Result { + ) -> anyhow::Result { #[allow(deprecated)] { - Ok(bitcoin::bip32::ExtendedPrivKey::new_master(network, &self.0)?) + Ok(bitcoin::bip32::Xpriv::new_master(network, &self.0)?) } } diff --git a/swap/tests/child_fee_accounts_for_unconfirmed_parent.rs b/swap/tests/child_fee_accounts_for_unconfirmed_parent.rs deleted file mode 100644 index f6eb55c72..000000000 --- a/swap/tests/child_fee_accounts_for_unconfirmed_parent.rs +++ /dev/null @@ -1,112 +0,0 @@ -pub mod harness; - -use anyhow::{Context, Result}; -use bitcoin::Amount; -use harness::FastCancelConfig; - -fn psbt_fee_sats(psbt: &bitcoin::psbt::Psbt) -> u64 { - let input_value: u64 = psbt - .inputs - .iter() - .map(|input| { - input - .witness_utxo - .as_ref() - .expect("missing witness_utxo") - .value - .to_sat() - }) - .sum(); - - let output_value: u64 = psbt - .unsigned_tx - .output - .iter() - .map(|output| output.value.to_sat()) - .sum(); - - input_value.saturating_sub(output_value) -} - -#[tokio::test] -async fn child_fee_accounts_for_unconfirmed_parent() -> Result<()> { - harness::setup_test(FastCancelConfig, None, None, |mut ctx| async move { - let (bob_swap, bob_handle) = ctx.bob_swap().await; - let wallet = bob_swap.bitcoin_wallet.clone(); - - bob_handle.abort(); - - let balance = wallet.balance().await?; - let parent_fee = Amount::from_sat(200); - let parent_amount = balance - .checked_sub(parent_fee) - .context("balance too small for parent transaction")?; - - let parent_dest = wallet.new_address().await?; - let parent_psbt = wallet - .send_to_address(parent_dest, parent_amount, parent_fee, None) - .await?; - - let parent_tx = wallet.sign_and_finalize(parent_psbt).await?; - let parent_txid = parent_tx.compute_txid(); - - wallet - .ensure_broadcasted(parent_tx.clone(), "low-fee-parent") - .await?; - - wallet.sync().await?; - - let child_dest = wallet.new_address().await?; - let child_amount = Amount::from_sat(100_000); - let child_psbt = wallet - .send_to_address_dynamic_fee(child_dest, child_amount, None) - .await?; - - assert!( - child_psbt - .unsigned_tx - .input - .iter() - .any(|input| input.previous_output.txid == parent_txid), - "child tx did not spend the unconfirmed parent output" - ); - - let child_fee_sat = psbt_fee_sats(&child_psbt); - let child_vbytes = child_psbt.unsigned_tx.weight().to_vbytes_floor() as u64; - let parent_vbytes = parent_tx.weight().to_vbytes_floor() as u64; - - // Minimum child fee needed for the combined package - // (parent + child) to reach ~1 sat/vB relay feerate - let min_package_relay_child_fee = parent_vbytes - .saturating_add(child_vbytes) - .saturating_sub(parent_fee.to_sat()); - - assert!( - child_fee_sat >= min_package_relay_child_fee, - "CPFP fee too low: child_fee={} sat, expected at least {} sat (parent_vbytes={}, child_vbytes={}, parent_fee={} sat)", - child_fee_sat, - min_package_relay_child_fee, - parent_vbytes, - child_vbytes, - parent_fee.to_sat(), - ); - - let package_fee_sat = parent_fee.to_sat().saturating_add(child_fee_sat); - let package_vbytes = parent_vbytes.saturating_add(child_vbytes); - - let parent_feerate = parent_fee.to_sat() as f64 / parent_vbytes as f64; - let package_feerate = package_fee_sat as f64 / package_vbytes as f64; - - assert!( - package_feerate > parent_feerate, - "CPFP did not improve package feerate (parent={} sat/vB, package={} sat/vB)", - parent_feerate, - package_feerate, - ); - - Ok(()) - }) - .await; - - Ok(()) -}