test: Test that migration automatically repairs corrupted metadata with doubled derivation path #29124

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:fix-double-keypath changing 1 files +83 −1
  1. achow101 commented at 8:47 pm on December 20, 2023: member

    A bug in 0.21.x and 22.x resulted in some wallets having invalid derivation paths that are the concatenation of two derivation paths. These appear only when inactive hd chains are topped up.

    Since key metadata is a legacy wallet only record, migrating legacy wallets to descriptor wallets will fix this issue as all key metadata records are deleted. The derivation path information is derived on-the-fly from the descriptor that is produced for the inactive hd chain.

    Thus we only need a test to verify that the derivation paths are good, and that all key metadata records are deleted from the migrated wallet.

  2. DrahtBot commented at 8:47 pm on December 20, 2023: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29124.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK rkrux, furszy, murchandamus

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label Wallet on Dec 20, 2023
  4. achow101 force-pushed on Dec 20, 2023
  5. DrahtBot added the label CI failed on Dec 20, 2023
  6. achow101 force-pushed on Dec 20, 2023
  7. achow101 force-pushed on Dec 21, 2023
  8. achow101 force-pushed on Dec 21, 2023
  9. DrahtBot removed the label CI failed on Dec 21, 2023
  10. DrahtBot added the label CI failed on Jan 15, 2024
  11. DrahtBot commented at 7:42 am on April 5, 2024: contributor
    Could rebase for fresh CI, if still relevant?
  12. achow101 force-pushed on Apr 5, 2024
  13. DrahtBot removed the label CI failed on Apr 5, 2024
  14. in src/wallet/walletdb.cpp:699 in 627216a174 outdated 
    694         return DBErrors::LOAD_OK;
695     });
696     result = std::max(result, keymeta_res.m_result);
697 
698+    // Update any changed metadata
699+    if (updated_metas.size()) {
    furszy commented at 2:40 pm on May 22, 2024:
    nit: !updated_metas.empty()
    achow101 commented at 11:54 pm on June 4, 2024:
    Done
  15. in src/wallet/walletdb.cpp:530 in 627216a174 outdated 
    526@@ -527,7 +527,7 @@ static LoadResult LoadRecords(CWallet* pwallet, DatabaseBatch& batch, const std:
527     return LoadRecords(pwallet, batch, key, prefix, load_func);
528 }
529 
530-static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, int last_client) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
531+static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, WalletBatch& w_batch, int last_client) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
    furszy commented at 2:46 pm on May 22, 2024:
    Non-blocking because it is the only way now but it is kinda unnatural to provide batch and w_batch when w_batch contains the batch object.
    achow101 commented at 11:55 pm on June 4, 2024:
    I’ve made m_batch public so that we can just pass the WalletBatch but still use the underlying batch object. However I’m not sure if that’s something we want to do in the long term, but I think it should be okay.
    furszy commented at 1:49 pm on June 5, 2024:

    I’ve made m_batch public so that we can just pass the WalletBatch but still use the underlying batch object. However I’m not sure if that’s something we want to do in the long term, but I think it should be okay.

    Yeah.. I’m not happy reading all the the batch.m_batch usages neither but np.

  16. in src/wallet/walletdb.cpp:704 in 627216a174 outdated 
    699+    if (updated_metas.size()) {
700+        LegacyScriptPubKeyMan* spkm = pwallet->GetLegacyScriptPubKeyMan();
701+        Assert(spkm);
702+        LOCK(spkm->cs_KeyStore);
703+        for (const auto& pubkey : updated_metas) {
704+            w_batch.WriteKeyMetadata(spkm->mapKeyMetadata.at(pubkey.GetID()), pubkey, true);
    furszy commented at 2:46 pm on May 22, 2024:
    nit: should check WriteKeyMetadata return value.
    achow101 commented at 11:55 pm on June 4, 2024:
    Done
  17. furszy commented at 2:54 pm on May 22, 2024: member
    Code review ACK 078c2d74c2779dd459e42daf67ab450fbf223506. Nothing blocking, just tiny nits.
  18. achow101 force-pushed on Jun 4, 2024
  19. DrahtBot added the label CI failed on Jun 5, 2024
  20. DrahtBot commented at 3:21 am on June 5, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/25814613888

  21. achow101 force-pushed on Jun 5, 2024
  22. furszy commented at 1:50 pm on June 5, 2024: member
    ACK 9db2d8b
  23. DrahtBot removed the label CI failed on Jun 6, 2024
  24. DrahtBot added the label Needs rebase on Jul 11, 2024
  25. achow101 force-pushed on Jul 22, 2024
  26. DrahtBot removed the label Needs rebase on Jul 22, 2024
  27. furszy commented at 1:42 pm on July 23, 2024: member

    ACK 1c3462e908994f0d63c1f39b5908f48d083dd10e

    Could minimize the first commit diff https://github.com/furszy/bitcoin-core/commit/145737e46e71859608334ab738817f7b323df367.

  28. DrahtBot added the label CI failed on Sep 6, 2024
  29. achow101 force-pushed on Sep 10, 2024
  30. DrahtBot removed the label CI failed on Sep 11, 2024
  31. achow101 requested review from murchandamus on Oct 15, 2024
  32. achow101 commented at 3:53 pm on October 15, 2024: member
    cc @davidgumberg
  33. DrahtBot added the label CI failed on Oct 20, 2024
  34. DrahtBot added the label Needs rebase on Oct 24, 2024
  35. achow101 force-pushed on Oct 24, 2024
  36. DrahtBot removed the label Needs rebase on Oct 24, 2024
  37. DrahtBot removed the label CI failed on Oct 25, 2024
  38. in test/functional/wallet_backwards_compatibility.py:109 in e92dc1a3d3 outdated 
    150+        with open(dump_path, encoding="utf8") as f:
151+            for line in f:
152+                if "hdkeypath=m/0'/0'/9'" in line:
153+                    addr = line.split(" ")[4].split("=")[1]
154+                elif " hdseed=1 " in line:
155+                    seed = line.split(" ")[0]
    rkrux commented at 10:58 am on February 3, 2025:
    For others, reference: https://github.com/bitcoin/bitcoin/blob/28.x/src/wallet/rpc/wallet.cpp#L521
  39. in src/wallet/walletdb.cpp:650 in 92f4ab6dd7 outdated 
    647+                    // See https://github.com/bitcoin/bitcoin/issues/29109
648+                    // Markers for bad derivation:
649+                    // - 6 indexes
650+                    // - path[5] == path[2] + 1
651+                    // - path[0:2] == path[3:5]
652+                    // - path[3:6] matches hdKeypath
    rkrux commented at 11:11 am on February 3, 2025:
    I had been wondering why such a path can’t occur besides this case, this comment in DeriveNewChildKey function answers it: https://github.com/bitcoin/bitcoin/blob/dbc450c1b59b24421ba93f3e21faa8c673c0df4c/src/wallet/scriptpubkeyman.cpp#L1129
    rkrux commented at 2:06 pm on April 7, 2025:
    Could be helpful to add a comment here either referring it or copy-pasting the linked comment.
    achow101 commented at 3:58 am on April 10, 2025:
    Expanded the comment a little bit.
  40. in test/functional/wallet_backwards_compatibility.py:152 in e92dc1a3d3 outdated 
    147+        wallet.dumpwallet(dump_path)
148+        addr = None
149+        seed = None
150+        with open(dump_path, encoding="utf8") as f:
151+            for line in f:
152+                if "hdkeypath=m/0'/0'/9'" in line:
    rkrux commented at 11:37 am on February 3, 2025:

    I spent some time trying to understand why this one is picked here only to realise later that 9 is the last index at this chain because of keypool size set to 10 at the top of this test here: https://github.com/bitcoin/bitcoin/pull/29124/commits/e92dc1a3d36be02a8b16eed26159a43cfbe3dbcc#diff-b5055a65809b46e2775f6e28aa6eb8171e3fcc7a535cce5687a548b9ff233669R44

    This 9 and 10 can be tied to each other with a constant for better readability.

    rkrux commented at 11:38 am on February 3, 2025:
    Can this happen for internal keychain as well?
    rkrux commented at 8:58 am on February 4, 2025:
    Yes, it can as per this comment in the issue: #29109 (comment) The cpp code condition covers it as well though.
    achow101 commented at 8:01 pm on February 10, 2025:
    Added a constant.
  41. in test/functional/wallet_backwards_compatibility.py:169 in e92dc1a3d3 outdated 
    164+        self.sync_all(nodes=[self.nodes[0], node_master, node_v22])
165+        node_v22.loadwallet(wallet_name)
166+
167+        # Dump again to find bad hd keypath
168+        bad_deriv_path = "m/0'/0'/9'/0'/0'/10'"
169+        good_deriv_path = "m/0'/0'/10'"
    rkrux commented at 11:39 am on February 3, 2025:
    Similar tying to a constant here if possible.
    achow101 commented at 8:01 pm on February 10, 2025:
    Done
  42. in test/functional/wallet_backwards_compatibility.py:107 in e92dc1a3d3 outdated 
    148+        addr = None
149+        seed = None
150+        with open(dump_path, encoding="utf8") as f:
151+            for line in f:
152+                if "hdkeypath=m/0'/0'/9'" in line:
153+                    addr = line.split(" ")[4].split("=")[1]
    rkrux commented at 11:46 am on February 3, 2025:
    Reference for line format: https://github.com/bitcoin/bitcoin/blob/28.x/src/wallet/rpc/backup.cpp#L778-L802
  43. rkrux commented at 11:58 am on February 3, 2025: contributor
    Build and tests successful at e92dc1a3d36be02a8b16eed26159a43cfbe3dbcc Some initial comments, will give it another pass again.
  44. in test/functional/wallet_backwards_compatibility.py:185 in e92dc1a3d3 outdated 
    180+        # Verify that this addr is actually at m/0'/0'/10' by making a new wallet with the same seed
181+        node_v22.createwallet(wallet_name="path_verify", descriptors=False, blank=True)
182+        verify_wallet = node_v22.get_wallet_rpc("path_verify")
183+        verify_wallet.sethdseed(True, seed)
184+        # Bad addr is after keypool, so need to generate it by refilling
185+        verify_wallet.keypoolrefill(12)
    rkrux commented at 9:30 am on February 4, 2025:
    I’m realising that tying these constants up to the keypool size would make it easier to reason about this test.
    achow101 commented at 8:02 pm on February 10, 2025:
    Done
  45. in test/functional/wallet_backwards_compatibility.py:199 in e92dc1a3d3 outdated 
    195+
196+        # Check persistence
197+        wallet_master.unloadwallet()
198+        with node_master.assert_debug_log(expected_msgs=[], unexpected_msgs=["Repairing derivation path for"]):
199+            node_master.loadwallet(wallet_name)
200+        assert_equal(wallet_master.getaddressinfo(bad_path_addr)["hdkeypath"], good_deriv_path)
    rkrux commented at 9:33 am on February 4, 2025:
    Nice touch testing for persistence by loading it again!
  46. in test/functional/wallet_backwards_compatibility.py:193 in e92dc1a3d3 outdated 
    189+        backup_path = node_v22.wallets_path / f"{wallet_name}.bak"
190+        wallet.backupwallet(backup_path)
191+        with node_master.assert_debug_log(expected_msgs=["Repairing derivation path for"]):
192+            node_master.restorewallet(wallet_name, backup_path)
193+        wallet_master = node_master.get_wallet_rpc(wallet_name)
194+        assert_equal(wallet_master.getaddressinfo(bad_path_addr)["hdkeypath"], good_deriv_path)
    rkrux commented at 10:45 am on February 4, 2025: 
    0'pubkey': '029edc63efe154f7f0da891f0def969b65cc4706893f8277580093eccbd09cb11d', 'ischange': True, 'timestamp': 1738665397, 'hdkeypath': "m/0'/0'/10'",

    I found it a little weird that an external keychain address is categorised as change. I am assuming that internal keychain addresses are for changes.

    achow101 commented at 7:19 pm on February 10, 2025:
    ischange does not actually look at the derivation path. Instead it looks at some heuristics which result in derived-but-not-yet-requested external addresses being identified as change.
  47. in src/wallet/walletdb.h:307 in 92f4ab6dd7 outdated 
    301@@ -302,8 +302,8 @@ class WalletBatch
302     //! Registers db txn callback functions
303     void RegisterTxnListener(const DbTxnListener& l);
304 
305-private:
306     std::unique_ptr<DatabaseBatch> m_batch;
307+private:
    rkrux commented at 10:50 am on February 4, 2025:
    Nit: This change and the corresponding function call updates can be the first commit followed by the actual repairing. But I can understand if it’s a bit much for this PR given its limited focus area.
    achow101 commented at 7:20 pm on February 10, 2025:
    Going to leave this as is.
  48. rkrux approved
  49. rkrux commented at 10:53 am on February 4, 2025: contributor

    tACK e92dc1a3d36be02a8b16eed26159a43cfbe3dbcc

    Thanks for this PR, I enjoyed reviewing it. I ran the tests and debugged the compatibility test.

    Can / Will this also be back-ported to the affected version or is the affected version too far back to be back-ported into?

  50. DrahtBot requested review from furszy on Feb 4, 2025
  51. achow101 force-pushed on Feb 10, 2025
  52. achow101 force-pushed on Feb 10, 2025
  53. achow101 force-pushed on Feb 10, 2025
  54. achow101 commented at 10:48 pm on February 10, 2025: member
    Updated this to work even after the legacy wallet is deleted.
  55. in test/functional/wallet_backwards_compatibility.py:44 in 78cb0df4d5 outdated 
    40@@ -41,7 +41,7 @@ def set_test_params(self):
41             ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v25.0
42             ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v24.0.1
43             ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v23.0
44-            ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v22.0
45+            ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1", "-keypool=10"], # v22.0
    rkrux commented at 11:26 am on February 12, 2025:
    I had in mind tying this 10 to the constant as well. Consider if you end up retouching, otherwise it’s fine.
    achow101 commented at 3:58 am on April 10, 2025:
    Done
  56. in test/functional/wallet_backwards_compatibility.py:139 in 78cb0df4d5 outdated 
    179+        # Verify that this addr is actually at m/0'/0'/10' by making a new wallet with the same seed
180+        node_v22.createwallet(wallet_name="path_verify", descriptors=False, blank=True)
181+        verify_wallet = node_v22.get_wallet_rpc("path_verify")
182+        verify_wallet.sethdseed(True, seed)
183+        # Bad addr is after keypool, so need to generate it by refilling
184+        verify_wallet.keypoolrefill(LAST_KEYPOOL_INDEX + 2)
    rkrux commented at 8:30 am on February 13, 2025:
    This was using 12 earlier, now 11. Fine since the test passes.
  57. rkrux commented at 8:52 am on February 13, 2025: contributor

    reACK 78cb0df4d5b9cc30dbe60a6cf24ab6cac5d3f421

    I reviewed the range diff.

    0git range-diff e92dc1a3d36be02a8b16eed26159a43cfbe3dbcc...78cb0df4d5b9cc30dbe60a6cf24ab6cac5d3f421

    Newer changes are related to using the constant LAST_KEYPOOL_INDEX as suggested and use migratewallet instead of restorewallet due to which the format of good_deriv_path is changed from ' to ‘h’. Using migratewallet is fine because it also ends up calling LoadLegacyWalletRecords internally where the bug fix lies, and additionally it also ensures the fix works for the newer wallets.

  58. in src/wallet/walletdb.cpp:649 in 2eaa7e2de6 outdated 
    651+                    // Detect if this metadata has a bad derivation path from a bug in inactive hd key derivation that was present in 0.21 and 22.x
652+                    // See https://github.com/bitcoin/bitcoin/issues/29109
653+                    // Markers for bad derivation:
654+                    // - 6 indexes
655+                    // - path[5] == path[2] + 1
656+                    // - path[0:2] == path[3:5]
    murchandamus commented at 10:05 pm on April 2, 2025:

    Isn’t the line 648 in contradiction with line 649?

    Looking at #29109, I’m thinking that the indices are referring to the elements separated by “/” after “m” of the hdkeypath variable here.

    0-[..] hdkeypath=m/0'/0'/299'/0'/0'/300'
1+[..] hdkeypath=m/0'/0'/300'

    I am wondering whether this should be:

    0                    // - path[5] == path[2] + 1
1                    // - path[0:1] == path[3:4]
    murchandamus commented at 3:54 pm on April 4, 2025:
    Oh, are these ranges meant to be exclusive of the second parameter?
    rkrux commented at 1:48 pm on April 7, 2025:

    Yes, I believe from the comments standpoint, they are exclusive of the second parameter, which is quite confusing now that I re-read this diff after some time, the code below used hard-coded indices that are inclusive of the second parameter (except in case of end()).

    Consistency between code and comments would make it easier to read.

    achow101 commented at 3:46 am on April 10, 2025:
    Yes, it is pythonic
    achow101 commented at 3:59 am on April 10, 2025:
    Changed as suggested. Also added a bit more of an explainer to the comment that should make it clearer.
  59. in src/wallet/walletdb.cpp:650 in 2eaa7e2de6 outdated 
    652+                    // See https://github.com/bitcoin/bitcoin/issues/29109
653+                    // Markers for bad derivation:
654+                    // - 6 indexes
655+                    // - path[5] == path[2] + 1
656+                    // - path[0:2] == path[3:5]
657+                    // - path[3:6] matches hdKeypath
    murchandamus commented at 10:10 pm on April 2, 2025:

    If we only have six indexes, should this perhaps be path[3:5]?

    0                    // - path[3:5] matches hdKeypath
  60. murchandamus commented at 10:17 pm on April 2, 2025: contributor
    Concept ACK
  61. in src/wallet/walletdb.cpp:654 in 2eaa7e2de6 outdated 
    656+                    // - path[0:2] == path[3:5]
657+                    // - path[3:6] matches hdKeypath
658+                    if (keyMeta.key_origin.path.size() == 6
659+                        && keyMeta.key_origin.path[5] == keyMeta.key_origin.path[2] + 1
660+                        && keyMeta.key_origin.path[0] == keyMeta.key_origin.path[3]
661+                        && keyMeta.key_origin.path[1] == keyMeta.key_origin.path[4]
    rkrux commented at 2:01 pm on April 7, 2025:

    Before suggesting, I checked whether this function is going to stick around after the legacy wallet removal but after taking a cursory look at #28710, I see it will.

    In which case, I feel going at these checks in order would be slightly easier on the eyes. Might be nitty but easier for me at least.

    0-                        && keyMeta.key_origin.path[5] == keyMeta.key_origin.path[2] + 1
1-                        && keyMeta.key_origin.path[0] == keyMeta.key_origin.path[3]
2-                        && keyMeta.key_origin.path[1] == keyMeta.key_origin.path[4]
3+                        && keyMeta.key_origin.path[3] == keyMeta.key_origin.path[0]
4+                        && keyMeta.key_origin.path[4] == keyMeta.key_origin.path[1]
5+                        && keyMeta.key_origin.path[5] == keyMeta.key_origin.path[2] + 1
    achow101 commented at 3:59 am on April 10, 2025:
    Done
  62. rkrux changes_requested
  63. rkrux commented at 2:07 pm on April 7, 2025: contributor
    Concept ACK 78cb0df4d5b9cc30dbe60a6cf24ab6cac5d3f421
  64. DrahtBot added the label Needs rebase on Apr 10, 2025
  65. achow101 force-pushed on Apr 10, 2025
  66. achow101 force-pushed on Apr 10, 2025
  67. achow101 force-pushed on Apr 10, 2025
  68. achow101 force-pushed on Apr 10, 2025
  69. DrahtBot removed the label Needs rebase on Apr 10, 2025
  70. in test/functional/wallet_backwards_compatibility.py:190 in bdcd435b48 outdated 
    185+        backup_path = node_v22.wallets_path / f"{wallet_name}.bak"
186+        wallet.backupwallet(backup_path)
187+        wallet_dir_master = os.path.join(node_master.wallets_path, wallet_name)
188+        os.makedirs(wallet_dir_master, exist_ok=True)
189+        shutil.copy(backup_path, os.path.join(wallet_dir_master, "wallet.dat"))
190+        with node_master.assert_debug_log(expected_msgs=["Repairing derivation path for"]):
    rkrux commented at 12:36 pm on April 11, 2025:

    Nit if you end up retouching:

    0- with node_master.assert_debug_log(expected_msgs=["Repairing derivation path for"]):
1+ with node_master.assert_debug_log(expected_msgs=["Repairing derivation path for"], unexpected_msgs=["Unable to write repaired keymeta for"]):
    achow101 commented at 8:30 pm on April 17, 2025:
    Done
  71. in test/functional/wallet_backwards_compatibility.py:139 in bdcd435b48 outdated 
    134+        # Make sure that this is being detected and automatically cleaned up
135+        node_master = self.nodes[1]
136+        node_v22 = self.nodes[self.num_nodes - 5]
137+        wallet_name = "bad_deriv_path"
138+        node_v22.createwallet(wallet_name=wallet_name, descriptors=False)
139+        wallet = node_v22.get_wallet_rpc(wallet_name)
    rkrux commented at 1:23 pm on April 11, 2025:

    Nit if retouched:

    0- wallet = node_v22.get_wallet_rpc(wallet_name)
1+ bad_deriv_wallet = node_v22.get_wallet_rpc(wallet_name)

    For easy identification because this wallet is used across the whole test where other wallets are also created later down.

    achow101 commented at 8:30 pm on April 17, 2025:
    Done
  72. in test/functional/wallet_backwards_compatibility.py:176 in bdcd435b48 outdated 
    171+                if f"hdkeypath={bad_deriv_path}" in line:
172+                    bad_path_addr = line.split(" ")[4].split("=")[1]
173+        assert bad_path_addr is not None
174+        assert_equal(wallet.getaddressinfo(bad_path_addr)["hdkeypath"], bad_deriv_path)
175+
176+        # Verify that this addr is actually at m/0'/0'/10' by making a new wallet with the same seed
    rkrux commented at 1:23 pm on April 11, 2025: 
    0- # Verify that this addr is actually at m/0'/0'/10' by making a new wallet with the same seed
1+ # Verify that this bad derivation path addr is actually at m/0'/0'/10' by making a new wallet with the same seed but with a higher keypool size
    achow101 commented at 8:30 pm on April 17, 2025:
    Done
  73. rkrux approved
  74. rkrux commented at 1:44 pm on April 11, 2025: contributor

    tACK bdcd435b48d4979751e6dd74cb996073721e5c9d

    Build and functional tests good, I left few clarification nits if you end up retouching.

    The main change detects bad derivation paths when loading the metadata within legacy wallet records, updates the metadata in the legacy SPKM, and then writes the updated metadata in the DB.

    git range-diff 78cb0df…bdcd435

    Newer changes are improving the bad derivation path detection inline comments that makes it easier to follow by appearing consistent with the code right below, reusing LAST_KEYPOOL_INDEX in the node setup of the functional test and rebase changes - using HasLegacyRecords(), using an updated functional test node count.

    The addition of the detection of the bad derivation path in the LoadFunc is slightly distracting when I read the LoadLegacyWalletRecords() as a whole but I have not suggested changes because this is legacy wallet and intended to be used less now.

    The comprehensive functional test covers the buggy scenario along with its resolution. Also, the original issue #29109 is resolved for the user after using this fix that gives more confidence as well.

  75. DrahtBot requested review from murchandamus on Apr 11, 2025
  76. in src/wallet/walletdb.cpp:733 in afce141c17 outdated 
    728+    if (!updated_metas.empty()) {
729+        LegacyDataSPKM* spkm = pwallet->GetLegacyDataSPKM();
730+        Assert(spkm);
731+        LOCK(spkm->cs_KeyStore);
732+        for (const auto& pubkey : updated_metas) {
733+            if (!batch.WriteKeyMetadata(spkm->mapKeyMetadata.at(pubkey.GetID()), pubkey, true)) {
    furszy commented at 2:29 pm on April 17, 2025:
    it would be better to use find() instead of at() in case the key metadata is not in the wallet. E.g. an edge case.. could probably happen when hdKeypath path matches the 0.21 bug pattern but fails the hardened bits checks (this requires user’s manual intervention to occur).
    furszy commented at 6:02 pm on April 17, 2025:
    Also, another nit; could batch the db writes and fail if commit fails.
    achow101 commented at 8:30 pm on April 17, 2025:
    Done
  77. DrahtBot requested review from furszy on Apr 17, 2025
  78. achow101 force-pushed on Apr 17, 2025
  79. in test/functional/wallet_backwards_compatibility.py:190 in be7a7eab0e outdated 
    185+        backup_path = node_v22.wallets_path / f"{wallet_name}.bak"
186+        bad_deriv_wallet.backupwallet(backup_path)
187+        wallet_dir_master = os.path.join(node_master.wallets_path, wallet_name)
188+        os.makedirs(wallet_dir_master, exist_ok=True)
189+        shutil.copy(backup_path, os.path.join(wallet_dir_master, "wallet.dat"))
190+        with node_master.assert_debug_log(expected_msgs=["Repairing derivation path for"], unexpected_msgs=["Unable to write repaired ketmeta for"]):
    DrahtBot commented at 8:33 pm on April 17, 2025:

    (✨ LLM generated, experimental)

    ketmeta → keymeta

  80. achow101 force-pushed on Apr 17, 2025
  81. achow101 force-pushed on Apr 17, 2025
  82. rkrux approved
  83. rkrux commented at 9:19 am on April 21, 2025: contributor

    reACK 670d02bc380c22fa4c985b72e8209f475ed0e7d0

    0git range-diff bdcd435...670d02b

    Using find instead of at now, few other functional test naming & comments changes done as suggested earlier.

  84. achow101 commented at 6:09 pm on April 21, 2025: member
    @furszy has uncovered that it seems like migrating legacy to descriptors also solves the problem. Since removal of the legacy wallet is close, I will look into this a bit further and probably turn this PR into just a test.
  85. achow101 marked this as a draft on Apr 21, 2025
  86. achow101 force-pushed on Apr 21, 2025
  87. achow101 marked this as ready for review on Apr 21, 2025
  88. achow101 renamed this:
    wallet: Automatically repair corrupted metadata with doubled derivation path
    test: Test that migration automatically repairs corrupted metadata with doubled derivation path
    on Apr 21, 2025
  89. achow101 force-pushed on Apr 21, 2025
  90. achow101 commented at 8:03 pm on April 21, 2025: member
    Removed the fix commit since we are expecting legacy wallets to go away soon, and migration is a valid way to fix this problem. The remaining test commit is updated to work without the automatic repair being logged. It additionally is changed to check that no keymeta records exist after the migration.
  91. achow101 force-pushed on Apr 21, 2025
  92. DrahtBot added the label CI failed on Apr 21, 2025
  93. DrahtBot commented at 8:04 pm on April 21, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/40895590474

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  94. DrahtBot removed the label CI failed on Apr 21, 2025
  95. in test/functional/wallet_backwards_compatibility.py:203 in 4c2d246151 outdated 
    198+        try:
199+            import sqlite3
200+            wallet_db = node_master.wallets_path / wallet_name / "wallet.dat"
201+            conn = sqlite3.connect(wallet_db)
202+            with conn:
203+                # Retrieve all records that have the keymeta prefix
    rkrux commented at 12:16 pm on April 22, 2025:

    Nit:

    0- # Retrieve all records that have the keymeta prefix
1+ # Retrieve all records that have the "keymeta" prefix, rest of the key data is the pubkey that differs for each record
    achow101 commented at 5:07 pm on April 22, 2025:
    Done
  96. in test/functional/wallet_backwards_compatibility.py:204 in 4c2d246151 outdated 
    199+            import sqlite3
200+            wallet_db = node_master.wallets_path / wallet_name / "wallet.dat"
201+            conn = sqlite3.connect(wallet_db)
202+            with conn:
203+                # Retrieve all records that have the keymeta prefix
204+                keymeta_rec = conn.execute("SELECT value FROM main where key >= x'076b65796d6574' AND key < x'076b65796d6575'").fetchone()
    rkrux commented at 12:18 pm on April 22, 2025:

    Isn’t [076b65796d6574, 076b65796d6575) a little too wide of a search range?

    “keymeta” in hex is “6b65796d657461”, prepending it with the compact size length “07” should make the key prefix “076b65796d657461” in the serialised form imo, making the range as [076b65796d657461, 076b65796d657462)

    achow101 commented at 5:07 pm on April 22, 2025:
    Oops yes, fixed.
  97. in test/functional/wallet_backwards_compatibility.py:134 in 4c2d246151 outdated 
    126@@ -126,6 +127,86 @@ def test_v19_addmultisigaddress(self):
127         wallet = node_v19.get_wallet_rpc("w1_v19")
128         assert wallet.getaddressinfo(address_18075)["solvable"]
129 
130+
131+    def test_v22_inactivehdchain_path(self):
132+        self.log.info("Testing inactive hd chain bad derivation path cleanup")
133+        # 0.21.x and 22.x would both produce bad derivation paths when topping up an inactive hd chain
134+        # Make sure that this is being detected and automatically cleaned up
    rkrux commented at 12:19 pm on April 22, 2025: 
    0- # Make sure that this is being detected and automatically cleaned up
1+ # Make sure that this is being automatically cleaned up during migration
    achow101 commented at 5:07 pm on April 22, 2025:
    Done
  98. rkrux commented at 12:39 pm on April 22, 2025: contributor
    Nice that the migration flow can fix it automatically and no new custom handling is required now while loading the legacy wallets!
  99. achow101 force-pushed on Apr 22, 2025
  100. achow101 force-pushed on Apr 22, 2025
  101. achow101 force-pushed on Apr 22, 2025
  102. rkrux commented at 2:42 pm on April 23, 2025: contributor

    LGTM, ACK bfcf5c5ab1fdba46d492aa0872c3d5f9a4a87633 - it’s just one functional test now, a detailed one at that.

    Couple of observations/questions after rotating the seed and unloading the wallet that I found noteworthy enough to share here.

    1. The listreceivedbyaddress RPC doesn’t show any entry for addr to which the 1 BTC was sent, but listunspent RPC does show.

    2. The desc field in the bad_deriv_wallet.listunspent and bad_deriv_wallet_master.listunspent calls for the same addr was different slightly as I see ' was used in the former & h in the latter due to which the checksum differs. Is this the default behaviour in migration? I vaguely recall reading about it somewhere else too.

    -> bad_deriv_wallet.listunspent

    0desc': "wpkh([7c198af4/0'/0'/9']03ef7ecec2fc90ce130c7a244762d7dae68f0176977debc2bfe9839c7b097342ff)#jlrcq70p"

    -> bad_deriv_wallet_master.listunspent

    0'desc': 'wpkh([7c198af4/0h/0h/9h]03ef7ecec2fc90ce130c7a244762d7dae68f0176977debc2bfe9839c7b097342ff)#4n2zmzyr'
  103. achow101 commented at 6:14 pm on April 23, 2025: member
    1. The listreceivedbyaddress RPC doesn’t show any entry for addr to which the 1 BTC was sent, but listunspent RPC does show.

    This is expected since we didn’t get addr through normal means, and it therefore is missing expected items for receiving addresses like an address book entry.

    1. The desc field in the bad_deriv_wallet.listunspent and bad_deriv_wallet_master.listunspent calls for the same addr was different slightly as I see ' was used in the former & h in the latter due to which the checksum differs. Is this the default behaviour in migration? I vaguely recall reading about it somewhere else too.

    This is expected. In a bunch of places, the standard hardened derivation marker is '. However, for descriptors, since #26076, we use h instead as it is easier to copy-paste in terminals.

  104. murchandamus commented at 11:40 pm on April 23, 2025: contributor
    ACK bfcf5c5ab1fdba46d492aa0872c3d5f9a4a87633
  105. DrahtBot requested review from rkrux on Apr 23, 2025
  106. furszy commented at 3:01 pm on April 24, 2025: member
    utACK bfcf5c5ab1fdba46d492aa0872c3d5f9a4a87633
  107. DrahtBot added the label Needs rebase on Apr 25, 2025
  108. achow101 force-pushed on Apr 25, 2025
  109. tests: Test migration cleans up bad inactive chain derivation path 
    A bug in 0.21.x and 22.x resulted in some wallets having invalid
derivation paths that are the concatenation of two derivation paths.
These appear only when inactive hd chains are topped up.

Since key metadata is a legacy wallet only record, migrating legacy
wallets to descriptor wallets will fix this issue as all key metadata
records are deleted. The derivation path information is derived
on-the-fly from the descriptor that is produced for the inactive hd
chain.

Thus we only need a test to verify that the derivation paths are good,
and that all key metadata records are deleted from the migrated wallet.
    c7e2b9e264
  110. DrahtBot removed the label Needs rebase on Apr 25, 2025
  111. rkrux approved
  112. rkrux commented at 11:36 am on April 26, 2025: contributor

    re-ACK c7e2b9e2644442b147880becb8a659f3d00092d9

    0git range-diff bfcf5c5ab1fdba46d492aa0872c3d5f9a4a87633..c7e2b9e2644442b147880becb8a659f3d00092d9

    The range-diff contains only rebase changes now that #31250 is merged.

  113. DrahtBot requested review from murchandamus on Apr 26, 2025
  114. DrahtBot requested review from furszy on Apr 26, 2025
  115. furszy commented at 7:18 pm on April 28, 2025: member
    utACK c7e2b9e2644442b147880becb8a659f3d00092d9
  116. murchandamus commented at 7:45 pm on April 28, 2025: contributor

    re-ACK c7e2b9e2644442b147880becb8a659f3d00092d9 via range-diff:

    git range-diff b8cefeb221490868d62b7a0695f8b5ea392d3654..bfcf5c5ab1fdba46d492aa0872c3d5f9a4a87633 80e6ad9e3023a57a4ef19b7d0edf9ac5be71a584..c7e2b9e2644442b147880becb8a659f3d00092d9
  117. glozow merged this on Apr 28, 2025
  118. glozow closed this on Apr 28, 2025


achow101 DrahtBot furszy rkrux murchandamus


murchandamus

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-05-26 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me