11115169a1 ci: Build fuzz with libsqlite3-dev (MarcoFalke)
fa7c6efca6 fuzz: Add wallet fuzz test (MarcoFalke)
fa59d2ce5b refactor: Use local args instead of global gArgs in CWallet::Create (MarcoFalke)
fadb44606f build: Inline FUZZ_SUITE_LDFLAGS_COMMON (MarcoFalke)
Pull request description:
Initial sketch to fuzz descriptor wallets. Can be improved in the future.
ACKs for top commit:
mjdietzx:
Code review ACK 1111516
Tree-SHA512: b1d2f24504d1ed5f3c6a031210f04c27c13d4e15576c4acbf50ded37ac45f7b7a5c7553e91d81d4a06e9ea73b3d745a552218d3ef3b2932fa5325a8331b0d3fd
54011e7aa2 refactor: use CWallet const shared pointers when possible (Karl-Johan Alm)
96461989a2 refactor: const shared_ptrs (Karl-Johan Alm)
Pull request description:
```C++
const std::shared_ptr<CWallet> wallet = x;
```
means we can not do `wallet = y`, but we can totally do `wallet->DestructiveOperation()`, contrary to what that line looks like.
This PR
* introduces a new convention: always use const shared pointers to `CWallet`s (even when we mutate the pointed-to thing)
* uses `const shared_ptr<const CWallet>` everywhere where wallets are not modified
In the future, this should preferably apply to all shared pointers, not limited to just `CWallet`s.
Both of these serve the same purpose: to dispell the misconception that `const shared_ptr<X>` immutates `X`. It doesn't, and it's dangerous to leave this misconception as is, for obvious reasons.
ACKs for top commit:
theStack:
re-ACK 54011e7aa2
Tree-SHA512: 3bf4062fc821751be30770c6b4ead10a016847970f155a0a5156f304347d221b9830840030c2fbfba8cd1e282f4eda45f5b4107fe6df8138afdcb6c2e95a2836
Introduce convention to use const shared pointers everywhere, unless the shared pointer is modified at some point, which it very rarely is.
We want this convention, as it helps alleviate the misconception that a const shared pointer somehow results in a pointer to an immutable object, which is false.
Instead of using AddToWallet so that making a COutput will work,
directly add the transaction into wallet.mapWallet. This bypasses many
checks that AddToWallet will do which are pointless and just slow down
this test.
For the generic wallet tests, make DescriptorScriptPubKeyMans. There are
still some wallet tests that test legacy wallet things. Those remain
unchanged.
The subtract fee from outputs assumes that the leftover input amount
will be dropped to fees. However this only happens if that amount is
less than the cost of change. In the event that it is higher than the
cost of change, the leftover amount will actually become a change
output. To avoid this scenario, force a change type which has a high
cost of change.
To avoid issues with test data leaking across tests cases, the global
vCoins and testWallet are removed from coinselector_tests and all of the
relevant functions reworked to not need them.
There is no change in behavior. This just helps prepare for the
transition from boost::filesystem to std::filesystem by avoiding calls
to methods which will be unsafe after the transaction to std::filesystem
to due lack of a boost::filesystem::path::imbue equivalent and inability
to set a predictable locale.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: Kiminuo <kiminuo@protonmail.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
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
There are two more cases where waste can be 0, when:
- (Fee - LTF) == -Change Cost
- (Fee - LTF) == -Excess
Adding these two conditions explicitly in the unit test will help
pin the behavior, also demonstrate waste calculation scenarios to new
readers.
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
5021810650 Make CanFlushToDisk a const member function (practicalswift)
281cf99554 Do not run functions with necessary side-effects in assert() (practicalswift)
Pull request description:
Do not run functions with necessary side-effects in `assert()`.
ACKs for top commit:
laanwj:
Code review ACK 5021810650
sipa:
utACK 5021810650
theStack:
Code Review ACK 5021810650🟢
Tree-SHA512: 38b7faccc2f16a499f9b7b1b962b49eb58580b2a2bbf63ea49dcc418a5ecc8f21a0972fa953f66db9509c7239af67cfa2f9266423fd220963d091034d7332b96
f3d870fc22 wallet: List all wallets in non-SQLite or non-BDB builds (Russell Yanofsky)
d70dc89e78 refactor: Consolidate redundant wallet database path and exists functions (Russell Yanofsky)
6a7a63644c refactor: Drop call to GetWalletEnv in wallet salvage code (Russell Yanofsky)
6ee9cbdd18 refactor: Replace ListWalletDir() function with ListDatabases() (Russell Yanofsky)
5aaeb6cf87 MOVEONLY: Move IsBDBFile, IsSQLiteFile, and ListWalletDir (Russell Yanofsky)
Pull request description:
This PR does not change behavior when bitcoin is built normally with both the SQLite and BDB libraries. It just makes non-SQLite and non-BDB builds more similar to the normal build. Specifically:
- It makes wallet directory lists always include all wallets so wallets don't appear missing depending on the build.
- It now triggers specific "Build does not support SQLite database format" and "Build does not support Berkeley DB database format" errors if a wallet can't be loaded instead of the more ambiguous and scary "Data is not in recognized format" error.
Both changes are implemented in the last commit. The previous commits are just refactoring cleanups that make the last commit possible and consolidate and reduce code.
ACKs for top commit:
achow101:
ACK f3d870fc22
promag:
Tested ACK f3d870fc22. Tested a --without-sqlite build with sqlite wallets.
Tree-SHA512: 029ad21559dbc338b5f351d05113c51bc25bce830f4f4e18bcd82287bc528275347a60249da65b91d252632aeb70b25d057bd59c704bfcaafb9f790bc5b59762
No change in behavior. Just remove a little bit of code, reduce macro usage,
remove duplicative functions, and make BDB and SQLite implementations more
consistent with each other.
7486e2771e Tests: Unit test related to WalletDB ReadKeyValue (Bushstar)
32def8d1c2 Catch ios_base::failure specifically (Peter Bushnell)
Pull request description:
In https://github.com/bitcoin/bitcoin/pull/2950 a hash of the pubkey and private was added to speed up key import, this was made backwards compatible by reading the hash in a try block with an ellipses catch all in case the hash was not present.
CDataStream::read() specifically throws std::ios_base::failure, backwards compatibility expects only that error to be thrown, if something else gets thrown we should not be catching it. The change in this commit is to catch that exception only. If any other exception is thrown other than std::ios_base::failure it will be caught by the wider try block and an error written to the log and/or console.
CDataStream::read() throwing std::ios_base::failure.
2c364fde42/src/streams.h (L191)
Wider catch statements that pick up all others exceptions other than ios_base::failure.
2c364fde42/src/wallet/walletdb.cpp (L425)2c364fde42/src/wallet/walletdb.cpp (L430)
ACKs for top commit:
laanwj:
Code review ACK 7486e2771e
Tree-SHA512: 5364bf935af8ec603bf5b8fef8c23b5cdaa4fe3506090cff988413221f2eaa99f7a91929afb42a35f8881ce2328744a0d32052da51ca0a5b2e65b6809e97f604
72a1d5c6f3 validation: Remove review-only comments + assertions (Carl Dong)
3756853b15 docs: Move FindFilesToPrune{,Manual} doxygen comment (Carl Dong)
485899a93c style: Make FindFilesToPrune{,Manual} match style guide (Carl Dong)
3f5b5f3f6d validation: Move FindFilesToPrune{,Manual} to BlockManager (Carl Dong)
f8d4975ab3 validation: Move PruneOneBlockFile to BlockManager (Carl Dong)
74f73c783d validation: Pass in chainman to UnloadBlockIndex (Carl Dong)
4668ded6d6 validation: Move ~CMainCleanup logic to ~BlockManager (Carl Dong)
Pull request description:
This PR paves the way for de-globalizing `g_chainman` entirely by removing the usage of `g_chainman` in the following functions/methods:
- `~CMainCleanup`
- `CChainState::FlushStateToDisk`
- `UnloadBlockIndex`
The remaining direct uses of `g_chainman` are as follows:
1. In initialization codepaths:
- `AppTests`
- `AppInitMain`
- `TestingSetup::TestingSetup`
2. `::ChainstateActive`
3. `LookupBlockIndex`
- Note: `LookupBlockIndex` is used extensively throughout the codebase and require a much larger set of changes, therefore I've left it out of this initial PR
ACKs for top commit:
MarcoFalke:
re-ACK 72a1d5c6f3👚
jnewbery:
utACK 72a1d5c6f3
Tree-SHA512: 944a4fa8405eecf39706ff944375d6824373aaeea849d11473f08181eff26b12f70043a8348a5b08e6e9021b243b481842fbdfbc7c3140ca795fce3688b7f5c3
[META] This is a pure refactor commit.
Move PruneBlockFile to BlockManager because:
1. PruneOneBlockFile only acts on BlockManager
2. Eliminates the need for callers (FindFilesToPrune{,Manual}) to have a
reference to the larger ChainstateManager, just a reference to
BlockManager is enough. See following commits.
No longer create a default wallet. The default wallet will still be
loaded if it exists and not other wallets were specified (anywhere,
including settings.json, bitcoin.conf, and command line).
Tests are updated to be started with -wallet= if they need the default
wallet.
Added test to wallet_startup.py testing that no default wallet is
created and that it is loaded if it exists and no other wallets were
specified.
This removes a source of complexity and indirection that makes it harder to
understand path checking code. Path checks will be simplified in upcoming
commits.
There is no change in behavior in this commit other than a slightly more
descriptive error message in `loadwallet` if the default "" wallet can't be
found. (The error message is improved more in upcoming commit "wallet: Remove
path checking code from loadwallet RPC".)
Enable the GUI to also use the load_on_startup feature.
Wallets loaded in the GUI always have load_on_startup=true.
When they are unloaded from the GUI, load_on_startup=false.
To facilitate this change, UpdateWalletSetting is moved into the wallet
module and called from within LoadWallet, RemoveWallet, and
Createwallet. This change does not actually touch the GUI code but
rather the wallet functions that are shared between the GUI and RPC.
fa3d9ce325 rpc: Assert that RPCArg names are equal to CRPCCommand ones (rpcdump) (MarcoFalke)
fa32c1d5ec rpc: Assert that RPCArg names are equal to CRPCCommand ones (zmq) (MarcoFalke)
faaa46dc20 rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining) (MarcoFalke)
fa93bc14c7 rpc: Remove unused return type from appendCommand (MarcoFalke)
Pull request description:
This is split out from #18531 to just touch the RPC methods in misc. Description from the main pr:
### Motivation
RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here.
### Changes
The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`.
### Future work
> Here or follow up, makes sense to also assert type of returned UniValue?
Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including:
* Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
* Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table
* Auto-formatting and sanity checking the RPCExamples with RPCMan
* Checking passed-in json in self-check. Removing redundant checks
* Checking returned json against documentation to avoid regressions or false documentation
* Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static
### Bugs found
* The assert identified issue #18607
* The changes itself fixed bug #19250
ACKs for top commit:
fjahr:
tested ACK fa3d9ce325
promag:
Code review ACK fa3d9ce325.
Tree-SHA512: 068ade4b55cc195868d53b7f9a27151d45b440857bb069e261a49d102a49a38fdba5d68868516a1d66a54a73ba34681362f934ded7349e894042bde873b75719
Add WalletClient interface so node interface is cleaner and don't need
wallet-specific methods.
The new NodeContext::wallet_client pointer will also be needed to eliminate
global wallet variables like ::vpwallets, because createWallet(), loadWallet(),
getWallets(), etc methods called by the GUI need a way to get a reference to
the list of open wallets if it is no longer a global variable.
Also tweaks splash screen registration for load wallet events to be delayed
until after wallet client is created.
1554b54d47 Static asserts for consistency of fee defaults. (Daniel Kraft)
Pull request description:
This adds `static_assert`'s that ensure that the default values given for fee levels in the wallet (minimum fee and incremental feerate increase) are at least as high as the corresponding levels configured in the core node policy. Since the core policy values are enforced by the network, it makes sense for the wallet to be conservative and above (or at least not below) this.
ACKs for top commit:
laanwj:
code review ACK 1554b54d47, these assumptions seem straightforward
Tree-SHA512: 50e5adf082f467062334377f82a3ee75bcfd436afc65bd0eb33c8d0549d6d90fd1f48c31f60cabe523eb59be9efa8ae0879e9e09cd51ca9c1bd466631ce03cf4
fa73493930 refactor: Use C++11 range-based for loop (MarcoFalke)
fa7b164d62 wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off (MarcoFalke)
faf8401c19 wallet: Pass unused args to StartWallets (MarcoFalke)
fa6c186436 gui tests: Limit life-time of dummy testing setup (MarcoFalke)
fa28a61897 test: Add smoke test to check that wallets are flushed by default (MarcoFalke)
Pull request description:
User-facing, this is a refactor. Internally, the scheduler does not have to call a mostly empty function every half a second.
ACKs for top commit:
jnewbery:
utACK fa73493930
meshcollider:
utACK fa73493930
ryanofsky:
Code review ACK fa73493930. Just rebased since last review
Tree-SHA512: 99e1fe1b2c22a3f4b19de3e566241d38693f4fd8d5a68ba1838d86740aa6c08e3325c11a072e30fd262a8861af4278bed52eb9374c85179b8f536477f528247c
