This allows a transaction's weight to be bound under a certain
weight if possible and desired. This can be beneficial for future
RBF attempts, or whenever a more restricted spend topology is
desired.
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
- This commit renames the coin selection algorithms input parameter `max_weight`
to `max_selection_weight` for clarity.
The parameter represent the maximum weight of the UTXOs the coin selection algorithm
should select, not the transaction maximum weight.
- The commit updates the parameter docstring to provide correct description.
- Also updates coin selection unit and fuzzing test variables to match the new name.
bd34dd85e7b8b4cc26d2173d84bbeda2e9c27624 Use `exact_target` shorthand in coinselector_tests (Murch)
7aa7e30441fe77bf8e8092916e36b004bbbfe2a7 Fold GetSelectionWaste() into ComputeAndSetWaste() (Murch)
Pull request description:
PR #26152 moved waste calculation into SelectionResult to be able to correct the waste score on basis of the bump_fee_group_discount for overlapping ancestries. This left two functions with largely overlapping purpose, where one was simply a wrapper of the other. This PR cleans up the overlap, and fixes the double-meaning of `change_cost` where the `GetChange()` function assumed that no change was created when `change_cost` was set to 0. This behavior was exploited in a bunch of tests, but is problematic, because a `change_cost` of 0 is permitted with custom settings for feerate and discard_feerate (i.e. when they’re both 0).
ACKs for top commit:
achow101:
ACK bd34dd85e7b8b4cc26d2173d84bbeda2e9c27624
furszy:
Code ACK bd34dd85e7b8b4cc26d2173d84bbeda2e9c27624
ismaelsadeeq:
Code Review ACK bd34dd85e7b8b4cc26d2173d84bbeda2e9c27624
Tree-SHA512: 83a2688d45d719dc61a64b5180fe136107faccf401a59df65245c05d701748a03e85ed56fde8c9b7ef39a3ab54374dd3718c559bda5b3f55dafedfd7fed25161
Both `GetSelectionWaste()` and `ComputeAndSetWaste()` now are part of
`SelectionResult`. Instead of `ComputeAndSetWaste()` being a wrapper for
`GetSelectionWaste()`, we combine them to a new function
`RecalculateWaste()`.
As I was combining the logic of the two functions, I noticed that
`GetSelectionWaste()` was making the odd assumption that the
`change_cost` being set to zero means that no change is created.
However, if we build transactions at a feerate of zero with the
`discard_feerate` also set to zero, we'd organically have a
`change_cost` of zero, even when we create change on a transaction.
This commit cleans up this duplicate meaning of `change_cost` and relies
on `GetChange()` to figure out whether there is change on basis of the
`min_viable_change` and whatever is left after deducting fees.
Since this broke a bunch of tests that relied on the double-meaning of
`change_cost` a bunch of tests had to be fixed.
Copying util::Result values is less efficient than moving them because they
allocate memory and contain strings. Also this is needed to avoid compile
errors in https://github.com/bitcoin/bitcoin/pull/25722 which adds a
std::unique_ptr member to util::Result which implicity disables copying.
The valid results should have a target below the sum of
the selected inputs amounts. Also, it increases the
minimum value for target to make it more realistic.
Instead of using `cost_of_change` for `min_viable_change`
and `change_cost`, and 0 for `change_fee`, use values from
`coin_params`. The previous values don't generate any effects
that is relevant for that context.
I discovered via fuzzing of another coin selection approach that at
extremely high feerates SRD may find input sets that lead to
transactions without change outputs. This is an unintended outcome since
SRD is meant to always produce a transaction with a change output—we use
other algorithms to specifically search for changeless solutions.
The issue occures when the flat allowance of 50,000 ṩ for change is
insufficient to pay for the creation of a change output with a non-dust
amount, at and above 1,613 ṩ/vB. Increasing the change budget by
change_fees makes SRD behave as expected at any feerates.
Uses a min-effective-value heap, so we can remove the least valuable input/s
while the selected weight exceeds the maximum allowed weight.
Co-authored-by: Murch <murch@murch.one>
The simplest scenario where this is useful is on the 'check_max_weight' unit test
already:
We create 1515 UTXOs with 0.033 BTC each, and 1 UTXO with 50 BTC. Then perform
Coin Selection.
As the selection of the 1515 small UTXOs exceeds the max allowed tx size, the
expectation here is to receive a selection result that only contain the big
UTXO (which is not happening for the reasons stated below).
As knapsack returns a result that exceeds the max allowed transaction size, we
fallback to SRD, which selects coins randomly up until the target is met. So
we end up with a selection result with lot more coins than what is needed.
And not hide it inside the `OutputGroup::Insert` method.
This method does not return anything if insertion fails.
We can know before calling `Insert` whether the coin
will be accepted or not.
and also fix spelling in test/lint/lint-locale-dependence.py not caught by the
spelling linter and fix up a paragraph we are touching here in test/README.md.
