From 4a3fc35629a2820cecd15898f112fa730e5adcec Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 19 Apr 2017 12:55:32 -0400 Subject: [PATCH 1/3] Track keypool entries as internal vs external in memory This resolves a super minor performance regressions in several keypool-handling functions --- src/wallet/wallet.cpp | 183 ++++++++++++++++++++---------------------- src/wallet/wallet.h | 19 +++-- 2 files changed, 102 insertions(+), 100 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 58153dfc3..cc4d99d30 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2941,7 +2941,8 @@ DBErrors CWallet::LoadWallet(bool& fFirstRunRet) if (dbw->Rewrite("\x04pool")) { LOCK(cs_wallet); - setKeyPool.clear(); + setInternalKeyPool.clear(); + setExternalKeyPool.clear(); // Note: can't top-up keypool here, because wallet is locked. // User will be prompted to unlock wallet the next operation // that requires a new key. @@ -2969,7 +2970,8 @@ DBErrors CWallet::ZapSelectTx(std::vector& vHashIn, std::vectorRewrite("\x04pool")) { - setKeyPool.clear(); + setInternalKeyPool.clear(); + setExternalKeyPool.clear(); // Note: can't top-up keypool here, because wallet is locked. // User will be prompted to unlock wallet the next operation // that requires a new key. @@ -2994,7 +2996,8 @@ DBErrors CWallet::ZapWalletTx(std::vector& vWtx) if (dbw->Rewrite("\x04pool")) { LOCK(cs_wallet); - setKeyPool.clear(); + setInternalKeyPool.clear(); + setExternalKeyPool.clear(); // Note: can't top-up keypool here, because wallet is locked. // User will be prompted to unlock wallet the next operation // that requires a new key. @@ -3078,9 +3081,12 @@ bool CWallet::NewKeyPool() { LOCK(cs_wallet); CWalletDB walletdb(*dbw); - for (int64_t nIndex : setKeyPool) + for (int64_t nIndex : setInternalKeyPool) walletdb.ErasePool(nIndex); - setKeyPool.clear(); + setInternalKeyPool.clear(); + BOOST_FOREACH(int64_t nIndex, setExternalKeyPool) + walletdb.ErasePool(nIndex); + setExternalKeyPool.clear(); if (!TopUpKeyPool()) { return false; @@ -3092,25 +3098,8 @@ bool CWallet::NewKeyPool() size_t CWallet::KeypoolCountExternalKeys() { - AssertLockHeld(cs_wallet); // setKeyPool - - // immediately return setKeyPool's size if HD or HD_SPLIT is disabled or not supported - if (!IsHDEnabled() || !CanSupportFeature(FEATURE_HD_SPLIT)) - return setKeyPool.size(); - - CWalletDB walletdb(*dbw); - - // count amount of external keys - size_t amountE = 0; - for(const int64_t& id : setKeyPool) - { - CKeyPool tmpKeypool; - if (!walletdb.ReadPool(id, tmpKeypool)) - throw std::runtime_error(std::string(__func__) + ": read failed"); - amountE += !tmpKeypool.fInternal; - } - - return amountE; + AssertLockHeld(cs_wallet); // setExternalKeyPool + return setExternalKeyPool.size(); } bool CWallet::TopUpKeyPool(unsigned int kpSize) @@ -3130,10 +3119,8 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize) // count amount of available keys (internal, external) // make sure the keypool of external and internal keys fits the user selected target (-keypool) - int64_t amountExternal = KeypoolCountExternalKeys(); - int64_t amountInternal = setKeyPool.size() - amountExternal; - int64_t missingExternal = std::max(std::max((int64_t) nTargetSize, (int64_t) 1) - amountExternal, (int64_t) 0); - int64_t missingInternal = std::max(std::max((int64_t) nTargetSize, (int64_t) 1) - amountInternal, (int64_t) 0); + int64_t missingExternal = std::max(std::max((int64_t) nTargetSize, (int64_t) 1) - (int64_t)setExternalKeyPool.size(), (int64_t) 0); + int64_t missingInternal = std::max(std::max((int64_t) nTargetSize, (int64_t) 1) - (int64_t)setInternalKeyPool.size(), (int64_t) 0); if (!IsHDEnabled() || !CanSupportFeature(FEATURE_HD_SPLIT)) { @@ -3147,18 +3134,26 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize) int64_t nEnd = 1; if (i < missingInternal) internal = true; - if (!setKeyPool.empty()) - nEnd = *(--setKeyPool.end()) + 1; + if (!setInternalKeyPool.empty()) + nEnd = *(--setInternalKeyPool.end()) + 1; + if (!setExternalKeyPool.empty()) + nEnd = std::max(nEnd, *(--setExternalKeyPool.end()) + 1); + if (!walletdb.WritePool(nEnd, CKeyPool(GenerateNewKey(internal), internal))) throw std::runtime_error(std::string(__func__) + ": writing generated key failed"); - setKeyPool.insert(nEnd); - LogPrintf("keypool added key %d, size=%u, internal=%d\n", nEnd, setKeyPool.size(), internal); + + if (internal) { + setInternalKeyPool.insert(nEnd); + } else { + setExternalKeyPool.insert(nEnd); + } + LogPrintf("keypool added key %d, size=%u (%u internal), new key is %s\n", nEnd, setInternalKeyPool.size() + setExternalKeyPool.size(), setInternalKeyPool.size(), internal ? "internal" : "external"); } } return true; } -void CWallet::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool internal) +void CWallet::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRequestedInternal) { nIndex = -1; keypool.vchPubKey = CPubKey(); @@ -3168,30 +3163,30 @@ void CWallet::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool int if (!IsLocked()) TopUpKeyPool(); + bool fReturningInternal = IsHDEnabled() && CanSupportFeature(FEATURE_HD_SPLIT) && fRequestedInternal; + std::set& setKeyPool = fReturningInternal ? setInternalKeyPool : setExternalKeyPool; + // Get the oldest key if(setKeyPool.empty()) return; CWalletDB walletdb(*dbw); - // try to find a key that matches the internal/external filter - for(const int64_t& id : setKeyPool) - { - CKeyPool tmpKeypool; - if (!walletdb.ReadPool(id, tmpKeypool)) - throw std::runtime_error(std::string(__func__) + ": read failed"); - if (!HaveKey(tmpKeypool.vchPubKey.GetID())) - throw std::runtime_error(std::string(__func__) + ": unknown key in key pool"); - if (!IsHDEnabled() || !CanSupportFeature(FEATURE_HD_SPLIT) || tmpKeypool.fInternal == internal) - { - nIndex = id; - keypool = tmpKeypool; - setKeyPool.erase(id); - assert(keypool.vchPubKey.IsValid()); - LogPrintf("keypool reserve %d\n", nIndex); - return; - } + auto it = setKeyPool.begin(); + nIndex = *it; + setKeyPool.erase(it); + if (!walletdb.ReadPool(nIndex, keypool)) { + throw std::runtime_error(std::string(__func__) + ": read failed"); } + if (!HaveKey(keypool.vchPubKey.GetID())) { + throw std::runtime_error(std::string(__func__) + ": unknown key in key pool"); + } + if (keypool.fInternal != fReturningInternal) { + throw std::runtime_error(std::string(__func__) + ": keypool entry misclassified"); + } + + assert(keypool.vchPubKey.IsValid()); + LogPrintf("keypool reserve %d\n", nIndex); } } @@ -3203,12 +3198,16 @@ void CWallet::KeepKey(int64_t nIndex) LogPrintf("keypool keep %d\n", nIndex); } -void CWallet::ReturnKey(int64_t nIndex) +void CWallet::ReturnKey(int64_t nIndex, bool fInternal) { // Return to key pool { LOCK(cs_wallet); - setKeyPool.insert(nIndex); + if (fInternal) { + setInternalKeyPool.insert(nIndex); + } else { + setExternalKeyPool.insert(nIndex); + } } LogPrintf("keypool return %d\n", nIndex); } @@ -3232,41 +3231,12 @@ bool CWallet::GetKeyFromPool(CPubKey& result, bool internal) return true; } -int64_t CWallet::GetOldestKeyPoolTime() -{ - LOCK(cs_wallet); - - // if the keypool is empty, return - if (setKeyPool.empty()) +static int64_t GetOldestKeyTimeInPool(const std::set& setKeyPool, CWalletDB& walletdb) { + if (setKeyPool.empty()) { return GetTime(); + } CKeyPool keypool; - CWalletDB walletdb(*dbw); - - if (IsHDEnabled() && CanSupportFeature(FEATURE_HD_SPLIT)) - { - // if HD & HD Chain Split is enabled, response max(oldest-internal-key, oldest-external-key) - int64_t now = GetTime(); - int64_t oldest_external = now, oldest_internal = now; - - for(const int64_t& id : setKeyPool) - { - if (!walletdb.ReadPool(id, keypool)) { - throw std::runtime_error(std::string(__func__) + ": read failed"); - } - if (keypool.fInternal && keypool.nTime < oldest_internal) { - oldest_internal = keypool.nTime; - } - else if (!keypool.fInternal && keypool.nTime < oldest_external) { - oldest_external = keypool.nTime; - } - if (oldest_internal != now && oldest_external != now) { - break; - } - } - return std::max(oldest_internal, oldest_external); - } - // load oldest key from keypool, get time and return int64_t nIndex = *(setKeyPool.begin()); if (!walletdb.ReadPool(nIndex, keypool)) throw std::runtime_error(std::string(__func__) + ": read oldest key in keypool failed"); @@ -3274,6 +3244,21 @@ int64_t CWallet::GetOldestKeyPoolTime() return keypool.nTime; } +int64_t CWallet::GetOldestKeyPoolTime() +{ + LOCK(cs_wallet); + + CWalletDB walletdb(*dbw); + + // load oldest key from keypool, get time and return + int64_t oldestKey = GetOldestKeyTimeInPool(setExternalKeyPool, walletdb); + if (IsHDEnabled() && CanSupportFeature(FEATURE_HD_SPLIT)) { + oldestKey = std::max(GetOldestKeyTimeInPool(setInternalKeyPool, walletdb), oldestKey); + } + + return oldestKey; +} + std::map CWallet::GetAddressBalances() { std::map balances; @@ -3432,6 +3417,7 @@ bool CReserveKey::GetReservedKey(CPubKey& pubkey, bool internal) else { return false; } + fInternal = keypool.fInternal; } assert(vchPubKey.IsValid()); pubkey = vchPubKey; @@ -3449,11 +3435,23 @@ void CReserveKey::KeepKey() void CReserveKey::ReturnKey() { if (nIndex != -1) - pwallet->ReturnKey(nIndex); + pwallet->ReturnKey(nIndex, fInternal); nIndex = -1; vchPubKey = CPubKey(); } +static void LoadReserveKeysToSet(std::set& setAddress, const std::set& setKeyPool, CWalletDB& walletdb) { + for (const int64_t& id : setKeyPool) + { + CKeyPool keypool; + if (!walletdb.ReadPool(id, keypool)) + throw std::runtime_error(std::string(__func__) + ": read failed"); + assert(keypool.vchPubKey.IsValid()); + CKeyID keyID = keypool.vchPubKey.GetID(); + setAddress.insert(keyID); + } +} + void CWallet::GetAllReserveKeys(std::set& setAddress) const { setAddress.clear(); @@ -3461,16 +3459,13 @@ void CWallet::GetAllReserveKeys(std::set& setAddress) const CWalletDB walletdb(*dbw); LOCK2(cs_main, cs_wallet); - for (const int64_t& id : setKeyPool) - { - CKeyPool keypool; - if (!walletdb.ReadPool(id, keypool)) - throw std::runtime_error(std::string(__func__) + ": read failed"); - assert(keypool.vchPubKey.IsValid()); - CKeyID keyID = keypool.vchPubKey.GetID(); - if (!HaveKey(keyID)) + LoadReserveKeysToSet(setAddress, setInternalKeyPool, walletdb); + LoadReserveKeysToSet(setAddress, setExternalKeyPool, walletdb); + + for (const CKeyID& keyID : setAddress) { + if (!HaveKey(keyID)) { throw std::runtime_error(std::string(__func__) + ": unknown key in key pool"); - setAddress.insert(keyID); + } } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index ad606b853..c1e05e861 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -696,7 +696,8 @@ private: /* HD derive new child key (on internal or external chain) */ void DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret, bool internal = false); - std::set setKeyPool; + std::set setInternalKeyPool; + std::set setExternalKeyPool; int64_t nTimeFirstKey; @@ -741,7 +742,11 @@ public: void LoadKeyPool(int nIndex, const CKeyPool &keypool) { - setKeyPool.insert(nIndex); + if (keypool.fInternal) { + setInternalKeyPool.insert(nIndex); + } else { + setExternalKeyPool.insert(nIndex); + } // If no metadata exists yet, create a default with the pool key's // creation time. Note that this may be overwritten by actually @@ -970,9 +975,9 @@ public: bool NewKeyPool(); size_t KeypoolCountExternalKeys(); bool TopUpKeyPool(unsigned int kpSize = 0); - void ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool internal); + void ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRequestedInternal); void KeepKey(int64_t nIndex); - void ReturnKey(int64_t nIndex); + void ReturnKey(int64_t nIndex, bool fInternal); bool GetKeyFromPool(CPubKey &key, bool internal = false); int64_t GetOldestKeyPoolTime(); void GetAllReserveKeys(std::set& setAddress) const; @@ -1026,8 +1031,8 @@ public: unsigned int GetKeyPoolSize() { - AssertLockHeld(cs_wallet); // setKeyPool - return setKeyPool.size(); + AssertLockHeld(cs_wallet); // set{Ex,In}ternalKeyPool + return setInternalKeyPool.size() + setExternalKeyPool.size(); } bool SetDefaultKey(const CPubKey &vchPubKey); @@ -1131,11 +1136,13 @@ protected: CWallet* pwallet; int64_t nIndex; CPubKey vchPubKey; + bool fInternal; public: CReserveKey(CWallet* pwalletIn) { nIndex = -1; pwallet = pwalletIn; + fInternal = false; } CReserveKey() = default; From 28301b9780b05b1d80767835987da1317ce4200a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 19 Apr 2017 13:11:16 -0400 Subject: [PATCH 2/3] Meet code style on lines changed in the previous commit --- src/wallet/wallet.cpp | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index cc4d99d30..61d975246 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3081,11 +3081,15 @@ bool CWallet::NewKeyPool() { LOCK(cs_wallet); CWalletDB walletdb(*dbw); - for (int64_t nIndex : setInternalKeyPool) + + for (int64_t nIndex : setInternalKeyPool) { walletdb.ErasePool(nIndex); + } setInternalKeyPool.clear(); - BOOST_FOREACH(int64_t nIndex, setExternalKeyPool) + + for (int64_t nIndex : setExternalKeyPool) { walletdb.ErasePool(nIndex); + } setExternalKeyPool.clear(); if (!TopUpKeyPool()) { @@ -3132,12 +3136,16 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize) for (int64_t i = missingInternal + missingExternal; i--;) { int64_t nEnd = 1; - if (i < missingInternal) + if (i < missingInternal) { internal = true; - if (!setInternalKeyPool.empty()) + } + + if (!setInternalKeyPool.empty()) { nEnd = *(--setInternalKeyPool.end()) + 1; - if (!setExternalKeyPool.empty()) + } + if (!setExternalKeyPool.empty()) { nEnd = std::max(nEnd, *(--setExternalKeyPool.end()) + 1); + } if (!walletdb.WritePool(nEnd, CKeyPool(GenerateNewKey(internal), internal))) throw std::runtime_error(std::string(__func__) + ": writing generated key failed"); @@ -3238,8 +3246,9 @@ static int64_t GetOldestKeyTimeInPool(const std::set& setKeyPool, CWall CKeyPool keypool; int64_t nIndex = *(setKeyPool.begin()); - if (!walletdb.ReadPool(nIndex, keypool)) + if (!walletdb.ReadPool(nIndex, keypool)) { throw std::runtime_error(std::string(__func__) + ": read oldest key in keypool failed"); + } assert(keypool.vchPubKey.IsValid()); return keypool.nTime; } @@ -3434,8 +3443,9 @@ void CReserveKey::KeepKey() void CReserveKey::ReturnKey() { - if (nIndex != -1) + if (nIndex != -1) { pwallet->ReturnKey(nIndex, fInternal); + } nIndex = -1; vchPubKey = CPubKey(); } From d40a72ccbb71d61b43cbf4d222ca2ab5d3ca7510 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 11 Jul 2017 12:02:42 -0400 Subject: [PATCH 3/3] Clarify *(--.end()) iterator semantics in CWallet::TopUpKeyPool --- src/wallet/wallet.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 61d975246..63d0a3c0c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3141,10 +3141,10 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize) } if (!setInternalKeyPool.empty()) { - nEnd = *(--setInternalKeyPool.end()) + 1; + nEnd = *(setInternalKeyPool.rbegin()) + 1; } if (!setExternalKeyPool.empty()) { - nEnd = std::max(nEnd, *(--setExternalKeyPool.end()) + 1); + nEnd = std::max(nEnd, *(setExternalKeyPool.rbegin()) + 1); } if (!walletdb.WritePool(nEnd, CKeyPool(GenerateNewKey(internal), internal)))