Fix the early InsufficientFunds error in the branch and bound #693
Conversation
| let optional_utxos: Vec<OutputGroup> = optional_utxos | ||
| .into_iter() | ||
| .map(|u| OutputGroup::new(u, fee_rate)) | ||
| .filter(|u| u.effective_value.is_positive()) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
I agree with your approach. I think that we can't do better than this with the current available fields. If we wanted to go further we would have to provide more information about the uneconomical outputs, adding more complexity. |
|
Since we are running late for the release I think we should just merge this as-is, since it's still a pretty big improvements over the current error. I tried experimenting with adding a So I'm gonna add some tests for my fixes and mark this ready for review |
afilini
force-pushed
the
fix/bnb-early-error
branch
from
45d8fc4
to
b535ad5
Compare
afilini
force-pushed
the
fix/bnb-early-error
branch
2 times, most recently
from
1c4910e
to
a399fd0
Compare
src/wallet/mod.rs.rej
Outdated
| @@ -0,0 +1,46 @@ | |||
| diff a/src/wallet/mod.rs b/src/wallet/mod.rs (rejected hunks) | |||
There was a problem hiding this comment.
This file has probably been commited by mistake :)
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.
afilini
force-pushed
the
fix/bnb-early-error
branch
from
a399fd0
to
9d85c96
Compare
|
utACK 9d85c96 |
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:
conversion because we assumed the sum of effective values would never
be negative
(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:
cargo fmtandcargo clippybefore committingBugfixes: