wallet: Various fixes and cleanup to keypool handling in LegacyScriptPubKeyMan and CWallet #17373

achow101 · 2019-11-04T23:21:11Z

The pwallet->CanGetAddresses() call in ReserveDestination::GetReservedDestination to LegacyScriptPubKeyMan::GetReservedDestination so that the sanity check results in a failure when a ScriptPubKeyMan individually cannot get a destination, not when any of the ScriptPubKeyMans can't.
ScriptPubKeyMan::GetReservedDestination is changed to return the destination so that future ScriptPubKeyMans can return destinations constructed in other ways. This is implemented for LegacyScriptPubKeyMan by moving key-to-destination code from CWallet to LegacyScriptPubKeyMan
In order for ScriptPubKeyMan to be generic and work with future ScriptPubKeyMans, ScriptPubKeyMan::ReturnDestination is changed to take a CTxDestination instead of a CPubKey. Since LegacyScriptPubKeyMan still deals with keys internally, a new map m_reserved_key_to_index is added in order to track the keypool indexes that have been reserved.
A bug is fixed in how the total keypool size is calculated as it was omitting set_pre_split_keypool which is a bug.

Split from #17261

DrahtBot · 2019-11-04T23:26:25Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17681 (wallet: Keep inactive seeds after sethdseed and derive keys from them as needed by achow101)
#17369 (Refactor: Move encryption code between KeyMan and Wallet by achow101)
#17261 (Make ScriptPubKeyMan an actual interface and the wallet to have multiple by achow101)
#17219 (wallet: allow transaction without change if keypool is empty by Sjors)
#16946 (wallet: include a checksum of encrypted private keys by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Sjors

In commit 79c0a7c Key pool: Move TopUp call @ryanofsky wrote:

but may be "annoying" and lead to unnecessary topups in some situations: #16341 (comment)

My comment there was:

In the context of a hardware wallet I want TopUp to fetch keys from the device. But it should only try that if the user explicitly calls topupkeypool (and the UI equivalent). Problem is that TopUp doesn't know where it's called from.

I'm not worried about this anymore. Initially we're probably only supporting standard BIP44/49/84 style address derivation used by most hardware manufactures default wallets. In a descriptor wallet you always have the account xpub, so you can always topup the keypool without pinging the device.

In order to support hardened derivation we'd have to fetch individual public keys / destinations from the device, using a topup call. However in that case we can simply ignore a failed topup, unless we actually need the key. This is the direction e.g. #17219 is taking.

Re b1ba8b6e5ef3fe86f650c0df1042d539d455ef29 Key pool: Change ReturnDestination interface to take address instead of key.

The change seems useful, but incomplete. CTxDestination is more generic than CPubKey, which offers more flexibility with (e.g. multisig) descriptor wallets. But then I think we should use CTxDestination everywhere, so maybe:

turn std::map<int64_t, CKeyID> m_reserved_key_to_index into std::map<int64_t, CTxDestination > m_reserved_destination_to_index; and/or
std::map<CKeyID, int64_t> m_pool_key_to_index; to std::map< CTxDestination, int64_t> m_pool_destination_to_index

Maybe that's too many changes, but it might make things more readable.

Also, @ryanofsky wrote:

The address argument is currently unused, and not having the key argument
requires the legacy keyman to maintain a new m_reserved_key_to_index map, so
it's not clear what benefit of this change is, but it might help with hardware
wallet support.

I'm not sure if it does. Maintaining a map of indexes might be useful if we want to do fancy things like having two change address to throw of chain analytics.

Re d7affb5fc Key pool: Make TopUp fail if unexpected wallet flags are set. Currently in the descriptor wallet PR you have to specify a range when importing a descriptor, which determines what goes into the keypool. That's probably why the check here for WALLET_FLAG_DISABLE_PRIVATE_KEYS doesn't cause any tests to fail. But this range argument shouldn't be necessary for descriptors without hardened derivation after the xpub / xpriv. For those types of descriptors, a watch-only descriptor wallet has the same "keypool" (expanding the descriptor) functionality as a wallet with private keys. The check for WALLET_FLAG_BLANK_WALLET does make sense.

src/wallet/wallet.cpp

Sjors · 2019-11-05T15:45:42Z

src/wallet/wallet.cpp

Move assert for vchPubKey.IsValid() as well? (I know it's deleted later, but that commit is still under discussion)

Ignoring that it's removed later, the check matters here. since the pubkey should always be valid after GetReservedDestination. Either it was fetched earlier, or it was fetched just then.

achow101 · 2019-11-05T19:06:56Z

The change seems useful, but incomplete. CTxDestination is more generic than CPubKey, which offers more flexibility with (e.g. multisig) descriptor wallets. But then I think we should use CTxDestination everywhere, so maybe:

I would prefer to clean this up in a followup.

ryanofsky · 2019-11-07T22:04:01Z

I would love for somebody to do a novelization of this PR like I did for #17369 (review). I think I understand what the changes here are doing, but not why they are being made. Particularly I don't understand motivation for commits:

8b9a1ed9fb5ef64385d3881b1423059e4daaef13 Key pool: Move TopUp call (3/6)
8fc748e8050525a4235e34f3d93f3849245177f0 Key pool: Change ReturnDestination interface to take address instead of key (4/6)
51ed5b0d73fc1e7010fc4b1bc26b4e1946cf60b0 Key pool: Make TopUp fail if unexpected wallet flags are set (5/6)

Also unclear if Sjors comment above about "Make TopUp fail" #17373 (review) needs to be addressed

achow101 · 2019-11-07T23:56:35Z

@ryanofsky I've updated the OP with something like that.

Sjors · 2019-11-08T08:37:06Z

Thanks for the novelization.

Also unclear if Sjors comment above about "Make TopUp fail" #17373 (review) needs to be addressed

I.e. descriptor wallets with WALLET_FLAG_DISABLE_PRIVATE_KEYS should be able to up their "keypool". Not too hard to change that later though.

ryanofsky

Started review (will update list below with progress).

f034e5748aa2fac9fb8c9010c806d00b2b8d79ba Key pool: Move CanGetAddresses call (1/6)
35c37b59c5bb918c15aca3092b6506ef3974e50c Key pool: Move LearnRelated and GetDestination calls (2/6)
8b9a1ed9fb5ef64385d3881b1423059e4daaef13 Key pool: Move TopUp call (3/6)
8fc748e8050525a4235e34f3d93f3849245177f0 Key pool: Change ReturnDestination interface to take address instead of key (4/6)
51ed5b0d73fc1e7010fc4b1bc26b4e1946cf60b0 Key pool: Make TopUp fail if unexpected wallet flags are set (5/6)
228a316e6390e512668095331417a5ae25130170 Key pool: Fix omitted pre-split count in GetKeyPoolSize (6/6)

src/wallet/wallet.cpp

src/wallet/scriptpubkeyman.cpp

src/wallet/wallet.cpp

ryanofsky · 2019-11-14T21:46:00Z

src/wallet/wallet.cpp

In commit "Key pool: Move TopUp call" (8b9a1ed9fb5ef64385d3881b1423059e4daaef13)

Since aside from the call in GetNewChangeDestination this code is also called from CWallet::CreateTransaction, I'm wondering if this is a change in behavior. Is topup already called when creating transactions? Wondering if this the "unsollicited keypool topup" refered to by #16341 (comment)

This is not a change in behavior because TopUp is called in LegacyScriptPubKeyMan::ReserveKeyFromKeyPool

This is not a change in behavior because TopUp is called in LegacyScriptPubKeyMan::ReserveKeyFromKeyPool

😵

So if it's already called then why do we need to add a call here?

In any case, could we move all of this stuff (keyman lookup and topup call) inside the nIndex == -1 block? It seems less confusing to do lookup, topup, reservation all at once, and also maybe more efficient

So if it's already called then why do we need to add a call here?

It's a belt-and-suspenders as not all ScriptPubKeyMans are guaranteed to do this.

I've moved it inside the block.

Sjors · 2019-11-15T12:03:53Z

ACK eb9c2601377d40c4d55798a5ffcbe8c4791b91e5

Nits:

you're not really selling 06d411488c1c669f00a1c947416267941dbb7f7b with the current commit message; maybe replace with what you wrote in the PR description.
maybe call reserved_key_to_index.erase(index) in KeepDestination() as well for completeness
m_reserved_key_to_index could be more accurately called m_index_to_reserved_key or even just m_reserved_keys (which begs the question why the keypool uses sets instead of maps, but that's for a later cleanup)

ryanofsky

I'm almost there...

b118878bb174941ab6fda8b137166f4c19d0833b Key pool: Move CanGetAddresses call (1/6)
863655ae1c1325d9dbb3b1e73ec82f4fe16e9425 Key pool: Move LearnRelated and GetDestination calls (2/6)
a175268f0e406336cc5d97a0ed458b8f4a805b91 Key pool: Move TopUp call (3/6)
06d411488c1c669f00a1c947416267941dbb7f7b Key pool: Change ReturnDestination interface to take address instead of key (4/6)
b12121d52f46ac7e8455479bc9ded100cfccc841 Key pool: Make TopUp fail if unexpected wallet flags are set (5/6)
eb9c2601377d40c4d55798a5ffcbe8c4791b91e5 Key pool: Fix omitted pre-split count in GetKeyPoolSize (6/6)

ryanofsky · 2019-11-19T20:20:40Z

src/wallet/scriptpubkeyman.cpp

In commit "Key pool: Move LearnRelated and GetDestination calls" (863655ae1c1325d9dbb3b1e73ec82f4fe16e9425)

Note (mainly for myself): #17237 moves LearnRelatedScripts

ReserveDestination::GetReservedDestination -> ReserveDestination::KeepDestination

while this moves it:

ReserveDestination::GetReservedDestination -> LegacyScriptPubKeyMan::GetReservedDestination

So I guess ultimately it will be called from LegacyScriptPubKeyMan::KeepDestination

ryanofsky · 2019-11-19T20:54:24Z

src/wallet/wallet.cpp

This is not a change in behavior because TopUp is called in LegacyScriptPubKeyMan::ReserveKeyFromKeyPool

😵

So if it's already called then why do we need to add a call here?

In any case, could we move all of this stuff (keyman lookup and topup call) inside the nIndex == -1 block? It seems less confusing to do lookup, topup, reservation all at once, and also maybe more efficient

src/wallet/wallet.cpp

src/wallet/scriptpubkeyman.cpp

src/wallet/scriptpubkeyman.h

src/wallet/scriptpubkeyman.cpp

Sjors

re-ACK 69533f2 modulo the lost assert.

It improves documentation, drops the flag checks in b12121d52f, drops KeepKey and ReturnKey, adds erase(nIndex) to KeepDestination (which doesn't change behavior), moves TopUp() call under nIndex == -1.

The inline discussions have become a bit of a mess, but I believe everything is addressed. @ryanofsky?

src/wallet/scriptpubkeyman.cpp

ryanofsky

Conditional code review ACK 69533f2193556aea7a46b704b6a961dc11957701 if compile error in intermediate commit 1fecf5d0e681a266ea6adb6ae3c695b404cd3b94 is fixed. I also left some other suggestions, which I think would make the changes here safer and easier to understand, and nice to implement before merging.

This code has been pretty difficult to work with and I think every change here makes a solid improvement.

src/wallet/wallet.cpp

src/wallet/scriptpubkeyman.cpp

ryanofsky

Code review ACK 42ddfe9125fce4da6738307ef3acf100daac9d75. Changes since last review: just small suggested tweaks and dropping the topup commit

ryanofsky · 2019-11-21T15:27:52Z

@instagibbs and @meshcollider, you may be interested to review this. This is code you previously acked when it was part of #16341

src/wallet/scriptpubkeyman.h

instagibbs · 2019-11-22T15:03:37Z

src/wallet/scriptpubkeyman.cpp

Do we really want to crash if that entry doesn't exist?

I guess this is supposed to never happen, since nIndex==-1 unless it was set otherwise in GetReservedDestination where m_reserved_key_to_index is also populated.

Sjors · 2019-11-22T15:37:26Z

re-ACK 42ddfe9125fce4da6738307ef3acf100daac9d75

3958295 wallet: LearnRelatedScripts only if KeepDestination (João Barbosa) 55295fb wallet: Lock address type in ReserveDestination (João Barbosa) Pull request description: Only mutates the wallet if the reserved key is kept. First commit is a refactor that makes the address type a class member. The second commit moves `LearnRelatedScripts` from `GetReservedDestination` to `KeepDestination` to avoid an unnecessary call to `AddCScript` - which in turn prevents multiple entries of the same script in the wallet DB. ACKs for top commit: achow101: Re-ACK 3958295 Sjors: ACK 3958295 ryanofsky: Code review ACK 3958295. I like this change. The new behavior makes more sense, and the change makes the code clearer, since the current LearnRelatedScripts call is hard to understand and explain. (Personally, I'd like it if this PR were merged before #17373 or that PR was rebased on top of this one so it would be less confusing.) meshcollider: utACK 3958295 Tree-SHA512: 49a5f4b022b28042ad37ea309b28378a3983cb904e234a25795b5a360356652e0f8e60f15e3e64d85094ea63af9be01812d90ccfc08ca4f1dd927fdd8566e33f

3958295 wallet: LearnRelatedScripts only if KeepDestination (João Barbosa) 55295fb wallet: Lock address type in ReserveDestination (João Barbosa) Pull request description: Only mutates the wallet if the reserved key is kept. First commit is a refactor that makes the address type a class member. The second commit moves `LearnRelatedScripts` from `GetReservedDestination` to `KeepDestination` to avoid an unnecessary call to `AddCScript` - which in turn prevents multiple entries of the same script in the wallet DB. ACKs for top commit: achow101: Re-ACK 3958295 Sjors: ACK 3958295 ryanofsky: Code review ACK 3958295. I like this change. The new behavior makes more sense, and the change makes the code clearer, since the current LearnRelatedScripts call is hard to understand and explain. (Personally, I'd like it if this PR were merged before bitcoin#17373 or that PR was rebased on top of this one so it would be less confusing.) meshcollider: utACK 3958295 Tree-SHA512: 49a5f4b022b28042ad37ea309b28378a3983cb904e234a25795b5a360356652e0f8e60f15e3e64d85094ea63af9be01812d90ccfc08ca4f1dd927fdd8566e33f

Call LegacyScriptPubKeyMan::CanGetAddresses directly instead of calling CWallet::CanGetAddresses to only query the relevant key manager This is a minor change in behavior: call now only happens if a new key needs to be reserved, since if a key is already reserved it might fail unnecessarily. This change also serves as a sanity check bitcoin#16341 (comment)

ryanofsky

Code review ACK 562a4b0e1c5362689ff63f744737b93abd88677c, but erase bug should probably be fixed.

re: #17373 (comment)

I don't see how this is at all "nonsensical" or "even worse'. But whatever, I've learned my lesson from the multiple days spent stuck on TopUp() so it's changed

Sorry if I am being too stringent in reviewing, or if I'm just failing to understand things that are more obvious to you. I am trying to be helpful and push these changes along, but occasionally I do wind up strong opinions about minor things and try to express them without getting in the way. In this case, I implemented all the suggestions I made for you, so I wasn't trying to waste your time or make you do extra work.

Writing data that will never be used is nonsensical because it doesn't make sense. Writing garbage to disk is "even worse" than wasting memory because a clunky runtime behavior can go away with a simple code change while junk in the database can stick around forever and cause difficult to reproduce problems in the future.

src/wallet/scriptpubkeyman.cpp

instagibbs

m_reserved_key_to_index should be renamed(it's backwards), and agree with @ryanofsky suggested fixes/cleanups

contingent utACK 562a4b0

src/wallet/scriptpubkeyman.h

instagibbs · 2019-11-27T16:17:25Z

utACK 66b8289

only changes are the ones suggested in my previous review

ryanofsky

Code review ACK 66b82896d5c9d40abb9e62a7035a97025aa8aad3. Just suggested changes since last review: updating commit message, renaming map and fixing erase

Sjors

re-ACK 66b8289

Nit: 5a8a2f20b4408ae71750a3254a4b74cc6d5b1bf3 description of still uses m_reserved_key_to_index instead of m_reserved_key_to_index

@ryanofsky wrote:

Sorry if I am being too stringent in reviewing, or if I'm just failing to understand things that are more obvious to you.

The bus factor (and test coverage) of our wallet code is still uncomfortably low, and there's a reason we're sticking legacy code in a box rather than completely refactoring it. Wallet bugs ain't fun. It's worth some extra effort to be careful and/or clearly explain stuff.

Sjors · 2019-11-29T10:37:54Z

src/wallet/wallet.cpp

Note to other reviewers: this assert is lost, but GetReservedDestination calls ReserveKeyFromKeyPool which throws if !keypool.vchPubKey.IsValid()

src/wallet/wallet.cpp

ryanofsky

Code review ACK 244f5c23f864068242e6b4721577aaa470762ed5. Just suggested dest return fix since the last review. Thanks for the fix.

src/wallet/scriptpubkeyman.cpp

Addresses are determined by LegacyScriptPubKeyMan::GetReservedDestination instead of ReserveDestination::GetReservedDestination as other ScriptPubKeyMan implementations may construct addresses differently This does not change behavior.

…of key In order for ScriptPubKeyMan to be generic and work with future ScriptPubKeyMans, ScriptPubKeyMan::ReturnDestination is changed to take a CTxDestination instead of a CPubKey. Since LegacyScriptPubKeyMan still deals with keys internally, a new map m_reserved_key_to_index is added in order to track the keypool indexes that have been reserved. The CPubKey argument of KeepDestination is also removed so that it is more generic. Instead of taking a CPubKey or a CTxDestination, we just use the nIndex given to find the pubkey.

This is a bugfix: bitcoin#16341 (comment)

instagibbs

code review re-ACK 886f173

instagibbs · 2019-12-02T17:09:58Z

src/wallet/wallet.cpp

    }
-    assert(vchPubKey.IsValid());
-    address = GetDestinationForKey(vchPubKey, type);
    dest = address;


future work: address vs dest clarification via renaming would be great, even just m_dest vs dest

ryanofsky

Code review ACK 886f173. Only change is moving earlier fix to a better commit (same end result).

Sjors · 2019-12-03T11:41:16Z

Code review re-ACK 886f173

(original nit about m_reserved_key_to_index in 386a994 description still stands, not worth losing ACKs)

promag

Code review ACK 886f173.

promag · 2019-12-06T17:46:20Z

src/wallet/scriptpubkeyman.cpp

-        m_pool_key_to_index[pubkey.GetID()] = nIndex;
+        CKeyID& pubkey_id = m_index_to_reserved_key.at(nIndex);
+        m_pool_key_to_index[pubkey_id] = nIndex;
+        m_index_to_reserved_key.erase(nIndex);


nit, could avoid 2nd lookup:

auto it = find(index) ... erase(it)

Leaving as is to keep ACKs

promag · 2019-12-06T17:49:05Z

src/wallet/scriptpubkeyman.cpp

        }

+        assert(m_index_to_reserved_key.count(nIndex) == 0);
+        m_index_to_reserved_key[nIndex] = keypool.vchPubKey.GetID();


nit, could also avoid 2nd lookup:

auto res = emplace(nIndex, ...) assert(res.second)

Leaving as is to keep ACKs

…n LegacyScriptPubKeyMan and CWallet 886f173 Key pool: Fix omitted pre-split count in GetKeyPoolSize (Andrew Chow) 386a994 Key pool: Change ReturnDestination interface to take address instead of key (Andrew Chow) ba41aa4 Key pool: Move LearnRelated and GetDestination calls (Andrew Chow) 65833a7 Add OutputType and CPubKey parameters to KeepDestination (Andrew Chow) 9fcf8ce Rename Keep/ReturnKey to Keep/ReturnDestination and remove the wrapper (Andrew Chow) 596f646 Key pool: Move CanGetAddresses call (Andrew Chow) Pull request description: * The `pwallet->CanGetAddresses()` call in `ReserveDestination::GetReservedDestination` to `LegacyScriptPubKeyMan::GetReservedDestination` so that the sanity check results in a failure when a `ScriptPubKeyMan` individually cannot get a destination, not when any of the `ScriptPubKeyMan`s can't. * `ScriptPubKeyMan::GetReservedDestination` is changed to return the destination so that future `ScriptPubKeyMan`s can return destinations constructed in other ways. This is implemented for `LegacyScriptPubKeyMan` by moving key-to-destination code from `CWallet` to `LegacyScriptPubKeyMan` * In order for `ScriptPubKeyMan` to be generic and work with future `ScriptPubKeyMan`s, `ScriptPubKeyMan::ReturnDestination` is changed to take a `CTxDestination` instead of a `CPubKey`. Since `LegacyScriptPubKeyMan` still deals with keys internally, a new map `m_reserved_key_to_index` is added in order to track the keypool indexes that have been reserved. * A bug is fixed in how the total keypool size is calculated as it was omitting `set_pre_split_keypool` which is a bug. Split from #17261 ACKs for top commit: ryanofsky: Code review ACK 886f173. Only change is moving earlier fix to a better commit (same end result). promag: Code review ACK 886f173. instagibbs: code review re-ACK 886f173 Sjors: Code review re-ACK 886f173 Tree-SHA512: f4be290759f63fdc920d5c02bd0d09acc4b06a5f053787d4afcd3c921b2e35d2bd97617fadae015da853dc189f559fb8d2c6e58d53e4cabfac9af151cd97ad19

…dling in LegacyScriptPubKeyMan and CWallet 886f173 Key pool: Fix omitted pre-split count in GetKeyPoolSize (Andrew Chow) 386a994 Key pool: Change ReturnDestination interface to take address instead of key (Andrew Chow) ba41aa4 Key pool: Move LearnRelated and GetDestination calls (Andrew Chow) 65833a7 Add OutputType and CPubKey parameters to KeepDestination (Andrew Chow) 9fcf8ce Rename Keep/ReturnKey to Keep/ReturnDestination and remove the wrapper (Andrew Chow) 596f646 Key pool: Move CanGetAddresses call (Andrew Chow) Pull request description: * The `pwallet->CanGetAddresses()` call in `ReserveDestination::GetReservedDestination` to `LegacyScriptPubKeyMan::GetReservedDestination` so that the sanity check results in a failure when a `ScriptPubKeyMan` individually cannot get a destination, not when any of the `ScriptPubKeyMan`s can't. * `ScriptPubKeyMan::GetReservedDestination` is changed to return the destination so that future `ScriptPubKeyMan`s can return destinations constructed in other ways. This is implemented for `LegacyScriptPubKeyMan` by moving key-to-destination code from `CWallet` to `LegacyScriptPubKeyMan` * In order for `ScriptPubKeyMan` to be generic and work with future `ScriptPubKeyMan`s, `ScriptPubKeyMan::ReturnDestination` is changed to take a `CTxDestination` instead of a `CPubKey`. Since `LegacyScriptPubKeyMan` still deals with keys internally, a new map `m_reserved_key_to_index` is added in order to track the keypool indexes that have been reserved. * A bug is fixed in how the total keypool size is calculated as it was omitting `set_pre_split_keypool` which is a bug. Split from bitcoin#17261 ACKs for top commit: ryanofsky: Code review ACK 886f173. Only change is moving earlier fix to a better commit (same end result). promag: Code review ACK 886f173. instagibbs: code review re-ACK bitcoin@886f173 Sjors: Code review re-ACK 886f173 Tree-SHA512: f4be290759f63fdc920d5c02bd0d09acc4b06a5f053787d4afcd3c921b2e35d2bd97617fadae015da853dc189f559fb8d2c6e58d53e4cabfac9af151cd97ad19

…TopUp()s 6e77a7b keypool: Add comment about TopUp and when to use it (Andrew Chow) ea50e34 keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination (Andrew Chow) bb2c8ce 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: #17373 (comment) ACKs for top commit: instagibbs: utACK 6e77a7b only change is slight elaboration on comment ryanofsky: Code review ACK 6e77a7b. Only the comment changed since my previous review. Tree-SHA512: bdfc8d303842c3fb7c3d40af7abfa6d9dac4ef71a24922bb92229674ee89bfe3113ebb46d3903ac48ef99f0a7d6eaac33282495844f2b31f91b8df55084c421f

…fluous TopUp()s 6e77a7b keypool: Add comment about TopUp and when to use it (Andrew Chow) ea50e34 keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination (Andrew Chow) bb2c8ce 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: bitcoin#17373 (comment) ACKs for top commit: instagibbs: utACK bitcoin@6e77a7b only change is slight elaboration on comment ryanofsky: Code review ACK 6e77a7b. Only the comment changed since my previous review. Tree-SHA512: bdfc8d303842c3fb7c3d40af7abfa6d9dac4ef71a24922bb92229674ee89bfe3113ebb46d3903ac48ef99f0a7d6eaac33282495844f2b31f91b8df55084c421f

…g in LegacyScriptPubKeyMan and CWallet Summary: 886f1731bec4393dd342403ac34069a3a4f95eea Key pool: Fix omitted pre-split count in GetKeyPoolSize (Andrew Chow) 386a994b853bc5b3a2ed0d812673465b8ffa4849 Key pool: Change ReturnDestination interface to take address instead of key (Andrew Chow) ba41aa4969169cd73d6b4f57444ed7d8d875de10 Key pool: Move LearnRelated and GetDestination calls (Andrew Chow) 65833a74076cddf986037c6eb3b29a9b9dbe31c5 Add OutputType and CPubKey parameters to KeepDestination (Andrew Chow) 9fcf8ce7ae02bf170b9bf0c2887fd709d752cbf7 Rename Keep/ReturnKey to Keep/ReturnDestination and remove the wrapper (Andrew Chow) 596f6460f9fd8273665c8754ccd673d93a4f25f0 Key pool: Move CanGetAddresses call (Andrew Chow) Pull request description: * The `pwallet->CanGetAddresses()` call in `ReserveDestination::GetReservedDestination` to `LegacyScriptPubKeyMan::GetReservedDestination` so that the sanity check results in a failure when a `ScriptPubKeyMan` individually cannot get a destination, not when any of the `ScriptPubKeyMan`s can't. * `ScriptPubKeyMan::GetReservedDestination` is changed to return the destination so that future `ScriptPubKeyMan`s can return destinations constructed in other ways. This is implemented for `LegacyScriptPubKeyMan` by moving key-to-destination code from `CWallet` to `LegacyScriptPubKeyMan` * In order for `ScriptPubKeyMan` to be generic and work with future `ScriptPubKeyMan`s, `ScriptPubKeyMan::ReturnDestination` is changed to take a `CTxDestination` instead of a `CPubKey`. Since `LegacyScriptPubKeyMan` still deals with keys internally, a new map `m_reserved_key_to_index` is added in order to track the keypool indexes that have been reserved. * A bug is fixed in how the total keypool size is calculated as it was omitting `set_pre_split_keypool` which is a bug. Split from #17261 --- Depends on D7690 Backport of Core [[bitcoin/bitcoin#17373 | PR17373]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7697

…us TopUp()s Summary: 6e77a7b65cda1b46ce42f0c99ca91562255aeb28 keypool: Add comment about TopUp and when to use it (Andrew Chow) ea50e34b287e0da0806c1116bb55ade730e8ff6c keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination (Andrew Chow) bb2c8ce23c9d7ba8d0e5538243e07218443c85b4 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: bitcoin/bitcoin#17373 (comment) --- Backport of Core [[bitcoin/bitcoin#17537 | PR17537]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7722

3958295 wallet: LearnRelatedScripts only if KeepDestination (João Barbosa) 55295fb wallet: Lock address type in ReserveDestination (João Barbosa) Pull request description: Only mutates the wallet if the reserved key is kept. First commit is a refactor that makes the address type a class member. The second commit moves `LearnRelatedScripts` from `GetReservedDestination` to `KeepDestination` to avoid an unnecessary call to `AddCScript` - which in turn prevents multiple entries of the same script in the wallet DB. ACKs for top commit: achow101: Re-ACK 3958295 Sjors: ACK 3958295 ryanofsky: Code review ACK 3958295. I like this change. The new behavior makes more sense, and the change makes the code clearer, since the current LearnRelatedScripts call is hard to understand and explain. (Personally, I'd like it if this PR were merged before bitcoin#17373 or that PR was rebased on top of this one so it would be less confusing.) meshcollider: utACK 3958295 Tree-SHA512: 49a5f4b022b28042ad37ea309b28378a3983cb904e234a25795b5a360356652e0f8e60f15e3e64d85094ea63af9be01812d90ccfc08ca4f1dd927fdd8566e33f

…dling in LegacyScriptPubKeyMan and CWallet 886f173 Key pool: Fix omitted pre-split count in GetKeyPoolSize (Andrew Chow) 386a994 Key pool: Change ReturnDestination interface to take address instead of key (Andrew Chow) ba41aa4 Key pool: Move LearnRelated and GetDestination calls (Andrew Chow) 65833a7 Add OutputType and CPubKey parameters to KeepDestination (Andrew Chow) 9fcf8ce Rename Keep/ReturnKey to Keep/ReturnDestination and remove the wrapper (Andrew Chow) 596f646 Key pool: Move CanGetAddresses call (Andrew Chow) Pull request description: * The `pwallet->CanGetAddresses()` call in `ReserveDestination::GetReservedDestination` to `LegacyScriptPubKeyMan::GetReservedDestination` so that the sanity check results in a failure when a `ScriptPubKeyMan` individually cannot get a destination, not when any of the `ScriptPubKeyMan`s can't. * `ScriptPubKeyMan::GetReservedDestination` is changed to return the destination so that future `ScriptPubKeyMan`s can return destinations constructed in other ways. This is implemented for `LegacyScriptPubKeyMan` by moving key-to-destination code from `CWallet` to `LegacyScriptPubKeyMan` * In order for `ScriptPubKeyMan` to be generic and work with future `ScriptPubKeyMan`s, `ScriptPubKeyMan::ReturnDestination` is changed to take a `CTxDestination` instead of a `CPubKey`. Since `LegacyScriptPubKeyMan` still deals with keys internally, a new map `m_reserved_key_to_index` is added in order to track the keypool indexes that have been reserved. * A bug is fixed in how the total keypool size is calculated as it was omitting `set_pre_split_keypool` which is a bug. Split from bitcoin#17261 ACKs for top commit: ryanofsky: Code review ACK 886f173. Only change is moving earlier fix to a better commit (same end result). promag: Code review ACK 886f173. instagibbs: code review re-ACK bitcoin@886f173 Sjors: Code review re-ACK 886f173 Tree-SHA512: f4be290759f63fdc920d5c02bd0d09acc4b06a5f053787d4afcd3c921b2e35d2bd97617fadae015da853dc189f559fb8d2c6e58d53e4cabfac9af151cd97ad19

…fluous TopUp()s 6e77a7b keypool: Add comment about TopUp and when to use it (Andrew Chow) ea50e34 keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination (Andrew Chow) bb2c8ce 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: bitcoin#17373 (comment) ACKs for top commit: instagibbs: utACK bitcoin@6e77a7b only change is slight elaboration on comment ryanofsky: Code review ACK 6e77a7b. Only the comment changed since my previous review. Tree-SHA512: bdfc8d303842c3fb7c3d40af7abfa6d9dac4ef71a24922bb92229674ee89bfe3113ebb46d3903ac48ef99f0a7d6eaac33282495844f2b31f91b8df55084c421f

fanquake added the Wallet label Nov 4, 2019

achow101 mentioned this pull request Nov 4, 2019

Make ScriptPubKeyMan an actual interface and the wallet to have multiple #17261

Merged

Sjors reviewed Nov 5, 2019

View reviewed changes

DrahtBot added the Needs rebase label Nov 6, 2019

achow101 force-pushed the wallet-box-pr-6 branch 2 times, most recently from c810114 to 228a316 Compare November 6, 2019 23:04

DrahtBot removed the Needs rebase label Nov 7, 2019

ryanofsky reviewed Nov 14, 2019

View reviewed changes

achow101 force-pushed the wallet-box-pr-6 branch from 228a316 to eb9c260 Compare November 14, 2019 22:16

ryanofsky reviewed Nov 19, 2019

View reviewed changes

achow101 force-pushed the wallet-box-pr-6 branch 2 times, most recently from 68dbf52 to 69533f2 Compare November 20, 2019 00:10

Sjors reviewed Nov 20, 2019

View reviewed changes

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

ryanofsky approved these changes Nov 20, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the wallet-box-pr-6 branch from 69533f2 to 42ddfe9 Compare November 20, 2019 17:31

achow101 mentioned this pull request Nov 20, 2019

wallet: Cleanup and move opportunistic and superfluous TopUp()s #17537

Merged

ryanofsky approved these changes Nov 20, 2019

View reviewed changes

instagibbs reviewed Nov 22, 2019

View reviewed changes

DrahtBot added the Needs rebase label Nov 22, 2019

ryanofsky approved these changes Nov 26, 2019

View reviewed changes

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

instagibbs suggested changes Nov 27, 2019

View reviewed changes

src/wallet/scriptpubkeyman.h Outdated Show resolved Hide resolved

achow101 force-pushed the wallet-box-pr-6 branch 2 times, most recently from 24b25ee to 66b8289 Compare November 27, 2019 16:15

ryanofsky approved these changes Nov 27, 2019

View reviewed changes

Sjors approved these changes Nov 29, 2019

View reviewed changes

ryanofsky reviewed Dec 2, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the wallet-box-pr-6 branch from 66b8289 to 244f5c2 Compare December 2, 2019 14:56

ryanofsky approved these changes Dec 2, 2019

View reviewed changes

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

achow101 added 3 commits December 2, 2019 11:57

Key pool: Fix omitted pre-split count in GetKeyPoolSize

886f173

This is a bugfix: bitcoin#16341 (comment)

achow101 force-pushed the wallet-box-pr-6 branch from 244f5c2 to 886f173 Compare December 2, 2019 16:59

instagibbs reviewed Dec 2, 2019

View reviewed changes

ryanofsky approved these changes Dec 2, 2019

View reviewed changes

promag reviewed Dec 6, 2019

View reviewed changes

fanquake merged commit 886f173 into bitcoin:master Dec 6, 2019

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

wallet: Various fixes and cleanup to keypool handling in LegacyScriptPubKeyMan and CWallet #17373

wallet: Various fixes and cleanup to keypool handling in LegacyScriptPubKeyMan and CWallet #17373

Uh oh!

Conversation

achow101 commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Nov 5, 2019

Uh oh!

ryanofsky commented Nov 7, 2019

Uh oh!

achow101 commented Nov 7, 2019

Uh oh!

Sjors commented Nov 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Nov 15, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Nov 21, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Nov 22, 2019

achow101 commented Nov 4, 2019 •

edited

Loading

DrahtBot commented Nov 4, 2019 •

edited

Loading

Sjors commented Nov 8, 2019 •

edited

Loading

ryanofsky left a comment •

edited

Loading

instagibbs left a comment •

edited

Loading

Sjors left a comment •

edited

Loading