Skip to content

Commit 4ee8a58

Browse files
fanquakefanquake
committed
Merge #17373: wallet: Various fixes and cleanup to keypool handling 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 #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/bitcoin@886f173 Sjors: Code review re-ACK 886f173 Tree-SHA512: f4be290759f63fdc920d5c02bd0d09acc4b06a5f053787d4afcd3c921b2e35d2bd97617fadae015da853dc189f559fb8d2c6e58d53e4cabfac9af151cd97ad19
2 parents da1af85 + 886f173 commit 4ee8a58

File tree

4 files changed

+36
-43
lines changed

4 files changed

+36
-43
lines changed

‎src/wallet/scriptpubkeyman.cpp

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestinat
1818

1919
// Generate a new key that is added to wallet
2020
CPubKey new_key;
21-
if (!GetKeyFromPool(new_key)) {
21+
if (!GetKeyFromPool(new_key, type)) {
2222
error = "Error: Keypool ran out, please call keypoolrefill first";
2323
return false;
2424
}
@@ -262,24 +262,19 @@ bool LegacyScriptPubKeyMan::EncryptKeys(CKeyingMaterial& vMasterKeyIn)
262262
return true;
263263
}
264264

265-
bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool)
265+
bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool)
266266
{
267+
if (!CanGetAddresses(internal)) {
268+
return false;
269+
}
270+
267271
if (!ReserveKeyFromKeyPool(index, keypool, internal)) {
268272
return false;
269273
}
274+
address = GetDestinationForKey(keypool.vchPubKey, type);
270275
return true;
271276
}
272277

273-
void LegacyScriptPubKeyMan::KeepDestination(int64_t index)
274-
{
275-
KeepKey(index);
276-
}
277-
278-
void LegacyScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey)
279-
{
280-
ReturnKey(index, internal, pubkey);
281-
}
282-
283278
void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
284279
{
285280
AssertLockHeld(cs_wallet);
@@ -460,7 +455,7 @@ size_t LegacyScriptPubKeyMan::KeypoolCountExternalKeys()
460455
unsigned int LegacyScriptPubKeyMan::GetKeyPoolSize() const
461456
{
462457
AssertLockHeld(cs_wallet);
463-
return setInternalKeyPool.size() + setExternalKeyPool.size();
458+
return setInternalKeyPool.size() + setExternalKeyPool.size() + set_pre_split_keypool.size();
464459
}
465460

466461
int64_t LegacyScriptPubKeyMan::GetTimeFirstKey() const
@@ -1092,15 +1087,20 @@ void LegacyScriptPubKeyMan::AddKeypoolPubkeyWithDB(const CPubKey& pubkey, const
10921087
m_pool_key_to_index[pubkey.GetID()] = index;
10931088
}
10941089

1095-
void LegacyScriptPubKeyMan::KeepKey(int64_t nIndex)
1090+
void LegacyScriptPubKeyMan::KeepDestination(int64_t nIndex, const OutputType& type)
10961091
{
10971092
// Remove from key pool
10981093
WalletBatch batch(m_storage.GetDatabase());
10991094
batch.ErasePool(nIndex);
1095+
CPubKey pubkey;
1096+
bool have_pk = GetPubKey(m_index_to_reserved_key.at(nIndex), pubkey);
1097+
assert(have_pk);
1098+
LearnRelatedScripts(pubkey, type);
1099+
m_index_to_reserved_key.erase(nIndex);
11001100
WalletLogPrintf("keypool keep %d\n", nIndex);
11011101
}
11021102

1103-
void LegacyScriptPubKeyMan::ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey)
1103+
void LegacyScriptPubKeyMan::ReturnDestination(int64_t nIndex, bool fInternal, const CTxDestination&)
11041104
{
11051105
// Return to key pool
11061106
{
@@ -1112,13 +1112,15 @@ void LegacyScriptPubKeyMan::ReturnKey(int64_t nIndex, bool fInternal, const CPub
11121112
} else {
11131113
setExternalKeyPool.insert(nIndex);
11141114
}
1115-
m_pool_key_to_index[pubkey.GetID()] = nIndex;
1115+
CKeyID& pubkey_id = m_index_to_reserved_key.at(nIndex);
1116+
m_pool_key_to_index[pubkey_id] = nIndex;
1117+
m_index_to_reserved_key.erase(nIndex);
11161118
NotifyCanGetAddressesChanged();
11171119
}
11181120
WalletLogPrintf("keypool return %d\n", nIndex);
11191121
}
11201122

1121-
bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, bool internal)
1123+
bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, const OutputType type, bool internal)
11221124
{
11231125
if (!CanGetAddresses(internal)) {
11241126
return false;
@@ -1134,7 +1136,7 @@ bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, bool internal)
11341136
result = GenerateNewKey(batch, internal);
11351137
return true;
11361138
}
1137-
KeepKey(nIndex);
1139+
KeepDestination(nIndex, type);
11381140
result = keypool.vchPubKey;
11391141
}
11401142
return true;
@@ -1179,6 +1181,8 @@ bool LegacyScriptPubKeyMan::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& key
11791181
throw std::runtime_error(std::string(__func__) + ": keypool entry invalid");
11801182
}
11811183

1184+
assert(m_index_to_reserved_key.count(nIndex) == 0);
1185+
m_index_to_reserved_key[nIndex] = keypool.vchPubKey.GetID();
11821186
m_pool_key_to_index.erase(keypool.vchPubKey.GetID());
11831187
WalletLogPrintf("keypool reserve %d\n", nIndex);
11841188
}

‎src/wallet/scriptpubkeyman.h

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -150,9 +150,9 @@ class ScriptPubKeyMan
150150
virtual bool GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error) { return false; }
151151
virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; }
152152

153-
virtual bool GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) { return false; }
154-
virtual void KeepDestination(int64_t index) {}
155-
virtual void ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey) {}
153+
virtual bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) { return false; }
154+
virtual void KeepDestination(int64_t index, const OutputType& type) {}
155+
virtual void ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) {}
156156

157157
virtual bool TopUp(unsigned int size = 0) { return false; }
158158

@@ -246,9 +246,11 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
246246
std::set<int64_t> set_pre_split_keypool GUARDED_BY(cs_wallet);
247247
int64_t m_max_keypool_index GUARDED_BY(cs_wallet) = 0;
248248
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_index_to_reserved_key;
249251

250252
//! Fetches a key from the keypool
251-
bool GetKeyFromPool(CPubKey &key, bool internal = false);
253+
bool GetKeyFromPool(CPubKey &key, const OutputType type, bool internal = false);
252254

253255
/**
254256
* Reserves a key from the keypool and sets nIndex to its index
@@ -266,19 +268,16 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
266268
*/
267269
bool ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRequestedInternal);
268270

269-
void KeepKey(int64_t nIndex);
270-
void ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey);
271-
272271
public:
273272
bool GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error) override;
274273
isminetype IsMine(const CScript& script) const override;
275274

276275
//! will encrypt previously unencrypted keys
277276
bool EncryptKeys(CKeyingMaterial& vMasterKeyIn);
278277

279-
bool GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) override;
280-
void KeepDestination(int64_t index) override;
281-
void ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey) override;
278+
bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) override;
279+
void KeepDestination(int64_t index, const OutputType& type) override;
280+
void ReturnDestination(int64_t index, bool internal, const CTxDestination&) override;
282281

283282
bool TopUp(unsigned int size = 0) override;
284283

‎src/wallet/wallet.cpp

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3295,43 +3295,34 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool inter
32953295
return false;
32963296
}
32973297

3298-
if (!pwallet->CanGetAddresses(internal)) {
3299-
return false;
3300-
}
33013298

33023299
if (nIndex == -1)
33033300
{
33043301
CKeyPool keypool;
3305-
if (!m_spk_man->GetReservedDestination(type, internal, nIndex, keypool)) {
3302+
if (!m_spk_man->GetReservedDestination(type, internal, address, nIndex, keypool)) {
33063303
return false;
33073304
}
3308-
vchPubKey = keypool.vchPubKey;
33093305
fInternal = keypool.fInternal;
33103306
}
3311-
assert(vchPubKey.IsValid());
3312-
address = GetDestinationForKey(vchPubKey, type);
33133307
dest = address;
33143308
return true;
33153309
}
33163310

33173311
void ReserveDestination::KeepDestination()
33183312
{
33193313
if (nIndex != -1) {
3320-
m_spk_man->KeepDestination(nIndex);
3321-
m_spk_man->LearnRelatedScripts(vchPubKey, type);
3314+
m_spk_man->KeepDestination(nIndex, type);
33223315
}
33233316
nIndex = -1;
3324-
vchPubKey = CPubKey();
33253317
address = CNoDestination();
33263318
}
33273319

33283320
void ReserveDestination::ReturnDestination()
33293321
{
33303322
if (nIndex != -1) {
3331-
m_spk_man->ReturnDestination(nIndex, fInternal, vchPubKey);
3323+
m_spk_man->ReturnDestination(nIndex, fInternal, address);
33323324
}
33333325
nIndex = -1;
3334-
vchPubKey = CPubKey();
33353326
address = CNoDestination();
33363327
}
33373328

‎src/wallet/wallet.h

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -139,12 +139,11 @@ class ReserveDestination
139139
protected:
140140
//! The wallet to reserve from
141141
CWallet* const pwallet;
142-
LegacyScriptPubKeyMan* m_spk_man{nullptr};
142+
//! The ScriptPubKeyMan to reserve from. Based on type when GetReservedDestination is called
143+
ScriptPubKeyMan* m_spk_man{nullptr};
143144
OutputType const type;
144145
//! The index of the address's key in the keypool
145146
int64_t nIndex{-1};
146-
//! The public key for the address
147-
CPubKey vchPubKey;
148147
//! The destination
149148
CTxDestination address;
150149
//! Whether this is from the internal (change output) keypool

0 commit comments

Comments
 (0)