Merge #824: Migrate legacy wallets that are not loaded

8f2522d gui: Use menu for wallet migration (Ava Chow)
d56a450 gui: Use wallet name for wallet migration rather than WalletModel (Ava Chow)
c391858 gui: don't remove wallet manually before migration (furszy)
bfba638 gui: Consolidate wallet display name to GUIUtil function (Ava Chow)
28fc562 wallet, interfaces: Include database format in listWalletDir (Ava Chow)

Pull request description:

  Currently the Migrate Wallet menu item can only be used to migrate the currently loaded wallet. This is not suitable for the future when legacy wallets can no longer be loaded at all, but should still be able to be migrated. This PR changes that menu item into a menu list like Open Wallet and lets users migrate any legacy wallet in their wallet directory regardless of the wallets loaded.

  One issue I ran into was dealing with encrypted wallets. Ideally, we would detect whether a wallet is encrypted, and prompt the user for their passphrase at that time. However, that's actually difficult to do in the GUI since migration will unload the wallet if it was already loaded, and reload it without connecting it to any signals or interfaces. Only then can it detect whether a wallet is encrypted, but then there is no `WalletModel` or even an `interfaces::Wallet` that the GUI could use to unlock it via a callback.

  To deal with this, I've opted to just add a button to the migration dialog box that has the user enter their passphrase first, along with instructional text to use that button if their wallet was encrypted. If the user enters the wrong passphrase or clicked the other button that does not prompt for the passphrase, migration will fail with a message indicating that the passphrase was incorrect.

ACKs for top commit:
  hebasto:
    ACK 8f2522d.
  furszy:
    ACK 8f2522d

Tree-SHA512: a0e3b70dbfcacb89617956510ebcea94cad8617a987c68fe39fa16ac1721190b7cf7afc156c39b9032920cfb67b5d4ca28791681f5021d92d16acc691387afa1

src/interfaces/wallet.h

@@ -342,8 +342,11 @@ class WalletLoader : public ChainClient
     //! Migrate a wallet
     virtual util::Result<WalletMigrationResult> migrateWallet(const std::string& name, const SecureString& passphrase) = 0;
 
+    //! Returns true if wallet stores encryption keys
+    virtual bool isEncrypted(const std::string& wallet_name) = 0;
+
     //! Return available wallets in wallet directory.
-    virtual std::vector<std::string> listWalletDir() = 0;
+    virtual std::vector<std::pair<std::string, std::string>> listWalletDir() = 0;
 
     //! Return interfaces for accessing wallets (if any).
     virtual std::vector<std::unique_ptr<Wallet>> getWallets() = 0;

src/qt/askpassphrasedialog.cpp

@@ -44,6 +44,7 @@ AskPassphraseDialog::AskPassphraseDialog(Mode _mode, QWidget *parent, SecureStri
             ui->passEdit1->hide();
             setWindowTitle(tr("Encrypt wallet"));
             break;
+        case UnlockMigration:
         case Unlock: // Ask passphrase
             ui->warningLabel->setText(tr("This operation needs your wallet passphrase to unlock the wallet."));
             ui->passLabel2->hide();
@@ -80,7 +81,7 @@ void AskPassphraseDialog::setModel(WalletModel *_model)
 void AskPassphraseDialog::accept()
 {
     SecureString oldpass, newpass1, newpass2;
-    if (!model && mode != Encrypt)
+    if (!model && mode != Encrypt && mode != UnlockMigration)
         return;
     oldpass.reserve(MAX_PASSPHRASE_SIZE);
     newpass1.reserve(MAX_PASSPHRASE_SIZE);
@@ -181,6 +182,10 @@ void AskPassphraseDialog::accept()
             QMessageBox::critical(this, tr("Wallet unlock failed"), e.what());
         }
         break;
+    case UnlockMigration:
+        Assume(m_passphrase_out)->assign(oldpass);
+        QDialog::accept();
+        break;
     case ChangePass:
         if(newpass1 == newpass2)
         {
@@ -224,6 +229,7 @@ void AskPassphraseDialog::textChanged()
     case Encrypt: // New passphrase x2
         acceptable = !ui->passEdit2->text().isEmpty() && !ui->passEdit3->text().isEmpty();
         break;
+    case UnlockMigration:
     case Unlock: // Old passphrase x1
         acceptable = !ui->passEdit1->text().isEmpty();
         break;

src/qt/askpassphrasedialog.h

@@ -26,6 +26,7 @@ class AskPassphraseDialog : public QDialog
         Encrypt,    /**< Ask passphrase twice and encrypt */
         Unlock,     /**< Ask passphrase and unlock */
         ChangePass, /**< Ask old passphrase + new passphrase twice */
+        UnlockMigration, /**< Ask passphrase for unlocking during migration */
     };
 
     explicit AskPassphraseDialog(Mode mode, QWidget *parent, SecureString* passphrase_out = nullptr);

src/qt/bitcoingui.cpp

@@ -360,6 +360,7 @@ void BitcoinGUI::createActions()
     m_migrate_wallet_action = new QAction(tr("Migrate Wallet"), this);
     m_migrate_wallet_action->setEnabled(false);
     m_migrate_wallet_action->setStatusTip(tr("Migrate a wallet"));
+    m_migrate_wallet_menu = new QMenu(this);
 
     showHelpMessageAction = new QAction(tr("&Command-line options"), this);
     showHelpMessageAction->setMenuRole(QAction::NoRole);
@@ -396,15 +397,15 @@ void BitcoinGUI::createActions()
         connect(openAction, &QAction::triggered, this, &BitcoinGUI::openClicked);
         connect(m_open_wallet_menu, &QMenu::aboutToShow, [this] {
             m_open_wallet_menu->clear();
-            for (const std::pair<const std::string, bool>& i : m_wallet_controller->listWalletDir()) {
-                const std::string& path = i.first;
-                QString name = path.empty() ? QString("["+tr("default wallet")+"]") : QString::fromStdString(path);
+            for (const auto& [path, info] : m_wallet_controller->listWalletDir()) {
+                const auto& [loaded, _] = 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("&&"));
                 QAction* action = m_open_wallet_menu->addAction(name);
 
-                if (i.second) {
+                if (loaded) {
                     // This wallet is already loaded
                     action->setEnabled(false);
                     continue;
@@ -455,10 +456,31 @@ void BitcoinGUI::createActions()
         connect(m_close_all_wallets_action, &QAction::triggered, [this] {
             m_wallet_controller->closeAllWallets(this);
         });
-        connect(m_migrate_wallet_action, &QAction::triggered, [this] {
-            auto activity = new MigrateWalletActivity(m_wallet_controller, this);
-            connect(activity, &MigrateWalletActivity::migrated, this, &BitcoinGUI::setCurrentWallet);
-            activity->migrate(walletFrame->currentWalletModel());
+        connect(m_migrate_wallet_menu, &QMenu::aboutToShow, [this] {
+            m_migrate_wallet_menu->clear();
+            for (const auto& [wallet_name, info] : m_wallet_controller->listWalletDir()) {
+                const auto& [loaded, format] = info;
+
+                if (format != "bdb") { // Skip already migrated wallets
+                    continue;
+                }
+
+                QString name = GUIUtil::WalletDisplayName(wallet_name);
+                // 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("&&"));
+                QAction* action = m_migrate_wallet_menu->addAction(name);
+
+                connect(action, &QAction::triggered, [this, wallet_name] {
+                    auto activity = new MigrateWalletActivity(m_wallet_controller, this);
+                    connect(activity, &MigrateWalletActivity::migrated, this, &BitcoinGUI::setCurrentWallet);
+                    activity->migrate(wallet_name);
+                });
+            }
+            if (m_migrate_wallet_menu->isEmpty()) {
+                QAction* action = m_migrate_wallet_menu->addAction(tr("No wallets available"));
+                action->setEnabled(false);
+            }
         });
         connect(m_mask_values_action, &QAction::toggled, this, &BitcoinGUI::setPrivacy);
         connect(m_mask_values_action, &QAction::toggled, this, &BitcoinGUI::enableHistoryAction);
@@ -691,6 +713,8 @@ void BitcoinGUI::setWalletController(WalletController* wallet_controller, bool s
     m_open_wallet_action->setEnabled(true);
     m_open_wallet_action->setMenu(m_open_wallet_menu);
     m_restore_wallet_action->setEnabled(true);
+    m_migrate_wallet_action->setEnabled(true);
+    m_migrate_wallet_action->setMenu(m_migrate_wallet_menu);
 
     GUIUtil::ExceptionSafeConnect(wallet_controller, &WalletController::walletAdded, this, &BitcoinGUI::addWallet);
     connect(wallet_controller, &WalletController::walletRemoved, this, &BitcoinGUI::removeWallet);
@@ -771,7 +795,6 @@ void BitcoinGUI::setCurrentWallet(WalletModel* wallet_model)
         }
     }
     updateWindowTitle();
-    m_migrate_wallet_action->setEnabled(wallet_model->wallet().isLegacy());
 }
 
 void BitcoinGUI::setCurrentWalletBySelectorIndex(int index)
@@ -805,7 +828,6 @@ void BitcoinGUI::setWalletActionsEnabled(bool enabled)
     openAction->setEnabled(enabled);
     m_close_wallet_action->setEnabled(enabled);
     m_close_all_wallets_action->setEnabled(enabled);
-    m_migrate_wallet_action->setEnabled(enabled);
 }
 
 void BitcoinGUI::createTrayIcon()

src/qt/guiutil.cpp

@@ -1008,4 +1008,13 @@ void ShowModalDialogAsynchronously(QDialog* dialog)
     dialog->show();
 }
 
+QString WalletDisplayName(const QString& name)
+{
+    return name.isEmpty() ? "[" + QObject::tr("default wallet") + "]" : name;
+}
+
+QString WalletDisplayName(const std::string& name)
+{
+    return WalletDisplayName(QString::fromStdString(name));
+}
 } // namespace GUIUtil

src/qt/guiutil.h

@@ -436,6 +436,9 @@ namespace GUIUtil
         return false;
     }
 
+    QString WalletDisplayName(const std::string& name);
+    QString WalletDisplayName(const QString& name);
+
 } // namespace GUIUtil
 
 #endif // BITCOIN_QT_GUIUTIL_H

src/qt/walletcontroller.cpp

@@ -65,16 +65,16 @@ WalletController::~WalletController()
     delete m_activity_worker;
 }
 
-std::map<std::string, bool> WalletController::listWalletDir() const
+std::map<std::string, std::pair<bool, std::string>> WalletController::listWalletDir() const
 {
     QMutexLocker locker(&m_mutex);
-    std::map<std::string, bool> wallets;
-    for (const std::string& name : m_node.walletLoader().listWalletDir()) {
-        wallets[name] = false;
+    std::map<std::string, std::pair<bool, std::string>> wallets;
+    for (const auto& [name, format] : m_node.walletLoader().listWalletDir()) {
+        wallets[name] = std::make_pair(false, format);
     }
     for (WalletModel* wallet_model : m_wallets) {
         auto it = wallets.find(wallet_model->wallet().getWalletName());
-        if (it != wallets.end()) it->second = true;
+        if (it != wallets.end()) it->second.first = true;
     }
     return wallets;
 }
@@ -150,9 +150,10 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal
     assert(called);
 
     connect(wallet_model, &WalletModel::unload, this, [this, wallet_model] {
-        // Defer removeAndDeleteWallet when no modal widget is active.
+        // Defer removeAndDeleteWallet when no modal widget is actively waiting for an action.
         // TODO: remove this workaround by removing usage of QDialog::exec.
-        if (QApplication::activeModalWidget()) {
+        QWidget* active_dialog = QApplication::activeModalWidget();
+        if (active_dialog && dynamic_cast<QProgressDialog*>(active_dialog) == nullptr) {
             connect(qApp, &QApplication::focusWindowChanged, wallet_model, [this, wallet_model]() {
                 if (!QApplication::activeModalWidget()) {
                     removeAndDeleteWallet(wallet_model);
@@ -343,7 +344,7 @@ void OpenWalletActivity::finish()
 
 void OpenWalletActivity::open(const std::string& path)
 {
-    QString name = path.empty() ? QString("["+tr("default wallet")+"]") : QString::fromStdString(path);
+    QString name = GUIUtil::WalletDisplayName(path);
 
     showProgressDialog(
         //: Title of window indicating the progress of opening of a wallet.
@@ -436,12 +437,12 @@ void RestoreWalletActivity::finish()
     Q_EMIT finished();
 }
 
-void MigrateWalletActivity::migrate(WalletModel* wallet_model)
+void MigrateWalletActivity::migrate(const std::string& name)
 {
     // Warn the user about migration
     QMessageBox box(m_parent_widget);
     box.setWindowTitle(tr("Migrate wallet"));
-    box.setText(tr("Are you sure you wish to migrate the wallet <i>%1</i>?").arg(GUIUtil::HtmlEscape(wallet_model->getDisplayName())));
+    box.setText(tr("Are you sure you wish to migrate the wallet <i>%1</i>?").arg(GUIUtil::HtmlEscape(GUIUtil::WalletDisplayName(name))));
     box.setInformativeText(tr("Migrating the wallet will convert this wallet to one or more descriptor wallets. A new wallet backup will need to be made.\n"
                 "If this wallet contains any watchonly scripts, a new wallet will be created which contains those watchonly scripts.\n"
                 "If this wallet contains any solvable but not watched scripts, a different and new wallet will be created which contains those scripts.\n\n"
@@ -452,31 +453,25 @@ void MigrateWalletActivity::migrate(WalletModel* wallet_model)
     box.setDefaultButton(QMessageBox::Yes);
     if (box.exec() != QMessageBox::Yes) return;
 
-    // Get the passphrase if it is encrypted regardless of it is locked or unlocked. We need the passphrase itself.
     SecureString passphrase;
-    WalletModel::EncryptionStatus enc_status = wallet_model->getEncryptionStatus();
-    if (enc_status == WalletModel::EncryptionStatus::Locked || enc_status == WalletModel::EncryptionStatus::Unlocked) {
-        AskPassphraseDialog dlg(AskPassphraseDialog::Unlock, m_parent_widget, &passphrase);
-        dlg.setModel(wallet_model);
-        dlg.exec();
+    if (node().walletLoader().isEncrypted(name)) {
+        // Get the passphrase for the wallet
+        AskPassphraseDialog dlg(AskPassphraseDialog::UnlockMigration, m_parent_widget, &passphrase);
+        if (dlg.exec() == QDialog::Rejected) return;
     }
 
-    // GUI needs to remove the wallet so that it can actually be unloaded by migration
-    const std::string name = wallet_model->wallet().getWalletName();
-    m_wallet_controller->removeAndDeleteWallet(wallet_model);
-
     showProgressDialog(tr("Migrate Wallet"), tr("Migrating Wallet <b>%1</b>…").arg(GUIUtil::HtmlEscape(name)));
 
     QTimer::singleShot(0, worker(), [this, name, passphrase] {
         auto res{node().walletLoader().migrateWallet(name, passphrase)};
 
         if (res) {
-            m_success_message = tr("The wallet '%1' was migrated successfully.").arg(GUIUtil::HtmlEscape(res->wallet->getWalletName()));
+            m_success_message = tr("The wallet '%1' was migrated successfully.").arg(GUIUtil::HtmlEscape(GUIUtil::WalletDisplayName(res->wallet->getWalletName())));
             if (res->watchonly_wallet_name) {
-                m_success_message += QChar(' ') + tr("Watchonly scripts have been migrated to a new wallet named '%1'.").arg(GUIUtil::HtmlEscape(res->watchonly_wallet_name.value()));
+                m_success_message += QChar(' ') + tr("Watchonly scripts have been migrated to a new wallet named '%1'.").arg(GUIUtil::HtmlEscape(GUIUtil::WalletDisplayName(res->watchonly_wallet_name.value())));
             }
             if (res->solvables_wallet_name) {
-                m_success_message += QChar(' ') + tr("Solvable but not watched scripts have been migrated to a new wallet named '%1'.").arg(GUIUtil::HtmlEscape(res->solvables_wallet_name.value()));
+                m_success_message += QChar(' ') + tr("Solvable but not watched scripts have been migrated to a new wallet named '%1'.").arg(GUIUtil::HtmlEscape(GUIUtil::WalletDisplayName(res->solvables_wallet_name.value())));
             }
             m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(res->wallet));
         } else {

src/qt/walletcontroller.h

@@ -61,13 +61,11 @@ class WalletController : public QObject
 
     //! Returns all wallet names in the wallet dir mapped to whether the wallet
     //! is loaded.
-    std::map<std::string, bool> listWalletDir() const;
+    std::map<std::string, std::pair<bool, std::string>> listWalletDir() const;
 
     void closeWallet(WalletModel* wallet_model, QWidget* parent = nullptr);
     void closeAllWallets(QWidget* parent = nullptr);
 
-    void migrateWallet(WalletModel* wallet_model, QWidget* parent = nullptr);
-
 Q_SIGNALS:
     void walletAdded(WalletModel* wallet_model);
     void walletRemoved(WalletModel* wallet_model);
@@ -186,7 +184,7 @@ class MigrateWalletActivity : public WalletControllerActivity
 public:
     MigrateWalletActivity(WalletController* wallet_controller, QWidget* parent) : WalletControllerActivity(wallet_controller, parent) {}
 
-    void migrate(WalletModel* wallet_model);
+    void migrate(const std::string& path);
 
 Q_SIGNALS:
     void migrated(WalletModel* wallet_model);

src/qt/walletmodel.cpp

@@ -594,8 +594,7 @@ QString WalletModel::getWalletName() const
 
 QString WalletModel::getDisplayName() const
 {
-    const QString name = getWalletName();
-    return name.isEmpty() ? "["+tr("default wallet")+"]" : name;
+    return GUIUtil::WalletDisplayName(getWalletName());
 }
 
 bool WalletModel::isMultiwallet() const

src/wallet/db.cpp

@@ -19,9 +19,9 @@ namespace wallet {
 bool operator<(BytePrefix a, Span<const std::byte> b) { return a.prefix < b.subspan(0, std::min(a.prefix.size(), b.size())); }
 bool operator<(Span<const std::byte> a, BytePrefix b) { return a.subspan(0, std::min(a.size(), b.prefix.size())) < b.prefix; }
 
-std::vector<fs::path> ListDatabases(const fs::path& wallet_dir)
+std::vector<std::pair<fs::path, std::string>> ListDatabases(const fs::path& wallet_dir)
 {
-    std::vector<fs::path> paths;
+    std::vector<std::pair<fs::path, std::string>> paths;
     std::error_code ec;
 
     for (auto it = fs::recursive_directory_iterator(wallet_dir, ec); it != fs::recursive_directory_iterator(); it.increment(ec)) {
@@ -38,21 +38,29 @@ std::vector<fs::path> ListDatabases(const fs::path& wallet_dir)
         try {
             const fs::path path{it->path().lexically_relative(wallet_dir)};
 
-            if (it->status().type() == fs::file_type::directory &&
-                (IsBDBFile(BDBDataFile(it->path())) || IsSQLiteFile(SQLiteDataFile(it->path())))) {
-                // Found a directory which contains wallet.dat btree file, add it as a wallet.
-                paths.emplace_back(path);
+            if (it->status().type() == fs::file_type::directory) {
+                if (IsBDBFile(BDBDataFile(it->path()))) {
+                    // Found a directory which contains wallet.dat btree file, add it as a wallet with BERKELEY format.
+                    paths.emplace_back(path, "bdb");
+                } else if (IsSQLiteFile(SQLiteDataFile(it->path()))) {
+                    // Found a directory which contains wallet.dat sqlite file, add it as a wallet with SQLITE format.
+                    paths.emplace_back(path, "sqlite");
+                }
             } else if (it.depth() == 0 && it->symlink_status().type() == fs::file_type::regular && it->path().extension() != ".bak") {
                 if (it->path().filename() == "wallet.dat") {
-                    // Found top-level wallet.dat btree file, add top level directory ""
+                    // Found top-level wallet.dat file, add top level directory ""
                     // as a wallet.
-                    paths.emplace_back();
+                    if (IsBDBFile(it->path())) {
+                        paths.emplace_back(fs::path(), "bdb");
+                    } else if (IsSQLiteFile(it->path())) {
+                        paths.emplace_back(fs::path(), "sqlite");
+                    }
                 } else if (IsBDBFile(it->path())) {
                     // Found top-level btree file not called wallet.dat. Current bitcoin
                     // software will never create these files but will allow them to be
                     // opened in a shared database environment for backwards compatibility.
                     // Add it to the list of available wallets.
-                    paths.emplace_back(path);
+                    paths.emplace_back(path, "bdb");
                 }
             }
         } catch (const std::exception& e) {

src/wallet/db.h

@@ -216,7 +216,7 @@ enum class DatabaseStatus {
 };
 
 /** Recursively list database paths in directory. */
-std::vector<fs::path> ListDatabases(const fs::path& path);
+std::vector<std::pair<fs::path, std::string>> ListDatabases(const fs::path& path);
 
 void ReadDatabaseArgs(const ArgsManager& args, DatabaseOptions& options);
 std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);

src/wallet/interfaces.cpp

@@ -652,15 +652,30 @@ class WalletLoaderImpl : public WalletLoader
         };
         return out;
     }
+    bool isEncrypted(const std::string& wallet_name) override
+    {
+        auto wallets{GetWallets(m_context)};
+        auto it = std::find_if(wallets.begin(), wallets.end(), [&](std::shared_ptr<CWallet> w){ return w->GetName() == wallet_name; });
+        if (it != wallets.end()) return (*it)->IsCrypted();
+
+        // Unloaded wallet, read db
+        DatabaseOptions options;
+        options.require_existing = true;
+        DatabaseStatus status;
+        bilingual_str error;
+        auto db = MakeWalletDatabase(wallet_name, options, status, error);
+        if (!db) return false;
+        return WalletBatch(*db).IsEncrypted();
+    }
     std::string getWalletDir() override
     {
         return fs::PathToString(GetWalletDir());
     }
-    std::vector<std::string> listWalletDir() override
+    std::vector<std::pair<std::string, std::string>> listWalletDir() override
     {
-        std::vector<std::string> paths;
-        for (auto& path : ListDatabases(GetWalletDir())) {
-            paths.push_back(fs::PathToString(path));
+        std::vector<std::pair<std::string, std::string>> paths;
+        for (auto& [path, format] : ListDatabases(GetWalletDir())) {
+            paths.emplace_back(fs::PathToString(path), format);
         }
         return paths;
     }