wallet: Handle duplicate fileid exception

Summary: Loading two identical wallet files caused a crash > Handle the duplicate fileid exception thrown at CheckUniqueFileid in tow cases: > > - when duplicate wallets are set on the command line - catch in LoadWallets; > - when a duplicate wallet is loaded dynamically - catch in LoadWallet. This is a backport of Core [[bitcoin/bitcoin#16923 | PR16923]] Test Plan: `ninja all check-all` In bitcoin-qt, backup the currently open wallet to a file named wallet.dat, then in the console window type `loadwallet /path/to/wallet/`. Check that the program does not crash and you get a useful error message dialog. Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8898
Bitcoin-ABC · Jan 13, 2021 · 3fcc6ed · 3fcc6ed
1 parent fdcd3a0
commit 3fcc6ed
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 31 deletions.
diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp
@@ -175,6 +175,7 @@ WalletControllerActivity::~WalletControllerActivity() {
 }
 
 void WalletControllerActivity::showProgressDialog(const QString &label_text) {
+    assert(!m_progress_dialog);
     m_progress_dialog = new QProgressDialog(m_parent_widget);
 
     m_progress_dialog->setLabelText(label_text);
@@ -184,6 +185,12 @@ void WalletControllerActivity::showProgressDialog(const QString &label_text) {
     GUIUtil::PolishProgressDialog(m_progress_dialog);
 }
 
+void WalletControllerActivity::destroyProgressDialog() {
+    assert(m_progress_dialog);
+    delete m_progress_dialog;
+    m_progress_dialog = nullptr;
+}
+
 CreateWalletActivity::CreateWalletActivity(WalletController *wallet_controller,
                                            QWidget *parent_widget,
                                            const CChainParams &chainparams)
@@ -243,7 +250,7 @@ void CreateWalletActivity::createWallet() {
 }
 
 void CreateWalletActivity::finish() {
-    m_progress_dialog->hide();
+    destroyProgressDialog();
 
     if (!m_error_message.empty()) {
         QMessageBox::critical(
@@ -287,7 +294,7 @@ OpenWalletActivity::OpenWalletActivity(WalletController *wallet_controller,
     : WalletControllerActivity(wallet_controller, parent_widget, chainparams) {}
 
 void OpenWalletActivity::finish() {
-    m_progress_dialog->hide();
+    destroyProgressDialog();
 
     if (!m_error_message.empty()) {
         QMessageBox::critical(

diff --git a/src/qt/walletcontroller.h b/src/qt/walletcontroller.h
@@ -104,6 +104,7 @@ class WalletControllerActivity : public QObject {
     QObject *worker() const { return m_wallet_controller->m_activity_worker; }
 
     void showProgressDialog(const QString &label_text);
+    void destroyProgressDialog();
 
     WalletController *const m_wallet_controller;
     QWidget *const m_parent_widget;

diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp
@@ -77,22 +77,27 @@ bool VerifyWallets(const CChainParams &chainParams, interfaces::Chain &chain,
 
 bool LoadWallets(const CChainParams &chainParams, interfaces::Chain &chain,
                  const std::vector<std::string> &wallet_files) {
-    for (const std::string &walletFile : wallet_files) {
-        bilingual_str error;
-        std::vector<bilingual_str> warnings;
-        std::shared_ptr<CWallet> pwallet = CWallet::CreateWalletFromFile(
-            chainParams, chain, WalletLocation(walletFile), error, warnings);
-        if (!warnings.empty()) {
-            chain.initWarning(Join(warnings, Untranslated("\n")));
+    try {
+        for (const std::string &walletFile : wallet_files) {
+            bilingual_str error;
+            std::vector<bilingual_str> warnings;
+            std::shared_ptr<CWallet> pwallet = CWallet::CreateWalletFromFile(
+                chainParams, chain, WalletLocation(walletFile), error,
+                warnings);
+            if (!warnings.empty()) {
+                chain.initWarning(Join(warnings, Untranslated("\n")));
+            }
+            if (!pwallet) {
+                chain.initError(error);
+                return false;
+            }
+            AddWallet(pwallet);
         }
-        if (!pwallet) {
-            chain.initError(error);
-            return false;
-        }
-        AddWallet(pwallet);
+        return true;
+    } catch (const std::runtime_error &e) {
+        chain.initError(Untranslated(e.what()));
+        return false;
     }
-
-    return true;
 }
 
 void StartWallets(CScheduler &scheduler, const ArgsManager &args) {

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -156,22 +156,27 @@ std::shared_ptr<CWallet> LoadWallet(const CChainParams &chainParams,
                                     const WalletLocation &location,
                                     bilingual_str &error,
                                     std::vector<bilingual_str> &warnings) {
-    if (!CWallet::Verify(chainParams, chain, location, error, warnings)) {
-        error = Untranslated("Wallet file verification failed.") +
-                Untranslated(" ") + error;
-        return nullptr;
-    }
+    try {
+        if (!CWallet::Verify(chainParams, chain, location, error, warnings)) {
+            error = Untranslated("Wallet file verification failed.") +
+                    Untranslated(" ") + error;
+            return nullptr;
+        }
 
-    std::shared_ptr<CWallet> wallet = CWallet::CreateWalletFromFile(
-        chainParams, chain, location, error, warnings);
-    if (!wallet) {
-        error =
-            Untranslated("Wallet loading failed.") + Untranslated(" ") + error;
+        std::shared_ptr<CWallet> wallet = CWallet::CreateWalletFromFile(
+            chainParams, chain, location, error, warnings);
+        if (!wallet) {
+            error = Untranslated("Wallet loading failed.") + Untranslated(" ") +
+                    error;
+            return nullptr;
+        }
+        AddWallet(wallet);
+        wallet->postInitProcess();
+        return wallet;
+    } catch (const std::runtime_error &e) {
+        error = Untranslated(e.what());
         return nullptr;
     }
-    AddWallet(wallet);
-    wallet->postInitProcess();
-    return wallet;
 }
 
 std::shared_ptr<CWallet> LoadWallet(const CChainParams &chainParams,

diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py
@@ -283,13 +283,13 @@ def wallet_file(name):
             'wallet.dat')
 
         # Fail to load if one wallet is a copy of another
-        assert_raises_rpc_error(-1, "BerkeleyBatch: Can't open database w8_copy (duplicates fileid",
+        assert_raises_rpc_error(-4, "BerkeleyBatch: Can't open database w8_copy (duplicates fileid",
                                 self.nodes[0].loadwallet, 'w8_copy')
 
         # Fail to load if one wallet is a copy of another.
         # Test this twice to make sure that we don't re-introduce
         # https://github.com/bitcoin/bitcoin/issues/14304
-        assert_raises_rpc_error(-1, "BerkeleyBatch: Can't open database w8_copy (duplicates fileid",
+        assert_raises_rpc_error(-4, "BerkeleyBatch: Can't open database w8_copy (duplicates fileid",
                                 self.nodes[0].loadwallet, 'w8_copy')
 
         # Fail to load if wallet file is a symlink