From 596f6460f9fd8273665c8754ccd673d93a4f25f0 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Oct 2019 14:11:34 -0400 Subject: 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 --- src/wallet/scriptpubkeyman.cpp | 4 ++++ src/wallet/wallet.cpp | 3 --- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 3eaaf3786c..9c5f2b5098 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -264,6 +264,10 @@ bool LegacyScriptPubKeyMan::EncryptKeys(CKeyingMaterial& vMasterKeyIn) bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) { + if (!CanGetAddresses(internal)) { + return false; + } + if (!ReserveKeyFromKeyPool(index, keypool, internal)) { return false; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b1e1385ca3..89309a01d6 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3298,9 +3298,6 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool inter return false; } - if (!pwallet->CanGetAddresses(internal)) { - return false; - } if (nIndex == -1) { -- cgit v1.2.3 From 9fcf8ce7ae02bf170b9bf0c2887fd709d752cbf7 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 19 Nov 2019 18:33:20 -0500 Subject: 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. --- src/wallet/scriptpubkeyman.cpp | 16 +++------------- src/wallet/scriptpubkeyman.h | 3 --- 2 files changed, 3 insertions(+), 16 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 9c5f2b5098..76a678d06a 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -274,16 +274,6 @@ bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool i 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(const CScript& script) { AssertLockHeld(cs_wallet); @@ -1096,7 +1086,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 WalletBatch batch(m_storage.GetDatabase()); @@ -1104,7 +1094,7 @@ void LegacyScriptPubKeyMan::KeepKey(int64_t 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 CPubKey& pubkey) { // Return to key pool { @@ -1138,7 +1128,7 @@ bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, bool internal) result = GenerateNewKey(batch, internal); return true; } - KeepKey(nIndex); + KeepDestination(nIndex); result = keypool.vchPubKey; } return true; diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 4f17156792..5d366f441e 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -266,9 +266,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(const OutputType type, CTxDestination& dest, std::string& error) override; isminetype IsMine(const CScript& script) const override; -- cgit v1.2.3 From 65833a74076cddf986037c6eb3b29a9b9dbe31c5 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 26 Nov 2019 11:52:51 -0500 Subject: Add OutputType and CPubKey parameters to KeepDestination These need to be added so that LearnRelatedScripts can be called from within KeepDestination later. --- src/wallet/scriptpubkeyman.cpp | 8 ++++---- src/wallet/scriptpubkeyman.h | 6 +++--- src/wallet/wallet.cpp | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 76a678d06a..f7153a751f 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -18,7 +18,7 @@ bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestinat // Generate a new key that is added to wallet CPubKey new_key; - if (!GetKeyFromPool(new_key)) { + if (!GetKeyFromPool(new_key, type)) { error = "Error: Keypool ran out, please call keypoolrefill first"; return false; } @@ -1086,7 +1086,7 @@ void LegacyScriptPubKeyMan::AddKeypoolPubkeyWithDB(const CPubKey& pubkey, const m_pool_key_to_index[pubkey.GetID()] = index; } -void LegacyScriptPubKeyMan::KeepDestination(int64_t nIndex) +void LegacyScriptPubKeyMan::KeepDestination(int64_t nIndex, const OutputType& type, const CPubKey& pubkey) { // Remove from key pool WalletBatch batch(m_storage.GetDatabase()); @@ -1112,7 +1112,7 @@ void LegacyScriptPubKeyMan::ReturnDestination(int64_t nIndex, bool fInternal, co WalletLogPrintf("keypool return %d\n", nIndex); } -bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, bool internal) +bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, const OutputType type, bool internal) { if (!CanGetAddresses(internal)) { return false; @@ -1128,7 +1128,7 @@ bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, bool internal) result = GenerateNewKey(batch, internal); return true; } - KeepDestination(nIndex); + KeepDestination(nIndex, type, keypool.vchPubKey); result = keypool.vchPubKey; } return true; diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 5d366f441e..16901952a6 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -151,7 +151,7 @@ public: virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; } virtual bool GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) { return false; } - virtual void KeepDestination(int64_t index) {} + virtual void KeepDestination(int64_t index, const OutputType& type, const CPubKey& pubkey) {} virtual void ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey) {} virtual bool TopUp(unsigned int size = 0) { return false; } @@ -248,7 +248,7 @@ private: std::map m_pool_key_to_index; //! Fetches a key from the keypool - bool GetKeyFromPool(CPubKey &key, bool internal = false); + bool GetKeyFromPool(CPubKey &key, const OutputType type, bool internal = false); /** * Reserves a key from the keypool and sets nIndex to its index @@ -274,7 +274,7 @@ public: bool EncryptKeys(CKeyingMaterial& vMasterKeyIn); bool GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) override; - void KeepDestination(int64_t index) override; + void KeepDestination(int64_t index, const OutputType& type, const CPubKey& pubkey) override; void ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey) override; bool TopUp(unsigned int size = 0) override; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 89309a01d6..179e7b39ea 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3317,7 +3317,7 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool inter void ReserveDestination::KeepDestination() { if (nIndex != -1) { - m_spk_man->KeepDestination(nIndex); + m_spk_man->KeepDestination(nIndex, type, vchPubKey); m_spk_man->LearnRelatedScripts(vchPubKey, type); } nIndex = -1; -- cgit v1.2.3 From ba41aa4969169cd73d6b4f57444ed7d8d875de10 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Oct 2019 14:11:34 -0400 Subject: 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. --- src/wallet/scriptpubkeyman.cpp | 4 +++- src/wallet/scriptpubkeyman.h | 4 ++-- src/wallet/wallet.cpp | 4 +--- src/wallet/wallet.h | 3 ++- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index f7153a751f..4986871fb7 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -262,7 +262,7 @@ bool LegacyScriptPubKeyMan::EncryptKeys(CKeyingMaterial& vMasterKeyIn) return true; } -bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) +bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) { if (!CanGetAddresses(internal)) { return false; @@ -271,6 +271,7 @@ bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool i if (!ReserveKeyFromKeyPool(index, keypool, internal)) { return false; } + address = GetDestinationForKey(keypool.vchPubKey, type); return true; } @@ -1091,6 +1092,7 @@ void LegacyScriptPubKeyMan::KeepDestination(int64_t nIndex, const OutputType& ty // Remove from key pool WalletBatch batch(m_storage.GetDatabase()); batch.ErasePool(nIndex); + LearnRelatedScripts(pubkey, type); WalletLogPrintf("keypool keep %d\n", nIndex); } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 16901952a6..bcc62ece1e 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -150,7 +150,7 @@ public: virtual bool GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error) { return false; } virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; } - virtual bool GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) { return false; } + virtual bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) { return false; } virtual void KeepDestination(int64_t index, const OutputType& type, const CPubKey& pubkey) {} virtual void ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey) {} @@ -273,7 +273,7 @@ public: //! will encrypt previously unencrypted keys bool EncryptKeys(CKeyingMaterial& vMasterKeyIn); - bool GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) override; + bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) override; void KeepDestination(int64_t index, const OutputType& type, const CPubKey& pubkey) override; void ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey) override; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 179e7b39ea..2bdec16f28 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3302,14 +3302,13 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool inter if (nIndex == -1) { CKeyPool keypool; - if (!m_spk_man->GetReservedDestination(type, internal, nIndex, keypool)) { + if (!m_spk_man->GetReservedDestination(type, internal, address, nIndex, keypool)) { return false; } vchPubKey = keypool.vchPubKey; fInternal = keypool.fInternal; } assert(vchPubKey.IsValid()); - address = GetDestinationForKey(vchPubKey, type); dest = address; return true; } @@ -3318,7 +3317,6 @@ void ReserveDestination::KeepDestination() { if (nIndex != -1) { m_spk_man->KeepDestination(nIndex, type, vchPubKey); - m_spk_man->LearnRelatedScripts(vchPubKey, type); } nIndex = -1; vchPubKey = CPubKey(); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index fce49ec56c..f6cbe6f6de 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -141,7 +141,8 @@ class ReserveDestination 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}; OutputType const type; //! The index of the address's key in the keypool int64_t nIndex{-1}; -- cgit v1.2.3 From 386a994b853bc5b3a2ed0d812673465b8ffa4849 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Oct 2019 14:11:34 -0400 Subject: 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. --- src/wallet/scriptpubkeyman.cpp | 16 ++++++++++++---- src/wallet/scriptpubkeyman.h | 10 ++++++---- src/wallet/wallet.cpp | 8 ++------ src/wallet/wallet.h | 2 -- 4 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 4986871fb7..e5b45a81a1 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1087,16 +1087,20 @@ void LegacyScriptPubKeyMan::AddKeypoolPubkeyWithDB(const CPubKey& pubkey, const m_pool_key_to_index[pubkey.GetID()] = index; } -void LegacyScriptPubKeyMan::KeepDestination(int64_t nIndex, const OutputType& type, const CPubKey& pubkey) +void LegacyScriptPubKeyMan::KeepDestination(int64_t nIndex, const OutputType& type) { // Remove from key pool WalletBatch batch(m_storage.GetDatabase()); batch.ErasePool(nIndex); + CPubKey pubkey; + bool have_pk = GetPubKey(m_index_to_reserved_key.at(nIndex), pubkey); + assert(have_pk); LearnRelatedScripts(pubkey, type); + m_index_to_reserved_key.erase(nIndex); WalletLogPrintf("keypool keep %d\n", nIndex); } -void LegacyScriptPubKeyMan::ReturnDestination(int64_t nIndex, bool fInternal, const CPubKey& pubkey) +void LegacyScriptPubKeyMan::ReturnDestination(int64_t nIndex, bool fInternal, const CTxDestination&) { // Return to key pool { @@ -1108,7 +1112,9 @@ void LegacyScriptPubKeyMan::ReturnDestination(int64_t nIndex, bool fInternal, co } 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); @@ -1130,7 +1136,7 @@ bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, const OutputType typ result = GenerateNewKey(batch, internal); return true; } - KeepDestination(nIndex, type, keypool.vchPubKey); + KeepDestination(nIndex, type); result = keypool.vchPubKey; } return true; @@ -1175,6 +1181,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 bcc62ece1e..6ed9a4787a 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -151,8 +151,8 @@ public: virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; } virtual bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) { return false; } - virtual void KeepDestination(int64_t index, const OutputType& type, const CPubKey& pubkey) {} - virtual void ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey) {} + virtual void KeepDestination(int64_t index, const OutputType& type) {} + virtual void ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) {} virtual bool TopUp(unsigned int size = 0) { return false; } @@ -246,6 +246,8 @@ private: std::set set_pre_split_keypool 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, const OutputType type, bool internal = false); @@ -274,8 +276,8 @@ public: bool EncryptKeys(CKeyingMaterial& vMasterKeyIn); bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) override; - void KeepDestination(int64_t index, const OutputType& type, const CPubKey& pubkey) override; - void ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey) override; + void KeepDestination(int64_t index, const OutputType& type) 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 2bdec16f28..e50dd1c127 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3305,10 +3305,8 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool inter if (!m_spk_man->GetReservedDestination(type, internal, address, nIndex, keypool)) { return false; } - vchPubKey = keypool.vchPubKey; fInternal = keypool.fInternal; } - assert(vchPubKey.IsValid()); dest = address; return true; } @@ -3316,20 +3314,18 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool inter void ReserveDestination::KeepDestination() { if (nIndex != -1) { - m_spk_man->KeepDestination(nIndex, type, vchPubKey); + m_spk_man->KeepDestination(nIndex, type); } 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 f6cbe6f6de..db5788ebb5 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -146,8 +146,6 @@ protected: OutputType const type; //! 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 -- cgit v1.2.3 From 886f1731bec4393dd342403ac34069a3a4f95eea Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Oct 2019 14:11:34 -0400 Subject: Key pool: Fix omitted pre-split count in GetKeyPoolSize This is a bugfix: https://github.com/bitcoin/bitcoin/pull/16341#discussion_r330669214 --- src/wallet/scriptpubkeyman.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index e5b45a81a1..2b9e8fbf2a 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -455,7 +455,7 @@ size_t LegacyScriptPubKeyMan::KeypoolCountExternalKeys() unsigned int LegacyScriptPubKeyMan::GetKeyPoolSize() const { AssertLockHeld(cs_wallet); - return setInternalKeyPool.size() + setExternalKeyPool.size(); + return setInternalKeyPool.size() + setExternalKeyPool.size() + set_pre_split_keypool.size(); } int64_t LegacyScriptPubKeyMan::GetTimeFirstKey() const -- cgit v1.2.3