wallet: Derive inactive HD chains in additional places

achow101 commented at 8:43 pm on October 18, 2021: member

Currently inactive HD chains are only derived from at the time a key in that chain is found to have been used. However, at that time, the wallet may not be able to derive keys (e.g. it is locked). Currently we would just move on and not derive any new keys, however this could result in missing funds.

This PR resolves this problem by adding memory only variables to CHDChain which track the highest known index. TopUp is modified to always try to top up the inactive HD chains, and this process will use the new variables to determine how much to top up. In this way, after an encrypted wallet is unlocked, the inactive HD chains will be topped up and hopefully funds will not be missed.

Note that because these variables are not persisted to disk (because CHDChains for inactive HD chains are not written to disk), if an encrypted wallet is not unlocked in the same session as a key from an inactive chain is found to be used, then it will not be topped up later unless more keys are found.

Additionally, wallets which do not have upgraded key metadata will not derive any keys from inactive HD chains. This is resolved by using the derivation path string in CKeyMetadata.hdKeypath to determine what indexes to derive.

achow101 force-pushed on Oct 18, 2021

DrahtBot added the label Wallet on Oct 18, 2021

DrahtBot commented at 8:30 am on October 19, 2021: 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:

#23829 (refactor: use braced init for integer literals instead of c style casts by PastaPastaPasta)

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.

achow101 force-pushed on Nov 3, 2021

achow101 marked this as ready for review on Nov 3, 2021

achow101 commented at 1:15 pm on November 3, 2021: member

Ready for review, supersedes #23277

laanwj commented at 11:17 am on November 29, 2021: member

Concept ACK, thanks for adding a test!

Add size check on meta.key_origin.path

Resolves segfault on legacy wallet

Log warning when meta.key_origin.path is below expected size

0652ee73ec

wallet: Parse hdKeypath if key_origin is not available

When topping up an inactive HD chain, either key_origin will be
available and we can use the path given there, or we need to figure out
the path from the string hdKeypath.

961b9e4e40

wallet: Properly set hd chain counters when loading

When build CHDChains out of CKeyMetadata, the chain counters are
actually 1 based, not 0 based, so 1 must be added to each index.

70134eb34f

achow101 commented at 4:27 pm on December 8, 2021: member

Rebased for hidden merge conflict.

achow101 force-pushed on Dec 8, 2021

in src/wallet/scriptpubkeyman.cpp:1285 in 38c949ae1b outdated

1293+    // Top up key pool
1294+    unsigned int nTargetSize;
1295+    if (kpSize > 0)
1296+        nTargetSize = kpSize;
1297+    else
1298+        nTargetSize = std::max(gArgs.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 0);

PastaPastaPasta commented at 1:10 am on January 14, 2022:

Since you’re touching this, please make it braces or same line to be inline with the style guide

achow101 commented at 8:03 pm on January 14, 2022:

Done

in src/wallet/scriptpubkeyman.cpp:1286 in 38c949ae1b outdated

1294+    unsigned int nTargetSize;
1295+    if (kpSize > 0)
1296+        nTargetSize = kpSize;
1297+    else
1298+        nTargetSize = std::max(gArgs.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 0);
1299+    int64_t target = std::max((int64_t) nTargetSize, (int64_t) 1);

PastaPastaPasta commented at 1:10 am on January 14, 2022:

please make this not use c-style cast

0int64_t{1}

achow101 commented at 8:03 pm on January 14, 2022:

Done

in src/wallet/scriptpubkeyman.cpp:1301 in 38c949ae1b outdated

1313-        if (!IsHDEnabled() || !m_storage.CanSupportFeature(FEATURE_HD_SPLIT))
1314-        {
1315-            // don't create extra internal keys
1316-            missingInternal = 0;
1317+    if (!IsHDEnabled() || !m_storage.CanSupportFeature(FEATURE_HD_SPLIT))
1318+    {

PastaPastaPasta commented at 1:11 am on January 14, 2022:

same line

0    if (!IsHDEnabled() || !m_storage.CanSupportFeature(FEATURE_HD_SPLIT)) {

achow101 commented at 8:03 pm on January 14, 2022:

Done

in src/wallet/scriptpubkeyman.cpp:1308 in 38c949ae1b outdated

1320+        missingInternal = 0;
1321+    }
1322+    bool internal = false;
1323+    WalletBatch batch(m_storage.GetDatabase());
1324+    for (int64_t i = missingInternal + missingExternal; i--;)
1325+    {

PastaPastaPasta commented at 1:11 am on January 14, 2022:

0    for (int64_t i = missingInternal + missingExternal; i--;) {

achow101 commented at 8:03 pm on January 14, 2022:

Done

in src/wallet/scriptpubkeyman.h:357 in 38c949ae1b outdated

353@@ -354,6 +354,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
354      */
355     bool TopUpInactiveHDChain(const CKeyID seed_id, int64_t index, bool internal);
356 
357+    bool TopUpChain(CHDChain& chain ,unsigned int size);

PastaPastaPasta commented at 1:12 am on January 14, 2022:

0    bool TopUpChain(CHDChain& chain, unsigned int size);

achow101 commented at 8:03 pm on January 14, 2022:

Done

PastaPastaPasta changes_requested

PastaPastaPasta commented at 1:14 am on January 14, 2022: contributor

some formatting suggestions (can also just run clang-diff-format.py)

achow101 force-pushed on Jan 14, 2022

wallet: Refactor TopUp to be able to top up inactive chains too

Refactors TopUp so that it also tops up inactive chains. The bulk of
TopUp is moved to TopUpChain.

CHDChain also has 2 new in memory variables to track its highest used
indexes. This is used only for inactive hd chains so that they can be
topped up later in the same session (e.g. if the wallet is encrypted and
not unlocked at the time of MarkUnusedAddresses).

8077862c5e

achow101 force-pushed on Jan 14, 2022

in src/wallet/scriptpubkeyman.cpp:1287 in f3b02bd316 outdated

1299+    if (kpSize > 0) {
1300+        nTargetSize = kpSize;
1301+    } else {
1302+        nTargetSize = std::max(gArgs.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), int64_t{0});
1303+    }
1304+    int64_t target = std::max((int64_t) nTargetSize, int64_t{1});

PastaPastaPasta commented at 4:50 am on January 28, 2022:

avoid c-style casts

0    int64_t target = std::max(int64_t(nTargetSize), int64_t{1});

in src/wallet/scriptpubkeyman.cpp:1295 in f3b02bd316 outdated

1307+    // make sure the keypool of external and internal keys fits the user selected target (-keypool)
1308+    int64_t missingExternal;
1309+    int64_t missingInternal;
1310+    if (chain == m_hd_chain) {
1311+        missingExternal = std::max(target - (int64_t)setExternalKeyPool.size(), int64_t{0});
1312+        missingInternal = std::max(target - (int64_t)setInternalKeyPool.size(), int64_t{0});

PastaPastaPasta commented at 4:50 am on January 28, 2022:

0        missingExternal = std::max(target - int64_t(setExternalKeyPool.size()), int64_t{0});
1        missingInternal = std::max(target - int64_t(setInternalKeyPool.size()), int64_t{0});

achow101 commented at 5:30 pm on February 17, 2022: member

Do we want this for 23.0? It fixes a segfault that a number of people have run into.

MarcoFalke renamed this:
~~wallet: Derive inactive HD chains in addtional places~~
wallet: Derive inactive HD chains in additional places
on Feb 17, 2022

MarcoFalke added this to the milestone 23.0 on Feb 17, 2022

fanquake requested review from instagibbs on Feb 21, 2022

fanquake requested review from meshcollider on Feb 21, 2022

in test/functional/wallet_inactive_hdchains.py:54 in f3b02bd316 outdated

49+        test_wallet.sethdseed(True, seed.privkey)
50+
51+        # Generate a new HD seed on the test wallet
52+        test_wallet.sethdseed()
53+
54+        # Get the address at index 6

instagibbs commented at 2:10 am on February 22, 2022:

where are these magic numbers coming from?

achow101 commented at 7:39 pm on February 22, 2022:

Not entirely sure, but probably something to do with the keypool defaults. However these don’t make sense with the keypool options we are using in the test, so I’ve changed this to match those.

in test/functional/wallet_inactive_hdchains.py:91 in f3b02bd316 outdated

114+        assert not test_wallet.getaddressinfo(addr2)["ismine"]
115+        test_wallet.walletpassphrase("pass", 1)
116+
117+        # The test wallet should now know about the second address as it
118+        # should have generated it in the inactive chain's keypool
119+        assert test_wallet.getaddressinfo(addr2)["ismine"]

instagibbs commented at 2:20 am on February 22, 2022:

seems kind of racey, do a while not true loop?

achow101 commented at 7:39 pm on February 22, 2022:

Done, but with gettransaction.

in test/functional/wallet_inactive_hdchains.py:116 in f3b02bd316 outdated

129+        # https://github.com/bitcoin/bitcoin/issues/21605
130+        self.log.info("Test that topping up inactive HD chains does not need upgraded key origin")
131+        default = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
132+
133+        self.nodes[0].createwallet(wallet_name="keymeta_base", descriptors=False, blank=True)
134+        self.nodes[1].createwallet_passthrough(wallet_name="keymeta_test")

instagibbs commented at 2:22 am on February 22, 2022:

I assume this is important, probably needs a comment on what this is doing because I don’t understand it

achow101 commented at 7:40 pm on February 22, 2022:

Added a comment.

It’s because we override createwallet in the test framework so that passing --descriptors will autofill the descriptor parameter. But that doesn’t work for old nodes, so we need a passthrough function.

achow101 force-pushed on Feb 22, 2022

tests: Tests for inactive HD chains

test cases are added for inactive HD chains: a basic case, a case
where the wallet is encrypted, and a case for the 21605 segfault.

c4d76c6faa

achow101 force-pushed on Feb 22, 2022

fanquake removed review request from instagibbs on Feb 23, 2022

fanquake requested review from sipa on Feb 23, 2022

laanwj commented at 12:21 pm on February 28, 2022: member

Do we want this for 23.0?

Yes, definitely.

It would be really good to have a tested ACK from someone experiencing the segfault problem.

laanwj commented at 3:37 pm on February 28, 2022: member

Code review ACK c4d76c6faa3adf06f192649e169ca860ce420d30

fanquake commented at 1:45 pm on March 1, 2022: member

@GITErnesO @IvRRimum are you able to test this change?

GITErnesO commented at 4:28 pm on March 1, 2022: none

@GITErnesO @IvRRimum are you able to test this change?

I have 7 empty wallets that throw this error. I’m still trying to build from sources.

I gave one of the wallets that throw this error to achow101 so my guess is that he checked that now it works OK. And my guess is that it is the same issue for the other wallets.

If I can get and RC or an internal version I will check all the 7 wallets. I’ll also continue to build from sources, so I can compile a version with this pull and test the wallets.

laanwj merged this on Mar 2, 2022

laanwj closed this on Mar 2, 2022

sidhujag referenced this in commit 4604cb4862 on Mar 2, 2022

DrahtBot locked this on Mar 2, 2023

wallet: Derive inactive HD chains in additional places #23304

Conflicts