wallet: Derive inactive HD chains in additional places #23304

achow101 · 2021-10-18T20:43:13Z

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.

DrahtBot · 2021-10-19T08:30:14Z

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 · 2021-11-03T13:15:42Z

Ready for review, supersedes #23277

laanwj · 2021-11-29T11:17:03Z

Concept ACK, thanks for adding a test!

Resolves segfault on legacy wallet Log warning when meta.key_origin.path is below expected size

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.

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

achow101 · 2021-12-08T16:27:16Z

Rebased for hidden merge conflict.

PastaPastaPasta

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

src/wallet/scriptpubkeyman.cpp

src/wallet/scriptpubkeyman.h

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).

PastaPastaPasta · 2022-01-28T04:50:01Z

src/wallet/scriptpubkeyman.cpp

+    } else {
+        nTargetSize = std::max(gArgs.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), int64_t{0});
+    }
+    int64_t target = std::max((int64_t) nTargetSize, int64_t{1});


avoid c-style casts

Suggested change

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

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

PastaPastaPasta · 2022-01-28T04:50:32Z

src/wallet/scriptpubkeyman.cpp

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


Suggested change

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

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

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

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

achow101 · 2022-02-17T17:30:03Z

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

test/functional/wallet_inactive_hdchains.py

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.

laanwj · 2022-02-28T12:21:06Z

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 · 2022-02-28T15:37:18Z

Code review ACK c4d76c6

fanquake · 2022-03-01T13:45:40Z

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

GITErnesO · 2022-03-01T16:28:03Z

@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.

…places c4d76c6 tests: Tests for inactive HD chains (Andrew Chow) 8077862 wallet: Refactor TopUp to be able to top up inactive chains too (Andrew Chow) 70134eb wallet: Properly set hd chain counters when loading (Andrew Chow) 961b9e4 wallet: Parse hdKeypath if key_origin is not available (Andrew Chow) 0652ee7 Add size check on meta.key_origin.path (Rob Fielding) Pull request description: 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 `CHDChain`s 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. ACKs for top commit: laanwj: Code review ACK c4d76c6 Tree-SHA512: b2b572ad7f1b1b2847edece09f7583543d63997e18ae32764e5a27ad608dd64b9bdb2d84ea27137894e986a8e82f047a3dba9c8015b74f5f179961911f0c4095

achow101 force-pushed the inactivehd-derive-keypath-string branch 2 times, most recently from a5d8957 to e9f203f Compare October 18, 2021 22:40

DrahtBot added the Wallet label Oct 18, 2021

DrahtBot mentioned this pull request Oct 19, 2021

wallet: Add size check on meta.key_origin.path #23277

Closed

achow101 force-pushed the inactivehd-derive-keypath-string branch from e9f203f to ee93c29 Compare November 3, 2021 13:13

achow101 marked this pull request as ready for review November 3, 2021 13:14

rooprob and others added 3 commits December 8, 2021 11:22

Add size check on meta.key_origin.path

0652ee7

Resolves segfault on legacy wallet Log warning when meta.key_origin.path is below expected size

wallet: Parse hdKeypath if key_origin is not available

961b9e4

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.

wallet: Properly set hd chain counters when loading

70134eb

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

achow101 force-pushed the inactivehd-derive-keypath-string branch from ee93c29 to 38c949a Compare December 8, 2021 16:27

This was referenced Dec 18, 2021

docs: avoid C-style casts; use modern C++ casts #23810

Merged

refactor: use braced init for integer literals instead of c style casts #23829

Merged

PastaPastaPasta suggested changes Jan 14, 2022

View reviewed changes

achow101 force-pushed the inactivehd-derive-keypath-string branch from 38c949a to f835693 Compare January 14, 2022 20:02

achow101 force-pushed the inactivehd-derive-keypath-string branch from f835693 to f3b02bd Compare January 14, 2022 20:03

PastaPastaPasta reviewed Jan 28, 2022

View reviewed changes

maflcko changed the title ~~wallet: Derive inactive HD chains in addtional places~~ wallet: Derive inactive HD chains in additional places Feb 17, 2022

maflcko added this to the 23.0 milestone Feb 17, 2022

fanquake requested review from instagibbs and meshcollider February 21, 2022 09:48

instagibbs reviewed Feb 22, 2022

View reviewed changes

test/functional/wallet_inactive_hdchains.py Outdated Show resolved Hide resolved

test/functional/wallet_inactive_hdchains.py Show resolved Hide resolved

test/functional/wallet_inactive_hdchains.py Show resolved Hide resolved

achow101 force-pushed the inactivehd-derive-keypath-string branch 2 times, most recently from 28d3370 to 87e090a Compare February 22, 2022 19:38

tests: Tests for inactive HD chains

c4d76c6

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.

achow101 force-pushed the inactivehd-derive-keypath-string branch from 87e090a to c4d76c6 Compare February 22, 2022 19:41

fanquake requested review from sipa and removed request for instagibbs February 23, 2022 11:05

laanwj mentioned this pull request Feb 24, 2022

Segmentation Fault in V21 and V22 releases #24335

Closed

fanquake linked an issue Feb 24, 2022 that may be closed by this pull request

Segmentation Fault in V21 and V22 releases #24335

Closed

laanwj merged commit 267917f into bitcoin:master Mar 2, 2022

bitcoin locked and limited conversation to collaborators Mar 2, 2023

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

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

wallet: Derive inactive HD chains in additional places #23304

wallet: Derive inactive HD chains in additional places #23304

Uh oh!

Conversation

achow101 commented Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

achow101 commented Nov 3, 2021

Uh oh!

laanwj commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Dec 8, 2021

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PastaPastaPasta Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

achow101 commented Feb 17, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laanwj commented Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Feb 28, 2022

Uh oh!

fanquake commented Mar 1, 2022

Uh oh!

GITErnesO commented Mar 1, 2022

Uh oh!

Uh oh!

achow101 commented Oct 18, 2021 •

edited

Loading

DrahtBot commented Oct 19, 2021 •

edited

Loading

laanwj commented Nov 29, 2021 •

edited

Loading

laanwj commented Feb 28, 2022 •

edited

Loading