Instead of failing during initialization and shutting down the app when encountering a legacy wallet, skip loading the wallet and notify the user accordingly.
This allows users to access migration functionalities without needing to manually remove the wallet from settings.json or resort to using the bitcoin-wallet utility.
This means that GUI users will be able to use the migration button, and bitcoin-cli users will be able to call the migratewallet RPC directly after init.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32449.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | pablomartin4btc, hebasto |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
281@@ -282,7 +282,13 @@ std::shared_ptr<CWallet> LoadWalletInternal(WalletContext& context, const std::s
282 context.chain->initMessage(_("Loading wallet…"));
283 std::shared_ptr<CWallet> wallet = CWallet::Create(context, name, std::move(database), options.create_flags, error, warnings);
284 if (!wallet) {
285- error = Untranslated("Wallet loading failed.") + Untranslated(" ") + error;
286+ bilingual_str output;
287+ if (!error.empty()) output = error;
288+ if (!warnings.empty()) {
289+ if (!output.empty()) output += Untranslated("\n\n");
290+ output = util::Join(warnings, Untranslated("\n"));
In 86f05d81a12851ab68c33c2447a000cce52ee25c “wallet: init, don’t error out when loading legacy wallets”
output is overwritten here, given the previous line, I assume it’s supposed to be appended to?
wallet_backwards_compatibility.py.
2879@@ -2874,7 +2880,7 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
2880 "The wallet might have been tampered with or created with malicious intent.\n"), walletFile);
2881 return nullptr;
2882 } else if (nLoadWalletRet == DBErrors::LEGACY_WALLET) {
2883- error = strprintf(_("Error loading %s: Wallet is a legacy wallet. Please migrate to a descriptor wallet using the migration tool (migratewallet RPC)."), walletFile);
2884+ warnings.push_back(strprintf(_("Failed to load legacy wallet '%s'.\n\nPlease migrate to a descriptor wallet using the migration tool (migratewallet RPC or the GUI option)."), walletFile));
nit: it’s not a failure on a load, it’s not allowed/ possible anymore… just a suggestion, could be another text message… (also checking a flag to send the warning only once… or with the list of all of them if many)
0 warnings.push_back(strprintf(_("There are legacy wallets in settings.json which are no longer supported.\n\nPlease migrate to a descriptor wallet using the migration tool (migratewallet RPC or the GUI option)."), walletFile));
Concept ACK
You could have a few/ several legacy wallets on the settings.json. Perhaps list them all together on a warning or just warn that there is at least one (on the GUI you would get a pop up on each otherwise).
136@@ -137,7 +137,14 @@ bool LoadWallets(WalletContext& context)
137 }
138 chain.initMessage(_("Loading wallet…"));
139 std::shared_ptr<CWallet> pwallet = database ? CWallet::Create(context, name, std::move(database), options.create_flags, error, warnings) : nullptr;
140- if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n")));
141+ if (!warnings.empty()) {
142+ chain.initWarning(Join(warnings, Untranslated("\n")));
143+ // If there were no errors but the wallet is still null, it means the wallet is being skipped for compatibility reasons.
Instead of failing during initialization when encountering a legacy wallet, skip
loading the wallet and notify the user accordingly.
This allows users to access migration functionalities without needing to manually
remove the wallet from settings.json or resort to using the bitcoin-wallet utility.
This means that GUI users will be able to use the migration button, and bitcoin-cli
users will be able to call the migratewallet RPC directly after init.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/42313088669
LLM reason (✨ experimental): The CI failure is due to a Python linting error (unused import) detected by ruff.
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.
181+ with settings_path.open("w") as fp:
182+ json.dump({"wallet": [wallet_name]}, fp)
183+
184+ # Restart latest node and verify that the legacy wallet load is skipped without exiting early during init.
185+ # TODO: Assert node warns about the legacy wallet not being loaded
186+ # (need to create a function similar to 'assert_raises_rpc_error' but for warning messages).
Updated per feedback. Additional test coverage included.
Is it worth it to verify that, previous to this PR, the node with the legacy wallet in its settings.json was failing to restart (using assert_start_raises_init_error)?
Is it worth it to verify that, previous to this PR, the node with the legacy wallet in its
settings.jsonwas failing to restart (usingassert_start_raises_init_error)?
That’s what is being tested. You can run the test on master and it will fail due to that behavior.
184+ # Restart latest node and verify that the legacy wallet load is skipped without exiting early during init.
185+ # TODO: Assert node warns about the legacy wallet not being loaded
186+ # (need to create a function similar to 'assert_raises_rpc_error' but for warning messages).
187+ self.restart_node(1, extra_args=[])
188+ # Verify node is active after restart
189+ self.nodes[1].getblockcount()
nit: I’d use the same ref to the node_master as above…
0 self.restart_node(node_master.index, extra_args=[])
1 # Verify node is active after restart
2 node_master.getblockcount()
182+ json.dump({"wallet": [wallet_name]}, fp)
183+
184+ # Restart latest node and verify that the legacy wallet load is skipped without exiting early during init.
185+ # TODO: Assert node warns about the legacy wallet not being loaded
186+ # (need to create a function similar to 'assert_raises_rpc_error' but for warning messages).
187+ self.restart_node(1, extra_args=[])
I think something like this (or within a function called assert_restart_raises_init_warning):
0 # Restart latest node and verify that the legacy wallet load is skipped with a warning during init.
1 with node_master.assert_debug_log([f"Failed to open database path '{wallet_path}'. The wallet appears to be a Legacy wallet, please use the wallet migration tool (migratewallet RPC or the GUI option)."]):
2 self.restart_node(node_master.index, extra_args=[])
self.stop_node with the warning in it that would do (I’ve check this is also done in a few .py’s - eg feature_config_args.py- and the buffer cleanup is done in mining_mainnet). Also because then you start the node without the settings, leaving it as before the test, could be cleaner, but up to you, it looks ok this way as well.
It seems that wallet_backwards_compatibility.py is being skipped in the CIs?
And I got this error when I run it locally:
02025-05-16T13:44:41.540000Z TestFramework (INFO): Testing inactive hd chain bad derivation path cleanup
12025-05-16T13:44:43.146000Z TestFramework (INFO): Test that legacy wallets are ignored during startup on v29+
22025-05-16T13:44:43.592000Z TestFramework (INFO): Stopping nodes
3Traceback (most recent call last):
4 File "/home/pablo/workspace/bitcoin/./build/test/functional/wallet_backwards_compatibility.py", line 417, in <module>
5 BackwardsCompatibilityTest(__file__).main()
6 File "/home/pablo/workspace/bitcoin/test/functional/test_framework/test_framework.py", line 207, in main
7 exit_code = self.shutdown()
8 File "/home/pablo/workspace/bitcoin/test/functional/test_framework/test_framework.py", line 366, in shutdown
9 self.stop_nodes()
10 File "/home/pablo/workspace/bitcoin/test/functional/test_framework/test_framework.py", line 615, in stop_nodes
11 node.wait_until_stopped()
12 File "/home/pablo/workspace/bitcoin/test/functional/test_framework/test_node.py", line 443, in wait_until_stopped
13 self.wait_until(lambda: self.is_node_stopped(**kwargs), timeout=timeout)
14 File "/home/pablo/workspace/bitcoin/test/functional/test_framework/test_node.py", line 845, in wait_until
15 return wait_until_helper_internal(test_function, timeout=timeout, timeout_factor=self.timeout_factor, check_interval=check_interval)
16 File "/home/pablo/workspace/bitcoin/test/functional/test_framework/util.py", line 314, in wait_until_helper_internal
17 if predicate():
18 File "/home/pablo/workspace/bitcoin/test/functional/test_framework/test_node.py", line 443, in <lambda>
19 self.wait_until(lambda: self.is_node_stopped(**kwargs), timeout=timeout)
20 File "/home/pablo/workspace/bitcoin/test/functional/test_framework/test_node.py", line 428, in is_node_stopped
21 raise AssertionError("Unexpected stderr {} != {}".format(stderr, expected_stderr))
22AssertionError: Unexpected stderr Warning: Failed to open database path '/tmp/bitcoin_func_test_123k7vzt/node1/regtest/wallets/legacy_up_250000'. The wallet appears to be a Legacy wallet, please use the wallet migration tool (migratewallet RPC or the GUI option). !=
23[node 7] Cleaning up leftover process
24[node 6] Cleaning up leftover process
25[node 5] Cleaning up leftover process
26[node 4] Cleaning up leftover process
27[node 3] Cleaning up leftover process
28[node 2] Cleaning up leftover process
29[node 1] Cleaning up leftover process
I think it needs to be stopped manually before the added test test_ignore_legacy_during_startup ends with:
self.stop_node(node_master.index, f"Warning: Failed to open database path '{wallet_path}'. The wallet appears to be a Legacy wallet, please use the wallet migration tool (migratewallet RPC or the GUI option).")
Then the node can be started (the settings were cleared) manually again before returning to the main function so it can be used later and the final stop_nodes won’t break as above.
It seems that wallet_backwards_compatibility.py is being skipped in the CIs?
It is run in the previous releases CI, which is the one that is failing in this PR: https://github.com/bitcoin/bitcoin/runs/42313448794.
Is it worth it to verify that, previous to this PR, the node with the legacy wallet in its
settings.jsonwas failing to restart (usingassert_start_raises_init_error)?That’s what is being tested. You can run the test on master and it will fail due to that behavior.
I meant without switching to master and once this PR gets merged, I thought there was a release with this failure and your fix will be in the next one, but both changes will be part of the same release so the answer to my previous question would be NO, ha.
161@@ -161,6 +162,41 @@ def test_v22_inactivehdchain_path(self):
162 except ImportError:
163 self.log.warning("sqlite3 module not available, skipping lack of keymeta records check")
164
165+ def test_ignore_legacy_during_startup(self, legacy_nodes, node_master):
166+ if not node_master.version_is_at_least(290000):
167+ return # do not execute test on previous releases
master can never be less than 29
168+
169+ self.log.info("Test that legacy wallets are ignored during startup on v29+")
170+
171+ legacy_node = legacy_nodes[0]
172+ wallet_name = f"legacy_up_{legacy_node.version}"
173+ legacy_node.rpc.loadwallet(wallet_name)
.rpc.?
why the
.rpc.?
because my subconscious still thinks about the legacy wallet proxy methods and we don’t have them anymore. Removed thx
178+
179+ # Write wallet so it is automatically loaded during init
180+ settings_path = node_master.chain_path / "settings.json"
181+ with settings_path.open("w") as fp:
182+ json.dump({"wallet": [wallet_name]}, fp)
183+
In 86e1111239cdb39dd32cfb5178653c608fa30515 “test: verify node skips loading legacy wallets during startup”
Is there a reason to use settings.json instead of passing in -wallet=legacy_up... as part of extra_args during the restart?
Is there a reason to use settings.json instead of passing in
-wallet=legacy_up...as part ofextra_argsduring the restart?
I just wanted to make the test case as realistic as possible (an existing legacy wallet with load_on_startup=true). The three extra lines of code made the decision easy. But there shouldn’t be any difference.
Labels
Wallet
Milestone
30.0