From 9f94de5bb54ff683bd4d3a7723617b34a4706bb6 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 8 May 2025 11:54:47 -0400 Subject: [PATCH 1/2] 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. --- src/wallet/db.h | 1 + src/wallet/load.cpp | 13 +++++++++++-- src/wallet/rpc/util.cpp | 1 + src/wallet/walletdb.cpp | 4 ++-- test/functional/wallet_backwards_compatibility.py | 2 +- 5 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/wallet/db.h b/src/wallet/db.h index 7870dcc4b22..f494a71552f 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -184,6 +184,7 @@ enum class DatabaseStatus { SUCCESS, FAILED_BAD_PATH, FAILED_BAD_FORMAT, + FAILED_LEGACY_DISABLED, FAILED_ALREADY_LOADED, FAILED_ALREADY_EXISTS, FAILED_NOT_FOUND, diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index 3fdad7d6fb1..fbcc5c3aed4 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -99,6 +99,10 @@ bool VerifyWallets(WalletContext& context) if (!MakeWalletDatabase(wallet_file, options, status, error_string)) { if (status == DatabaseStatus::FAILED_NOT_FOUND) { chain.initWarning(Untranslated(strprintf("Skipping -wallet path that doesn't exist. %s", error_string.original))); + } else if (status == DatabaseStatus::FAILED_LEGACY_DISABLED) { + // Skipping legacy wallets as they will not be loaded. + // This will be properly communicated to the user during the loading process. + continue; } else { chain.initError(error_string); return false; @@ -132,8 +136,13 @@ bool LoadWallets(WalletContext& context) bilingual_str error; std::vector warnings; std::unique_ptr database = MakeWalletDatabase(name, options, status, error); - if (!database && status == DatabaseStatus::FAILED_NOT_FOUND) { - continue; + if (!database) { + if (status == DatabaseStatus::FAILED_NOT_FOUND) continue; + if (status == DatabaseStatus::FAILED_LEGACY_DISABLED) { + // Inform user that legacy wallet is not loaded and suggest upgrade options + chain.initWarning(error); + continue; + } } chain.initMessage(_("Loading wallet…")); std::shared_ptr pwallet = database ? CWallet::Create(context, name, std::move(database), options.create_flags, error, warnings) : nullptr; diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp index 219378cfd44..a840a657f5f 100644 --- a/src/wallet/rpc/util.cpp +++ b/src/wallet/rpc/util.cpp @@ -123,6 +123,7 @@ void HandleWalletError(const std::shared_ptr wallet, DatabaseStatus& st switch (status) { case DatabaseStatus::FAILED_NOT_FOUND: case DatabaseStatus::FAILED_BAD_FORMAT: + case DatabaseStatus::FAILED_LEGACY_DISABLED: code = RPC_WALLET_NOT_FOUND; break; case DatabaseStatus::FAILED_ALREADY_LOADED: diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index b06787a5b36..12e92cb43e0 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1383,8 +1383,8 @@ std::unique_ptr MakeDatabase(const fs::path& path, const Databas // BERKELEY_RO can only be opened if require_format was set, which only occurs in migration. if (format && format == DatabaseFormat::BERKELEY_RO && (!options.require_format || options.require_format != DatabaseFormat::BERKELEY_RO)) { - error = Untranslated(strprintf("Failed to open database path '%s'. The wallet appears to be a Legacy wallet, please use the wallet migration tool (migratewallet RPC).", fs::PathToString(path))); - status = DatabaseStatus::FAILED_BAD_FORMAT; + error = Untranslated(strprintf("Failed to open database path '%s'. The wallet appears to be a Legacy wallet, please use the wallet migration tool (migratewallet RPC or the GUI option).", fs::PathToString(path))); + status = DatabaseStatus::FAILED_LEGACY_DISABLED; return nullptr; } diff --git a/test/functional/wallet_backwards_compatibility.py b/test/functional/wallet_backwards_compatibility.py index 62d28abcb3c..bb34003e9c4 100755 --- a/test/functional/wallet_backwards_compatibility.py +++ b/test/functional/wallet_backwards_compatibility.py @@ -378,7 +378,7 @@ class BackwardsCompatibilityTest(BitcoinTestFramework): # Restore the wallet to master # Legacy wallets are no longer supported. Trying to load these should result in an error - assert_raises_rpc_error(-18, "The wallet appears to be a Legacy wallet, please use the wallet migration tool (migratewallet RPC)", node_master.restorewallet, wallet_name, backup_path) + assert_raises_rpc_error(-18, "The wallet appears to be a Legacy wallet, please use the wallet migration tool (migratewallet RPC or the GUI option)", node_master.restorewallet, wallet_name, backup_path) self.test_v22_inactivehdchain_path() From 86e1111239cdb39dd32cfb5178653c608fa30515 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 12 May 2025 20:46:15 -0400 Subject: [PATCH 2/2] test: verify node skips loading legacy wallets during startup --- .../wallet_backwards_compatibility.py | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/functional/wallet_backwards_compatibility.py b/test/functional/wallet_backwards_compatibility.py index bb34003e9c4..864eed5a56a 100755 --- a/test/functional/wallet_backwards_compatibility.py +++ b/test/functional/wallet_backwards_compatibility.py @@ -14,6 +14,7 @@ Use only the latest patch version of each release, unless a test specifically needs an older patch version. """ +import json import os import shutil @@ -161,6 +162,38 @@ class BackwardsCompatibilityTest(BitcoinTestFramework): except ImportError: self.log.warning("sqlite3 module not available, skipping lack of keymeta records check") + def test_ignore_legacy_during_startup(self, legacy_nodes, node_master): + self.log.info("Test that legacy wallets are ignored during startup on v29+") + + legacy_node = legacy_nodes[0] + wallet_name = f"legacy_up_{legacy_node.version}" + legacy_node.loadwallet(wallet_name) + legacy_wallet = legacy_node.get_wallet_rpc(wallet_name) + + # Move legacy wallet to latest node + wallet_path = node_master.wallets_path / wallet_name + wallet_path.mkdir() + legacy_wallet.backupwallet(wallet_path / "wallet.dat") + legacy_wallet.unloadwallet() + + # Write wallet so it is automatically loaded during init + settings_path = node_master.chain_path / "settings.json" + with settings_path.open("w") as fp: + json.dump({"wallet": [wallet_name]}, fp) + + # Restart latest node and verify that the legacy wallet load is skipped without exiting early during init. + self.restart_node(node_master.index, extra_args=[]) + # Ensure we receive the warning message and clear the stderr pipe. + node_master.stderr.seek(0) + warning_msg = node_master.stderr.read().decode('utf-8').strip() + assert "The wallet appears to be a Legacy wallet, please use the wallet migration tool (migratewallet RPC or the GUI option)" in warning_msg + node_master.stderr.truncate(0), node_master.stderr.seek(0) # reset buffer + + # Verify the node is still running (no shutdown occurred during startup) + node_master.getblockcount() + # Reset settings for any subsequent test + os.remove(settings_path) + def run_test(self): node_miner = self.nodes[0] node_master = self.nodes[1] @@ -381,6 +414,7 @@ class BackwardsCompatibilityTest(BitcoinTestFramework): assert_raises_rpc_error(-18, "The wallet appears to be a Legacy wallet, please use the wallet migration tool (migratewallet RPC or the GUI option)", node_master.restorewallet, wallet_name, backup_path) self.test_v22_inactivehdchain_path() + self.test_ignore_legacy_during_startup(legacy_nodes, node_master) if __name__ == '__main__': BackwardsCompatibilityTest(__file__).main()