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

achow101 commented at 11:21 pm on November 4, 2019: member

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

fanquake added the label Wallet on Nov 4, 2019

DrahtBot commented at 11:26 pm on November 4, 2019: member

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.

in src/wallet/wallet.cpp:3303 in 4e9c2a59f5 outdated

3241@@ -3242,9 +3242,6 @@ bool ReserveDestination::GetReservedDestination(const OutputType type, CTxDestin
3242         return false;
3243     }
3244 
3245-    if (!pwallet->CanGetAddresses(internal)) {
3246-        return false;
3247-    }

Sjors commented at 3:41 pm on November 5, 2019:

nit: remove one more line

achow101 commented at 7:15 pm on November 5, 2019:

Does it really matter? It’s removed a few commits later anyways.

Sjors commented at 8:21 pm on November 5, 2019:

No, it’s a nit :-)

in src/wallet/wallet.cpp:3256 in 763c298e7f outdated

3252         }
3253         vchPubKey = keypool.vchPubKey;
3254         fInternal = keypool.fInternal;
3255+        address = dest;
3256     }
3257     assert(vchPubKey.IsValid());

Sjors commented at 3:45 pm on November 5, 2019:

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

achow101 commented at 7:16 pm on November 5, 2019:

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.

Sjors commented at 4:31 pm on November 5, 2019: member

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.

ryanofsky added this to the "PRs" column in a project

achow101 commented at 7:06 pm on November 5, 2019: member

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.

DrahtBot added the label Needs rebase on Nov 6, 2019

achow101 force-pushed on Nov 6, 2019

DrahtBot removed the label Needs rebase on Nov 7, 2019

laanwj added this to the "Blockers" column in a project

ryanofsky commented at 10:04 pm on November 7, 2019: member

I would love for somebody to do a novelization of this PR like I did for #17369#pullrequestreview-313636620. 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#pullrequestreview-311826649 needs to be addressed

achow101 commented at 11:56 pm on November 7, 2019: member

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

Sjors commented at 8:37 am on November 8, 2019: member

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.

in src/wallet/wallet.cpp:3301 in f034e5748a outdated

3251@@ -3252,9 +3252,6 @@ bool ReserveDestination::GetReservedDestination(const OutputType type, CTxDestin
3252         return false;
3253     }
3254 
3255-    if (!pwallet->CanGetAddresses(internal)) {

ryanofsky commented at 9:14 pm on November 14, 2019:

In commit “Key pool: Move CanGetAddresses call” (f034e5748aa2fac9fb8c9010c806d00b2b8d79ba)

Maybe say something like “Call LegacyScriptPubKeyMan::CanGetAddresses directly instead of calling CWallet::CanGetAddresses to only query the relevant key manager” in the commit description be clearer about what the change is supposed to do.

In case it helps anybody else, after 9b161a718f3c9d301419940cc9456f2065f8ca58 from #17261 there can more than keymanager and CWallet::CanGetAddresses will return false any of them can’t generate addresses, so it wouldn’t be right to call that method here.

achow101 commented at 10:16 pm on November 14, 2019:

Done

in src/wallet/scriptpubkeyman.cpp:274 in 35c37b59c5 outdated

270@@ -271,6 +271,8 @@ bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool i
271     if (!ReserveKeyFromKeyPool(index, keypool, internal)) {
272         return false;
273     }
274+    LearnRelatedScripts(keypool.vchPubKey, type);
275+    address = GetDestinationForKey(keypool.vchPubKey, type);

ryanofsky commented at 9:26 pm on November 14, 2019:

In commit “Key pool: Move LearnRelated and GetDestination calls” (35c37b59c5bb918c15aca3092b6506ef3974e50c)

Maybe point out in the commit description that addresses are determined by LegacyScriptPubKeyMan::GetReservedDestination instead of ReserveDestination::GetReservedDestination because other key manager implementations might construct them in other ways.

achow101 commented at 10:16 pm on November 14, 2019:

Done

in src/wallet/wallet.cpp:3125 in 8b9a1ed9fb outdated

3075@@ -3076,8 +3076,6 @@ bool CWallet::GetNewChangeDestination(const OutputType type, CTxDestination& des
3076 {
3077     error.clear();
3078 
3079-    m_spk_man->TopUp();

ryanofsky commented at 9:37 pm on November 14, 2019:

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

Sorry if this is a dumb question, because my understanding here is not great, but the PR description says this call has to be moved so “it will work after future changes where m_spk_man is not a global thing in CWallet and is fetched based on type and internal-ness”.

But why couldn’t the code here just look up the right keyman object using the type above and the internal-ness which it already knows?

Not saying this shouldn’t be moved, just that this doesn’t seem to explain the move

achow101 commented at 10:01 pm on November 14, 2019:

The place it is moved to does the lookup as well. It was redundant to do the lookup twice.

ryanofsky commented at 9:39 pm on November 19, 2019:

In commit “Key pool: Move TopUp call” (a175268f0e406336cc5d97a0ed458b8f4a805b91)

The place it is moved to does the lookup as well. It was redundant to do the lookup twice.

Ok, so “so that it will work after future changes” in the PR description could be clarified to “so that it will work without an extra address type -> keyman lookup after future changes”.

I’d suggest replacing this commit description something like “Not calling TopUp in CWallet::GetNewChangeDestination is a simplification that will avoid the need to look up a key manager based on address type when the wallet get multiple key managers later. Removing the TopUp call does not change behavior because the key pool will get topped up anyway in LegacyScriptPubKeyMan::ReserveKeyFromKeyPool.”

Sorry for getting stuck on this change. When I see change that looks good and safe, but I have no idea what motivated it (a hard requirement, a code cleanup, or a chance to optimize), sometimes it’s hard to get past.

achow101 commented at 0:06 am on November 20, 2019:

Updated the commit message.

in src/wallet/wallet.cpp:3256 in 8b9a1ed9fb outdated

3249@@ -3252,6 +3250,7 @@ bool ReserveDestination::GetReservedDestination(const OutputType type, CTxDestin
3250         return false;
3251     }
3252 
3253+    m_spk_man->TopUp();

ryanofsky commented at 9:46 pm on November 14, 2019:

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)

achow101 commented at 10:06 pm on November 14, 2019:

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

ryanofsky commented at 8:54 pm on November 19, 2019:

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

:dizzy_face:

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

achow101 commented at 0:07 am on November 20, 2019:

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.

ryanofsky commented at 9:47 pm on November 14, 2019: member

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)

achow101 force-pushed on Nov 14, 2019

Sjors commented at 12:03 pm on November 15, 2019: member

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)

in src/wallet/scriptpubkeyman.cpp:274 in 863655ae1c outdated

270@@ -271,6 +271,8 @@ bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool i
271     if (!ReserveKeyFromKeyPool(index, keypool, internal)) {
272         return false;
273     }
274+    LearnRelatedScripts(keypool.vchPubKey, type);

ryanofsky commented at 8:20 pm on November 19, 2019:

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

in src/wallet/wallet.cpp:3052 in b12121d52f outdated

3048@@ -3049,6 +3049,9 @@ unsigned int CWallet::GetKeyPoolSize() const
3049 
3050 bool CWallet::TopUpKeyPool(unsigned int kpSize)
3051 {
3052+    if (IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET) || IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {

ryanofsky commented at 10:15 pm on November 19, 2019:

In commit “Key pool: Make TopUp fail if unexpected wallet flags are set” (b12121d52f46ac7e8455479bc9ded100cfccc841)

There should be a code comment here to explain what’s mentioned in the PR description: that the blank wallet check is needed for TopUp to return false instead of true in a wallet without any keyman objects. It would also be good to say why that’s important, because on the surface it seems like a bad choice to return failure when nothing failed.

Relatedly, I would love to be able to drop this commit if it isn’t actually necessary or if there isn’t a specific example of a bug you could imagine this check preventing. Wallet flag code is confusing and just referencing flags in random places for no apparent reason and with no explanation isn’t the greatest thing.

It’s one thing to add sanity checks in asserts or CHECK_NONFATAL calls, but something else to add sanity checks in real code without clear “this condition is never expected to happen” comments so people reading the code don’t waste time trying to imagine cases where it could actually do anything.

achow101 commented at 0:07 am on November 20, 2019:

Looking at this change further, I don’t think it’s needed, so I’ve removed this commit.

in src/wallet/scriptpubkeyman.cpp:1184 in 06d411488c outdated

1185@@ -1185,6 +1186,7 @@ bool LegacyScriptPubKeyMan::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& key
1186             throw std::runtime_error(std::string(__func__) + ": keypool entry invalid");
1187         }
1188 
1189+        m_reserved_key_to_index[nIndex] = keypool.vchPubKey.GetID();

ryanofsky commented at 10:26 pm on November 19, 2019:

In commit “Key pool: Change ReturnDestination interface to take address instead of key” (06d411488c1c669f00a1c947416267941dbb7f7b)

assert(m_reserved_key_to_index.count(nIndex) == 0); could be reassuring here

ryanofsky commented at 10:39 pm on November 19, 2019:

In commit “Key pool: Change ReturnDestination interface to take address instead of key” (06d411488c1c669f00a1c947416267941dbb7f7b)

This commit needs a better description. The current handwavy placeholder text doesn’t explain very much. Suggest replacing it with the explanation in your PR description “In order for ScriptPubKeyMan to be generic…”

achow101 commented at 0:07 am on November 20, 2019:

Done

achow101 commented at 0:08 am on November 20, 2019:

Updated the commit message.

Sjors commented at 9:07 am on November 20, 2019:

You lost the assert (https://github.com/bitcoin/bitcoin/pull/17373#discussion_r348233531)

achow101 commented at 5:31 pm on November 20, 2019:

oops fixed

in src/wallet/scriptpubkeyman.h:250 in 06d411488c outdated

245@@ -246,6 +246,8 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
246     std::set<int64_t> set_pre_split_keypool GUARDED_BY(cs_wallet);
247     int64_t m_max_keypool_index GUARDED_BY(cs_wallet) = 0;
248     std::map<CKeyID, int64_t> m_pool_key_to_index;
249+    // Tracks keypool indexes to CKeyIDs of keys that have been taken out of the keypool but may be returned to it
250+    std::map<int64_t, CKeyID> m_reserved_key_to_index;

ryanofsky commented at 10:27 pm on November 19, 2019:

In commit “Key pool: Change ReturnDestination interface to take address instead of key” (06d411488c1c669f00a1c947416267941dbb7f7b)

Should there be separate map for internal / external key indexes? Or should map key be an (index_number, is_internal) pair? Otherwise how do you know the internal/external index numbers don’t overlap?

achow101 commented at 11:56 pm on November 19, 2019:

The indexes do not overlap. See https://github.com/bitcoin/bitcoin/blob/b4a1da9ef8e4b673c290d5b882527e627ae1b43a/src/wallet/scriptpubkeyman.cpp#L1079-L1093

instagibbs commented at 2:53 pm on November 22, 2019:

s/m_reserved_key_to_index/m_index_to_pool_key/ ? As-is I expected it to be a map from keys to index…

instagibbs commented at 2:47 pm on November 27, 2019:

as noted a few days ago, this should be renamed

achow101 commented at 4:16 pm on November 27, 2019:

Renamed it.

in src/wallet/scriptpubkeyman.cpp:287 in 06d411488c outdated

280@@ -281,9 +281,10 @@ void LegacyScriptPubKeyMan::KeepDestination(int64_t index)
281     KeepKey(index);
282 }
283 
284-void LegacyScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey)
285+void LegacyScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, const CTxDestination& addr)
286 {
287-    ReturnKey(index, internal, pubkey);
288+    ReturnKey(index, internal, m_reserved_key_to_index[index]);
289+    m_reserved_key_to_index.erase(index);

ryanofsky commented at 10:34 pm on November 19, 2019:

In commit “Key pool: Change ReturnDestination interface to take address instead of key” (06d411488c1c669f00a1c947416267941dbb7f7b)

I still don’t understand why there is this distinction between ReturnKey and ReturnDestination. Why not just rename ReturnKey to ReturnDestination and get rid of the awkward wrapping?

Previous: #17304 (review)

achow101 commented at 0:08 am on November 20, 2019:

The wrapper was for the original PR to make it clearer that things were being moved. As that no longer matters, I’ve added a commit to rename them.

in src/wallet/scriptpubkeyman.cpp:281 in 06d411488c outdated

280@@ -281,9 +281,10 @@ void LegacyScriptPubKeyMan::KeepDestination(int64_t index)
281     KeepKey(index);

ryanofsky commented at 10:36 pm on November 19, 2019:

In commit “Key pool: Change ReturnDestination interface to take address instead of key” (06d411488c1c669f00a1c947416267941dbb7f7b)

Why doesn’t this need to call m_reserved_key_to_index.erase?

achow101 commented at 0:11 am on November 20, 2019:

It should, and I’ve added it.

This wasn’t necessarily a bug as it doesn’t effect behavior.

ryanofsky commented at 10:45 pm on November 19, 2019: member

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)

achow101 force-pushed on Nov 20, 2019

in src/wallet/scriptpubkeyman.cpp:1090 in 8707ba99cf outdated

1087@@ -1088,15 +1088,16 @@ void LegacyScriptPubKeyMan::AddKeypoolPubkeyWithDB(const CPubKey& pubkey, const
1088     m_pool_key_to_index[pubkey.GetID()] = index;
1089 }
1090 
1091-void LegacyScriptPubKeyMan::KeepDestination(int64_t index)
1092+void LegacyScriptPubKeyMan::KeepDestination(int64_t nIndex)

Sjors commented at 9:04 am on November 20, 2019:

Nit: in the previous commit you renamed nIndex to index.

achow101 commented at 5:31 pm on November 20, 2019:

Fixed

Sjors commented at 9:17 am on November 20, 2019: member

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?

in src/wallet/wallet.cpp:3255 in d195dcf841 outdated

3249@@ -3252,9 +3250,10 @@ bool ReserveDestination::GetReservedDestination(const OutputType type, CTxDestin
3250         return false;
3251     }
3252 
3253-
3254     if (nIndex == -1)
3255     {
3256+        m_spk_man->TopUp();

ryanofsky commented at 4:28 pm on November 20, 2019:

In commit “Key pool: Move TopUp call” (d195dcf841e4037a517ead16db063db9d496b42b)

re: #17373 (review)

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

Would encourage adding this TopUp call in a new commit (see below), or possibly not doing it in this PR. Commit title “Key pool: Move TopUp call” is misleading. This isn’t moving a TopUp call so much as removing a useless TopUp call in one place, and adding a new, slightly justified “belt and suspenders” TopUp call in a different place.

Substantively, it looks like this is a good location for the the topup. I think in general the wallet, not the key manager should be responsible for any opportunistic, unchecked topups so we don’t have bugs caused by differences between key manager implementations. Also, since the wallet not the key manager is responsible for locking and unlocking, this would avoid key manager methods having different undocumented side effects depending on what wallet state they are called under.

Following this design would lead to three small commits:

One commit removing useless topup from CWallet::GetNewChangeDestination.
One commit moving opportunistic topup from LegacyScriptPubKeyMan::ReserveKeyFromKeyPool to ReserveDestination::GetReservedDestination for greater key manager simplicity and goodness.
One commit moving the other opportunistic topup from LegacyScriptPubKeyMan::GetNewDestination to CWallet::GetNewDestination

Relatedly, I want to emphasize how much wasted time and confusion undocumented “belt and suspenders” wallet code causes for me. I can understand code that does the right things, and I can understand code that does the wrong things, but when I see code checking for conditions that will never happen, or just not doing anything, I have 0 indication (positive or negative) code is doing what the author intended, and I waste time going back questioning fundamental assumptions to see if some inscrutable piece of code is superfluous or has some important effect in a case I never thought about. Personally, I prefer code that triggers errors or logs warnings when unexpected conditions happen instead of just silently continuing and “working anyway”. But in cases where you prefer the belt and suspenders style, having simple “This condition will never happen” or “This code should not be necessary” comments is hugely helpful.

achow101 commented at 5:31 pm on November 20, 2019:

I’ve dropped this commit for now.

achow101 commented at 5:55 pm on November 20, 2019:

Done as #17537

in src/wallet/scriptpubkeyman.cpp:1091 in 1fecf5d0e6 outdated

1087@@ -1098,15 +1088,15 @@ void LegacyScriptPubKeyMan::AddKeypoolPubkeyWithDB(const CPubKey& pubkey, const
1088     m_pool_key_to_index[pubkey.GetID()] = index;
1089 }
1090 
1091-void LegacyScriptPubKeyMan::KeepKey(int64_t nIndex)
1092+void LegacyScriptPubKeyMan::KeepDestination(int64_t index)

ryanofsky commented at 4:38 pm on November 20, 2019:

In commit “Rename Keep/ReturnKey to Keep/ReturnDestination and remove the wrapper” (1fecf5d0e681a266ea6adb6ae3c695b404cd3b94)

Should be nIndex (this commit doesn’t compile)

achow101 commented at 5:31 pm on November 20, 2019:

Fixed

in src/wallet/scriptpubkeyman.cpp:1112 in 8707ba99cf outdated

1108@@ -1108,7 +1109,9 @@ void LegacyScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, cons
1109         } else {
1110             setExternalKeyPool.insert(nIndex);
1111         }
1112-        m_pool_key_to_index[pubkey.GetID()] = nIndex;
1113+        CKeyID& pubkey_id = m_reserved_key_to_index[nIndex];

ryanofsky commented at 4:49 pm on November 20, 2019:

In commit “Key pool: Change ReturnDestination interface to take address instead of key” (8707ba99cf78c556ddda42bf74ddcda164ec9db9)

Would be good to change [nIndex] to .at(nIndex) here and move this before the inserts above. Safer throw an exception if a bad index is provided than insert nonsense into the keypool.

achow101 commented at 5:31 pm on November 20, 2019:

Done

in src/wallet/scriptpubkeyman.cpp:1100 in 8707ba99cf outdated

1097+    m_reserved_key_to_index.erase(nIndex);
1098     WalletLogPrintf("keypool keep %d\n", nIndex);
1099 }
1100 
1101-void LegacyScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey)
1102+void LegacyScriptPubKeyMan::ReturnDestination(int64_t nIndex, bool fInternal, const CTxDestination& addr)

ryanofsky commented at 4:53 pm on November 20, 2019:

In commit “Key pool: Change ReturnDestination interface to take address instead of key” (8707ba99cf78c556ddda42bf74ddcda164ec9db9)

Maybe change const CTxDestination& addr to const CTxDestination& to make it more obvious the address argument is not used by this implementation.

achow101 commented at 5:31 pm on November 20, 2019:

Done

ryanofsky approved

ryanofsky commented at 5:08 pm on November 20, 2019: member

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.

achow101 force-pushed on Nov 20, 2019

ryanofsky approved

ryanofsky commented at 5:57 pm on November 20, 2019: member

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

ryanofsky commented at 3:27 pm on November 21, 2019: member

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

in src/wallet/scriptpubkeyman.cpp:1114 in b4696cd3aa outdated

1108@@ -1108,7 +1109,9 @@ void LegacyScriptPubKeyMan::ReturnDestination(int64_t nIndex, bool fInternal, co
1109         } else {
1110             setExternalKeyPool.insert(nIndex);
1111         }
1112-        m_pool_key_to_index[pubkey.GetID()] = nIndex;
1113+        CKeyID& pubkey_id = m_reserved_key_to_index.at(nIndex);

instagibbs commented at 3:03 pm on November 22, 2019:

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 commented at 3:37 pm on November 22, 2019: member

re-ACK 42ddfe9125fce4da6738307ef3acf100daac9d75

meshcollider referenced this in commit 7127c31020 on Nov 22, 2019

sidhujag referenced this in commit 2c89dc9c98 on Nov 22, 2019

DrahtBot added the label Needs rebase on Nov 22, 2019

Key pool: Move CanGetAddresses call

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
https://github.com/bitcoin/bitcoin/pull/16341#discussion_r331238394

596f6460f9

achow101 force-pushed on Nov 23, 2019

DrahtBot removed the label Needs rebase on Nov 23, 2019

achow101 commented at 4:29 am on November 23, 2019: member

Rebased.

To resolve some issues during the rebase, I had to change a LearnRelatedScripts into a LearnAllRelatedScripts. This shoudn’t change behavior since all of the scripts were implicitly learned anyways.

Sjors commented at 12:32 pm on November 23, 2019: member

re-ACK 5daefe31c34e12d2a0678aa93e74e94b76d834f5

You could stick to LearnRelatedScripts with https://github.com/Sjors/bitcoin/commit/8b75834396b1731f36f57712ba962074b3f7c6c1 but that’s not pretty. The only way LearnAllRelatedScripts produces a different result from LearnRelatedScripts is if you reserved a key for a legacy change address. I’m a bit confused about the other code paths that could end up calling Learn(All)RelatedScripts, but even if we don’t already, I don’t see any harm in adding redundant segwit scripts for legacy change addresses.

promag commented at 9:11 pm on November 24, 2019: member

Code review ACK 5daefe31c34e12d2a0678aa93e74e94b76d834f5.

in src/wallet/scriptpubkeyman.cpp:1106 in c593c2e963 outdated

1101@@ -1101,6 +1102,9 @@ void LegacyScriptPubKeyMan::KeepKey(int64_t nIndex)
1102     // Remove from key pool
1103     WalletBatch batch(m_storage.GetDatabase());
1104     batch.ErasePool(nIndex);
1105+    CPubKey pk;
1106+    assert(GetPubKey(m_reserved_key_to_index.at(nIndex), pk));

ryanofsky commented at 0:38 am on November 26, 2019:

In commit “Key pool: Move LearnRelated and GetDestination calls” (c593c2e96351677821e367a6c7aae1ca58f81ff7)

Would move this call outside the assert. It’s bad to execute code that has side effects in an assert because in a build with NDEBUG defined it the code will be skipped. We don’t support these builds currently, but we may in the future, and it’s bad that the breakage caused by this could be subtle and hard to debug.

Also, running code in an assert can be bad for readability if you are used to being able to skim past logic in assert statements.

ryanofsky commented at 0:41 am on November 26, 2019:

In commit “Key pool: Move LearnRelated and GetDestination calls” (c593c2e96351677821e367a6c7aae1ca58f81ff7)

This commit doesn’t compile because m_reserved_key_to_index isn’t defined yet.

achow101 commented at 5:03 pm on November 26, 2019:

Done

achow101 commented at 5:03 pm on November 26, 2019:

Fixed

in src/wallet/scriptpubkeyman.cpp:1107 in c593c2e963 outdated

1101@@ -1101,6 +1102,9 @@ void LegacyScriptPubKeyMan::KeepKey(int64_t nIndex)
1102     // Remove from key pool
1103     WalletBatch batch(m_storage.GetDatabase());
1104     batch.ErasePool(nIndex);
1105+    CPubKey pk;
1106+    assert(GetPubKey(m_reserved_key_to_index.at(nIndex), pk));
1107+    LearnAllRelatedScripts(pk);

ryanofsky commented at 0:48 am on November 26, 2019:

In commit “Key pool: Move LearnRelated and GetDestination calls” (c593c2e96351677821e367a6c7aae1ca58f81ff7)

I don’t like calling LearnAllRelatedScripts here. There’s no reason for nonsense scripts to be added to memory, and it’s crazy when wallet code just does random stuff like this with no explanation. Also I don’t think this PR needs to be subtly changing behavior more than its already doing, especially without being explained or even mentioned in the commit.

achow101 commented at 5:58 am on November 26, 2019:

I don’t see a good way to fix that. i don’t really like adding the type as an argument to KeepDestination as it isn’t useful in other ScriptPubKeyMans.

This also doesn’t actually change anything since those scripts are already in memory as they are added via ImplicitlyLearnRelatedScripts which is called during key loading. This just writes them to the wallet file.

ryanofsky commented at 12:25 pm on November 26, 2019:

This just writes them to the wallet file.

Wow, that is even worse. If you think this is a good idea please add a comment here explaining that this is doing something nonsensical because design considerations make it not possible for the KeepDestination function to be aware of the destination type.

But I’d encourage you to take a look at my straight refactoring of this code in https://github.com/ryanofsky/bitcoin/commits/review.17373.6-fix which doesn’t change behavior, doesn’t write garbage to files with no explanation, but does (yes) add a type context parameter to the KeepDestination function, exactly analogous to the internal context parameter passed to ReturnDestination function, with the understanding that the whole point of having a ScriptPubKeyMan virtual class is to have subclasses that will be implemented differently, and not every implementation will require the exact same information do to every operation.

achow101 commented at 5:04 pm on November 26, 2019:

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.

in src/wallet/wallet.cpp:3321 in c593c2e963 outdated

3318 
3319 void ReserveDestination::KeepDestination()
3320 {
3321     if (nIndex != -1) {
3322         m_spk_man->KeepDestination(nIndex);
3323-        m_spk_man->LearnRelatedScripts(vchPubKey, type);

ryanofsky commented at 0:50 am on November 26, 2019:

In commit “Key pool: Move LearnRelated and GetDestination calls” (c593c2e96351677821e367a6c7aae1ca58f81ff7)

Second half of the commit message “There is a minor change in behavior” is now out of date and should be updated.

ryanofsky commented at 1:05 am on November 26, 2019: member

Code review meh 5daefe31c34e12d2a0678aa93e74e94b76d834f5. Just rebase since last review, but it’s kind of a bad rebase. I left a bunch of suggestions to fix, but if you want to just take my suggestions, I pushed a branch implementing them in https://github.com/ryanofsky/bitcoin/commits/review.17373.6-fix

Rename Keep/ReturnKey to Keep/ReturnDestination and remove the wrapper

There is no reason to have Keep/ReturnDestination to be a wrapper for
Keep/ReturnKey. Instead just make them the same function.

9fcf8ce7ae

Add OutputType and CPubKey parameters to KeepDestination

These need to be added so that LearnRelatedScripts can be called
from within KeepDestination later.

65833a7407

achow101 force-pushed on Nov 26, 2019

in src/wallet/scriptpubkeyman.cpp:1098 in 094f0545d2 outdated

1091@@ -1091,6 +1092,7 @@ void LegacyScriptPubKeyMan::KeepDestination(int64_t nIndex, const OutputType& ty
1092     // Remove from key pool
1093     WalletBatch batch(m_storage.GetDatabase());
1094     batch.ErasePool(nIndex);
1095+    LearnRelatedScripts(pubkey, type);

ryanofsky commented at 9:22 pm on November 26, 2019:

In commit “Key pool: Move LearnRelated and GetDestination calls” (094f0545d2d1309ad85eb66f107dec52746490df)

Commit description “There is a minor change in behavior” seems out of date. Would just say “This commit does not change behavior”.

The previous description was referring to LearnRelatedScripts call moving under the GetReservedDestination nIndex == -1 check in the old version of this PR before #17237.

achow101 commented at 4:15 pm on November 27, 2019:

Fixed.

ryanofsky commented at 4:41 pm on December 2, 2019:

re: #17373 (review)

Done as suggested.

Note: This change was done in the wrong commit “Change ReturnDestination interface” (82dbc9344c33217e01c16b27d1418f042d0d0d77) instead the suggested “Move LearnRelated and GetDestination” (393315354da454eb0f9322b92ad736bd23606f97), so dest return value is temporarily left unset by the earlier commit and then fixed again in a later unrelated commit. Would be nice to fix but not important.

in src/wallet/scriptpubkeyman.cpp:1090 in 8e70f2b415 outdated

1086@@ -1087,16 +1087,19 @@ void LegacyScriptPubKeyMan::AddKeypoolPubkeyWithDB(const CPubKey& pubkey, const
1087     m_pool_key_to_index[pubkey.GetID()] = index;
1088 }
1089 
1090-void LegacyScriptPubKeyMan::KeepDestination(int64_t nIndex, const OutputType& type, const CPubKey& pubkey)
1091+void LegacyScriptPubKeyMan::KeepDestination(int64_t nIndex, const OutputType& type)

ryanofsky commented at 9:25 pm on November 26, 2019:

In commit “Key pool: Change ReturnDestination interface to take address instead of key” (8e70f2b4150ca2d25f06cc65295cb27a86c49681)

This should call m_reserved_key_to_index.erase(nIndex); to clean up

achow101 commented at 4:15 pm on November 27, 2019:

Fixed.

ryanofsky approved

ryanofsky commented at 9:36 pm on November 26, 2019: member

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

re: #17373 (review)

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.

instagibbs changes_requested

instagibbs commented at 2:51 pm on November 27, 2019: member

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

contingent utACK https://github.com/bitcoin/bitcoin/pull/17373/commits/562a4b0e1c5362689ff63f744737b93abd88677c

achow101 force-pushed on Nov 27, 2019

instagibbs commented at 4:17 pm on November 27, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/17373/commits/66b82896d5c9d40abb9e62a7035a97025aa8aad3

only changes are the ones suggested in my previous review

ryanofsky approved

ryanofsky commented at 5:46 pm on November 27, 2019: member

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

in src/wallet/wallet.cpp:3314 in 5a8a2f20b4 outdated

3304@@ -3305,31 +3305,27 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool inter
3305         if (!m_spk_man->GetReservedDestination(type, internal, dest, nIndex, keypool)) {
3306             return false;
3307         }
3308-        vchPubKey = keypool.vchPubKey;
3309         fInternal = keypool.fInternal;
3310         address = dest;
3311     }
3312-    assert(vchPubKey.IsValid());

Sjors commented at 10:37 am on November 29, 2019:

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

Sjors approved

Sjors commented at 10:57 am on November 29, 2019: member

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.

in src/wallet/wallet.cpp:3309 in 393315354d outdated

3307             return false;
3308         }
3309         vchPubKey = keypool.vchPubKey;
3310         fInternal = keypool.fInternal;
3311+        address = dest;
3312     }

ryanofsky commented at 1:24 am on December 2, 2019:

In commit “Key pool: Move LearnRelated and GetDestination calls” (393315354da454eb0f9322b92ad736bd23606f97

Do we need:

0@@ -3308,6 +3308,8 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool inter
1         vchPubKey = keypool.vchPubKey;
2         fInternal = keypool.fInternal;
3         address = dest;
4+    } else {
5+        dest = address;
6     }
7     assert(vchPubKey.IsValid());
8     return true;

It seems like dest is currently left unset in the nIndex != -1 case where previously it was always set.

EDIT: Actually a better fix would probably be to undelete the previous dest = address; line below and pass address instead of dest to GetReservedDestination

achow101 commented at 2:56 pm on December 2, 2019:

Done as suggested.

achow101 commented at 4:59 pm on December 2, 2019:

Oops, moved it to the right commit.

achow101 force-pushed on Dec 2, 2019

ryanofsky approved

ryanofsky commented at 4:44 pm on December 2, 2019: member

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

Key pool: Move LearnRelated and GetDestination calls

Addresses are determined by LegacyScriptPubKeyMan::GetReservedDestination
instead of ReserveDestination::GetReservedDestination as other ScriptPubKeyMan
implementations may construct addresses differently

This does not change behavior.

ba41aa4969

Key pool: Change ReturnDestination interface to take address instead 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.

386a994b85

Key pool: Fix omitted pre-split count in GetKeyPoolSize

This is a bugfix: https://github.com/bitcoin/bitcoin/pull/16341#discussion_r330669214

886f1731be

achow101 force-pushed on Dec 2, 2019

in src/wallet/wallet.cpp:3310 in 886f1731be

3312-        vchPubKey = keypool.vchPubKey;
3313         fInternal = keypool.fInternal;
3314     }
3315-    assert(vchPubKey.IsValid());
3316-    address = GetDestinationForKey(vchPubKey, type);
3317     dest = address;

instagibbs commented at 5:09 pm on December 2, 2019:

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

instagibbs commented at 5:10 pm on December 2, 2019: member

code review re-ACK https://github.com/bitcoin/bitcoin/pull/17373/commits/886f1731bec4393dd342403ac34069a3a4f95eea

ryanofsky approved

ryanofsky commented at 5:16 pm on December 2, 2019: member

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

Sjors commented at 11:41 am on December 3, 2019: member

Code review re-ACK 886f1731bec4393dd342403ac34069a3a4f95eea

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

in src/wallet/scriptpubkeyman.cpp:1117 in 886f1731be

1111@@ -1112,13 +1112,15 @@ void LegacyScriptPubKeyMan::ReturnKey(int64_t nIndex, bool fInternal, const CPub
1112         } else {
1113             setExternalKeyPool.insert(nIndex);
1114         }
1115-        m_pool_key_to_index[pubkey.GetID()] = nIndex;
1116+        CKeyID& pubkey_id = m_index_to_reserved_key.at(nIndex);
1117+        m_pool_key_to_index[pubkey_id] = nIndex;
1118+        m_index_to_reserved_key.erase(nIndex);

promag commented at 5:46 pm on December 6, 2019:

nit, could avoid 2nd lookup:

0auto it = find(index)
1...
2erase(it)

achow101 commented at 6:03 pm on December 6, 2019:

Leaving as is to keep ACKs

in src/wallet/scriptpubkeyman.cpp:1185 in 886f1731be

1180@@ -1179,6 +1181,8 @@ bool LegacyScriptPubKeyMan::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& key
1181             throw std::runtime_error(std::string(__func__) + ": keypool entry invalid");
1182         }
1183 
1184+        assert(m_index_to_reserved_key.count(nIndex) == 0);
1185+        m_index_to_reserved_key[nIndex] = keypool.vchPubKey.GetID();

promag commented at 5:49 pm on December 6, 2019:

nit, could also avoid 2nd lookup:

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

achow101 commented at 6:03 pm on December 6, 2019:

Leaving as is to keep ACKs

promag commented at 5:51 pm on December 6, 2019: member

Code review ACK 886f1731bec4393dd342403ac34069a3a4f95eea.

fanquake referenced this in commit 4ee8a58ce7 on Dec 6, 2019

fanquake merged this on Dec 6, 2019

fanquake closed this on Dec 6, 2019

sidhujag referenced this in commit 18e4eb8dc8 on Dec 6, 2019

laanwj removed this from the "Blockers" column in a project

fanquake referenced this in commit e6acd9f72c on Dec 17, 2019

sidhujag referenced this in commit eff0f71f2b on Dec 17, 2019

fanquake moved this from the "PRs" to the "Done" column in a project

deadalnix referenced this in commit 0aad192a06 on Oct 1, 2020

jasonbcox referenced this in commit 34b1f1fca4 on Oct 1, 2020

sidhujag referenced this in commit fae7ed590d on Nov 10, 2020

sidhujag referenced this in commit 9a623ea048 on Nov 10, 2020

sidhujag referenced this in commit 3b26837621 on Nov 10, 2020

DrahtBot locked this on Feb 15, 2022

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

Conflicts