Followup to commit "MOVEONLY: CWallet transaction code out of
wallet.cpp/.h" that detaches and renames some CWalletTx methods, making
into them into standalone functions or CWallet methods instead.
There are no changes in behavior and no code changes that aren't purely
mechanical. It just gives spend and receive functions more consistent
names and removes the circular dependencies added by the earlier
MOVEONLY commit.
There are also no comment or documentation changes. Removed comments
from transaction.h are just migrated to spend.h, receive.h, and
wallet.h.
86beee0579 Use waste metric for deciding which selection to use (Andrew Chow)
b3df0caf7c tests: Test GetSelectionWaste (Andrew Chow)
4f5ad43b1e Add waste metric calculation function (Andrew Chow)
935b3ddf72 scripted-diff: tests: Use KnapsackSolver directly (Andrew Chow)
6a023a6f90 tests: Add KnapsackGroupOutputs helper function (Andrew Chow)
d5069fc1aa tests: Use SelectCoinsBnB directly instead of AttemptSelection (Andrew Chow)
54de7b4746 Allow the long term feerate to be configured, default of 10 sat/vb (Andrew Chow)
Pull request description:
Branch and Bound introduced a metric that we call waste. This metric is used as part of bounding the search tree, but it can be generalized to all coin selection solutions, including those with change. As such, this PR introduces the waste metric at a higher level so that we can run both of our coin selection algorithms (BnB and KnapsackSolver) and choose the one which has the least waste. In the event that both find a solution with the same change, we choose the one that spends more inputs.
Also this PR sets the long term feerate to 10 sat/vb rather than using the 1008 block estimate. This allows the long term feerate to be the feerate that we switch between consolidating and optimizing for fees. This also removes a bug where the long term feerate would incorrectly be set to the fallback fee. While this doesn't matter prior to this PR, it does have an effect following this. The long term feerate can be configured by the user through a new `-consolidatefeerate` option.
ACKs for top commit:
Xekyo:
reACK 86beee0 via git range-diff fe47558...86beee0
meshcollider:
re-utACK 86beee0579
Tree-SHA512: 54b154b346538eca68ae2a3b83a033b495c1605c14f842bfc43ded2256b110983ce674c647fe753cf0305b1b178403d8d60d6d4203c7a712bec784be52e90d42
In order to change the KnapsackSolver tests to call KnapsackSolver, we
need KnapsackGroupOutputs to create the OutputGroups filtered with the
filter criteria.
acb7aad27e Fix build with Boost 1.77.0 (Rafael Sadowski)
Pull request description:
BOOST_FILESYSTEM_C_STR changed to accept the path as an argument.
ACKs for top commit:
hebasto:
ACK acb7aad27e
benthecarman:
ACK acb7aad27e
fanquake:
ACK acb7aad27e - tested the fix with Boost 1.77.0 and 1.71.0.
Tree-SHA512: c25fcb56971ee7a448cfb074f8a13696b32c16c63f81076f8a76911f93aa849c8f3637555b0b4215fa0d8b958641d7e4e60d10e103b833545cbc6b1f4009b526
92993aa5cf Change SignTransaction's input_errors to use bilingual_str (Andrew Chow)
171366e89b Use bilingual_str for address fetching functions (Andrew Chow)
9571c69b51 Add bilingual_str::clear() (Andrew Chow)
Pull request description:
In a couple of places in the wallet, errors are `std::string`. In order for these errors to be translated, change them to use `bilingual_str`.
ACKs for top commit:
hebasto:
re-ACK 92993aa5cf, only rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/22337#pullrequestreview-694542729) review, verified with
klementtan:
Code review ACK 92993aa5cf
meshcollider:
Code review ACK 92993aa5cf
Tree-SHA512: 5400e419dd87db8c49b67ed0964de2d44b58010a566ca246f2f0760ed9ef6a9b6f6df7a6adcb211b315b74c727bfe8c7d07eb5690b5922fa5828ceef4c83461f
fe6dc76b7c wallet test: Add test for subtract fee from recipient behavior (Russell Yanofsky)
2565478c81 wallet test refactor: add CreateSyncedWallet function (Russell Yanofsky)
Pull request description:
This adds test coverage for wallet subtract from recipient behavior without changing it. Behavior seems to have changed recently in a minor way in #17331 without being noticed.
ACKs for top commit:
achow101:
ACK fe6dc76b7c
glozow:
ACK fe6dc76b7c
promag:
Code review ACK fe6dc76b7c.
Tree-SHA512: e00c5dfe467e4ccef5edb0dd4fff6c53f35a37828a4327bea2e166751e5ef971d519ffca7b8f735b12912bb4a547980626356bc1855981005aed1a6c2a57be0b
Behavior might have recently changed in #17331 (it is not clear) but not
noticed because there is no test coverage.
This adds test coverage for current subtract from recipient behavior
without changing it.
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
No change in behavior. This just moves some code from the ListCoins test
setup to a reusable util function, so it can be reused in a new test in
the next commit.
96c2c9520e scripted-diff: Rename SelectCoinsMinConf to AttemptSelection (Andrew Chow)
b583f73354 Move vin filling to before final fee setting (Andrew Chow)
d39cac0547 Set m_subtract_fee_outputs during recipients vector loop (Andrew Chow)
364e0698a5 Move variable initializations to where they are used (Andrew Chow)
32ab430651 Move recipients vector checks to beginning of CreateTransaction (Andrew Chow)
cd1d6d3324 Rename nSubtractFeeFromAmount in CreateTransaction (Andrew Chow)
dac21c793f Rename nValue and nValueToSelect (Andrew Chow)
d2aee3bbc7 Remove extraneous scope in CreateTransactionInternal (Andrew Chow)
b2995963b5 Move cs_wallet lock in CreateTransactionInternal to top of function (Andrew Chow)
Pull request description:
#17331 did some refactors and cleanup of `CreateTransactionInternal` to make it easier to understand, however it is still a bit convoluted even though it doesn't have to be. This PR does additional cleanup and refactoring to `CreateTransactionInternal` so that it is easier to understand. Some unnecessary code was removed, some variables moved around to where they matter, and several indents removed.
ACKs for top commit:
glozow:
reACK 96c2c95
ryanofsky:
Code review ACK 96c2c9520e also acked previously (was reverted).
meshcollider:
re-utACK 96c2c9520e
Tree-SHA512: 3dba67ed436968a07bfd82d435d566ad74e116c6e50ac9baed7144a46ad5c0f630b1ba59d91e8e8972ac2af559d7c0576f0560f09684d2ab20fad6689902866f
f5ba424cd4 wallet: Add IsAddressUsed / SetAddressUsed methods (Russell Yanofsky)
62252c95e5 interfaces: Stop exposing wallet destdata to gui (Russell Yanofsky)
985430d9b2 test: Add gui test for wallet receive requests (Russell Yanofsky)
Pull request description:
Stop giving GUI access to destdata rows in database. Replace with narrow API just for saving and reading receive request information.
This simplifies code and should prevent the GUI from interfering with other destdata like address-used status. It also adds some more GUI test coverage.
There are no changes in behavior.
ACKs for top commit:
jarolrod:
tACK f5ba424cd4
laanwj:
Code review ACK f5ba424cd4
Tree-SHA512: 5423df4786e537a59013cb5bfb9e1bc29a7ca4b8835360c00cc2165a59f925fdc355907a4ceb8bca0285bb4946ba235bffa7645537a951ad03fd3b4cee17b6b0
SelectCoinsMinConf is a bit of a misnomer now since it really just does
all of the coin selection given some parameters. So rename this to
something less annoying to say and makes a bit more sense.
-BEGIN VERIFY SCRIPT-
sed -i 's/SelectCoinsMinConf/AttemptSelection/g' $(git grep -l SelectCoinsMinConf ./src)
-END VERIFY SCRIPT-
51a3ac242c Have OutputGroup determine the value to use (Andrew Chow)
6d6d278475 Change SelectCoins_test to actually test SelectCoins (Andrew Chow)
9d3bd74ab4 Remove CreateTransaction while loop and some related variables (Andrew Chow)
6f0d5189af Remove use_bnb and bnb_used (Andrew Chow)
de26eb0e1f Do both BnB and Knapsack coin selection in SelectCoinsMinConf (Andrew Chow)
01dc8ebda5 Have KnapsackSolver actually use effective values (Andrew Chow)
bf26e018de Roll static tx fees into nValueToSelect instead of having it be separate (Andrew Chow)
cc3f14b27c Move output reductions for fee to after coin selection (Andrew Chow)
d97d25d950 Make cost_of_change part of CoinSelectionParams (Andrew Chow)
af5867c896 Move some calculations to common code in SelectCoinsMinConf (Andrew Chow)
1bf4a62cb6 scripted-diff: rename some variables (Andrew Chow)
Pull request description:
Changes `KnapsackSolver` to use effective values instead of just the nominal txout value. Since fees are taken into account during the selection itself, we finally get rid of the `CreateTransaction` loop as well as a few other things that only were only necessary because of that loop.
This should not change coin selection behavior at all (except maybe remove weird edge cases that were caused by the loop). In order to keep behavior the same, `KnapsackSolver` will select outputs with a negative effective value (as it did before).
ACKs for top commit:
ryanofsky:
Code review ACK 51a3ac242c. Looks good to go!
instagibbs:
review ACK 51a3ac242c
meshcollider:
re-light-utACK 51a3ac242c
Tree-SHA512: 372c27e00edcd5dbf85177421ba88f20bfdaf1791b6e3dc022c44876ecc379403e2375ed69e71c512c49e6af87641001ff385c4b25ab93684b3a08a53bf3824e
This was originally modified to use SelectCoinsMinConf in order to test
both BnB and Knapsack at the same time. But since SelectCoins does both
now, this is no longer necessary and we can revert back to actually
testing SelectCoins.
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.
41f891da50 tests: Skip SQLite fsyncs while testing (Andrew Chow)
Pull request description:
Since we want tests to run quickly, and since tests do a lot more db operations than expected we expect to see in actual usage, we disable sqlite's syncing behavior to make db operations run much faster. This syncing behavior is necessary for normal operation as it helps guarantee that data won't become lost or corrupted, but in tests, we don't care about that.
Fixes#21628
ACKs for top commit:
vasild:
ACK 41f891da50
Tree-SHA512: f36f969a182c622691ae5113573a3250e8d367437e83a1a9d3d2b55dd3a9cdf3c6474169a7bd271007bb9ce47f585aa7a6aeae6eebbaeb02d79409b02f47fd8b
Since we want tests to run quickly, and since tests do a lot more db
operations than expected we expect to see in actual usage, we disable
sqlite's syncing behavior to make db operations run much faster. This
syncing behavior is necessary for normal operation as it helps guarantee
that data won't become lost or corrupted, but in tests, we don't care
about that.
ebc4ab721b refactor: post Optional<> removal cleanups (fanquake)
57e980d13c scripted-diff: remove Optional & nullopt (fanquake)
Pull request description:
Same rationale & motivation as #21404, which turned out to be quite low in the number of potential conflicts. Lets see what the bot has to say here.
ACKs for top commit:
practicalswift:
cr ACK ebc4ab721b: patch looks correct
jnewbery:
utACK ebc4ab721b
laanwj:
Code review ACK ebc4ab721b
Tree-SHA512: 550fbeef09b9d35ddefaa805d1755c18c8fd499c4b0f77ebfece8c20296a7abd1cf6c699e2261f92fe3552deeb7555ec2a2287ffe3ab9e98bb9f8612a4d43be3
f9cd2bfbcc Rename CoinSelectionParams::effective_fee to m_effective_feerate (Andrew Chow)
bdd0c2934b wallet: Move discard feerate fetching to CreateTransaction (Andrew Chow)
448d04b931 wallet: Move long term feerate setting to CreateTransaction (Andrew Chow)
e2f429e6bb wallet: Replace nFeeRateNeeded with effective_fee (Andrew Chow)
1a6a0b0dfb wallet: Use existing feerate instead of getting a new one (Andrew Chow)
Pull request description:
During coin selection, there are various places where we need to have a feerate. We need the feerate for the transaction itself, the discard fee rate, and long term feerate. Fetching these each time we need them can lead to a race condition where two feerates that should be the same are actually different. One particular instance where this can happen is during the loop in `CreateTransactionInternal`. After inputs are chosen, the expected transaction fee is calculated using a newly fetched feerate. If `pick_new_inputs == false`, the loop will go again with the assumption that the fee for the transaction remains the same. However because the feerate is fetched again, it is possible that it actually isn't and this causes coin selection to fail.
Instead of fetching the feerate each time it is needed, we fetch them all at once at the top of `CreateTransactionInternal`, store them in `CoinSelectionParams`, and use them where needed.
While some of these fee rates probably don't need this caching, I've done it for consistency and the guarantee that they remain the same.
Fixes#19229
ACKs for top commit:
glozow:
reACK f9cd2bfbcc
fjahr:
Code review re-ACK f9cd2bfbcc
Xekyo:
tACK f9cd2bfbcc
meshcollider:
Code review + test run ACK f9cd2bfbcc
Tree-SHA512: be83ff64ba473c3cdd3469c812e214659b6e2a9584c22ed2b1595618fce0d4b35d0901e61068cd1069fc1a8fb911db01dd7312d05c3b8cbafbe2504ab7a3e863
Instead of fetching the discard feerate for each SelectCoinsMinConf
iteration, fetch and cache it once during CreateTransaction so that it
is shared for each SelectCoinsMinConf through
coin_selection_params.m_discard_feerate.
Does not change behavior.
Instead of setting the long term feerate for each SelectCoinsMinConf
iteration, set it once during CreateTransaction and let it be shared
with each SelectCoinsMinConf through
coin_selection_params.m_long_term_feerate.
Does not change behavior.
This simplifies code and adds a less cumbersome interface for accessing
address used information than CWallet AddDestData / EraseDestData /
GetDestData methods.
There is no change in behavior. Lower-level walletdb DestData methods
are also still available and not affected by this change. If there is
interest in consolidating destdata logic more and making it internal to
walletdb, #18608 could be considered as a followup.
Stop giving GUI access to destdata rows in database. Replace with narrow
API just for saving and reading receive request information.
This simplifies code and should prevent the GUI from interfering with
other destdata like address-used status.
Note: No user-visible behavior is changing in this commit. New
CWallet::SetAddressReceiveRequest() implementation avoids a bug in
CWallet::AddDestData() where a modification would leave the previous
value in memory while writing the new value to disk. But it doesn't
matter because the GUI doesn't currently expose the ability to modify
receive requests, only to add and erase them.
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: 5d4597666d
fjahr:
Code review ACK 5d4597666d
meshcollider:
Light utACK 5d4597666d
Tree-SHA512: 35965b6d49a87f4ebb366ec4f00aafaaf78e9282481ae2c9682b515a3a9f2cbcd3cd6e202fee29489d48fe7f3a7cede4270796f5e72bbaff76da647138fb3059
Leaving this value set interfered with the CreateWallet test if it happened to execute later in the test ordering. Specifically it would cause CreateWallet test to write data to the current directory instead of temporary test directory.
faa8f68943 Replace boost::variant with std::variant (MarcoFalke)
Pull request description:
Now that we can use std::variant from the vanilla standard library, drop the third-party boost variant dependency
ACKs for top commit:
fjahr:
Code review ACK faa8f68943
fanquake:
ACK faa8f68943
Tree-SHA512: 6e3aecd33b00c2e31a763f999247944d5b2ce5e3018f1965c516c1000cd08ff6703a8d50fb0be64883153da2925ae72986b8a6b96586db74057bd05d6f4986e6
