From 9d85c9667f7d12902afef3ba08ea7231f6868a78 Mon Sep 17 00:00:00 2001 From: Alekos Filini Date: Tue, 26 Jul 2022 17:27:40 +0200 Subject: [PATCH] Fix the early InsufficientFunds error in the branch and bound We were wrongly considering the sum of "effective value" (i.e. value - fee cost) when reporting an early "insufficient funds" error in the branch and bound coin selection. This commit fixes essentially two issues: - Very high fee rates could cause a panic during the i64 -> u64 conversion because we assumed the sum of effective values would never be negative - Since we were comparing the sum of effective values of *all* the UTXOs (even the optional UTXOs with negative effective value) with the target we'd like to reach, we could in some cases error and tell the user we don't have enough funds, while in fact we do! Since we are not required to spend any of the optional UTXOs, so we could just ignore the ones that *cost us* money to spend and excluding them could potentially allow us to reach the target. There's a third issue that was present before and remains even with this fix: when we report the "available" funds in the error, we are ignoring UTXOs with negative effective value, so it may look like there are less funds in the wallet than there actually are. I don't know how to convey the right message the user: if we actually consider them we just make the "needed" value larger and larger (which may be confusing, because if the user asks BDK to send 10k satoshis, why do we say that we actually need 100k?), while if we don't we could report an incorrect "available" value. --- src/wallet/coin_selection.rs | 142 ++++++++++++++++++++++++++++++----- 1 file changed, 124 insertions(+), 18 deletions(-) diff --git a/src/wallet/coin_selection.rs b/src/wallet/coin_selection.rs index 74dbe38bc..21921bc61 100644 --- a/src/wallet/coin_selection.rs +++ b/src/wallet/coin_selection.rs @@ -438,10 +438,12 @@ impl CoinSelectionAlgorithm for BranchAndBoundCoinSelection { .map(|u| OutputGroup::new(u, fee_rate)) .collect(); - // Mapping every (UTXO, usize) to an output group. + // Mapping every (UTXO, usize) to an output group, filtering UTXOs with a negative + // effective value let optional_utxos: Vec = optional_utxos .into_iter() .map(|u| OutputGroup::new(u, fee_rate)) + .filter(|u| u.effective_value.is_positive()) .collect(); let curr_value = required_utxos @@ -454,17 +456,39 @@ impl CoinSelectionAlgorithm for BranchAndBoundCoinSelection { let cost_of_change = self.size_of_change as f32 * fee_rate.as_sat_vb(); - let expected = (curr_available_value + curr_value) - .try_into() - .map_err(|_| { - Error::Generic("Sum of UTXO spendable values does not fit into u64".to_string()) - })?; - - if expected < target_amount { - return Err(Error::InsufficientFunds { - needed: target_amount, - available: expected, - }); + // `curr_value` and `curr_available_value` are both the sum of *effective_values* of + // the UTXOs. For the optional UTXOs (curr_available_value) we filter out UTXOs with + // negative effective value, so it will always be positive. + // + // Since we are required to spend the required UTXOs (curr_value) we have to consider + // all their effective values, even when negative, which means that curr_value could + // be negative as well. + // + // If the sum of curr_value and curr_available_value is negative or lower than our target, + // we can immediately exit with an error, as it's guaranteed we will never find a solution + // if we actually run the BnB. + let total_value: Result = (curr_available_value + curr_value).try_into(); + match total_value { + Ok(v) if v >= target_amount => {} + _ => { + // Assume we spend all the UTXOs we can (all the required + all the optional with + // positive effective value), sum their value and their fee cost. + let (utxo_fees, utxo_value) = required_utxos + .iter() + .chain(optional_utxos.iter()) + .fold((0, 0), |(mut fees, mut value), utxo| { + fees += utxo.fee; + value += utxo.weighted_utxo.utxo.txout().value; + + (fees, value) + }); + + // Add to the target the fee cost of the UTXOs + return Err(Error::InsufficientFunds { + needed: target_amount + utxo_fees, + available: utxo_value, + }); + } } let target_amount = target_amount @@ -1194,9 +1218,9 @@ mod test { ) .unwrap(); - assert_eq!(result.selected.len(), 3); - assert_eq!(result.selected_amount(), 300010); - assert_eq!(result.fee_amount, 204); + assert_eq!(result.selected.len(), 2); + assert_eq!(result.selected_amount(), 300000); + assert_eq!(result.fee_amount, 136); } #[test] @@ -1228,9 +1252,9 @@ mod test { ) .unwrap(); - assert_eq!(result.selected.len(), 3); - assert_eq!(result.selected_amount(), 300_010); - assert!((result.fee_amount as f32 - 204.0).abs() < f32::EPSILON); + assert_eq!(result.selected.len(), 2); + assert_eq!(result.selected_amount(), 300_000); + assert_eq!(result.fee_amount, 136); } #[test] @@ -1487,4 +1511,86 @@ mod test { assert!(result.selected_amount() > target_amount); assert_eq!(result.fee_amount, (result.selected.len() * 68) as u64); } + + #[test] + fn test_bnb_exclude_negative_effective_value() { + let utxos = get_test_utxos(); + let database = MemoryDatabase::default(); + let drain_script = Script::default(); + + let err = BranchAndBoundCoinSelection::default() + .coin_select( + &database, + vec![], + utxos, + FeeRate::from_sat_per_vb(10.0), + 500_000, + &drain_script, + ) + .unwrap_err(); + + assert!(matches!( + err, + Error::InsufficientFunds { + available: 300_000, + .. + } + )); + } + + #[test] + fn test_bnb_include_negative_effective_value_when_required() { + let utxos = get_test_utxos(); + let database = MemoryDatabase::default(); + let drain_script = Script::default(); + + let (required, optional) = utxos + .into_iter() + .partition(|u| matches!(u, WeightedUtxo { utxo, .. } if utxo.txout().value < 1000)); + + let err = BranchAndBoundCoinSelection::default() + .coin_select( + &database, + required, + optional, + FeeRate::from_sat_per_vb(10.0), + 500_000, + &drain_script, + ) + .unwrap_err(); + + assert!(matches!( + err, + Error::InsufficientFunds { + available: 300_010, + .. + } + )); + } + + #[test] + fn test_bnb_sum_of_effective_value_negative() { + let utxos = get_test_utxos(); + let database = MemoryDatabase::default(); + let drain_script = Script::default(); + + let err = BranchAndBoundCoinSelection::default() + .coin_select( + &database, + utxos, + vec![], + FeeRate::from_sat_per_vb(10_000.0), + 500_000, + &drain_script, + ) + .unwrap_err(); + + assert!(matches!( + err, + Error::InsufficientFunds { + available: 300_010, + .. + } + )); + } }