Merge #693: Fix the early InsufficientFunds error in the branch and b…

…ound

9d85c96 Fix the early InsufficientFunds error in the branch and bound (Alekos Filini)

Pull request description:

  ### Description

  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.

  ### Notes to the reviewers

  I'm opening this as a draft before adding tests because I want to gather some feedback on the available vs needed error reporting. I personally think reporting a reasonable "needed" value is more important than the "available", because in a wallet app I would expect this is the value that would be shown to the user.

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### Bugfixes:

  * [ ] This pull request breaks the existing API
  * [ ] I've added tests to reproduce the issue which are now passing
  * [ ] I'm linking the issue being fixed by this PR

ACKs for top commit:
  danielabrozzoni:
    utACK 9d85c96

Tree-SHA512: 9a06758cba61ade73198f35b08070987d5eb065e01750ce62409f86b37cd0b0894640e9f75c8b2c26543c0da04e3f77bd397fab540e789f221661aae828db224

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,
+                ..
+            }
+        ));
+    }
 }