05300c1439 Use SelectionResult in SelectCoins (Andrew Chow)
9d9b101d20 Use SelectionResult in AttemptSelection (Andrew Chow)
bb50850a44 Use SelectionResult for waste calculation (Andrew Chow)
e8f7ae5eb3 Make an OutputGroup for preset inputs (Andrew Chow)
51a9c00b4d Return SelectionResult from SelectCoinsSRD (Andrew Chow)
0ef6184575 Return SelectionResult from KnapsackSolver (Andrew Chow)
60d2ca72e3 Return SelectionResult from SelectCoinsBnB (Andrew Chow)
a339add471 Make member variables of SelectionResult private (Andrew Chow)
cbf0b9f4ff scripted-diff: Use SelectionResult in coin selector tests (Andrew Chow)
9d1d86da04 Introduce SelectionResult struct (Andrew Chow)
94d851d28c Fix bnb_search_test to use set equivalence for (Andrew Chow)
Pull request description:
Instead of returning a set of selected coins and their total value as separate items, encapsulate both of these, and other variables, into a new `SelectionResult` struct. This allows us to have all of the things relevant to a coin selection solution be in a single object. `SelectionResult` enables us to implement the waste calculation in a cleaner way.
All of the coin selection functions (`SelectCoinsBnB`, `KnapsackSolver`, `AttemptSelection`, and `SelectCoins`) are changed to use a `SelectionResult` as the output parameter.
Based on #22009
ACKs for top commit:
laanwj:
Code review ACK 05300c1439
Tree-SHA512: e4dbb4d78a6cda9c237d230b19e7265591efac5a101a64e6970f0654e2c4f93d13bb5d07b98e8c7b8d37321753dbfc94c28c3a7810cb1c59b5bc29b08a8493ef
Introduces a SelectionResult struct which contains the set of selected
inputs and the total transaction fee for the transaction. This will be
used by the various SelectCoins* functions. Additionally helpers are
provided to compute the total input value and result comparisons.
9d0379cea6 consensus: use <cstdint> over <stdint.h> in amount.h (fanquake)
863e52fe63 consensus: make COIN & MAX_MONEY constexpr (fanquake)
d09071da5b [MOVEONLY] consensus: move amount.h into consensus (fanquake)
Pull request description:
A first step (of a few) towards some source code reorganization, as well as making libbitcoinconsensus slightly more self contained.
Related to #15732.
ACKs for top commit:
MarcoFalke:
concept ACK 9d0379cea6 🏝
Tree-SHA512: 97fc79262dcb8c00996852a288fee69ddf8398ae2c95700bba5b326f1f38ffcfaf8fa66e29d0cb446d9b3f4e608a96525fae0c2ad9cd531ad98ad2a4a687cd6a
Before this patch, the log might be corrupted by other threads logging
at the same time. For example, another RPC thread:
[httpworker.1] [default wallet] keypool reserve 1296
[httpworker.1] SelectCoins() best subset: Received a POST request for / from 127.0.0.1:53732
[httpworker.3] ThreadRPCServer method=getnetworkinfo user=__cookie__
[httpworker.1] 0.78125 0.1953125 0.02441406 0.00610351 0.00305175 0.00152587 total 1.01025417
For target value calculations, GetSelectionAmount should be used, not
m_effective_value or m_value.
Specifically, ApproximateBestSubset mistakenly uses m_value when
calculating whether the target value has been met. This has been changed
to use GetSelectionAmount.
This removes a lot of the craziness that was added during the 0.21
rebase 240f03f586 (Bitcoin PR #17290)
as well as a lot of other upstream craziness. It's a very positive
change but will probably be a bear to review.
There are two places where I (purposely) nontrivially changed the
logic beyond what the upstream PR did:
1. When setting the change output positions, I used a uniform
distribution for all change outputs and also obeyed the user's
`change_position` choice. The old code was non-uniform and
had a bug where it would offset the change position.
For non-policy assets, I do not create zero-sized change
since we would just remove them later. For the policy asset,
I do sometimes create zero-sized change since the upstream
logic expects it to exist.
2. Rather than "reblinding whenever something changes" we blind
once for size/fee estimation purposes and then again at the
end after all potential adjustments have been made. The
resulting code should be closer to upstream.
We should really redo this code though because our use of the
`BlindDetails` structure is very confusing and stateful.
3. We consider whether or not we're using CT/CA when estimating
the size of change outpust. This is because we now use branch
and bound far more often (yay!) which selects coins such that
there will be no change output. This is implemented however
by adding a change output then dumping it to fees ... meaning
that if we dramatically overestimate the size of a change out
then we'll end up dramatically dumping too many coins to fees.
There are another couple small things, which I apologize for .. this
PR took me 2 hours to get compiling and 11 hours to get working(!!)
and some things got away from me.
One fun thing is that I changed CWallet::SelectCoinsMinConf to gate
an addition on coin_selection_params.m_subtract_fee_outputs ... this
is actually an upstream bug that I'll file whenever I get around to
producing a test that triggers it. (A bit hard as I have to gin up
a scenario where branch-and-bound fails to hit this codepath.)
Also, in test/functional/wallet_bumpfee.py I had to change the feerate
required to force selection to add another input from 500 to 800. I
don't know why and I don't care to figure it out.
This is a really non-trivial wallet refactor "effective value" which has been
in the Core queue for a pretty long time. There were a ton of merge conflicts
but they were mostly mechanical (all the benchmarks) or "take the Core side
of the diff" (most of wallet.cpp).
The two nontrivial things were:
* In `SelectCoinsMinConf` in wallet.cpp I had to slightly move the asset
filtering logic (but I didn't change it)
* In `OutputGroup::Insert' in coinselection.cpp I changed the effective
value computation to only consider policyAsset values
Instead of hijacking the effective_feerate to use the correct value
during coin selection, have OutputGroup be aware of whether we are
subtracting the fee from the outputs and provide the correct value to
use for selection.
To do this, OutputGroup now takes CoinSelectionParams and has a new
function GetSelectionAmount().
Although the CreateTransaction loop currently remains, it should be
largely unused. KnapsackSolver will now account for transaction fees
when doing its selection.
In the previous commit, SelectCoinsMinConf was refactored to have some
calculations become shared for KnapsackSolver and SelectCoinsBnB. In
this commit, KnapsackSolver will now use the not_input_fees and
effective_feerate so that it include the fee for non-input things
(excluding a change output) so that the algorithm will select enough to
cover those fees. This is necessary for selecting on effective values.
Additionally, the OutputGroups
created for KnapsackSolver will actually have their effective values
calculated and set, and KnapsackSolver will do its selection on those
effective values.
Lastly, SelectCoins is modified to use the same value for preselected
inputs for BnB and KnapsackSolver. While it will still use the real
value when subtracting the fee from outputs, this behavior will be
the same regardless of the algo used for selecting additional inputs.
The fees for transaction overhead and recipient outputs are now included
in nTargetValue instead of being a separate parameter. For the coin
selection algorithms, it doesn't matter that these are separate as in
either case, the algorithm needs to select enough to cover these fees.
Note that setting nValueToSelect is changed as it now includes
not_input_fees. Without the change to how nValueToSelect is increased
for KnapsackSolver, this would result in overpaying fees. The change to
increase by the difference between nFeeRet and not_input_fees allows
this to have the same behavior as previously.
Additionally, because we assume that KnapsackSolver will always find a
solution that requires change (we assume that BnB always finds a
non-change solution), we also include the fee for the change output in
KnapsackSolver's target. As part of this, we also use the changeless
nFeeRet when iterating for KnapsackSolver. This is because we include
the change fee when doing KnapsackSolver, so nFeeRet on further
iterations won't include the change fee.
5d4597666d Rewrite OutputGroups to be clearer and to use scriptPubKeys (Andrew Chow)
f6b3052739 Explicitly filter out partial groups when we don't want them (Andrew Chow)
416d74fb16 Move OutputGroup positive only filtering into Insert (Andrew Chow)
d895e98b59 Move EligibleForSpending into GroupOutputs (Andrew Chow)
99b399aba5 Move fee setting of OutputGroup to Insert (Andrew Chow)
6148a8acda Move GroupOutputs into SelectCoinsMinConf (Andrew Chow)
2acad03657 Remove OutputGroup non-default constructors (Andrew Chow)
Pull request description:
Even after #17458, we still deal with setting fees of an `OutputGroup` and filtering the `OutputGroup` outside of the struct. We currently make all of the `OutputGroup`s in `SelectCoins` and then copy and modify them within each `SelectCoinsMinConf` scenario. This PR changes this to constructing the `OutputGroup`s within the `SelectCoinsMinConf` so that the scenario can be taken into account during the group construction. Furthermore, setting of fees and filtering for effective value is moved into `OutputGroup::Insert` itself so that we don't add undesirable outputs to an `OutputGroup` rather than deleting them afterwards.
To facilitate fee calculation and effective value filtering during `OutputGroup::Insert`, `OutputGroup` now takes the feerates in its constructor and computes the fees and effective value for each output during `Insert`.
While removing `OutputGroup`s in accordance with the `CoinEligibilityFilter` still requires creating the `OutputGroup`s first, we can do that within the function that makes them - `GroupOutput`s.
ACKs for top commit:
Xekyo:
Code review ACK: https://github.com/bitcoin/bitcoin/pull/20040/commits/5d4597666d589e39354e0d8dd5b2afbe1a5d7d8e
fjahr:
Code review ACK 5d4597666d
meshcollider:
Light utACK 5d4597666d
Tree-SHA512: 35965b6d49a87f4ebb366ec4f00aafaaf78e9282481ae2c9682b515a3a9f2cbcd3cd6e202fee29489d48fe7f3a7cede4270796f5e72bbaff76da647138fb3059
OutputGroup will handle the fee and effective value computations
inside of Insert. It now needs to take the effective feerate and long
term feerates as arguments to its constructor.
Instead of having callers set the fees, effective values, and filtering
of outputs, do these within OutputGroups themselves as member functions.
m_fee and m_long_term_fee is added to OutputGroup to track the fees of
the OutputGroup.
If we find a solution which has no waste, just use that. This solution
is what we would consider to be optimal, and other solutions we find
would have to also have 0 waste, so they are equivalent to the first
one with 0 waste. Thus we can optimize by just choosing the first one
with 0 waste.
After this:
- `boost::optional` is no longer used directly (only through `Optional`
which is an alias for it)
- `boost/optional.hpp` is only included in one place
