Merge bitcoin/bitcoin#32619: wallet, rpc, gui: List legacy wallets with a message about migration

f3a444c45f gui: Disallow loading legacy wallets (Ava Chow)
09955172f3 wallet, rpc: Give warning in listwalletdir for legacy wallets (Ava Chow)

Pull request description:

  A new field `warnings` is added for each wallet in `listwalletdir`. If a legacy wallet is detected, the warning will contain a message that the wallet is a legacy wallet and will need to be migrated before it can be loaded.

  In the GUI, the "Open Wallet" menu is changed to show legacy wallets greyed out with "(needs migration)" appended to their name to indicate to the user that the legacy wallet will need to be migrated.

ACKs for top commit:
  maflcko:
    lgtm ACK f3a444c45f
  adyshimony:
    Test ACK [f3a444c](f3a444c45f)
  furszy:
    Code review ACK f3a444c45f
  w0xlt:
    Code Review ACK f3a444c45f

Tree-SHA512: 496caec0ca37845487bd709e592240315eb23461fbd697e68a7fde8e4d9b74b48aab1212c88dbbcc8a107a896b824c2e1f69691068641812ae903f873fa2f22b

src/qt/bitcoingui.cpp

@@ -398,15 +398,19 @@ void BitcoinGUI::createActions()
         connect(m_open_wallet_menu, &QMenu::aboutToShow, [this] {
             m_open_wallet_menu->clear();
             for (const auto& [path, info] : m_wallet_controller->listWalletDir()) {
-                const auto& [loaded, _] = info;
+                const auto& [loaded, format] = info;
                 QString name = GUIUtil::WalletDisplayName(path);
                 // An single ampersand in the menu item's text sets a shortcut for this item.
                 // Single & are shown when && is in the string. So replace & with &&.
                 name.replace(QChar('&'), QString("&&"));
+                bool is_legacy = format == "bdb";
+                if (is_legacy) {
+                    name += " (needs migration)";
+                }
                 QAction* action = m_open_wallet_menu->addAction(name);
 
-                if (loaded) {
-                    // This wallet is already loaded
+                if (loaded || is_legacy) {
+                    // This wallet is already loaded or it is a legacy wallet
                     action->setEnabled(false);
                     continue;
                 }

src/wallet/rpc/wallet.cpp

@@ -135,6 +135,10 @@ static RPCHelpMan listwalletdir()
                             {RPCResult::Type::OBJ, "", "",
                             {
                                 {RPCResult::Type::STR, "name", "The wallet name"},
+                                {RPCResult::Type::ARR, "warnings", /*optional=*/true, "Warning messages, if any, related to loading the wallet.",
+                                {
+                                    {RPCResult::Type::STR, "", ""},
+                                }},
                             }},
                         }},
                     }
@@ -146,9 +150,14 @@ static RPCHelpMan listwalletdir()
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
     UniValue wallets(UniValue::VARR);
-    for (const auto& [path, _] : ListDatabases(GetWalletDir())) {
+    for (const auto& [path, db_type] : ListDatabases(GetWalletDir())) {
         UniValue wallet(UniValue::VOBJ);
         wallet.pushKV("name", path.utf8string());
+                UniValue warnings(UniValue::VARR);
+        if (db_type == "bdb") {
+            warnings.push_back("This wallet is a legacy wallet and will need to be migrated with migratewallet before it can be loaded");
+        }
+        wallet.pushKV("warnings", warnings);
         wallets.push_back(std::move(wallet));
     }
 

test/functional/wallet_migration.py

@@ -113,6 +113,11 @@ class WalletMigrationTest(BitcoinTestFramework):
             shutil.copyfile(self.old_node.wallets_path / "wallet.dat", self.master_node.wallets_path / "wallet.dat")
         else:
             shutil.copytree(self.old_node.wallets_path / wallet_name, self.master_node.wallets_path / wallet_name)
+        # Check that the wallet shows up in listwalletdir with a warning about migration
+        wallets = self.master_node.listwalletdir()
+        for w in wallets["wallets"]:
+            if w["name"] == wallet_name:
+                assert_equal(w["warnings"], ["This wallet is a legacy wallet and will need to be migrated with migratewallet before it can be loaded"])
         # Migrate, checking that rescan does not occur
         with self.master_node.assert_debug_log(expected_msgs=[], unexpected_msgs=["Rescanning"]):
             migrate_info = self.master_node.migratewallet(wallet_name=wallet_name, **kwargs)
@@ -548,7 +553,7 @@ class WalletMigrationTest(BitcoinTestFramework):
         assert_equal(info["format"], "sqlite")
 
         walletdir_list = wallet.listwalletdir()
-        assert {"name": info["walletname"]} in walletdir_list["wallets"]
+        assert {"name": info["walletname"]} in [{"name": w["name"]} for w in walletdir_list["wallets"]]
 
         # Check backup existence and its non-empty wallet filename
         backup_filename = f"default_wallet_{curr_time}.legacy.bak"

test/functional/wallet_multiwallet.py

@@ -72,7 +72,7 @@ class MultiWalletTest(BitcoinTestFramework):
                 return wallet_dir(name, "wallet.dat")
             return wallet_dir(name)
 
-        assert_equal(self.nodes[0].listwalletdir(), {'wallets': [{'name': self.default_wallet_name}]})
+        assert_equal(self.nodes[0].listwalletdir(), {'wallets': [{'name': self.default_wallet_name, "warnings": []}]})
 
         # check wallet.dat is created
         self.stop_nodes()