5f9c0b6360215636cfa62a70d3a70f1feb3977ab wallet: Remove -upgradewallet from dummywallet (MarcoFalke)
a314271f08215feba53ead27096ac7fda34acb3c test: Remove unused wallet.dat (MarcoFalke)
bf7635963c03203e7189ddaa56c6b086a0108cbf tests: Test specific upgradewallet scenarios and that upgrades work (Andrew Chow)
4b418a9decc3e855ee4b0bbf9e61121c8e9904e5 test: Add test_framework/bdb.py module for inspecting bdb files (Andrew Chow)
092fc434854f881330771a93a1280ac67b1d3549 tests: Add a sha256sum_file function to util (Andrew Chow)
0bd995aa19be65b0dd23df1df571c71428c2bc32 wallet: upgrade the CHDChain version number when upgrading to split hd (Andrew Chow)
8e32e1c41c995e832e643f605d35a7aa112837e6 wallet: remove nWalletMaxVersion (Andrew Chow)
bd7398cc6258c258e9f4411c50630ec4a552341b wallet: have ScriptPubKeyMan::Upgrade check against the new version (Andrew Chow)
5f720544f34dedf75b063b962845fa8eca604514 wallet: Add GetClosestWalletFeature function (Andrew Chow)
842ae3842df489f1b8d68e67a234788966218184 wallet: Add utility method for CanSupportFeature (Andrew Chow)
Pull request description:
This PR cleans up the wallet upgrade mechanism a bit, fixes some probably bugs, and adds more test cases.
The `nWalletMaxVersion` member variable has been removed as it made `CanSupportFeature` unintuitive and was causing a couple of bugs. The reason this was introduced originally was to allow a wallet upgrade to only occur when the new feature is first used. While this makes sense for the old `-upgradewallet` option, for an RPC, this does not quite make sense. It's more intuitive for an upgrade to occur if possible if the `upgradewallet` RPC is used as that's an explicit request to upgrade a particular wallet to a newer version. `nWalletMaxVersion` was only relevant for upgrades to `FEATURE_WALLETCRYPT` and `FEATURE_COMPRPUBKEY` both of which are incredibly old features. So for such wallets, the behavior of `upgradewallet` will be that the feature is enabled immediately without the wallet needing to be encrypted at that time (note that `FEATURE_WALLETCRYPT` indicates support for encryption, not that the wallet is encrypted) or for a new key to be generated.
`CanSupportFeature` would previously indicate whether we could upgrade to `nWalletMaxVersion` not just whether the current wallet version supported a feature. While this property was being used to determine whether we should upgrade to HD and HD chain split, it was also causing a few bugs. Determining whether we should upgrade to HD or HD chain split is resolved by passing into `ScriptPubKeyMan::Upgrade` the version we are upgrading to and checking against that. By removing `nWalletMaxVersion` we also fix a bug where you could upgrade to HD chain split without the pre-split keypool.
`nWalletMaxVersion` was also the version that was being reported by `getwalletinfo` which meant that the version reported was not always consistent across restarts as it depended on whether `upgradewallet` was used. Additionally to make the wallet versions consistent with actually supported versions, instead of just setting the wallet version to whatever is given to `upgradewallet`, we normalize the version number to the closest supported version number. For example, if given 150000, we would store and report 139900.
Another bug where CHDChain was not being upgraded to the version supporting HD chain split is also fixed by this PR.
Lastly several more tests have been added. Some refactoring to the test was made to make these tests easier. These tests check specific upgrading scenarios, such as from non-HD (version 60000) to HD to pre-split keypool. Although not specifically related to `upgradewallet`, `UpgradeKeyMetadata` is now being tested too.
Part of the new tests is checking that the wallet files are identical before and after failed upgrades. To facilitate this, a utility function `sha256sum_file` has been added. Another part of the tests is to examine the wallet file itself to ensure that the records in the wallet.dat file have been correctly modified. So a new `bdb.py` module has been added to deserialize the BDB db of the wallet.dat file. This format isn't explicitly documented anywhere, but the code and comments in BDB's source code in file `dbinc/db_page.h` describe it. This module just dumps all of the fields into a dict.
ACKs for top commit:
MarcoFalke:
approach ACK 5f9c0b6360
laanwj:
Code review ACK 5f9c0b6360215636cfa62a70d3a70f1feb3977ab
jonatack:
ACK 5f9c0b6360215636cfa62a70d3a70f1feb3977ab, approach seems fine, code review, only skimmed the test changes but they look well done, rebased on current master, debug built and verified the `wallet_upgradewallet.py` test runs green both before and after running `test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2`
Tree-SHA512: 7c4ebf420850d596a586cb6dd7f2ef39c6477847d12d105fcd362abb07f2a8aa4f7afc5bfd36cbc8b8c72fcdd1de8d2d3f16ad8e8ba736b6f4f31f133fe5feba
9c08f3332c12aa30c70aaf390c876cc5c1f90617 doc: Missing comments for signet parameters (kanon)
Pull request description:
We have such comment in chainparams.cpp. However in Signet the comments are missing.
In example...
- Mainnet
d67883d01e/src/chainparams.cpp (L83-L84)
- Testnet
d67883d01e/src/chainparams.cpp (L196-L197)
- Regtest
d67883d01e/src/chainparams.cpp (L392-L393)
ACKs for top commit:
theStack:
ACK 9c08f3332c12aa30c70aaf390c876cc5c1f90617
Tree-SHA512: d4e488cf01e50d6320282b29d776c11e6b3d423f9268226749f738a57a51f456b6bd48334d2d5a43afa782df65ea15525a0af1688003c1be6ef915c05650e147
89836a82eec63f93bbe6c3bd6a52be26e71ab54d style: minor improvements as a followup to #19845 (Vasil Dimov)
Pull request description:
Address suggestions:
https://github.com/bitcoin/bitcoin/pull/19845#discussion_r495486760https://github.com/bitcoin/bitcoin/pull/19845#discussion_r495488051https://github.com/bitcoin/bitcoin/pull/19845#discussion_r495730125
ACKs for top commit:
jonatack:
re-ACK 89836a8 change since previous review is replacing std::runtime_error with std::exception, built/ran unit tests with gcc debian 10.2.0-15, then broke a few v3 net_tests involving `BOOST_CHECK_EXCEPTION`, rebuilt, ran `src/test/test_bitcoin -t net_tests -l all` and checked the error reporting.
hebasto:
re-ACK 89836a82eec63f93bbe6c3bd6a52be26e71ab54d
theStack:
ACK 89836a82eec63f93bbe6c3bd6a52be26e71ab54d
Tree-SHA512: 36477fdccabe5a8ad91fbabb4655cc363a3a7ca237a98ae6dd4a9fae4a4113762040f864d4ca13a47d081f7d16e5bd487edbfb61ab50a37e4a0424e9bec30b24
440f8d3abe97b96f434dad5216d417a08fc10253 fix potential devision by 0 (Jonas Schnelli)
Pull request description:
#20344 removed the divide-by-zero sanitizer suppression in `wallet/wallet.cpp` but kept a potential devision by zero in `wallet.cpp`'s fee logging.
Detected here https://bitcoinbuilds.org/index.php?job=ffb7d59f-379f-4f27-a273-a5595b8c5f07
ACKs for top commit:
practicalswift:
ACK 440f8d3abe97b96f434dad5216d417a08fc10253
laanwj:
Code review ACK 440f8d3abe97b96f434dad5216d417a08fc10253
hebasto:
re-ACK 440f8d3abe97b96f434dad5216d417a08fc10253
Tree-SHA512: 9f7903d1e567497c5f972d39e9629c059151e705dbed0a6b88f7c6650c50ecf820f78e3e0f3e629c661d45a938c5d7659faae7c61e47ca8b3bdb029661bca55a
d04a17a7907c57f7b570e1b9743fd63489bdad68 fuzz: Use ConsumeRandomLengthBitVector(...) in src/test/fuzz/connman and src/test/fuzz/net (practicalswift)
e6bb9fde851422808f5d9870782c394f74a1f400 tests: Add fuzzing harness for CAddrMan (practicalswift)
Pull request description:
Add fuzzing harness for `CAddrMan`.
~~Fill some fuzzing coverage gaps for functions in `addrdb.h`, `merkleblock.h` and `outputtype.h`.~~
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
MarcoFalke:
review ACK d04a17a7907c57f7b570e1b9743fd63489bdad68
Tree-SHA512: a6b627e3a0cb51e3a8cb02ad0f19088fc0e965ca34ab110b68d5822d0ea7f473207ae312b49fb217cb6cf2f9f211d00bb69c83bac9f50d79c9ed1e157e85775d
38ada892ed0ed9aaa46b1791db12a371a3c0c419 addrman: ensure old versions don't parse peers.dat (Vasil Dimov)
Pull request description:
Even though the format of `peers.dat` was changed in a backwards
incompatible way, it is not guaranteed that old versions will fail to
parse it. There is a chance that old versions parse its contents as
garbage and use it.
Old versions expect the "key size" field to be 32 and fail the parsing
if it is not. Thus, we put something other than 32 in it. This will make
versions between 0.11.0 and 0.20.1 deterministically fail on the new
format. Versions prior to https://github.com/bitcoin/bitcoin/pull/5941
will still parse it as garbage.
Also, introduce a way to increment the `peers.dat` format in a way that
does not necessary make older versions refuse to read it.
ACKs for top commit:
jnewbery:
ACK 38ada892ed0ed9aaa46b1791db12a371a3c0c419
laanwj:
Code review ACK 38ada892ed0ed9aaa46b1791db12a371a3c0c419
MarcoFalke:
re-ACK 38ada892ed0ed9aaa46b1791db12a371a3c0c419 🥐
Tree-SHA512: 550bd660c5019dba0f9c334aca8a11c4a0463cfddf11efe7a4a5585ffb05549c82b95066fba5d073ae37893e0eccc158a7ffea9b33ea031d9be4a39e44f6face
241434200ec2067673d8522fee4f1228abfd8247 refactor: qt: Use vQueueNotifications.clear() (João Barbosa)
989e579d07bb5031639060b717f7a0be15d10e29 qt: Make transaction notification queue wallet specific (João Barbosa)
7b3b2303f44031c3545651858f697a495c3ea37a move-only: Define TransactionNotification before TransactionTablePriv (João Barbosa)
Pull request description:
Currently `vQueueNotifications` holds transactions of any wallet, but the queue is dispatched on a given wallet and it assumes notifications are of that wallet.
This means that some transactions can be missed if multiple wallets are loaded.
Fix this by having a queue for each wallet.
ACKs for top commit:
jonasschnelli:
utACK 241434200ec2067673d8522fee4f1228abfd8247
hebasto:
ACK 241434200ec2067673d8522fee4f1228abfd8247, I have reviewed the code and it looks OK, I agree it can be merged.
ryanofsky:
Code review ACK 241434200ec2067673d8522fee4f1228abfd8247. Only change is dropping one commit
Tree-SHA512: 61beac5a16ed659e3a25ad145dbceafcef963aaf8f9838355298949ec2324e2bd760f59353cd251d30cf0334d8dc1642a1f3821d8a9eec092533b581f6ce86db
79ef8324d4c85ed16a304e98805724b8a59022ac tests: Add fuzzing harness for CConnman (practicalswift)
Pull request description:
Add fuzzing harness for `CConnman`.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
MarcoFalke:
review ACK 79ef8324d4c85ed16a304e98805724b8a59022ac
Tree-SHA512: eb9ffae20e939b818f8b9def064544b9a8fcd127ca22d1a54af1afedf1d24143be42419f3a03d684be59a5ff07b29d8bfa34ef2aaf1d9f9f75c4c1aaa90a29a8
3c77b8009de9457c356c0bf4362d11bb99a17bb7 fuzz: Improve coverage for CPartialMerkleTree fuzzing harness (practicalswift)
Pull request description:
Improve coverage for `CPartialMerkleTree` fuzzing harness.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
MarcoFalke:
ACK 3c77b8009de9457c356c0bf4362d11bb99a17bb7
Tree-SHA512: a1fa0f7650a5ee5ff83f35e41b9faf6c34671fc304b9af00e5b83073f21d50bcbe91c2428fa64d05dc42a7c521bfd24031e307c7f4abf9ded469d69a55c5d64a
ee11a412a537f62aa46e8862678ce2069a2df5b7 Avoid signed integer overflow when loading a mempool.dat file with a malformed time field (practicalswift)
Pull request description:
Avoid signed integer overflow when loading a `mempool.dat` file with a malformed time field.
Avoid the following signed integer overflow:
```
$ xxd -p -r > mempool.dat-crash-1 <<EOF
0100000000000000000000000004000000000000000000000000ffffffff
ffffff7f00000000000000000000000000
EOF
$ cp mempool.dat-crash-1 ~/.bitcoin/regtest/mempool.dat
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" src/bitcoind -regtest
validation.cpp:5079:23: runtime error: signed integer overflow: 9223372036854775807 + 1209600 cannot be represented in type 'long'
#0 0x5618d335197f in LoadMempool(CTxMemPool&) src/validation.cpp:5079:23
#1 0x5618d3350df3 in CChainState::LoadMempool(ArgsManager const&) src/validation.cpp:4217:9
#2 0x5618d2b9345f in ThreadImport(ChainstateManager&, std::vector<boost::filesystem::path, std::allocator<boost::filesystem::path> >, ArgsManager const&) src/init.cpp:762:33
#3 0x5618d2b92162 in AppInitMain(util::Ref const&, NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_14::operator()() const src/init.cpp:1881:9
```
This PR was broken out from PR #20089. Hopefully this PR is trivial to review.
Fixes a subset of #19278.
ACKs for top commit:
MarcoFalke:
review ACK ee11a412a537f62aa46e8862678ce2069a2df5b7
Crypt-iQ:
crACK ee11a412a537f62aa46e8862678ce2069a2df5b7
Tree-SHA512: 227ab95cd7d22f62f3191693b455eacfa8e36534961bee12c622fc9090957cfb29992eabafa74d806a336e03385aa8f98b7ce734f04b0b400e33aa187d353337
24d2d3341d07509ad3f37bb6f130446ad20ac807 QA: wallet_multiwallet: Check that recursive symlink directory and wallet.dat loops are ignored (Luke Dashjr)
69f59af54d15ee9800d5df86bcdb0e962c71e7c3 Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks (Luke Dashjr)
Pull request description:
Previously, an exception would be thrown, which could kill the node in some circumstances.
Includes test changes to cause failure.
Review with `?w=1`
ACKs for top commit:
hebasto:
re-ACK 24d2d3341d07509ad3f37bb6f130446ad20ac807, rebased only since my [previous](https://github.com/bitcoin/bitcoin/pull/19502#pullrequestreview-520552944) review.
promag:
Tested ACK 24d2d3341d07509ad3f37bb6f130446ad20ac807, test change fails on master.
meshcollider:
utACK 24d2d3341d07509ad3f37bb6f130446ad20ac807
Tree-SHA512: f701f81b3aa3d3e15cee52ac9e7c31a73c0d8166e56bf077235294507cbcee099829fedc432a1c4b6d8780885f4e37897b44b980b08125771de3c849c000499e
5e146022daa4336de94447e5b8e5418296286927 wallet: fix scanning progress calculation for single block range (Sebastian Falbesoner)
Pull request description:
If the blockchain is rescanned for a single block (i.e. start and stop hashes are equal, and with that also the estimated start/stop verification progress values) the progress calculation could lead to a NaN value caused by a division by zero (0.0/0.0), resulting in an invalid JSON result for the `getwalletinfo` RPC. This PR fixes this behaviour by setting the progress to zero in that special case. Fixes#20297.
The behaviour can easily be reproduced by continuously running single block rescans in an endless loop, e.g. via
```bash
#!/bin/bash
while true
do
bitcoin-cli rescanblockchain $(bitcoin-cli getblockcount)
done
```
and at the same time perform some `getwalletinfo` RPCs.
On the master branch, this leads to frequent invalid responses (tested on mainchain):
```
$ bitcoin-cli getwalletinfo
error: couldn't parse reply from server
$ curl --user `cat ~/.bitcoin/.cookie` --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getwalletinfo", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
{"result":{"walletname":"","walletversion":169900,"format":"bdb","balance":0.00000000,"unconfirmed_balance":0.00000000,"immature_balance":0.00000000,"txcount":0,"keypoololdest":1603677276,"keypoolsize":1000,"hdseedid":"3196e33ecb47c7130e6ca60f2f895f9259860dca","keypoolsize_hd_internal":1000,"paytxfee":0.00000000,"private_keys_enabled":true,"avoid_reuse":false,"scanning":{"duration":0,"progress":},"descriptors":false},"error":null,"id":"curltest"}
```
(note that missing value for "progress" in the JSON result).
On the PR branch, the behaviour doesn't occur anymore.
ACKs for top commit:
MarcoFalke:
review ACK 5e146022daa4336de94447e5b8e5418296286927
promag:
Core review ACK 5e146022daa4336de94447e5b8e5418296286927.
Tree-SHA512: f0e6aad5a6cd08b36c5fe820fff0ef26663229b39169a4dbe757f3c795a41cf5c69c9dc90efe7515675ae1059307f8971123781a0514d10704123a6f28b125ab
Even though the format of `peers.dat` was changed in an incompatible
way (old software versions <0.21 cannot understand the new file format),
it is not guaranteed that old versions will fail to parse it. There is a
chance that old versions parse its contents as garbage and use it.
Old versions expect the "key size" field to be 32 and fail the parsing
if it is not. Thus, we put something other than 32 in it. This will make
versions between 0.11.0 and 0.20.1 deterministically fail on the new
format. Versions prior to https://github.com/bitcoin/bitcoin/pull/5941
(<0.11.0) will still parse it as garbage.
Also, introduce a way to increment the `peers.dat` format in a way that
does not necessary make older versions refuse to read it.
If the blockchain is rescanned for a single block (i.e. start and stop hashes
are equal, and with that also the estimated verification progress) the progress
calculation could lead to a NaN value caused by a division by zero, resulting in
an invalid JSON result for the getwalletinfo RPC. Fixed by setting the progress
to zero in that special case.
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
fa4234d877ea3193bfd0e18ff68dcb8fb84b47b5 test: Mock IBD in net_processing fuzzers (MarcoFalke)
Pull request description:
Without this the fuzzers fail to detect trivial crasher bugs, such as https://github.com/bitcoin/bitcoin/pull/20317#issuecomment-723047111
ACKs for top commit:
practicalswift:
Tested ACK fa4234d877ea3193bfd0e18ff68dcb8fb84b47b5
Tree-SHA512: ce5da5c0a604b7559805a98ffdde882b44ca4f91b003b493d6e1be230714ce4cccb11dbfc1fc175f9d8fc779551c0a4103ceb4b473552928207d7d78ae329e10
79b8f8d5749fc676d9eeca0296172b7cdbbf2238 fuzz: Assert roundtrip equality for both addrv1 and addrv2 versions of CService (practicalswift)
0e3a78a8ab7ab5da71bd5e2f428ec3a2c9ad0901 fuzz: Check for addrv1 compatibility before using addrv1 serializer/deserializer on CSubNet (practicalswift)
Pull request description:
Check for `addrv1` compatibility before using `addrv1` serializer/deserializer on `CSubNet`. As requested by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/20289#issuecomment-724012969.
Assert roundtrip equality for both `addrv1` and `addrv2` versions of `CService`.
ACKs for top commit:
MarcoFalke:
review ACK 79b8f8d5749fc676d9eeca0296172b7cdbbf2238
Tree-SHA512: 3f758aa89ab0c253b593fbe8fe9adc5c6db9afec8856facfe635053a32b4feb438c951323ae0c9e27f1d7e89d12a9b62d81f094dc96159233c12f64d4b95c290
538be4219ae7e65862e4aff540af88c9421e6061 wallet: fix importdescriptor silent fail (Ivan Metlushko)
Pull request description:
Currently `importdescriptor` command will successfully import a descriptor with hardened derivations into a watch-only wallet while silently failing to expand the descriptor to fill the cache. This leads to a broken wallet state and failure to load such wallet due to missing cache on subsequent restart.
ACKs for top commit:
laanwj:
Code review ACK 538be4219ae7e65862e4aff540af88c9421e6061
achow101:
ACK 538be4219ae7e65862e4aff540af88c9421e6061
meshcollider:
utACK 538be4219ae7e65862e4aff540af88c9421e6061
Tree-SHA512: 4bdd0ab4437d55b3f1a79c3a300a0b186089155c020fe220a73d0cce274de47d90371d88918d39fd795f9fccf8db328f1e322d29a6062f9ce94a1c254398f004
bd93fc9945bfd4be117990c5d861f61ddd451f96 Fix change detection of imported internal descriptors (Andrew Chow)
Pull request description:
Import internal descriptors were having address book entries added which meant they would be detected as non-change. Fix this and add a test for it.
ACKs for top commit:
laanwj:
Code review ACK bd93fc9945bfd4be117990c5d861f61ddd451f96
meshcollider:
utACK bd93fc9945bfd4be117990c5d861f61ddd451f96
promag:
Code review ACK bd93fc9945bfd4be117990c5d861f61ddd451f96.
Tree-SHA512: 8fa9e364be317627ec171eedffdb505976c0e7f1e55bc7e8cfdffa3aeea5db24d231f55166602cd0e97a5ba621acc871de0a765c75d0c65678f83e93c3b657c5
f1f433e8ca014f69925e8d5487877e06ca784ec8 Make it easier to reason about node eviction by removing unused NodeEvictionCandidate::addr (CAddress) (practicalswift)
Pull request description:
Make it easier to reason about node eviction by removing unused `NodeEvictionCandidate::addr` (`CAddress`).
ACKs for top commit:
jnewbery:
utACK f1f433e8ca014f69925e8d5487877e06ca784ec8
Tree-SHA512: fef91d7b412b8a4f172370cff6c37eb8c3db0ba618f5daf2dcc8737c8fcef7b9b820d7ee99cd0a9eae7dd653a096cf83d5113776b0d1d9a324147581674e9ede
d7901ab8d2fdd2f6e68c4fa48078111bf5f0fa73 fuzz: Assert expected DecodeHexTx behaviour when using legacy decoding (practicalswift)
Pull request description:
Assert expected `DecodeHexTx` behaviour when using legacy decoding.
As suggested by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/20290#issuecomment-720989597.
ACKs for top commit:
MarcoFalke:
review ACK d7901ab8d2fdd2f6e68c4fa48078111bf5f0fa73
Tree-SHA512: 3285680059e6fa73b0fb2c52b775f6319de1ac616f731206662b742764dc888cdfd1ac1f1fcfdfd5418d2006475a852d1c1a56a7035f772f0a6b2a84f5de93bc
58cfbc38e040925b51cb8d35d23b50e9cf06fb2a Ignoring (but warn) on duplicate -wallet parameters (Jonas Schnelli)
Pull request description:
I expect that there are many users with load on startup wallet definitions in `bitcoin.conf` or via startup CLI argument.
With the new `settings.json` r/w configuration file, users unloading and loading a wallet through the GUI or via the RPC calls might end up with a duplicate `-wallet` entry (one that still remains in bitcoin.conf or CLI) plus the new duplication in `settings.json` due to the unload/load.
Steps to reproduce
* create wallet (if via RPC set `load_on_startup` or unloadwallet/loadwallet then set `load_on_startup`).
* stop bitcoin
* start bitcoind again with same `--wallet=mywallet`
I guess it is acceptable to skip duplicates.
ACKs for top commit:
achow101:
Tested ACK 58cfbc38e040925b51cb8d35d23b50e9cf06fb2a
meshcollider:
Code review ACK 58cfbc38e040925b51cb8d35d23b50e9cf06fb2a
ryanofsky:
Code review ACK 58cfbc38e040925b51cb8d35d23b50e9cf06fb2a. Changes since previous review: rebased, tweaked warning message, squashed/fixed test
Tree-SHA512: f94e5a999bdd7dc291f0bc142911b0a8033929350d6f6a35b58c4a06a3c8f83147be0f0c402d4e946dedbbcc85b7e023b672c731b6d7a8984d4780017c961cfb
5cafe2b25c1d2f6825b3c8103c280020929dd645 fuzz: Add missing ECC_Start to descriptor_parse test (Ivan Metlushko)
Pull request description:
Fixes fuzzing harness.
I also observed that the corpus for this test consists only of `xprv...` keys while we are using regtest parameters. So for proper fuzzing we need either A) to update the corpus and replace `xprv...` with `tprv...` B) switch to main net in the test
ACKs for top commit:
MarcoFalke:
review ACK 5cafe2b25c1d2f6825b3c8103c280020929dd645
practicalswift:
Tested ACK 5cafe2b25c1d2f6825b3c8103c280020929dd645
Tree-SHA512: 7415a98a445ce0f96219637d2362fecfc1191ad104f55d79ca92b0c92cde165e00646be5bf3fda956385e3cb22540eca457e575048493367cdf0e00a27d7cdb8
nWalletMaxVersion was used to allow an upgrade to a version only
when the new feature was used. This makes sense for the old
-upgradewallet startup option. But because upgradewallet is now a RPC,
putting off the version bump like this does not make sense. Instead,
immediately upgrading to the given version number makes sense.
Instead of using CanSupportFeature and relying on nWalletMaxVersion,
take the new version we are upgrading to and use IsSupportedFeature
with that and the previous wallet version.
af3b0dfc5463c42fb9bff39f020fc1728ed44bc7 net: fix output of peer address in version message (Vasil Dimov)
Pull request description:
If `-logips -debug=net` is specified then we print the contents of the
version message we send to the peer, including his address. Because the
addresses in the version message use pre-BIP155 encoding they cannot
represent a Tor v3 address and we would actually send 16 `0`s instead (a
dummy IPv6 address). However we would print the full address in the log
message. Before this fix:
```
2020-10-21T12:24:17Z send version message: version 70016, blocks=653500, us=[::]:0, them=xwjtp3mj427zdp4tljiiivg2l5ijfvmt5lcsfaygtpp6cw254kykvpyd.onion:8333, peer=0
```
This is confusing because we pretend to send one thing while we actually
send another. Adjust the printout to reflect what we are sending. After
this fix:
```
2020-10-21T12:26:54Z send version message: version 70016, blocks=653500, us=[::]:0, them=[::]:0, peer=0
```
ACKs for top commit:
MarcoFalke:
review ACK af3b0dfc5463c42fb9bff39f020fc1728ed44bc7
jnewbery:
utACK af3b0dfc5463c42fb9bff39f020fc1728ed44bc7
Tree-SHA512: f169d7b4f07c219e541f7c37ea23b82c77e50085fc72ec62f1dd46970389916e177268d07d45c7be94dd209d1903f8f23eaff62b7fa782f6057dd36bb96bba82
fa3967efdb07f1d22372f4ee2e602ea1fad04a57 test: Replace ARRAYLEN with C++11 ranged for loop (MarcoFalke)
fafc5290538fde76c3780976f4b2c11dc9f24d19 test: Run AssetTest even if built --with-libs=no (MarcoFalke)
faf58ab139949ca35b33217d010b350c9a59c61d ci: Add --with-libs=no to one ci config (MarcoFalke)
Pull request description:
`script_assets_test` doesn't call libbitcoinconsensus, so it seems confusing to require it
ACKs for top commit:
fanquake:
ACK fa3967efdb07f1d22372f4ee2e602ea1fad04a57 - looks ok to me.
Tree-SHA512: 8744fef64c5d7dc19a0ef4ae9b3df5b3f356253bf000f12723064794c5e50df071824d436059985f1112d94c1b530b65cbeb6b8435114a91b195480620eafc59
0be29000c011dec0722481dbebb159873da6fa54 rpc: update conf_target helps for correctness/consistency (Jon Atack)
778b9be40667876c705e586849ea9c9e44cf451c wallet, rpc: fix send subtract_fee_from_outputs help (Jon Atack)
603c0050837ec65765208dd54dde354145fbe098 wallet: add rpc send explicit fee rate coverage (Jon Atack)
dd341e602d5160fc621c0299179b91403756b61d wallet: add sendtoaddress/sendmany explicit fee rate coverage (Jon Atack)
44e7bfa60313e4ae67da49e5ba4535038b71b453 wallet: add walletcreatefundedpsbt explicit fee rate coverage (Jon Atack)
6e1ea4273e52fdcd86c87628aa595c03a071ca8c test: refactor for walletcreatefundedpsbt fee rate coverage (Jon Atack)
3ac7b0c6f1c68e74a84d868a454f508bada6b09d wallet: fundrawtx fee rate coverage, fixup ParseConfirmTarget() (Jon Atack)
2d8eba8f8425a2515022d51f1f5b4911329fbf55 wallet: combine redundant bumpfee invalid params and args tests (Jon Atack)
1697a40b6f841a54ee0d9744ed7fd09034b0ddad wallet: improve bumpfee error/help, add explicit fee rate coverage (Jon Atack)
fc5721723d34f76f9e1ffd2e31f274ea6b22f894 wallet: fix SetFeeEstimateMode() error message (Jon Atack)
052427eef1c9da84c474c5161b1910d3328ef0da wallet, bugfix: fix bumpfee with explicit fee rate modes (Jon Atack)
Pull request description:
Follow-up to #11413 providing a base to build on for #19543:
- bugfix for `bumpfee` raising a JSON error with explicit feerates, fixes issue #20219
- adds explicit feerate test coverage for `bumpfee`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send`, `sendtoaddress`, and `sendmany`
- improves a few related RPC error messages and `ParseConfirmTarget()` / error message
- fixes/improves the explicit fee rate information in the 6 RPC helps, of which 2 were also missing `conf_target` sat/B units
This provides a spec and regression coverage for the potential next step of a universal `sat/vB` feerate argument (see #19543), as well as immediate coverage and minimum fixes for 0.21.
ACKs for top commit:
kallewoof:
Concept/Tested ACK 0be29000c011dec0722481dbebb159873da6fa54
meshcollider:
Code review + functional test run ACK 0be29000c011dec0722481dbebb159873da6fa54
Tree-SHA512: efd965003e991cba51d4504e2940f06ab3d742e34022e96a673606b44fad85596aa03a8c1809f06df7ebcf21a38e18a891e54392fe3d6fb4d120bbe4ea0cf5e0
2ead31fb1b17c9b183a4b81f0ae4f48e5cf67d64 [wallet] Return object from upgradewallet RPC (Sishir Giri)
Pull request description:
Change the return type of upgradewallet to be an object for future extensibility.
Also return any error string returned from the `UpgradeWallet()` function.
ACKs for top commit:
MarcoFalke:
ACK 2ead31fb1b17c9b183a4b81f0ae4f48e5cf67d64
meshcollider:
Tested ACK 2ead31fb1b17c9b183a4b81f0ae4f48e5cf67d64
Tree-SHA512: bcc7432d2f35093ec2463ea19e894fa885b698c0e8d8e4bd2f979bd4d722cbfed53ec589d6280968917893c64649dc9e40800b8d854273b0f9a1380f51afbdb1
