diff --git a/crates/bdk-wallet-aggregate/src/lib.rs b/crates/bdk-wallet-aggregate/src/lib.rs index 6215359f1..c41bb9b37 100644 --- a/crates/bdk-wallet-aggregate/src/lib.rs +++ b/crates/bdk-wallet-aggregate/src/lib.rs @@ -989,7 +989,7 @@ where let drain_script = self.primary_wallet().peek_address(KeychainKind::Internal, 0).address.script_pubkey(); - let selected = utxo::select_utxos_with_algorithm( + let selected = utxo::select_utxos_for_deficit( deficit.to_sat(), available, new_fee_rate, @@ -1025,15 +1025,17 @@ where &self, target_amount: u64, available_utxos: Vec, fee_rate: FeeRate, algorithm: CoinSelectionAlgorithm, drain_script: &Script, excluded_outpoints: &[OutPoint], ) -> Result, Error> { - utxo::select_utxos_with_algorithm( + utxo::select_utxos_with_algorithm(utxo::SelectionRequest { target_amount, available_utxos, fee_rate, algorithm, drain_script, excluded_outpoints, - &self.wallets, - ) + wallets: &self.wallets, + recipient_script: None, + include_payment_overhead: true, + }) } // ─── Fee Calculation ───────────────────────────────────────────────── @@ -1128,7 +1130,7 @@ where let drain_script = self.primary_wallet().peek_address(KeychainKind::Internal, 0).address.script_pubkey(); - let selected_outpoints = utxo::select_utxos_with_algorithm( + let selected_outpoints = utxo::select_utxos_for_deficit( deficit.to_sat(), non_primary, fee_rate, @@ -1172,15 +1174,17 @@ where .address .script_pubkey(); - let selected = utxo::select_utxos_with_algorithm( - amount.to_sat(), - all_utxos, + let selected = utxo::select_utxos_with_algorithm(utxo::SelectionRequest { + target_amount: amount.to_sat(), + available_utxos: all_utxos, fee_rate, algorithm, - &drain_script, - &[], - &self.wallets, - )?; + drain_script: &drain_script, + excluded_outpoints: &[], + wallets: &self.wallets, + recipient_script: Some(&output_script), + include_payment_overhead: true, + })?; let infos = self.prepare_outpoints_for_psbt(&selected)?; if infos.is_empty() { @@ -1652,6 +1656,7 @@ mod tests { use bitcoin::bip32::Xpriv; use bitcoin::{ Amount, Block, FeeRate, Network, OutPoint, ScriptBuf, Transaction, TxIn, TxOut, Txid, + Weight, }; use super::*; @@ -1839,6 +1844,23 @@ mod tests { ScriptBuf::new_p2wpkh(&bitcoin::WPubkeyHash::from_byte_array([0xab; 20])) } + fn p2wsh_recipient_script() -> ScriptBuf { + ScriptBuf::new_p2wsh(&bitcoin::WScriptHash::from_byte_array([0xcd; 32])) + } + + fn p2tr_recipient_script() -> ScriptBuf { + let secp = bitcoin::secp256k1::Secp256k1::new(); + let secret = bitcoin::secp256k1::SecretKey::from_slice(&[0x01; 32]).unwrap(); + let keypair = bitcoin::secp256k1::Keypair::from_secret_key(&secp, &secret); + let (xonly, _) = keypair.x_only_public_key(); + ScriptBuf::new_p2tr(&secp, xonly, None) + } + + fn witness_v2_40_byte_recipient_script() -> ScriptBuf { + let program = bitcoin::WitnessProgram::new(bitcoin::WitnessVersion::V2, &[0; 40]).unwrap(); + ScriptBuf::new_witness_program(&program) + } + #[test] fn persistence_fails_when_a_wallet_has_no_persister() { let mut persister = NoopPersister; @@ -1939,6 +1961,304 @@ mod tests { assert!(aggregate.calculate_fee_from_psbt(&psbt).unwrap() > 0); } + /// A selection returned by `select_utxos` must be accepted by the transaction + /// builder for the same target amount and fee rate, for every algorithm. + /// Regression test for https://github.com/synonymdev/ldk-node/issues/104. + #[test] + fn selected_utxos_satisfy_tx_builder_for_all_algorithms() { + // The wallet layout from the issue report: 18 P2WPKH UTXOs, 88,900 sats total. + let utxo_amounts = [ + 2_000u64, 3_500, 4_200, 5_000, 6_100, 7_800, 2_500, 3_000, 8_900, 4_500, 2_100, 5_500, + 3_200, 6_700, 9_100, 4_800, 2_700, 7_300, + ]; + let target_amount = 35_000u64; + let fee_rate = FeeRate::from_sat_per_vb(1).unwrap(); + + let mut persister = NoopPersister; + let mut wallet = create_empty_wallet(&mut persister); + for (index, amount) in utxo_amounts.iter().enumerate() { + fund_wallet(&mut wallet, Amount::from_sat(*amount), index as u8 + 1); + } + let mut aggregate = AggregateWallet::::new(wallet, persister, 0, vec![]); + + let drain_script = aggregate + .primary_wallet() + .peek_address(KeychainKind::Internal, 0) + .address + .script_pubkey(); + + // SingleRandomDraw is intentionally omitted: its randomized selection can + // occasionally land on a dust-change result that selection legitimately + // rejects, making the test flaky. The invariant under test — a returned + // selection must be acceptable to the builder — is exercised + // deterministically by the three deterministic algorithms below, including + // the changeless BranchAndBound path from issue #104. + for algorithm in [ + CoinSelectionAlgorithm::BranchAndBound, + CoinSelectionAlgorithm::LargestFirst, + CoinSelectionAlgorithm::OldestFirst, + ] { + let available_utxos = aggregate.list_unspent(); + let selected = aggregate + .select_utxos( + target_amount, + available_utxos, + fee_rate, + algorithm, + &drain_script, + &[], + ) + .unwrap_or_else(|e| panic!("{algorithm:?} selection failed: {e:?}")); + + // Build the spend exactly like the manual-UTXO path of ldk-node's + // `build_transaction_psbt` does. + let infos = aggregate.prepare_outpoints_for_psbt(&selected).unwrap(); + let mut builder = aggregate.primary_wallet_mut().build_tx(); + builder + .add_recipient(recipient_script(), Amount::from_sat(target_amount)) + .fee_rate(fee_rate); + utxo::add_utxos_to_tx_builder(&mut builder, &infos).unwrap(); + builder.manually_selected_only(); + builder.finish().unwrap_or_else(|e| panic!("{algorithm:?} selection rejected: {e:?}")); + } + } + + /// The exact low-fee case from the PR #109 review: at 250 sat/kwu the + /// changeless branch-and-bound match for a 35,000-sat target totals ~35,600 + /// sats — below the 36,000 sats the removed 1,000-sat minimum fee-buffer + /// precheck required — and must be accepted by the transaction builder. + #[test] + fn changeless_low_fee_selection_is_accepted_by_the_builder() { + let utxo_amounts = [ + 2_000u64, 3_500, 4_200, 5_000, 6_100, 7_800, 2_500, 3_000, 8_900, 4_500, 2_100, 5_500, + 3_200, 6_700, 9_100, 4_800, 2_700, 7_300, + ]; + let target_amount = 35_000u64; + let fee_rate = FeeRate::from_sat_per_kwu(250); + + let mut persister = NoopPersister; + let mut wallet = create_empty_wallet(&mut persister); + for (index, amount) in utxo_amounts.iter().enumerate() { + fund_wallet(&mut wallet, Amount::from_sat(*amount), index as u8 + 1); + } + let mut aggregate = AggregateWallet::::new(wallet, persister, 0, vec![]); + let drain_script = aggregate + .primary_wallet() + .peek_address(KeychainKind::Internal, 0) + .address + .script_pubkey(); + + let selected = aggregate + .select_utxos( + target_amount, + aggregate.list_unspent(), + fee_rate, + CoinSelectionAlgorithm::BranchAndBound, + &drain_script, + &[], + ) + .unwrap(); + + // The tight changeless match quoted in the review: eight P2WPKH inputs + // totaling ~35,600 sats, which the old precheck would have rejected. + let selected_total: u64 = aggregate + .list_unspent() + .iter() + .filter(|u| selected.contains(&u.outpoint)) + .map(|u| u.txout.value.to_sat()) + .sum(); + assert_eq!(selected.len(), 8); + assert!(selected_total < 36_000, "selection total: {selected_total}"); + + let infos = aggregate.prepare_outpoints_for_psbt(&selected).unwrap(); + let mut builder = aggregate.primary_wallet_mut().build_tx(); + builder + .add_recipient(recipient_script(), Amount::from_sat(target_amount)) + .fee_rate(fee_rate); + utxo::add_utxos_to_tx_builder(&mut builder, &infos).unwrap(); + builder.manually_selected_only(); + builder.finish().expect("builder must accept the changeless low-fee selection"); + } + + #[test] + fn p2wpkh_change_below_generic_dust_limit_is_accepted() { + let mut persister = NoopPersister; + let mut wallet = create_empty_wallet(&mut persister); + fund_wallet(&mut wallet, Amount::from_sat(35_655), 0x45); + let mut aggregate = AggregateWallet::::new(wallet, persister, 0, vec![]); + let recipient = recipient_script(); + let drain_script = aggregate + .primary_wallet() + .peek_address(KeychainKind::Internal, 0) + .address + .script_pubkey(); + let fee_rate = FeeRate::from_sat_per_kwu(250); + let selected = aggregate + .select_utxos( + 35_000, + aggregate.list_unspent(), + fee_rate, + CoinSelectionAlgorithm::BranchAndBound, + &drain_script, + &[], + ) + .unwrap(); + + let infos = aggregate.prepare_outpoints_for_psbt(&selected).unwrap(); + let mut builder = aggregate.primary_wallet_mut().build_tx(); + builder.add_recipient(recipient, Amount::from_sat(35_000)).fee_rate(fee_rate); + utxo::add_utxos_to_tx_builder(&mut builder, &infos).unwrap(); + builder.manually_selected_only(); + let psbt = builder.finish().expect("BDK should accept valid P2WPKH change"); + let change = psbt + .unsigned_tx + .output + .iter() + .find(|output| output.script_pubkey == drain_script) + .expect("selection should retain change"); + assert!((294..546).contains(&change.value.to_sat())); + } + + /// A genuinely insufficient manual selection must still be rejected by the + /// transaction builder with an insufficient-funds error (which ldk-node + /// maps to `Error::InsufficientFunds`). + #[test] + fn builder_rejects_insufficient_manual_selection() { + let mut persister = NoopPersister; + let mut wallet = create_empty_wallet(&mut persister); + // Covers the 35,000-sat target but not the fee for spending it. + fund_wallet(&mut wallet, Amount::from_sat(35_050), 0x41); + let mut aggregate = AggregateWallet::::new(wallet, persister, 0, vec![]); + let outpoint = aggregate.list_unspent()[0].outpoint; + + let infos = aggregate.prepare_outpoints_for_psbt(&[outpoint]).unwrap(); + let mut builder = aggregate.primary_wallet_mut().build_tx(); + builder + .add_recipient(recipient_script(), Amount::from_sat(35_000)) + .fee_rate(FeeRate::from_sat_per_kwu(250)); + utxo::add_utxos_to_tx_builder(&mut builder, &infos).unwrap(); + builder.manually_selected_only(); + + assert!(matches!(builder.finish(), Err(CreateTxError::CoinSelection(_)))); + } + + /// The conservative base-overhead allowance (43 vB recipient output) must + /// keep selector/builder agreement for the largest standard recipient + /// scripts, P2TR and P2WSH, not just P2WPKH. + /// + /// Boundary fixture: at 1,000 sat/kwu (1 sat/vB), 10,470 sats covers the + /// 10,000-sat target + 1 input fee (271 sats) + 188 WU base fee (188 sats), + /// so a 188-WU allowance selects only the 10,470-sat UTXO. However, spending + /// 1 P2WPKH input to a 43 vB output needs 485 sats fee (10,485 total), + /// making 10,470 sats insufficient for TxBuilder. The 224-WU allowance + /// targets 10,495 sats, correctly forcing selection of the second (500 sat) + /// UTXO, which TxBuilder accepts (756 sat fee, 10,756 total needed <= 10,970). + #[test] + fn selection_is_accepted_for_p2tr_and_p2wsh_recipients() { + let utxo_amounts = [10_470u64, 500]; + let target_amount = 10_000u64; + let fee_rate = FeeRate::from_sat_per_kwu(1_000); + + for recipient in [p2tr_recipient_script(), p2wsh_recipient_script()] { + let mut persister = NoopPersister; + let mut wallet = create_empty_wallet(&mut persister); + for (index, amount) in utxo_amounts.iter().enumerate() { + fund_wallet(&mut wallet, Amount::from_sat(*amount), index as u8 + 1); + } + let mut aggregate = AggregateWallet::::new(wallet, persister, 0, vec![]); + let drain_script = aggregate + .primary_wallet() + .peek_address(KeychainKind::Internal, 0) + .address + .script_pubkey(); + + let selected = aggregate + .select_utxos( + target_amount, + aggregate.list_unspent(), + fee_rate, + CoinSelectionAlgorithm::BranchAndBound, + &drain_script, + &[], + ) + .unwrap(); + + let infos = aggregate.prepare_outpoints_for_psbt(&selected).unwrap(); + let mut builder = aggregate.primary_wallet_mut().build_tx(); + builder.add_recipient(recipient, Amount::from_sat(target_amount)).fee_rate(fee_rate); + utxo::add_utxos_to_tx_builder(&mut builder, &infos).unwrap(); + builder.manually_selected_only(); + builder.finish().expect("builder must accept the selection for a 43 vB recipient"); + } + } + + #[test] + fn selection_is_accepted_for_witness_v2_40_byte_recipient() { + let recipient = witness_v2_40_byte_recipient_script(); + let mut persister = NoopPersister; + let mut wallet = create_empty_wallet(&mut persister); + fund_wallet(&mut wallet, Amount::from_sat(10_510), 0x42); + fund_wallet(&mut wallet, Amount::from_sat(700), 0x43); + let mut aggregate = AggregateWallet::::new(wallet, persister, 0, vec![]); + let drain_script = aggregate + .primary_wallet() + .peek_address(KeychainKind::Internal, 0) + .address + .script_pubkey(); + let target_amount = 10_000; + let fee_rate = FeeRate::from_sat_per_kwu(1_000); + + assert_eq!( + TxOut { value: Amount::ZERO, script_pubkey: recipient.clone() }.weight(), + Weight::from_wu(204) + ); + let selected = utxo::select_utxos_with_algorithm(utxo::SelectionRequest { + target_amount, + available_utxos: aggregate.list_unspent(), + fee_rate, + algorithm: CoinSelectionAlgorithm::LargestFirst, + drain_script: &drain_script, + excluded_outpoints: &[], + wallets: &aggregate.wallets, + recipient_script: Some(&recipient), + include_payment_overhead: true, + }) + .unwrap(); + assert_eq!(selected.len(), 2); + + let infos = aggregate.prepare_outpoints_for_psbt(&selected).unwrap(); + let mut builder = aggregate.primary_wallet_mut().build_tx(); + builder.add_recipient(recipient, Amount::from_sat(target_amount)).fee_rate(fee_rate); + utxo::add_utxos_to_tx_builder(&mut builder, &infos).unwrap(); + builder.manually_selected_only(); + builder.finish().expect("builder must accept the selection for a 40-byte witness program"); + } + + #[test] + fn max_payment_target_returns_insufficient_funds() { + let mut persister = NoopPersister; + let mut wallet = create_empty_wallet(&mut persister); + fund_wallet(&mut wallet, Amount::from_sat(1_000), 0x44); + let aggregate = AggregateWallet::::new(wallet, persister, 0, vec![]); + let drain_script = aggregate + .primary_wallet() + .peek_address(KeychainKind::Internal, 0) + .address + .script_pubkey(); + + assert_eq!( + aggregate.select_utxos( + u64::MAX, + aggregate.list_unspent(), + FeeRate::from_sat_per_kwu(250), + CoinSelectionAlgorithm::BranchAndBound, + &drain_script, + &[], + ), + Err(Error::InsufficientFunds) + ); + } + #[test] fn only_coin_selection_errors_trigger_the_rbf_fallback() { let insufficient = bdk_wallet::coin_selection::InsufficientFunds { diff --git a/crates/bdk-wallet-aggregate/src/utxo.rs b/crates/bdk-wallet-aggregate/src/utxo.rs index bcc0f4de0..7cbd10f57 100644 --- a/crates/bdk-wallet-aggregate/src/utxo.rs +++ b/crates/bdk-wallet-aggregate/src/utxo.rs @@ -14,18 +14,41 @@ use std::hash::Hash; #[allow(deprecated)] use bdk_wallet::coin_selection::CoinSelectionAlgorithm as BdkCoinSelectionAlgorithm; use bdk_wallet::coin_selection::{ - BranchAndBoundCoinSelection, Excess, LargestFirstCoinSelection, OldestFirstCoinSelection, + BranchAndBoundCoinSelection, LargestFirstCoinSelection, OldestFirstCoinSelection, SingleRandomDraw, }; use bdk_wallet::{LocalOutput, PersistedWallet, WalletPersister, WeightedUtxo}; use bip39::rand::rngs::OsRng; -use bitcoin::{psbt, Amount, FeeRate, OutPoint, Script, ScriptBuf, Weight}; +use bitcoin::{psbt, Amount, FeeRate, OutPoint, Script, ScriptBuf, TxOut, Weight}; use crate::types::{CoinSelectionAlgorithm, Error, UtxoPsbtInfo}; /// Minimum economical output value (dust limit). pub const DUST_LIMIT_SATS: u64 = 546; +/// Weight of the parts of a payment transaction that do not scale with the +/// number of selected inputs: the transaction overhead (version, segwit +/// marker/flag, input/output count varints, locktime) plus one recipient +/// output. +/// +/// BDK's `CoinSelectionAlgorithm::coin_select` only adds the *per-input* +/// satisfaction fee on top of `target_amount`; it does not account for this +/// fixed overhead. The transaction builder, however, pays for the whole +/// transaction. To make a selection returned here acceptable to the builder +/// for the same target and fee rate, the selection target must be inflated by +/// the fee for this fixed overhead. See +/// https://github.com/synonymdev/ldk-node/issues/104. +/// +/// When the recipient script is available, its exact serialized output weight +/// is used. APIs without a recipient retain the conservative standard-output +/// allowance below. +/// - tx overhead: 4 (version) + 2 (marker/flag) + ~3 (varints) + 4 (locktime) ~ 13 vB +/// - recipient output: 43 vB (P2TR/P2WSH) +/// +/// Total ~56 vB = 224 WU. +const TX_ENVELOPE_WEIGHT: Weight = Weight::from_wu(42); +const CONSERVATIVE_PAYMENT_OVERHEAD: Weight = Weight::from_wu(224); + /// Calculate the satisfaction weight for a UTXO based on its script type. pub fn calculate_utxo_weight(script_pubkey: &ScriptBuf) -> Weight { if script_pubkey.is_p2wpkh() { @@ -161,9 +184,32 @@ pub fn add_utxos_to_tx_builder( Ok(()) } +pub(crate) struct SelectionRequest<'a, K, P> { + pub(crate) target_amount: u64, + pub(crate) available_utxos: Vec, + pub(crate) fee_rate: FeeRate, + pub(crate) algorithm: CoinSelectionAlgorithm, + pub(crate) drain_script: &'a Script, + pub(crate) excluded_outpoints: &'a [OutPoint], + pub(crate) wallets: &'a HashMap>, + pub(crate) recipient_script: Option<&'a Script>, + pub(crate) include_payment_overhead: bool, +} + /// Run coin selection across UTXOs from any wallet. -#[allow(clippy::too_many_arguments)] -pub fn select_utxos_with_algorithm( +pub(crate) fn select_utxos_with_algorithm( + request: SelectionRequest<'_, K, P>, +) -> Result, Error> +where + K: Eq + Hash + Copy + Debug, + P: WalletPersister, +{ + select_utxos_with_algorithm_inner(request) +} + +/// Run coin selection for a precomputed fee deficit, without adding payment +/// transaction overhead a second time. +pub(crate) fn select_utxos_for_deficit( target_amount: u64, available_utxos: Vec, fee_rate: FeeRate, algorithm: CoinSelectionAlgorithm, drain_script: &Script, excluded_outpoints: &[OutPoint], wallets: &HashMap>, @@ -172,6 +218,37 @@ where K: Eq + Hash + Copy + Debug, P: WalletPersister, { + select_utxos_with_algorithm_inner(SelectionRequest { + target_amount, + available_utxos, + fee_rate, + algorithm, + drain_script, + excluded_outpoints, + wallets, + recipient_script: None, + include_payment_overhead: false, + }) +} + +fn select_utxos_with_algorithm_inner( + request: SelectionRequest<'_, K, P>, +) -> Result, Error> +where + K: Eq + Hash + Copy + Debug, + P: WalletPersister, +{ + let SelectionRequest { + target_amount, + available_utxos, + fee_rate, + algorithm, + drain_script, + excluded_outpoints, + wallets, + recipient_script, + include_payment_overhead, + } = request; let safe_utxos: Vec = available_utxos .into_iter() .filter(|utxo| !excluded_outpoints.contains(&utxo.outpoint)) @@ -195,7 +272,17 @@ where }) .collect(); - let target = Amount::from_sat(target_amount); + // BDK's coin selection accounts for input fees, while the transaction + // builder also charges the fixed transaction envelope and recipient output. + let target = if include_payment_overhead { + let payment_overhead = recipient_script + .map(|script| TX_ENVELOPE_WEIGHT + recipient_output_weight(script)) + .unwrap_or(CONSERVATIVE_PAYMENT_OVERHEAD); + let base_fee = fee_rate.fee_wu(payment_overhead).ok_or(Error::InvalidFeeRate)?; + Amount::from_sat(target_amount).checked_add(base_fee).ok_or(Error::InsufficientFunds)? + } else { + Amount::from_sat(target_amount) + }; let mut rng = OsRng; let result = match algorithm { @@ -239,12 +326,6 @@ where Error::CoinSelectionFailed })?; - if let Excess::Change { amount, .. } = result.excess { - if amount.to_sat() > 0 && amount.to_sat() < DUST_LIMIT_SATS { - return Err(Error::CoinSelectionFailed); - } - } - let selected_outputs: Vec = result .selected .into_iter() @@ -262,3 +343,7 @@ where ); Ok(selected_outputs.into_iter().map(|u| u.outpoint).collect()) } + +fn recipient_output_weight(recipient_script: &Script) -> Weight { + TxOut { value: Amount::ZERO, script_pubkey: recipient_script.to_owned() }.weight() +} diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index 26fde71f8..12b3620f2 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -110,6 +110,13 @@ fn additional_input_weight(utxos: &[UtxoPsbtInfo]) -> Result { Ok(Weight::from_wu(total)) } +fn checked_sum(values: I) -> Result +where + I: IntoIterator, +{ + values.into_iter().try_fold(0, u64::checked_add).ok_or(Error::InsufficientFunds) +} + #[derive(Clone, Copy)] pub(crate) enum OnchainSendAmount { ExactRetainingReserve { amount_sats: u64, cur_anchor_reserve_sats: u64 }, @@ -1530,32 +1537,19 @@ impl Wallet { } // Calculate total value of selected UTXOs - let selected_value: u64 = all_utxos - .iter() - .filter(|u| outpoints.contains(&u.outpoint)) - .map(|u| u.txout.value.to_sat()) - .sum(); - - // For exact amounts, ensure we have enough value - if let OnchainSendAmount::ExactRetainingReserve { amount_sats, .. } = send_amount { - // Calculate a fee buffer based on fee rate - // Assume a typical tx with 1 input and 2 outputs (~200 vbytes) - let typical_tx_weight = Weight::from_vb(200).expect("Valid weight"); - let fee_buffer = - fee_rate.fee_wu(typical_tx_weight).expect("Valid fee calculation").to_sat(); - // Use at least 1000 sats as minimum buffer - let min_fee_buffer = fee_buffer.max(1000); - let min_required = amount_sats.saturating_add(min_fee_buffer); - if selected_value < min_required { - log_error!( - self.logger, - "Selected UTXOs have insufficient value. Have: {}sats, Need at least: {}sats", - selected_value, - min_required - ); - return Err(Error::InsufficientFunds); - } - } + let selected_value = checked_sum( + all_utxos + .iter() + .filter(|u| outpoints.contains(&u.outpoint)) + .map(|u| u.txout.value.to_sat()), + )?; + + // No fee-buffer precheck here: the transaction builder below charges + // for the exact selected inputs and the actual recipient output, and + // returns `InsufficientFunds` if they cannot fund the payment. A + // heuristic estimate (e.g. clamping to a fixed minimum buffer) can + // reject selections the builder would accept, see the review + // discussion on https://github.com/synonymdev/ldk-node/pull/109. log_debug!( self.logger, @@ -2409,7 +2403,7 @@ impl ChangeDestinationSource for WalletKeysManager { #[cfg(test)] mod tests { use super::{ - additional_input_weight, map_wallet_account_error, validate_derivation_index, + additional_input_weight, checked_sum, map_wallet_account_error, validate_derivation_index, validate_derivation_range, BIP32_MAX_NORMAL_INDEX, MAX_ADDRESS_INFO_BATCH_COUNT, }; use crate::config::{AddressType, OnchainWalletAccount}; @@ -2485,4 +2479,9 @@ mod tests { TxIn::default().segwit_weight() + satisfaction_weight ); } + + #[test] + fn checked_sum_rejects_overflow() { + assert_eq!(checked_sum([u64::MAX, 1]), Err(Error::InsufficientFunds)); + } } diff --git a/tests/multi_address_types_tests.rs b/tests/multi_address_types_tests.rs index 060716e41..01aaf977f 100644 --- a/tests/multi_address_types_tests.rs +++ b/tests/multi_address_types_tests.rs @@ -38,6 +38,15 @@ mod helpers { .unwrap() } + /// Standard regtest P2TR recipient address (43-byte output). + pub fn test_p2tr_recipient() -> Address { + let secp = bitcoin::secp256k1::Secp256k1::new(); + let secret = bitcoin::secp256k1::SecretKey::from_slice(&[0x01; 32]).unwrap(); + let keypair = bitcoin::secp256k1::Keypair::from_secret_key(&secp, &secret); + let (xonly, _) = keypair.x_only_public_key(); + Address::p2tr(&secp, xonly, None, bitcoin::Network::Regtest) + } + /// Fund a single address, mine 6 blocks, sync the node, and sleep. pub async fn fund_and_sync( bitcoind: &BitcoinD, electrsd: &ElectrsD, node: &Node, addr: Address, amount: u64, @@ -2548,17 +2557,22 @@ mod cpfp { // Coin Selection // --------------------------------------------------------------------------- mod coin_selection { - use bitcoin::FeeRate; + use std::collections::HashMap; + + use bitcoin::{FeeRate, Txid}; use electrum_client::ElectrumApi; + use ldk_node::bitcoin::Amount; use ldk_node::config::AddressType; + use ldk_node::NodeError; + use serde_json::{json, Value}; use crate::common::{ - api_fee_rate, open_channel, setup_bitcoind_and_electrsd, setup_node, wait_for_tx, - TestChainSource, + api_fee_rate, generate_blocks_and_wait, open_channel, premine_blocks, + setup_bitcoind_and_electrsd, setup_node, wait_for_tx, TestChainSource, }; use crate::helpers::{ fund_and_sync, fund_multiple_and_sync, fund_peer_node_and_sync, node_config, - test_recipient, CHANNEL_PEER_FUNDING_SATS, + test_p2tr_recipient, test_recipient, CHANNEL_PEER_FUNDING_SATS, }; // --- API & fee calculation --- @@ -2852,6 +2866,127 @@ mod coin_selection { node.stop().unwrap(); } + + /// A selection from `select_utxos_with_algorithm` must be accepted by + /// `send_to_address` for the same target amount and fee rate, for every + /// supported algorithm. Regression test for + /// https://github.com/synonymdev/ldk-node/issues/104 (BranchAndBound always failed). + /// + /// Boundary fixture: at 1,000 sat/kwu (1 sat/vB), 10,470 sats covers the + /// 10,000-sat target + 1 input fee (271 sats) + 188 WU base fee (188 sats), + /// so a 188-WU allowance selects only the 10,470-sat UTXO. However, spending + /// 1 P2WPKH input to a 43 vB P2TR output needs 485 sats fee (10,485 total), + /// making 10,470 sats insufficient for TxBuilder. The 224-WU allowance + /// targets 10,495 sats, correctly forcing selection of the second (500 sat) + /// UTXO, which TxBuilder accepts (756 sat fee, 10,756 total needed <= 10,970). + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] + async fn test_selected_utxos_are_accepted_by_send_to_address() { + let (bitcoind, electrsd) = setup_bitcoind_and_electrsd(); + let chain_source = TestChainSource::Esplora(&electrsd); + + let utxo_amounts = [10_470u64, 500]; + let algorithms = [ + ldk_node::CoinSelectionAlgorithm::BranchAndBound, + ldk_node::CoinSelectionAlgorithm::LargestFirst, + ldk_node::CoinSelectionAlgorithm::OldestFirst, + ]; + + // One node per algorithm, so each send starts from the identical wallet state. + let mut nodes = Vec::new(); + for _ in &algorithms { + let config = node_config(AddressType::NativeSegwit, vec![]); + nodes.push(setup_node(&chain_source, config, None)); + } + + premine_blocks(&bitcoind.client, &electrsd.client).await; + let mut amounts = HashMap::::new(); + for node in &nodes { + for amount in utxo_amounts { + let addr = node.onchain_payment().new_address().unwrap(); + amounts.insert(addr.to_string(), Amount::from_sat(amount).to_btc()); + } + } + let funding_txid: Txid = bitcoind + .client + .call::("sendmany", &[json!(""), json!(amounts)]) + .unwrap() + .as_str() + .unwrap() + .parse() + .unwrap(); + wait_for_tx(&electrsd.client, funding_txid).await; + generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 6).await; + for node in &nodes { + node.sync_wallets().unwrap(); + } + std::thread::sleep(std::time::Duration::from_secs(2)); + + let target_amount_sats = 10_000; + let fee_rate = api_fee_rate(FeeRate::from_sat_per_kwu(1_000)); + let recipient = test_p2tr_recipient(); + + for (node, algorithm) in nodes.iter().zip(algorithms.iter()) { + let selected = node + .onchain_payment() + .select_utxos_with_algorithm( + target_amount_sats, + Some(fee_rate.clone()), + *algorithm, + None, + ) + .unwrap_or_else(|e| panic!("{algorithm:?} selection failed: {e:?}")); + + let txid = node + .onchain_payment() + .send_to_address( + &recipient, + target_amount_sats, + Some(fee_rate.clone()), + Some(selected), + ) + .unwrap_or_else(|e| { + panic!("{algorithm:?} selection rejected by send_to_address: {e:?}") + }); + wait_for_tx(&electrsd.client, txid).await; + } + + for node in &nodes { + node.stop().unwrap(); + } + } + + /// A manually selected UTXO set that cannot cover the amount plus the + /// actual transaction fee must be rejected with `InsufficientFunds` by the + /// transaction builder, now that the fee-buffer precheck is gone. + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] + async fn test_insufficient_manual_selection_is_rejected() { + let (bitcoind, electrsd) = setup_bitcoind_and_electrsd(); + let chain_source = TestChainSource::Esplora(&electrsd); + + let config = node_config(AddressType::NativeSegwit, vec![]); + let node = setup_node(&chain_source, config, None); + + let addr = node.onchain_payment().new_address().unwrap(); + fund_and_sync(&bitcoind, &electrsd, &node, addr, 35_050).await; + + let utxos = node.onchain_payment().list_spendable_outputs().unwrap(); + assert_eq!(utxos.len(), 1); + + // 35,050 sats covers the 35,000-sat payment but not the fee for + // spending the input at this fee rate, so the builder must reject it. + let err = node + .onchain_payment() + .send_to_address( + &test_recipient(), + 35_000, + Some(api_fee_rate(FeeRate::from_sat_per_kwu(250))), + Some(utxos), + ) + .unwrap_err(); + assert!(matches!(err, NodeError::InsufficientFunds), "unexpected error: {err:?}"); + + node.stop().unwrap(); + } } // ---------------------------------------------------------------------------