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.
7f13dfb587 test: test the implicit avoid partial spends functionality (Karl-Johan Alm)
b82067bf69 wallet: try -avoidpartialspends mode and use its result if fees are below threshold (Karl-Johan Alm)
Pull request description:
The `-avoidpartialspends` feature is normally disabled, as it may affect the optimal fee for payments. This PR introduces a new parameter `-maxapsfee` (max avoid partial spends fee) which acts on the following values:
* -1: disable partial spend avoidance completely (do not even try it)
* 0: only do partial spend avoidance if fees are the same or better as the regular coin selection
* 1..∞: use APS variant if the absolute fee difference is less than or equal to the max APS fee
For values other than -1, the code will now try partial spend avoidance once, and if that gives a value within the accepted range, it will use that.
Example: -maxapsfee=0.00001000 means the wallet will do regular coin select, APS coin select, and then pick AKS iff the absolute fee difference is <= 1000 satoshi.
Edit: updated this to reflect the fact we are now using a max fee.
ACKs for top commit:
fjahr:
tested ACK 7f13dfb587
achow101:
ACK 7f13dfb587
jonatack:
ACK 7f13dfb58, code review, debug build, verified the test fails with `AssertionError: not(2 == 1)` for the number of vouts when `-maxapsfee=0.0001` is changed to 0, and verified the new logging with an added assertion.
meshcollider:
Code review ACK 7f13dfb587
Tree-SHA512: 475929df57f6191bb4e36bfbcad5a280a64bb0ecd8767b76cb2e44e2301235d0eb294a3f2fac5bbf15d35d7ecfba47acb2285feadb883c9ce31c08377e3afb3c
9adc2f80fc Refactor OutputGroups to handle effective values, fees, and filtering (Andrew Chow)
7d07e864b8 Use real value when calculating OutputGroup value (Andrew Chow)
Pull request description:
Currently, the effective values and filtering for positive effective values is done outside of the OutputGroup. We should instead have functions in Outputgroup to do this and call those for each OutputGroup. So this PR does that.
This makes future changes for effective values in coin selection much easier.
ACKs for top commit:
instagibbs:
reACK 9adc2f80fc
fjahr:
re-ACK 9adc2f80fc
meshcollider:
Light code review ACK 9adc2f80fc
Tree-SHA512: 7445c94b7295b45bcd83a6f8a5c8f6961a89453fcc856335192d4b4a66aec7724513616b04e5111588ab208c89b311055399d6279cd9c4ce452aefb85f04b64a
e7448d6680 wallet: Don't override signing errors (Fabian Jahr)
Pull request description:
While reviewing #17204 I noticed that the errors in `input_errors` from `::SignTransaction` where being overridden by `CWallet::SignTransaction`. For example, a Script related error led to incomplete signature data which led to `CWallet::SignTransaction` reporting that keys were missing, which was a less precise error than the original one.
Additionally, the error `"Input not found or already spent"` is [duplicated in `sign.cpp`](c7b4968552/src/script/sign.cpp (L481)), so the error here is redundant at the moment. So technically the whole error block could be removed, I think. However, this code is affected by the ongoing work on the wallet so there might be a reason why these errors are here. But even if there is a reason to keep them, I don't think existing, potentially more precise errors should be overridden here unless we want to hide them from the users. I am looking for feedback if this is a work in progress state where these errors could be more useful in the future or if they can be removed.
On testing: even though [the errors in `CWallet` are covered](https://marcofalke.github.io/btc_cov/total.coverage/src/wallet/wallet.cpp.gcov.html), all tests still pass after removing them. I am not sure if there is a desire to cover these specific error messages, tests in `test/functional/rpc_signrawtransaction.py` seem to aim for a more generic approach.
ACKs for top commit:
achow101:
ACK e7448d6680
meshcollider:
Code review ACK e7448d6680
Tree-SHA512: 3e2bc11d05379d2aef87b093a383d1b044787efc70e35955b2f8ecd028b6acef02f386180566af6a1a63193635f5d685466e2f6141c96326c49ffc5c81ca3e23
Instead of having callers set the fees, effective values, and filtering
of outputs, do these within OutputGroups themselves as member functions.
m_fee and m_long_term_fee is added to OutputGroup to track the fees of
the OutputGroup.
3a9aba21a4 Split SetWalletFlags into Add/LoadWalletFlags (Andrew Chow)
d9cd095b59 Split SetActiveScriptPubKeyMan into Add/LoadActiveScriptPubKeyMan (Andrew Chow)
0122fbab4c Split SetHDChain into AddHDChain and LoadHDChain (Andrew Chow)
Pull request description:
`SetHDChaiin`, `SetActiveScriptPubKeyMan`, and `SetWalletFlags` have a `memonly` argument which is kind of confusing, as noted in https://github.com/bitcoin/bitcoin/pull/17681#discussion_r427633081. This PR replaces those functions with `Add*` and `Load*` variants so that they follow the pattern used elsewhere in the wallet.
`AddHDChain`, `AddActiveScriptPubKeyMan`, and `AddWalletFlags` both set their respective variables in `CWallet` and writes them to disk. These functions are used by the actions which modify the wallet such as `sethdseed`, `importdescriptors`, and creating a new wallet.
`LoadHDChain`, `LoadActiveScriptPubKeyMan`, and `LoadWalletFlags` just set the `CWallet` variables. These functions are used by `LoadWallet` when loading the wallet from disk.
ACKs for top commit:
jnewbery:
Code review ACK 3a9aba21a4
ryanofsky:
Code review ACK 3a9aba21a4. Only changes since last review tweaks making m_wallet_flags updates more safe
meshcollider:
utACK 3a9aba21a4
Tree-SHA512: 365aeaafc5ba42879c0eb797ec3beb29ab70e27f917dc880763f743420b3be6ddf797240996beed8a9ad70fb212c2590253c6b44c9dc244529c3939d9538983f
9c59f9c285 Fix ZapSelectTx to sync wallet spends (Anthony Fieroni)
Pull request description:
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
ACKs for top commit:
achow101:
ACK 9c59f9c285
ryanofsky:
Code review ACK 9c59f9c285. Only change since last review tweaking the for loop as suggested
jonatack:
ACK 9c59f9c285 tested rebased on current master b33136b6ba and the new unit test does indeed fail without the change.
meshcollider:
utACK 9c59f9c285
Tree-SHA512: 71672a5ab0c659550c3a40577614ea896412b79566b5672636ab18765e4c71b9d0a990d94dc6b6e623b03a05737022b04026b5699438809c7c54782d0fd0a5d2
d8e9ca66d1 walletdb: Move Rewrite into BerkeleyDatabase (Andrew Chow)
91d109156d walletdb: Move PeriodicFlush into WalletDatabase (Andrew Chow)
8f1bcf8b7b walletdb: Combine VerifyDatabaseFile and VerifyEnvironment (Andrew Chow)
Pull request description:
The `BerkeleyBatch` class has 4 static functions that operate on `BerkeleyDatabase` or `BerkeleyEnvironment`. It doesn't make sense for these to be standalone nor for them to be static functions. So instead, move them from `BerkeleyBatch` into `BerkeleyDatabase` and make them member functions instead of static.
`BerkeleyBatch::VerifyEnvironment` and `BerkeleyBatch::VerifyDatabaseFile` are combined into a single `BerkeleyDatabase::Verify` function that operates on that `BerkeleyDatabase` object.
`BerkeleyBatch::Rewrite` and `BerkeleyBatch::PeriodicFlush` both took a `BerkeleyDatabase` as an argument and did stuff on it. So we just make it a member function so it doesn't need to take a database as an argument.
Part of #18971
ACKs for top commit:
MarcoFalke:
re-ACK d8e9ca66d1 only change is test fixup 🤞
promag:
Code review ACK d8e9ca66d1, good stuff.
Tree-SHA512: 9847e55b13d98bf4e5636cc14bc3f5351d56737f7e320fafffaed128606240765599e5400382c5aecac06690f7e36265ca3e1031f3f6d8a9688f6d5cb1bacd2a
84d295e513 tests: Check that segwit inputs in psbt have both UTXO types (Andrew Chow)
4600479058 psbt: always put a non_witness_utxo and don't remove it (Andrew Chow)
5279d8bc07 psbt: Allow both non_witness_utxo and witness_utxo (Andrew Chow)
72f6bec1da rpc: show both UTXOs in decodepsbt (Andrew Chow)
Pull request description:
Due to recent changes to hardware wallets, the full previous transaction will need to be provided for segwit inputs. Since some software may be checking for the existence of a `witness_utxo` to determine whether to produce a segwit signature, we keep that field to ease the transition.
Because all of the sanity checks implemented by the `IsSane` functions were related to having mixed segwit and non-segwit data in a PSBT, those functions are removed as those checks are no longer proper.
Some tests are updated/removed to accommodate this and a simple test added to check that both UTXOs are being added to segwit inputs.
As discussed in the wallet IRC meeting, our own signer will not require `non_witness_utxo` for segwit inputs.
ACKs for top commit:
Sjors:
utACK 84d295e513 (didn't retest compared to 836d6fc, but fortunately HWI's CI tracks our master branch, with a bunch of hardware wallet simulators)
ryanofsky:
Code review re-ACK 84d295e513. No changes since last review, but now I understand the context better. I think it would good to improve the comments as suggested https://github.com/bitcoin/bitcoin/pull/19215#discussion_r447889473 and maybe refer to
meshcollider:
utACK 84d295e513
Tree-SHA512: ccc1fd3c16ac3859f5aca4fa489bd40f68be0b81bbdc4dd51188bbf28827a8642dc8b605a37318e5f16cf40f1c4910052dace2f27eca21bb58435f02a443e940
fa927ff884 Enable Wswitch for OutputType (MarcoFalke)
faddad71f6 Remove confusing OutputType::CHANGE_AUTO (MarcoFalke)
fa2eb38352 interfaces: Remove unused getDefaultChangeType (MarcoFalke)
Pull request description:
`OutputType::CHANGE_AUTO` is problematic for several reasons:
* An output that is not change must never be described by `CHANGE_AUTO`. Simply allowing that option makes the code confusing and review harder than it needs to be.
* To make review even harder, `CHANGE_AUTO` requires `-Wswitch` to be disabled for `OutputType`
Fix both issues by removing `CHANGE_AUTO` and then enabling `-Wswitch` for `OutputType`
ACKs for top commit:
promag:
Code review ACK fa927ff884.
laanwj:
Code review ACK fa927ff884
Tree-SHA512: 24fd809757aa343866c94dafe9a7130b50cda4f77c97666d407f99b813f75b115a7d8e688a6bc2a737e87cba64ddd4e43f2b3c5538fd35fabb5845807bb39134
9b009fae6e qa: Test concurrent wallet loading (João Barbosa)
b9971ae585 wallet: Handle concurrent wallet loading (João Barbosa)
Pull request description:
This PR handles concurrent wallet loading.
This can be tested by running in parallel the following script a couple of times:
```sh
for i in {1..10}
do
src/bitcoin-cli -regtest loadwallet foo
src/bitcoin-cli -regtest unloadwallet foo
done
```
Eventually the error occurs:
```
error code: -4
error message:
Wallet already being loading.
```
For reference, loading and already loaded wallet gives:
```
error code: -4
error message:
Wallet file verification failed. Error loading wallet w1. Duplicate -wallet filename specified.
```
Fixes#19232.
ACKs for top commit:
MarcoFalke:
Concept ACK 9b009fae6e I have not reviewed the code
hebasto:
ACK 9b009fae6e, tested on Linux Mint 20 (x86_64):
ryanofsky:
Code review good-but-not-ideal ACK 9b009fae6e
Tree-SHA512: 0ccd77b03c0926e4c4e51efb31e193b93cb4b9ffe8bac6bb018f7344c55dfd939b873b8cf5e657dca73e6202eb75aa672de2acb787cc133184b0b3b51e47b972
931dd47608 Make lint-spelling.py happy (Glenn Willen)
11a0ffb29d [gui] Load PSBT from clipboard (Glenn Willen)
a6cb0b0c29 [gui] PSBT Operations Dialog (sign & broadcast) (Glenn Willen)
5dd0c03ffa FillPSBT: report number of inputs signed (or would sign) (Glenn Willen)
9e7b23b733 Improve TransactionErrorString messages. (Glenn Willen)
Pull request description:
Add a "PSBT Operations" dialog, reached from the "Load PSBT..." menu item, giving options to sign or broadcast the loaded PSBT as appropriate, as well as copying the result to the clipboard or saving it to a file.
This is based on Sjors' #17509, and depends on that PR going in first. (It effectively replaces the small "load PSBT" dialog from that PR with a more feature-rich one.)
Some notes:
* The way I display status information is maybe unusual (a status bar, rather than messageboxes.) I think it's helpful to have the information in it be persistent rather than transitory. But if people dislike it, I would probably move the "current state of the transaction" info to the top line of the main label, and the "what action just happened, and did it succeed" info into a messagebox.
* I don't really know much about the translation/localization stuff. I put tr() in all the places it seemed like it ought to go. I did not attempt to translate the result of TransactionErrorString (which is shared by GUI and non-GUI code); I don't know if that's correct, but it matches the "error messages in logs should be googleable in English" heuristic. I don't know whether there are things I should be doing to reduce translator effort (like minimizing the total number of distinct message strings I use, or something.)
* I don't really know how (if?) automated testing is applied to GUI code. I can make a list of PSBTs exercising all the codepaths for manual testing, if that's the right approach. Input appreciated.
ACKs for top commit:
instagibbs:
tested ACK 931dd47608
Sjors:
re-tACK 931dd47608
jb55:
ACK 931dd47608
achow101:
ACK 931dd47608
Tree-SHA512: ade52471a2242f839a8bd6a1fd231443cc4b43bb9c1de3fb5ace7c5eb59eca99b1f2e9f17dfdb4b08d84d91f5fd65677db1433dd03eef51c7774963ef4e2e74f
e5327f947c [rpc] fundrawtransaction: add_inputs option to control automatic input adding (Sjors Provoost)
79804fe24b [rpc] walletcreatefundedpsbt: don't automatically append inputs (Sjors Provoost)
Pull request description:
When the user doesn't specificy inputs, it makes sense to automatically select them. But when the user does specify inputs, `walletcreatefundedpsbt` now fails if the amount is insufficient, unless `addInputs` is set to `true`.
Similarly for `fundrawtransaction` if the original transaction already specified inputs, we only add more if `addInputs` is set to `true`.
This protects against fat finger mistakes in the amount or fee rate (see also #16257). The behavior is also more similar to GUI coin selection.
ACKs for top commit:
achow101:
ACK e5327f947c
meshcollider:
utACK e5327f947c
Tree-SHA512: d8653b820914396c7c25b0d0a2b7e92de214aa023bc1aa085feb37d3b20fab361ebea90416a7db989f19bdc37e26cf0adfbcb712c80985c87afa67a9bd44fecb
In FillPSBT, optionally report the number of inputs we successfully
signed, as an out parameter. If "sign" is false, instead report the
number of inputs for which GetSigningProvider does not return nullptr.
(This is a potentially overbroad estimate of inputs we could sign.)
44cc75f80e wallet: error if an explicit fee rate was given but the needed fee rate differed (Karl-Johan Alm)
Pull request description:
This ensures that the code doesn't silently ignore too low fee reates. It will now trigger an error in the QT client, if the user provides a fee rate below the minimum, and becomes a necessary check for #11413.
ACKs for top commit:
Sjors:
utACK 44cc75f80e (rebased)
fjahr:
re-ACK 44cc75f80e
Tree-SHA512: cd5a60ee496e64f7ab37aaa53f7748a7393357b1629ccd9660839d366c6191b6413b871ce3aa7293fce1539336222c300ef6f86304f30a1ae8fe361b02310483
- Move the decision whether to translate an error message to where it is
defined. This simplifies call sites: no more `InitError(Untranslated(...))`.
- Make all functions in `util/error.h` consistently return a
`bilingual_str`. We've decided to use this as error message type so
let's roll with it.
This has no functional changes: no messages are changed, no new
translation messages are defined.
7eaf86d3bf trivial: Suggested cleanups to surrounding code (Russell Yanofsky)
b604c5c8b5 wallet: Minimal fix to restore conflicted transaction notifications (Russell Yanofsky)
Pull request description:
This fix is a based on the fix by Antoine Riard (ariard) in https://github.com/bitcoin/bitcoin/pull/18600.
Unlike that PR, which implements some new behavior, this just restores previous wallet notification and status behavior for transactions removed from the mempool because they conflict with transactions in a block. The behavior was accidentally changed in two `CWallet::BlockConnected` updates: a31be09bfd and 7e89994133 from https://github.com/bitcoin/bitcoin/pull/16624, causing issue https://github.com/bitcoin/bitcoin/issues/18325.
The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations.
Fixes#18325
Co-authored-by: Antoine Riard (ariard)
ACKs for top commit:
jonatack:
Re-ACK 7eaf86d3bf
ariard:
ACK 7eaf86d, reviewed, built and ran tests.
MarcoFalke:
ACK 7eaf86d3bf🍡
Tree-SHA512: 9a1efe975969bb522a9dd73c41064a9348887cb67883cd92c6571fd2df4321b9f4568363891abdaae14a3b9b168ef8142e95c373fc04677e46289b251fb84689
84ae0578b6 Add release notes about salvage changes (Andrew Chow)
ea337f2d03 Move RecoverKeysOnlyFilter into RecoverDataBaseFile (Andrew Chow)
9ea2d258b4 Move RecoverDatabaseFile and RecoverKeysOnlyFilter into salvage.{cpp/h} (Andrew Chow)
b426c7764d Make BerkeleyBatch::Recover and WalletBatch::RecoverKeysOnlyFilter standalone (Andrew Chow)
2741774214 Expose a version of ReadKeyValue and use it in RecoverKeysOnlyFilter (Andrew Chow)
ced95d0e43 Move BerkeleyEnvironment::Salvage into BerkeleyBatch::Recover (Andrew Chow)
07250b8dce walletdb: remove fAggressive from Salvage (Andrew Chow)
8ebcbc85c6 walletdb: don't automatically salvage when corruption is detected (Andrew Chow)
d321046f4b wallet: remove -salvagewallet (Andrew Chow)
cdd955e580 Add basic test for bitcoin-wallet salvage (Andrew Chow)
c87770915b wallettool: Add a salvage command (Andrew Chow)
Pull request description:
Removes the `-salvagewallet` startup option and adds a `salvage` command to the `bitcoin-wallet` tool. As such, `-salvagewallet` is removed. Additionally, the automatic salvage that is done if the wallet file fails to load is removed.
Lastly the salvage code entirely is moved out entirely into `bitcoin-wallet` from `walletdb.{cpp/h}` and `db.{cpp/h}`.
ACKs for top commit:
jonatack:
ACK 84ae0578b6 feedback taken, and compared to my previous review, the bitcoin-wallet salvage command now seems to run and it exits without raising. The new test passes at both 9454105 and 84ae057 so as a sanity check I'd agree there is room for improvement, if possible.
MarcoFalke:
re-ACK 84ae0578b6🏉
Empact:
Code Review ACK 84ae0578b6
ryanofsky:
Code review ACK 84ae0578b6. Lot of small changes since previous review: added verify step before salvage, added basic test in new commit, removed unused scanstate variable and warnings parameter, tweaked various comments and strings, moved fsuccess variable declaration
meshcollider:
Concept / light code review ACK 84ae0578b6
Tree-SHA512: 05be116b56ecade1c58faca1728c8fe4b78f0a082dbc2544a3f7507dd155f1f4f39070bd1fe90053444384337bc48b97149df5c1010230d78f8ecc08e69d93af
ca2a09640f Change SetType to SetInternal and remove m_address_type (Andrew Chow)
89b1ce1140 Remove unimplemented SetCrypted from DescriptorScriptPubKeyMan (Andrew Chow)
b9073c8f13 rpc: createwallet warning that descriptor wallets are experimental (Andrew Chow)
610030d95c docs: Add release notes for descriptor wallets (Andrew Chow)
Pull request description:
Some docs and cleanup following #16528.
* Added release notes to explain a bit of motivation for descriptor wallets, what was changed, and how users will be effected by it. Also mentions the caveats regarding multsigs and watchonly that we have discussed on IRC.
* Adds a warning to `createwallet` that descriptor wallets are experimental.
* Removed unused `SetCrypted` as suggestioned: https://github.com/bitcoin/bitcoin/pull/16528#discussion_r415300916
* Removed `m_address_type` as mentioned in https://github.com/bitcoin/bitcoin/pull/18782#issuecomment-620167077
ACKs for top commit:
Sjors:
tACK ca2a09640f
instagibbs:
utACK ca2a09640f
meshcollider:
utACK ca2a09640f
Tree-SHA512: 987188a912c191430e5d3f89bcef54ba6773692fc2d95b16a3ec11d9007ded210466ed980a3857e8b7196beef6422f07f9c85cc157f996c02d16f4dbde2e7b2a
This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in
https://github.com/bitcoin/bitcoin/pull/18600.
Unlike that PR, which implements some new behavior, this just restores previous
wallet notification and status behavior for transactions removed from the
mempool because they conflict with transactions in a block. The behavior was
accidentally changed in two `CWallet::BlockConnected` updates:
a31be09bfd and
7e89994133 from
https://github.com/bitcoin/bitcoin/pull/16624, causing issue
https://github.com/bitcoin/bitcoin/issues/18325.
The change here could be improved and replaced with a more comprehensive
cleanup, so it includes a detailed comment explaining future considerations.
Fixes#18325
Co-authored-by: Antoine Riard <ariard@student.42.fr>
fa47cf9d95 wallet: Fix typo in assert that is compile-time true (MarcoFalke)
Pull request description:
Commit 92bcd70808 presumably added a check that a `dest` of type `CNoDestination` implies an empty `scriptChange`.
However, it accidentally checked for `boost::variant::empty`, which always returns false: https://www.boost.org/doc/libs/1_72_0/doc/html/boost/variant.html#id-1_3_46_5_4_1_1_16_2-bb
ACKs for top commit:
Sjors:
utACK fa47cf9d95
Tree-SHA512: 9626b1e2947039853703932a362c2ee204e002d3344856eb93eef0e0f833401336f2dfa80fd43b83c8ec6eac624e6302aee771fb67aec436ba6483be02b8d615
28b112e9bd Get rid of BindWallet (Russell Yanofsky)
d002f9d15d Disable CWalletTx copy constructor (Russell Yanofsky)
65b9d8f8dd Avoid copying CWalletTx in LoadToWallet (Russell Yanofsky)
bd2fbc7cdb Get rid of unneeded CWalletTx::Init parameter (Russell Yanofsky)
2b9cba2065 Remove CWalletTx merging logic from AddToWallet (Russell Yanofsky)
Pull request description:
This is a pure refactoring, no behavior is changing.
Instead of AddToWallet taking a temporary CWalletTx object and then potentially merging it with a pre-existing CWalletTx, have it take a callback so callers can update the pre-existing CWalletTx directly.
This makes AddToWallet simpler because now it is only has to be concerned with saving CWalletTx objects and not merging them.
This makes AddToWallet calls clearer because they can now make direct updates to CWalletTx entries without having to make temporary objects and then worry about how they will be merged.
Motivation for this change came from the bumpfee PR #8456 where we wanted to be able to call AddToWallet to make a simple update to an existing transaction, but were reluctant to, because the existing CWalletTx merging logic did not apply and seemed dangerous try to update as part of that PR. After this refactoring, the bumpfee PR could call AddToWallet safely instead of implementing a duplicate AddToWallet function.
This also allows getting rid of the CWalletTx copy constructor to prevent unintentional copying.
ACKs for top commit:
MarcoFalke:
Anyway, re-ACK 28b112e9bd
Tree-SHA512: 528dd088714472a237500b200f4433db850bdb7fc29c5e5d81cae48072061dfb967f7c37edd90b33f24901239f9be982988547c1f8c80abc25fb243fbf7330ef
m_address_type was used for two things:
1. Determine the type of descriptor to generate during
SetupDescriptorGeneration
2. Sanity check during GetNewDestination.
There is no need to have this variable to accomplish those things.
1. Add a argument to SetupDescriptorGeneration indicating the address
type to use
2. Use Descriptor::GetOutputType for the sanity check.
Also, mark feebumper bilingual_str as Untranslated
They are technical and have previously not been translated either.
It is questionable whether they can even appear in the GUI.
Common errors and warnings should be translated when displayed in the
GUI, but not translated when displayed elsewhere. The wallet method
CreateWalletFromFile does not know its caller, so this commit changes it
to return a bilingual_str to the caller.
Disable copying of CWalletTx objects to prevent bugs where instances get copied
in and out of the mapWallet map and fields are updated in the wrong copy.
Instead of AddToWallet taking a temporary CWalletTx object and then potentially
merging it with a pre-existing CWalletTx, have it take a callback so callers
can update the pre-existing CWalletTx directly.
This makes AddToWallet simpler because now it is only has to be concerned with
saving CWalletTx objects and not merging them.
This makes AddToWallet calls clearer because they can now make direct updates to
CWalletTx entries without having to make temporary objects and then worry about
how they will be merged.
This is a pure refactoring, no behavior is changing.
This change is intended to make the bitcoin node and its rpc, network
and gui interfaces more responsive while the wallet is in use. Currently
because the node's cs_main mutex is always locked before the wallet's
cs_wallet mutex (to prevent deadlocks), cs_main currently stays locked
while the wallet does relatively slow things like creating and listing
transactions.
This commit only remmove chain lock tacking in wallet code, and invert
lock order from cs_main, cs_wallet to cs_wallet, cs_main.
must happen at once to avoid any deadlock. Previous commit were only
removing Chain::Lock methods to Chain interface and enforcing they
take cs_main.
Remove LockChain method from CWallet and Chain::Lock interface.
50fc4df6c4 [mempool] Persist unbroadcast set to mempool.dat (Amiti Uttarwar)
297a178536 [test] Integration tests for unbroadcast functionality (Amiti Uttarwar)
6851502472 [refactor/test] Extract P2PTxInvStore into test framework (Amiti Uttarwar)
dc1da48dc5 [wallet] Update the rebroadcast frequency to be ~1/day. (Amiti Uttarwar)
e25e42f20a [p2p] Reattempt initial send of unbroadcast transactions (Amiti Uttarwar)
7e93eecce3 [util] Add method that returns random time in milliseconds (Amiti Uttarwar)
89eeb4a333 [mempool] Track "unbroadcast" transactions (Amiti Uttarwar)
Pull request description:
This PR introduces mempool tracking of unbroadcast transactions and periodic reattempts at initial broadcast. This is a part of the rebroadcast project, and a standalone privacy win.
The current rebroadcast logic is terrible for privacy because 1. only the source wallet rebroadcasts transactions and 2. it does so quite frequently. In the current system, if a user submits a transaction that does not immediately get broadcast to the network (eg. they are offline), this "rebroadcast" behavior is the safety net that can actually serve as the initial broadcast. So, keeping the attempts frequent is important for initial delivery within a reasonable timespan.
This PR aims to improve # 2 by reducing the wallet rebroadcast frequency to ~1/day from ~1/15 min. It achieves this by separating the notion of initial broadcast from rebroadcasts. With these changes, the mempool tracks locally submitted transactions & periodically reattempts initial broadcast. Transactions submitted via the wallet or RPC are added to an "unbroadcast" set & are removed when a peer sends a `getdata` request, or the transaction is removed from the mempool. Every 10-15 minutes, the node reattempts an initial broadcast. This enables reducing the wallet rebroadcast frequency while ensuring the transactions will be propagated to the network.
For privacy improvements around # 1, please see #16698.
Thank you to gmaxwell for the idea of how to break out this subset of functionality (https://github.com/bitcoin/bitcoin/pull/16698#issuecomment-571399346)
ACKs for top commit:
fjahr:
Code review ACK 50fc4df6c4
MarcoFalke:
ACK 50fc4df6c4, I think this is ready for merge now 👻
amitiuttarwar:
The current tip `50fc4df` currently has 6 ACKs on it, so I've opened #18807 to address the last bits.
jnewbery:
utACK 50fc4df6c4.
ariard:
Code Review ACK 50fc4df (minor points no need to invalid other ACKs)
robot-visions:
ACK 50fc4df6c4
sipa:
utACK 50fc4df6c4
naumenkogs:
utACK 50fc4df
Tree-SHA512: 2dd935d645d5e209f8abf87bfaa3ef0e4492705ce7e89ea64279cb27ffd37f4727fa94ad62d41be331177332f8edbebf3c7f4972f8cda10dd951b80a28ab3c0f
Since the mempool unbroadcast mechanism handles the reattempts for initial
broadcast, the wallet rebroadcast attempts can be much less frequent
(previously ~1/30 min)
When a CWallet doesn't have a ScriptPubKeyMan for the requested type
in GetNewDestination, give a meaningful error. Also handle this in
Qt which did not do anything with errors.
FillPSBT will add our own scripts to the PSBT if those inputs are ours.
If an input also lists pubkeys that we happen to know the private keys
for, we will sign those inputs too.
0d32d66148 Remove -upgradewallet startup option (Andrew Chow)
92263cce5b Add upgradewallet RPC (Andrew Chow)
1e48796c99 Make UpgradeWallet a member function of CWallet (Andrew Chow)
c988f27937 Have UpgradeWallet take the version to upgrade to and an error message out parameter (Andrew Chow)
1833237123 Only run UpgradeWallet if the wallet needs to be upgraded (Andrew Chow)
9c16b1735f Move wallet upgrading to its own function (Andrew Chow)
Pull request description:
`-upgradewallet` is largely incompatible with many recent wallet features and versions. For example, it was disabled if multiple wallets were used and would not work with encrypted wallets that were being upgraded to HD.
This PR does away with the old method of upgrading upon startup and instead allows users to upgrade their wallets via an `upgradewallet` RPC. This does largely the same thing as the old `-upgradewallet` option but because the wallet is loaded, it can be unlocked to upgrade to HD. Furthermore it is compatible with multiwallet as it works on the individual wallet that is specified by the RPC.
ACKs for top commit:
meshcollider:
Code review ACK 0d32d66148
darosior:
ACK 0d32d66148
MarcoFalke:
ACK 0d32d66148🚵
Tree-SHA512: b425bf6f5d605e26506889d63c780895482f07cbc086193218e031e8504d3072d41e90d65cd41bcc98ee4c1eb048954bc5d4ac85435f7394892373aac89a3b0a
92bcd70808 [wallet] allow transaction without change if keypool is empty (Sjors Provoost)
709f8685ac [wallet] CreateTransaction: simplify change address check (Sjors Provoost)
5efc25f963 [wallet] translate "Keypool ran out" message (Sjors Provoost)
Pull request description:
Extracted from #16944
First this PR simplifies the check when generating a change address, by dropping `CanGetAddresses` and just letting `reservedest.GetReservedDestination` do this check.
Second, when the keypool is empty, instead of immediately giving up, we create a dummy change address and pass that to coin selection. If we didn't need the change address (e.g. when spending the entire balance), then it's all good. If we did need a change address, we throw the original error.
ACKs for top commit:
fjahr:
Code review ACK 92bcd70808
jonasschnelli:
utACK 92bcd70808
achow101:
ACK 92bcd70808
meshcollider:
Code review ACK 92bcd70808
Tree-SHA512: 07b8c8251f57061c58a85ebf0359be63583c23bac7a2c4cefdc14820c0cdebcc90a2bb218e5ede0db11d1e204cda149e056dfd18614642070b3d56efe2735006
When a wallet uses avoid_reuse and has a large number of outputs in
a single destination, it groups these outputs in OutputGroups that
are no larger than OUTPUT_GROUP_MAX_ENTRIES. The goal is to spend
as many outputs as possible from the destination while not breaking
consensus due to a huge number of inputs and also not surprise the
use with high fees. If there are n outputs in a destination and
n > OUTPUT_GROUP_MAX_ENTRIES then this results in one or many groups
of size OUTPUT_GROUP_MAX_ENTRIES and possibly one group of size
< OUTPUT_GROUP_MAX_ENTRIES.
Prior to this commit the coin selection in the case where
n > OUTPUT_GROUP_MAX_ENTRIES was skewed towards the one group of
size < OUTPUT_GROUP_MAX_ENTRIES if it exists and the amount to be
spent by the transaction is smaller than the aggregate of those
of the group size < OUTPUT_GROUP_MAX_ENTRIES. The reason is that
the coin selection decides between the different groups based on
fees and mostly the smaller group will cause smaller fees.
The behavior that users of the avoid_reuse flag seek is that the
full groups of size OUTPUT_GROUP_MAX_ENTRIES get used first. This
commit implements this by pretending that the small group has
a large number of ancestors (one smallet than the maximum allowed
for this wallet). This dumps the small group to the bottom of the
list of priorities in the coin selection algorithm.
48973402d8 wallet: Avoid use of Chain::Lock in CWallet::GetKeyBirthTimes (Russell Yanofsky)
e958ff9ab5 wallet: Avoid use of Chain::Lock in CWallet::CreateTransaction (Russell Yanofsky)
c0d07dc4cb wallet: Avoid use of Chain::Lock in CWallet::ScanForWalletTransactions (Russell Yanofsky)
1be8ff280c wallet: Avoid use of Chain::Lock in rescanblockchain (Russell Yanofsky)
3cb85ac594 wallet refactor: Avoid use of Chain::Lock in CWallet::RescanFromTime (Russell Yanofsky)
f7ba881bc6 wallet: Avoid use of Chain::Lock in listsinceblock (Russell Yanofsky)
bc96a9bfc6 wallet: Avoid use of Chain::Lock in importmulti (Russell Yanofsky)
25a9fcf9e5 wallet: Avoid use of Chain::Lock in importwallet and dumpwallet (Russell Yanofsky)
c1694ce6bb wallet: Avoid use of Chain::Lock in importprunedfunds (Russell Yanofsky)
ade5f87971 wallet refactor: Avoid use of Chain::Lock in qt wallettests (Russell Yanofsky)
f6da44ccce wallet: Avoid use of Chain::Lock in tryGetTxStatus and tryGetBalances (Russell Yanofsky)
bf30cd4922 refactor: Add interfaces::FoundBlock class to selectively return block data (Russell Yanofsky)
Pull request description:
This is a set of changes updating wallet code to make fewer calls to `Chain::Lock` methods, so the `Chain::Lock` class will be easier to remove in #16426 with fewer code changes and small changes to behavior.
ACKs for top commit:
MarcoFalke:
re-ACK 48973402d8, only change is fixing bug 📀
fjahr:
re-ACK 48973402d8, reviewed rebase and changes since last review, built and ran tests locally
ariard:
Coce Review ACK 4897340, only changes are one suggested by last review on more accurate variable naming, human-readable output, args comments in `findCommonAncestor`
Tree-SHA512: cfd2f559f976b6faaa032794c40c9659191d5597b013abcb6c7968d36b2abb2b14d4e596f8ed8b9a077e96522365261299a241a939b3111eaf729ba0c3ef519b
b5795a7886 Wallet: Add warning comments and assert to CWallet::DelAddressBook (Luke Dashjr)
6d2905f57a Wallet: Avoid unnecessary/redundant m_address_book lookups (Luke Dashjr)
c751d886f4 Wallet: Avoid treating change-in-the-addressbook as non-change everywhere (Luke Dashjr)
8e64b8c84b Wallet: New FindAddressBookEntry method to filter out change entries (and skip ->second everywhere) (Luke Dashjr)
65b6bdc2b1 Wallet: Add CAddressBookData::IsChange which returns true iff label has never been set (Luke Dashjr)
144b2f85da Wallet: Require usage of new CAddressBookData::setLabel to change label (Luke Dashjr)
b86cd155f6 scripted-diff: Wallet: Rename mapAddressBook to m_address_book (Luke Dashjr)
Pull request description:
In many places, our code assumes that presence in the address book indicates a non-change key, and absence of an entry in mapAddressBook indicates change.
This no longer holds true after #13756 (first released in 0.19) since it added a "used" DestData populated even for change addresses. Only avoid-reuse wallets should be affected by this issue.
Thankfully, populating DestData does not write a label to the database, so we can retroactively fix this (so long as the user didn't see the change address and manually assign it a real label).
Fixing it is accomplished by:
* Adding a new bool to CAddressBookData to track if the label has ever been assigned, either by loading one from the database, or by assigning one at runtime.
* `CAddressBookData::IsChange` and `CWallet::FindAddressBookEntry` are new methods to assist in excluding change from code that doesn't expect to see them.
* For safety in merging, `CAddressBookData::name` has been made read-only (the actual data is stored in `m_label`, a new private member, and can be changed only with `setLabel` which updates the `m_change` flag), and `mapAddressBook` has been renamed to `m_address_book` (to force old code to be rebased to compile).
A final commit also does some minor optimisation, avoiding redundant lookups in `m_address_book` when we already have a pointer to the `CAddressBookData`.
ACKs for top commit:
ryanofsky:
Code review ACK b5795a7886. Pretty clever and nicely implemented fix!
jonatack:
ACK b5795a7886 nice improvements -- code review, built/ran tests rebased on current master ff53433fe4 and tested manually with rpc/cli
jnewbery:
Good fix. utACK b5795a788.
Tree-SHA512: 40525185a0bcc1723f602243c269499ec86ecb298fecb5ef24d626bbdd5e3efece86cdb1084ad7eebf7eeaf251db4a6e056bcd25bc8457b417fcbb53d032ebf0
Previous versions assumed absence of an entry in mapAddressBook indicated change.
This no longer holds true (due to bugs) and will shortly be made intentional.
Renaming the field helps ensure that old code using mapAddressBook directly gets checked for necessary rebasing.
-BEGIN VERIFY SCRIPT-
sed -i -e 's/mapAddressBook/m_address_book/g' $(git grep -l 'mapAddressBook' ./src)
-END VERIFY SCRIPT-
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change only affects behavior in the case where wallet last block processed
falls behind the chain tip, where it will treat the last block processed as the
current tip.
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change only affects behavior in the case where wallet last block processed
falls behind the chain tip, where it may set a different lock time.
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change affects behavior in a few small ways.
- If there's no max_height specified, percentage progress is measured ending at
wallet last processed block instead of node tip
- More consistent error reporting: Early check to see if start_block is on the
active chain is removed, so start_block is always read and the triggers an
error if it's unavailable
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change only affects behavior in the case where wallet last block processed
falls behind the chain tip. The rescanblockchain error height error checking
will just be stricter in this case and only accept values up to the last
processed height
FoundBlock class allows interfaces::Chain::findBlock to return more block
information without having lots of optional output parameters. FoundBlock class
is also used by other chain methods in upcoming commits.
There is mostly no change in behavior. Only exception is
CWallet::RescanFromTime now throwing NonFatalCheckError instead of
std::logic_error.
Currently it's possible for ReleaseWallet to delete the CWallet pointer while
it is processing BlockConnected, etc chain notifications.
To fix this, unregister from notifications earlier in UnloadWallet instead of
ReleaseWallet, and use a new RegisterSharedValidationInterface function to
prevent the CValidationInterface shared_ptr from being deleted until the last
notification is actually finished.
e57980b473 [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks (John Newbery)
2dd561f361 [validation] Remove pool member from ConnectTrace (John Newbery)
969b65f3f5 [validation] Remove NotifyEntryRemoved callback from ConnectTrace (John Newbery)
5613f9842b [validation] Remove conflictedTxs from PerBlockConnectTrace (John Newbery)
cdb893443c [validation interface] Remove vtxConflicted from BlockConnected (John Newbery)
1168394d75 [wallet] Notify conflicted transactions in TransactionRemovedFromMempool (John Newbery)
Pull request description:
These boost signals were added in #9371, before we had a `TransactionRemovedFromMempool` method in the validation interface. The `NotifyEntryAdded` callback was used by validation to build a vector of conflicted transactions when connecting a block, which the wallet was notified of in the `BlockConnected` CValidationInterface callback.
Now that we have a `TransactionRemovedFromMempool` callback, we can fire that signal directly from the mempool for conflicted transactions.
Note that #9371 was implemented to ensure `-walletnotify` events were fired for these conflicted transaction. We inadvertently stopped sending these notifications in #16624 (Sep 2019 commit 7e89994). We should probably fix that, but in a different PR.
ACKs for top commit:
jonatack:
Re-ACK e57980b
ryanofsky:
Code review ACK e57980b473, no code changes since previous review, but helpful new code comments have been added and the PR description is now more clear about where the old code came from
Tree-SHA512: 3bdbaf1ef2731e788462d4756e69c42a1efdcf168691ce1bbfdaa4b7b55ac3c5b1fd4ab7b90bcdec653703600501b4224d252cfc086aef28f9ce0da3b0563a69
When the user doesn't specificy inputs, it makes sense to automatically select them. But when the user does specify inputs, we now fail if the amount is insufficient, unless addInputs is set to true.
The only CValidationInterface client that cares about transactions that
are removed from the mempool because of CONFLICT is the wallet.
Start using the TransactionRemovedFromMempool method to notify about
conflicted transactions instead of using the vtxConflicted vector in
BlockConnected.
Not all ScriptPubKeyMans will be able to provide private keys,
but pubkeys and scripts should be. So only provide public-only
SigningProviders, i.e. ones that can help with Solving.
Instead of fetching a SigningProvider from ScriptPubKeyMan in order
to fill and sign the keys and scripts for a PSBT, just pass that
PSBT to a new FillPSBT function that does all that for us.
CWallet::CanGetAddresses() is used to check whether the wallet has available or is able to produce keys for addresses. It uses the ScriptPubKeyMan::CanGetAddresses(), which in turn uses the const KeypoolCountExternalKeys() method, all which do counting and no modifications.
These functions are used to mark/check if a key of our own has been used to spend (and only for avoid-reuse wallets), which has nothing to do with the destination/address itself.
Give them more accurate names to avoid confusion.
-BEGIN VERIFY SCRIPT-
sed -i -e 's/UsedDestination/SpentKey/g' $(git grep -l 'UsedDestination' ./src)
-END VERIFY SCRIPT-
4e9efac678 test: Check wallet name in -walletnotify script (João Barbosa)
9a5b5ee81f wallet: Replace %w by wallet name in -walletnotify script (João Barbosa)
Pull request description:
Fixes#13237.
ACKs for top commit:
laanwj:
ACK 4e9efac678
Tree-SHA512: 189dd1c785485f2e974d7c12531851b2a977778b3b954aa95efd527322ba3345924cfd587fb9c90b0fa979202af0ab2d90e53d125fe266a36c94f757e4176203
Needed for future ScriptPubKeyMans which may need to create
SigningProviders dynamically and thus a normal pointer is not enough
This commit does not change behavior.
Add wallet logic for dealing with multiple ScriptPubKeyMan instances. This
doesn't change current behavior because there is still only a single
LegacyScriptPubKeyMan. But in the future the new logic will be used to support
descriptor wallets.
6dd59d2e49 Don't allow implementers to think ScriptHash(Witness*()) results in nesting computation (Gregory Sanders)
4b8f1e989f IsUsedDestination shouldn't use key id as script id for ScriptHash (Gregory Sanders)
Pull request description:
Regression introduced in https://github.com/bitcoin/bitcoin/pull/17621 which causes p2sh-segwit addresses to be erroneously missed.
Tests are only failing in 0.19 branch, likely because that release still uses p2sh-segwit addresses rather than bech32 by default.
I'll devise a test case to catch this going forward.
ACKs for top commit:
achow101:
ACK 6dd59d2e49
MarcoFalke:
ACK 6dd59d2
meshcollider:
Code review ACK 6dd59d2e49
Tree-SHA512: b3e0f320c97b8c1f814cc386840240cbde2761fee9711617b713d3f75a4a5dce2dff2df573d80873df42a1f4b74e816ab8552a573fa1d62c344997fbb6af9950
6fc554f591 wallet: Reset reused transactions cache (Fabian Jahr)
Pull request description:
Fixes#17603 (together with #17824)
`getbalances` is using the cache within `GetAvailableCredit` under certain conditions [here](35fff5be60/src/wallet/wallet.cpp (L1826)). For a wallet with `avoid_reuse` activated this can lead to inconsistent reporting of `used` transactions/balances between `getbalances` and `listunspent` as pointed out in #17603. When an address is reused before the first transaction is spending from this address, the cache is not updated even after the transaction is sent. This means the remaining outputs at the reused address are not showing up as `used` in `getbalances`.
With this change, any newly incoming transaction belonging to the wallet marks all the other outputs at the same address as dirty.
ACKs for top commit:
kallewoof:
Code review re-ACK 6fc554f591
promag:
ACK 6fc554f591.
achow101:
Re-ACK 6fc554f591
meshcollider:
Code review ACK 6fc554f591
Tree-SHA512: c4cad2c752176d16d77b4a4202291d20baddf9f27250896a40274d74a6945e0f6b34be04c2f9b1b2e756d3ac669b794969df8f82a98e0b16f10e92f276649ea2
-BEGIN VERIFY SCRIPT-
# Delete outdated alias for RecursiveMutex
sed -i -e '/CCriticalSection/d' ./src/sync.h
# Replace use of outdated alias with RecursiveMutex
sed -i -e 's/CCriticalSection/RecursiveMutex/g' $(git grep -l CCriticalSection)
-END VERIFY SCRIPT-
If a destination is reused we mark the cache of the other transactions going to that destination dirty so they are not accidentally reported as trusted when the cache is hit.
6d6a7a8403 gui: Fix duplicate wallet showing up (João Barbosa)
81ea66c30e Drop signal CClientUIInterface::LoadWallet (Russell Yanofsky)
Pull request description:
This PR includes 2 fixes:
- prevent GUI LoadWallet handlers from crashing on startup when multiple handlers are attached, because the first handler takes ownership of the wallet unique pointer. Now every handler will receive its own unique pointer;
- prevent showing a wallet twice in the GUI on startup due to a race with `loadwallet`.
Fixes#16937
ACKs for top commit:
fjahr:
code review ACK 6d6a7a8403
ryanofsky:
Code review ACK 6d6a7a8403. No changes since last ACK other than rebase due to #17070
kallewoof:
Code review ACK 6d6a7a8403
Tree-SHA512: 7f0658c9011f81dfa176a094c2263448ee1d14fda7dc94e8b55ee9c8b81538bd2d1e4bf8a8dbfcd029ebfc9feb6d3cda9dee3f911122df0a4b1e0ca75f653ba4
e1e1442f3e Activate no-privkey -> ISMINE_WATCH_ONLY behavior for LegacySPKM only (Gregory Sanders)
Pull request description:
Slight cleanup following https://github.com/bitcoin/bitcoin/pull/16944
This should allow future scriptpubkeymans to transparently work, since the current plan is to have ismine always be spendable.
ACKs for top commit:
achow101:
ACK e1e1442f3e
Sjors:
Code review ACK e1e1442f3e
meshcollider:
Code review ACK e1e1442f3e
Tree-SHA512: c0a86587d33b8b1646494a5cb0bf8681ee4a88e6913918157746943a0996b501903e0e6ee954cf04154c1e0faee0cbb375c74ca789f46ba9244eb5296632b042
09502452bb IsUsedDestination should count any known single-key address (Gregory Sanders)
Pull request description:
This plugs the privacy leak detailed at https://github.com/bitcoin/bitcoin/issues/17605, at least for the single-key case.
ACKs for top commit:
meshcollider:
Code Review ACK 09502452bb
Tree-SHA512: e1d68281675f05072b3087171cba1df9416a69c9ccf70c72e8555e55eadda2d0fd339e5a894e3a3438ff94b9e3827fb19b8b701faade70c08756b19ff157ee0c
6e77a7b65c keypool: Add comment about TopUp and when to use it (Andrew Chow)
ea50e34b28 keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination (Andrew Chow)
bb2c8ce23c keypool: Remove superfluous topup from CWallet::GetNewChangeDestination (Andrew Chow)
Pull request description:
* The `TopUp()` in `CWallet::GetNewChangeDestination` is unnecessary as currently m_spk_man calls TopUp further down the call stack inside LegacyScriptPubKeyMan::ReserveKeyFromKeyPool (called by LegacyScriptPubKeyMan::GetReservedDestination). This also lets us prepare for future changes with multiple ScriptPubKeyMans in the wallet.
* An opportunistic `TopUp()` is moved from `LegacyScriptPubKeyMan::GetNewDestination` to `CWallet::GetNewDestination`.
* Another opportunistic `TopUp()` is moved from `LegacyScriptPubKeyMan::ReserveKeyFromKeyPool`
Moving opportunistic TopUps ensures that ScriptPubKeyMans will always be topped up before requesting Destinations from them as we cannot always rely on future ScriptPubKeyMan implementaions topping up internally.
See also: https://github.com/bitcoin/bitcoin/pull/17373#discussion_r348598174
ACKs for top commit:
instagibbs:
utACK 6e77a7b65c only change is slight elaboration on comment
ryanofsky:
Code review ACK 6e77a7b65c. Only the comment changed since my previous review.
Tree-SHA512: bdfc8d303842c3fb7c3d40af7abfa6d9dac4ef71a24922bb92229674ee89bfe3113ebb46d3903ac48ef99f0a7d6eaac33282495844f2b31f91b8df55084c421f
