wallet, test: Ancient Wallet Migration from v0.14.3 (no-HD and Single Chain) #33186
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33186. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
224915a to
a305380
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
0116ce0 to
5237377
Compare
5237377 to
3b1b154
Compare
|
|
||
| # Setup fresh nodes for this test | ||
| self.add_nodes( | ||
| 2, |
There was a problem hiding this comment.
| 2, | |
| self.num_nodes, |
| @@ -428,7 +429,12 @@ def main(): | |||
| logging.basicConfig(format='%(message)s', level=logging_level) | |||
|
|
|||
| # Create base test directory | |||
| tmpdir = "%s/test_runner_₿_🏃_%s" % (args.tmpdirprefix, datetime.datetime.now().strftime("%Y%m%d_%H%M%S")) | |||
| # Special case for tests using old binaries (e.g. version v0.14.3) that don't handle Unicode on Windows | |||
| has_migration_tests = any('wallet_ancient_migration' in script for script in BASE_SCRIPTS) | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| has_migration_tests = any('wallet_ancient_migration' in script for script in BASE_SCRIPTS) | |
| requires_ascii_datadir = any('wallet_ancient_migration' in script for script in BASE_SCRIPTS) | |
| tmpdirprefix = "%s/test_runner_₿_🏃_%s" | |
| if platform.system() == 'Windows' and requires_ascii_datadir: | |
| tmpdir = "%s/test_runner_btc_run_%s" | |
| 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
feature_unsupported_utxo_db.pydoesn't copy the directory from one node to another. This test needs to do this because version 0.14.3 doesn't have thesyncwithvalidationinterfacequeueRPC, 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
| # Stop the old node | ||
| self.log.info("Stopping old node...") | ||
| self.stop_node(0) | ||
|
|
||
| # Copy blockchain data from node0 to node1 | ||
| self.log.info("Copying blockchain data from old node to modern node...") | ||
| old_blocks_dir = os.path.join(node0_datadir, self.chain, "blocks") | ||
| new_blocks_dir = os.path.join(node1_datadir, self.chain, "blocks") | ||
|
|
||
| # Clean up any existing blocks directory in node1 | ||
| if os.path.exists(new_blocks_dir): | ||
| shutil.rmtree(new_blocks_dir) | ||
|
|
||
| if os.path.exists(old_blocks_dir): | ||
| shutil.copytree(old_blocks_dir, new_blocks_dir) | ||
| self.log.info(" Copied blocks directory") | ||
| assert(os.path.exists(new_blocks_dir)) | ||
|
|
||
| # Start the modern node with -reindex-chainstate | ||
| self.log.info("Starting modern node with -reindex-chainstate...") | ||
| self.start_node(1, extra_args=["-reindex-chainstate"]) | ||
|
|
||
| # Wait for reindex to complete | ||
| self.log.info("Waiting for chainstate reindex to complete...") | ||
| self.wait_until( | ||
| lambda: self.nodes[1].getblockcount() == 102, | ||
| timeout=30 | ||
| ) | ||
|
|
||
| # Verify the blockchain was loaded correctly | ||
| node1_info = self.nodes[1].getblockchaininfo() | ||
| self.log.info(f"Modern node blockchain info: height={node1_info['blocks']}") | ||
| assert_equal(node1_info['blocks'], 102) | ||
|
|
There was a problem hiding this comment.
I think you may be able to skip this by using dumb_sync_blocks from c847dee#diff-e48d83ef13f93a84b5686eb797eeed80dd194cf39db7f2972cf82a5ed25415a7L78-L95
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:
-usehd=0)VERSION_HD_BASE)The node v0.14.3 cannot be synced because it doesn't have the
syncwithvalidationinterfacequeueRPC implemented (required bytest_framework.py), so the solution is to copy theblockfolder 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.