Merge bitcoin/bitcoin#32449: wallet: init, don't error out when loading legacy wallets

86e1111 test: verify node skips loading legacy wallets during startup (furszy)
9f94de5 wallet: init, don't error out when loading legacy wallets (furszy)

Pull request description:

  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.

ACKs for top commit:
  achow101:
    ACK 86e1111
  w0xlt:
    ACK bitcoin/bitcoin@86e1111

Tree-SHA512: 85d594a503ee7a833a23754b71b6ba4869ca34ed802c9ac0cd7b2fa56978f5fcad84ee4bd3acdcc61cf8e7f08f0789336febc5d76beae1eebf7bd51462512b78

Author: achow101

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,

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<bilingual_str> warnings;
             std::unique_ptr<WalletDatabase> 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<CWallet> pwallet = database ? CWallet::Create(context, name, std::move(database), options.create_flags, error, warnings) : nullptr;

src/wallet/rpc/util.cpp

@@ -123,6 +123,7 @@ void HandleWalletError(const std::shared_ptr<CWallet> 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:

src/wallet/walletdb.cpp

@@ -1390,8 +1390,8 @@ std::unique_ptr<WalletDatabase> 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;
     }
 

test/functional/wallet_backwards_compatibility.py

@@ -14,6 +14,7 @@
 needs an older patch version.
 """
 
+import json
 import os
 import shutil
 
@@ -161,6 +162,38 @@ def test_v22_inactivehdchain_path(self):
         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]
@@ -378,9 +411,10 @@ def run_test(self):
 
             # 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()
+        self.test_ignore_legacy_during_startup(legacy_nodes, node_master)
 
 if __name__ == '__main__':
     BackwardsCompatibilityTest(__file__).main()