From b4cb18bce3c9a6adab2fa32f3dad4ad966b33be0 Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Mon, 7 Oct 2019 14:11:34 -0400
Subject: [PATCH 01/18] 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.h | 193 +++++++++++++++++------------------
 1 file changed, 96 insertions(+), 97 deletions(-)

diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index 16a5c9b97..5b2f25f09 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -150,36 +150,22 @@ public:
 class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProvider
 {
 private:
-    using CryptedKeyMap = std::map<CKeyID, std::pair<CPubKey, std::vector<unsigned char>>>;
     using WatchOnlySet = std::set<CScript>;
     using WatchKeyMap = std::map<CKeyID, CPubKey>;
 
-    //! will encrypt previously unencrypted keys
-    bool EncryptKeys(CKeyingMaterial& vMasterKeyIn);
+    WalletBatch *encrypted_batch GUARDED_BY(cs_wallet) = nullptr;
+
+    using CryptedKeyMap = std::map<CKeyID, std::pair<CPubKey, std::vector<unsigned char>>>;
 
     CryptedKeyMap mapCryptedKeys GUARDED_BY(cs_KeyStore);
     WatchOnlySet setWatchOnly GUARDED_BY(cs_KeyStore);
     WatchKeyMap mapWatchKeys GUARDED_BY(cs_KeyStore);
 
-    bool AddCryptedKeyInner(const CPubKey &vchPubKey, const std::vector<unsigned char> &vchCryptedSecret);
-    bool AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey);
-
-    WalletBatch *encrypted_batch GUARDED_BY(cs_wallet) = nullptr;
-
-    /* the HD chain data model (external chain counters) */
-    CHDChain hdChain;
-
-    /* HD derive new child key (on internal or external chain) */
-    void DeriveNewChildKey(WalletBatch& batch, CKeyMetadata& metadata, CKey& secret, bool internal = false) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
-
-    std::set<int64_t> setInternalKeyPool GUARDED_BY(cs_wallet);
-    std::set<int64_t> setExternalKeyPool GUARDED_BY(cs_wallet);
-    std::set<int64_t> set_pre_split_keypool GUARDED_BY(cs_wallet);
-    int64_t m_max_keypool_index GUARDED_BY(cs_wallet) = 0;
-    std::map<CKeyID, int64_t> m_pool_key_to_index;
-
     int64_t nTimeFirstKey GUARDED_BY(cs_wallet) = 0;
 
+    bool AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey);
+    bool AddCryptedKeyInner(const CPubKey &vchPubKey, const std::vector<unsigned char> &vchCryptedSecret);
+
     /**
      * Private version of AddWatchOnly method which does not accept a
      * timestamp, and which will reset the wallet's nTimeFirstKey value to 1 if
@@ -192,80 +178,34 @@ private:
     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);
-
-    /** Add a KeyOriginInfo to the wallet */
-    bool AddKeyOriginWithDB(WalletBatch& batch, const CPubKey& pubkey, const KeyOriginInfo& info);
+    //! 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);
 
     //! 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);
 
-    //! 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);
-
     void AddKeypoolPubkeyWithDB(const CPubKey& pubkey, const bool internal, WalletBatch& batch);
 
     //! Adds a script to the store and saves it to disk
     bool AddCScriptWithDB(WalletBatch& batch, const CScript& script);
 
- public:
+    /** 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;
+
+    /* HD derive new child key (on internal or external chain) */
+    void DeriveNewChildKey(WalletBatch& batch, CKeyMetadata& metadata, CKey& secret, bool internal = false) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+
+    std::set<int64_t> setInternalKeyPool GUARDED_BY(cs_wallet);
+    std::set<int64_t> setExternalKeyPool GUARDED_BY(cs_wallet);
+    std::set<int64_t> set_pre_split_keypool GUARDED_BY(cs_wallet);
+    int64_t m_max_keypool_index GUARDED_BY(cs_wallet) = 0;
+    std::map<CKeyID, int64_t> m_pool_key_to_index;
+
     //! Fetches a key from the keypool
     bool GetKeyFromPool(CPubKey &key, bool internal = false);
-    void LoadKeyPool(int64_t nIndex, const CKeyPool &keypool) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
-    void MarkPreSplitKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
-
-    // Map from Key ID to key metadata.
-    std::map<CKeyID, CKeyMetadata> mapKeyMetadata GUARDED_BY(cs_wallet);
-
-    // Map from Script ID to key metadata (for watch-only keys).
-    std::map<CScriptID, CKeyMetadata> m_script_metadata GUARDED_BY(cs_wallet);
-
-    /**
-     * keystore implementation
-     * Generate a new key
-     */
-    CPubKey GenerateNewKey(WalletBatch& batch, bool internal = 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);
-
-    //! Adds an encrypted key to the store, and saves it to disk.
-    bool AddCryptedKey(const CPubKey &vchPubKey, const std::vector<unsigned char> &vchCryptedSecret);
-    //! Adds an encrypted key to the store, without saving it to disk (used by LoadWallet)
-    bool LoadCryptedKey(const CPubKey &vchPubKey, const std::vector<unsigned char> &vchCryptedSecret);
-    bool GetKey(const CKeyID &address, CKey& keyOut) const override;
-    bool GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const override;
-    bool HaveKey(const CKeyID &address) const override;
-    std::set<CKeyID> GetKeys() const override;
-    bool AddCScript(const CScript& redeemScript) override;
-    bool LoadCScript(const CScript& redeemScript);
-
-    //! 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 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;
-    //! Fetches a pubkey from mapWatchKeys if it exists there
-    bool GetWatchPubKey(const CKeyID &address, CPubKey &pubkey_out) const;
-
-    bool ImportScripts(const std::set<CScript> scripts, int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
-    bool ImportPrivKeys(const std::map<CKeyID, CKey>& privkey_map, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
-    bool ImportPubKeys(const std::vector<CKeyID>& ordered_pubkeys, const std::map<CKeyID, CPubKey>& pubkey_map, const std::map<CKeyID, std::pair<CPubKey, KeyOriginInfo>>& 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<CScript>& 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);
-    bool TopUpKeyPool(unsigned int kpSize = 0);
 
     /**
      * Reserves a key from the keypool and sets nIndex to its index
@@ -282,31 +222,86 @@ private:
      *     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);
-    int64_t GetOldestKeyPoolTime();
-    /**
-     * 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<CKeyID, int64_t>& GetAllReserveKeys() const { return m_pool_key_to_index; }
-    bool GetNewDestination(const OutputType type, const std::string label, CTxDestination& dest, std::string& error);
 
+public:
+    bool GetNewDestination(const OutputType type, const std::string label, CTxDestination& dest, std::string& error);
     isminetype IsMine(const CScript& script) const;
 
+    //! will encrypt previously unencrypted keys
+    bool EncryptKeys(CKeyingMaterial& vMasterKeyIn);
+    void UpgradeKeyMetadata() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+
+    bool IsHDEnabled() const;
+
+    int64_t GetOldestKeyPoolTime();
+    size_t KeypoolCountExternalKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+
+    /* 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);
+
+    // Map from Key ID to key metadata.
+    std::map<CKeyID, CKeyMetadata> mapKeyMetadata GUARDED_BY(cs_wallet);
+
+    // Map from Script ID to key metadata (for watch-only keys).
+    std::map<CScriptID, CKeyMetadata> m_script_metadata GUARDED_BY(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); }
+    //! Adds an encrypted key to the store, and saves it to disk.
+    bool AddCryptedKey(const CPubKey &vchPubKey, const std::vector<unsigned char> &vchCryptedSecret);
+    //! Adds an encrypted key to the store, without saving it to disk (used by LoadWallet)
+    bool LoadCryptedKey(const CPubKey &vchPubKey, const std::vector<unsigned char> &vchCryptedSecret);
+    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);
+    void LoadScriptMetadata(const CScriptID& script_id, const CKeyMetadata &metadata) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    //! Generate a new key
+    CPubKey GenerateNewKey(WalletBatch& batch, bool internal = false) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+
     /* Set the HD chain model (chain child index counters) */
     void SetHDChain(const CHDChain& chain, bool memonly);
     const CHDChain& GetHDChain() const { return hdChain; }
 
-    /* Returns true if HD is enabled */
-    bool IsHDEnabled() const;
+    //! 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);
+    bool AddWatchOnly(const CScript& dest, int64_t nCreateTime) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+
+    //! Fetches a pubkey from mapWatchKeys if it exists there
+    bool GetWatchPubKey(const CKeyID &address, CPubKey &pubkey_out) const;
+
+    bool HaveKey(const CKeyID &address) const override;
+    bool GetKey(const CKeyID &address, CKey& keyOut) const override;
+    bool GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const override;
+    bool AddCScript(const CScript& redeemScript) override;
+    bool GetKeyOrigin(const CKeyID& keyid, KeyOriginInfo& info) const override;
+
+    //! Load a keypool entry
+    void LoadKeyPool(int64_t nIndex, const CKeyPool &keypool) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    bool TopUpKeyPool(unsigned int kpSize = 0);
+    bool NewKeyPool();
+    void MarkPreSplitKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+
+    bool ImportScripts(const std::set<CScript> scripts, int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    bool ImportPrivKeys(const std::map<CKeyID, CKey>& privkey_map, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    bool ImportPubKeys(const std::vector<CKeyID>& ordered_pubkeys, const std::map<CKeyID, CPubKey>& pubkey_map, const std::map<CKeyID, std::pair<CPubKey, KeyOriginInfo>>& 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<CScript>& script_pub_keys, const bool have_solving_data, const bool apply_label, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     /* 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 seed (will not be activated) */
     CPubKey GenerateNewSeed();
 
@@ -333,9 +328,13 @@ private:
      */
     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<CKeyID, int64_t>& GetAllReserveKeys() const { return m_pool_key_to_index; }
 
+    std::set<CKeyID> GetKeys() const override;
     // Temporary CWallet accessors and aliases.
     friend class CWallet;
     friend class ReserveDestination;

From 533d8b364f4e589aa1acb28cdea5e6e6e80d34dc Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Mon, 7 Oct 2019 14:11:34 -0400
Subject: [PATCH 02/18] Refactor: Declare LegacyScriptPubKeyMan methods as
 virtual

This commit does not change behavior.
---
 src/wallet/scriptpubkeyman.h | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index 5b2f25f09..cef153d2b 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -145,6 +145,21 @@ protected:
 
 public:
     ScriptPubKeyMan(WalletStorage& storage) : m_storage(storage) {}
+    virtual ~ScriptPubKeyMan() {};
+    virtual isminetype IsMine(const CScript& script) 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; }
 };
 
 class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProvider
@@ -228,19 +243,18 @@ private:
 
 public:
     bool GetNewDestination(const OutputType type, const std::string label, CTxDestination& dest, std::string& error);
-    isminetype IsMine(const CScript& script) const;
+    isminetype IsMine(const CScript& script) const override;
 
     //! will encrypt previously unencrypted keys
     bool EncryptKeys(CKeyingMaterial& vMasterKeyIn);
-    void UpgradeKeyMetadata() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    void UpgradeKeyMetadata() override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
-    bool IsHDEnabled() const;
+    bool IsHDEnabled() const override;
 
-    int64_t GetOldestKeyPoolTime();
-    size_t KeypoolCountExternalKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    int64_t GetOldestKeyPoolTime() override;
+    size_t KeypoolCountExternalKeys() override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
-    /* 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<CKeyID, CKeyMetadata> mapKeyMetadata GUARDED_BY(cs_wallet);
@@ -282,6 +296,7 @@ public:
     //! Fetches a pubkey from mapWatchKeys if it exists there
     bool GetWatchPubKey(const CKeyID &address, CPubKey &pubkey_out) const;
 
+    /* SigningProvider overrides */
     bool HaveKey(const CKeyID &address) const override;
     bool GetKey(const CKeyID &address, CKey& keyOut) const override;
     bool GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const override;

From acedc5b8230ed9ad07f96f51f0ef862ab3a41d5e Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Mon, 7 Oct 2019 14:11:34 -0400
Subject: [PATCH 03/18] Refactor: Add new ScriptPubKeyMan virtual methods

This commit does not change behavior.
---
 src/wallet/scriptpubkeyman.cpp | 25 +++++++++++++++++++++++++
 src/wallet/scriptpubkeyman.h   | 17 ++++++++++++++---
 src/wallet/wallet.cpp          | 16 ++++++++--------
 3 files changed, 47 insertions(+), 11 deletions(-)

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 259bfcd76..41ef12107 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -265,6 +265,31 @@ bool LegacyScriptPubKeyMan::EncryptKeys(CKeyingMaterial& vMasterKeyIn)
     return true;
 }
 
+bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, 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);
diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index cef153d2b..ac29fdc5e 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -148,8 +148,11 @@ public:
     virtual ~ScriptPubKeyMan() {};
     virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; }
 
-    //! Upgrade stored CKeyMetadata objects to store key origin info as KeyOriginInfo
-    virtual void UpgradeKeyMetadata() {}
+    virtual bool GetReservedDestination(const OutputType type, 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; }
@@ -247,7 +250,15 @@ public:
 
     //! will encrypt previously unencrypted keys
     bool EncryptKeys(CKeyingMaterial& vMasterKeyIn);
-    void UpgradeKeyMetadata() override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+
+    bool GetReservedDestination(const OutputType type, 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);
 
     bool IsHDEnabled() const override;
 
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 069ae5787..7a2ec1ef4 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -878,7 +878,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::St
                         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__);
                         }
                     }
@@ -3027,7 +3027,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;
 }
@@ -3047,7 +3047,7 @@ bool CWallet::GetNewChangeDestination(const OutputType type, CTxDestination& des
 {
     error.clear();
 
-    m_spk_man->TopUpKeyPool();
+    m_spk_man->TopUp();
 
     ReserveDestination reservedest(this);
     if (!reservedest.GetReservedDestination(type, dest, true)) {
@@ -3229,7 +3229,7 @@ bool ReserveDestination::GetReservedDestination(const OutputType type, CTxDestin
     if (nIndex == -1)
     {
         CKeyPool keypool;
-        if (!m_spk_man->ReserveKeyFromKeyPool(nIndex, keypool, internal)) {
+        if (!m_spk_man->GetReservedDestination(type, internal, nIndex, keypool)) {
             return false;
         }
         vchPubKey = keypool.vchPubKey;
@@ -3245,7 +3245,7 @@ bool ReserveDestination::GetReservedDestination(const OutputType type, CTxDestin
 void ReserveDestination::KeepDestination()
 {
     if (nIndex != -1)
-        m_spk_man->KeepKey(nIndex);
+        m_spk_man->KeepDestination(nIndex);
     nIndex = -1;
     vchPubKey = CPubKey();
     address = CNoDestination();
@@ -3254,7 +3254,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();
@@ -3623,7 +3623,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
         }
         // Regenerate the keypool if upgraded to HD
         if (hd_upgrade) {
-            if (!walletInstance->m_spk_man->TopUpKeyPool()) {
+            if (!walletInstance->m_spk_man->TopUp()) {
                 error = _("Unable to generate keys").translated;
                 return nullptr;
             }
@@ -3643,7 +3643,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
         }
 
         // Top up the keypool
-        if (walletInstance->m_spk_man->CanGenerateKeys() && !walletInstance->m_spk_man->TopUpKeyPool()) {
+        if (walletInstance->m_spk_man->CanGenerateKeys() && !walletInstance->m_spk_man->TopUp()) {
             error = _("Unable to generate initial keys").translated;
             return nullptr;
         }

From 769acef85730a6c0e2f5603fbd673b176c2d34d0 Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Mon, 7 Oct 2019 14:11:34 -0400
Subject: [PATCH 04/18] Refactor: Move SetAddressBook call out of
 LegacyScriptPubKeyMan::GetNewDestination

This commit does not change behavior.
---
 src/wallet/scriptpubkeyman.cpp | 5 +----
 src/wallet/scriptpubkeyman.h   | 3 ++-
 src/wallet/wallet.cpp          | 7 ++++++-
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 41ef12107..03a7b8f28 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -11,9 +11,8 @@
 #include <wallet/scriptpubkeyman.h>
 #include <wallet/wallet.h>
 
-bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, const std::string label, CTxDestination& dest, std::string& error)
+bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error)
 {
-    LOCK(cs_wallet);
     error.clear();
     TopUpKeyPool();
 
@@ -25,8 +24,6 @@ bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, const std::
     }
     LearnRelatedScripts(new_key, type);
     dest = GetDestinationForKey(new_key, type);
-
-    m_wallet.SetAddressBook(dest, label, "receive");
     return true;
 }
 
diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index ac29fdc5e..f7a3b81d3 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -146,6 +146,7 @@ protected:
 public:
     ScriptPubKeyMan(WalletStorage& storage) : m_storage(storage) {}
     virtual ~ScriptPubKeyMan() {};
+    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; }
@@ -245,7 +246,7 @@ private:
     void ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey);
 
 public:
-    bool GetNewDestination(const OutputType type, const std::string label, CTxDestination& dest, std::string& error);
+    bool GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error) override;
     isminetype IsMine(const CScript& script) const override;
 
     //! will encrypt previously unencrypted keys
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 7a2ec1ef4..2f1de5220 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3034,12 +3034,17 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize)
 
 bool CWallet::GetNewDestination(const OutputType type, 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(type, label, dest, error);
+        result = spk_man->GetNewDestination(type, dest, error);
     }
+    if (result) {
+        SetAddressBook(dest, label, "receive");
+    }
+
     return result;
 }
 

From 4c5491f99c6b8ca779c48be63eb2ba18d98ee52a Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Mon, 7 Oct 2019 14:11:34 -0400
Subject: [PATCH 05/18] 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 03a7b8f28..df03f5c88 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -320,8 +320,6 @@ void LegacyScriptPubKeyMan::UpgradeKeyMetadata()
             }
         }
     }
-    batch.reset(); //write before setting the flag
-    m_storage.SetWalletFlag(WALLET_FLAG_KEY_ORIGIN_METADATA);
 }
 
 bool LegacyScriptPubKeyMan::IsHDEnabled() const
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 2f1de5220..7af2de224 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -249,10 +249,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::Unlock(const SecureString& strWalletPassphrase, bool accept_no_keys)

From 0391aba52dcc33613295fd6099c804ac7134b063 Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Tue, 29 Oct 2019 17:49:39 -0400
Subject: [PATCH 06/18] Remove SetWalletFlag from WalletStorage

SetWalletFlag is unused.

Does not change any behavior
---
 src/wallet/scriptpubkeyman.h | 1 -
 src/wallet/wallet.h          | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index f7a3b81d3..0ef62e3f5 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -28,7 +28,6 @@ public:
     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 UnsetWalletFlagWithDB(WalletBatch&, uint64_t) = 0;
     virtual bool CanSupportFeature(enum WalletFeature) const = 0;
     virtual void SetMinVersion(enum WalletFeature, WalletBatch* = nullptr, bool = false) = 0;
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index f3b791441..cb4dd71b1 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -1090,7 +1090,7 @@ public:
     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 78e7cbc7ba5938ab2ae6347141ca793a8fc09853 Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Mon, 7 Oct 2019 14:11:34 -0400
Subject: [PATCH 07/18] Refactor: Remove UnsetWalletFlag call from
 LegacyScriptPubKeyMan::SetHDSeed

This commit does not change behavior.
---
 src/wallet/scriptpubkeyman.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index df03f5c88..3c57232d1 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -875,7 +875,8 @@ void LegacyScriptPubKeyMan::SetHDSeed(const CPubKey& seed)
     newHdChain.seed_id = seed.GetID();
     SetHDChain(newHdChain, false);
     NotifyCanGetAddressesChanged();
-    m_wallet.UnsetWalletFlag(WALLET_FLAG_BLANK_WALLET);
+    WalletBatch batch(m_storage.GetDatabase());
+    m_storage.UnsetWalletFlagWithDB(batch, WALLET_FLAG_BLANK_WALLET);
 }
 
 /**

From fc2867fdf50f47e5ad5f43445e500e3a477a8074 Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Tue, 29 Oct 2019 17:53:35 -0400
Subject: [PATCH 08/18] 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 | 8 ++++----
 src/wallet/scriptpubkeyman.h   | 2 +-
 src/wallet/wallet.cpp          | 5 +++++
 src/wallet/wallet.h            | 5 ++++-
 4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 3c57232d1..1d7483df1 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -440,7 +440,7 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyWithDB(WalletBatch& batch, const CKey& s
                                                  secret.GetPrivKey(),
                                                  mapKeyMetadata[pubkey.GetID()]);
     }
-    m_storage.UnsetWalletFlagWithDB(batch, WALLET_FLAG_BLANK_WALLET);
+    m_storage.UnsetBlankWalletFlag(batch);
     return true;
 }
 
@@ -597,7 +597,7 @@ bool LegacyScriptPubKeyMan::AddWatchOnlyWithDB(WalletBatch &batch, const CScript
     UpdateTimeFirstKey(meta.nCreateTime);
     NotifyWatchonlyChanged(true);
     if (batch.WriteWatchOnly(dest, meta)) {
-        m_storage.UnsetWalletFlagWithDB(batch, WALLET_FLAG_BLANK_WALLET);
+        m_storage.UnsetBlankWalletFlag(batch);
         return true;
     }
     return false;
@@ -876,7 +876,7 @@ void LegacyScriptPubKeyMan::SetHDSeed(const CPubKey& seed)
     SetHDChain(newHdChain, false);
     NotifyCanGetAddressesChanged();
     WalletBatch batch(m_storage.GetDatabase());
-    m_storage.UnsetWalletFlagWithDB(batch, WALLET_FLAG_BLANK_WALLET);
+    m_storage.UnsetBlankWalletFlag(batch);
 }
 
 /**
@@ -1155,7 +1155,7 @@ bool LegacyScriptPubKeyMan::AddCScriptWithDB(WalletBatch& batch, const CScript&
     if (!FillableSigningProvider::AddCScript(redeemScript))
         return false;
     if (batch.WriteCScript(Hash160(redeemScript), redeemScript)) {
-        m_storage.UnsetWalletFlagWithDB(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 0ef62e3f5..730229beb 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -28,7 +28,7 @@ public:
     virtual const std::string GetDisplayName() const = 0;
     virtual WalletDatabase& GetDatabase() = 0;
     virtual bool IsWalletFlagSet(uint64_t) const = 0;
-    virtual void UnsetWalletFlagWithDB(WalletBatch&, 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() const = 0;
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 7af2de224..88acfdf2a 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1309,6 +1309,11 @@ void CWallet::UnsetWalletFlagWithDB(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 cb4dd71b1..28cbdd66c 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -660,7 +660,10 @@ private:
     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) override;
+    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;

From 67be6b9e213da1fd7b2e8438c445c5a64092e831 Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Mon, 7 Oct 2019 14:11:34 -0400
Subject: [PATCH 09/18] 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 1d7483df1..b4abf0bad 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -1250,7 +1250,7 @@ bool LegacyScriptPubKeyMan::ImportPubKeys(const std::vector<CKeyID>& ordered_pub
     return true;
 }
 
-bool LegacyScriptPubKeyMan::ImportScriptPubKeys(const std::string& label, const std::set<CScript>& script_pub_keys, const bool have_solving_data, const bool apply_label, const int64_t timestamp)
+bool LegacyScriptPubKeyMan::ImportScriptPubKeys(const std::set<CScript>& script_pub_keys, const bool have_solving_data, const int64_t timestamp)
 {
     WalletBatch batch(m_storage.GetDatabase());
     for (const CScript& script : script_pub_keys) {
@@ -1259,11 +1259,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 730229beb..5bf58877e 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -323,7 +323,7 @@ public:
     bool ImportScripts(const std::set<CScript> scripts, int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     bool ImportPrivKeys(const std::map<CKeyID, CKey>& privkey_map, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     bool ImportPubKeys(const std::vector<CKeyID>& ordered_pubkeys, const std::map<CKeyID, CPubKey>& pubkey_map, const std::map<CKeyID, std::pair<CPubKey, KeyOriginInfo>>& 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<CScript>& 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<CScript>& 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 88acfdf2a..f2d6d5e60 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1410,9 +1410,19 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set<CScri
         return false;
     }
     AssertLockHeld(spk_man->cs_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 9716bbe0f8081814cbf052e8211d1c6a838e5262 Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Mon, 7 Oct 2019 14:11:34 -0400
Subject: [PATCH 10/18] 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 b4abf0bad..5641ef7b1 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -398,6 +398,11 @@ void LegacyScriptPubKeyMan::UpdateTimeFirstKey(int64_t nCreateTime)
     }
 }
 
+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 5bf58877e..dc3d3de51 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -276,7 +276,7 @@ public:
     //! 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<unsigned char> &vchCryptedSecret);
     //! Adds an encrypted key to the store, without saving it to disk (used by LoadWallet)

From a18edd7b383d667b15b6d4b87aa3a055a9fa5051 Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Mon, 7 Oct 2019 14:11:34 -0400
Subject: [PATCH 11/18] 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       | 34 ++++++++++++++++------------------
 src/wallet/scriptpubkeyman.cpp | 15 +++++++++++++++
 src/wallet/scriptpubkeyman.h   |  4 ++++
 src/wallet/wallet.h            |  2 --
 4 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index bfa4cf2bb..8d8e63066 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -3756,26 +3756,24 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
         ret.pushKV("label", pwallet->mapAddressBook[dest].name);
     }
     ret.pushKV("ischange", pwallet->IsChange(scriptPubKey));
-    const CKeyMetadata* meta = nullptr;
-    CKeyID key_id = GetKeyForDestination(*provider, dest);
-    if (!key_id.IsNull()) {
-        auto it = pwallet->mapKeyMetadata.find(key_id);
-        if (it != pwallet->mapKeyMetadata.end()) {
-            meta = &it->second;
+
+    ScriptPubKeyMan* spk_man = pwallet->GetScriptPubKeyMan();
+    if (spk_man) {
+        CKeyID key_id = GetKeyForDestination(*provider, dest);
+        const CKeyMetadata* meta = nullptr;
+        if (!key_id.IsNull()) {
+            meta = spk_man->GetMetadata(key_id);
         }
-    }
-    if (!meta) {
-        auto it = pwallet->m_script_metadata.find(CScriptID(scriptPubKey));
-        if (it != pwallet->m_script_metadata.end()) {
-            meta = &it->second;
+        if (!meta) {
+            meta = spk_man->GetMetadata(CScriptID(scriptPubKey));
         }
-    }
-    if (meta) {
-        ret.pushKV("timestamp", meta->nCreateTime);
-        if (meta->has_key_origin) {
-            ret.pushKV("hdkeypath", WriteHDKeypath(meta->key_origin.path));
-            ret.pushKV("hdseedid", meta->hd_seed_id.GetHex());
-            ret.pushKV("hdmasterfingerprint", HexStr(meta->key_origin.fingerprint, meta->key_origin.fingerprint + 4));
+        if (meta) {
+            ret.pushKV("timestamp", meta->nCreateTime);
+            if (meta->has_key_origin) {
+                ret.pushKV("hdkeypath", WriteHDKeypath(meta->key_origin.path));
+                ret.pushKV("hdseedid", meta->hd_seed_id.GetHex());
+                ret.pushKV("hdmasterfingerprint", HexStr(meta->key_origin.fingerprint, meta->key_origin.fingerprint + 4));
+            }
         }
     }
 
diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 5641ef7b1..8041a28d6 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -382,6 +382,21 @@ size_t LegacyScriptPubKeyMan::KeypoolCountExternalKeys()
     return setExternalKeyPool.size() + set_pre_split_keypool.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 dc3d3de51..e0dcae0d5 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -163,6 +163,8 @@ public:
     virtual int64_t GetOldestKeyPoolTime() { return GetTime(); }
 
     virtual size_t KeypoolCountExternalKeys() { return 0; }
+
+    virtual const CKeyMetadata* GetMetadata(uint160 id) const { return nullptr; }
 };
 
 class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProvider
@@ -265,6 +267,8 @@ public:
     int64_t GetOldestKeyPoolTime() override;
     size_t KeypoolCountExternalKeys() override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
+    const CKeyMetadata* GetMetadata(uint160 id) const override;
+
     bool CanGetAddresses(bool internal = false) override;
 
     // Map from Key ID to key metadata.
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 28cbdd66c..cfc3c6522 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -1134,8 +1134,6 @@ public:
     std::set<int64_t>& setInternalKeyPool GUARDED_BY(cs_wallet) = m_spk_man->setInternalKeyPool;
     std::set<int64_t>& setExternalKeyPool GUARDED_BY(cs_wallet) = m_spk_man->setExternalKeyPool;
     int64_t& nTimeFirstKey GUARDED_BY(cs_wallet) = m_spk_man->nTimeFirstKey;
-    std::map<CKeyID, CKeyMetadata>& mapKeyMetadata GUARDED_BY(cs_wallet) = m_spk_man->mapKeyMetadata;
-    std::map<CScriptID, CKeyMetadata>& m_script_metadata GUARDED_BY(cs_wallet) = m_spk_man->m_script_metadata;
     void MarkPreSplitKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(m_spk_man->cs_wallet); m_spk_man->MarkPreSplitKeys(); }
     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 46865ec958b6b9bde04a827de598975f14bdb5e7 Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Mon, 7 Oct 2019 14:11:34 -0400
Subject: [PATCH 12/18] Refactor: Move MarkUnusedAddresses code out of
 CWallet::AddToWalletIfInvolvingMe

This commit does not change behavior.
---
 src/wallet/scriptpubkeyman.cpp | 17 +++++++++++++++++
 src/wallet/scriptpubkeyman.h   |  7 +++++++
 src/wallet/wallet.cpp          | 15 ++-------------
 src/wallet/wallet.h            |  1 -
 4 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 8041a28d6..a866b5e4c 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -287,6 +287,23 @@ bool LegacyScriptPubKeyMan::TopUp(unsigned int size)
     return TopUpKeyPool(size);
 }
 
+void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
+{
+    AssertLockHeld(cs_wallet);
+    // extract addresses and check if they match with an unused keypool key
+    for (const auto& keyid : GetAffectedKeys(script, *this)) {
+        std::map<CKeyID, int64_t>::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__);
+            }
+        }
+    }
+}
+
 void LegacyScriptPubKeyMan::UpgradeKeyMetadata()
 {
     AssertLockHeld(cs_wallet);
diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index e0dcae0d5..be55d3cb1 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -37,6 +37,8 @@ public:
 //! Default for -keypool
 static const unsigned int DEFAULT_KEYPOOL_SIZE = 1000;
 
+std::vector<CKeyID> GetAffectedKeys(const CScript& spk, const SigningProvider& provider);
+
 /** A key from a CWallet's keypool
  *
  * The wallet holds one (for pre HD-split wallets) or several keypools. These
@@ -154,6 +156,9 @@ public:
 
     virtual bool TopUp(unsigned int size = 0) { return false; }
 
+    //! Mark unused addresses as being used
+    virtual void MarkUnusedAddresses(const CScript& script) {}
+
     /* Returns true if HD is enabled */
     virtual bool IsHDEnabled() const { return false; }
 
@@ -259,6 +264,8 @@ public:
 
     bool TopUp(unsigned int size = 0) override;
 
+    void MarkUnusedAddresses(const CScript& script) 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 f2d6d5e60..c35ba4fcd 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -236,8 +236,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<CKeyID> GetAffectedKeys(const CScript& spk, const SigningProvider& provider);
-
 const CWalletTx* CWallet::GetWalletTx(const uint256& hash) const
 {
     LOCK(cs_wallet);
@@ -876,17 +874,8 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::St
 
             // loop though all outputs
             for (const CTxOut& txout: tx.vout) {
-                // extract addresses and check if they match with an unused keypool key
-                for (const auto& keyid : GetAffectedKeys(txout.scriptPubKey, *m_spk_man)) {
-                    std::map<CKeyID, int64_t>::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 (auto spk_man = m_spk_man.get()) {
+                    spk_man->MarkUnusedAddresses(txout.scriptPubKey);
                 }
             }
 
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index cfc3c6522..f9a7bd1d1 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -1135,7 +1135,6 @@ public:
     std::set<int64_t>& setExternalKeyPool GUARDED_BY(cs_wallet) = m_spk_man->setExternalKeyPool;
     int64_t& nTimeFirstKey GUARDED_BY(cs_wallet) = m_spk_man->nTimeFirstKey;
     void MarkPreSplitKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(m_spk_man->cs_wallet); m_spk_man->MarkPreSplitKeys(); }
-    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 8b0d82bb428de9e7f1da7c61574e7a8376a62d43 Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Mon, 7 Oct 2019 14:11:34 -0400
Subject: [PATCH 13/18] Refactor: Move Upgrade code out of
 CWallet::CreateWalletFromFile

This commit does not change behavior.
---
 src/wallet/scriptpubkeyman.cpp | 35 ++++++++++++++++++++++++++++++++++
 src/wallet/scriptpubkeyman.h   |  5 +++++
 src/wallet/wallet.cpp          | 29 ++++------------------------
 src/wallet/wallet.h            |  1 -
 4 files changed, 44 insertions(+), 26 deletions(-)

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index a866b5e4c..9300630f6 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -361,6 +361,41 @@ bool LegacyScriptPubKeyMan::CanGetAddresses(bool internal)
     return keypool_has_keys;
 }
 
+bool LegacyScriptPubKeyMan::Upgrade(int prev_version, std::string& error)
+{
+    AssertLockHeld(cs_wallet);
+    error = "";
+    bool hd_upgrade = false;
+    bool split_upgrade = false;
+    if (m_storage.CanSupportFeature(FEATURE_HD) && !IsHDEnabled()) {
+        WalletLogPrintf("Upgrading wallet to HD\n");
+        m_storage.SetMinVersion(FEATURE_HD);
+
+        // generate a new master key
+        CPubKey masterPubKey = GenerateNewSeed();
+        SetHDSeed(masterPubKey);
+        hd_upgrade = true;
+    }
+    // Upgrade to HD chain split if necessary
+    if (m_storage.CanSupportFeature(FEATURE_HD_SPLIT)) {
+        WalletLogPrintf("Upgrading wallet to use HD chain split\n");
+        m_storage.SetMinVersion(FEATURE_PRE_SPLIT_KEYPOOL);
+        split_upgrade = FEATURE_HD_SPLIT > prev_version;
+    }
+    // Mark all keys currently in the keypool as pre-split
+    if (split_upgrade) {
+        MarkPreSplitKeys();
+    }
+    // Regenerate the keypool if upgraded to HD
+    if (hd_upgrade) {
+        if (!TopUpKeyPool()) {
+            error = _("Unable to generate keys").translated;
+            return false;
+        }
+    }
+    return true;
+}
+
 static int64_t GetOldestKeyTimeInPool(const std::set<int64_t>& setKeyPool, WalletBatch& batch) {
     if (setKeyPool.empty()) {
         return GetTime();
diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index be55d3cb1..e2a706e9a 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -165,6 +165,9 @@ public:
     /* 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; }
 
+    /** Upgrades the wallet to the specified version */
+    virtual bool Upgrade(int prev_version, std::string& error) { return false; }
+
     virtual int64_t GetOldestKeyPoolTime() { return GetTime(); }
 
     virtual size_t KeypoolCountExternalKeys() { return 0; }
@@ -271,6 +274,8 @@ public:
 
     bool IsHDEnabled() const override;
 
+    bool Upgrade(int prev_version, std::string& error) override;
+
     int64_t GetOldestKeyPoolTime() override;
     size_t KeypoolCountExternalKeys() override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index c35ba4fcd..2c4507a1b 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3614,31 +3614,10 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
             return nullptr;
         }
 
-        bool hd_upgrade = false;
-        bool split_upgrade = false;
-        if (walletInstance->CanSupportFeature(FEATURE_HD) && !walletInstance->m_spk_man->IsHDEnabled()) {
-            walletInstance->WalletLogPrintf("Upgrading wallet to HD\n");
-            walletInstance->SetMinVersion(FEATURE_HD);
-
-            // generate a new master key
-            CPubKey masterPubKey = walletInstance->m_spk_man->GenerateNewSeed();
-            walletInstance->m_spk_man->SetHDSeed(masterPubKey);
-            hd_upgrade = true;
-        }
-        // Upgrade to HD chain split if necessary
-        if (walletInstance->CanSupportFeature(FEATURE_HD_SPLIT)) {
-            walletInstance->WalletLogPrintf("Upgrading wallet to use HD chain split\n");
-            walletInstance->SetMinVersion(FEATURE_PRE_SPLIT_KEYPOOL);
-            split_upgrade = FEATURE_HD_SPLIT > prev_version;
-        }
-        // Mark all keys currently in the keypool as pre-split
-        if (split_upgrade) {
-            walletInstance->MarkPreSplitKeys();
-        }
-        // Regenerate the keypool if upgraded to HD
-        if (hd_upgrade) {
-            if (!walletInstance->m_spk_man->TopUp()) {
-                error = _("Unable to generate keys").translated;
+        if (auto spk_man = walletInstance->m_spk_man.get()) {
+            std::string error;
+            if (!spk_man->Upgrade(prev_version, error)) {
+                chain.initError(error);
                 return nullptr;
             }
         }
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index f9a7bd1d1..2a8039564 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -1134,7 +1134,6 @@ public:
     std::set<int64_t>& setInternalKeyPool GUARDED_BY(cs_wallet) = m_spk_man->setInternalKeyPool;
     std::set<int64_t>& setExternalKeyPool GUARDED_BY(cs_wallet) = m_spk_man->setExternalKeyPool;
     int64_t& nTimeFirstKey GUARDED_BY(cs_wallet) = m_spk_man->nTimeFirstKey;
-    void MarkPreSplitKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(m_spk_man->cs_wallet); m_spk_man->MarkPreSplitKeys(); }
     using CryptedKeyMap = LegacyScriptPubKeyMan::CryptedKeyMap;
 };
 

From f45d12b36cee05aa3c2685b951d27bd8a58539ae Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Mon, 7 Oct 2019 14:11:34 -0400
Subject: [PATCH 14/18] Refactor: Move HavePrivateKeys code out of
 CWallet::CreateWalletFromFile

This commit does not change behavior.
---
 src/wallet/scriptpubkeyman.cpp | 6 ++++++
 src/wallet/scriptpubkeyman.h   | 4 ++++
 src/wallet/wallet.cpp          | 7 ++++---
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 9300630f6..9d90edeea 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -396,6 +396,12 @@ bool LegacyScriptPubKeyMan::Upgrade(int prev_version, std::string& error)
     return true;
 }
 
+bool LegacyScriptPubKeyMan::HavePrivateKeys() const
+{
+    LOCK(cs_KeyStore);
+    return !mapKeys.empty() || !mapCryptedKeys.empty();
+}
+
 static int64_t GetOldestKeyTimeInPool(const std::set<int64_t>& setKeyPool, WalletBatch& batch) {
     if (setKeyPool.empty()) {
         return GetTime();
diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index e2a706e9a..927401be2 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -168,6 +168,8 @@ public:
     /** Upgrades the wallet to the specified version */
     virtual bool Upgrade(int prev_version, std::string& error) { return false; }
 
+    virtual bool HavePrivateKeys() const { return false; }
+
     virtual int64_t GetOldestKeyPoolTime() { return GetTime(); }
 
     virtual size_t KeypoolCountExternalKeys() { return 0; }
@@ -276,6 +278,8 @@ public:
 
     bool Upgrade(int prev_version, std::string& error) override;
 
+    bool HavePrivateKeys() const override;
+
     int64_t GetOldestKeyPoolTime() override;
     size_t KeypoolCountExternalKeys() override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 2c4507a1b..e6ef932ac 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3648,9 +3648,10 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
         error = strprintf(_("Error loading %s: Private keys can only be disabled during creation").translated, 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").translated, 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").translated, walletFile));
+            }
         }
     }
 

From 0eac7088ab878372a72f85f9c5f7662a14199083 Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Mon, 7 Oct 2019 14:11:34 -0400
Subject: [PATCH 15/18] Refactor: Move SetupGeneration code out of CWallet

This commit does not change behavior.
---
 src/wallet/scriptpubkeyman.cpp | 13 +++++++++++++
 src/wallet/scriptpubkeyman.h   |  8 ++++++++
 src/wallet/wallet.cpp          | 34 ++++++++++++++++++----------------
 3 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 9d90edeea..14e4163be 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -339,6 +339,19 @@ void LegacyScriptPubKeyMan::UpgradeKeyMetadata()
     }
 }
 
+bool LegacyScriptPubKeyMan::SetupGeneration(bool force)
+{
+    if ((CanGenerateKeys() && !force) || m_storage.IsLocked()) {
+        return false;
+    }
+
+    SetHDSeed(GenerateNewSeed());
+    if (!NewKeyPool()) {
+        return false;
+    }
+    return true;
+}
+
 bool LegacyScriptPubKeyMan::IsHDEnabled() const
 {
     return !hdChain.seed_id.IsNull();
diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index 927401be2..0e38cbf65 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -159,6 +159,12 @@ public:
     //! Mark unused addresses as being used
     virtual void MarkUnusedAddresses(const CScript& script) {}
 
+    /** Sets up the key generation stuff, i.e. generates new HD seeds and sets them as active.
+      * Returns false if already setup or setup fails, true if setup is successful
+      * Set force=true to make it re-setup if already setup, used for upgrades
+      */
+    virtual bool SetupGeneration(bool force = false) { return false; }
+
     /* Returns true if HD is enabled */
     virtual bool IsHDEnabled() const { return false; }
 
@@ -276,6 +282,8 @@ public:
 
     bool IsHDEnabled() const override;
 
+    bool SetupGeneration(bool force = false) override;
+
     bool Upgrade(int prev_version, std::string& error) override;
 
     bool HavePrivateKeys() const override;
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index e6ef932ac..1b27077d6 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -210,9 +210,14 @@ WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString&
             }
 
             // Set a seed for the wallet
-            CPubKey master_pub_key = wallet->m_spk_man->GenerateNewSeed();
-            wallet->m_spk_man->SetHDSeed(master_pub_key);
-            wallet->m_spk_man->NewKeyPool();
+            {
+                if (auto spk_man = wallet->m_spk_man.get()) {
+                    if (!spk_man->SetupGeneration()) {
+                        error = "Unable to generate initial keys";
+                        return WalletCreationStatus::CREATION_FAILED;
+                    }
+                }
+            }
 
             // Relock the wallet
             wallet->Lock();
@@ -565,11 +570,11 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
         Unlock(strWalletPassphrase);
 
         // if we are using HD, replace the HD seed with a new one
-        if (m_spk_man->IsHDEnabled()) {
-            m_spk_man->SetHDSeed(m_spk_man->GenerateNewSeed());
+        if (auto spk_man = m_spk_man.get()) {
+            if (spk_man->IsHDEnabled()) {
+                spk_man->SetupGeneration(true);
+            }
         }
-
-        m_spk_man->NewKeyPool();
         Lock();
 
         // Need to completely rewrite the wallet file; if we don't, bdb might keep
@@ -3630,15 +3635,12 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
 
         walletInstance->SetWalletFlags(wallet_creation_flags, false);
         if (!(wallet_creation_flags & (WALLET_FLAG_DISABLE_PRIVATE_KEYS | WALLET_FLAG_BLANK_WALLET))) {
-            // generate a new seed
-            CPubKey seed = walletInstance->m_spk_man->GenerateNewSeed();
-            walletInstance->m_spk_man->SetHDSeed(seed);
-        }
-
-        // Top up the keypool
-        if (walletInstance->m_spk_man->CanGenerateKeys() && !walletInstance->m_spk_man->TopUp()) {
-            error = _("Unable to generate initial keys").translated;
-            return nullptr;
+            if (auto spk_man = walletInstance->m_spk_man.get()) {
+                if (!spk_man->SetupGeneration()) {
+                    error = _("Unable to generate initial keys").translated;
+                    return nullptr;
+                }
+            }
         }
 
         auto locked_chain = chain.lock();

From 089e17d45c8147bd0303bcbf02dc0f7d6b387f2a Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Mon, 7 Oct 2019 14:11:34 -0400
Subject: [PATCH 16/18] 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          | 28 +++++++++-------------------
 3 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 14e4163be..8c3a2a831 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -415,6 +415,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 GetOldestKeyTimeInPool(const std::set<int64_t>& setKeyPool, WalletBatch& batch) {
     if (setKeyPool.empty()) {
         return GetTime();
diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index 0e38cbf65..24b0a2788 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -176,6 +176,9 @@ public:
 
     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; }
@@ -288,6 +291,8 @@ public:
 
     bool HavePrivateKeys() const override;
 
+    void RewriteDB() override;
+
     int64_t GetOldestKeyPoolTime() override;
     size_t KeypoolCountExternalKeys() override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 1b27077d6..575ae7e04 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2903,12 +2903,9 @@ DBErrors CWallet::LoadWallet(bool& fFirstRunRet)
     {
         if (database->Rewrite("\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();
+            }
         }
     }
 
@@ -2940,12 +2937,9 @@ DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256
     {
         if (database->Rewrite("\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();
+            }
         }
     }
 
@@ -2964,13 +2958,9 @@ DBErrors CWallet::ZapWalletTx(std::vector<CWalletTx>& vWtx)
     {
         if (database->Rewrite("\x04pool"))
         {
-            LOCK(cs_wallet);
-            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 7ef47b88e67718766c92d23973742d08436176e0 Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Mon, 7 Oct 2019 14:11:34 -0400
Subject: [PATCH 17/18] 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 8c3a2a831..a046a7c93 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -464,6 +464,12 @@ size_t LegacyScriptPubKeyMan::KeypoolCountExternalKeys()
     return setExternalKeyPool.size() + set_pre_split_keypool.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 24b0a2788..b9722ea63 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -182,6 +182,7 @@ public:
     virtual int64_t GetOldestKeyPoolTime() { return GetTime(); }
 
     virtual size_t KeypoolCountExternalKeys() { return 0; }
+    virtual unsigned int GetKeyPoolSize() const { return 0; }
 
     virtual const CKeyMetadata* GetMetadata(uint160 id) const { return nullptr; }
 };
@@ -295,6 +296,7 @@ public:
 
     int64_t GetOldestKeyPoolTime() override;
     size_t KeypoolCountExternalKeys() override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    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 575ae7e04..3dff6f7d6 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3027,6 +3027,17 @@ size_t CWallet::KeypoolCountExternalKeys()
     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 2a8039564..49c27eb9f 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -992,11 +992,7 @@ public:
 
     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;
@@ -1131,8 +1127,6 @@ public:
     LegacyScriptPubKeyMan::WatchOnlySet& setWatchOnly GUARDED_BY(cs_KeyStore) = m_spk_man->setWatchOnly;
     LegacyScriptPubKeyMan::WatchKeyMap& mapWatchKeys GUARDED_BY(cs_KeyStore) = m_spk_man->mapWatchKeys;
     WalletBatch*& encrypted_batch GUARDED_BY(cs_wallet) = m_spk_man->encrypted_batch;
-    std::set<int64_t>& setInternalKeyPool GUARDED_BY(cs_wallet) = m_spk_man->setInternalKeyPool;
-    std::set<int64_t>& setExternalKeyPool GUARDED_BY(cs_wallet) = m_spk_man->setExternalKeyPool;
     int64_t& nTimeFirstKey GUARDED_BY(cs_wallet) = m_spk_man->nTimeFirstKey;
     using CryptedKeyMap = LegacyScriptPubKeyMan::CryptedKeyMap;
 };

From 152b0a00d8e681dd098f6b548447b82ab54ebe3c Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Mon, 7 Oct 2019 14:11:34 -0400
Subject: [PATCH 18/18] Refactor: Move nTimeFirstKey accesses out of CWallet

This commit does not change behavior.
---
 src/wallet/scriptpubkeyman.cpp | 6 ++++++
 src/wallet/scriptpubkeyman.h   | 4 ++++
 src/wallet/wallet.cpp          | 9 +++++++--
 src/wallet/wallet.h            | 1 -
 4 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index a046a7c93..bb13db11b 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -470,6 +470,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);
diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index b9722ea63..0dbf98ee9 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -184,6 +184,8 @@ public:
     virtual size_t KeypoolCountExternalKeys() { 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; }
 };
 
@@ -298,6 +300,8 @@ public:
     size_t KeypoolCountExternalKeys() override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     unsigned int GetKeyPoolSize() const override;
 
+    int64_t GetTimeFirstKey() const override;
+
     const CKeyMetadata* GetMetadata(uint160 id) const override;
 
     bool CanGetAddresses(bool internal = false) override;
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 3dff6f7d6..b10a5deed 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3804,8 +3804,13 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
 
         // No need to read and scan block if block was created before
         // our wallet birthday (as adjusted for block time variability)
-        if (walletInstance->nTimeFirstKey) {
-            if (Optional<int> first_block = locked_chain->findFirstBlockWithTimeAndHeight(walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW, rescan_height, nullptr)) {
+        Optional<int64_t> 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<int> first_block = locked_chain->findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, nullptr)) {
                 rescan_height = *first_block;
             }
         }
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 49c27eb9f..7d0fae0bc 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -1127,7 +1127,6 @@ public:
     LegacyScriptPubKeyMan::WatchOnlySet& setWatchOnly GUARDED_BY(cs_KeyStore) = m_spk_man->setWatchOnly;
     LegacyScriptPubKeyMan::WatchKeyMap& mapWatchKeys GUARDED_BY(cs_KeyStore) = m_spk_man->mapWatchKeys;
     WalletBatch*& encrypted_batch GUARDED_BY(cs_wallet) = m_spk_man->encrypted_batch;
-    int64_t& nTimeFirstKey GUARDED_BY(cs_wallet) = m_spk_man->nTimeFirstKey;
     using CryptedKeyMap = LegacyScriptPubKeyMan::CryptedKeyMap;
 };