wallet, test: Ancient Wallet Migration from v0.14.3 (no-HD and Single Chain) #33186

pull w0xlt wants to merge 1 commits into bitcoin:master from w0xlt:test_no_or_base_hd_wallet_migration changing 4 files +162 −12
  1. w0xlt commented at 6:50 pm on August 13, 2025: contributor

    This PR adds test coverage for migrating legacy Bitcoin Core wallets from v0.14.3 (released in 2017) to the descriptor wallet format. The test validates that users can safely upgrade their wallets while preserving all funds, transaction history, and addresses.

    This test was originally developed on top of #32977, as it was requested in reviews. However, since it also increases test coverage, it can be merged independently.

    The test covers two wallet migration scenarios:

    • Non-HD Wallet Migration - Tests migration of non-HD wallets (created with -usehd=0)
    • Single Chain HD Wallet Migration - Tests migration of HD wallets from v0.14.3 (VERSION_HD_BASE)

    The node v0.14.3 cannot be synced using the normal test framework helpers because it does not have the syncwithvalidationinterfacequeue RPC, so the test uses dumb_sync_blocks to submit blocks from the ancient node to the modern node before migrating the wallet.

    Each scenario uses its own dedicated old/new node pair, which keeps the setup isolated and makes this testing best managed in a separate file rather than in the existing migration test files.

    On the Windows cross-built CI job, this test is excluded from the main functional test runner and re-run sequentially in an ASCII-only tmpdir, because the v0.14.3 binary cannot handle non-ASCII characters in the temporary directory path.

  2. DrahtBot commented at 6:50 pm on August 13, 2025: 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/33186.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK pablomartin4btc
    Stale ACK furszy

    If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #34683 (RFC: Support a formal description of our JSON-RPC interface by willcl-ark)

    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.

    LLM Linter (✨ experimental)

    Possible typos and grammar issues:

    • “# because they are excluded above due to lack of UTF-8 support in the” -> “# because they are excluded above due to lack of UTF-8 support in ancient releases” [the added comment line ends with “in the”, leaving the sentence incomplete; completion is needed for comprehension]

    2026-03-17 19:38:11

  3. w0xlt force-pushed on Aug 13, 2025
  4. DrahtBot added the label CI failed on Aug 13, 2025
  5. DrahtBot commented at 6:57 pm on August 13, 2025: contributor

    🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/48028643718 LLM reason (✨ experimental): The CI failure is caused by a lint error due to trailing whitespace in the code.

    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.

  6. w0xlt force-pushed on Aug 13, 2025
  7. w0xlt force-pushed on Aug 13, 2025
  8. w0xlt force-pushed on Aug 13, 2025
  9. in test/functional/wallet_ancient_migration.py:62 in 3b1b154c31 
    57+        # Clean up any existing nodes and data
58+        self.cleanup_nodes()
59+
60+        # Setup fresh nodes for this test
61+        self.add_nodes(
62+            2,
    pablomartin4btc commented at 7:45 pm on August 13, 2025: 
    0            self.num_nodes,
    w0xlt commented at 10:14 pm on January 16, 2026:
    Done. Thanks.
  10. pablomartin4btc commented at 7:53 pm on August 13, 2025: member

    Concept ACK

    Left a couple of suggestions.

  11. in test/functional/test_runner.py:433 in 3b1b154c31 outdated 
    428@@ -428,7 +429,12 @@ def main():
429     logging.basicConfig(format='%(message)s', level=logging_level)
430 
431     # Create base test directory
432-    tmpdir = "%s/test_runner_₿_🏃_%s" % (args.tmpdirprefix, datetime.datetime.now().strftime("%Y%m%d_%H%M%S"))
433+    # Special case for tests using old binaries (e.g. version v0.14.3) that don't handle Unicode on Windows
434+    has_migration_tests = any('wallet_ancient_migration' in script for script in BASE_SCRIPTS)
    pablomartin4btc commented at 8:21 pm on August 13, 2025:
    more than has_migration_tests shouldn’t indicate like it’s an old binary? (there are other tests that uses migration and feature_unsupported_utxo_db.py uses a node v0.14.3 as well)
    w0xlt commented at 8:47 pm on August 13, 2025:

    Without this, the Windows, test cross-built CI task will fail because it can’t handle non-ASCII characters in -datadir.

    feature_unsupported_utxo_db.py doesn’t copy the directory from one node to another. This test needs to do this because version 0.14.3 doesn’t have the syncwithvalidationinterfacequeue RPC, so it can’t be synchronized.

    pablomartin4btc commented at 9:43 pm on August 13, 2025:

    Yeah, it’s already well explained in the PR description. Then perhaps the variable name could be supports_unicode_datadir/ (requires_ascii_datadir) or something like that.

    0    requires_ascii_datadir = any('wallet_ancient_migration' in script for script in BASE_SCRIPTS)
1    tmpdirprefix = "%s/test_runner_₿_🏃_%s"
2    if platform.system() == 'Windows' and requires_ascii_datadir:
3        tmpdir = "%s/test_runner_btc_run_%s"
4    tmpdir = tmpdirprefix % (args.tmpdirprefix, datetime.datetime.now().strftime("%Y%m%d_%H%M%S"))

    Wouldn’t this affect the datadir for all the tests running on Windows platform?

    w0xlt commented at 9:46 pm on August 13, 2025:
    Thanks for the suggestion. It’s much clearer. Yes, it affects all tests run on the Windows platform. As far as I know, there’s no way to filter by this task specifically.
    maflcko commented at 7:49 am on August 18, 2025:

    feature_unsupported_utxo_db.py doesn’t copy the directory from one node to another. This test needs to do this because version 0.14.3 doesn’t have the syncwithvalidationinterfacequeue RPC, so it can’t be synchronized.

    What’s the error message? It seems odd that one test works fine with unicode and the other does not. See also the suggestion https://github.com/bitcoin/bitcoin/pull/33186/files#r2281561637

    furszy commented at 2:52 pm on December 5, 2025:

    feature_unsupported_utxo_db.py doesn’t copy the directory from one node to another. This test needs to do this because version 0.14.3 doesn’t have the syncwithvalidationinterfacequeue RPC, so it can’t be synchronized.

    What’s the error message? It seems odd that one test works fine with unicode and the other does not. See also the suggestion https://github.com/bitcoin/bitcoin/pull/33186/files#r2281561637 @w0xlt can you please check this ^^ ?

    Also, aren’t we essentially removing the unicode coverage from the CI with this change? wallet_ancient_migration.py test should always be enabled there.

    w0xlt commented at 10:12 pm on January 16, 2026:
    Applied the same approach as used in https://github.com/bitcoin/bitcoin/pull/34240
  12. DrahtBot removed the label CI failed on Aug 13, 2025
  13. in test/functional/wallet_ancient_migration.py:167 in 3b1b154c31 outdated 
    162+
163+        # Verify the blockchain was loaded correctly
164+        node1_info = self.nodes[1].getblockchaininfo()
165+        self.log.info(f"Modern node blockchain info: height={node1_info['blocks']}")
166+        assert_equal(node1_info['blocks'], 102)
167+
    w0xlt commented at 10:11 pm on January 16, 2026:
    I couldn’t find reference to dumb_sync_blocks in that commit. I used the same approach as in feature_unsupported_utxo_db.py
    maflcko commented at 10:02 am on January 19, 2026:

    I couldn’t find reference to dumb_sync_blocks in that commit.

    You should be able to use this command locally:

     0$ git show c847dee1488a294c9a9632a00ba1134b21e41947 -- ./test/functional/wallet_upgradewallet.py | grep -A19 'def dumb_sync'
 1-    def dumb_sync_blocks(self):
 2-        """
 3-        Little helper to sync older wallets.
 4-        Notice that v0.15.2's regtest is hardforked, so there is
 5-        no sync for it.
 6-        v0.15.2 is only being used to test for version upgrade
 7-        and master hash key presence.
 8-        v0.16.3 is being used to test for version upgrade and balances.
 9-        Further info: [#18774 (review)](/bitcoin-bitcoin/18774/#discussion_r416967844)
10-        """
11-        node_from = self.nodes[0]
12-        v16_3_node = self.nodes[1]
13-        to_height = node_from.getblockcount()
14-        height = self.nodes[1].getblockcount()
15-        for i in range(height, to_height+1):
16-            b = node_from.getblock(blockhash=node_from.getblockhash(i), verbose=0)
17-            v16_3_node.submitblock(b)
18-        assert_equal(v16_3_node.getblockcount(), to_height)
19-
20-    def test_upgradewallet(self, wallet, previous_version, requested_version=None, expected_version=None):
    w0xlt commented at 4:30 am on February 10, 2026:

    I added dumb_sync_blocks as suggested, which eliminates the copytree for the blocks directory.

    However, this doesn’t resolve the Windows CI issue. The v0.14.3 binary itself can’t handle Unicode characters in the temp directory path, so it still needs to be excluded from the main test runner and re-run with an ASCII-only tmpdir (same workaround as feature_unsupported_utxo_db.py).

    maflcko commented at 8:41 am on March 4, 2026:

    I added dumb_sync_blocks as suggested, which eliminates the copytree for the blocks directory.

    Turns out I needed the helper in https://github.com/bitcoin/bitcoin/pull/34728/files, so in case that is merged, you can rebase and just import it from util.py

    w0xlt commented at 9:41 pm on March 11, 2026:
    Updated the test with the helper introduced in #34728
  14. fanquake commented at 10:52 am on December 3, 2025: member
    @achow101 can you leave a conceptual review?
  15. furszy commented at 2:58 pm on December 3, 2025: member
    Concept ACK, will review soon
  16. in test/functional/wallet_ancient_migration.py:95 in 3b1b154c31 
    90+
91+        # Verify wallet type
92+        if "non-HD" in wallet_type:
93+            assert('hdmasterkeyid' not in old_wallet_info)
94+        else:
95+            assert('hdmasterkeyid' in old_wallet_info)
    furszy commented at 3:11 pm on December 5, 2025:
    Could check the wallet version here. The non-hd one should have version 6000 (WalletFeature::FEATURE_COMPRPUBKEY) and the hd one should have version 130000 (WalletFeature::FEATURE_HD)
    w0xlt commented at 10:09 pm on January 16, 2026:
    Done. Thanks.
  17. in test/functional/wallet_ancient_migration.py:195 in 3b1b154c31 
    190+        self.log.info(f"Backup created at: {migration_result['backup_path']}")
191+        assert(os.path.exists(migration_result['backup_path']))
192+
193+        # Rescan the blockchain
194+        self.log.info("Rescanning blockchain...")
195+        self.nodes[1].rescanblockchain()
    furszy commented at 3:14 pm on December 5, 2025:
    rescanning after migration shouldn’t be needed
    w0xlt commented at 10:09 pm on January 16, 2026:
    Done. Thanks.
  18. furszy commented at 3:21 pm on December 5, 2025: member

    The node v0.14.3 cannot be synced because it doesn’t have the syncwithvalidationinterfacequeue RPC implemented (required by test_framework.py), so the solution is to copy the block folder to the modern node and start it with -reindex-chainstate. Because of this additional complexity, this testing is best managed in a separate file rather than in the existing migration test files.

    In order to know when the node is synced, cannot you just wait for a specific log line? See busy_wait_for_debug_log.

  19. w0xlt force-pushed on Jan 7, 2026
  20. w0xlt force-pushed on Jan 16, 2026
  21. w0xlt force-pushed on Jan 16, 2026
  22. DrahtBot added the label CI failed on Jan 16, 2026
  23. DrahtBot removed the label CI failed on Jan 16, 2026
  24. w0xlt commented at 10:13 pm on January 16, 2026: contributor
    Updated the PR with the same solution used in #34240.
  25. w0xlt force-pushed on Feb 10, 2026
  26. w0xlt force-pushed on Feb 10, 2026
  27. DrahtBot added the label CI failed on Feb 10, 2026
  28. DrahtBot removed the label CI failed on Feb 10, 2026
  29. w0xlt commented at 6:03 am on February 10, 2026: contributor
    Updated the PR with the dumb_sync_blocks function as requested in #33186 (review)
  30. DrahtBot added the label Needs rebase on Feb 13, 2026
  31. w0xlt force-pushed on Mar 11, 2026
  32. DrahtBot removed the label Needs rebase on Mar 11, 2026
  33. w0xlt force-pushed on Mar 11, 2026
  34. DrahtBot added the label CI failed on Mar 11, 2026
  35. w0xlt commented at 9:41 pm on March 11, 2026: contributor
    Rebased and the test was updated with the helper introduced in #34728
  36. DrahtBot removed the label CI failed on Mar 11, 2026
  37. in test/functional/wallet_ancient_migration.py:107 in bf86d56a86 
    102+        assert_equal(self.nodes[1].getbalance(), old_balance)
103+        assert_equal(len(self.nodes[1].listtransactions("*", 1000)), len(old_txs))
104+
105+        # Verify all addresses are still owned
106+        for addr in all_addresses:
107+            assert self.nodes[1].getaddressinfo(addr)['ismine']
    furszy commented at 8:07 pm on March 12, 2026:
    if all_addresses is ever empty, this check would still pass when it shouldn’t.
    w0xlt commented at 8:02 pm on March 17, 2026:
    Thanks. Done in 3a4a7b6859.
  38. furszy commented at 8:25 pm on March 12, 2026: member
    utACK bf86d56a8696cfb5f4adff1dd2fb99e61d1f655d
  39. DrahtBot requested review from pablomartin4btc on Mar 12, 2026
  40. in test/functional/test_framework/util.py:729 in bf86d56a86 outdated 
    725@@ -726,7 +726,7 @@ def dumb_sync_blocks(*, src, dst, height=None):
726     height = height or src.getblockcount()
727     for i in range(dst.getblockcount() + 1, height + 1):
728         block_hash = src.getblockhash(i)
729-        block = src.getblock(blockhash=block_hash, verbose=0)
730+        block = src.getblock(blockhash=block_hash, verbose=False)
    maflcko commented at 2:53 pm on March 13, 2026:
    nit: Not sure. I think the tests should move away from bool and toward int verbosity
  41. in test/functional/wallet_ancient_migration.py:46 in bf86d56a86 
    41+            if node.running:
42+                self.stop_node(i)
43+
44+        for i in range(self.num_nodes):
45+            datadir = get_datadir_path(self.options.tmpdir, i)
46+            shutil.rmtree(datadir, ignore_errors=True)
    maflcko commented at 2:57 pm on March 13, 2026:
    q: Why is ignore_errors needed?
    maflcko commented at 2:59 pm on March 13, 2026:
    Also, https://github.com/bitcoin/bitcoin/pull/34439
    maflcko commented at 3:01 pm on March 13, 2026:

    Would it not be easier to just have 4 nodes [old_1, master, old_2, master] and avoid all of the reset/restart logic?

    Maybe even three nodes [old_1, old_2, and master] are sufficient?

  42. in test/functional/wallet_ancient_migration.py:75 in bf86d56a86 
    70+        self.generatetoaddress(self.nodes[0], 1, old_addresses[0], sync_fun=self.no_op)
71+
72+        old_balance = self.nodes[0].getbalance()
73+        old_txs = self.nodes[0].listtransactions("*", 1000)
74+        assert old_balance > 0
75+        assert len(old_txs) > 0
    maflcko commented at 3:04 pm on March 13, 2026:

    I guess the better error message doesn’t matter in this context, but the LLM said:

    Possible places where comparison-specific test macros should replace generic comparisons:

    test/functional/wallet_ancient_migration.py: assert old_balance > 0 -> use assert_greater_than(old_balance, 0)
test/functional/wallet_ancient_migration.py: assert len(old_txs) > 0 -> use assert_greater_than(len(old_txs), 0)

    2026-03-11 20:50:35

    (up to you, feel free to ignore)

  43. maflcko commented at 3:05 pm on March 13, 2026: member

    I think the pull description is outdated?

    The node v0.14.3 cannot be synced because it doesn’t have the syncwithvalidationinterfacequeue RPC implemented (required by test_framework.py), so the solution is to copy the block folder to the modern node and start it with -reindex-chainstate. Because of this additional complexity, this testing is best managed in a separate file rather than in the existing migration test files.

  44. DrahtBot added the label Needs rebase on Mar 13, 2026
  45. w0xlt force-pushed on Mar 17, 2026
  46. test: add wallet migration test for v0.14.3 legacy wallets 
    Test migratewallet RPC on ancient wallet formats from v0.14.3:
- Non-HD wallet (FEATURE_COMPRPUBKEY, version 60000)
- HD single-chain wallet (VERSION_HD_BASE, version 130000)

Verifies balance, transactions, and address ownership are preserved
after migration to descriptor wallets.
    3a4a7b6859
  47. w0xlt force-pushed on Mar 17, 2026
  48. DrahtBot added the label CI failed on Mar 17, 2026
  49. w0xlt commented at 8:02 pm on March 17, 2026: contributor
    PR description updated.
  50. DrahtBot removed the label Needs rebase on Mar 17, 2026
  51. DrahtBot removed the label CI failed on Mar 17, 2026


w0xlt DrahtBot pablomartin4btc maflcko furszy fanquake


pablomartin4btc

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: 2026-03-19 03:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me