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

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

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, furszy

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

w0xlt force-pushed on Aug 13, 2025

DrahtBot added the label CI failed on Aug 13, 2025

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.

w0xlt force-pushed on Aug 13, 2025

Add test to non-HD and single chain wallets 3b1b154c31

w0xlt force-pushed on Aug 13, 2025

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,

pablomartin4btc commented at 7:53 pm on August 13, 2025: member

Concept ACK

Left a couple of suggestions.

in test/functional/test_runner.py:433 in 3b1b154c31

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.

DrahtBot removed the label CI failed on Aug 13, 2025

in test/functional/wallet_ancient_migration.py:167 in 3b1b154c31

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+

maflcko commented at 7:44 am on August 18, 2025:

I think you may be able to skip this by using dumb_sync_blocks from https://github.com/bitcoin/bitcoin/commit/c847dee1488a294c9a9632a00ba1134b21e41947#diff-e48d83ef13f93a84b5686eb797eeed80dd194cf39db7f2972cf82a5ed25415a7L78-L95

fanquake commented at 10:52 am on December 3, 2025: member

@achow101 can you leave a conceptual review?

furszy commented at 2:58 pm on December 3, 2025: member

Concept ACK, will review soon

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)

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

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.

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

Code Coverage & Benchmarks

Reviews