From 4b6a09bbd30acabdf6dc409e3a927d19e5a51aea Mon Sep 17 00:00:00 2001 From: fanquake Date: Fri, 6 Dec 2019 13:37:12 -0500 Subject: [PATCH] Merge #17373: wallet: Various fixes and cleanup to keypool handling in LegacyScriptPubKeyMan and CWallet 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 ACKs for top commit: ryanofsky: Code review ACK 886f1731bec4393dd342403ac34069a3a4f95eea. Only change is moving earlier fix to a better commit (same end result). promag: Code review ACK 886f1731bec4393dd342403ac34069a3a4f95eea. instagibbs: code review re-ACK https://github.com/bitcoin/bitcoin/pull/17373/commits/886f1731bec4393dd342403ac34069a3a4f95eea Sjors: Code review re-ACK 886f1731bec4393dd342403ac34069a3a4f95eea Tree-SHA512: f4be290759f63fdc920d5c02bd0d09acc4b06a5f053787d4afcd3c921b2e35d2bd97617fadae015da853dc189f559fb8d2c6e58d53e4cabfac9af151cd97ad19 --- src/wallet/scriptpubkeyman.cpp | 34 +++++++++++++++++++--------------- src/wallet/scriptpubkeyman.h | 14 +++++++------- src/wallet/wallet.cpp | 13 ++----------- src/wallet/wallet.h | 5 ++--- 4 files changed, 30 insertions(+), 36 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index aafde4adcd..ca1bd2f251 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -263,24 +263,19 @@ bool LegacyScriptPubKeyMan::EncryptKeys(CKeyingMaterial& vMasterKeyIn) return true; } -bool LegacyScriptPubKeyMan::GetReservedDestination(bool internal, int64_t& index, CKeyPool& keypool) +bool LegacyScriptPubKeyMan::GetReservedDestination(bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) { + if (!CanGetAddresses(internal)) { + return false; + } + if (!ReserveKeyFromKeyPool(index, keypool, internal)) { return false; } + address = keypool.vchPubKey.GetID(); return true; } -void LegacyScriptPubKeyMan::KeepDestination(int64_t index) -{ - KeepKey(index); -} - -void LegacyScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey) -{ - ReturnKey(index, internal, pubkey); -} - void LegacyScriptPubKeyMan::MarkUnusedAddresses(WalletBatch &batch, const CScript& script, const uint256& hashBlock) { AssertLockHeld(cs_wallet); @@ -1447,7 +1442,7 @@ void LegacyScriptPubKeyMan::AddKeypoolPubkeyWithDB(const CPubKey& pubkey, const m_pool_key_to_index[pubkey.GetID()] = index; } -void LegacyScriptPubKeyMan::KeepKey(int64_t nIndex) +void LegacyScriptPubKeyMan::KeepDestination(int64_t nIndex) { // Remove from key pool { @@ -1457,11 +1452,16 @@ void LegacyScriptPubKeyMan::KeepKey(int64_t nIndex) --m_wallet.nKeysLeftSinceAutoBackup; if (!nWalletBackups) m_wallet.nKeysLeftSinceAutoBackup = 0; + + CPubKey pubkey; + bool have_pk = GetPubKey(m_index_to_reserved_key.at(nIndex), pubkey); + assert(have_pk); + m_index_to_reserved_key.erase(nIndex); } WalletLogPrintf("keypool keep %d\n", nIndex); } -void LegacyScriptPubKeyMan::ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey) +void LegacyScriptPubKeyMan::ReturnDestination(int64_t nIndex, bool fInternal, const CTxDestination&) { // Return to key pool { @@ -1471,7 +1471,9 @@ void LegacyScriptPubKeyMan::ReturnKey(int64_t nIndex, bool fInternal, const CPub } else { setExternalKeyPool.insert(nIndex); } - 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); NotifyCanGetAddressesChanged(); } WalletLogPrintf("keypool return %d\n", nIndex); @@ -1494,7 +1496,7 @@ bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, bool internal) result = GenerateNewKey(batch, 0, internal); return true; } - KeepKey(nIndex); + KeepDestination(nIndex); result = keypool.vchPubKey; } return true; @@ -1536,6 +1538,8 @@ bool LegacyScriptPubKeyMan::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& key throw std::runtime_error(std::string(__func__) + ": keypool entry invalid"); } + assert(m_index_to_reserved_key.count(nIndex) == 0); + m_index_to_reserved_key[nIndex] = keypool.vchPubKey.GetID(); m_pool_key_to_index.erase(keypool.vchPubKey.GetID()); WalletLogPrintf("keypool reserve %d\n", nIndex); } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 44dfaa4d52..2f2eeab3e2 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -145,9 +145,9 @@ public: virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; } virtual isminetype IsMine(const CTxDestination& dest) const { return ISMINE_NO; } - virtual bool GetReservedDestination(bool internal, int64_t& index, CKeyPool& keypool) { return false; } + virtual bool GetReservedDestination(bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) { return false; } virtual void KeepDestination(int64_t index) {} - virtual void ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey) {} + virtual void ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) {} virtual bool TopUp(unsigned int size = 0) { return false; } @@ -232,9 +232,12 @@ private: std::set setExternalKeyPool GUARDED_BY(cs_wallet); int64_t m_max_keypool_index GUARDED_BY(cs_wallet) = 0; std::map m_pool_key_to_index; + // Tracks keypool indexes to CKeyIDs of keys that have been taken out of the keypool but may be returned to it + std::map m_index_to_reserved_key; //! Fetches a key from the keypool bool GetKeyFromPool(CPubKey &key, bool fInternal /*= false*/); + /** * Reserves a key from the keypool and sets nIndex to its index * @@ -251,9 +254,6 @@ private: */ bool ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRequestedInternal); - void KeepKey(int64_t nIndex); - void ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey); - public: bool GetNewDestination(CTxDestination& dest, std::string& error) override; isminetype IsMine(const CScript& script) const override; @@ -262,9 +262,9 @@ public: //! will encrypt previously unencrypted keys bool EncryptKeys(CKeyingMaterial& vMasterKeyIn); - bool GetReservedDestination(bool internal, int64_t& index, CKeyPool& keypool) override; + bool GetReservedDestination(bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) override; void KeepDestination(int64_t index) override; - void ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey) override; + void ReturnDestination(int64_t index, bool internal, const CTxDestination&) override; bool TopUp(unsigned int size = 0) override; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c2cefd30c6..05688461f0 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3966,21 +3966,14 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool fInte return false; } - if (!pwallet->CanGetAddresses(fInternalIn)) { - return false; - } - if (nIndex == -1) { CKeyPool keypool; - if (!m_spk_man->GetReservedDestination(fInternalIn, nIndex, keypool)) { + if (!m_spk_man->GetReservedDestination(fInternalIn, address, nIndex, keypool)) { return false; } - vchPubKey = keypool.vchPubKey; fInternal = keypool.fInternal; } - assert(vchPubKey.IsValid()); - address = vchPubKey.GetID(); dest = address; return true; } @@ -3991,17 +3984,15 @@ void ReserveDestination::KeepDestination() m_spk_man->KeepDestination(nIndex); } nIndex = -1; - vchPubKey = CPubKey(); address = CNoDestination(); } void ReserveDestination::ReturnDestination() { if (nIndex != -1) { - m_spk_man->ReturnDestination(nIndex, fInternal, vchPubKey); + m_spk_man->ReturnDestination(nIndex, fInternal, address); } nIndex = -1; - vchPubKey = CPubKey(); address = CNoDestination(); } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 37f10ac1aa..922ad18c08 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -149,12 +149,11 @@ class ReserveDestination : public CReserveScript protected: //! The wallet to reserve from CWallet* const pwallet; - LegacyScriptPubKeyMan* m_spk_man{nullptr}; + //! The ScriptPubKeyMan to reserve from. Based on type when GetReservedDestination is called + ScriptPubKeyMan* m_spk_man{nullptr}; //! The index of the address's key in the keypool int64_t nIndex{-1}; - //! The public key for the address - CPubKey vchPubKey; //! The destination CTxDestination address; //! Whether this is from the internal (change output) keypool