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

achow101 commented at 5:43 pm on May 22, 2020: member

Previously the seed was stored with keypath ’m’ so we need to skip this as well when determining inactive seeds.

Fixes #19051

MarcoFalke commented at 5:48 pm on May 22, 2020: member

Are there steps to test for people that are not me? Or, alternatively, is it possible to write a test for this?

achow101 commented at 5:54 pm on May 22, 2020: member

I think this can be tested in feature_backwards_compatibility.py. I’ll see if I can write a test.

Otherwise we would need to directly modify the wallet.dat file.

achow101 commented at 6:58 pm on May 22, 2020: member

0.16 uses the old ’m’ for the hd seed, so I’ve updated feature_backwards_compatibility.py to do an up/downgrade test with a 0.16 wallet. This test also checks that there is no warning when loading the wallet. It catches this error.

DrahtBot added the label Tests on May 22, 2020

DrahtBot added the label Wallet on May 22, 2020

MarcoFalke removed the label Tests on May 23, 2020

in src/wallet/walletdb.cpp:448 in 57acd1b27f outdated

443@@ -444,7 +444,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
444                 // Not applicable when path is "s" as that indicates a seed
445                 bool internal = false;
446                 uint32_t index = 0;
447-                if (keyMeta.hdKeypath != "s") {
448+                if (keyMeta.hdKeypath != "s" && keyMeta.hdKeypath != "m") {

ryanofsky commented at 6:58 pm on May 26, 2020:

In commit “Skip hdKeypath of ’m’” (57acd1b27fb6cf46b919a05e3fe50a1c2947321f)

Would be good to update “Not applicable when…” comment above

achow101 commented at 10:12 pm on June 2, 2020:

Done

in test/functional/feature_backwards_compatibility.py:343 in 7a87270296 outdated

339@@ -331,6 +340,29 @@ def run_test(self):
340         hdkeypath = v17_info["hdkeypath"]
341         pubkey = v17_info["pubkey"]
342 
343+        # Copy the 0.16 wallet to the last Bitcoin Core version and open it:

ryanofsky commented at 7:07 pm on May 26, 2020:

In commit “tests: feature_backwards_compatibility.py test 0.16 up/downgrade” (7a872702965755ce28f1f17a8d0e0eb93ae4c907)

Implemention seems ideal for this PR, but for future cleanup it could be nice for someone to write some python helper functions and get rid of duplicate code in the 0.16, 0.17, and 0.19 test cases.

Sjors commented at 12:57 pm on June 15, 2020:

Indeed, I’ve been kicking that down the road too, see #19013. Things are getting a bit brittle.

ryanofsky approved

ryanofsky commented at 7:11 pm on May 26, 2020: member

Code review ACK 7a872702965755ce28f1f17a8d0e0eb93ae4c907. Simple bugfix, and it’s nice how the new 0.16 test can potentially catch not just this bug, but other similar bugs from loading these wallets.

gmaxwell commented at 10:38 pm on May 30, 2020: contributor

Does this mean that older hd wallets opened prior to this fix have been corrupted?

Also, post-fix does this mean that the #17681 functionality (following old hd chains forward) will silently not work for these wallets (since their hd keys just get skipped)?

achow101 commented at 10:55 pm on May 30, 2020: member

Does this mean that older hd wallets opened prior to this fix have been corrupted?

Also, post-fix does this mean that the #17681 functionality (following old hd chains forward) will silently not work for these wallets (since their hd keys just get skipped)?

No to both. #17681 functionality works regardless of this fix, regardless of age of the HD wallet. This fix is to merely remove an erroneous warning for those older HD wallets.

This fix does not skip any HD keys at all. It just skips the keymeta record for the HD seed when it is computing the inactive HD chain from those keymetadata records. The record for the HD seed is not used in that process, hence this patch to skip it.

achow101 force-pushed on Jun 2, 2020

in src/wallet/walletdb.cpp:444 in b9eca01556 outdated

440@@ -441,10 +441,10 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
441             // Extract some CHDChain info from this metadata if it has any
442             if (keyMeta.nVersion >= CKeyMetadata::VERSION_WITH_HDDATA && !keyMeta.hd_seed_id.IsNull() && keyMeta.hdKeypath.size() > 0) {
443                 // Get the path from the key origin or from the path string
444-                // Not applicable when path is "s" as that indicates a seed
445+                // Not applicable when path is "s" or "m" as those indicates a seed

luke-jr commented at 4:04 am on June 3, 2020:

nit: “indicate”

Sjors commented at 1:23 pm on June 15, 2020:

Let’s point to https://github.com/bitcoin/bitcoin/commit/c75c351419dac3dfe9578604ea5b2f7599452b4a#diff-b2bb174788c7409b671c46ccc86034bdL1470 here or in the functional test.

achow101 commented at 3:02 pm on June 15, 2020:

Fixed typo. Added a reference to the PR.

instagibbs commented at 1:06 pm on June 12, 2020: member

utACK https://github.com/bitcoin/bitcoin/pull/19054/commits/b9eca01556169041025e2ed85061bc90c64a5d8d

in test/functional/feature_backwards_compatibility.py:326 in b9eca01556 outdated

321@@ -322,6 +322,15 @@ def run_test(self):
322         info = wallet.getwalletinfo()
323         assert info['keypoolsize'] == 1
324 
325+        # Create upgrade wallet in v0.16
326+        self.stop_node(5)

Sjors commented at 12:59 pm on June 15, 2020:

Use -1 instead. Maybe cherry-pick 99d9f7cf7b7460c976d6ca7508e77b4ee31bf576

achow101 commented at 3:02 pm on June 15, 2020:

Done

in test/functional/feature_backwards_compatibility.py:350 in b9eca01556 outdated

344+        shutil.copyfile(
345+            os.path.join(node_v16_wallets_dir, "wallets/u1_v16"),
346+            os.path.join(node_master_wallets_dir, "u1_v16")
347+        )
348+        load_res = node_master.loadwallet("u1_v16")
349+        assert_equal(load_res['warning'], '')

Sjors commented at 1:02 pm on June 15, 2020:

nit: refer to this PR in a comment

achow101 commented at 3:02 pm on June 15, 2020:

Done

Sjors commented at 1:33 pm on June 15, 2020: member

Concept ACK. When I run the new test without 5fb09ed it catches a warning.

For future reference, the change from ~~“s” to “m”~~ “m” to “s” was introduced in #12924 (cc @jnewbery, @jonasschnelli). At the time this change wasn’t a problem, because the data was serialised, but afaik not used anywhere.

Skip hdKeypath of 'm'

Previously the seed was stored with keypath 'm' so we need to skip this
as well when determining inactive seeds.

3a03a11e8c

tests: feature_backwards_compatibility.py test 0.16 up/downgrade 951bca61d7

achow101 force-pushed on Jun 15, 2020

Sjors commented at 5:49 pm on June 15, 2020: member

ACK 951bca61d7376be44fad0775e8abb06ff667e4bf

jnewbery commented at 7:02 pm on June 15, 2020: member

For future reference, the change from “s” to “m” was introduced in #12924 @Sjors

You’ve got it the wrong way round. Prior to 12924, the hd keypath of a BIP32 seed would incorrectly be set as ’m’. 12924 fixed that so that the seed’s keypath is ’s'.

DrahtBot commented at 11:06 pm on June 15, 2020: 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:

#18836 (wallet: upgradewallet fixes and additional tests by achow101)

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.

ryanofsky approved

ryanofsky commented at 10:30 pm on June 18, 2020: member

Code review ACK 951bca61d7376be44fad0775e8abb06ff667e4bf. No significant changes since last review, just updated comment and some test tweaks

fanquake requested review from instagibbs on Jun 19, 2020

instagibbs commented at 8:04 pm on June 19, 2020: member

re-utACK https://github.com/bitcoin/bitcoin/pull/19054/commits/951bca61d7376be44fad0775e8abb06ff667e4bf

MarcoFalke merged this on Jun 19, 2020

MarcoFalke closed this on Jun 19, 2020

Fabcien referenced this in commit 5848be501e on Feb 11, 2021

DrahtBot locked this on Feb 15, 2022

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

Conflicts