286f197704e82045c762d332aba5d1ac52e0212d Add util_ArgParsing test (Russell Yanofsky)
Pull request description:
ArgsManager test coverage for parsing of integer and boolean values is
currently very poor and doesn't give us a way of knowing whether changes to
ArgsManager may unintentionally break backwards compatibility, so this adds a
new test to catch regressions.
ACKs for top commit:
promag:
ACK 286f197, more surprising results 😱
laanwj:
ACK 286f197704e82045c762d332aba5d1ac52e0212d
Tree-SHA512: 9e1db3ef87e55abbc280af60c088f35765a1f9e2ec20507ad0c1992027b875490016868dcb8cc287e6df279dd0e00f10550901af3de3d36287867249e0bd8207
3d05d332693ec860626fc77e6ba50dec94e4e83c cli: fix -getinfo output when compiled with no wallet (fanquake)
Pull request description:
master (33b155f28732487854cf0ca29ca17c50f8c6872e):
```bash
src/bitcoin-cli -getinfo
{
"version": 199900,
"protocolversion": 70015,
"blocks": 602348,
"headers": 602348,
"verificationprogress": 0.9999995592310106,
"timeoffset": 0,
"connections": 10,
"proxy": "",
"difficulty": 13691480038694.45,
"chain": "main",
"walletversion": null,
"balance": null,
"keypoololdest": null,
"keypoolsize": null,
"paytxfee": null,
"relayfee": 0.00001000,
"warnings": "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
}
```
This PR (3d05d332693ec860626fc77e6ba50dec94e4e83c):
```bash
{
"version": 199900,
"protocolversion": 70015,
"blocks": 602348,
"headers": 602348,
"verificationprogress": 0.9999996313568186,
"timeoffset": 0,
"connections": 10,
"proxy": "",
"difficulty": 13691480038694.45,
"chain": "main",
"relayfee": 0.00001000,
"warnings": "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
}
```
ACKs for top commit:
MarcoFalke:
ouch ACK 3d05d332693ec860626fc77e6ba50dec94e4e83c
laanwj:
ACK 3d05d332693ec860626fc77e6ba50dec94e4e83c
darosior:
ACK 3d05d332693ec860626fc77e6ba50dec94e4e83c
Tree-SHA512: 055424e122a082cbfea410da287d9ceb7ed405fd68d53e2f5bef62beea80bc374a7d00366de0479d23faecb7f063b232aca52e9fdbdb97c58ddf46e7749136a9
3645e4ca0033bb6365f41ef710111780c139370f Add missing newline in util_ChainMerge test (Russell Yanofsky)
Pull request description:
This was causing a lot of test cases not not be very meaningful because
multiple configuration options were combined into one line.
The changes in test output with this fix make sense and look like:
```diff
- testnet=1 regtest=1 || test
+ testnet=1 regtest=1 || error: Invalid combination of -regtest, -testnet and -chain. Can use at most one.
```
Issue was reported and debugged by
Wladimir J. van der Laan <laanwj@protonmail.com> in
https://github.com/bitcoin/bitcoin/pull/17385#issuecomment-550033222
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always welcome.
* All other changes should have accompanying unit tests (see `src/test/`) or
functional tests (see `test/`). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
* Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Bitcoin Core, if possible.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
-->
<!--
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
ACKs for top commit:
laanwj:
ACK 3645e4ca0033bb6365f41ef710111780c139370f
practicalswift:
ACK 3645e4ca0033bb6365f41ef710111780c139370f -- diff looks correct
Tree-SHA512: ca5bde9b9f553811d4827113f4880d15d7b8f4f1455b95bbf34c9a1512fdd53062f1a2133c50d9b54f94160a1ee77a54bc82681a5f3bf25d2b0d01f8a8e95165
ArgsManager test coverage for parsing of integer and boolean values is
currently very poor and doesn't give us a way of knowing whether changes to
ArgsManager may unintentionally break backwards compatibility, so this adds a
new test to catch regressions.
fa5facd3e72b6d61374b0b93b722b55e2b090020 rpc: Remove unused boost::this_thread::interruption_point (MarcoFalke)
Pull request description:
There are predefined interruption points for `boost::thread`: https://www.boost.org/doc/libs/1_71_0/doc/html/thread/thread_management.html#interruption_points
However, the rpc threads are `std::thread`, which does not have an `std:🧵:interrupt` member function to request interruption: https://dev.visucore.com/bitcoin/doxygen/httpserver_8cpp.html#ae1a63374e18b9abd348eb74e4243ea34
Thus, the interruption points can be removed.
ACKs for top commit:
laanwj:
ACK fa5facd3e72b6d61374b0b93b722b55e2b090020, this does nothing.
practicalswift:
ACK fa5facd3e72b6d61374b0b93b722b55e2b090020
jamesob:
ACK fa5facd3e7
Tree-SHA512: 4e29a44df1f2702cbd1ffdffa559440a8bb800baab64b4116e2c3d27cd64d8d1e8aafe1dc21b1a4e3988470d03be19cae294bd5669f7abf6d487685dc8fd8d7e
dcef9a2922317cb2849f397366b6c56d755db6c9 logs: add timing information to FlushStateToDisk() (James O'Beirne)
41edaf227a69bc4846d5996675e8763fdfe0f367 logs: add BCLog::Timer and related macros (James O'Beirne)
Pull request description:
It's currently annoying to detect FlushStateToDisk() calls when benchmarking since they have to be inferred from a drop in coins count from the `UpdateTip: ` log messages. This adds a new logging utility, `BCLog::Timer`, and some related macros that are generally useful for printing timing-related logging messages, and a message that is unconditionally written when the coins cache is flushed to disk.
```
2019-09-04T20:17:51Z FlushStateToDisk: write block and undo data to disk completed (3ms)
2019-09-04T20:17:51Z FlushStateToDisk: write block index to disk completed (370ms)
2019-09-04T20:17:51Z FlushStateToDisk: write coins cache to disk (2068451 coins, 294967kB) completed (21481ms)
```
ACKs for top commit:
laanwj:
Thanks, ACK dcef9a2922317cb2849f397366b6c56d755db6c9
ryanofsky:
Code review ACK dcef9a2922317cb2849f397366b6c56d755db6c9. No changes since last review other than moving code to new timer.h header
Tree-SHA512: 6d61e48a062d3edb48d0e056a6f0b1f8031773cc99289ee4544f8349d24526b88519e1e304009d56e428f1eaf76c857bf8e7e1c0b6873a6f270306accb5edc3d
This was causing a lot of test cases not not be very meaningful because
multiple configuration options were combined into one line.
The changes in test output with this fix make sense and look like:
```diff
- testnet=1 regtest=1 || test
+ testnet=1 regtest=1 || error: Invalid combination of -regtest, -testnet and -chain. Can use at most one.
```
Issue was reported and debugged by
Wladimir J. van der Laan <laanwj@protonmail.com> in
https://github.com/bitcoin/bitcoin/pull/17385#issuecomment-550033222
1c26c16065182ca2d2cdbb05fae79cac8c75f17d Improve "Hide" button tool-tip message (Danny-Scott)
Pull request description:
Cleaned up the tool tip text, it looks as though it just got included back in 2014 when the whole section was added.
Changed hide button tool tip within transaction fee settings area from "collapse fee-settings" to "Hide transaction fee settings" to be more user friendly and fit with other tool tips.
ACKs for top commit:
laanwj:
ACK 1c26c16065182ca2d2cdbb05fae79cac8c75f17d
Tree-SHA512: e2c83271c273f785ac625da9f88e095076043e21a9c59792049c271747837d19483e0cae5466c26ef3231947b6245680c4c136a530ba6f1885f9ddc18f2560d6
fa2c44c3ccc3e7a54e2afc862addd555d5a6e51b test: Add ASSERT_DEBUG_LOG to unit test framework (MarcoFalke)
fa1936f57bbf5aebb1f8fc18701441d79219d443 logging: Add member for arbitrary print callbacks (MarcoFalke)
Pull request description:
Similar to `assert_debug_log` in the functional test framework
Top commit has no ACKs.
Tree-SHA512: aa9eaeca386b61d806867c04a33275f6eb4624fa5bf50f2928d16c83f5634bac96bcac46f9e8eda3b00b4251c5f12d7b01d6ffd84ba8e05c09eeec810cc31251
92b2f5306ba0b3f031293cb8f415b67cb002c2f1 test: add dumptxoutset RPC test (James O'Beirne)
c1ccbc3ddef931896a7e9dcfa6704e305a69fbff devtools: add utxo_snapshot.sh (James O'Beirne)
57cf74c9918d10c69a46e6ceb3cb1a5e04edf5bc rpc: add dumptxoutset (James O'Beirne)
92fafb3a7da66f737e960e541fcfbcadedf6043a coinstats: add coins_count (James O'Beirne)
707fde7b9ba522c22179e2db0ed7b462c65138d9 add unused SnapshotMetadata class (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
---
This changeset defines the serialization format for UTXO snapshots and adds an RPC command for creating them, `dumptxoutset`. It also adds a convenience script for generating and verifying snapshots at a certain height, since that requires doing a hacky rewind of the chain via `invalidateblock`.
All of this is unused at the moment.
ACKs for top commit:
laanwj:
ACK 92b2f5306ba0b3f031293cb8f415b67cb002c2f1
Tree-SHA512: 200dff87767f157d627e99506ec543465d9329860a6cd49363081619c437163a640a46d008faa92b1f44fd403bfc7a7c9e851c658b5a4849efa9a34ca976bf31
104f7de5934f13b837fcf21f6d6b2559799eabe2 remove old bootstrap relevant code (tryphe)
Pull request description:
This picks up #15954
I fixed the code and added at a functional test utilizing the scripts in `contrib/linearize` as suggested by @MarcoFalke .
ACKs for top commit:
laanwj:
ACK 104f7de5934f13b837fcf21f6d6b2559799eabe2
Tree-SHA512: acac9f285f9785fcbc3afc78118461e45bec2962f90ab90e9f82f3ad28adc90a44f0443b712458ccf486e46d891eb8a67f53e7bee5fa6d89e4387814fe03f117
b2ff500fb3e4fa05de366ab1900825bea1f70377 test: add "diamond" unit test to MempoolAncestryTests (Sebastian Falbesoner)
Pull request description:
Approaches #17271 (_Missing Unit Test for Ancestors "diamond"_).
If ancestors are represented more than once (in this case `ta` and `tb`), check that those are not overcounted.
ACKs for top commit:
laanwj:
ACK b2ff500fb3e4fa05de366ab1900825bea1f70377
Tree-SHA512: 82a6573cc7f0e82bf6fcfe207d7ddecbf297d2a203d22e95b73d887e3cb280f45a3c5f649161561c1be1eb560ff81b9b385868f205d1c12284211c2377e5ad99
To do so we update CValidationInterface::BlockDisconnect to take a
CBlockIndex pointing to the block being disconnected.
This new parameter will be use in the following commit to establish
wallet height.
1a8f0d5a74d5cc0000456932babf35301f5c1686 [tools] update nNextInvSend to use mockable time (Amiti Uttarwar)
4de630354fc6808b9b13b9e82da1a82f2f50f26a [tools] add PoissonNextSend method that returns mockable time (Amiti Uttarwar)
Pull request description:
Introduce a Poisson helper method that wraps the existing method to return `std::chrono::duration` type, which is mockable.
Needed for https://github.com/bitcoin/bitcoin/pull/16698.
ACKs for top commit:
ajtowns:
ACK 1a8f0d5a74d5cc0000456932babf35301f5c1686
MarcoFalke:
re-ACK 1a8f0d5a74d5cc0000456932babf35301f5c1686
naumenkogs:
ACK 1a8f0d5, and let's merge it and come back to it later.
Tree-SHA512: 7e2325d7c55fc0b4357cb86b83e0c218ba269f678c1786342d8bc380bfd9696373bc24ff124b9ff17a6e761c62b2b44ff5247c3911e2afdc7cc5c20417e8290b
Suggested https://github.com/bitcoin/bitcoin/pull/17304#discussion_r341194391
by Gregory Sanders <gsanders87@gmail.com>
Reason for keeping the `return true` `return false` verbosity is that more code
will be added after the ReserveKeyFromKeyPool() call before returning.
4671fc3d9e669da8b8781f0cbefee43cb9acd527 Expand on wallet_balance.py comment from https://github.com/bitcoin/bitcoin/pull/16766\#issuecomment-527563982 (Jeremy Rubin)
91f3073f08aff395dd813296bf99fd8ccc81bb27 Update release notes to mention changes to IsTrusted and impact on wallet (Jeremy Rubin)
8f174ef112199aa4e98d756039855cc561687c2e Systematize style of IsTrusted single line if (Jeremy Rubin)
b49dcbedf79613f0e0f61bfd742ed265213ed280 update variable naming conventions for IsTrusted (Jeremy Rubin)
5ffe0d144923f365cb1c2fad181eca15d1668692 Update comment in test/functional/wallet_balance.py (Jeremy Rubin)
a550c58267f50c59c2eea1d46edaa5019a8ad5d8 Update wallet_balance.py test to reflect new behavior (Jeremy Rubin)
5dd7da4ccd1354f09e2d00bab29288db0d5665d0 Reuse trustedParents in looped calls to IsTrusted (Jeremy Rubin)
595f09d6de7f1b94428cdd1310777aa6a4c584e5 Cache tx Trust per-call to avoid DoS (Jeremy Rubin)
dce032ce294fe0d531770f540b1de00dc1d13f4b Make IsTrusted scan parents recursively (Jeremy Rubin)
Pull request description:
This slightly modifies the behavior of IsTrusted to recursively check the parents of a transaction. Otherwise, it's possible that a parent is not IsTrusted but a child is. If a parent is not trusted, then a child should not be either.
This recursive scan can be a little expensive, so ~it might be beneficial to have a way of caching IsTrusted state, but this is a little complex because various conditions can change between calls to IsTrusted (e.g., re-org).~ I added a cache which works per call/across calls, but does not store the results semi-permanently. Which reduces DoS risk of this change. There is no risk of untrusted parents causing a resource exploitation, as we immediately return once that is detected.
This is a change that came up as a bug-fix esque change while working on OP_SECURETHEBAG. You can see the branch where this change is important here: https://github.com/bitcoin/bitcoin/compare/master...JeremyRubin:stb-with-rpc?expand=1. Essentially, without this change, we can be tricked into accepting an OP_SECURETHEBAG output because we don't properly check the parents. As this was a change which, on its own, was not dependent on OP_SECURETHEBAG, I broke it out as I felt the change stands on its own by fixing a long standing wallet bug.
The test wallet_balance.py has been corrected to meet the new behavior. The below comment, reproduced, explains what the issue is and the edge cases that can arise before this change.
# Before `test_balance()`, we have had two nodes with a balance of 50
# each and then we:
#
# 1) Sent 40 from node A to node B with fee 0.01
# 2) Sent 60 from node B to node A with fee 0.01
#
# Then we check the balances:
#
# 1) As is
# 2) With transaction 2 from above with 2x the fee
#
# Prior to #16766, in this situation, the node would immediately report
# a balance of 30 on node B as unconfirmed and trusted.
#
# After #16766, we show that balance as unconfirmed.
#
# The balance is indeed "trusted" and "confirmed" insofar as removing
# the mempool transactions would return at least that much money. But
# the algorithm after #16766 marks it as unconfirmed because the 'taint'
# tracking of transaction trust for summing balances doesn't consider
# which inputs belong to a user. In this case, the change output in
# question could be "destroyed" by replace the 1st transaction above.
#
# The post #16766 behavior is correct; we shouldn't be treating those
# funds as confirmed. If you want to rely on that specific UTXO existing
# which has given you that balance, you cannot, as a third party
# spending the other input would destroy that unconfirmed.
#
# For example, if the test transactions were:
#
# 1) Sent 40 from node A to node B with fee 0.01
# 2) Sent 10 from node B to node A with fee 0.01
#
# Then our node would report a confirmed balance of 40 + 50 - 10 = 80
# BTC, which is more than would be available if transaction 1 were
# replaced.
The release notes have been updated to note the new behavior.
ACKs for top commit:
ariard:
Code Review ACK 4671fc3, maybe extend DoS protection in a follow-up PR.
fjahr:
Code review ACK 4671fc3d9e669da8b8781f0cbefee43cb9acd527
ryanofsky:
Code review ACK 4671fc3d9e669da8b8781f0cbefee43cb9acd527. Changes since last review: 2 new commits adding suggested release note and python test comment, also a clean rebase with no changes to the earlier commits. The PR description is more comprehensive now, too. Looks good!
promag:
Code review ACK 4671fc3d9e669da8b8781f0cbefee43cb9acd527.
Tree-SHA512: 6b183ff425304fef49724290053514cb2770f4a2350dcb83660ef24af5c54f7c4c2c345b0f62bba60eb2d2f70625ee61a7fab76a7f491bb5a84be5c4cc86b92f
436ad436434b94982bcb7dc1d13a21949263ef73 Fix issue with conflicted mempool tx in listsinceblock (Adam Jonas)
Pull request description:
Closes #8752 by bringing back abandoned #10470.
This now checks that returned transactions are not conflicting with any transactions that are filtered out by the given blockhash and add a functional test to prevent this in the future.
For more context, #8757 was closed in favor of #10470.
ACKs for top commit:
instagibbs:
utACK 436ad43643
kallewoof:
utACK 436ad436434b94982bcb7dc1d13a21949263ef73
jonatack:
I'm not qualifed to give an ACK here but 436ad436434b94982bcb7dc1d13a21949263ef73 appears reasonable. Built/ran tests/verified that this test fails without the change in rpcwallet.cpp:
Tree-SHA512: 63d75cd3d3f19fc84dc38899b200c96179b82b24db263cd0116ee5b715265be647157855c2e35912d2fbc49c7b37db9375d6aab0ac672f0f09bece8431de5ea9
fa07b8beb598642655b1207afd275b801ff8cec2 test: Reset global args between test suites (MarcoFalke)
Pull request description:
Ideally there wouldn't be any globals in Bitcoin Core. However, as we still have globals, they need to be reset between runs of test cases. One way to do this is to run each suite in a different process. `make check` does that. However, `./src/test/test_bitcoin` when run manually or on appveyor is a single process, where all globals are preserved between test cases.
This leads to hard to debug issues such as https://github.com/bitcoin/bitcoin/pull/15845#pullrequestreview-310852164.
Fix that by resetting the global arg for each test suite. Note that this wont reset the arg between test cases, as the constructor/destructor is not called for them.
Addendum: This is not a general fix, only for `-segwitheight`. I don't know if clearing all args can be done with today's argsmanager. Nor do I know if it makes sense. Maybe we want datadir set to a temp path to not risk accidentally corrupting the default data dir?
ACKs for top commit:
laanwj:
ACK fa07b8beb598642655b1207afd275b801ff8cec2
practicalswift:
ACK fa07b8beb598642655b1207afd275b801ff8cec2
mzumsande:
ACK fa07b8beb598642655b1207afd275b801ff8cec2, I also tested that this fixes the issue in #15845.
Tree-SHA512: 1e30b06f0d2829144a61cc1bc9bdd6a694cbd911afff83dd3ad2a3f15b577fd30acdf9f1469f8cb724d0642ad5d297364fd5a8a2a9c8619a7a71fa9ae2837cdc
c98bd13e675fbf5641ed64d551b63aaf55a1a8e9 replace asserts in RPC code with CHECK_NONFATAL and add linter (Adam Jonas)
Pull request description:
- Replace instances of assert in /rpc files and rpcwallet with CHECK_NONFATAL(condition)
- Add a linter to prevent future usage of assert being used in RPC code
ref https://github.com/bitcoin/bitcoin/pull/17192
ACKs for top commit:
practicalswift:
ACK c98bd13e675fbf5641ed64d551b63aaf55a1a8e9 -- diff looks correct
Tree-SHA512: a16036b6bbcca73a5334665f66e17e1756377d582317568291da1d727fc9cf8c84bac9d9bd099534e1be315345336e5f7b66b93793135155f320dc5862a2d875
b6d2183858975abc961207c125c15791e531edcc Minor refactoring to remove implied m_addr_relay_peer. (User)
a552e8477c5bcd22a5457f4f73a2fd6db8acd2c2 added asserts to check m_addr_known when it's used (User)
090b75c14be6b9ba2efe38a17d141c6e6af575cb p2p: Avoid allocating memory for addrKnown where we don't need it (User)
Pull request description:
We should allocate memory for addrKnown filter only for those peers which are expected to participate in address relay.
Currently, we do it for all peers (including SPV and block-relay-only), which results in extra RAM where it's not needed.
Upd:
In future, we would still allow SPVs to ask for addrs, so allocation still will be done by default.
However, they will be able to opt-out via [this proposal](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-October/017428.html) and then we could save some more memory.
This PR still saves memory for block-relay-only peers immediately after merging.
Top commit has no ACKs.
Tree-SHA512: e84d93b2615556d466f5ca0e543580fde763911a3bfea3127c493ddfaba8f05c8605cb94ff795d165af542b594400995a2c51338185c298581408687e7812463
152b0a00d8e681dd098f6b548447b82ab54ebe3c Refactor: Move nTimeFirstKey accesses out of CWallet (Andrew Chow)
7ef47b88e67718766c92d23973742d08436176e0 Refactor: Move GetKeypoolSize code out of CWallet (Andrew Chow)
089e17d45c8147bd0303bcbf02dc0f7d6b387f2a Refactor: Move RewriteDB code out of CWallet (Andrew Chow)
0eac7088ab878372a72f85f9c5f7662a14199083 Refactor: Move SetupGeneration code out of CWallet (Andrew Chow)
f45d12b36cee05aa3c2685b951d27bd8a58539ae Refactor: Move HavePrivateKeys code out of CWallet::CreateWalletFromFile (Andrew Chow)
8b0d82bb428de9e7f1da7c61574e7a8376a62d43 Refactor: Move Upgrade code out of CWallet::CreateWalletFromFile (Andrew Chow)
46865ec958b6b9bde04a827de598975f14bdb5e7 Refactor: Move MarkUnusedAddresses code out of CWallet::AddToWalletIfInvolvingMe (Andrew Chow)
a18edd7b383d667b15b6d4b87aa3a055a9fa5051 Refactor: Move GetMetadata code out of getaddressinfo (Andrew Chow)
9716bbe0f8081814cbf052e8211d1c6a838e5262 Refactor: Move LoadKey LegacyScriptPubKeyMan method definition (Andrew Chow)
67be6b9e213da1fd7b2e8438c445c5a64092e831 Refactor: Move SetAddressBookWithDB call out of LegacyScriptPubKeyMan::ImportScriptPubKeys (Andrew Chow)
fc2867fdf50f47e5ad5f43445e500e3a477a8074 refactor: Replace UnsetWalletFlagWithDB with UnsetBlankWalletFlag in ScriptPubKeyMan (Andrew Chow)
78e7cbc7ba5938ab2ae6347141ca793a8fc09853 Refactor: Remove UnsetWalletFlag call from LegacyScriptPubKeyMan::SetHDSeed (Andrew Chow)
0391aba52dcc33613295fd6099c804ac7134b063 Remove SetWalletFlag from WalletStorage (Andrew Chow)
4c5491f99c6b8ca779c48be63eb2ba18d98ee52a Refactor: Move SetWalletFlag out of LegacyScriptPubKeyMan::UpgradeKeyMetadata (Andrew Chow)
769acef85730a6c0e2f5603fbd673b176c2d34d0 Refactor: Move SetAddressBook call out of LegacyScriptPubKeyMan::GetNewDestination (Andrew Chow)
acedc5b8230ed9ad07f96f51f0ef862ab3a41d5e Refactor: Add new ScriptPubKeyMan virtual methods (Andrew Chow)
533d8b364f4e589aa1acb28cdea5e6e6e80d34dc Refactor: Declare LegacyScriptPubKeyMan methods as virtual (Andrew Chow)
b4cb18bce3c9a6adab2fa32f3dad4ad966b33be0 MOVEONLY: Reorder LegacyScriptPubKeyMan methods (Andrew Chow)
Pull request description:
Moves several more key management and metadata functions into LegacyScriptPubKeyMan from CWallet to further separate the two.
Note to reviewers: All of the `if (auto spk_man = walletInstance->m_spk_man.get()) {` blocks will be replaced with for loops in the next PR so you may see some things in those blocks that don't necessarily make sense with an `if` but will with a `for`.
ACKs for top commit:
laanwj:
code review ACK 152b0a00d8e681dd098f6b548447b82ab54ebe3c
Sjors:
re-ACK 152b0a00d8e681dd098f6b548447b82ab54ebe3c
promag:
Code review ACK 152b0a00d8e681dd098f6b548447b82ab54ebe3c.
Tree-SHA512: ff9872a3ef818922166cb15d72363004ec184e1015a3928a66091bddf48995423602ccd7e55b814de85d25ad7c69058280b1fde2e633570c680dc7d6084b3122
fa8919889f3c1bd3e2700ecbb56493e3cd1e25ad bench: Remove redundant copy constructor in mempool_stress (MarcoFalke)
29f84343681831baf02a17d3af566c5c57ecf3c2 refactor: Remove redundant PSBT copy constructor (Hennadii Stepanov)
Pull request description:
I fail to see why people add these copy constructors manually without explanation, when the compiler can generate them at least as good automatically with less code.
ACKs for top commit:
promag:
ACK fa8919889f3c1bd3e2700ecbb56493e3cd1e25ad.
hebasto:
ACK fa8919889f3c1bd3e2700ecbb56493e3cd1e25ad, nit s/constructor/operator/ in commit fa8919889f3c1bd3e2700ecbb56493e3cd1e25ad message, as @promag [mentioned](https://github.com/bitcoin/bitcoin/pull/17349#discussion_r341776389) above.
jonatack:
ACK fa8919889f3c1bd3e2700ecbb56493e3cd1e25ad
Tree-SHA512: ce024fdb894328f41037420b881169b8b1b48c87fbae5f432edf371a35c82e77e21468ef97cda6f54d34f1cf9bb010235d62904bb0669793457ed1c3b2a89723
fa0a731d007a8e76d2740a2e6ead2289de77e475 test: Add RegTestingSetup to setup_common (MarcoFalke)
fa54b3e2485f701aa420c37f09a2859a5b805161 test: move-only ComputeFilter to src/test/lib/blockfilter (MarcoFalke)
Pull request description:
The default chain for `TestingSetup` is the main chain. However, any test that wants to mine blocks on demand needs to switch to regtest. This is done manually and in-line right now.
Fix that by creating an explicit `RegTestingSetup` and use it where appropriate.
Also, add a move-only commit to move `ComputeFilter` into the newly created unit test library.
Both commits are part of #15845, but split up because they are useful on their own.
ACKs for top commit:
practicalswift:
ACK fa0a731d007a8e76d2740a2e6ead2289de77e475 -- diff looks correct
Tree-SHA512: 02b9765580b355ed8d1be555f8ae11fa6e3d575f5cb177bbdda0319378837e29de5555c126c477dc8a1e8a5be47335afdcff152cf2dea2fbdd1a988ddde3689b
