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.

diff --git a/src/wallet/coin_selection.rs b/src/wallet/coin_selection.rs
index 74dbe38bc84a68c316ba456afc67e082f7936528..21921bc61dc053768bca5fc2c919aea298a8a3be 100644 (file)
--- a/src/wallet/coin_selection.rs
+++ b/src/wallet/coin_selection.rs
@@ -438,10 +438,12 @@ impl<D: Database> CoinSelectionAlgorithm<D> 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<OutputGroup> = 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<D: Database> CoinSelectionAlgorithm<D> 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<u64, _> = (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,
+                ..
+            }
+        ));
+    }
 }