Merge #17681: wallet: Keep inactive seeds after sethdseed and derive keys from them as needed

1ed52fb Remove IBD check in sethdseed (Andrew Chow)
b1810a1 Test that keys from inactive seeds are generated (Andrew Chow)
c93082e Generate new keys for inactive seeds after marking used (Andrew Chow)
45f2f6a Determine inactive HD seeds from key metadata and track them in LegacyScriptPubKeyMan (Andrew Chow)
b59b450 have GenerateNewKey and DeriveNewChildKey take a CHDChain as an argument (Andrew Chow)

Pull request description:

  Largely implements the suggestion from bitcoin/bitcoin#17484 (comment).

  After `sethdseed` is called, the CHDChain for the old seed is kept in the wallet. It is kept on the file as a new `inactivehdseed` record and in memory in a map `m_inactive_hd_seeds`. In `LegacyScriptPubKeyMan::MarkUnusedAddresses` we check each used key's metadata for whether it was derived from an inactive seed. If it is, we then check to see how many keys after that key were derived from the inactive seed. If that number does not match the keypool parameter, we derive more keys from the inactive seed until it does match. This way we won't miss transactions belonging to keys outside of the range of the keypool initially.

  The indexes and internal-ness of a key is gotten by checking it's key origin data.

  Because of this change, we no longer need to wait for IBD to finish before `sethdseed` can work so that check is also removed.

  A test case for this is added as well which fails on master.

ACKs for top commit:
  ryanofsky:
    Code review ACK 1ed52fb. Changes since last review: various commit message, code comment, log message, error checking improvements, and fix for topping up inactive seeds if wallet isn't reloaded after calling sethdseed and test for this
  ariard:
    Code Review ACK 1ed52fb
  jonatack:
    ACK 1ed52fb thanks for addressing the previous review feedback; would be happy to see the new review questions answered and feedback addressed and re-ack.

Tree-SHA512: e658ae0e1dab94be55d2b62cdda506c94815e73a6881533fd30d41cc77477f82fee2095144957a3a1df0c129e256bdd7b7abe3737d515f393610446cae4edf1c

Author: meshcollider

src/wallet/rpcwallet.cpp

@@ -3979,10 +3979,6 @@ UniValue sethdseed(const JSONRPCRequest& request)
 
     LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*pwallet, true);
 
-    if (pwallet->chain().isInitialBlockDownload()) {
-        throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, "Cannot set a new HD seed while still in Initial Block Download");
-    }
-
     if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
         throw JSONRPCError(RPC_WALLET_ERROR, "Cannot set a HD seed to a wallet with private keys disabled");
     }

src/wallet/scriptpubkeyman.cpp

@@ -12,6 +12,9 @@
 #include <util/translation.h>
 #include <wallet/scriptpubkeyman.h>
 
+//! Value for the first BIP 32 hardened derivation. Can be used as a bit mask and as a value. See BIP 32 for more details.
+const uint32_t BIP32_HARDENED_KEY_LIMIT = 0x80000000;
+
 bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error)
 {
     LOCK(cs_KeyStore);
@@ -295,20 +298,72 @@ bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool i
     return true;
 }
 
+bool LegacyScriptPubKeyMan::TopUpInactiveHDChain(const CKeyID seed_id, int64_t index, bool internal)
+{
+    LOCK(cs_KeyStore);
+
+    if (m_storage.IsLocked()) return false;
+
+    auto it = m_inactive_hd_chains.find(seed_id);
+    if (it == m_inactive_hd_chains.end()) {
+        return false;
+    }
+
+    CHDChain& chain = it->second;
+
+    // Top up key pool
+    int64_t target_size = std::max(gArgs.GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 1);
+
+    // "size" of the keypools. Not really the size, actually the difference between index and the chain counter
+    // Since chain counter is 1 based and index is 0 based, one of them needs to be offset by 1.
+    int64_t kp_size = (internal ? chain.nInternalChainCounter : chain.nExternalChainCounter) - (index + 1);
+
+    // make sure the keypool fits the user-selected target (-keypool)
+    int64_t missing = std::max(target_size - kp_size, (int64_t) 0);
+
+    if (missing > 0) {
+        WalletBatch batch(m_storage.GetDatabase());
+        for (int64_t i = missing; i > 0; --i) {
+            GenerateNewKey(batch, chain, internal);
+        }
+        if (internal) {
+            WalletLogPrintf("inactive seed with id %s added %d internal keys\n", HexStr(seed_id), missing);
+        } else {
+            WalletLogPrintf("inactive seed with id %s added %d keys\n", HexStr(seed_id), missing);
+        }
+    }
+    return true;
+}
+
 void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
 {
     LOCK(cs_KeyStore);
     // 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__);
+            WalletLogPrintf("%s: Detected a used keypool key, mark all keypool keys up to this key as used\n", __func__);
             MarkReserveKeysAsUsed(mi->second);
 
             if (!TopUp()) {
                 WalletLogPrintf("%s: Topping up keypool failed (locked wallet)\n", __func__);
             }
         }
+
+        // Find the key's metadata and check if it's seed id (if it has one) is inactive, i.e. it is not the current m_hd_chain seed id.
+        // If so, TopUp the inactive hd chain
+        auto it = mapKeyMetadata.find(keyid);
+        if (it != mapKeyMetadata.end()){
+            CKeyMetadata meta = it->second;
+            if (!meta.hd_seed_id.IsNull() && meta.hd_seed_id != m_hd_chain.seed_id) {
+                bool internal = (meta.key_origin.path[1] & ~BIP32_HARDENED_KEY_LIMIT) != 0;
+                int64_t index = meta.key_origin.path[2] & ~BIP32_HARDENED_KEY_LIMIT;
+
+                if (!TopUpInactiveHDChain(meta.hd_seed_id, index, internal)) {
+                    WalletLogPrintf("%s: Adding inactive seed keys failed\n", __func__);
+                }
+            }
+        }
     }
 }
 
@@ -362,7 +417,7 @@ bool LegacyScriptPubKeyMan::SetupGeneration(bool force)
 
 bool LegacyScriptPubKeyMan::IsHDEnabled() const
 {
-    return !hdChain.seed_id.IsNull();
+    return !m_hd_chain.seed_id.IsNull();
 }
 
 bool LegacyScriptPubKeyMan::CanGetAddresses(bool internal) const
@@ -848,10 +903,27 @@ bool LegacyScriptPubKeyMan::AddWatchOnly(const CScript& dest, int64_t nCreateTim
 void LegacyScriptPubKeyMan::SetHDChain(const CHDChain& chain, bool memonly)
 {
     LOCK(cs_KeyStore);
-    if (!memonly && !WalletBatch(m_storage.GetDatabase()).WriteHDChain(chain))
-        throw std::runtime_error(std::string(__func__) + ": writing chain failed");
+    // memonly == true means we are loading the wallet file
+    // memonly == false means that the chain is actually being changed
+    if (!memonly) {
+        // Store the new chain
+        if (!WalletBatch(m_storage.GetDatabase()).WriteHDChain(chain)) {
+            throw std::runtime_error(std::string(__func__) + ": writing chain failed");
+        }
+        // When there's an old chain, add it as an inactive chain as we are now rotating hd chains
+        if (!m_hd_chain.seed_id.IsNull()) {
+            AddInactiveHDChain(m_hd_chain);
+        }
+    }
 
-    hdChain = chain;
+    m_hd_chain = chain;
+}
+
+void LegacyScriptPubKeyMan::AddInactiveHDChain(const CHDChain& chain)
+{
+    LOCK(cs_KeyStore);
+    assert(!chain.seed_id.IsNull());
+    m_inactive_hd_chains[chain.seed_id] = chain;
 }
 
 bool LegacyScriptPubKeyMan::HaveKey(const CKeyID &address) const
@@ -930,7 +1002,7 @@ bool LegacyScriptPubKeyMan::GetPubKey(const CKeyID &address, CPubKey& vchPubKeyO
     return GetWatchPubKey(address, vchPubKeyOut);
 }
 
-CPubKey LegacyScriptPubKeyMan::GenerateNewKey(WalletBatch &batch, bool internal)
+CPubKey LegacyScriptPubKeyMan::GenerateNewKey(WalletBatch &batch, CHDChain& hd_chain, bool internal)
 {
     assert(!m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));
     assert(!m_storage.IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET));
@@ -945,7 +1017,7 @@ CPubKey LegacyScriptPubKeyMan::GenerateNewKey(WalletBatch &batch, bool internal)
 
     // use HD key derivation if HD was enabled during wallet creation and a seed is present
     if (IsHDEnabled()) {
-        DeriveNewChildKey(batch, metadata, secret, (m_storage.CanSupportFeature(FEATURE_HD_SPLIT) ? internal : false));
+        DeriveNewChildKey(batch, metadata, secret, hd_chain, (m_storage.CanSupportFeature(FEATURE_HD_SPLIT) ? internal : false));
     } else {
         secret.MakeNewKey(fCompressed);
     }
@@ -967,9 +1039,7 @@ CPubKey LegacyScriptPubKeyMan::GenerateNewKey(WalletBatch &batch, bool internal)
     return pubkey;
 }
 
-const uint32_t BIP32_HARDENED_KEY_LIMIT = 0x80000000;
-
-void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata& metadata, CKey& secret, bool internal)
+void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata& metadata, CKey& secret, CHDChain& hd_chain, bool internal)
 {
     // for now we use a fixed keypath scheme of m/0'/0'/k
     CKey seed;                     //seed (256bit)
@@ -979,7 +1049,7 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata&
     CExtKey childKey;              //key at m/0'/0'/<n>'
 
     // try to get the seed
-    if (!GetKey(hdChain.seed_id, seed))
+    if (!GetKey(hd_chain.seed_id, seed))
         throw std::runtime_error(std::string(__func__) + ": seed not found");
 
     masterKey.SetSeed(seed.begin(), seed.size());
@@ -998,30 +1068,30 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata&
         // childIndex | BIP32_HARDENED_KEY_LIMIT = derive childIndex in hardened child-index-range
         // example: 1 | BIP32_HARDENED_KEY_LIMIT == 0x80000001 == 2147483649
         if (internal) {
-            chainChildKey.Derive(childKey, hdChain.nInternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
-            metadata.hdKeypath = "m/0'/1'/" + ToString(hdChain.nInternalChainCounter) + "'";
+            chainChildKey.Derive(childKey, hd_chain.nInternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
+            metadata.hdKeypath = "m/0'/1'/" + ToString(hd_chain.nInternalChainCounter) + "'";
             metadata.key_origin.path.push_back(0 | BIP32_HARDENED_KEY_LIMIT);
             metadata.key_origin.path.push_back(1 | BIP32_HARDENED_KEY_LIMIT);
-            metadata.key_origin.path.push_back(hdChain.nInternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
-            hdChain.nInternalChainCounter++;
+            metadata.key_origin.path.push_back(hd_chain.nInternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
+            hd_chain.nInternalChainCounter++;
         }
         else {
-            chainChildKey.Derive(childKey, hdChain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
-            metadata.hdKeypath = "m/0'/0'/" + ToString(hdChain.nExternalChainCounter) + "'";
+            chainChildKey.Derive(childKey, hd_chain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
+            metadata.hdKeypath = "m/0'/0'/" + ToString(hd_chain.nExternalChainCounter) + "'";
             metadata.key_origin.path.push_back(0 | BIP32_HARDENED_KEY_LIMIT);
             metadata.key_origin.path.push_back(0 | BIP32_HARDENED_KEY_LIMIT);
-            metadata.key_origin.path.push_back(hdChain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
-            hdChain.nExternalChainCounter++;
+            metadata.key_origin.path.push_back(hd_chain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
+            hd_chain.nExternalChainCounter++;
         }
     } while (HaveKey(childKey.key.GetPubKey().GetID()));
     secret = childKey.key;
-    metadata.hd_seed_id = hdChain.seed_id;
+    metadata.hd_seed_id = hd_chain.seed_id;
     CKeyID master_id = masterKey.key.GetPubKey().GetID();
     std::copy(master_id.begin(), master_id.begin() + 4, metadata.key_origin.fingerprint);
     metadata.has_key_origin = true;
     // update the chain model in the database
-    if (!batch.WriteHDChain(hdChain))
-        throw std::runtime_error(std::string(__func__) + ": Writing HD chain model failed");
+    if (hd_chain.seed_id == m_hd_chain.seed_id && !batch.WriteHDChain(hd_chain))
+        throw std::runtime_error(std::string(__func__) + ": writing HD chain model failed");
 }
 
 void LegacyScriptPubKeyMan::LoadKeyPool(int64_t nIndex, const CKeyPool &keypool)
@@ -1176,7 +1246,7 @@ bool LegacyScriptPubKeyMan::TopUp(unsigned int kpSize)
                 internal = true;
             }
 
-            CPubKey pubkey(GenerateNewKey(batch, internal));
+            CPubKey pubkey(GenerateNewKey(batch, m_hd_chain, internal));
             AddKeypoolPubkeyWithDB(pubkey, internal, batch);
         }
         if (missingInternal + missingExternal > 0) {
@@ -1249,7 +1319,7 @@ bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, const OutputType typ
         if (!ReserveKeyFromKeyPool(nIndex, keypool, internal) && !m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
             if (m_storage.IsLocked()) return false;
             WalletBatch batch(m_storage.GetDatabase());
-            result = GenerateNewKey(batch, internal);
+            result = GenerateNewKey(batch, m_hd_chain, internal);
             return true;
         }
         KeepDestination(nIndex, type);

src/wallet/scriptpubkeyman.h

@@ -18,6 +18,8 @@
 
 #include <boost/signals2/signal.hpp>
 
+#include <unordered_map>
+
 enum class OutputType;
 struct bilingual_str;
 
@@ -144,6 +146,17 @@ class CKeyPool
     }
 };
 
+class KeyIDHasher
+{
+public:
+    KeyIDHasher() {}
+
+    size_t operator()(const CKeyID& id) const
+    {
+        return id.GetUint64(0);
+    }
+};
+
 /*
  * A class implementing ScriptPubKeyMan manages some (or all) scriptPubKeys used in a wallet.
  * It contains the scripts and keys related to the scriptPubKeys it manages.
@@ -288,10 +301,11 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
     bool AddKeyOriginWithDB(WalletBatch& batch, const CPubKey& pubkey, const KeyOriginInfo& info);
 
     /* the HD chain data model (external chain counters) */
-    CHDChain hdChain;
+    CHDChain m_hd_chain;
+    std::unordered_map<CKeyID, CHDChain, KeyIDHasher> m_inactive_hd_chains;
 
     /* 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_KeyStore);
+    void DeriveNewChildKey(WalletBatch& batch, CKeyMetadata& metadata, CKey& secret, CHDChain& hd_chain, bool internal = false) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore);
 
     std::set<int64_t> setInternalKeyPool GUARDED_BY(cs_KeyStore);
     std::set<int64_t> setExternalKeyPool GUARDED_BY(cs_KeyStore);
@@ -320,6 +334,18 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
      */
     bool ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRequestedInternal);
 
+    /**
+     * Like TopUp() but adds keys for inactive HD chains.
+     * Ensures that there are at least -keypool number of keys derived after the given index.
+     *
+     * @param seed_id the CKeyID for the HD seed.
+     * @param index the index to start generating keys from
+     * @param internal whether the internal chain should be used. true for internal chain, false for external chain.
+     *
+     * @return true if seed was found and keys were derived. false if unable to derive seeds
+     */
+    bool TopUpInactiveHDChain(const CKeyID seed_id, int64_t index, bool internal);
+
 public:
     using ScriptPubKeyMan::ScriptPubKeyMan;
 
@@ -393,11 +419,12 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
     void LoadKeyMetadata(const CKeyID& keyID, const CKeyMetadata &metadata);
     void LoadScriptMetadata(const CScriptID& script_id, const CKeyMetadata &metadata);
     //! Generate a new key
-    CPubKey GenerateNewKey(WalletBatch& batch, bool internal = false) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore);
+    CPubKey GenerateNewKey(WalletBatch& batch, CHDChain& hd_chain, bool internal = false) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore);
 
     /* Set the HD chain model (chain child index counters) */
     void SetHDChain(const CHDChain& chain, bool memonly);
-    const CHDChain& GetHDChain() const { return hdChain; }
+    const CHDChain& GetHDChain() const { return m_hd_chain; }
+    void AddInactiveHDChain(const CHDChain& chain);
 
     //! Adds a watch-only address to the store, without saving it to disk (used by LoadWallet)
     bool LoadWatchOnly(const CScript &dest);