Skip to content

Commit 5cfdda2

Browse files
sipasipa
committed
Merge bitcoin#10235: Track keypool entries as internal vs external in memory
d40a72c Clarify *(--.end()) iterator semantics in CWallet::TopUpKeyPool (Matt Corallo) 28301b9 Meet code style on lines changed in the previous commit (Matt Corallo) 4a3fc35 Track keypool entries as internal vs external in memory (Matt Corallo) Pull request description: This is an alternative version of bitcoin#10184. As @jonasschnelli points out there, the performance regressions are pretty minimal, but given that this is a pretty simple, mechanical change, its probably worth doing. Tree-SHA512: e83f9ebf2998f8164d1b2eebe5e6dcdeadea8c30b7612861f830758c08bf4093cd6a67b3bcfa9cfcb139e5e0b106fc8898a975fc69f334981aefc756568ab613
2 parents c5904e8 + d40a72c commit 5cfdda2

2 files changed

Lines changed: 115 additions & 103 deletions

File tree

‎src/wallet/wallet.cpp‎

Lines changed: 102 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -2977,7 +2977,8 @@ DBErrors CWallet::LoadWallet(bool& fFirstRunRet)
29772977
if (dbw->Rewrite("\x04pool"))
29782978
{
29792979
LOCK(cs_wallet);
2980-
setKeyPool.clear();
2980+
setInternalKeyPool.clear();
2981+
setExternalKeyPool.clear();
29812982
// Note: can't top-up keypool here, because wallet is locked.
29822983
// User will be prompted to unlock wallet the next operation
29832984
// that requires a new key.
@@ -3005,7 +3006,8 @@ DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256
30053006
{
30063007
if (dbw->Rewrite("\x04pool"))
30073008
{
3008-
setKeyPool.clear();
3009+
setInternalKeyPool.clear();
3010+
setExternalKeyPool.clear();
30093011
// Note: can't top-up keypool here, because wallet is locked.
30103012
// User will be prompted to unlock wallet the next operation
30113013
// that requires a new key.
@@ -3030,7 +3032,8 @@ DBErrors CWallet::ZapWalletTx(std::vector<CWalletTx>& vWtx)
30303032
if (dbw->Rewrite("\x04pool"))
30313033
{
30323034
LOCK(cs_wallet);
3033-
setKeyPool.clear();
3035+
setInternalKeyPool.clear();
3036+
setExternalKeyPool.clear();
30343037
// Note: can't top-up keypool here, because wallet is locked.
30353038
// User will be prompted to unlock wallet the next operation
30363039
// that requires a new key.
@@ -3114,9 +3117,16 @@ bool CWallet::NewKeyPool()
31143117
{
31153118
LOCK(cs_wallet);
31163119
CWalletDB walletdb(*dbw);
3117-
for (int64_t nIndex : setKeyPool)
3120+
3121+
for (int64_t nIndex : setInternalKeyPool) {
3122+
walletdb.ErasePool(nIndex);
3123+
}
3124+
setInternalKeyPool.clear();
3125+
3126+
for (int64_t nIndex : setExternalKeyPool) {
31183127
walletdb.ErasePool(nIndex);
3119-
setKeyPool.clear();
3128+
}
3129+
setExternalKeyPool.clear();
31203130

31213131
if (!TopUpKeyPool()) {
31223132
return false;
@@ -3128,25 +3138,8 @@ bool CWallet::NewKeyPool()
31283138

31293139
size_t CWallet::KeypoolCountExternalKeys()
31303140
{
3131-
AssertLockHeld(cs_wallet); // setKeyPool
3132-
3133-
// immediately return setKeyPool's size if HD or HD_SPLIT is disabled or not supported
3134-
if (!IsHDEnabled() || !CanSupportFeature(FEATURE_HD_SPLIT))
3135-
return setKeyPool.size();
3136-
3137-
CWalletDB walletdb(*dbw);
3138-
3139-
// count amount of external keys
3140-
size_t amountE = 0;
3141-
for(const int64_t& id : setKeyPool)
3142-
{
3143-
CKeyPool tmpKeypool;
3144-
if (!walletdb.ReadPool(id, tmpKeypool))
3145-
throw std::runtime_error(std::string(__func__) + ": read failed");
3146-
amountE += !tmpKeypool.fInternal;
3147-
}
3148-
3149-
return amountE;
3141+
AssertLockHeld(cs_wallet); // setExternalKeyPool
3142+
return setExternalKeyPool.size();
31503143
}
31513144

31523145
bool CWallet::TopUpKeyPool(unsigned int kpSize)
@@ -3166,10 +3159,8 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize)
31663159

31673160
// count amount of available keys (internal, external)
31683161
// make sure the keypool of external and internal keys fits the user selected target (-keypool)
3169-
int64_t amountExternal = KeypoolCountExternalKeys();
3170-
int64_t amountInternal = setKeyPool.size() - amountExternal;
3171-
int64_t missingExternal = std::max(std::max((int64_t) nTargetSize, (int64_t) 1) - amountExternal, (int64_t) 0);
3172-
int64_t missingInternal = std::max(std::max((int64_t) nTargetSize, (int64_t) 1) - amountInternal, (int64_t) 0);
3162+
int64_t missingExternal = std::max(std::max((int64_t) nTargetSize, (int64_t) 1) - (int64_t)setExternalKeyPool.size(), (int64_t) 0);
3163+
int64_t missingInternal = std::max(std::max((int64_t) nTargetSize, (int64_t) 1) - (int64_t)setInternalKeyPool.size(), (int64_t) 0);
31733164

31743165
if (!IsHDEnabled() || !CanSupportFeature(FEATURE_HD_SPLIT))
31753166
{
@@ -3181,20 +3172,32 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize)
31813172
for (int64_t i = missingInternal + missingExternal; i--;)
31823173
{
31833174
int64_t nEnd = 1;
3184-
if (i < missingInternal)
3175+
if (i < missingInternal) {
31853176
internal = true;
3186-
if (!setKeyPool.empty())
3187-
nEnd = *(--setKeyPool.end()) + 1;
3177+
}
3178+
3179+
if (!setInternalKeyPool.empty()) {
3180+
nEnd = *(setInternalKeyPool.rbegin()) + 1;
3181+
}
3182+
if (!setExternalKeyPool.empty()) {
3183+
nEnd = std::max(nEnd, *(setExternalKeyPool.rbegin()) + 1);
3184+
}
3185+
31883186
if (!walletdb.WritePool(nEnd, CKeyPool(GenerateNewKey(internal), internal)))
31893187
throw std::runtime_error(std::string(__func__) + ": writing generated key failed");
3190-
setKeyPool.insert(nEnd);
3191-
LogPrintf("keypool added key %d, size=%u, internal=%d\n", nEnd, setKeyPool.size(), internal);
3188+
3189+
if (internal) {
3190+
setInternalKeyPool.insert(nEnd);
3191+
} else {
3192+
setExternalKeyPool.insert(nEnd);
3193+
}
3194+
LogPrintf("keypool added key %d, size=%u (%u internal), new key is %s\n", nEnd, setInternalKeyPool.size() + setExternalKeyPool.size(), setInternalKeyPool.size(), internal ? "internal" : "external");
31923195
}
31933196
}
31943197
return true;
31953198
}
31963199

3197-
void CWallet::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool internal)
3200+
void CWallet::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRequestedInternal)
31983201
{
31993202
nIndex = -1;
32003203
keypool.vchPubKey = CPubKey();
@@ -3204,30 +3207,30 @@ void CWallet::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool int
32043207
if (!IsLocked())
32053208
TopUpKeyPool();
32063209

3210+
bool fReturningInternal = IsHDEnabled() && CanSupportFeature(FEATURE_HD_SPLIT) && fRequestedInternal;
3211+
std::set<int64_t>& setKeyPool = fReturningInternal ? setInternalKeyPool : setExternalKeyPool;
3212+
32073213
// Get the oldest key
32083214
if(setKeyPool.empty())
32093215
return;
32103216

32113217
CWalletDB walletdb(*dbw);
32123218

3213-
// try to find a key that matches the internal/external filter
3214-
for(const int64_t& id : setKeyPool)
3215-
{
3216-
CKeyPool tmpKeypool;
3217-
if (!walletdb.ReadPool(id, tmpKeypool))
3218-
throw std::runtime_error(std::string(__func__) + ": read failed");
3219-
if (!HaveKey(tmpKeypool.vchPubKey.GetID()))
3220-
throw std::runtime_error(std::string(__func__) + ": unknown key in key pool");
3221-
if (!IsHDEnabled() || !CanSupportFeature(FEATURE_HD_SPLIT) || tmpKeypool.fInternal == internal)
3222-
{
3223-
nIndex = id;
3224-
keypool = tmpKeypool;
3225-
setKeyPool.erase(id);
3226-
assert(keypool.vchPubKey.IsValid());
3227-
LogPrintf("keypool reserve %d\n", nIndex);
3228-
return;
3229-
}
3219+
auto it = setKeyPool.begin();
3220+
nIndex = *it;
3221+
setKeyPool.erase(it);
3222+
if (!walletdb.ReadPool(nIndex, keypool)) {
3223+
throw std::runtime_error(std::string(__func__) + ": read failed");
3224+
}
3225+
if (!HaveKey(keypool.vchPubKey.GetID())) {
3226+
throw std::runtime_error(std::string(__func__) + ": unknown key in key pool");
32303227
}
3228+
if (keypool.fInternal != fReturningInternal) {
3229+
throw std::runtime_error(std::string(__func__) + ": keypool entry misclassified");
3230+
}
3231+
3232+
assert(keypool.vchPubKey.IsValid());
3233+
LogPrintf("keypool reserve %d\n", nIndex);
32313234
}
32323235
}
32333236

@@ -3239,12 +3242,16 @@ void CWallet::KeepKey(int64_t nIndex)
32393242
LogPrintf("keypool keep %d\n", nIndex);
32403243
}
32413244

3242-
void CWallet::ReturnKey(int64_t nIndex)
3245+
void CWallet::ReturnKey(int64_t nIndex, bool fInternal)
32433246
{
32443247
// Return to key pool
32453248
{
32463249
LOCK(cs_wallet);
3247-
setKeyPool.insert(nIndex);
3250+
if (fInternal) {
3251+
setInternalKeyPool.insert(nIndex);
3252+
} else {
3253+
setExternalKeyPool.insert(nIndex);
3254+
}
32483255
}
32493256
LogPrintf("keypool return %d\n", nIndex);
32503257
}
@@ -3268,48 +3275,35 @@ bool CWallet::GetKeyFromPool(CPubKey& result, bool internal)
32683275
return true;
32693276
}
32703277

3271-
int64_t CWallet::GetOldestKeyPoolTime()
3272-
{
3273-
LOCK(cs_wallet);
3274-
3275-
// if the keypool is empty, return <NOW>
3276-
if (setKeyPool.empty())
3278+
static int64_t GetOldestKeyTimeInPool(const std::set<int64_t>& setKeyPool, CWalletDB& walletdb) {
3279+
if (setKeyPool.empty()) {
32773280
return GetTime();
3281+
}
32783282

32793283
CKeyPool keypool;
3280-
CWalletDB walletdb(*dbw);
3281-
3282-
if (IsHDEnabled() && CanSupportFeature(FEATURE_HD_SPLIT))
3283-
{
3284-
// if HD & HD Chain Split is enabled, response max(oldest-internal-key, oldest-external-key)
3285-
int64_t now = GetTime();
3286-
int64_t oldest_external = now, oldest_internal = now;
3287-
3288-
for(const int64_t& id : setKeyPool)
3289-
{
3290-
if (!walletdb.ReadPool(id, keypool)) {
3291-
throw std::runtime_error(std::string(__func__) + ": read failed");
3292-
}
3293-
if (keypool.fInternal && keypool.nTime < oldest_internal) {
3294-
oldest_internal = keypool.nTime;
3295-
}
3296-
else if (!keypool.fInternal && keypool.nTime < oldest_external) {
3297-
oldest_external = keypool.nTime;
3298-
}
3299-
if (oldest_internal != now && oldest_external != now) {
3300-
break;
3301-
}
3302-
}
3303-
return std::max(oldest_internal, oldest_external);
3304-
}
3305-
// load oldest key from keypool, get time and return
33063284
int64_t nIndex = *(setKeyPool.begin());
3307-
if (!walletdb.ReadPool(nIndex, keypool))
3285+
if (!walletdb.ReadPool(nIndex, keypool)) {
33083286
throw std::runtime_error(std::string(__func__) + ": read oldest key in keypool failed");
3287+
}
33093288
assert(keypool.vchPubKey.IsValid());
33103289
return keypool.nTime;
33113290
}
33123291

3292+
int64_t CWallet::GetOldestKeyPoolTime()
3293+
{
3294+
LOCK(cs_wallet);
3295+
3296+
CWalletDB walletdb(*dbw);
3297+
3298+
// load oldest key from keypool, get time and return
3299+
int64_t oldestKey = GetOldestKeyTimeInPool(setExternalKeyPool, walletdb);
3300+
if (IsHDEnabled() && CanSupportFeature(FEATURE_HD_SPLIT)) {
3301+
oldestKey = std::max(GetOldestKeyTimeInPool(setInternalKeyPool, walletdb), oldestKey);
3302+
}
3303+
3304+
return oldestKey;
3305+
}
3306+
33133307
std::map<CTxDestination, CAmount> CWallet::GetAddressBalances()
33143308
{
33153309
std::map<CTxDestination, CAmount> balances;
@@ -3468,6 +3462,7 @@ bool CReserveKey::GetReservedKey(CPubKey& pubkey, bool internal)
34683462
else {
34693463
return false;
34703464
}
3465+
fInternal = keypool.fInternal;
34713466
}
34723467
assert(vchPubKey.IsValid());
34733468
pubkey = vchPubKey;
@@ -3484,32 +3479,42 @@ void CReserveKey::KeepKey()
34843479

34853480
void CReserveKey::ReturnKey()
34863481
{
3487-
if (nIndex != -1)
3488-
pwallet->ReturnKey(nIndex);
3482+
if (nIndex != -1) {
3483+
pwallet->ReturnKey(nIndex, fInternal);
3484+
}
34893485
nIndex = -1;
34903486
vchPubKey = CPubKey();
34913487
}
34923488

3493-
void CWallet::GetAllReserveKeys(std::set<CKeyID>& setAddress) const
3494-
{
3495-
setAddress.clear();
3496-
3497-
CWalletDB walletdb(*dbw);
3498-
3499-
LOCK2(cs_main, cs_wallet);
3489+
static void LoadReserveKeysToSet(std::set<CKeyID>& setAddress, const std::set<int64_t>& setKeyPool, CWalletDB& walletdb) {
35003490
for (const int64_t& id : setKeyPool)
35013491
{
35023492
CKeyPool keypool;
35033493
if (!walletdb.ReadPool(id, keypool))
35043494
throw std::runtime_error(std::string(__func__) + ": read failed");
35053495
assert(keypool.vchPubKey.IsValid());
35063496
CKeyID keyID = keypool.vchPubKey.GetID();
3507-
if (!HaveKey(keyID))
3508-
throw std::runtime_error(std::string(__func__) + ": unknown key in key pool");
35093497
setAddress.insert(keyID);
35103498
}
35113499
}
35123500

3501+
void CWallet::GetAllReserveKeys(std::set<CKeyID>& setAddress) const
3502+
{
3503+
setAddress.clear();
3504+
3505+
CWalletDB walletdb(*dbw);
3506+
3507+
LOCK2(cs_main, cs_wallet);
3508+
LoadReserveKeysToSet(setAddress, setInternalKeyPool, walletdb);
3509+
LoadReserveKeysToSet(setAddress, setExternalKeyPool, walletdb);
3510+
3511+
for (const CKeyID& keyID : setAddress) {
3512+
if (!HaveKey(keyID)) {
3513+
throw std::runtime_error(std::string(__func__) + ": unknown key in key pool");
3514+
}
3515+
}
3516+
}
3517+
35133518
void CWallet::GetScriptForMining(std::shared_ptr<CReserveScript> &script)
35143519
{
35153520
std::shared_ptr<CReserveKey> rKey = std::make_shared<CReserveKey>(this);

‎src/wallet/wallet.h‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -699,7 +699,8 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
699699
/* HD derive new child key (on internal or external chain) */
700700
void DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret, bool internal = false);
701701

702-
std::set<int64_t> setKeyPool;
702+
std::set<int64_t> setInternalKeyPool;
703+
std::set<int64_t> setExternalKeyPool;
703704

704705
int64_t nTimeFirstKey;
705706

@@ -744,7 +745,11 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
744745

745746
void LoadKeyPool(int nIndex, const CKeyPool &keypool)
746747
{
747-
setKeyPool.insert(nIndex);
748+
if (keypool.fInternal) {
749+
setInternalKeyPool.insert(nIndex);
750+
} else {
751+
setExternalKeyPool.insert(nIndex);
752+
}
748753

749754
// If no metadata exists yet, create a default with the pool key's
750755
// creation time. Note that this may be overwritten by actually
@@ -974,9 +979,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
974979
bool NewKeyPool();
975980
size_t KeypoolCountExternalKeys();
976981
bool TopUpKeyPool(unsigned int kpSize = 0);
977-
void ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool internal);
982+
void ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRequestedInternal);
978983
void KeepKey(int64_t nIndex);
979-
void ReturnKey(int64_t nIndex);
984+
void ReturnKey(int64_t nIndex, bool fInternal);
980985
bool GetKeyFromPool(CPubKey &key, bool internal = false);
981986
int64_t GetOldestKeyPoolTime();
982987
void GetAllReserveKeys(std::set<CKeyID>& setAddress) const;
@@ -1030,8 +1035,8 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
10301035

10311036
unsigned int GetKeyPoolSize()
10321037
{
1033-
AssertLockHeld(cs_wallet); // setKeyPool
1034-
return setKeyPool.size();
1038+
AssertLockHeld(cs_wallet); // set{Ex,In}ternalKeyPool
1039+
return setInternalKeyPool.size() + setExternalKeyPool.size();
10351040
}
10361041

10371042
bool SetDefaultKey(const CPubKey &vchPubKey);
@@ -1135,11 +1140,13 @@ class CReserveKey : public CReserveScript
11351140
CWallet* pwallet;
11361141
int64_t nIndex;
11371142
CPubKey vchPubKey;
1143+
bool fInternal;
11381144
public:
11391145
CReserveKey(CWallet* pwalletIn)
11401146
{
11411147
nIndex = -1;
11421148
pwallet = pwalletIn;
1149+
fInternal = false;
11431150
}
11441151

11451152
CReserveKey() = default;

0 commit comments

Comments
 (0)