Skip to content

Fix the early InsufficientFunds error in the branch and bound #693

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
danielabrozzoni merged 1 commit into from
Aug 3, 2022
Merged

Fix the early InsufficientFunds error in the branch and bound #693

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 124 additions & 18 deletions src/wallet/coin_selection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Copy link
Copy Markdown
Contributor

@csralvall csralvall Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine to compute the curr_value but there might be a reason to still include the output_groups with negative effective value in the vector consumed by bnb and single_random_draw. E.g.:
There are three utxos in the wallet:
(A) with effective value of 100
(B) with effective value of 100
(C) with effective value of -75
If actual_target is equal to 125, wouldn't be Vec<A,B,C> the best selection possible for bnb or single_random_draw?

Copy link
Copy Markdown
Member Author

@afilini afilini Aug 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, with your solution you would spend all three UTXOs and not create a change. The final balance of the wallet would be zero.

If you exclude C, then the coin selection would pick A + B and create a change so at the end the balance of your wallet is C + the change. Which is arguably a better outcome for the user.

Remember that the effective value isn't fixed for a given UTXO, it depends on the fee rate. So maybe at a high fee rate both the change and C are negative, but at a lower fee rate they are not. Maybe today there's a fee spike or you are just in a hurry and use a high fee rate, tomorrow you can consolidate whatever is left at the min relay fee.

Copy link
Copy Markdown
Contributor

@csralvall csralvall Aug 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's clear. I was thinking in a situation where even at the min_relay_fee the utxos were uneconomical. But, would be probably fixed by the consolidation you mentioned in the last line.

.collect();

let curr_value = required_utxos
Expand All @@ -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
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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,
..
}
));
}
}