-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
[Bitcoin]: UTXO selection and dust improvements #3675
Conversation
* Filter out Dust UTXOs before actual UTXO selecting
* Filter out Dust UTXOs before actual UTXO selecting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So disclaimer - I don't grasp the full C++ implementation, but generally this LGTM.
} | ||
|
||
Amount LegacyDustCalculator::dustAmount([[maybe_unused]] Amount byteFee) noexcept { | ||
return feeCalculator.calculateSingleInput(byteFee); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking, is it even appropriate to refer to this as dustAmount
?
int64_t LinearFeeCalculator::calculateSingleInput(int64_t byteFee) const noexcept {
return static_cast<int64_t>(std::ceil(bytesPerInput)) * byteFee; // std::ceil(101.25) = 102
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but this is how our current chains work. This is a tradeoff to avoid breaking changes for other Bitcoin forks.
33ae5c4
Description
SigningInput::Proto::dust_policy
with thefixed_dust_threshold
option.InputSelector::select
returns entire list of UTXOs if it couldn't find a combination of inputs to cover estimated transaction fee and the target amount. This is needed, becauseFeeEstimator
estimates Segwit transaction too rough, however later, inestimateSegwitFee
function, the transaction fee is significantly less, so the given UTXOs enough to cover it.How to test
Run C++ tests
Types of changes
Checklist
If you're adding a new blockchain