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

achow101 commented at 9:54 pm on December 5, 2019: member

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.

fanquake added the label Wallet on Dec 5, 2019

DrahtBot commented at 10:21 pm on December 5, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18923 (wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off by MarcoFalke)
#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.

fanquake requested review from instagibbs on Dec 6, 2019

DrahtBot added the label Needs rebase on Dec 6, 2019

achow101 force-pushed on Dec 6, 2019

DrahtBot removed the label Needs rebase on Dec 6, 2019

in src/wallet/scriptpubkeyman.cpp:283 in c4ae01c812 outdated

274@@ -275,6 +275,48 @@ bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool i
275     return true;
276 }
277 
278+bool LegacyScriptPubKeyMan::FillInactiveChain(const CKeyID seed_id, int64_t index, bool internal)

paymog commented at 9:22 am on December 7, 2019:

should index and internal be made const too?

paymog commented at 9:29 am on December 7, 2019:

Documentation for the function would be nice, whether here or in the header file. What does this function do? What does the return value mean?

achow101 commented at 4:59 pm on December 8, 2019:

I don’t think it really matters. They are not references and are primitives.

achow101 commented at 6:00 pm on December 8, 2019:

Added a comment.

jonatack commented at 11:38 am on March 16, 2020:

agree with @achow101, see https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f16-for-in-parameters-pass-cheaply-copied-types-by-value-and-others-by-reference-to-const

in src/wallet/scriptpubkeyman.cpp:307 in c4ae01c812 outdated

284+    auto it = m_inactive_hd_chains.find(seed_id);
285+    if (it == m_inactive_hd_chains.end()) {
286+        return false;
287+    }
288+
289+    CHDChain& chain = it->second;

paymog commented at 9:31 am on December 7, 2019:

const? Seems like this isn’t be modified later in the function

achow101 commented at 5:04 pm on December 8, 2019:

GenerateNewKey modifies it.

in src/wallet/scriptpubkeyman.cpp:292 in c4ae01c812 outdated

287+    }
288+
289+    CHDChain& chain = it->second;
290+
291+    // Top up key pool
292+    unsigned int target_size = std::max(gArgs.GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 0);

paymog commented at 9:40 am on December 7, 2019:

should the type on the left hand side be int64_t? It seems GetArg returns that type and you’re casting 0 to that type too: https://github.com/bitcoin/bitcoin/blob/23cecd6cd56f952c757f469c46d7593c2ffaa419/src/util/system.h#L234

achow101 commented at 6:00 pm on December 8, 2019:

Done

in src/wallet/scriptpubkeyman.cpp:317 in c4ae01c812 outdated

295+    int64_t kp_size = (internal ? chain.nInternalChainCounter : chain.nExternalChainCounter) - index;
296+
297+    // count amount of available keys (internal, external)
298+    // make sure the keypool of external and internal keys fits the user selected target (-keypool)
299+    // available is determined based on the index given
300+    int64_t missing = std::max(std::max((int64_t) target_size, (int64_t) 1) - kp_size, (int64_t) 0);

paymog commented at 9:42 am on December 7, 2019:

(int64_t) 0 is used twice now, should it be made into a constant?

achow101 commented at 5:05 pm on December 8, 2019:

I don’t think it’s worth it.

in src/wallet/scriptpubkeyman.cpp:303 in c4ae01c812 outdated

298+    // make sure the keypool of external and internal keys fits the user selected target (-keypool)
299+    // available is determined based on the index given
300+    int64_t missing = std::max(std::max((int64_t) target_size, (int64_t) 1) - kp_size, (int64_t) 0);
301+
302+    WalletBatch batch(m_storage.GetDatabase());
303+    for (int64_t i = missing; i--;)

paymog commented at 9:44 am on December 7, 2019:

is this missing a termination condition?

achow101 commented at 6:01 pm on December 8, 2019:

I copied this out of TopUp and technically this is correct. But it’s hard to read and reason about, so I’ve changed it to the standard way of doing for loops.

in src/wallet/scriptpubkeyman.cpp:796 in c4ae01c812 outdated

792+    if (!memonly) {
793+        if (!WalletBatch(m_storage.GetDatabase()).WriteHDChain(chain)) {
794+            throw std::runtime_error(std::string(__func__) + ": writing chain failed");
795+        }
796+        if (!hdChain.seed_id.IsNull()) {
797+            AddInactiveHDChain(hdChain, false);

paymog commented at 9:03 am on December 8, 2019:

sanity check: should this be hdChain or chain?

achow101 commented at 5:12 pm on December 8, 2019:

It is supposed to be hdChain.

SetHDChain happens when we change hdChain. We want to store what is currently hdChain as an inactive seed before we switch it out for the new one. So this is correct, we add it as an inactive chain before it is set to the new chain.

in src/wallet/scriptpubkeyman.cpp:917 in c4ae01c812 outdated

805+{
806+    LOCK(cs_wallet);
807+    if (!memonly && !WalletBatch(m_storage.GetDatabase()).WriteInactiveHDChain(chain))
808+        throw std::runtime_error(std::string(__func__) + ": writing inactive chain failed");
809+
810+    m_inactive_hd_chains[chain.seed_id] = chain;

paymog commented at 9:04 am on December 8, 2019:

should we check if chain.seed_id is null here?

achow101 commented at 6:01 pm on December 8, 2019:

It should never be null. I’ve added an assert to check that.

in src/wallet/scriptpubkeyman.cpp:901 in c4ae01c812 outdated

786@@ -732,12 +787,27 @@ bool LegacyScriptPubKeyMan::AddWatchOnly(const CScript& dest, int64_t nCreateTim
787 void LegacyScriptPubKeyMan::SetHDChain(const CHDChain& chain, bool memonly)
788 {
789     LOCK(cs_wallet);
790-    if (!memonly && !WalletBatch(m_storage.GetDatabase()).WriteHDChain(chain))
791-        throw std::runtime_error(std::string(__func__) + ": writing chain failed");
792+    if (!memonly) {
793+        if (!WalletBatch(m_storage.GetDatabase()).WriteHDChain(chain)) {

paymog commented at 9:12 am on December 8, 2019:

I’m new to Bitcoin and I’m still learning the testing standards - should there be a test where this exception is expected to be thrown?

achow101 commented at 4:56 pm on December 8, 2019:

No. This is only expected to be thrown under corruption and/or hardware failure conditions.

achow101 force-pushed on Dec 8, 2019

achow101 commented at 6:02 pm on December 8, 2019: member

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

in src/wallet/scriptpubkeyman.cpp:749 in cf80607b12 outdated

746     hdChain = chain;
747 }
748 
749+void LegacyScriptPubKeyMan::AddInactiveHDChain(const CHDChain& chain, bool memonly)
750+{
751+    LOCK(cs_wallet);

ariard commented at 10:17 pm on December 9, 2019:

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

achow101 commented at 11:17 pm on December 9, 2019:

Done

in src/wallet/scriptpubkeyman.cpp:740 in cf80607b12 outdated

737+    if (!memonly) {
738+        if (!WalletBatch(m_storage.GetDatabase()).WriteHDChain(chain)) {
739+            throw std::runtime_error(std::string(__func__) + ": writing chain failed");
740+        }
741+        if (!hdChain.seed_id.IsNull()) {
742+            AddInactiveHDChain(hdChain, false);

ariard commented at 10:22 pm on December 9, 2019:

Pass memonly? (even if value is always false for now)

achow101 commented at 11:17 pm on December 9, 2019:

Done

ariard commented at 10:55 pm on December 9, 2019: member

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.

achow101 force-pushed on Dec 9, 2019

ariard commented at 2:31 am on December 10, 2019: member

@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 commented at 2:49 am on December 10, 2019: member

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 commented at 11:24 pm on December 10, 2019: member

Okay thanks for answer, added a commit on top of your branch (https://github.com/ariard/bitcoin/commit/3c25ab7f36319cbe38d1fb65a321b1234635648e) 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 commented at 1:48 am on December 11, 2019: member

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

ariard commented at 3:49 am on December 11, 2019: member

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 commented at 3:54 am on December 11, 2019: member

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

in src/wallet/scriptpubkeyman.cpp:305 in fc7a5e4d53 outdated

300+    // available is determined based on the index given
301+    int64_t missing = std::max(std::max((int64_t) target_size, (int64_t) 1) - kp_size, (int64_t) 0);
302+
303+    WalletBatch batch(m_storage.GetDatabase());
304+    for (int64_t i = missing; i > 0; --i)
305+    {

promag commented at 11:21 pm on December 29, 2019:

nit, above.

achow101 commented at 9:57 pm on December 30, 2019:

Fixed

promag commented at 11:47 pm on December 29, 2019: member

Concept ACK.

achow101 force-pushed on Dec 30, 2019

luke-jr commented at 6:54 pm on January 3, 2020: member

Concept ACK

promag commented at 11:43 pm on January 4, 2020: member

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

achow101 commented at 5:03 pm on January 6, 2020: member

Renamed it.

achow101 force-pushed on Jan 6, 2020

jonasschnelli commented at 5:26 am on January 9, 2020: contributor

Nice work! Concept ACK.

meshcollider referenced this in commit aabec94541 on Jan 29, 2020

DrahtBot added the label Needs rebase on Jan 30, 2020

jnewbery commented at 3:54 pm on January 30, 2020: member

Concept ACK

achow101 force-pushed on Jan 30, 2020

DrahtBot removed the label Needs rebase on Jan 30, 2020

sidhujag referenced this in commit 7aec48ac80 on Feb 1, 2020

Sjors commented at 12:14 pm on February 19, 2020: member

Travis is tripping over some lock issue. As is macOS with --enable-debug:

0wallet/walletdb.cpp:399:58: error: calling function 'AddInactiveHDChain' requires holding mutex 'pwallet->GetOrCreateLegacyScriptPubKeyMan().cs_KeyStore' exclusively [-Werror,-Wthread-safety-analysis]
1            pwallet->GetOrCreateLegacyScriptPubKeyMan()->AddInactiveHDChain(chain, true);

achow101 force-pushed on Feb 19, 2020

achow101 force-pushed on Feb 20, 2020

in test/functional/wallet_hd.py:156 in d435512094 outdated

152@@ -153,5 +153,74 @@ def run_test(self):
153         assert_raises_rpc_error(-5, "Already have this key", self.nodes[1].sethdseed, False, new_seed)
154         assert_raises_rpc_error(-5, "Already have this key", self.nodes[1].sethdseed, False, self.nodes[1].dumpprivkey(self.nodes[1].getnewaddress()))
155 
156+        self.log.info('Test sethdseed restoring with keys outside of the intial keypool')

jonatack commented at 5:34 pm on March 15, 2020:

nit: initial

achow101 commented at 5:44 pm on March 16, 2020:

Done

in test/functional/wallet_hd.py:158 in d435512094 outdated

152@@ -153,5 +153,74 @@ def run_test(self):
153         assert_raises_rpc_error(-5, "Already have this key", self.nodes[1].sethdseed, False, new_seed)
154         assert_raises_rpc_error(-5, "Already have this key", self.nodes[1].sethdseed, False, self.nodes[1].dumpprivkey(self.nodes[1].getnewaddress()))
155 
156+        self.log.info('Test sethdseed restoring with keys outside of the intial keypool')
157+        self.nodes[0].generate(10)
158+        # Restart node 1 with keypool of 5 and a different wallet

jonatack commented at 5:37 pm on March 15, 2020:

keypool of 3

achow101 commented at 5:44 pm on March 16, 2020:

Done

in test/functional/wallet_hd.py:183 in d435512094 outdated

178+
179+        # Empty keypool and get an address that is beyond the initial keypool
180+        origin_rpc.getnewaddress()
181+        origin_rpc.getnewaddress()
182+        last_addr = origin_rpc.getnewaddress()
183+        addr = origin_rpc.getnewaddress()

jonatack commented at 5:54 pm on March 15, 2020:

suggestion

0-        # Empty keypool and get an address that is beyond the initial keypool
1+        # Empty the origin keypool and get an address that is beyond the initial keypool of 3
2         origin_rpc.getnewaddress()
3         origin_rpc.getnewaddress()
4-        last_addr = origin_rpc.getnewaddress()
5-        addr = origin_rpc.getnewaddress()
6+        last_addr = origin_rpc.getnewaddress() # last address of initial keypool
7+        addr = origin_rpc.getnewaddress() # address beyond initial keypool of 3

achow101 commented at 5:44 pm on March 16, 2020:

Done

in test/functional/wallet_hd.py:192 in d435512094 outdated

186+        info = restore_rpc.getaddressinfo(last_addr)
187+        assert_equal(info['ismine'], True)
188+        info = restore_rpc.getaddressinfo(addr)
189+        assert_equal(info['ismine'], False)
190+        info = origin_rpc.getaddressinfo(addr)
191+        assert_equal(info['ismine'], True)

jonatack commented at 6:10 pm on March 15, 2020:

suggest mentioning in order of checks

0-        # Check that the restored seed does not have addr but does have last_addr
1+        # Check that the restored seed has last_addr but does not have addr
2         info = restore_rpc.getaddressinfo(last_addr)
3         assert_equal(info['ismine'], True)
4         info = restore_rpc.getaddressinfo(addr)
5         assert_equal(info['ismine'], False)
6+        # Check that the origin seed does have addr
7         info = origin_rpc.getaddressinfo(addr)
8         assert_equal(info['ismine'], True)

achow101 commented at 5:44 pm on March 16, 2020:

Done

in test/functional/wallet_hd.py:172 in d435512094 outdated

167+        origin_rpc.sethdseed(True, seed)
168+
169+        self.nodes[1].createwallet(wallet_name='restore', blank=True)
170+        restore_rpc = self.nodes[1].get_wallet_rpc('restore')
171+        restore_rpc.sethdseed(True, seed)
172+        restore_rpc.sethdseed(True)

jonatack commented at 6:15 pm on March 15, 2020:

This line seems redundant?

achow101 commented at 4:29 pm on March 16, 2020:

It is not. We are first setting the original seed (we set it to make sure it is shared), then setting a new seed to simulate a seed rotation.

jonatack commented at 5:17 pm on March 16, 2020:

Oh, right – reading the cli help again, both calls are flushing; the first call sets the seed with the passed seed value and the second call sets a random seed. Maybe add a comment to explain the intention or simulation.

achow101 commented at 5:47 pm on March 16, 2020:

Added a comment.

in test/functional/wallet_hd.py:219 in d435512094 outdated

214+        origin_rpc.getnewaddress()
215+        origin_rpc.getnewaddress()
216+        last_addr = origin_rpc.getnewaddress()
217+        addr = origin_rpc.getnewaddress()
218+
219+        # Check that the restored seed does not have addr but does have last_addr

jonatack commented at 6:37 pm on March 15, 2020:

suggest mentioning in order of checks

0-        # Check that the restored seed does not have addr but does have last_addr
1+        # Check that the restored seed has last_addr but does not have addr

achow101 commented at 5:44 pm on March 16, 2020:

Done

in test/functional/wallet_hd.py:218 in d435512094 outdated

205+        assert_raises_rpc_error(-5, 'Invalid or non-wallet transaction id', restore_rpc.gettransaction, out_of_kp_txid)
206+
207+        restore_rpc.rescanblockchain()
208+        restore_rpc.gettransaction(out_of_kp_txid)
209+        info = restore_rpc.getaddressinfo(addr)
210+        assert_equal(info['ismine'], True)

jonatack commented at 6:38 pm on March 15, 2020:

Could you add comments for the 3 sections above?

achow101 commented at 5:44 pm on March 16, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:1017 in d435512094 outdated

1023+    if (hd_chain.seed_id == m_hd_chain.seed_id) {
1024+        if (!batch.WriteHDChain(hd_chain))
1025+            throw std::runtime_error(std::string(__func__) + ": Writing HD chain model failed");
1026+    } else {
1027+        if (!batch.WriteInactiveHDChain(hd_chain))
1028+            throw std::runtime_error(std::string(__func__) + ": Writing HD chain model failed");

jonatack commented at 10:57 am on March 16, 2020:

nit suggestion here and line 1014 just above: git grepping indicates these are lowercased elsewhere, except when the message begins with a class name

0- throw std::runtime_error(std::string(__func__) + ": Writing HD chain model failed");
1+ throw std::runtime_error(std::string(__func__) + ": writing HD chain model failed");

achow101 commented at 5:44 pm on March 16, 2020:

Done

in src/wallet/walletdb.h:123 in d435512094 outdated

109@@ -110,6 +110,11 @@ class CHDChain
110         nInternalChainCounter = 0;
111         seed_id.SetNull();
112     }
113+
114+    bool operator==(const CHDChain& chain) const
115+    {
116+        return seed_id == chain.seed_id;
117+    }

jonatack commented at 11:29 am on March 16, 2020:

should also define bool operator!=…?

achow101 commented at 5:45 pm on March 16, 2020:

It should be implicitly defined.

sipa commented at 6:05 pm on March 16, 2020:

Default comparison operators are only being added in C++20. That said, if nothing needs !=, no need to define it.

in src/wallet/scriptpubkeyman.h:325 in d435512094 outdated

319@@ -306,6 +320,18 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
320      */
321     bool ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRequestedInternal);
322 
323+    /**
324+     * Like TopUp() but adds keys for inactive HD chains.
325+     * Ensures that there are at least -keypoolsize number of keys derived after the given index.

jonatack commented at 11:34 am on March 16, 2020:

-keypool (keypoolsize)

achow101 commented at 5:45 pm on March 16, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:304 in d435512094 outdated

299+    // "size" of the keypools. Not really the size, actually the difference between index and the chain counter
300+    // Since chain counter is 1 based and index is 0 based, one of them needs to be offset by 1.
301+    int64_t kp_size = (internal ? chain.nInternalChainCounter : chain.nExternalChainCounter) - (index + 1);
302+
303+    // count amount of available keys (internal, external)
304+    // make sure the keypool of external and internal keys fits the user selected target (-keypool)

jonatack commented at 11:37 am on March 16, 2020:

nit: s/user selected/user-selected/

achow101 commented at 5:45 pm on March 16, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:317 in d435512094 outdated

301+    int64_t kp_size = (internal ? chain.nInternalChainCounter : chain.nExternalChainCounter) - (index + 1);
302+
303+    // count amount of available keys (internal, external)
304+    // make sure the keypool of external and internal keys fits the user selected target (-keypool)
305+    // available is determined based on the index given
306+    int64_t missing = std::max(std::max((int64_t) target_size, (int64_t) 1) - kp_size, (int64_t) 0);

jonatack commented at 12:20 pm on March 16, 2020:

naming/documentation: both here in FillInactiveChain() and in TopUp(), “available” and missing seem like opposites

achow101 commented at 5:45 pm on March 16, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:283 in d435512094 outdated

279@@ -280,6 +280,48 @@ bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool i
280     return true;
281 }
282 
283+bool LegacyScriptPubKeyMan::FillInactiveChain(const CKeyID seed_id, int64_t index, bool internal)

jonatack commented at 12:25 pm on March 16, 2020:

musing on naming here: consider naming the function FillUpInactiveHDChain or TopUpInactiveHDChain:

to say it’s an HD chain (which shows why no need to check !CanGenerateKeys() here IIUC)
naming similar to AddInactiveHDChain and WriteInActiveHDChain
best to avoid “I” next to “ll”
and if desired, show similarity to TopUp as the functions are similar, unless you don’t want grepping for TopUp to return both

achow101 commented at 5:45 pm on March 16, 2020:

Renamed to TopUpInactiveHDChain

in test/functional/wallet_hd.py:268 in d435512094 outdated

219+        # Check that the restored seed does not have addr but does have last_addr
220+        info = restore_rpc.getaddressinfo(last_addr)
221+        assert_equal(info['ismine'], True)
222+        info = restore_rpc.getaddressinfo(addr)
223+        assert_equal(info['ismine'], False)
224+

jonatack commented at 1:24 pm on March 16, 2020:

for info I appended these checks after the rescan so that all checks before the rescan are also repeated after it, and all still pass

 0         assert_equal(info['ismine'], False)
 1+        info = origin_rpc.getaddressinfo(addr)
 2+        assert_equal(info['ismine'], True)
 3+
 4+        txid = self.nodes[0].sendtoaddress(addr, 1)
 5+        origin_rpc.sendrawtransaction(self.nodes[0].gettransaction(txid)['hex'])
 6+        self.nodes[0].generate(1)
 7+        origin_rpc.gettransaction(txid)
 8+        assert_raises_rpc_error(-5, 'Invalid or non-wallet transaction id', restore_rpc.gettransaction, txid)
 9+        out_of_kp_txid = txid
10+
11+        txid = self.nodes[0].sendtoaddress(last_addr, 1)
12+        origin_rpc.sendrawtransaction(self.nodes[0].gettransaction(txid)['hex'])
13+        self.nodes[0].generate(1)
14+        origin_rpc.gettransaction(txid)
15+        restore_rpc.gettransaction(txid)
16+        assert_raises_rpc_error(-5, 'Invalid or non-wallet transaction id', restore_rpc.gettransaction, out_of_kp_txid)
17+
18+        restore_rpc.rescanblockchain()
19+        restore_rpc.gettransaction(out_of_kp_txid)
20+        info = restore_rpc.getaddressinfo(addr)
21+        assert_equal(info['ismine'], True)
22 
23 if __name__ == '__main__':
24     WalletHDTest().main ()

ryanofsky commented at 2:57 pm on May 7, 2020:

In commit “Test that keys from inactive seeds are generated” (5932c00fef5054207bd133daa1aea9857d9c3611)

re: #17681 (review)

for info I appended these checks after the rescan so that all checks before the rescan are also repeated after it, and all still pass

Not sure if this review comment was made based on an older version of the test, but while these checks do good as a sanity check for rescan, I think adding them here would muddle the purpose of this test, and wouldn’t be great to add here unless preceded by an explanatory comment, or pulled into a function to keep the test readble

in test/functional/wallet_hd.py:216 in d435512094 outdated

203+        origin_rpc.gettransaction(txid)
204+        restore_rpc.gettransaction(txid)
205+        assert_raises_rpc_error(-5, 'Invalid or non-wallet transaction id', restore_rpc.gettransaction, out_of_kp_txid)
206+
207+        restore_rpc.rescanblockchain()
208+        restore_rpc.gettransaction(out_of_kp_txid)

jonatack commented at 1:41 pm on March 16, 2020:

On current master d402c1e4d, the new test fails here after the rescan with Invalid or non-wallet transaction id (-5), is this where it fails for you, and should it not fail on one of the assertions?

achow101 commented at 4:27 pm on March 16, 2020:

No, this is the correct place for it to fail. The asserts above should pass because on mater, we still have the keypool of 3 so all the things existing/not existing is the same. The difference is that the out_of_kp_txid is not seen by master because it uses a key that is out of the original keypool. Since master does not continue to extend the original keypool, it fails to detect out_of_kp_txid so it is supposed to fail here.

jonatack commented at 5:21 pm on March 16, 2020:

Ok thanks. Upgraded the Concept ACK to an ACK with non-blocking comments.

jonatack commented at 1:52 pm on March 16, 2020: member

ACK d435512094ccd447ce0d582fa0e9ea85c46ebec3. Code review, built and tested on the branch and rebased on current master, verified the new test fails on master. A few non-blocking comments follow.

achow101 force-pushed on Mar 16, 2020

in src/wallet/scriptpubkeyman.cpp:310 in 0715613ce4 outdated

292+    }
293+
294+    CHDChain& chain = it->second;
295+
296+    // Top up key pool
297+    int64_t target_size = std::max(gArgs.GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 0);

ariard commented at 7:09 pm on March 16, 2020:

0715613

If you switch -keypool arg after switching seed to a smaller one, you may end up with less look-ahead protection for a seed which may be used for higher amounts or different funds policy. That’s already an issue today without seed rotation, but I would be okay to make it a static parameter part of m_inactive_hd_chains.

achow101 commented at 7:23 pm on March 16, 2020:

I don’t think that’s necessary. Since sethdseed’s purpose is seed rotation, it doesn’t really make sense that there would be different funds policies.

in src/wallet/scriptpubkeyman.cpp:296 in 0715613ce4 outdated

279@@ -280,6 +280,47 @@ bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool i
280     return true;
281 }
282 
283+bool LegacyScriptPubKeyMan::TopUpInactiveHDChain(const CKeyID seed_id, int64_t index, bool internal)

ariard commented at 7:12 pm on March 16, 2020:

0715613

Too much work for parameterizing TopUp with seed_id or other reasons?

achow101 commented at 7:26 pm on March 16, 2020:

Needing index makes it harder to parameterize.

ariard commented at 7:13 pm on March 16, 2020: member

Code Review ACK 5fafe5e

jonatack commented at 9:20 pm on March 16, 2020: member

Re-ACK 5fafe5e reviewed changes since last review with git diff d435512..5fafe5e, rebased on master @ 25424cf57, built/ran tests/bitcoind

DrahtBot added the label Needs rebase on Mar 25, 2020

achow101 force-pushed on Mar 25, 2020

DrahtBot removed the label Needs rebase on Mar 25, 2020

ryanofsky commented at 1:58 am on May 1, 2020: member

Code review almost-ACK d27ced4eeaf59d093b4eedeed6c344f3f80082c1

Arrived here while reviewing #17484, and happy to see another really nicely implemented PR! Clean changes and great code comments and tests. Only thing that I think is a problem (and easily addressed) is decision to read/write/rely on new “inactivehdchain” database rows. I guess there are two slightly different decisions I’m concerned about:

Decision to rely on “inactivehdchain” rows being written to top up old seeds instead of always topping up old seeds.
Decision to write “inactivehdchain” rows when sethdseed is called.

I think (1) isn’t good because it results in behavior that could be unnecessarily confusing and seem unreliable to users. Wallet loading code could just populate the m_inactive_hd_chains map from existing key origin data during loading, without requiring “inactivehdchain” database rows to have been written previously. A problem with requiring the rows to top up is that now instead of having two types of seeds in the wallet: active and inactive, there are three kinds: active, inactive and topped up, inactive and dormant which could be confusing to users who will see different behavior between the different inactive types, and not have control over what type they end up with without switching software versions. So it would be great to see m_inactive_hd_chains map and counters just populated automatically with origin data when wallet is loaded.

Decision (2) to write “inactivehdchain” rows may be justified even if the rows aren’t required for topping up, because maybe there is some useful metadata or history they provide, but it would be good to figure out if there is actually anything in there worth keeping when a new seed is set. Also, WalletBatch::WriteInactiveHDChain implemented in 40635251387758aac8a578b72e01b265dcc15d77, seems like it only writes a single row to the database with each new inactive chain overwriting the last, so I wonder if that behaves as intended in the current implementation.

Everything else seems really good aside from this one point. Great to see this PR!

have GenerateNewKey and DeriveNewChildKey take a CHDChain as an argument b59b4504ab

achow101 force-pushed on May 2, 2020

achow101 commented at 2:03 am on May 2, 2020: member

I’ve changed this to get rid of the inactivehdseed records and just interpret the keymeta records during loading.

in test/functional/wallet_hd.py:86 in 65dee3410c outdated

82@@ -83,6 +83,7 @@ def run_test(self):
83         shutil.rmtree(os.path.join(self.nodes[1].datadir, self.chain, "blocks"))
84         shutil.rmtree(os.path.join(self.nodes[1].datadir, self.chain, "chainstate"))
85         shutil.copyfile(os.path.join(self.nodes[1].datadir, "hd.bak"), os.path.join(self.nodes[1].datadir, self.chain, 'wallets', "wallet.dat"))
86+        return

ryanofsky commented at 2:02 pm on May 7, 2020:

In commit “fixup! Determine inactive HD seeds from key metadata and track them in LegacyScriptPubKeyMan” (65dee3410cd9807bb4aa6b6677c2e0832e050ea2)

Should fix: probably need to drop this performance optimization

achow101 commented at 3:59 pm on May 7, 2020:

Whoops. Fixed

in src/wallet/walletdb.cpp:689 in 65dee3410c outdated

685@@ -685,7 +686,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
686                 if (IsKeyType(strType) || strType == DBKeys::DEFAULTKEY) {
687                     result = DBErrors::CORRUPT;
688                 } else if (strType == DBKeys::FLAGS) {
689-                    // reading the wallet flags can only fail if unknown flags are present
690+                    // reading the wallet flags only fail if unknown flags are present

ryanofsky commented at 2:04 pm on May 7, 2020:

In commit “fixup! Determine inactive HD seeds from key metadata and track them in LegacyScriptPubKeyMan” (65dee3410cd9807bb4aa6b6677c2e0832e050ea2)

Note: looks like unintended comment change here

achow101 commented at 3:59 pm on May 7, 2020:

Fixed

in src/wallet/walletdb.cpp:423 in 22361d241b outdated

418+                    std::vector<uint32_t> path;
419+                    if (keyMeta.has_key_origin) {
420+                        // We have a key origin, so pull it from it's path vector
421+                        if (keyMeta.key_origin.path.size() != 3) {
422+                            strErr = "Error reading wallet database: keymeta found with unexpected path";
423+                            return false;

ryanofsky commented at 2:16 pm on May 7, 2020:

In commit “Determine inactive HD seeds from key metadata and track them in LegacyScriptPubKeyMan” (22361d241b4d236b2b8f84b59094c296172db0ff)

Note: I initially questioned this, but it does seems reasonable to treat unexpected paths as errors. Triggering an error instead of ignoring these keys means we may need to be a little more careful about backwards compatibility in the future, and store possible paths with different lengths in a new field, but this seems like a reasonable tradeoff for getting better error checking now

in src/wallet/walletdb.cpp:424 in 65dee3410c outdated

424-                        }
425                         path = keyMeta.key_origin.path;
426-                    } else if (keyMeta.hdKeypath != "s") {
427+                    } else {
428                         // No key origin, have to parse the string
429                         // "s" is for a seed, no path info to parse so we skip those

ryanofsky commented at 2:18 pm on May 7, 2020:

In commit “fixup! Determine inactive HD seeds from key metadata and track them in LegacyScriptPubKeyMan” (65dee3410cd9807bb4aa6b6677c2e0832e050ea2)

Note: Could drop the comment about “s” here since it’s now noted and handled above

achow101 commented at 3:59 pm on May 7, 2020:

Done

in src/wallet/walletdb.cpp:440 in 22361d241b outdated

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

ryanofsky commented at 2:30 pm on May 7, 2020:

In commit “Determine inactive HD seeds from key metadata and track them in LegacyScriptPubKeyMan” (22361d241b4d236b2b8f84b59094c296172db0ff)

Should fix: It would probably be good to skip keys or error if path[1] is not 0 or 1 or path[0] is not 0

achow101 commented at 4:00 pm on May 7, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:324 in 8b8f982973 outdated

319+        CPubKey pubkey(GenerateNewKey(batch, chain, internal));
320+        AddKeypoolPubkeyWithDB(pubkey, internal, batch);
321+    }
322+    if (missing > 0) {
323+        if (internal) {
324+            WalletLogPrintf("inactive seed added %d internal keys\n", missing);

ryanofsky commented at 2:35 pm on May 7, 2020:

In commit “Generate new keys for inactive seeds after marking used” (8b8f982973a83ea77b5159235b282f20bd430677)

Note: Maybe would be good to include seed id in log message

in test/functional/wallet_hd.py:241 in 5932c00fef outdated

225+            self.nodes[0].generate(1)
226+            origin_rpc.gettransaction(txid)
227+            restore_rpc.gettransaction(txid)
228+            assert_raises_rpc_error(-5, 'Invalid or non-wallet transaction id', restore_rpc.gettransaction, out_of_kp_txid)
229+
230+            # After rescanning, restore_rpc should now see out_of_kp_txid and generate an additional key.

ryanofsky commented at 2:51 pm on May 7, 2020:

In commit “Test that keys from inactive seeds are generated” (5932c00fef5054207bd133daa1aea9857d9c3611)

Clever test! Nice way to ensure the inactive pool wasn’t topped up while still active

ryanofsky approved

ryanofsky commented at 3:06 pm on May 7, 2020: member

Code review almost-ACK 65dee3410cd9807bb4aa6b6677c2e0832e050ea2 with one or two small blocking comments below. Overall looks great!

achow101 force-pushed on May 7, 2020

in src/wallet/walletdb.cpp:441 in daa9e347de outdated

436+                        return false;
437+                    }
438+                    if (path[0] != (0 & 0x80000000)) {
439+                        strErr = "Unexpected path index for the element at index 0";
440+                    }
441+                    if (path[1] != (0 & 0x80000000) && path[1] != (1 & 0x80000000)) {

ryanofsky commented at 5:19 pm on May 7, 2020:

In commit “Determine inactive HD seeds from key metadata and track them in LegacyScriptPubKeyMan” (daa9e347deceedb354aa0c73e34e34d50fafb5d5)

Doesn’t & need to be | here (and above) (and below)? I’m actually not sure how the test seems to pass with this

ryanofsky commented at 5:25 pm on May 7, 2020:

I’m actually not sure how the test seems to pass with this

Oh, there actually are “Unexpected path index” errors in the wallet_hd.py debug logs. I guess these errors don’t prevent the wallet from being loaded like I initially thought #17681 (review), or these checks should be returning false

achow101 commented at 5:28 pm on May 7, 2020:

No, you are correct, they should be |. The lack of failure is because I forgot to return false.

ryanofsky commented at 5:21 pm on May 7, 2020: member

b79e53fd78bab2a4541512d1d015c1ad3d8ee660 looks good except for one comment. Only changes since last review were implementing suggestions

achow101 force-pushed on May 7, 2020

in src/wallet/walletdb.cpp:446 in 01c65e3f31 outdated

441+                    }
442+                    if (path[1] != (0 | 0x80000000) && path[1] != (1 | 0x80000000)) {
443+                        strErr = "Unexpected path index for the element at index 1";
444+                        return false;
445+                    }
446+                    internal = path[1] == (1 & 0x80000000);

ryanofsky commented at 5:31 pm on May 7, 2020:

In commit “Determine inactive HD seeds from key metadata and track them in LegacyScriptPubKeyMan” (01c65e3f3119108c904c19c18921efc2d934a3a3)

& should be | here too

achow101 commented at 7:05 pm on May 7, 2020:

Fixed

achow101 force-pushed on May 7, 2020

ryanofsky approved

ryanofsky commented at 8:13 pm on May 7, 2020: member

Code review ACK 769b03a83c2aa2b97f344b58dc689be26c6e08e5

Main change since last review is internal = path[1] == (1 | 0x80000000) fix fixing topup of inactive change keypools (would be nice if there’s an easy way to tweak python test to cover this case). I think this was also broken in previous versions of the PR I acked. Other change was fixing detection of invalid paths

It would be good to get this merged to make the sethdseed implementation more flexible, and to have as a prerequisite to #17484. This PR could also be a good candidate for review club.

jnewbery commented at 6:44 pm on May 11, 2020: member

We’re covering this in review club on May 13: https://bitcoincore.reviews/17681.html

jnewbery added the label Review club on May 11, 2020

in src/wallet/scriptpubkeyman.cpp:332 in 769b03a83c outdated

327+        }
328+    }
329+    return true;
330+}
331+
332+const uint32_t BIP32_HARDENED_KEY_LIMIT = 0x80000000;

jnewbery commented at 9:15 pm on May 11, 2020:

nit: since you’re moving this anyway, the normal convention is to declare constants at the top of the file. Perhaps also add a doxygen comment?

achow101 commented at 0:25 am on May 12, 2020:

Done

in src/wallet/walletdb.cpp:420 in 769b03a83c outdated

415+                bool internal = false;
416+                uint32_t index = 0;
417+                if (keyMeta.hdKeypath != "s") {
418+                    std::vector<uint32_t> path;
419+                    if (keyMeta.has_key_origin) {
420+                        // We have a key origin, so pull it from it's path vector

jnewbery commented at 9:22 pm on May 11, 2020:

grammar nit: s/it’s/its/

achow101 commented at 0:25 am on May 12, 2020:

Done

in src/wallet/walletdb.cpp:438 in 769b03a83c outdated

433+                    // k == 0 for external, 1 for internal. i is the index
434+                    if (path.size() != 3) {
435+                        strErr = "Error reading wallet database: keymeta found with unexpected path";
436+                        return false;
437+                    }
438+                    if (path[0] != (0 | 0x80000000)) {

jnewbery commented at 9:35 pm on May 11, 2020:

Personally, I think this would be clearer as (path[0] != (0x80000000)) (remove the 0 |), or even better, define a constant for BIP_32_HARDENED_BIT.

Same for check below.

achow101 commented at 0:25 am on May 12, 2020:

Done

in src/wallet/walletdb.cpp:449 in 769b03a83c outdated

440+                        return false;
441+                    }
442+                    if (path[1] != (0 | 0x80000000) && path[1] != (1 | 0x80000000)) {
443+                        strErr = "Unexpected path index for the element at index 1";
444+                        return false;
445+                    }

jnewbery commented at 9:36 pm on May 11, 2020:

add a check that path[2] & 0x80000000 == 0x80000000?

achow101 commented at 0:25 am on May 12, 2020:

Done

in src/wallet/walletdb.cpp:460 in 769b03a83c outdated

455+                    chain.nVersion = CHDChain::VERSION_HD_BASE;
456+                    // Set the seed id
457+                    chain.seed_id = keyMeta.hd_seed_id;
458+                }
459+                if (internal) {
460+                    if (chain.nVersion < CHDChain::VERSION_HD_CHAIN_SPLIT) {

jnewbery commented at 9:46 pm on May 11, 2020:

nit: Is this if statement required? Can’t you just set chain.nVersion to CHDChain::VERSION_HD_CHAIN_SPLIT every time you’re in the internal branch? If not, the following seems preferable:

0chain.nVersion = std::max(chain.nVersion, CHDChain::VERSION_HD_CHAIN_SPLIT);

to match the formatting below.

achow101 commented at 0:26 am on May 12, 2020:

I suppose the if isn’t required. Removed it.

in src/wallet/scriptpubkeyman.cpp:353 in 769b03a83c outdated

344@@ -304,6 +345,19 @@ void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
345                 WalletLogPrintf("%s: Topping up keypool failed (locked wallet)\n", __func__);
346             }
347         }
348+
349+        auto it = mapKeyMetadata.find(keyid);
350+        if (it != mapKeyMetadata.end()){
351+            CKeyMetadata meta = it->second;
352+            if (!meta.hd_seed_id.IsNull() && meta.hd_seed_id != m_hd_chain.seed_id) {
353+                bool internal = meta.key_origin.path[1] - BIP32_HARDENED_KEY_LIMIT != 0;

jnewbery commented at 10:01 pm on May 11, 2020:

nit: it seems slightly inconsistent that we’re sometimes using BIP32_HARDENED_KEY_LIMIT as a value for arithmetic operations and sometimes as a bit for bitwise operations. Functionally it’s fine, but It seems slightly clearer to me to do:

0bool internal = meta.key_origin.path[1] & ~BIP32_HARDENED_KEY_LIMIT != 0;

and the same below for index

achow101 commented at 0:26 am on May 12, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:357 in 769b03a83c outdated

352+            if (!meta.hd_seed_id.IsNull() && meta.hd_seed_id != m_hd_chain.seed_id) {
353+                bool internal = meta.key_origin.path[1] - BIP32_HARDENED_KEY_LIMIT != 0;
354+                int64_t index = meta.key_origin.path[2] - BIP32_HARDENED_KEY_LIMIT;
355+
356+                if (!TopUpInactiveHDChain(meta.hd_seed_id, index, internal)) {
357+                    WalletLogPrintf("%s: Adding inactive seed keys failed (locked wallet)\n", __func__);

jnewbery commented at 10:14 pm on May 11, 2020:

Remove “(locked wallet)”. TopUpInactiveHDChain() can return false for multiple reasons.

achow101 commented at 0:26 am on May 12, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:313 in 769b03a83c outdated

308+
309+    // "size" of the keypools. Not really the size, actually the difference between index and the chain counter
310+    // Since chain counter is 1 based and index is 0 based, one of them needs to be offset by 1.
311+    int64_t kp_size = (internal ? chain.nInternalChainCounter : chain.nExternalChainCounter) - (index + 1);
312+
313+    // count number of missing keys (internal, external)

jnewbery commented at 10:21 pm on May 11, 2020:

You’ve copied this comment from TopUp(), but it doesn’t make sense in this context, because you’re not topping up both internal and external keypools here.

achow101 commented at 0:26 am on May 12, 2020:

Removed

in src/wallet/scriptpubkeyman.cpp:320 in 769b03a83c outdated

315+    int64_t missing = std::max(std::max((int64_t) target_size, (int64_t) 1) - kp_size, (int64_t) 0);
316+
317+    WalletBatch batch(m_storage.GetDatabase());
318+    for (int64_t i = missing; i > 0; --i) {
319+        CPubKey pubkey(GenerateNewKey(batch, chain, internal));
320+        AddKeypoolPubkeyWithDB(pubkey, internal, batch);

jnewbery commented at 10:25 pm on May 11, 2020:

This doesn’t seem right. It adds the key to setInternalKeyPool or setExternalKeyPool, but I don’t think we want to do that?

achow101 commented at 0:26 am on May 12, 2020:

Removed.

ryanofsky commented at 4:19 pm on May 13, 2020:

This doesn’t seem right. It adds the key to setInternalKeyPool or setExternalKeyPool, but I don’t think we want to do that?

To clarify, this wasn’t just wasteful, but also a potentially dangerous bug right? LIke it could have caused new wallet addresses to be generated with the previous hd seed, and possibly stolen funds if money was sent to those addresses and the previous seed was unencrypted or shared with another wallet?

jnewbery commented at 10:27 pm on May 11, 2020: member

Mostly looks good. I’ve left a bunch of nits and one question which confused me (about calling AddKeypoolPubkeyWithDB() in TopUpInactiveHDChain()). I haven’t reviewed the tests yet.

achow101 force-pushed on May 12, 2020

in src/wallet/scriptpubkeyman.cpp:352 in 2feb5a2d04 outdated

343@@ -304,6 +344,19 @@ void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
344                 WalletLogPrintf("%s: Topping up keypool failed (locked wallet)\n", __func__);
345             }
346         }
347+
348+        auto it = mapKeyMetadata.find(keyid);
349+        if (it != mapKeyMetadata.end()){
350+            CKeyMetadata meta = it->second;
351+            if (!meta.hd_seed_id.IsNull() && meta.hd_seed_id != m_hd_chain.seed_id) {
352+                bool internal = meta.key_origin.path[1] & ~BIP32_HARDENED_KEY_LIMIT != 0;

jonatack commented at 3:11 pm on May 12, 2020:

This line is causing a -Wparentheses build warning suggest parentheses around comparison in operand of ‘&’

achow101 commented at 4:28 pm on May 12, 2020:

Done

in src/wallet/walletdb.cpp:793 in 2feb5a2d04 outdated

785@@ -728,6 +786,20 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
786         result = DBErrors::CORRUPT;
787     }
788 
789+    // Set the inactive chain
790+    if (wss.m_hd_chains.size() > 0) {
791+        LegacyScriptPubKeyMan* legacy_spkm = pwallet->GetLegacyScriptPubKeyMan();
792+        if (!legacy_spkm) {
793+            pwallet->WalletLogPrintf("Inactive HD Chains found but no Legacy ScriptPubKeyMan");

jonatack commented at 3:12 pm on May 12, 2020:

This line is making test/lint/lint-logs.sh unhappy, appeased with:

0-            pwallet->WalletLogPrintf("Inactive HD Chains found but no Legacy ScriptPubKeyMan");
1+            pwallet->WalletLogPrintf("Inactive HD Chains found but no Legacy ScriptPubKeyMan\n");

achow101 commented at 4:28 pm on May 12, 2020:

Done

achow101 force-pushed on May 12, 2020

in src/wallet/scriptpubkeyman.cpp:319 in a5c3f3218b outdated

318+
319+    WalletBatch batch(m_storage.GetDatabase());
320+    for (int64_t i = missing; i > 0; --i) {
321+        CPubKey pubkey(GenerateNewKey(batch, chain, internal));
322+    }
323+    if (missing > 0) {

jonatack commented at 5:16 pm on May 12, 2020:

f5eaa903 I think we can tuck this object and loop inside the conditional, to skip it if missing is zero.

 0-    WalletBatch batch(m_storage.GetDatabase());
 1-    for (int64_t i = missing; i > 0; --i) {
 2-        GenerateNewKey(batch, chain, internal);
 3-    }
 4     if (missing > 0) {
 5+        WalletBatch batch(m_storage.GetDatabase());
 6+        for (int64_t i = missing; i > 0; --i) {
 7+            GenerateNewKey(batch, chain, internal);
 8+        }
 9         if (internal) {
10             WalletLogPrintf("inactive seed with id %s added %d internal keys\n", HexStr(seed_id), missing);
11         } else {

achow101 commented at 10:00 pm on May 15, 2020:

Done

achow101 force-pushed on May 12, 2020

in src/wallet/scriptpubkeyman.h:344 in 218c4f6405 outdated

339+     *
340+     * @param seed_id the CKeyID for the HD seed.
341+     * @param index the index to start generating keys from
342+     * @param internal whether the internal chain should be used. true for internal chain, false for external chain.
343+     *
344+     * @@return true if seed was found and keys were derived. false if unable to derive seeds

jonatack commented at 7:10 pm on May 12, 2020:

f5eaa903 s/@@return/@return/

achow101 commented at 10:00 pm on May 15, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:317 in 218c4f6405 outdated

312+    // "size" of the keypools. Not really the size, actually the difference between index and the chain counter
313+    // Since chain counter is 1 based and index is 0 based, one of them needs to be offset by 1.
314+    int64_t kp_size = (internal ? chain.nInternalChainCounter : chain.nExternalChainCounter) - (index + 1);
315+
316+    // make sure the keypool fits the user-selected target (-keypool)
317+    int64_t missing = std::max(std::max((int64_t) target_size, (int64_t) 1) - kp_size, (int64_t) 0);

jonatack commented at 7:33 pm on May 12, 2020:

f5eaa90 suggestion:

 0@@ -307,14 +307,14 @@ bool LegacyScriptPubKeyMan::TopUpInactiveHDChain(const CKeyID seed_id, int64_t i
 1     CHDChain& chain = it->second;
 2 
 3     // Top up key pool
 4-    int64_t target_size = std::max(gArgs.GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 0);
 5+    int64_t target_size = std::max(gArgs.GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 1);
 6 
 7     // "size" of the keypools. Not really the size, actually the difference between index and the chain counter
 8     // Since chain counter is 1 based and index is 0 based, one of them needs to be offset by 1.
 9     int64_t kp_size = (internal ? chain.nInternalChainCounter : chain.nExternalChainCounter) - (index + 1);
10 
11     // make sure the keypool fits the user-selected target (-keypool)
12-    int64_t missing = std::max(std::max((int64_t) target_size, (int64_t) 1) - kp_size, (int64_t) 0);
13+    int64_t missing = std::max(target_size - kp_size, (int64_t) 0);

fjahr commented at 1:47 pm on May 13, 2020:

Fallback to 1 can be done where target_size is defined initially so it can just be removed in this line.

jonatack commented at 4:50 pm on May 13, 2020:

Agreed; updated the suggestion.

achow101 commented at 10:00 pm on May 15, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:347 in 218c4f6405 outdated

343@@ -304,6 +344,19 @@ void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
344                 WalletLogPrintf("%s: Topping up keypool failed (locked wallet)\n", __func__);
345             }
346         }
347+

jonatack commented at 7:38 pm on May 12, 2020:

f5eaa90 nit line 341 if you feel like touching it up:

0-            WalletLogPrintf("%s: Detected a used keypool key, mark all keypool key up to this key as used\n", __func__);
1+            WalletLogPrintf("%s: Detected a used keypool key, mark all keypool keys up to this key as used\n", __func__);

achow101 commented at 10:00 pm on May 15, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:350 in 218c4f6405 outdated

343@@ -304,6 +344,19 @@ void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
344                 WalletLogPrintf("%s: Topping up keypool failed (locked wallet)\n", __func__);
345             }
346         }
347+
348+        auto it = mapKeyMetadata.find(keyid);

jonatack commented at 7:42 pm on May 12, 2020:

f5eaa90 nit: I reckon this new code section could use a comment.

achow101 commented at 10:00 pm on May 15, 2020:

Done

in src/wallet/walletdb.cpp:451 in 218c4f6405 outdated

442+                    if (path[1] != 0x80000000 && path[1] != (1 | 0x80000000) && (path[2] & 0x80000000)) {
443+                        strErr = "Unexpected path index for the element at index 1";
444+                        return false;
445+                    }
446+                    internal = path[1] == (1 | 0x80000000);
447+                    index = path[2] & ~0x80000000;

jonatack commented at 7:46 pm on May 12, 2020:

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)

achow101 commented at 9:14 pm on May 15, 2020:

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.

jonatack commented at 7:59 pm on May 12, 2020: member

ACK 218c4f6405df2b389411f8370057319bfdc59f48

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

in src/wallet/scriptpubkeyman.cpp:310 in f5eaa90301 outdated

305+    }
306+
307+    CHDChain& chain = it->second;
308+
309+    // Top up key pool
310+    int64_t target_size = std::max(gArgs.GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 0);

fjahr commented at 1:45 pm on May 13, 2020:

I think target_size can fall back to 1 here so it doesn’t have to be done below.

0    int64_t target_size = std::max(gArgs.GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 1);

achow101 commented at 10:00 pm on May 15, 2020:

Done

fjahr approved

fjahr commented at 2:35 pm on May 13, 2020: member

Code review ACK 218c4f6405df2b389411f8370057319bfdc59f48

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

theStack commented at 4:27 pm on May 13, 2020: member

Concept ACK

in src/wallet/walletdb.cpp:449 in 9e33fa1b9c outdated

440+                        return false;
441+                    }
442+                    if (path[1] != 0x80000000 && path[1] != (1 | 0x80000000) && (path[2] & 0x80000000)) {
443+                        strErr = "Unexpected path index for the element at index 1";
444+                        return false;
445+                    }

jkczyz commented at 4:33 pm on May 13, 2020:

Would it be useful to include the unexpected path in strErr?

achow101 commented at 10:01 pm on May 15, 2020:

Done

in src/wallet/walletdb.cpp:456 in 9e33fa1b9c outdated

451+                auto ins = wss.m_hd_chains.emplace(keyMeta.hd_seed_id, CHDChain());
452+                CHDChain& chain = ins.first->second;
453+                if (ins.second) {
454+                    // For new chains, we want to default to VERSION_HD_BASE until we see an internal
455+                    chain.nVersion = CHDChain::VERSION_HD_BASE;
456+                    // Set the seed id

jkczyz commented at 4:37 pm on May 13, 2020:

This comment doesn’t seem to add any value. Remove?

achow101 commented at 10:01 pm on May 15, 2020:

Done

jkczyz commented at 4:47 pm on May 13, 2020: contributor

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?

in src/wallet/scriptpubkeyman.cpp:913 in 218c4f6405 outdated

894@@ -842,7 +895,14 @@ void LegacyScriptPubKeyMan::SetHDChain(const CHDChain& chain, bool memonly)
895     if (!memonly && !WalletBatch(m_storage.GetDatabase()).WriteHDChain(chain))
896         throw std::runtime_error(std::string(__func__) + ": writing chain failed");
897 
898-    hdChain = chain;
899+    m_hd_chain = chain;
900+}
901+
902+void LegacyScriptPubKeyMan::AddInactiveHDChain(const CHDChain& chain)

jonatack commented at 6:18 pm on May 13, 2020:

Per today’s review club session, should AddInactiveHDChain be called not only from LoadWallet but also from sethdseed (or elsewhere), and tests updated to check for that case.

achow101 commented at 10:01 pm on May 15, 2020:

Added a call to AddInactiveHDChain to SetHDCHain which is where chain rotation occurs.

Also updated the tests with a third wallet (restore2) which is not reloaded. The things that are checked with restore are also checked in restore2. I’ve checked that this test fails without the fix.

rajarshimaitra commented at 2:56 pm on May 15, 2020: contributor

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

in src/wallet/scriptpubkeyman.cpp:895 in 218c4f6405 outdated

894@@ -842,7 +895,14 @@ void LegacyScriptPubKeyMan::SetHDChain(const CHDChain& chain, bool memonly)
895     if (!memonly && !WalletBatch(m_storage.GetDatabase()).WriteHDChain(chain))

rajarshimaitra commented at 3:07 pm on May 15, 2020:

As per the review club discussion, the intended behavior is only effective when the wallet is reloaded. As SetHdChain is writing the chains to the disk, CWalletScanState can only observe the presence of the old chains if it reads back from the disk. This means after setting the seed user must reload the wallet to track inactive chains which might not be readily obvious. could SetHdChain detect that a previous chain is being replaced and correspondingly call AddInactiveHDChain before writing the new one to the disk?

achow101 commented at 10:01 pm on May 15, 2020:

See #17681 (review)

Determine inactive HD seeds from key metadata and track them in LegacyScriptPubKeyMan 45f2f6a0e8

Generate new keys for inactive seeds after marking used

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

c93082ece4

Test that keys from inactive seeds are generated b1810a145a

Remove IBD check in sethdseed

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.

1ed52fbb4d

achow101 force-pushed on May 15, 2020

ryanofsky approved

ryanofsky commented at 8:39 pm on May 19, 2020: member

Code review ACK 1ed52fbb4d81f7b7634fd4fb6d1d00e1478129dc. 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

in src/wallet/walletdb.h:120 in 1ed52fbb4d

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

jnewbery commented at 9:08 pm on May 19, 2020:

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.

in src/wallet/scriptpubkeyman.cpp:1083 in 1ed52fbb4d

1087-    metadata.hd_seed_id = hdChain.seed_id;
1088+    metadata.hd_seed_id = hd_chain.seed_id;
1089     CKeyID master_id = masterKey.key.GetPubKey().GetID();
1090     std::copy(master_id.begin(), master_id.begin() + 4, metadata.key_origin.fingerprint);
1091     metadata.has_key_origin = true;
1092     // update the chain model in the database

jnewbery commented at 10:10 pm on May 19, 2020:

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

in src/wallet/scriptpubkeyman.cpp:897 in 1ed52fbb4d

893@@ -839,10 +894,27 @@ bool LegacyScriptPubKeyMan::AddWatchOnly(const CScript& dest, int64_t nCreateTim
894 void LegacyScriptPubKeyMan::SetHDChain(const CHDChain& chain, bool memonly)
895 {
896     LOCK(cs_KeyStore);
897-    if (!memonly && !WalletBatch(m_storage.GetDatabase()).WriteHDChain(chain))
898-        throw std::runtime_error(std::string(__func__) + ": writing chain failed");
899+    // memonly == true means we are loading the wallet file

jnewbery commented at 10:19 pm on May 19, 2020:

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.

ryanofsky commented at 11:35 pm on May 19, 2020:

re: #17681 (review)

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

jnewbery commented at 0:29 am on May 20, 2020:

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.

achow101 commented at 6:26 pm on May 20, 2020:

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.

in src/wallet/scriptpubkeyman.cpp:348 in 1ed52fbb4d

344             if (!TopUp()) {
345                 WalletLogPrintf("%s: Topping up keypool failed (locked wallet)\n", __func__);
346             }
347         }
348+
349+        // 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.

jnewbery commented at 10:22 pm on May 19, 2020:

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

in src/wallet/scriptpubkeyman.cpp:298 in 1ed52fbb4d

292@@ -290,20 +293,72 @@ bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool i
293     return true;
294 }
295 
296+bool LegacyScriptPubKeyMan::TopUpInactiveHDChain(const CKeyID seed_id, int64_t index, bool internal)
297+{
298+    LOCK(cs_KeyStore);

jnewbery commented at 10:39 pm on May 19, 2020:

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.

in src/wallet/scriptpubkeyman.h:158 in 1ed52fbb4d

153+    size_t operator()(const CKeyID& id) const
154+    {
155+        return id.GetUint64(0);
156+    }
157+};
158+

jnewbery commented at 10:56 pm on May 19, 2020:

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:

0struct KeyIDHasher
1{
2    size_t operator()(const CKeyID& id) const { return id.GetUint64(0);}
3};

in src/wallet/scriptpubkeyman.cpp:843 in 45f2f6a0e8 outdated

838@@ -839,12 +839,29 @@ bool LegacyScriptPubKeyMan::AddWatchOnly(const CScript& dest, int64_t nCreateTim
839 void LegacyScriptPubKeyMan::SetHDChain(const CHDChain& chain, bool memonly)
840 {
841     LOCK(cs_KeyStore);
842-    if (!memonly && !WalletBatch(m_storage.GetDatabase()).WriteHDChain(chain))
843-        throw std::runtime_error(std::string(__func__) + ": writing chain failed");
844+    // memonly == true means we are loading the wallet file
845+    // memonly == false means that the chain is actually being changed

ariard commented at 11:10 pm on May 19, 2020:

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.

achow101 commented at 6:27 pm on May 20, 2020:

see #17681 (review)

in src/wallet/walletdb.cpp:433 in 45f2f6a0e8 outdated

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

ariard commented at 0:06 am on May 20, 2020:

45f2f6a

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

achow101 commented at 6:29 pm on May 20, 2020:

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.

in src/wallet/scriptpubkeyman.cpp:310 in c93082ece4 outdated

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

ariard commented at 0:56 am on May 20, 2020:

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.

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

Feel free to ignore, it’s really IMO.

in src/wallet/scriptpubkeyman.h:344 in c93082ece4 outdated

339+     *
340+     * @param seed_id the CKeyID for the HD seed.
341+     * @param index the index to start generating keys from
342+     * @param internal whether the internal chain should be used. true for internal chain, false for external chain.
343+     *
344+     * @return true if seed was found and keys were derived. false if unable to derive seeds

ariard commented at 0:59 am on May 20, 2020:

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 ?

ariard commented at 1:00 am on May 20, 2020: member

Code Review ACK 1ed52fb

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

jonatack commented at 4:44 am on May 20, 2020: member

ACK 1ed52fbb4d81f7 thanks for addressing the previous review feedback; would be happy to see the new review questions answered and feedback addressed and re-ack.

  0diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
  1index 7a62752b12..1bd33449fc 100644
  2--- a/src/wallet/scriptpubkeyman.cpp
  3+++ b/src/wallet/scriptpubkeyman.cpp
  4@@ -307,20 +307,20 @@ bool LegacyScriptPubKeyMan::TopUpInactiveHDChain(const CKeyID seed_id, int64_t i
  5     CHDChain& chain = it->second;
  6 
  7     // Top up key pool
  8-    int64_t target_size = std::max(gArgs.GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 0);
  9+    int64_t target_size = std::max(gArgs.GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 1);
 10 
 11     // "size" of the keypools. Not really the size, actually the difference between index and the chain counter
 12     // Since chain counter is 1 based and index is 0 based, one of them needs to be offset by 1.
 13     int64_t kp_size = (internal ? chain.nInternalChainCounter : chain.nExternalChainCounter) - (index + 1);
 14 
 15     // make sure the keypool fits the user-selected target (-keypool)
 16-    int64_t missing = std::max(std::max((int64_t) target_size, (int64_t) 1) - kp_size, (int64_t) 0);
 17+    int64_t missing = std::max(target_size - kp_size, (int64_t) 0);
 18 
 19-    WalletBatch batch(m_storage.GetDatabase());
 20-    for (int64_t i = missing; i > 0; --i) {
 21-        GenerateNewKey(batch, chain, internal);
 22-    }
 23     if (missing > 0) {
 24+        WalletBatch batch(m_storage.GetDatabase());
 25+        for (int64_t i = missing; i > 0; --i) {
 26+            GenerateNewKey(batch, chain, internal);
 27+        }
 28         if (internal) {
 29             WalletLogPrintf("inactive seed with id %s added %d internal keys\n", HexStr(seed_id), missing);
 30         } else {
 31@@ -337,7 +337,7 @@ void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
 32     for (const auto& keyid : GetAffectedKeys(script, *this)) {
 33         std::map<CKeyID, int64_t>::const_iterator mi = m_pool_key_to_index.find(keyid);
 34         if (mi != m_pool_key_to_index.end()) {
 35-            WalletLogPrintf("%s: Detected a used keypool key, mark all keypool key up to this key as used\n", __func__);
 36+            WalletLogPrintf("%s: Detected a used keypool key, mark all keypool keys up to this key as used\n", __func__);
 37             MarkReserveKeysAsUsed(mi->second);
 38 
 39             if (!TopUp()) {
 40@@ -345,6 +345,8 @@ void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
 41             }
 42         }
 43 
 44+        // 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.
 45+        // If so, TopUp the inactive hd chain
 46         auto it = mapKeyMetadata.find(keyid);
 47         if (it != mapKeyMetadata.end()){
 48             CKeyMetadata meta = it->second;
 49@@ -892,8 +894,18 @@ bool LegacyScriptPubKeyMan::AddWatchOnly(const CScript& dest, int64_t nCreateTim
 50 void LegacyScriptPubKeyMan::SetHDChain(const CHDChain& chain, bool memonly)
 51 {
 52     LOCK(cs_KeyStore);
 53-    if (!memonly && !WalletBatch(m_storage.GetDatabase()).WriteHDChain(chain))
 54-        throw std::runtime_error(std::string(__func__) + ": writing chain failed");
 55+    // memonly == true means we are loading the wallet file
 56+    // memonly == false means that the chain is actually being changed
 57+    if (!memonly) {
 58+        // Store the new chain
 59+        if (!WalletBatch(m_storage.GetDatabase()).WriteHDChain(chain)) {
 60+            throw std::runtime_error(std::string(__func__) + ": writing chain failed");
 61+        }
 62+        // When there's an old chain, add it as an inactive chain as we are now rotating hd chains
 63+        if (!m_hd_chain.seed_id.IsNull()) {
 64+            AddInactiveHDChain(m_hd_chain);
 65+        }
 66+    }
 67 
 68     m_hd_chain = chain;
 69 }
 70diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
 71index 817bdceb87..5aaa17334c 100644
 72--- a/src/wallet/scriptpubkeyman.h
 73+++ b/src/wallet/scriptpubkeyman.h
 74@@ -341,7 +341,7 @@ private:
 75      * [@param](/bitcoin-bitcoin/contributor/param/) index the index to start generating keys from
 76      * [@param](/bitcoin-bitcoin/contributor/param/) internal whether the internal chain should be used. true for internal chain, false for external chain.
 77      *
 78-     * @@return true if seed was found and keys were derived. false if unable to derive seeds
 79+     * [@return](/bitcoin-bitcoin/contributor/return/) true if seed was found and keys were derived. false if unable to derive seeds
 80      */
 81     bool TopUpInactiveHDChain(const CKeyID seed_id, int64_t index, bool internal);
 82 
 83diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
 84index fe94ca59ba..49db7914e4 100644
 85--- a/src/wallet/walletdb.cpp
 86+++ b/src/wallet/walletdb.cpp
 87@@ -436,11 +436,15 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
 88                         return false;
 89                     }
 90                     if (path[0] != 0x80000000) {
 91-                        strErr = "Unexpected path index for the element at index 0";
 92+                        strErr = strprintf("Unexpected path index of 0x%08x (expected 0x80000000) for the element at index 0", path[0]);
 93                         return false;
 94                     }
 95-                    if (path[1] != 0x80000000 && path[1] != (1 | 0x80000000) && (path[2] & 0x80000000)) {
 96-                        strErr = "Unexpected path index for the element at index 1";
 97+                    if (path[1] != 0x80000000 && path[1] != (1 | 0x80000000)) {
 98+                        strErr = strprintf("Unexpected path index of 0x%08x (expected 0x80000000 or 0x80000001) for the element at index 1", path[1]);
 99+                        return false;
100+                    }
101+                    if ((path[2] & 0x80000000) == 0) {
102+                        strErr = strprintf("Unexpected path index of 0x%08x (expected to be greater than or equal to 0x80000000)", path[2]);
103                         return false;
104                     }
105                     internal = path[1] == (1 | 0x80000000);
106@@ -453,7 +457,6 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
107                 if (ins.second) {
108                     // For new chains, we want to default to VERSION_HD_BASE until we see an internal
109                     chain.nVersion = CHDChain::VERSION_HD_BASE;
110-                    // Set the seed id
111                     chain.seed_id = keyMeta.hd_seed_id;
112                 }
113                 if (internal) {
114diff --git a/test/functional/wallet_hd.py b/test/functional/wallet_hd.py
115index 6d0ca680e2..5b083a5398 100755
116--- a/test/functional/wallet_hd.py
117+++ b/test/functional/wallet_hd.py
118@@ -188,7 +188,12 @@ class WalletHDTest(BitcoinTestFramework):
119             restore_rpc.sethdseed(True, seed) # Set to be the same seed as origin_rpc
120             restore_rpc.sethdseed(True) # Rotate to a new seed, making original `seed` inactive
121 
122-            # Check persistence of inactive seed
123+            self.nodes[1].createwallet(wallet_name='restore2', blank=True)
124+            restore2_rpc = self.nodes[1].get_wallet_rpc('restore2')
125+            restore2_rpc.sethdseed(True, seed) # Set to be the same seed as origin_rpc
126+            restore2_rpc.sethdseed(True) # Rotate to a new seed, making original `seed` inactive
127+
128+            # Check persistence of inactive seed by reloading restore. restore2 is still loaded to test the case where the wallet is not reloaded
129             restore_rpc.unloadwallet()
130             self.nodes[1].loadwallet('restore')
131             restore_rpc = self.nodes[1].get_wallet_rpc('restore')
132@@ -204,6 +209,10 @@ class WalletHDTest(BitcoinTestFramework):
133             assert_equal(info['ismine'], True)
134             info = restore_rpc.getaddressinfo(addr)
135             assert_equal(info['ismine'], False)
136+            info = restore2_rpc.getaddressinfo(last_addr)
137+            assert_equal(info['ismine'], True)
138+            info = restore2_rpc.getaddressinfo(addr)
139+            assert_equal(info['ismine'], False)
140             # Check that the origin seed has addr
141             info = origin_rpc.getaddressinfo(addr)
142             assert_equal(info['ismine'], True)
143@@ -226,6 +235,8 @@ class WalletHDTest(BitcoinTestFramework):
144             origin_rpc.gettransaction(txid)
145             restore_rpc.gettransaction(txid)
146             assert_raises_rpc_error(-5, 'Invalid or non-wallet transaction id', restore_rpc.gettransaction, out_of_kp_txid)
147+            restore2_rpc.gettransaction(txid)
148+            assert_raises_rpc_error(-5, 'Invalid or non-wallet transaction id', restore2_rpc.gettransaction, out_of_kp_txid)
149 
150             # After rescanning, restore_rpc should now see out_of_kp_txid and generate an additional key.
151             # addr should now be part of restore_rpc and be ismine
152@@ -233,6 +244,10 @@ class WalletHDTest(BitcoinTestFramework):
153             restore_rpc.gettransaction(out_of_kp_txid)
154             info = restore_rpc.getaddressinfo(addr)
155             assert_equal(info['ismine'], True)
156+            restore2_rpc.rescanblockchain()
157+            restore2_rpc.gettransaction(out_of_kp_txid)
158+            info = restore2_rpc.getaddressinfo(addr)
159+            assert_equal(info['ismine'], True)
160 
161             # Check again that 3 keys were derived.
162             # Empty keypool and get an address that is beyond the initial keypool
163@@ -246,6 +261,10 @@ class WalletHDTest(BitcoinTestFramework):
164             assert_equal(info['ismine'], True)
165             info = restore_rpc.getaddressinfo(addr)
166             assert_equal(info['ismine'], False)
167+            info = restore2_rpc.getaddressinfo(last_addr)
168+            assert_equal(info['ismine'], True)
169+            info = restore2_rpc.getaddressinfo(addr)
170+            assert_equal(info['ismine'], False)
171 
172 if __name__ == '__main__':
173     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 commented at 6:42 pm on May 20, 2020: member

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

meshcollider merged this on May 22, 2020

meshcollider closed this on May 22, 2020

sidhujag referenced this in commit 22b0c45607 on May 22, 2020

meshcollider referenced this in commit 89899a3448 on Jul 11, 2020

sidhujag referenced this in commit 51b42c40d7 on Jul 11, 2020

jasonbcox referenced this in commit 9df09227a2 on Oct 10, 2020

sidhujag referenced this in commit 93a20c3437 on Nov 10, 2020

Fabcien referenced this in commit 3e3efbd77f on Feb 10, 2021

MarcoFalke locked this on Feb 15, 2022

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

Conflicts