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

achow101 · 2019-12-05T21:54:08Z

Largely implements the suggestion from #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.

DrahtBot · 2019-12-05T22:21:41Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off #18923 (wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off by MarcoFalke)
Use shared pointers only in validation interface #18354 (Protect wallet by using shared pointers by bvbfan)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

src/wallet/scriptpubkeyman.cpp

achow101 · 2019-12-08T18:02:07Z

Found a few bugs where the wrong CHDChain was being used/updated/written. Should be fixed now.

ariard

Concept ACK,

I think new logic is robust enough, even if we set new seed during IBD, caching the inactive seeds and keep generating from them to keep a constant-size keypool. It should work also in case of loading a old backup where we may have handouts new keys since backup assuming we rescan the missing blocks.

ariard · 2019-12-09T22:17:38Z

src/wallet/scriptpubkeyman.cpp


    hdChain = chain;
 }

+void LegacyScriptPubKeyMan::AddInactiveHDChain(const CHDChain& chain, bool memonly)
+{
+    LOCK(cs_wallet);


Is needed to take lock again? Can't you AssertLockHeld and EXCLUSIVE_LOCKREQUIRED instead?

src/wallet/scriptpubkeyman.cpp

ariard · 2019-12-10T02:31:52Z

@achow101 after thoughts, I was thinking of the case where we have initial keypool=1000, we create a wallet backup B1 at block N, then we exhaust keypool until reaching index 1100, none of the address in the range 0 to 1000 are confirmed on chain, address index 1100 get confirmed at block N+10000. We restore wallet backup B1 and rescan from N until N+10000, wallet is not going to see address index 1100 because we only advance look-ahead key buffer when we detect an address in range 0 to 1000.

This scenario is really unlikely but it this current code behavior ? Just to be sure, if yes maybe comment of CKeyPool should be updated to avoid people setting to small -keypool.

achow101 · 2019-12-10T02:49:44Z

Yes, such a scenario is possible and is unavoidable. The only thing that you can do is to have a large enough keypool (or gap limit as other wallets call it) where this is unlikely to happen. This is why the default -keypool was raised to 1000 from 100.

ariard · 2019-12-10T23:24:58Z

Okay thanks for answer, added a commit on top of your branch (ariard@3c25ab7) to clarify the risk of losing funds by lowering the default keypool value. With the default value, this scenario is really unlikely, but we should inform as best user to avoid one of them removing the footgun protection by mistake.

We could also go further and return error at wallet init if -keypool < 100.

achow101 · 2019-12-11T01:48:08Z

I think updating the docs about -keypool settings is out of scope for this PR.

ariard · 2019-12-11T03:49:46Z

No worries I'll take it on its own. But on the raw idea, do you think it's pertinent to update the doc to inform user ?

achow101 · 2019-12-11T03:54:30Z

Yes, we should keep docs up to date with behavior.

promag

Concept ACK.

src/wallet/scriptpubkeyman.cpp

luke-jr · 2020-01-03T18:54:01Z

Concept ACK

promag

nit in commit fe7e9ae, could rename LegacyScriptPubKeyMan::hdChain to m_hd_chain so it's clear there isn't invalid usage of hdChain.

achow101 · 2020-01-06T17:03:06Z

Renamed it.

jonasschnelli · 2020-01-09T05:26:43Z

Nice work!
Concept ACK.

f41d589 Document better -keypool as a look-ahead safety mechanism (Antoine Riard) Pull request description: If after a backup, an address is issued beyond the initial keypool range and none of the addresses in this range is seen onchain, if a wallet is restored from backup, even in case of rescan, funds may be loss due to the look-ahead buffer not being incremented and so restored wallet not detecting onchain out-of-range address as derived from its seed. This scenario is theoretically unavoidable due to the requirement of the keypool to have a max size. However, given the default keypool size, this is unlikely. Document better keypool size implications to avoid user setting a too low value. While reviewing #17681, it took me a while to figure out the safety implications of keypool, I find it would be better to document this a bit farther to avoid users shooting themselves in the foot. For further context & discussion, see #17681 (comment) ACKs for top commit: ryanofsky: Code review ACK f41d589. Just "Warning:" prefix added since the last review jonatack: ACK f41d589 code review and build/test. The added `Warning:` since last review is a good addition. Tree-SHA512: d3d0ee88fcdfc5c8841a2bd4bada0e4eeb412a0dce5054e5fb023643c2fa57206a0f3efb06890c245528dc4431413ed2fd5645b9319d26245d044c490b7f0db0

jnewbery · 2020-01-30T15:54:51Z

Concept ACK

jonatack

ACK 218c4f6

This PR has improved since the last review. A few non-blocking comments follow; feel free to ignore.

src/wallet/scriptpubkeyman.cpp

src/wallet/scriptpubkeyman.h

src/wallet/scriptpubkeyman.cpp

jonatack · 2020-05-12T19:46:02Z

src/wallet/walletdb.cpp

+                        return false;
+                    }
+                    internal = path[1] == (1 | 0x80000000);
+                    index = path[2] & ~0x80000000;


f5eaa90 now that BIP32_HARDENED_KEY_LIMIT has been added, perhaps replace the 6 instances of 0x80000000 here with it (can be done as a follow-up, as there may be others in the codebase that can use this constant)

I tried but ran into far too many linker errors than I cared to try to fix. The logical places to define this constant aren't necessarily included by or linked with the places that actually use it.

fjahr

~~Code review ACK 218c4f6~~

EDIT: need to withdraw because of the potential bug discovered by @ryanofsky in the or review club.

src/wallet/scriptpubkeyman.cpp

theStack · 2020-05-13T16:27:08Z

Concept ACK

jkczyz

Concept ACK

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

Could you specify why in the relevant commit message?

src/wallet/walletdb.cpp

src/wallet/scriptpubkeyman.cpp

rajarshimaitra · 2020-05-15T14:56:35Z

Code Review ACK. Verified test, verified test failing in master. Overall nice easy to understand PR. Agreed with the motivation.

src/wallet/scriptpubkeyman.cpp

…yScriptPubKeyMan

When a key from an inactive seed is used, generate replacements to fill a keypool that would have been there.

It is no longer necessary to wait for IBD to be complete before setting a HD seed. This check was originally to ensure that restoring an old seed on an out of sync node would scan the entire blockchain and thus not miss transactions that involved keys that were not in the keypool. This was necessary as once the seed was changed, no further keys would be derived from the old seed(s). As we are now topping up inactive seeds as we find those keys to be used, this check is no longer necessary. During IBD, each time we find a used key belonging to an inactive hd seed, we will still generate more keys from that inactive seed.

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

jnewbery · 2020-05-19T21:08:31Z

src/wallet/walletdb.h

@@ -116,6 +116,11 @@ class CHDChain
        nInternalChainCounter = 0;
        seed_id.SetNull();
    }
+
+    bool operator==(const CHDChain& chain) const


This doesn't seem to be used (I can comment it out and the branch still compiles).

You're comparing equality of seed_ids directly here: https://github.com/bitcoin/bitcoin/pull/17681/files#diff-5462ceb8a760a507152ab8b76bd48774R1084. I suggest you either change that to compare equality of the CHDChain objects, or remove this operator overload.

jnewbery · 2020-05-19T22:10:54Z

src/wallet/scriptpubkeyman.cpp

        }
    } 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


nit: this could be updated to say "If this is the active HDChain, then update the chain model in the database"

jnewbery · 2020-05-19T22:19:11Z

src/wallet/scriptpubkeyman.cpp

@@ -839,10 +894,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


This is really crying out to be cleaned up:

there's a bool flag that substantially changes the logic of this function

the function is called in two places only, once with true and once with false

the argument name is confusing enough that it requires an inline comment that explains where the function is being called from.

It seems to me that this function should be split into a WriteHDChain() and SetHDChain() functions.

re: #17681 (comment)

This is really crying out to be cleaned up:

Agree with this, but my immediate reaction here (without looking too deeply) is that the changes suggested here are only tangentially related to this PR and should be made in a separate PR before or after this one. There's already a lot going on here and it seems like you could make similar comments about a lot of the other wallet functions this PR touches.

It seems to me that this function should be split into a WriteHDChain() and SetHDChain() functions.

Again didn't look too closely, but a current convention is to have CWallet and KeyMan LoadXXX and AddXXX methods

the changes suggested here are only tangentially related to this PR

Prior to this PR, the memonly flag only controlled whether the new seed was written to the database. This PR changes things so the majority of the function is in an if block that depends on the flag, so I'd argue it's not tangential.

I think it's fine to do this in a follow-up, but if this PR gets retouched, no problem doing it here either.

I agree that this should be cleaned up. There are a few other functions that follow this same pattern, so I think it would be better to change them all at the same time in a followup PR.

jnewbery · 2020-05-19T22:22:44Z

src/wallet/scriptpubkeyman.cpp

            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.


grammar nit: s/it's/its/ :slightly_smiling_face:

jnewbery · 2020-05-19T22:39:46Z

src/wallet/scriptpubkeyman.cpp

@@ -290,20 +293,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);


This is only called from within MarkUnusedAddresses(), which has already taken this lock. Rather than taking the lock recursively, I think it makes more sense to add an EXCLUSIVE_LOCKS_REQUIRED annotation.

jnewbery · 2020-05-19T22:56:51Z

src/wallet/scriptpubkeyman.h

+class KeyIDHasher
+{
+public:
+    KeyIDHasher() {}
+
+    size_t operator()(const CKeyID& id) const
+    {
+        return id.GetUint64(0);
+    }
+};
+


A few suggestions:

drop the default constructor. The implicitly-defined default ctor will do exactly the same (ie nothing since this struct has no data members).

make it a struct so you can drop the public: access specifier

put the operator() definition on one line:

Suggested change

class KeyIDHasher

{

public:

KeyIDHasher() {}

size_t operator()(const CKeyID& id) const

{

return id.GetUint64(0);

}

};

struct KeyIDHasher

{

size_t operator()(const CKeyID& id) const { return id.GetUint64(0);}

};

ariard

Code Review ACK 1ed52fb

I think comments could be made better but not going to hold on this.

ariard · 2020-05-19T23:10:38Z

src/wallet/scriptpubkeyman.cpp

-    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


45f2f6a

Instead of commenting parameters inside function it could be on its top comment and name changed to loading or rotating. IMO function is a bit blurred it sets a chain but also writes it down to DB, and now does also rotation.

see #17681 (comment)

ariard · 2020-05-20T00:06:31Z

src/wallet/walletdb.cpp

+                    // Extract the index and internal from the path
+                    // Path string is m/0'/k'/i'
+                    // Path vector is [0', k', i'] (but as ints OR'd with the hardened bit
+                    // k == 0 for external, 1 for internal. i is the index


45f2f6a

Why internal is OR'd with the hardened bit ? Isn't the hardening implied by 0x80000000 ?

The meaning is supposed to be that the path vector contains ints that correspond to 0, k, and i with those ints being OR'd with the hardened bitmask. I guess it's not very clear and that sentence could probably be dropped.

ariard · 2020-05-20T00:56:39Z

src/wallet/scriptpubkeyman.cpp

+    CHDChain& chain = it->second;
+
+    // Top up key pool
+    int64_t target_size = std::max(gArgs.GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 1);


c93082e

I think this is right but code could be made more intuitive IMO with better naming.

AFAICT target_size is the size of keypool we want to keep constant. To do so you need to add new keys it after detecting onchain a key to which index is passed. We compute the "detected_diff" or "consumed_diff" based on chain counter tip. And missing is a bit deluding, these keys doesn't "miss", we just want to achieve constant-size ahead safety buffer.

// We detect that the keypool has been consumed onchain until index. Increase counter by the diff to keep it constant int64_t keypool_size = std::max(gArgs.GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 1); int64_t to_extend = keypool_size + (index + 1) - (internal ? chain.nInternalChainCounter : chain.nExternalChainCounter);

Feel free to ignore, it's really IMO.

ariard · 2020-05-20T00:59:21Z

src/wallet/scriptpubkeyman.h

+     * @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


c93082e

I think a better param name for internal could be layout and comment `which layout chain should be used, either true for internal or false for external".

Also unable to derive _keys_ or unable to find HD-chain ?

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.

changes since last review per git diff 218c4f6 1ed52fb

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 7a62752b12..1bd33449fc 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -307,20 +307,20 @@ bool LegacyScriptPubKeyMan::TopUpInactiveHDChain(const CKeyID seed_id, int64_t i
     CHDChain& chain = it->second;
 
     // Top up key pool
-    int64_t target_size = std::max(gArgs.GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 0);
+    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(std::max((int64_t) target_size, (int64_t) 1) - kp_size, (int64_t) 0);
+    int64_t missing = std::max(target_size - kp_size, (int64_t) 0);
 
-    WalletBatch batch(m_storage.GetDatabase());
-    for (int64_t i = missing; i > 0; --i) {
-        GenerateNewKey(batch, chain, internal);
-    }
     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 {
@@ -337,7 +337,7 @@ void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
     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()) {
@@ -345,6 +345,8 @@ void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
             }
         }
 
+        // 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;
@@ -892,8 +894,18 @@ 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);
+        }
+    }
 
     m_hd_chain = chain;
 }
diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index 817bdceb87..5aaa17334c 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -341,7 +341,7 @@ private:
      * @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
+     * @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);
 
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
index fe94ca59ba..49db7914e4 100644
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -436,11 +436,15 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
                         return false;
                     }
                     if (path[0] != 0x80000000) {
-                        strErr = "Unexpected path index for the element at index 0";
+                        strErr = strprintf("Unexpected path index of 0x%08x (expected 0x80000000) for the element at index 0", path[0]);
                         return false;
                     }
-                    if (path[1] != 0x80000000 && path[1] != (1 | 0x80000000) && (path[2] & 0x80000000)) {
-                        strErr = "Unexpected path index for the element at index 1";
+                    if (path[1] != 0x80000000 && path[1] != (1 | 0x80000000)) {
+                        strErr = strprintf("Unexpected path index of 0x%08x (expected 0x80000000 or 0x80000001) for the element at index 1", path[1]);
+                        return false;
+                    }
+                    if ((path[2] & 0x80000000) == 0) {
+                        strErr = strprintf("Unexpected path index of 0x%08x (expected to be greater than or equal to 0x80000000)", path[2]);
                         return false;
                     }
                     internal = path[1] == (1 | 0x80000000);
@@ -453,7 +457,6 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
                 if (ins.second) {
                     // For new chains, we want to default to VERSION_HD_BASE until we see an internal
                     chain.nVersion = CHDChain::VERSION_HD_BASE;
-                    // Set the seed id
                     chain.seed_id = keyMeta.hd_seed_id;
                 }
                 if (internal) {
diff --git a/test/functional/wallet_hd.py b/test/functional/wallet_hd.py
index 6d0ca680e2..5b083a5398 100755
--- a/test/functional/wallet_hd.py
+++ b/test/functional/wallet_hd.py
@@ -188,7 +188,12 @@ class WalletHDTest(BitcoinTestFramework):
             restore_rpc.sethdseed(True, seed) # Set to be the same seed as origin_rpc
             restore_rpc.sethdseed(True) # Rotate to a new seed, making original `seed` inactive
 
-            # Check persistence of inactive seed
+            self.nodes[1].createwallet(wallet_name='restore2', blank=True)
+            restore2_rpc = self.nodes[1].get_wallet_rpc('restore2')
+            restore2_rpc.sethdseed(True, seed) # Set to be the same seed as origin_rpc
+            restore2_rpc.sethdseed(True) # Rotate to a new seed, making original `seed` inactive
+
+            # Check persistence of inactive seed by reloading restore. restore2 is still loaded to test the case where the wallet is not reloaded
             restore_rpc.unloadwallet()
             self.nodes[1].loadwallet('restore')
             restore_rpc = self.nodes[1].get_wallet_rpc('restore')
@@ -204,6 +209,10 @@ class WalletHDTest(BitcoinTestFramework):
             assert_equal(info['ismine'], True)
             info = restore_rpc.getaddressinfo(addr)
             assert_equal(info['ismine'], False)
+            info = restore2_rpc.getaddressinfo(last_addr)
+            assert_equal(info['ismine'], True)
+            info = restore2_rpc.getaddressinfo(addr)
+            assert_equal(info['ismine'], False)
             # Check that the origin seed has addr
             info = origin_rpc.getaddressinfo(addr)
             assert_equal(info['ismine'], True)
@@ -226,6 +235,8 @@ class WalletHDTest(BitcoinTestFramework):
             origin_rpc.gettransaction(txid)
             restore_rpc.gettransaction(txid)
             assert_raises_rpc_error(-5, 'Invalid or non-wallet transaction id', restore_rpc.gettransaction, out_of_kp_txid)
+            restore2_rpc.gettransaction(txid)
+            assert_raises_rpc_error(-5, 'Invalid or non-wallet transaction id', restore2_rpc.gettransaction, out_of_kp_txid)
 
             # After rescanning, restore_rpc should now see out_of_kp_txid and generate an additional key.
             # addr should now be part of restore_rpc and be ismine
@@ -233,6 +244,10 @@ class WalletHDTest(BitcoinTestFramework):
             restore_rpc.gettransaction(out_of_kp_txid)
             info = restore_rpc.getaddressinfo(addr)
             assert_equal(info['ismine'], True)
+            restore2_rpc.rescanblockchain()
+            restore2_rpc.gettransaction(out_of_kp_txid)
+            info = restore2_rpc.getaddressinfo(addr)
+            assert_equal(info['ismine'], True)
 
             # Check again that 3 keys were derived.
             # Empty keypool and get an address that is beyond the initial keypool
@@ -246,6 +261,10 @@ class WalletHDTest(BitcoinTestFramework):
             assert_equal(info['ismine'], True)
             info = restore_rpc.getaddressinfo(addr)
             assert_equal(info['ismine'], False)
+            info = restore2_rpc.getaddressinfo(last_addr)
+            assert_equal(info['ismine'], True)
+            info = restore2_rpc.getaddressinfo(addr)
+            assert_equal(info['ismine'], False)
 
 if __name__ == '__main__':
     WalletHDTest().main ()

Changeset review, gcc and clang builds with the works + ran tests and bitcoind.

Verified that the new tests halt at lines 248 ("Invalid or non-wallet transaction id"), 249 (assertion false), and 262 (assertion false), if the new AddInactiveHDChain call in LegacyScriptPubKeyMan::SetHDChain is removed.

achow101 · 2020-05-20T18:42:03Z

With 3 ACKs now, I think the remaining comments can be done in follow up PRs.

…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#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

…dd/Load variants 3a9aba2 Split SetWalletFlags into Add/LoadWalletFlags (Andrew Chow) d9cd095 Split SetActiveScriptPubKeyMan into Add/LoadActiveScriptPubKeyMan (Andrew Chow) 0122fba Split SetHDChain into AddHDChain and LoadHDChain (Andrew Chow) Pull request description: `SetHDChaiin`, `SetActiveScriptPubKeyMan`, and `SetWalletFlags` have a `memonly` argument which is kind of confusing, as noted in #17681 (comment). This PR replaces those functions with `Add*` and `Load*` variants so that they follow the pattern used elsewhere in the wallet. `AddHDChain`, `AddActiveScriptPubKeyMan`, and `AddWalletFlags` both set their respective variables in `CWallet` and writes them to disk. These functions are used by the actions which modify the wallet such as `sethdseed`, `importdescriptors`, and creating a new wallet. `LoadHDChain`, `LoadActiveScriptPubKeyMan`, and `LoadWalletFlags` just set the `CWallet` variables. These functions are used by `LoadWallet` when loading the wallet from disk. ACKs for top commit: jnewbery: Code review ACK 3a9aba2 ryanofsky: Code review ACK 3a9aba2. Only changes since last review tweaks making m_wallet_flags updates more safe meshcollider: utACK 3a9aba2 Tree-SHA512: 365aeaafc5ba42879c0eb797ec3beb29ab70e27f917dc880763f743420b3be6ddf797240996beed8a9ad70fb212c2590253c6b44c9dc244529c3939d9538983f

… with Add/Load variants 3a9aba2 Split SetWalletFlags into Add/LoadWalletFlags (Andrew Chow) d9cd095 Split SetActiveScriptPubKeyMan into Add/LoadActiveScriptPubKeyMan (Andrew Chow) 0122fba Split SetHDChain into AddHDChain and LoadHDChain (Andrew Chow) Pull request description: `SetHDChaiin`, `SetActiveScriptPubKeyMan`, and `SetWalletFlags` have a `memonly` argument which is kind of confusing, as noted in bitcoin#17681 (comment). This PR replaces those functions with `Add*` and `Load*` variants so that they follow the pattern used elsewhere in the wallet. `AddHDChain`, `AddActiveScriptPubKeyMan`, and `AddWalletFlags` both set their respective variables in `CWallet` and writes them to disk. These functions are used by the actions which modify the wallet such as `sethdseed`, `importdescriptors`, and creating a new wallet. `LoadHDChain`, `LoadActiveScriptPubKeyMan`, and `LoadWalletFlags` just set the `CWallet` variables. These functions are used by `LoadWallet` when loading the wallet from disk. ACKs for top commit: jnewbery: Code review ACK 3a9aba2 ryanofsky: Code review ACK 3a9aba2. Only changes since last review tweaks making m_wallet_flags updates more safe meshcollider: utACK 3a9aba2 Tree-SHA512: 365aeaafc5ba42879c0eb797ec3beb29ab70e27f917dc880763f743420b3be6ddf797240996beed8a9ad70fb212c2590253c6b44c9dc244529c3939d9538983f

…anism Summary: f41d58966995fe69df433fa684117fae74a56e66 Document better -keypool as a look-ahead safety mechanism (Antoine Riard) Pull request description: If after a backup, an address is issued beyond the initial keypool range and none of the addresses in this range is seen onchain, if a wallet is restored from backup, even in case of rescan, funds may be loss due to the look-ahead buffer not being incremented and so restored wallet not detecting onchain out-of-range address as derived from its seed. This scenario is theoretically unavoidable due to the requirement of the keypool to have a max size. However, given the default keypool size, this is unlikely. Document better keypool size implications to avoid user setting a too low value. While reviewing #17681, it took me a while to figure out the safety implications of keypool, I find it would be better to document this a bit farther to avoid users shooting themselves in the foot. For further context & discussion, see bitcoin/bitcoin#17681 (comment) ACKs for top commit: ryanofsky: Code review ACK f41d58966995fe69df433fa684117fae74a56e66. Just "Warning:" prefix added since the last review jonatack: ACK f41d58966995fe69df433fa684117fae74a56e66 code review and build/test. The added `Warning:` since last review is a good addition. --- Backport of Core [[bitcoin/bitcoin#17719 | PR17719]] Test Plan: ninja read it Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7836

…mechanism f41d589 Document better -keypool as a look-ahead safety mechanism (Antoine Riard) Pull request description: If after a backup, an address is issued beyond the initial keypool range and none of the addresses in this range is seen onchain, if a wallet is restored from backup, even in case of rescan, funds may be loss due to the look-ahead buffer not being incremented and so restored wallet not detecting onchain out-of-range address as derived from its seed. This scenario is theoretically unavoidable due to the requirement of the keypool to have a max size. However, given the default keypool size, this is unlikely. Document better keypool size implications to avoid user setting a too low value. While reviewing bitcoin#17681, it took me a while to figure out the safety implications of keypool, I find it would be better to document this a bit farther to avoid users shooting themselves in the foot. For further context & discussion, see bitcoin#17681 (comment) ACKs for top commit: ryanofsky: Code review ACK f41d589. Just "Warning:" prefix added since the last review jonatack: ACK f41d589 code review and build/test. The added `Warning:` since last review is a good addition. Tree-SHA512: d3d0ee88fcdfc5c8841a2bd4bada0e4eeb412a0dce5054e5fb023643c2fa57206a0f3efb06890c245528dc4431413ed2fd5645b9319d26245d044c490b7f0db0

…ve keys from them as needed Summary: 1ed52fbb4d81f7b7634fd4fb6d1d00e1478129dc Remove IBD check in sethdseed (Andrew Chow) b1810a145a601a8064e4094350cfb6ddafbdb4d8 Test that keys from inactive seeds are generated (Andrew Chow) c93082ece40b1c72f05b3e2085c022c09eaa4d65 Generate new keys for inactive seeds after marking used (Andrew Chow) 45f2f6a0e8514a0438a87554400bf73cbb90707f Determine inactive HD seeds from key metadata and track them in LegacyScriptPubKeyMan (Andrew Chow) b59b4504abf96cec860badfed2ac793ae5d40ced 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. --- Backport of Core [[bitcoin/bitcoin#17681 | PR17681]] Test Plan: CC=clang CXX=clang++ cmake .. -GNinja -DENABLE_SANITIZERS=thread ninja all check check-functional cmake .. -GNinja -DCMAKE_BUILD_TYPE=Debug ninja all check check-functional Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D9196

fanquake added the Wallet label Dec 5, 2019

achow101 mentioned this pull request Dec 5, 2019

wallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload #17484

Closed

fanquake requested a review from instagibbs December 6, 2019 18:31

DrahtBot added the Needs rebase label Dec 6, 2019

achow101 force-pushed the keep-inactive-seeds branch from 7a80187 to c4ae01c Compare December 6, 2019 20:04

DrahtBot removed the Needs rebase label Dec 6, 2019

paymog reviewed Dec 8, 2019

View reviewed changes

achow101 force-pushed the keep-inactive-seeds branch from c4ae01c to c16b8d4 Compare December 8, 2019 18:00

ariard reviewed Dec 9, 2019

View reviewed changes

achow101 force-pushed the keep-inactive-seeds branch from c16b8d4 to fc7a5e4 Compare December 9, 2019 23:17

ariard mentioned this pull request Dec 11, 2019

Document better -keypool as a look-ahead safety mechanism #17719

Merged

promag reviewed Dec 29, 2019

View reviewed changes

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the keep-inactive-seeds branch from fc7a5e4 to ff49e4d Compare December 30, 2019 21:57

promag reviewed Jan 4, 2020

View reviewed changes

achow101 force-pushed the keep-inactive-seeds branch from ff49e4d to 023cc98 Compare January 6, 2020 17:03

DrahtBot added the Needs rebase label Jan 30, 2020

achow101 force-pushed the keep-inactive-seeds branch from 023cc98 to b7e4f19 Compare January 30, 2020 16:56

achow101 force-pushed the keep-inactive-seeds branch from a5c3f32 to 218c4f6 Compare May 12, 2020 17:58

jonatack reviewed May 12, 2020

View reviewed changes

fjahr approved these changes May 13, 2020

View reviewed changes

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

jkczyz reviewed May 13, 2020

View reviewed changes

src/wallet/walletdb.cpp Outdated Show resolved Hide resolved

src/wallet/walletdb.cpp Outdated Show resolved Hide resolved

jonatack reviewed May 13, 2020

View reviewed changes

src/wallet/scriptpubkeyman.cpp Show resolved Hide resolved

rajarshimaitra reviewed May 15, 2020

View reviewed changes

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

achow101 added 4 commits May 15, 2020 18:00

Determine inactive HD seeds from key metadata and track them in Legac…

45f2f6a

…yScriptPubKeyMan

Generate new keys for inactive seeds after marking used

c93082e

When a key from an inactive seed is used, generate replacements to fill a keypool that would have been there.

Test that keys from inactive seeds are generated

b1810a1

achow101 force-pushed the keep-inactive-seeds branch from 218c4f6 to 1ed52fb Compare May 15, 2020 22:00

This was referenced May 16, 2020

wallet: Refactor the classes in wallet/db.{cpp/h} #18971

Closed

wallet: Move salvagewallet into wallettool #18918

Merged

ryanofsky approved these changes May 19, 2020

View reviewed changes

jnewbery reviewed May 19, 2020

View reviewed changes

ariard reviewed May 20, 2020

View reviewed changes

jonatack reviewed May 20, 2020

View reviewed changes

meshcollider merged commit ccd85b5 into bitcoin:master May 22, 2020

achow101 mentioned this pull request May 22, 2020

Replace CWallet::Set* functions that use memonly with Add/Load variants #19046

Merged

gmaxwell mentioned this pull request May 30, 2020

wallet: Skip hdKeypath of 'm' when determining inactive hd seeds #19054

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Conversation

achow101 commented Dec 5, 2019 • edited Loading

DrahtBot commented Dec 5, 2019 • edited Loading

Conflicts

achow101 commented Dec 8, 2019

ariard left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard commented Dec 10, 2019

achow101 commented Dec 10, 2019

ariard commented Dec 10, 2019

achow101 commented Dec 11, 2019

ariard commented Dec 11, 2019

achow101 commented Dec 11, 2019

promag left a comment

Choose a reason for hiding this comment

luke-jr commented Jan 3, 2020

promag left a comment • edited Loading

Choose a reason for hiding this comment

achow101 commented Jan 6, 2020

jonasschnelli commented Jan 9, 2020

jnewbery commented Jan 30, 2020

jonatack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjahr left a comment • edited Loading

Choose a reason for hiding this comment

theStack commented May 13, 2020

jkczyz left a comment

Choose a reason for hiding this comment

rajarshimaitra commented May 15, 2020

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 May 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack left a comment

Choose a reason for hiding this comment

achow101 commented May 20, 2020

achow101 commented Dec 5, 2019 •

edited

Loading

DrahtBot commented Dec 5, 2019 •

edited

Loading

ariard left a comment •

edited

Loading

promag left a comment •

edited

Loading

fjahr left a comment •

edited

Loading

achow101 May 20, 2020 •

edited

Loading