From b90233b661750efb5a8e00ea6919ab85a9f44e19 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Oct 2019 14:11:34 -0400 Subject: [PATCH 01/21] MOVEONLY: Reorder LegacyScriptPubKeyMan methods Can verify move-only with: git log -p -n1 --color-moved This commit is move-only and doesn't change code or affect behavior. --- src/wallet/scriptpubkeyman.cpp | 2 + src/wallet/scriptpubkeyman.h | 235 +++++++++++++++++---------------- 2 files changed, 123 insertions(+), 114 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 2f4e21e45dcc..3a62909ab15e 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1331,12 +1331,14 @@ bool LegacyScriptPubKeyMan::TopUpKeyPool(unsigned int kpSize) return true; } +/* void LegacyScriptPubKeyMan::AddKeypoolPubkey(const CPubKey& pubkey, const bool internal) { WalletBatch batch(m_storage.GetDatabase()); AddKeypoolPubkeyWithDB(pubkey, internal, batch); NotifyCanGetAddressesChanged(); } +*/ void LegacyScriptPubKeyMan::AddKeypoolPubkeyWithDB(const CPubKey& pubkey, const bool internal, WalletBatch& batch) { diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 7e3255c434ca..aa5993cde2a1 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -137,26 +137,46 @@ class ScriptPubKeyMan class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProvider { private: - using CryptedKeyMap = std::map>>; using WatchOnlySet = std::set; using WatchKeyMap = std::map; using HDPubKeyMap = std::map; - //! will encrypt previously unencrypted keys - bool EncryptKeys(CKeyingMaterial& vMasterKeyIn); + + WalletBatch *encrypted_batch GUARDED_BY(cs_wallet) = nullptr; + + using CryptedKeyMap = std::map>>; CryptedKeyMap mapCryptedKeys GUARDED_BY(cs_KeyStore); WatchOnlySet setWatchOnly GUARDED_BY(cs_KeyStore); WatchKeyMap mapWatchKeys GUARDED_BY(cs_KeyStore); HDPubKeyMap mapHdPubKeys GUARDED_BY(cs_KeyStore); // &vchCryptedSecret); bool AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey); + bool AddCryptedKeyInner(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret); bool GetKeyInner(const CKeyID &address, CKey& keyOut) const; bool GetPubKeyInner(const CKeyID &address, CPubKey& vchPubKeyOut) const; - WalletBatch *encrypted_batch GUARDED_BY(cs_wallet) = nullptr; + /** + * Private version of AddWatchOnly method which does not accept a + * timestamp, and which will reset the wallet's nTimeFirstKey value to 1 if + * the watch key did not previously have a timestamp associated with it. + * Because this is an inherited virtual method, it is accessible despite + * being marked private, but it is marked private anyway to encourage use + * of the other AddWatchOnly which accepts a timestamp and sets + * nTimeFirstKey more intelligently for more efficient rescans. + */ + bool AddWatchOnly(const CScript& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool AddWatchOnlyInMem(const CScript &dest); + + bool AddWatchOnlyWithDB(WalletBatch &batch, const CScript& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + void AddKeypoolPubkeyWithDB(const CPubKey& pubkey, const bool internal, WalletBatch& batch); + + /** Add a KeyOriginInfo to the wallet */ + bool AddKeyOriginWithDB(WalletBatch& batch, const CPubKey& pubkey, const KeyOriginInfo& info); /* the HD chain data model (external chain counters) */ CHDChain hdChain GUARDED_BY(cs_KeyStore); @@ -170,37 +190,49 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv int64_t m_max_keypool_index GUARDED_BY(cs_wallet) = 0; std::map m_pool_key_to_index; - int64_t nTimeFirstKey GUARDED_BY(cs_wallet) = 0; - /** - * Private version of AddWatchOnly method which does not accept a - * timestamp, and which will reset the wallet's nTimeFirstKey value to 1 if - * the watch key did not previously have a timestamp associated with it. - * Because this is an inherited virtual method, it is accessible despite - * being marked private, but it is marked private anyway to encourage use - * of the other AddWatchOnly which accepts a timestamp and sets - * nTimeFirstKey more intelligently for more efficient rescans. + * Reserves a key from the keypool and sets nIndex to its index + * + * @param[out] nIndex the index of the key in keypool + * @param[out] keypool the keypool the key was drawn from, which could be the + * the pre-split pool if present, or the internal or external pool + * @param fRequestedInternal true if the caller would like the key drawn + * from the internal keypool, false if external is preferred + * + * @return true if succeeded, false if failed due to empty keypool + * @throws std::runtime_error if keypool read failed, key was invalid, + * was not found in the wallet, or was misclassified in the internal + * or external keypool */ - bool AddWatchOnly(const CScript& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - bool AddWatchOnlyInMem(const CScript &dest); - - /** Add a KeyOriginInfo to the wallet */ - bool AddKeyOriginWithDB(WalletBatch& batch, const CPubKey& pubkey, const KeyOriginInfo& info); + bool ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRequestedInternal); - void AddKeypoolPubkeyWithDB(const CPubKey& pubkey, const bool internal, WalletBatch& batch); + void KeepKey(int64_t nIndex); + void ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey); public: - //! Adds a key to the store, and saves it to disk. - bool AddKeyPubKeyWithDB(WalletBatch &batch,const CKey& key, const CPubKey &pubkey) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool GetNewDestination(const std::string label, CTxDestination& dest, std::string& error); - //! Adds a watch-only address to the store, and saves it to disk. - bool AddWatchOnlyWithDB(WalletBatch &batch, const CScript& dest, int64_t create_time) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + isminetype IsMine(const CScript& script) const; + isminetype IsMine(const CTxDestination& dest) const; - //! Adds a script to the store and saves it to disk - bool AddCScriptWithDB(WalletBatch& batch, const CScript& script); + //! will encrypt previously unencrypted keys + bool EncryptKeys(CKeyingMaterial& vMasterKeyIn); + //! Upgrade stored CKeyMetadata objects to store key origin info as KeyOriginInfo + void UpgradeKeyMetadata() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + /* Returns true if HD is enabled */ + bool IsHDEnabled() const; + + int64_t GetOldestKeyPoolTime(); + size_t KeypoolCountExternalKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + size_t KeypoolCountInternalKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + //! Fetches a key from the keypool + bool GetKeyFromPool(CPubKey &key, bool fInternal /*= false*/); + + /* Returns true if the wallet can give out new addresses. This means it has keys in the keypool or can generate new keys */ + bool CanGetAddresses(bool internal = false); - public: - void LoadKeyPool(int64_t nIndex, const CKeyPool &keypool) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); // Map from Key ID to key metadata. std::map mapKeyMetadata GUARDED_BY(cs_wallet); @@ -208,95 +240,89 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv // Map from Script ID to key metadata (for watch-only keys). std::map m_script_metadata GUARDED_BY(cs_wallet); - bool WriteKeyMetadata(const CKeyMetadata& meta, const CPubKey& pubkey, bool overwrite); + //! Adds a script to the store and saves it to disk + bool AddCScriptWithDB(WalletBatch& batch, const CScript& script); + + //! Adds a key to the store, and saves it to disk. + bool AddKeyPubKeyWithDB(WalletBatch &batch,const CKey& key, const CPubKey &pubkey) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - /** - * keystore implementation - * Generate a new key - */ - CPubKey GenerateNewKey(WalletBatch& batch, uint32_t nAccountIndex, bool fInternal /*= false*/) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Adds a key to the store, and saves it to disk. bool AddKeyPubKey(const CKey& key, const CPubKey &pubkey) override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Adds a key to the store, without saving it to disk (used by LoadWallet) bool LoadKey(const CKey& key, const CPubKey &pubkey) { return AddKeyPubKeyInner(key, pubkey); } - //! Load metadata (used by LoadWallet) - void LoadKeyMetadata(const CKeyID& keyID, const CKeyMetadata &metadata) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - void LoadScriptMetadata(const CScriptID& script_id, const CKeyMetadata &metadata) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - //! Upgrade stored CKeyMetadata objects to store key origin info as KeyOriginInfo - void UpgradeKeyMetadata() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - void UpdateTimeFirstKey(int64_t nCreateTime) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - int64_t GetTimeFirstKey() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - //! Adds an encrypted key to the store, and saves it to disk. bool AddCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret); //! Adds an encrypted key to the store, without saving it to disk (used by LoadWallet) bool LoadCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret); - //! GetKey implementation that can derive a HD private key on the fly - bool GetKey(const CKeyID &address, CKey& keyOut) const override; - //! GetPubKey implementation that also checks the mapHdPubKeys - bool GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const override; - //! HaveKey implementation that also checks the mapHdPubKeys - bool HaveKey(const CKeyID &address) const override; - //! Adds a HDPubKey into the wallet(database) - bool AddHDPubKey(WalletBatch &batch, const CExtPubKey &extPubKey, bool fInternal); - //! loads a HDPubKey into the wallets memory - bool LoadHDPubKey(const CHDPubKey &hdPubKey) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - std::set GetKeys() const override; - bool AddCScript(const CScript& redeemScript) override; + void UpdateTimeFirstKey(int64_t nCreateTime) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + //! Adds a CScript to the store bool LoadCScript(const CScript& redeemScript); + //! Load metadata (used by LoadWallet) + void LoadKeyMetadata(const CKeyID& keyID, const CKeyMetadata &metadata) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool WriteKeyMetadata(const CKeyMetadata& meta, const CPubKey& pubkey, bool overwrite); + void LoadScriptMetadata(const CScriptID& script_id, const CKeyMetadata &metadata) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + //! Generate a new key + CPubKey GenerateNewKey(WalletBatch& batch, uint32_t nAccountIndex, bool fInternal /*= false*/) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + /* Set the HD chain model (chain child index counters) */ + bool SetHDChain(WalletBatch &batch, const CHDChain& chain, bool memonly); + bool SetCryptedHDChain(WalletBatch &batch, const CHDChain& chain, bool memonly); + /** + * Set the HD chain model (chain child index counters) using temporary wallet db object + * which causes db flush every time these methods are used + */ + bool SetHDChainSingle(const CHDChain& chain, bool memonly); + bool SetCryptedHDChainSingle(const CHDChain& chain, bool memonly); - //! Adds a watch-only address to the store, and saves it to disk. - bool AddWatchOnly(const CScript& dest, int64_t nCreateTime) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - bool AddWatchOnlyWithDB(WalletBatch &batch, const CScript& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - bool RemoveWatchOnly(const CScript &dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Adds a watch-only address to the store, without saving it to disk (used by LoadWallet) bool LoadWatchOnly(const CScript &dest); //! Returns whether the watch-only script is in the wallet bool HaveWatchOnly(const CScript &dest) const; //! Returns whether there are any watch-only things in the wallet bool HaveWatchOnly() const; + //! Remove a watch only script from the keystore + bool RemoveWatchOnly(const CScript &dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + //! Adds a watch-only address to the store, and saves it to disk. + bool AddWatchOnly(const CScript& dest, int64_t nCreateTime) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + //! Adds a watch-only address to the store, and saves it to disk. + bool AddWatchOnlyWithDB(WalletBatch &batch, const CScript& dest, int64_t create_time) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + //! Fetches a pubkey from mapWatchKeys if it exists there bool GetWatchPubKey(const CKeyID &address, CPubKey &pubkey_out) const; + //! HaveKey implementation that also checks the mapHdPubKeys + bool HaveKey(const CKeyID &address) const override; + //! GetKey implementation that can derive a HD private key on the fly + bool GetKey(const CKeyID &address, CKey& keyOut) const override; + //! GetPubKey implementation that also checks the mapHdPubKeys + bool GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const override; + bool AddCScript(const CScript& redeemScript) override; + /** Implement lookup of key origin information through wallet key metadata. */ + bool GetKeyOrigin(const CKeyID& keyid, KeyOriginInfo& info) const override; + /** Add a KeyOriginInfo to the wallet */ + bool AddKeyOrigin(const CPubKey& pubkey, const KeyOriginInfo& info); + + void LoadKeyPool(int64_t nIndex, const CKeyPool &keypool) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool TopUpKeyPool(unsigned int kpSize = 0); + bool NewKeyPool(); + // Seems as not used now anywhere in code + // void AddKeypoolPubkey(const CPubKey& pubkey, const bool internal); + bool ImportScripts(const std::set scripts, int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool ImportPrivKeys(const std::map& privkey_map, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool ImportPubKeys(const std::vector& ordered_pubkeys, const std::map& pubkey_map, const std::map>& key_origins, const bool add_keypool, const bool internal, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool ImportScriptPubKeys(const std::string& label, const std::set& script_pub_keys, const bool have_solving_data, const bool apply_label, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - bool NewKeyPool(); - size_t KeypoolCountExternalKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - size_t KeypoolCountInternalKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - bool TopUpKeyPool(unsigned int kpSize = 0); - void AddKeypoolPubkey(const CPubKey& pubkey, const bool internal); - - /** - * Reserves a key from the keypool and sets nIndex to its index - * - * @param[out] nIndex the index of the key in keypool - * @param[out] keypool the keypool the key was drawn from, which could be the - * the pre-split pool if present, or the internal or external pool - * @param fRequestedInternal true if the caller would like the key drawn - * from the internal keypool, false if external is preferred - * - * @return true if succeeded, false if failed due to empty keypool - * @throws std::runtime_error if keypool read failed, key was invalid, - * was not found in the wallet, or was misclassified in the internal - * or external keypool - */ - bool ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRequestedInternal); - void KeepKey(int64_t nIndex); - void ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey); - //! Fetches a key from the keypool - bool GetKeyFromPool(CPubKey &key, bool internal /* = false */); - int64_t GetOldestKeyPoolTime(); + /* Returns true if the wallet can generate new keys */ + bool CanGenerateKeys(); - /** - * Marks all keys in the keypool up to and including reserve_key as used. - */ - void MarkReserveKeysAsUsed(int64_t keypool_id) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + int64_t GetTimeFirstKey() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - isminetype IsMine(const CScript& script) const; - isminetype IsMine(const CTxDestination& dest) const; + //! Adds a HDPubKey into the wallet(database) + bool AddHDPubKey(WalletBatch &batch, const CExtPubKey &extPubKey, bool fInternal); + //! loads a HDPubKey into the wallets memory + bool LoadHDPubKey(const CHDPubKey &hdPubKey) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** * HD Wallet Functions @@ -309,29 +335,10 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv bool SetCryptedHDChain(const CHDChain& chain); bool GetDecryptedHDChain(CHDChain& hdChainRet); - /* Returns true if HD is enabled */ - bool IsHDEnabled() const; - - /* Returns true if the wallet can generate new keys */ - bool CanGenerateKeys(); - - /* Returns true if the wallet can give out new addresses. This means it has keys in the keypool or can generate new keys */ - bool CanGetAddresses(bool internal = false); - /* Generates a new HD chain */ void GenerateNewHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase); bool GenerateNewHDChainEncrypted(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase); - /* Set the HD chain model (chain child index counters) */ - bool SetHDChain(WalletBatch &batch, const CHDChain& chain, bool memonly); - bool SetCryptedHDChain(WalletBatch &batch, const CHDChain& chain, bool memonly); - /** - * Set the HD chain model (chain child index counters) using temporary wallet db object - * which causes db flush every time these methods are used - */ - bool SetHDChainSingle(const CHDChain& chain, bool memonly); - bool SetCryptedHDChainSingle(const CHDChain& chain, bool memonly); - /** * Explicitly make the wallet learn the related scripts for outputs to the * given key. This is purely to make the wallet file compatible with older @@ -346,13 +353,13 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv */ // void LearnAllRelatedScripts(const CPubKey& key); - /** Implement lookup of key origin information through wallet key metadata. */ - bool GetKeyOrigin(const CKeyID& keyid, KeyOriginInfo& info) const override; + /** + * Marks all keys in the keypool up to and including reserve_key as used. + */ + void MarkReserveKeysAsUsed(int64_t keypool_id) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); const std::map& GetAllReserveKeys() const { return m_pool_key_to_index; } - bool GetNewDestination(const std::string label, CTxDestination& dest, std::string& error); - /** Add a KeyOriginInfo to the wallet */ - bool AddKeyOrigin(const CPubKey& pubkey, const KeyOriginInfo& info); + std::set GetKeys() const override; // Temporary CWallet accessors and aliases. friend class CWallet; From 99678961e57b3be067f2d9e1aa23bc63449b6127 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Oct 2019 14:11:34 -0400 Subject: [PATCH 02/21] Refactor: Declare LegacyScriptPubKeyMan methods as virtual This commit does not change behavior. --- src/wallet/scriptpubkeyman.h | 37 ++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index aa5993cde2a1..34ba14253272 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -132,6 +132,24 @@ class ScriptPubKeyMan public: ScriptPubKeyMan(WalletStorage& storage) : m_storage(storage) {} + + virtual ~ScriptPubKeyMan() {}; + virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; } + virtual isminetype IsMine(const CTxDestination& dest) const { return ISMINE_NO; } + + //! Upgrade stored CKeyMetadata objects to store key origin info as KeyOriginInfo + virtual void UpgradeKeyMetadata() {} + + /* Returns true if HD is enabled */ + virtual bool IsHDEnabled() const { return false; } + + /* Returns true if the wallet can give out new addresses. This means it has keys in the keypool or can generate new keys */ + virtual bool CanGetAddresses(bool internal = false) { return false; } + + virtual int64_t GetOldestKeyPoolTime() { return GetTime(); } + + virtual size_t KeypoolCountExternalKeys() { return 0; } + virtual size_t KeypoolCountInternalKeys() { return 0; } }; class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProvider @@ -212,27 +230,25 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv public: bool GetNewDestination(const std::string label, CTxDestination& dest, std::string& error); - isminetype IsMine(const CScript& script) const; - isminetype IsMine(const CTxDestination& dest) const; + isminetype IsMine(const CScript& script) const override; + isminetype IsMine(const CTxDestination& dest) const override; //! will encrypt previously unencrypted keys bool EncryptKeys(CKeyingMaterial& vMasterKeyIn); //! Upgrade stored CKeyMetadata objects to store key origin info as KeyOriginInfo - void UpgradeKeyMetadata() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + void UpgradeKeyMetadata() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) override; /* Returns true if HD is enabled */ - bool IsHDEnabled() const; + bool IsHDEnabled() const override; - int64_t GetOldestKeyPoolTime(); - size_t KeypoolCountExternalKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - size_t KeypoolCountInternalKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + int64_t GetOldestKeyPoolTime() override; + size_t KeypoolCountExternalKeys() override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + size_t KeypoolCountInternalKeys() override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Fetches a key from the keypool bool GetKeyFromPool(CPubKey &key, bool fInternal /*= false*/); - /* Returns true if the wallet can give out new addresses. This means it has keys in the keypool or can generate new keys */ - bool CanGetAddresses(bool internal = false); - + bool CanGetAddresses(bool internal = false) override; // Map from Key ID to key metadata. std::map mapKeyMetadata GUARDED_BY(cs_wallet); @@ -290,6 +306,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv //! Fetches a pubkey from mapWatchKeys if it exists there bool GetWatchPubKey(const CKeyID &address, CPubKey &pubkey_out) const; + /* SigningProvider overrides */ //! HaveKey implementation that also checks the mapHdPubKeys bool HaveKey(const CKeyID &address) const override; //! GetKey implementation that can derive a HD private key on the fly From 3486f575d05541b634e551f847c099cd7edb2334 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Oct 2019 14:11:34 -0400 Subject: [PATCH 03/21] Refactor: Add new ScriptPubKeyMan virtual methods This commit does not change behavior. --- src/wallet/scriptpubkeyman.cpp | 25 +++++++++++++++++++++++++ src/wallet/scriptpubkeyman.h | 16 +++++++++++++--- src/wallet/wallet.cpp | 15 ++++++++------- 3 files changed, 46 insertions(+), 10 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 3a62909ab15e..42c35598fd4c 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -266,6 +266,31 @@ bool LegacyScriptPubKeyMan::EncryptKeys(CKeyingMaterial& vMasterKeyIn) return true; } +bool LegacyScriptPubKeyMan::GetReservedDestination(bool internal, int64_t& index, CKeyPool& keypool) +{ + { + if (!ReserveKeyFromKeyPool(index, keypool, internal)) { + return false; + } + } + 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); +} + +bool LegacyScriptPubKeyMan::TopUp(unsigned int size) +{ + return TopUpKeyPool(size); +} + void LegacyScriptPubKeyMan::UpgradeKeyMetadata() { AssertLockHeld(cs_wallet); // mapKeyMetadata diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 34ba14253272..6a12d1148cc6 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -137,8 +137,11 @@ class ScriptPubKeyMan virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; } virtual isminetype IsMine(const CTxDestination& dest) const { return ISMINE_NO; } - //! Upgrade stored CKeyMetadata objects to store key origin info as KeyOriginInfo - virtual void UpgradeKeyMetadata() {} + virtual bool GetReservedDestination(bool internal, 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 bool TopUp(unsigned int size = 0) { return false; } /* Returns true if HD is enabled */ virtual bool IsHDEnabled() const { return false; } @@ -235,8 +238,15 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv //! will encrypt previously unencrypted keys bool EncryptKeys(CKeyingMaterial& vMasterKeyIn); + + bool GetReservedDestination(bool internal, int64_t& index, CKeyPool& keypool) override; + void KeepDestination(int64_t index) override; + void ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey) override; + + bool TopUp(unsigned int size = 0) override; + //! Upgrade stored CKeyMetadata objects to store key origin info as KeyOriginInfo - void UpgradeKeyMetadata() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) override; + void UpgradeKeyMetadata() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /* Returns true if HD is enabled */ bool IsHDEnabled() const override; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c2601a7292d9..2c0bb305af78 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -984,7 +984,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Co WalletLogPrintf("%s: Detected a used keypool key, mark all keypool key up to this key as used\n", __func__); MarkReserveKeysAsUsed(mi->second); - if (!m_spk_man->TopUpKeyPool()) { + if (!m_spk_man->TopUp()) { WalletLogPrintf("%s: Topping up keypool failed (locked wallet)\n", __func__); } } @@ -3736,7 +3736,7 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize) { bool res = true; if (auto spk_man = m_spk_man.get()) { - res &= spk_man->TopUpKeyPool(kpSize); + res &= spk_man->TopUp(kpSize); } return res; } @@ -3755,7 +3755,8 @@ bool CWallet::GetNewDestination(const std::string label, CTxDestination& dest, s bool CWallet::GetNewChangeDestination(CTxDestination& dest, std::string& error) { error.clear(); - m_spk_man->TopUpKeyPool(); + + m_spk_man->TopUp(); ReserveDestination reservedest(this); if (!reservedest.GetReservedDestination(dest, true)) { @@ -3928,7 +3929,7 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool fInte if (nIndex == -1) { CKeyPool keypool; - if (!m_spk_man->ReserveKeyFromKeyPool(nIndex, keypool, fInternalIn)) { + if (!m_spk_man->GetReservedDestination(fInternalIn, nIndex, keypool)) { return false; } vchPubKey = keypool.vchPubKey; @@ -3943,7 +3944,7 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool fInte void ReserveDestination::KeepDestination() { if (nIndex != -1) { - m_spk_man->KeepKey(nIndex); + m_spk_man->KeepDestination(nIndex); } nIndex = -1; vchPubKey = CPubKey(); @@ -3953,7 +3954,7 @@ void ReserveDestination::KeepDestination() void ReserveDestination::ReturnDestination() { if (nIndex != -1) { - m_spk_man->ReturnKey(nIndex, fInternal, vchPubKey); + m_spk_man->ReturnDestination(nIndex, fInternal, vchPubKey); } nIndex = -1; vchPubKey = CPubKey(); @@ -4341,7 +4342,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, } // Otherwise, do not create a new HD chain // Top up the keypool - if (walletInstance->m_spk_man->CanGenerateKeys() && !walletInstance->TopUpKeyPool()) { + if (walletInstance->m_spk_man->CanGenerateKeys() && !walletInstance->m_spk_man->TopUp()) { return unload_wallet(_("Unable to generate initial keys")); } From a5f1865e2d63cec59f992d7d75daf68480c687ac Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Oct 2019 14:11:34 -0400 Subject: [PATCH 04/21] Refactor: Move SetAddressBook call out of LegacyScriptPubKeyMan::GetNewDestination This commit does not change behavior. --- src/wallet/scriptpubkeyman.cpp | 5 +---- src/wallet/scriptpubkeyman.h | 4 ++-- src/wallet/wallet.cpp | 7 ++++++- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 42c35598fd4c..fe5890f8455c 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -12,9 +12,8 @@ #include #include -bool LegacyScriptPubKeyMan::GetNewDestination(const std::string label, CTxDestination& dest, std::string& error) +bool LegacyScriptPubKeyMan::GetNewDestination(CTxDestination& dest, std::string& error) { - LOCK(cs_wallet); error.clear(); TopUpKeyPool(); @@ -26,8 +25,6 @@ bool LegacyScriptPubKeyMan::GetNewDestination(const std::string label, CTxDestin } //LearnRelatedScripts(new_key); dest = new_key.GetID(); - - m_wallet.SetAddressBook(dest, label, "receive"); return true; } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 6a12d1148cc6..d761d6a9130a 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -134,6 +134,7 @@ class ScriptPubKeyMan ScriptPubKeyMan(WalletStorage& storage) : m_storage(storage) {} virtual ~ScriptPubKeyMan() {}; + virtual bool GetNewDestination(CTxDestination& dest, std::string& error) { return false; } virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; } virtual isminetype IsMine(const CTxDestination& dest) const { return ISMINE_NO; } @@ -231,8 +232,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv void ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey); public: - bool GetNewDestination(const std::string label, CTxDestination& dest, std::string& error); - + bool GetNewDestination(CTxDestination& dest, std::string& error) override; isminetype IsMine(const CScript& script) const override; isminetype IsMine(const CTxDestination& dest) const override; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 2c0bb305af78..bf3ad4ad658e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3743,12 +3743,17 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize) bool CWallet::GetNewDestination(const std::string label, CTxDestination& dest, std::string& error) { + LOCK(cs_wallet); error.clear(); bool result = false; auto spk_man = m_spk_man.get(); if (spk_man) { - result = spk_man->GetNewDestination(label, dest, error); + result = spk_man->GetNewDestination(dest, error); + } + if (result) { + SetAddressBook(dest, label, "receive"); } + return result; } From 7df44e302b47119fa53c4f48884ee6cab2207d3e Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Oct 2019 14:11:34 -0400 Subject: [PATCH 05/21] Refactor: Move SetWalletFlag out of LegacyScriptPubKeyMan::UpgradeKeyMetadata This commit does not change behavior. --- src/wallet/scriptpubkeyman.cpp | 2 -- src/wallet/wallet.cpp | 5 +++++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index fe5890f8455c..439f450f195f 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -336,8 +336,6 @@ void LegacyScriptPubKeyMan::UpgradeKeyMetadata() } } } - batch.reset(); //write before setting the flag - m_storage.SetWalletFlag(WALLET_FLAG_KEY_ORIGIN_METADATA); } void LegacyScriptPubKeyMan::GenerateNewHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index bf3ad4ad658e..5ad2bc051c29 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -350,10 +350,15 @@ const CWalletTx* CWallet::GetWalletTx(const uint256& hash) const void CWallet::UpgradeKeyMetadata() { + if (IsLocked() || IsWalletFlagSet(WALLET_FLAG_KEY_ORIGIN_METADATA)) { + return; + } + if (m_spk_man) { AssertLockHeld(m_spk_man->cs_wallet); m_spk_man->UpgradeKeyMetadata(); } + SetWalletFlag(WALLET_FLAG_KEY_ORIGIN_METADATA); } bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, const SecureString& strNewWalletPassphrase) From 19f31ba51b97e2b67cf549aca09b7fafa1b52788 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 29 Oct 2019 17:49:39 -0400 Subject: [PATCH 06/21] Remove SetWalletFlag from WalletStorage SetWalletFlag is unused. Does not change any behavior --- src/wallet/wallet.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index bfb5fed35369..5b6d6f63e0b4 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1229,7 +1229,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati void BlockUntilSyncedToCurrentChain() LOCKS_EXCLUDED(cs_main, cs_wallet); /** set a single wallet flag */ - void SetWalletFlag(uint64_t flags) override; + void SetWalletFlag(uint64_t flags); /** Unsets a single wallet flag */ void UnsetWalletFlag(uint64_t flag); From 0b44815eb82351b1cbaeece2d24ea679eaff64d0 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Oct 2019 14:11:34 -0400 Subject: [PATCH 07/21] Refactor: Remove UnsetWalletFlag call from LegacyScriptPubKeyMan::SetHDSeed This commit does not change behavior. From 1c8a86f709dcda8f362e8e281fbae192cb9fc8e1 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 29 Oct 2019 17:53:35 -0400 Subject: [PATCH 08/21] refactor: Replace UnsetWalletFlagWithDB with UnsetBlankWalletFlag in ScriptPubKeyMan ScriptPubKeyMan is only using UnsetWalletFlagWithDB to unset the blank wallet flag. Just make that it's own function and not expose the flag writing directly. This does not change behavior. --- src/wallet/scriptpubkeyman.cpp | 12 ++++++------ src/wallet/scriptpubkeyman.h | 2 +- src/wallet/wallet.cpp | 9 +++++++-- src/wallet/wallet.h | 10 ++++++++-- 4 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 439f450f195f..9043b7ea08a1 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -441,7 +441,7 @@ bool LegacyScriptPubKeyMan::SetHDChain(WalletBatch &batch, const CHDChain& chain throw std::runtime_error(std::string(__func__) + ": WriteHDChain failed"); } - m_wallet.UnsetWalletFlag(batch, WALLET_FLAG_BLANK_WALLET); + m_wallet.UnsetWalletFlagWithDB(batch, WALLET_FLAG_BLANK_WALLET); } return true; @@ -462,7 +462,7 @@ bool LegacyScriptPubKeyMan::SetCryptedHDChain(WalletBatch &batch, const CHDChain if (!batch.WriteCryptedHDChain(chain)) throw std::runtime_error(std::string(__func__) + ": WriteCryptedHDChain failed"); } - m_wallet.UnsetWalletFlag(batch, WALLET_FLAG_BLANK_WALLET); + m_wallet.UnsetWalletFlagWithDB(batch, WALLET_FLAG_BLANK_WALLET); } return true; @@ -742,7 +742,7 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyWithDB(WalletBatch& batch, const CKey& s secret.GetPrivKey(), mapKeyMetadata[pubkey.GetID()]); } - m_wallet.UnsetWalletFlag(batch, WALLET_FLAG_BLANK_WALLET); + m_storage.UnsetBlankWalletFlag(batch); return true; } @@ -952,7 +952,7 @@ bool LegacyScriptPubKeyMan::AddWatchOnlyWithDB(WalletBatch &batch, const CScript UpdateTimeFirstKey(meta.nCreateTime); NotifyWatchonlyChanged(true); if (batch.WriteWatchOnly(dest, meta)) { - m_wallet.UnsetWalletFlag(batch, WALLET_FLAG_BLANK_WALLET); + m_storage.UnsetBlankWalletFlag(batch); return true; } return false; @@ -1035,7 +1035,7 @@ bool LegacyScriptPubKeyMan::AddHDPubKey(WalletBatch &batch, const CExtPubKey &ex if (!batch.WriteHDPubKey(hdPubKey, mapKeyMetadata[extPubKey.pubkey.GetID()])) { return false; } - m_wallet.UnsetWalletFlag(batch, WALLET_FLAG_BLANK_WALLET); + m_wallet.UnsetWalletFlagWithDB(batch, WALLET_FLAG_BLANK_WALLET); return true; } @@ -1518,7 +1518,7 @@ bool LegacyScriptPubKeyMan::AddCScriptWithDB(WalletBatch& batch, const CScript& if (!FillableSigningProvider::AddCScript(redeemScript)) return false; if (batch.WriteCScript(Hash160(redeemScript), redeemScript)) { - m_wallet.UnsetWalletFlag(batch, WALLET_FLAG_BLANK_WALLET); + m_storage.UnsetBlankWalletFlag(batch); return true; } return false; diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index d761d6a9130a..945dcce53dec 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -26,7 +26,7 @@ class WalletStorage virtual const std::string GetDisplayName() const = 0; virtual WalletDatabase& GetDatabase() = 0; virtual bool IsWalletFlagSet(uint64_t) const = 0; - virtual void SetWalletFlag(uint64_t) = 0; + virtual void UnsetBlankWalletFlag(WalletBatch&) = 0; virtual bool CanSupportFeature(enum WalletFeature) const = 0; virtual void SetMinVersion(enum WalletFeature, WalletBatch* = nullptr, bool = false) = 0; virtual bool IsLocked(bool fForMixing = false) const = 0; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 5ad2bc051c29..6b17a68ff7db 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1586,10 +1586,10 @@ void CWallet::UnsetWalletFlag(uint64_t flag) { LOCK(cs_wallet); WalletBatch batch(*database); - UnsetWalletFlag(batch, flag); + UnsetWalletFlagWithDB(batch, flag); } -void CWallet::UnsetWalletFlag(WalletBatch& batch, uint64_t flag) +void CWallet::UnsetWalletFlagWithDB(WalletBatch& batch, uint64_t flag) { LOCK(cs_wallet); m_wallet_flags &= ~flag; @@ -1597,6 +1597,11 @@ void CWallet::UnsetWalletFlag(WalletBatch& batch, uint64_t flag) throw std::runtime_error(std::string(__func__) + ": writing wallet flags failed"); } +void CWallet::UnsetBlankWalletFlag(WalletBatch& batch) +{ + UnsetWalletFlagWithDB(batch, WALLET_FLAG_BLANK_WALLET); +} + bool CWallet::IsWalletFlagSet(uint64_t flag) const { return (m_wallet_flags & flag); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 5b6d6f63e0b4..14dcdb12ea53 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -730,6 +730,14 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati std::atomic m_wallet_flags{0}; + bool SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& address, const std::string& strName, const std::string& strPurpose); + + //! Unsets a wallet flag and saves it to disk + void UnsetWalletFlagWithDB(WalletBatch& batch, uint64_t flag); + + //! Unset the blank wallet flag and saves it to disk + void UnsetBlankWalletFlag(WalletBatch& batch) override; + /** Interface for accessing chain state. */ interfaces::Chain* m_chain; @@ -1096,7 +1104,6 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati void AutoLockMasternodeCollaterals(); DBErrors ZapSelectTx(std::vector& vHashIn, std::vector& vHashOut) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - bool SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& address, const std::string& strName, const std::string& strPurpose); bool SetAddressBook(const CTxDestination& address, const std::string& strName, const std::string& purpose); bool DelAddressBook(const CTxDestination& address); @@ -1233,7 +1240,6 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati /** Unsets a single wallet flag */ void UnsetWalletFlag(uint64_t flag); - void UnsetWalletFlag(WalletBatch& batch, uint64_t flag); /** check if a certain wallet flag is set */ bool IsWalletFlagSet(uint64_t flag) const override; From 7bb82e4b97839ce568cd766cf453727d46ac59ff Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Oct 2019 14:11:34 -0400 Subject: [PATCH 09/21] Refactor: Move SetAddressBookWithDB call out of LegacyScriptPubKeyMan::ImportScriptPubKeys This commit does not change behavior. --- src/wallet/scriptpubkeyman.cpp | 7 +------ src/wallet/scriptpubkeyman.h | 2 +- src/wallet/wallet.cpp | 12 +++++++++++- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 9043b7ea08a1..4fd8a3e72af7 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1602,7 +1602,7 @@ bool LegacyScriptPubKeyMan::ImportPubKeys(const std::vector& ordered_pub return true; } -bool LegacyScriptPubKeyMan::ImportScriptPubKeys(const std::string& label, const std::set& script_pub_keys, const bool have_solving_data, const bool apply_label, const int64_t timestamp) +bool LegacyScriptPubKeyMan::ImportScriptPubKeys(const std::set& script_pub_keys, const bool have_solving_data, const int64_t timestamp) { WalletBatch batch(m_storage.GetDatabase()); for (const CScript& script : script_pub_keys) { @@ -1611,11 +1611,6 @@ bool LegacyScriptPubKeyMan::ImportScriptPubKeys(const std::string& label, const return false; } } - CTxDestination dest; - ExtractDestination(script, dest); - if (apply_label && IsValidDestination(dest)) { - m_wallet.SetAddressBookWithDB(batch, dest, label, "receive"); - } } return true; } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 945dcce53dec..565d71566df4 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -339,7 +339,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv bool ImportPrivKeys(const std::map& privkey_map, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool ImportPubKeys(const std::vector& ordered_pubkeys, const std::map& pubkey_map, const std::map>& key_origins, const bool add_keypool, const bool internal, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - bool ImportScriptPubKeys(const std::string& label, const std::set& script_pub_keys, const bool have_solving_data, const bool apply_label, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool ImportScriptPubKeys(const std::set& script_pub_keys, const bool have_solving_data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /* Returns true if the wallet can generate new keys */ bool CanGenerateKeys(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6b17a68ff7db..7efd96a16d66 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1698,9 +1698,19 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::setcs_wallet); - if (!spk_man->ImportScriptPubKeys(label, script_pub_keys, have_solving_data, apply_label, timestamp)) { + if (!spk_man->ImportScriptPubKeys(script_pub_keys, have_solving_data, timestamp)) { return false; } + if (apply_label) { + WalletBatch batch(*database); + for (const CScript& script : script_pub_keys) { + CTxDestination dest; + ExtractDestination(script, dest); + if (IsValidDestination(dest)) { + SetAddressBookWithDB(batch, dest, label, "receive"); + } + } + } return true; } From f5b61333f4705fa2a7a5d365af978af435d7acfd Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Oct 2019 14:11:34 -0400 Subject: [PATCH 10/21] Refactor: Move LoadKey LegacyScriptPubKeyMan method definition This commit does not change behavior. --- src/wallet/scriptpubkeyman.cpp | 5 +++++ src/wallet/scriptpubkeyman.h | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 4fd8a3e72af7..f2e16cb5e2bd 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -700,6 +700,11 @@ int64_t LegacyScriptPubKeyMan::GetTimeFirstKey() const return nTimeFirstKey; } +bool LegacyScriptPubKeyMan::LoadKey(const CKey& key, const CPubKey &pubkey) +{ + return AddKeyPubKeyInner(key, pubkey); +} + bool LegacyScriptPubKeyMan::AddKeyPubKey(const CKey& secret, const CPubKey &pubkey) { WalletBatch batch(m_storage.GetDatabase()); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 565d71566df4..9102a9fe87f1 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -275,7 +275,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv //! Adds a key to the store, and saves it to disk. bool AddKeyPubKey(const CKey& key, const CPubKey &pubkey) override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Adds a key to the store, without saving it to disk (used by LoadWallet) - bool LoadKey(const CKey& key, const CPubKey &pubkey) { return AddKeyPubKeyInner(key, pubkey); } + bool LoadKey(const CKey& key, const CPubKey &pubkey); //! Adds an encrypted key to the store, and saves it to disk. bool AddCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret); //! Adds an encrypted key to the store, without saving it to disk (used by LoadWallet) From 12550f785560905b5553bbb9ac7974db022c974c Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Oct 2019 14:11:34 -0400 Subject: [PATCH 11/21] Refactor: Move GetMetadata code out of getaddressinfo Easier to review ignoring whitespace: git log -p -n1 -w This commit does not change behavior. --- src/wallet/rpcwallet.cpp | 51 ++++++++++++++++------------------ src/wallet/scriptpubkeyman.cpp | 15 ++++++++++ src/wallet/scriptpubkeyman.h | 4 +++ 3 files changed, 43 insertions(+), 27 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 717377e10fe8..00ed39c33b12 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3617,34 +3617,31 @@ UniValue getaddressinfo(const JSONRPCRequest& request) ret.pushKV("label", pwallet->mapAddressBook[dest].name); } ret.pushKV("ischange", pwallet->IsChange(scriptPubKey)); - const CKeyMetadata* meta = nullptr; - const CKeyID *key_id = boost::get(&dest); - if (key_id != nullptr && !key_id->IsNull()) { - auto it = pwallet->mapKeyMetadata.find(*key_id); - if (it != pwallet->mapKeyMetadata.end()) { - meta = &it->second; - } - } - if (!meta) { - auto it = pwallet->m_script_metadata.find(CScriptID(scriptPubKey)); - if (it != pwallet->m_script_metadata.end()) { - meta = &it->second; - } - } - if (meta) { - ret.pushKV("timestamp", meta->nCreateTime); - CHDChain hdChainCurrent; - LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan(); - if (spk_man != nullptr) { - LOCK(pwallet->cs_KeyStore); - AssertLockHeld(spk_man->cs_KeyStore); - if (key_id && pwallet->mapHdPubKeys.count(*key_id) && spk_man->GetHDChain(hdChainCurrent)) { - ret.pushKV("hdchainid", hdChainCurrent.GetID().GetHex()); + ScriptPubKeyMan* spk_man = pwallet->GetScriptPubKeyMan(); + if (spk_man) { + const CKeyID *key_id = boost::get(&dest); + const CKeyMetadata* meta = nullptr; + if (key_id != nullptr && !key_id->IsNull()) { + meta = spk_man->GetMetadata(*key_id); + } + if (!meta) { + meta = spk_man->GetMetadata(CScriptID(scriptPubKey)); + } + if (meta) { + ret.pushKV("timestamp", meta->nCreateTime); + CHDChain hdChainCurrent; + LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan(); + if (spk_man != nullptr) { + LOCK(pwallet->cs_KeyStore); + AssertLockHeld(spk_man->cs_KeyStore); + if (key_id && pwallet->mapHdPubKeys.count(*key_id) && spk_man->GetHDChain(hdChainCurrent)) { + ret.pushKV("hdchainid", hdChainCurrent.GetID().GetHex()); + } + } + if (meta->has_key_origin) { + ret.pushKV("hdkeypath", WriteHDKeypath(meta->key_origin.path)); + ret.pushKV("hdmasterfingerprint", HexStr(meta->key_origin.fingerprint)); } - } - if (meta->has_key_origin) { - ret.pushKV("hdkeypath", WriteHDKeypath(meta->key_origin.path)); - ret.pushKV("hdmasterfingerprint", HexStr(meta->key_origin.fingerprint)); } } diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index f2e16cb5e2bd..ec1b80c03e65 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -678,6 +678,21 @@ size_t LegacyScriptPubKeyMan::KeypoolCountInternalKeys() return setInternalKeyPool.size(); } +const CKeyMetadata* LegacyScriptPubKeyMan::GetMetadata(uint160 id) const +{ + AssertLockHeld(cs_wallet); + auto it = mapKeyMetadata.find(CKeyID(id)); + if (it != mapKeyMetadata.end()) { + return &it->second; + } else { + auto it2 = m_script_metadata.find(CScriptID(id)); + if (it2 != m_script_metadata.end()) { + return &it2->second; + } + } + return nullptr; +} + /** * Update wallet first key creation time. This should be called whenever keys * are added to the wallet, with the oldest key creation time. diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 9102a9fe87f1..3fb9b618c646 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -154,6 +154,8 @@ class ScriptPubKeyMan virtual size_t KeypoolCountExternalKeys() { return 0; } virtual size_t KeypoolCountInternalKeys() { return 0; } + + virtual const CKeyMetadata* GetMetadata(uint160 id) const { return nullptr; } }; class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProvider @@ -258,6 +260,8 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv //! Fetches a key from the keypool bool GetKeyFromPool(CPubKey &key, bool fInternal /*= false*/); + const CKeyMetadata* GetMetadata(uint160 id) const override; + bool CanGetAddresses(bool internal = false) override; // Map from Key ID to key metadata. From 25094096744ad709aa064e4fefcd0b1b4f7d0be5 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Oct 2019 14:11:34 -0400 Subject: [PATCH 12/21] Refactor: Move MarkUnusedAddresses code out of CWallet::AddToWalletIfInvolvingMe This commit does not change behavior. --- src/wallet/scriptpubkeyman.cpp | 31 +++++++++++++++++++++++++++++++ src/wallet/scriptpubkeyman.h | 7 +++++++ src/wallet/wallet.cpp | 31 ++----------------------------- src/wallet/wallet.h | 1 - 4 files changed, 40 insertions(+), 30 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index ec1b80c03e65..258a324b4d47 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -288,6 +288,37 @@ bool LegacyScriptPubKeyMan::TopUp(unsigned int size) return TopUpKeyPool(size); } +void LegacyScriptPubKeyMan::MarkUnusedAddresses(WalletBatch &batch, const CScript& script, const uint256& hashBlock) +{ + AssertLockHeld(cs_wallet); + // extract addresses and check if they match with an unused keypool key + for (const auto& keyid : GetAffectedKeys(script, *this)) { + std::map::const_iterator mi = m_pool_key_to_index.find(keyid); + if (mi != m_pool_key_to_index.end()) { + WalletLogPrintf("%s: Detected a used keypool key, mark all keypool key up to this key as used\n", __func__); + MarkReserveKeysAsUsed(mi->second); + + if (!TopUpKeyPool()) { + WalletLogPrintf("%s: Topping up keypool failed (locked wallet)\n", __func__); + } + } + if (!hashBlock.IsNull()) { + int64_t block_time; + bool found_block = m_wallet.chain().findBlock(hashBlock, nullptr /* block */, &block_time); + assert(found_block); + if (mapKeyMetadata[keyid].nCreateTime > block_time) { + WalletLogPrintf("%s: Found a key which appears to be used earlier than we expected, updating metadata\n", __func__); + CPubKey vchPubKey; + bool res = GetPubKey(keyid, vchPubKey); + assert(res); // this should never fail + mapKeyMetadata[keyid].nCreateTime = block_time; + batch.WriteKeyMetadata(mapKeyMetadata[keyid], vchPubKey, true); + UpdateTimeFirstKey(block_time); + } + } + } +} + void LegacyScriptPubKeyMan::UpgradeKeyMetadata() { AssertLockHeld(cs_wallet); // mapKeyMetadata diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 3fb9b618c646..a280b56ea1f8 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -35,6 +35,8 @@ class WalletStorage //! Default for -keypool static const unsigned int DEFAULT_KEYPOOL_SIZE = 1000; +std::vector GetAffectedKeys(const CScript& spk, const SigningProvider& provider); + /** A key from a CWallet's keypool * * The wallet holds several keypools. These are sets of keys that have not @@ -144,6 +146,9 @@ class ScriptPubKeyMan virtual bool TopUp(unsigned int size = 0) { return false; } + //! Mark unused addresses as being used + virtual void MarkUnusedAddresses(WalletBatch &batch, const CScript& script, const uint256& hashBlock) {} + /* Returns true if HD is enabled */ virtual bool IsHDEnabled() const { return false; } @@ -247,6 +252,8 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv bool TopUp(unsigned int size = 0) override; + void MarkUnusedAddresses(WalletBatch &batch, const CScript& script, const uint256& hashBlock) override; + //! Upgrade stored CKeyMetadata objects to store key origin info as KeyOriginInfo void UpgradeKeyMetadata() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7efd96a16d66..24b7e7c8c276 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -337,8 +337,6 @@ std::string COutput::ToString() const return strprintf("COutput(%s, %d, %d) [%s]", tx->GetHash().ToString(), i, nDepth, FormatMoney(tx->tx->vout[i].nValue)); } -std::vector GetAffectedKeys(const CScript& spk, const SigningProvider& provider); - const CWalletTx* CWallet::GetWalletTx(const uint256& hash) const { LOCK(cs_wallet); @@ -980,33 +978,8 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Co WalletBatch batch(*database); // loop though all outputs for (const CTxOut& txout: tx.vout) { - // extract addresses, check if they match with an unused keypool key, update metadata if needed - if (m_spk_man == nullptr) continue; - AssertLockHeld(m_spk_man->cs_wallet); - for (const auto& keyid : GetAffectedKeys(txout.scriptPubKey, *m_spk_man)) { - std::map::const_iterator mi = m_spk_man->m_pool_key_to_index.find(keyid); - if (mi != m_spk_man->m_pool_key_to_index.end()) { - WalletLogPrintf("%s: Detected a used keypool key, mark all keypool key up to this key as used\n", __func__); - MarkReserveKeysAsUsed(mi->second); - - if (!m_spk_man->TopUp()) { - WalletLogPrintf("%s: Topping up keypool failed (locked wallet)\n", __func__); - } - } - if (!confirm.hashBlock.IsNull()) { - int64_t block_time; - bool found_block = chain().findBlock(confirm.hashBlock, nullptr /* block */, &block_time); - assert(found_block); - if (mapKeyMetadata[keyid].nCreateTime > block_time) { - WalletLogPrintf("%s: Found a key which appears to be used earlier than we expected, updating metadata\n", __func__); - CPubKey vchPubKey; - bool res = m_spk_man->GetPubKey(keyid, vchPubKey); - assert(res); // this should never fail - mapKeyMetadata[keyid].nCreateTime = block_time; - batch.WriteKeyMetadata(mapKeyMetadata[keyid], vchPubKey, true); - m_spk_man->UpdateTimeFirstKey(block_time); - } - } + if (auto spk_man = m_spk_man.get()) { + spk_man->MarkUnusedAddresses(batch, txout.scriptPubKey, confirm.hashBlock); } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 14dcdb12ea53..482f0d02021d 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1295,7 +1295,6 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati int64_t& nTimeFirstKey GUARDED_BY(cs_wallet) = m_spk_man->nTimeFirstKey; std::map& mapKeyMetadata GUARDED_BY(cs_wallet) = m_spk_man->mapKeyMetadata; std::map& m_script_metadata GUARDED_BY(cs_wallet) = m_spk_man->m_script_metadata; - void MarkReserveKeysAsUsed(int64_t keypool_id) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(m_spk_man->cs_wallet); m_spk_man->MarkReserveKeysAsUsed(keypool_id); } using CryptedKeyMap = LegacyScriptPubKeyMan::CryptedKeyMap; }; From 6ebf30c9005bf197411fb5a0eb6c43ebbb5ec6ee Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Oct 2019 14:11:34 -0400 Subject: [PATCH 13/21] Refactor: Move HavePrivateKeys code out of CWallet::CreateWalletFromFile This commit does not change behavior. --- src/wallet/scriptpubkeyman.cpp | 11 +++++++++++ src/wallet/scriptpubkeyman.h | 4 ++++ src/wallet/wallet.cpp | 7 ++++--- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 258a324b4d47..1b8fe6b0a401 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -666,7 +666,18 @@ bool LegacyScriptPubKeyMan::CanGetAddresses(bool internal) return keypool_has_keys; } + +bool LegacyScriptPubKeyMan::HavePrivateKeys() const +{ + LOCK(cs_KeyStore); + return !mapKeys.empty() || !mapCryptedKeys.empty(); +} + static int64_t GetOldestKeyInPool(const std::set& setKeyPool, WalletBatch& batch) { + if (setKeyPool.empty()) { + return GetTime(); + } + CKeyPool keypool; int64_t nIndex = *(setKeyPool.begin()); if (!batch.ReadPool(nIndex, keypool)) { diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index a280b56ea1f8..a45ef79100b8 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -155,6 +155,8 @@ class ScriptPubKeyMan /* Returns true if the wallet can give out new addresses. This means it has keys in the keypool or can generate new keys */ virtual bool CanGetAddresses(bool internal = false) { return false; } + virtual bool HavePrivateKeys() const { return false; } + virtual int64_t GetOldestKeyPoolTime() { return GetTime(); } virtual size_t KeypoolCountExternalKeys() { return 0; } @@ -260,6 +262,8 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv /* Returns true if HD is enabled */ bool IsHDEnabled() const override; + bool HavePrivateKeys() const override; + int64_t GetOldestKeyPoolTime() override; size_t KeypoolCountExternalKeys() override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); size_t KeypoolCountInternalKeys() override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 24b7e7c8c276..3d321945808d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4358,9 +4358,10 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, error = strprintf(_("Error loading %s: Private keys can only be disabled during creation"), walletFile); return NULL; } else if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { - LOCK(walletInstance->cs_KeyStore); - if (!walletInstance->mapKeys.empty() || !walletInstance->mapCryptedKeys.empty()) { - warnings.push_back(strprintf(_("Warning: Private keys detected in wallet {%s} with disabled private keys"), walletFile)); + if (walletInstance->m_spk_man) { + if (walletInstance->m_spk_man->HavePrivateKeys()) { + warnings.push_back(strprintf(_("Warning: Private keys detected in wallet {%s} with disabled private keys"), walletFile)); + } } } else if (gArgs.IsArgSet("-usehd")) { From 9a4c2301144b1b419089d814d05445cb8f5979c3 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Oct 2019 14:11:34 -0400 Subject: [PATCH 14/21] Refactor: Move RewriteDB code out of CWallet This commit does not change behavior. --- src/wallet/scriptpubkeyman.cpp | 11 +++++++++++ src/wallet/scriptpubkeyman.h | 5 +++++ src/wallet/wallet.cpp | 18 ++++++------------ 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 1b8fe6b0a401..52fad8ae7350 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -673,6 +673,17 @@ bool LegacyScriptPubKeyMan::HavePrivateKeys() const return !mapKeys.empty() || !mapCryptedKeys.empty(); } +void LegacyScriptPubKeyMan::RewriteDB() +{ + AssertLockHeld(cs_wallet); + setInternalKeyPool.clear(); + setExternalKeyPool.clear(); + m_pool_key_to_index.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. +} + static int64_t GetOldestKeyInPool(const std::set& setKeyPool, WalletBatch& batch) { if (setKeyPool.empty()) { return GetTime(); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index a45ef79100b8..e9523d0374f6 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -157,6 +157,9 @@ class ScriptPubKeyMan virtual bool HavePrivateKeys() const { return false; } + //! The action to do when the DB needs rewrite + virtual void RewriteDB() {} + virtual int64_t GetOldestKeyPoolTime() { return GetTime(); } virtual size_t KeypoolCountExternalKeys() { return 0; } @@ -264,6 +267,8 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv bool HavePrivateKeys() const override; + void RewriteDB() override; + int64_t GetOldestKeyPoolTime() override; size_t KeypoolCountExternalKeys() override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); size_t KeypoolCountInternalKeys() override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3d321945808d..3fc04415b54f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3567,13 +3567,10 @@ DBErrors CWallet::LoadWallet(bool& fFirstRunRet) { if (database->Rewrite("\x04pool")) { - setInternalKeyPool.clear(); - setExternalKeyPool.clear(); + if (auto spk_man = m_spk_man.get()) { + spk_man->RewriteDB(); + } nKeysLeftSinceAutoBackup = 0; - m_spk_man->m_pool_key_to_index.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. } } @@ -3638,12 +3635,9 @@ DBErrors CWallet::ZapSelectTx(std::vector& vHashIn, std::vectorRewrite("\x04pool")) { - setInternalKeyPool.clear(); - setExternalKeyPool.clear(); - m_spk_man->m_pool_key_to_index.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. + if (auto spk_man = m_spk_man.get()) { + spk_man->RewriteDB(); + } } } From 8d8eaa643c5fc2cd5fcbf1d711525b2a123e08aa Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Oct 2019 14:11:34 -0400 Subject: [PATCH 15/21] Refactor: Move GetKeypoolSize code out of CWallet This commit does not change behavior. --- src/wallet/scriptpubkeyman.cpp | 6 ++++++ src/wallet/scriptpubkeyman.h | 2 ++ src/wallet/wallet.cpp | 11 +++++++++++ src/wallet/wallet.h | 8 +------- 4 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 52fad8ae7350..e5e6f66071dd 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -731,6 +731,12 @@ size_t LegacyScriptPubKeyMan::KeypoolCountInternalKeys() return setInternalKeyPool.size(); } +unsigned int LegacyScriptPubKeyMan::GetKeyPoolSize() const +{ + AssertLockHeld(cs_wallet); + return setInternalKeyPool.size() + setExternalKeyPool.size(); +} + const CKeyMetadata* LegacyScriptPubKeyMan::GetMetadata(uint160 id) const { AssertLockHeld(cs_wallet); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index e9523d0374f6..3a3f3e826e40 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -164,6 +164,7 @@ class ScriptPubKeyMan virtual size_t KeypoolCountExternalKeys() { return 0; } virtual size_t KeypoolCountInternalKeys() { return 0; } + virtual unsigned int GetKeyPoolSize() const { return 0; } virtual const CKeyMetadata* GetMetadata(uint160 id) const { return nullptr; } }; @@ -275,6 +276,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv //! Fetches a key from the keypool bool GetKeyFromPool(CPubKey &key, bool fInternal /*= false*/); + unsigned int GetKeyPoolSize() const override; const CKeyMetadata* GetMetadata(uint160 id) const override; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3fc04415b54f..dee706510df9 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3719,6 +3719,17 @@ size_t CWallet::KeypoolCountInternalKeys() return count; } +unsigned int CWallet::GetKeyPoolSize() const +{ + AssertLockHeld(cs_wallet); + + unsigned int count = 0; + if (auto spk_man = m_spk_man.get()) { + count += spk_man->GetKeyPoolSize(); + } + return count; +} + bool CWallet::TopUpKeyPool(unsigned int kpSize) { bool res = true; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 482f0d02021d..e6c3c0b40347 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1108,11 +1108,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati bool DelAddressBook(const CTxDestination& address); - unsigned int GetKeyPoolSize() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) - { - AssertLockHeld(cs_wallet); - return setInternalKeyPool.size() + setExternalKeyPool.size(); - } + unsigned int GetKeyPoolSize() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! signify that a particular wallet feature is now used. this may change nWalletVersion and nWalletMaxVersion if those are lower void SetMinVersion(enum WalletFeature, WalletBatch* batch_in = nullptr, bool fExplicit = false) override; @@ -1290,8 +1286,6 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati LegacyScriptPubKeyMan::WatchKeyMap& mapWatchKeys GUARDED_BY(cs_KeyStore) = m_spk_man->mapWatchKeys; LegacyScriptPubKeyMan::HDPubKeyMap& mapHdPubKeys GUARDED_BY(cs_KeyStore) = m_spk_man->mapHdPubKeys; WalletBatch*& encrypted_batch GUARDED_BY(cs_wallet) = m_spk_man->encrypted_batch; - std::set& setInternalKeyPool GUARDED_BY(cs_wallet) = m_spk_man->setInternalKeyPool; - std::set& setExternalKeyPool GUARDED_BY(cs_wallet) = m_spk_man->setExternalKeyPool; int64_t& nTimeFirstKey GUARDED_BY(cs_wallet) = m_spk_man->nTimeFirstKey; std::map& mapKeyMetadata GUARDED_BY(cs_wallet) = m_spk_man->mapKeyMetadata; std::map& m_script_metadata GUARDED_BY(cs_wallet) = m_spk_man->m_script_metadata; From 084cf053ea96f037f493fd258b40fd74f4e86e18 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Oct 2019 14:11:34 -0400 Subject: [PATCH 16/21] Refactor: Move nTimeFirstKey accesses out of CWallet This commit does not change behavior. --- src/wallet/scriptpubkeyman.cpp | 12 ++++++------ src/wallet/scriptpubkeyman.h | 6 ++++-- src/wallet/wallet.cpp | 11 ++++++++--- src/wallet/wallet.h | 3 --- 4 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index e5e6f66071dd..740d618a4eaa 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -737,6 +737,12 @@ unsigned int LegacyScriptPubKeyMan::GetKeyPoolSize() const return setInternalKeyPool.size() + setExternalKeyPool.size(); } +int64_t LegacyScriptPubKeyMan::GetTimeFirstKey() const +{ + AssertLockHeld(cs_wallet); + return nTimeFirstKey; +} + const CKeyMetadata* LegacyScriptPubKeyMan::GetMetadata(uint160 id) const { AssertLockHeld(cs_wallet); @@ -768,12 +774,6 @@ void LegacyScriptPubKeyMan::UpdateTimeFirstKey(int64_t nCreateTime) } } -int64_t LegacyScriptPubKeyMan::GetTimeFirstKey() const -{ - AssertLockHeld(cs_wallet); - return nTimeFirstKey; -} - bool LegacyScriptPubKeyMan::LoadKey(const CKey& key, const CPubKey &pubkey) { return AddKeyPubKeyInner(key, pubkey); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 3a3f3e826e40..9f042f66f7f8 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -166,6 +166,8 @@ class ScriptPubKeyMan virtual size_t KeypoolCountInternalKeys() { return 0; } virtual unsigned int GetKeyPoolSize() const { return 0; } + virtual int64_t GetTimeFirstKey() const { return 0; } + virtual const CKeyMetadata* GetMetadata(uint160 id) const { return nullptr; } }; @@ -278,6 +280,8 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv bool GetKeyFromPool(CPubKey &key, bool fInternal /*= false*/); unsigned int GetKeyPoolSize() const override; + int64_t GetTimeFirstKey() const override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + const CKeyMetadata* GetMetadata(uint160 id) const override; bool CanGetAddresses(bool internal = false) override; @@ -366,8 +370,6 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv /* Returns true if the wallet can generate new keys */ bool CanGenerateKeys(); - int64_t GetTimeFirstKey() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - //! Adds a HDPubKey into the wallet(database) bool AddHDPubKey(WalletBatch &batch, const CExtPubKey &extPubKey, bool fInternal); //! loads a HDPubKey into the wallets memory diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index dee706510df9..954ceb911bf0 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4531,8 +4531,13 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, // our wallet birthday (as adjusted for block time variability) // unless a full rescan was requested if (gArgs.GetArg("-rescan", 0) != 2) { - if (walletInstance->nTimeFirstKey) { - if (Optional first_block = chain.findFirstBlockWithTimeAndHeight(walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW, rescan_height, nullptr)) { + Optional time_first_key; + if (auto spk_man = walletInstance->m_spk_man.get()) { + int64_t time = spk_man->GetTimeFirstKey(); + if (!time_first_key || time < *time_first_key) time_first_key = time; + } + if (time_first_key) { + if (Optional first_block = chain.findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, nullptr)) { rescan_height = *first_block; } } @@ -4562,7 +4567,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, walletInstance->WalletLogPrintf("setInternalKeyPool.size() = %u\n", walletInstance->KeypoolCountInternalKeys()); walletInstance->WalletLogPrintf("mapWallet.size() = %u\n", walletInstance->mapWallet.size()); walletInstance->WalletLogPrintf("mapAddressBook.size() = %u\n", walletInstance->mapAddressBook.size()); - walletInstance->WalletLogPrintf("nTimeFirstKey = %u\n", walletInstance->nTimeFirstKey); + walletInstance->WalletLogPrintf("nTimeFirstKey = %u\n", walletInstance->m_spk_man->GetTimeFirstKey()); } return walletInstance; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index e6c3c0b40347..272299e397e8 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1286,9 +1286,6 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati LegacyScriptPubKeyMan::WatchKeyMap& mapWatchKeys GUARDED_BY(cs_KeyStore) = m_spk_man->mapWatchKeys; LegacyScriptPubKeyMan::HDPubKeyMap& mapHdPubKeys GUARDED_BY(cs_KeyStore) = m_spk_man->mapHdPubKeys; WalletBatch*& encrypted_batch GUARDED_BY(cs_wallet) = m_spk_man->encrypted_batch; - int64_t& nTimeFirstKey GUARDED_BY(cs_wallet) = m_spk_man->nTimeFirstKey; - std::map& mapKeyMetadata GUARDED_BY(cs_wallet) = m_spk_man->mapKeyMetadata; - std::map& m_script_metadata GUARDED_BY(cs_wallet) = m_spk_man->m_script_metadata; using CryptedKeyMap = LegacyScriptPubKeyMan::CryptedKeyMap; }; From 1e889f2439361e1f3b9b95bd1d9d5c33abda63a1 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Fri, 19 Aug 2022 00:16:36 +0700 Subject: [PATCH 17/21] Re-order methods of scriptpubkeyman for easier backporting changes in future --- src/wallet/scriptpubkeyman.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 9f042f66f7f8..359f8a2b2b36 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -206,9 +206,9 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv * nTimeFirstKey more intelligently for more efficient rescans. */ bool AddWatchOnly(const CScript& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool AddWatchOnlyWithDB(WalletBatch &batch, const CScript& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool AddWatchOnlyInMem(const CScript &dest); - bool AddWatchOnlyWithDB(WalletBatch &batch, const CScript& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void AddKeypoolPubkeyWithDB(const CPubKey& pubkey, const bool internal, WalletBatch& batch); @@ -227,6 +227,8 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv int64_t m_max_keypool_index GUARDED_BY(cs_wallet) = 0; std::map m_pool_key_to_index; + //! 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 * @@ -276,8 +278,6 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv size_t KeypoolCountExternalKeys() override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); size_t KeypoolCountInternalKeys() override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - //! Fetches a key from the keypool - bool GetKeyFromPool(CPubKey &key, bool fInternal /*= false*/); unsigned int GetKeyPoolSize() const override; int64_t GetTimeFirstKey() const override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); From 4b8f9845ace2bdd25bffa1fc48f880d807715163 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 30 Aug 2022 16:56:03 +0700 Subject: [PATCH 18/21] Fixup for missing cs_wallet lock: ``` wallet/wallet.cpp:4536:41: error: calling function 'GetTimeFirstKey' requires holding mutex 'spk_man->cs_wallet' exclusively [-Werror,-Wthread-safety-analysis] int64_t time = spk_man->GetTimeFirstKey(); ^ wallet/wallet.cpp:4570:106: error: calling function 'GetTimeFirstKey' requires holding mutex 'walletInstance->m_spk_man->cs_wallet' exclusively [-Werror,-Wthread-safety-analysis] walletInstance->WalletLogPrintf("nTimeFirstKey = %u\n", walletInstance->m_spk_man->GetTimeFirstKey()); ``` --- src/wallet/wallet.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 954ceb911bf0..cdf6b85ae6d6 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4533,6 +4533,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, if (gArgs.GetArg("-rescan", 0) != 2) { Optional time_first_key; if (auto spk_man = walletInstance->m_spk_man.get()) { + LOCK(walletInstance->cs_wallet); int64_t time = spk_man->GetTimeFirstKey(); if (!time_first_key || time < *time_first_key) time_first_key = time; } @@ -4567,7 +4568,10 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, walletInstance->WalletLogPrintf("setInternalKeyPool.size() = %u\n", walletInstance->KeypoolCountInternalKeys()); walletInstance->WalletLogPrintf("mapWallet.size() = %u\n", walletInstance->mapWallet.size()); walletInstance->WalletLogPrintf("mapAddressBook.size() = %u\n", walletInstance->mapAddressBook.size()); - walletInstance->WalletLogPrintf("nTimeFirstKey = %u\n", walletInstance->m_spk_man->GetTimeFirstKey()); + { + LOCK(walletInstance->cs_wallet); + walletInstance->WalletLogPrintf("nTimeFirstKey = %u\n", walletInstance->m_spk_man->GetTimeFirstKey()); + } } return walletInstance; From 62f7d93e34e2d3550754e8bf069f498118addb5c Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 31 Aug 2022 18:09:39 +0300 Subject: [PATCH 19/21] Fix 2 locks --- src/wallet/wallet.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index cdf6b85ae6d6..ccfdc70b4f9e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4533,7 +4533,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, if (gArgs.GetArg("-rescan", 0) != 2) { Optional time_first_key; if (auto spk_man = walletInstance->m_spk_man.get()) { - LOCK(walletInstance->cs_wallet); + LOCK(spk_man->cs_wallet); int64_t time = spk_man->GetTimeFirstKey(); if (!time_first_key || time < *time_first_key) time_first_key = time; } @@ -4568,9 +4568,9 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, walletInstance->WalletLogPrintf("setInternalKeyPool.size() = %u\n", walletInstance->KeypoolCountInternalKeys()); walletInstance->WalletLogPrintf("mapWallet.size() = %u\n", walletInstance->mapWallet.size()); walletInstance->WalletLogPrintf("mapAddressBook.size() = %u\n", walletInstance->mapAddressBook.size()); - { - LOCK(walletInstance->cs_wallet); - walletInstance->WalletLogPrintf("nTimeFirstKey = %u\n", walletInstance->m_spk_man->GetTimeFirstKey()); + if (auto spk_man = walletInstance->m_spk_man.get()) { + LOCK(spk_man->cs_wallet); + walletInstance->WalletLogPrintf("nTimeFirstKey = %u\n", spk_man->GetTimeFirstKey()); } } From 04fbaff48b71838249e012a3868e0e6885dda4f5 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 31 Aug 2022 18:54:02 +0300 Subject: [PATCH 20/21] more of "refactor: Replace UnsetWalletFlagWithDB with UnsetBlankWalletFlag in ScriptPubKeyMan" --- src/wallet/scriptpubkeyman.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 740d618a4eaa..c7e2ef9adff5 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -472,7 +472,7 @@ bool LegacyScriptPubKeyMan::SetHDChain(WalletBatch &batch, const CHDChain& chain throw std::runtime_error(std::string(__func__) + ": WriteHDChain failed"); } - m_wallet.UnsetWalletFlagWithDB(batch, WALLET_FLAG_BLANK_WALLET); + m_storage.UnsetBlankWalletFlag(batch); } return true; @@ -493,7 +493,7 @@ bool LegacyScriptPubKeyMan::SetCryptedHDChain(WalletBatch &batch, const CHDChain if (!batch.WriteCryptedHDChain(chain)) throw std::runtime_error(std::string(__func__) + ": WriteCryptedHDChain failed"); } - m_wallet.UnsetWalletFlagWithDB(batch, WALLET_FLAG_BLANK_WALLET); + m_storage.UnsetBlankWalletFlag(batch); } return true; @@ -1114,7 +1114,7 @@ bool LegacyScriptPubKeyMan::AddHDPubKey(WalletBatch &batch, const CExtPubKey &ex if (!batch.WriteHDPubKey(hdPubKey, mapKeyMetadata[extPubKey.pubkey.GetID()])) { return false; } - m_wallet.UnsetWalletFlagWithDB(batch, WALLET_FLAG_BLANK_WALLET); + m_storage.UnsetBlankWalletFlag(batch); return true; } From c1fbc8b6bb50fdc67ae3ff3686dd41c8dba6d303 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 13 Sep 2022 22:58:47 +0700 Subject: [PATCH 21/21] Refactoring GetOldestKeyInPool -> GetOldestKeyTimeInPool, partial bitcoin#10235 --- src/wallet/scriptpubkeyman.cpp | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index c7e2ef9adff5..f91e793c2a46 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -684,8 +684,9 @@ void LegacyScriptPubKeyMan::RewriteDB() // that requires a new key. } -static int64_t GetOldestKeyInPool(const std::set& setKeyPool, WalletBatch& batch) { +static int64_t GetOldestKeyTimeInPool(const std::set& setKeyPool, WalletBatch& batch) { if (setKeyPool.empty()) { + // if the keypool is empty, return return GetTime(); } @@ -702,19 +703,11 @@ int64_t LegacyScriptPubKeyMan::GetOldestKeyPoolTime() { LOCK(cs_wallet); - // if the keypool is empty, return - if (setExternalKeyPool.empty() && setInternalKeyPool.empty()) - return GetTime(); - WalletBatch batch(m_storage.GetDatabase()); - int64_t oldestKey = -1; + int64_t oldestKey = GetOldestKeyTimeInPool(setExternalKeyPool, batch); - // load oldest key from keypool, get time and return - if (!setInternalKeyPool.empty()) { - oldestKey = std::max(GetOldestKeyInPool(setInternalKeyPool, batch), oldestKey); - } - if (!setExternalKeyPool.empty()) { - oldestKey = std::max(GetOldestKeyInPool(setExternalKeyPool, batch), oldestKey); + if (IsHDEnabled()) { + oldestKey = std::max(GetOldestKeyTimeInPool(setInternalKeyPool, batch), oldestKey); } return oldestKey; }