be55f545d5 move-only: Extract common/args and common/config.cpp from util/system (TheCharlatan)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". It is part of a series of patches splitting up the `util/system` files. Its preceding pull request is https://github.com/bitcoin/bitcoin/pull/27254.
The pull request contains an extraction of ArgsManager related functions from util/system into their own common/ file.
The background of this commit is an ongoing effort to decouple the libbitcoinkernel library from the ArgsManager. The ArgsManager belongs into the common library, since the kernel library should not depend on it. See [doc/design/libraries.md](https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md) for more information on this rationale.
ACKs for top commit:
MarcoFalke:
re-ACK be55f545d5🚲
ryanofsky:
Code review ACK be55f545d5. Just small cleanups since the last review.
hebasto:
ACK be55f545d5, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 90eb03334af0155b823030b4f2ecf286d35058d700ee2ddbbaa445be19e31eb0fe982656f35bd14ecee3ad2c3d0db3746855cb8f3777eff7253713e42873e111
25ab14712b refactor: coinselector_tests, unify wallet creation code (furszy)
ba9431c505 test: coverage for bnb max weight (furszy)
5a2bc45ee0 wallet: clean post coin selection max weight filter (furszy)
2d112584e3 coin selection: BnB, don't return selection if exceeds max allowed tx weight (furszy)
d3a1c098e4 test: coin selection, add coverage for SRD (furszy)
9d9689e5a6 coin selection: heap-ify SRD, don't return selection if exceeds max tx weight (furszy)
6107ec2229 coin selection: knapsack, select closest UTXO above target if result exceeds max tx size (furszy)
1284223691 wallet: refactor coin selection algos to return util::Result (furszy)
Pull request description:
Coming from the following comment https://github.com/bitcoin/bitcoin/pull/25729#discussion_r1029324367.
The reason why we are adding hundreds of UTXO from different sources when the target
amount is covered only by one of them is because only SRD returns a usable result.
Context:
In the test, we create 1515 UTXOs with 0.033 BTC each, and 1 UTXO with 50 BTC. Then
perform Coin Selection to fund 49.5 BTC.
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 following reason:
Knapsack returns a result that exceeds the max allowed transaction size, when
it should return the closest utxo above the target, so we fallback to SRD who
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.
ACKs for top commit:
S3RK:
ACK 25ab14712b
achow101:
ACK 25ab14712b
Xekyo:
reACK 25ab14712b
theStack:
Code-review ACK 25ab14712b
Tree-SHA512: 2425de4cc479b4db999b3b2e02eb522a2130a06379cca0418672a51c4076971a1d427191173820db76a0f85a8edfff100114e1c38fb3b5dc51598d07cabe1a60
This is an extraction of ArgsManager related functions from util/system
into their own common file.
Config file related functions are moved to common/config.cpp.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager. The ArgsManager belongs
into the common library, since the kernel library should not depend on
it. See doc/design/libraries.md for more information on this rationale.
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.
The 'GroupOutputs()' function performs the same
calculations for only-positive and mixed groups,
the only difference is that when we look for
only-positive groups, we discard negative utxos.
So, instead of wasting resources calling GroupOutputs()
for positive-only first, then call it again to include
the negative ones in the result, we can execute
GroupOutputs() only once, including in the response
both group types (positive-only and mixed).
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.
76dc547ee7 gui: create tx, launch error dialog if backend throws runtime_error (furszy)
f4d79477ff wallet: coin selection, add duplicated inputs checks (furszy)
0aa065b14e wallet: return accurate error messages from Coin Selection (furszy)
7e8340ab1a wallet: make SelectCoins flow return util::Result (furszy)
e5e147fe97 wallet: refactor eight consecutive 'AttemptSelection' calls into a loop (furszy)
Pull request description:
Work decoupled from #25806, which cleanup and improves the Coin Selection flow further.
Adding the capability to propagate specific error messages from the Coin Selection process to the user.
Instead of always returning the general "Insufficient funds" message which is not always accurate to what happened internally.
Letting us instruct the user how to proceed under certain circumstances.
The following error messages were added:
1) If the selection result exceeds the maximum transaction weight,
we now will return:
-> "The inputs size exceeds the maximum weight. Please try sending
a smaller amount or manually consolidating your wallet's UTXOs".
2) If the user pre-selected inputs and disallowed the automatic coin
selection process (no other inputs are allowed), we now will
return:
-> "The preselected coins total amount does not cover the transaction
target. Please allow other inputs to be automatically selected or include
more coins manually".
3) The double-counted preset inputs during Coin Selection error will now
throw an "internal bug detected" message instead of crashing the node.
The essence of this work comes from several comments:
1. https://github.com/bitcoin/bitcoin/pull/26560#discussion_r1037395665
2. https://github.com/bitcoin/bitcoin/pull/25729#discussion_r940619491
3. https://github.com/bitcoin/bitcoin/pull/25269#pullrequestreview-1135240825
4. https://github.com/bitcoin/bitcoin/issues/23144 (which is connected to #24845)
ACKs for top commit:
ishaanam:
crACK 76dc547ee7
achow101:
ACK 76dc547ee7
aureleoules:
ACK 76dc547ee7
theStack:
ACK 76dc547ee7🌇
Tree-SHA512: 9de30792d7a5849cae77747aa978e70390b66ee9d082779a56088a024f82e725b0af050e6603aece0ac8229f6d73bc471ba97b4ab69dc7eddf419f5f56ae89a5
81d4a2b14f refactor: Move feerate comparison invariant outside of the loop (yancy)
365aca4045 refactor: Simplify feerate comparison statement (yancy)
Pull request description:
This is a small nit, however I think it's more understandable to write:
`utxo_pool.at(0).fee > utxo_pool.at(0).long_term_fee`
vs
`(utxo_pool.at(0).fee - utxo_pool.at(0).long_term_fee) > 0`
ACKs for top commit:
Xekyo:
ACK 81d4a2b14f
achow101:
ACK 81d4a2b14f
aureleoules:
ACK 81d4a2b14f
Tree-SHA512: 3e89377989c36716b53114fe40178261671dde5688075fab1c21ec173ac310f8c84ed6af90354d7c329176cb7262dfcaa7191fd19847d3b7147a9a10c3e31176
As no process should be able to trigger this error
using the regular transaction creation process, throw
a runtime_error if happens to tell users/devs to
report the bug if happens.
First step towards decoupling the pre-selected-inputs fetching functionality
from `SelectCoins`. Which, will let us not waste resources calculating the
available coins if one of the pre-set inputs has an error.
(right now, if one of the pre-set inputs is invalid, we first walk through
the entire wallet txes map just to end up failing right after it finish)
SelectionResult::m_target should be equal to actual selection target.
Selection target is the sum of all recipient amounts plus non input fees.
So we need to remove change_fee from the m_target. It's safe because change
target is always greater than the change fee, so we can always cover fees
if change output is created.
This reverts commit 9b5950db86.
Waste can be negative. At feerates lower than long_term_feerate this
means that a waste of 0 may be a suboptimal solution and this causes the
search to exit prematurely.
Only when the feerate is equal to the long_term_feerate would achieving
a waste of 0 indicate that we have achieved an optimal solution,
because it would mean that the excess is 0. It seems unlikely
that this would ever occur outside of test cases, and even then we
should prefer solutions with more inputs over solutions with fewer
according to previous decisions—but solutions with more inputs are found
later in the branch exploration.
The "optimization" described in #18257 and implemented in #18262 is
therefore a premature exit on a suboptimal solution and should be reverted.
Messages with level `WARN` or higher should be logged even when
the category is not provided with `-debug=`, to make sure important
warnings are not lost.
Previously in COutput, effective_value was initialized as the absolute
value of the txout, and fee as 0. effective_value along with fee were
calculated outside of the COutput constructor and set after the
object had been initialized. These changes will allow either the fee
or the feerate to be passed in a COutput constructor. If either are
provided, fee and effective_value are calculated and set in the
constructor. As a result, AvailableCoins also needs to be passed the
feerate when utxos are being spent. When balance is calculated or the
coins are being listed and feerate is neither available nor required,
AvailableCoinsListUnspent is used instead, which runs AvailableCoins
while providing the default value for feerate. Unit tests for the
calculation of effective value have also been added.
049003fe68 coinselection: Remove COutput operators == and != (Andrew Chow)
f6c39c6adb coinselection: Remove CInputCoin (Andrew Chow)
70f31f1a81 coinselection: Use COutput instead of CInputCoin (Andrew Chow)
14fbb57b79 coinselection: Add effective value and fees to COutput (Andrew Chow)
f0821230b8 moveonly: move COutput to coinselection.h (Andrew Chow)
42e974e15c wallet: Remove CWallet and CWalletTx from COutput's constructor (Andrew Chow)
14d04d5ad1 wallet: Replace CWalletTx in COutput with COutPoint and CTxOut (Andrew Chow)
0ba4d1916e wallet: Provide input bytes to COutput (Andrew Chow)
d51f27d3bb wallet: Store whether a COutput is from the wallet (Andrew Chow)
b799814bbd wallet: Store tx time in COutput (Andrew Chow)
46022953ee wallet: Remove use_max_sig default value (Andrew Chow)
10379f007f scripted-diff: Rename COutput member variables (Andrew Chow)
c7c64db41e wallet: cleanup COutput constructor (Andrew Chow)
Pull request description:
While working on coin selection code, it occurred to me that `CInputCoin` is really a subset of `COutput` and the conversion of a `COutput` to a `CInputCoin` does not appear to be all that useful. So this PR adds fields that are present in `CInputCoin` to `COutput` and replaces the usage of `CInputCoin` with `COutput`.
`COutput` is also moved to coinselection.h. As part of this move, the usage of `CWalletTx` is removed from `COutput`. It is instead replaced by storing a `COutPoint` and the `CTxOut` rather than the entire `CWalletTx` as coin selection does not really need the full `CWalletTx`. The `CWalletTx` was only used for figuring out whether the transaction containing the output was from the current wallet, and for the transaction's time. These are now parameters to `COutput`'s constructor.
ACKs for top commit:
ryanofsky:
Code review ACK 049003fe68, just adding comments and removing == operators since last review
w0xlt:
reACK 049003f
Xekyo:
reACK 049003fe68
Tree-SHA512: 048b4cd620a0415e1d9fe8597257ee4bc64656566e1d28a9bdd147d6d72dc87c3f34a3339fa9ab6acf42c388df7901fc4ee900ccaabc3de790ffad162b544c15
fa7deaa046 wallet: Pass FastRandomContext& to coin selection (MarcoFalke)
77773b061c wallet: Pass FastRandomContext& to DiscourageFeeSniping (MarcoFalke)
Pull request description:
Passing around a single randomness context shouldn't come with any downsides, but documents better where randomness is used and allows the unit test to be deterministic, if they wish to be so.
ACKs for top commit:
achow101:
ACK fa7deaa046
promag:
Code review ACK fa7deaa046.
glozow:
light code review ACK fa7deaa046
Tree-SHA512: c16287708cc82ce58311710595d0127af42fb156c93fbcaa5bde634ce323d325f4d8c99a74af24423ab22b5ad58163dd771e8b1a0e7d6bff39c9fb2a1cb21bc7
9d2005285c doc: Revise comments and whitespace to clarify (Ben Woosley)
def43a4d88 refactor: Rename i to curr_try in SelectCoinsBnB (Ben Woosley)
1dd0923677 refactor: Track BnB selection by index (Ben Woosley)
Pull request description:
This is prompted by #13167 and presented as a friendly alternative to it.
IMO you can improve code readability and performance by about 20% by tracking the selected utxos by index, rather than by position. This reduces the storage access complexity from roughly O(utxo_size) to O(selection_size).
On my machine (median of 5 trials):
```
BnBExhaustion, 5, 650, 2.2564, 0.000672999, 0.000711565, 0.000693112 - master
BnBExhaustion, 5, 650, 1.76232, 0.000528563, 0.000568806, 0.000539147 - this PR
```
ACKs for top commit:
achow101:
reACK 9d2005285c
glozow:
code review ACK 9d2005285c
Xekyo:
reACK 9d2005285c
Tree-SHA512: 453ea11ad58c48928dc76956e3e98916f6924e95510eb02fe89a899ff102fe9cc08a04d557f381ad0218a210275e5383101d971c1ffad38b06b1c57d81144315
This is a performance optimization - rather than track all visited values
in a bool vector, track the selected index in a vector. This results in a
complexity reduction of O(utxo_size) to O(selection_size).
These two implementations of waste calculation should never deviate.
Still keep the SelectCoinsBnB internal calculation because incremental
calculate-as-you-go is much more performant than calling
GetSelectionWaste() over and over again.
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.
