frost/bdk
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge bitcoindevkit/bdk#693: Fix the early InsufficientFunds error in the branch and bound

Browse Source 
9d85c9667f7d12902afef3ba08ea7231f6868a78 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 9d85c9667f7d12902afef3ba08ea7231f6868a78

Tree-SHA512: 9a06758cba61ade73198f35b08070987d5eb065e01750ce62409f86b37cd0b0894640e9f75c8b2c26543c0da04e3f77bd397fab540e789f221661aae828db224
This commit is contained in:
Daniela Brozzoni 2022-08-03 19:53:53 +02:00
commit ef03da0a76
No known key found for this signature in database
GPG Key ID: 7DE4F1FDCED0AB87
1 changed files with 123 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

140
src/wallet/coin_selection.rs
View File

@ -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())
})?;
// `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;
if expected < target_amount {
return Err(Error::InsufficientFunds {
needed: target_amount,
available: expected,
});
(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,
..
}
));
}
}