wallet: init, don't error out when loading legacy wallets

furszy commented at 4:27 pm on May 8, 2025: member

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.

DrahtBot commented at 4:27 pm on May 8, 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/32449.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, w0xlt
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.

DrahtBot added the label Wallet on May 8, 2025

in src/wallet/wallet.cpp:289 in 86f05d81a1 outdated

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"));

achow101 commented at 6:14 pm on May 8, 2025:

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?

furszy commented at 6:35 pm on May 8, 2025:

upps, yeah. Fixed.

furszy force-pushed on May 8, 2025

achow101 commented at 11:02 pm on May 8, 2025: member

I think we could have a test for this in wallet_backwards_compatibility.py.

maflcko added this to the milestone 30.0 on May 9, 2025

in src/wallet/wallet.cpp:2883 in 14cec40301 outdated

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));

pablomartin4btc commented at 3:56 pm on May 9, 2025:

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));

pablomartin4btc commented at 3:58 pm on May 9, 2025: member

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

in src/wallet/load.cpp:142 in 14cec40301 outdated

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.

1440000bytes commented at 9:58 pm on May 11, 2025:

Would this look better with a multi line comment?

hebasto commented at 11:56 am on May 12, 2025: member

Concept ACK.

furszy force-pushed on May 15, 2025

wallet: init, don't error out when loading legacy wallets

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.

9f94de5bb5

furszy force-pushed on May 15, 2025

DrahtBot added the label CI failed on May 15, 2025

DrahtBot commented at 7:15 pm on May 15, 2025: contributor

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

furszy force-pushed on May 15, 2025

furszy commented at 7:20 pm on May 15, 2025: member

Updated per feedback. Additional test coverage included.

in test/functional/wallet_backwards_compatibility.py:186 in 2f4f04c26d outdated

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

pablomartin4btc commented at 7:35 pm on May 15, 2025:

I think similar to this one “assert_start_raises_init_error”… no?

furszy commented at 7:53 pm on May 15, 2025:

yes, but without expecting a shutdown.

pablomartin4btc commented at 3:13 pm on May 16, 2025:

I’ve left a suggestion.

pablomartin4btc commented at 7:42 pm on May 15, 2025: member

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)?

furszy commented at 7:57 pm on May 15, 2025: member

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)?

That’s what is being tested. You can run the test on master and it will fail due to that behavior.

in test/functional/wallet_backwards_compatibility.py:189 in 2f4f04c26d outdated

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()

pablomartin4btc commented at 8:56 am on May 16, 2025:

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()

furszy commented at 4:24 pm on May 16, 2025:

sure, updated.

in test/functional/wallet_backwards_compatibility.py:187 in 2f4f04c26d outdated

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=[])

pablomartin4btc commented at 2:41 pm on May 16, 2025:

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=[])

furszy commented at 4:26 pm on May 16, 2025:

Would have liked to do this, but it’s not enough. The stderr pipe still needs to be cleared before the test finishes, or the framework will treat the warning message as an error during cleanup.

pablomartin4btc commented at 6:08 pm on May 16, 2025:

If you do the 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.

furszy commented at 6:45 pm on May 16, 2025:

Created #32538 as an alternative approach. Warning messages could be redirected to stdout instead of being sent through stderr. I’m not seeing a reason to treat them as errors.

pablomartin4btc commented at 3:09 pm on May 16, 2025: member

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.

fanquake commented at 3:11 pm on May 16, 2025: member

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.

pablomartin4btc commented at 3:11 pm on May 16, 2025: member

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)?

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.

furszy commented at 3:20 pm on May 16, 2025: member

The issue is caused by the stderr warning message not being consumed. The framework expects stderr to be empty at the end of the test, without accounting for warning messages. Will update the test to clear the buffer after startup.

furszy force-pushed on May 16, 2025

in test/functional/wallet_backwards_compatibility.py:167 in ad0c1cff15 outdated

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

maflcko commented at 6:14 pm on May 16, 2025:

looks like dead code. master can never be less than 29

furszy commented at 7:37 pm on May 16, 2025:

removed.

in test/functional/wallet_backwards_compatibility.py:173 in ad0c1cff15 outdated

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)

maflcko commented at 6:15 pm on May 16, 2025:

why the .rpc.?

furszy commented at 7:37 pm on May 16, 2025:

why the .rpc.?

because my subconscious still thinks about the legacy wallet proxy methods and we don’t have them anymore. Removed thx

test: verify node skips loading legacy wallets during startup 86e1111239

furszy force-pushed on May 16, 2025

in test/functional/wallet_backwards_compatibility.py:183 in 86e1111239

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+

achow101 commented at 10:28 pm on May 16, 2025:

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?

furszy commented at 11:32 pm on May 16, 2025:

Is there a reason to use settings.json instead of passing in -wallet=legacy_up... as part of extra_args during the restart?

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.

DrahtBot removed the label CI failed on May 17, 2025

bitcoin deleted a comment on May 17, 2025

achow101 commented at 8:18 pm on May 26, 2025: member

ACK 86e1111239cdb39dd32cfb5178653c608fa30515

DrahtBot requested review from pablomartin4btc on May 26, 2025

DrahtBot requested review from hebasto on May 26, 2025

w0xlt commented at 10:19 pm on May 28, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/pull/32449/commits/86e1111239cdb39dd32cfb5178653c608fa30515

nit: Perhaps the comment in HandleWalletError could also explain why DatabaseStatus::FAILED_LEGACY_DISABLED is also being mapped to RPC_WALLET_NOT_FOUND. While it seems clear from looking at this PR, it might be a useful for future reference.

wallet: init, don’t error out when loading legacy wallets #32449

Code Coverage & Benchmarks

Reviews