wallet: Fix unique_ptr usage in boost::signals2

Summary:
 * Drop signal CClientUIInterface::LoadWallet

 * gui: Fix duplicate wallet showing up

The slot BitcoinGUI::addWallet can be invoked twice for the same
WalletModel due to a concurrent wallet being loaded after the first `connect()`:

```cpp
 connect(wallet_controller, &WalletController::walletAdded, this, &BitcoinGUI::addWallet);
 connect(wallet_controller, &WalletController::walletRemoved, this, &BitcoinGUI::removeWallet);

 for (WalletModel* wallet_model : m_wallet_controller->getOpenWallets()) {
     addWallet(wallet_model);
```

This is a backport of Core [[bitcoin/bitcoin#16963 | PR16963]]

Because `WalletFrame::addWallet` already returns a bool, that part of the patch wasn't necessary.

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7699

src/dummywallet.cpp

@@ -13,7 +13,9 @@ enum class WalletCreationStatus;
 
 namespace interfaces {
 class Chain;
-}
+class Handler;
+class Wallet;
+} // namespace interfaces
 
 class DummyWalletInit : public WalletInitInterface {
 public:
@@ -68,9 +70,14 @@ WalletCreationStatus CreateWallet(const CChainParams &chainParams,
     throw std::logic_error("Wallet function called in non-wallet build.");
 }
 
-namespace interfaces {
+using LoadWalletFn =
+    std::function<void(std::unique_ptr<interfaces::Wallet> wallet)>;
+std::unique_ptr<interfaces::Handler>
+HandleLoadWallet(LoadWalletFn load_wallet) {
+    throw std::logic_error("Wallet function called in non-wallet build.");
+}
 
-class Wallet;
+namespace interfaces {
 
 std::unique_ptr<Wallet> MakeWallet(const std::shared_ptr<CWallet> &wallet) {
     throw std::logic_error("Wallet function called in non-wallet build.");

src/interfaces/chain.cpp

@@ -366,9 +366,6 @@ namespace {
         void initError(const std::string &message) override {
             InitError(message);
         }
-        void loadWallet(std::unique_ptr<Wallet> wallet) override {
-            ::uiInterface.LoadWallet(wallet);
-        }
         void showProgress(const std::string &title, int progress,
                           bool resume_possible) override {
             ::uiInterface.ShowProgress(title, progress, resume_possible);

src/interfaces/chain.h

@@ -49,7 +49,7 @@ class Wallet;
 //!   asynchronously
 //!   (https://github.com/bitcoin/bitcoin/pull/10973#issuecomment-380101269).
 //!
-//! * The initMessages() and loadWallet() methods which the wallet uses to send
+//! * The initMessage() and showProgress() methods which the wallet uses to send
 //!   notifications to the GUI should go away when GUI and wallet can directly
 //!   communicate with each other without going through the node
 //!   (https://github.com/bitcoin/bitcoin/pull/15288#discussion_r253321096).
@@ -215,9 +215,6 @@ class Chain {
     //! Send init error.
     virtual void initError(const std::string &message) = 0;
 
-    //! Send wallet load notification to the GUI.
-    virtual void loadWallet(std::unique_ptr<Wallet> wallet) = 0;
-
     //! Send progress indicator.
     virtual void showProgress(const std::string &title, int progress,
                               bool resume_possible) = 0;

src/interfaces/handler.cpp

@@ -22,10 +22,35 @@ namespace {
         boost::signals2::scoped_connection m_connection;
     };
 
+    class CleanupHandler : public Handler {
+    public:
+        explicit CleanupHandler(std::function<void()> cleanup)
+            : m_cleanup(std::move(cleanup)) {}
+        ~CleanupHandler() override {
+            if (!m_cleanup) {
+                return;
+            }
+            m_cleanup();
+            m_cleanup = nullptr;
+        }
+        void disconnect() override {
+            if (!m_cleanup) {
+                return;
+            }
+            m_cleanup();
+            m_cleanup = nullptr;
+        }
+        std::function<void()> m_cleanup;
+    };
+
 } // namespace
 
 std::unique_ptr<Handler> MakeHandler(boost::signals2::connection connection) {
     return std::make_unique<HandlerImpl>(std::move(connection));
 }
 
+std::unique_ptr<Handler> MakeHandler(std::function<void()> cleanup) {
+    return std::make_unique<CleanupHandler>(std::move(cleanup));
+}
+
 } // namespace interfaces

src/interfaces/handler.h

@@ -5,6 +5,7 @@
 #ifndef BITCOIN_INTERFACES_HANDLER_H
 #define BITCOIN_INTERFACES_HANDLER_H
 
+#include <functional>
 #include <memory>
 
 namespace boost {
@@ -29,6 +30,9 @@ class Handler {
 //! Return handler wrapping a boost signal connection.
 std::unique_ptr<Handler> MakeHandler(boost::signals2::connection connection);
 
+//! Return handler wrapping a cleanup function.
+std::unique_ptr<Handler> MakeHandler(std::function<void()> cleanup);
+
 } // namespace interfaces
 
 #endif // BITCOIN_INTERFACES_HANDLER_H

src/interfaces/node.cpp

@@ -53,11 +53,11 @@ WalletCreationStatus CreateWallet(const CChainParams &params,
                                   const std::string &name, std::string &error,
                                   std::vector<std::string> &warnings,
                                   std::shared_ptr<CWallet> &result);
+std::unique_ptr<interfaces::Handler>
+HandleLoadWallet(interfaces::Node::LoadWalletFn load_wallet);
 
 namespace interfaces {
 
-class Wallet;
-
 namespace {
 
     class NodeImpl : public Node {
@@ -312,10 +312,7 @@ namespace {
             return MakeHandler(::uiInterface.ShowProgress_connect(fn));
         }
         std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) override {
-            return MakeHandler(::uiInterface.LoadWallet_connect(
-                [fn](std::unique_ptr<Wallet> &wallet) {
-                    fn(std::move(wallet));
-                }));
+            return HandleLoadWallet(std::move(fn));
         }
         std::unique_ptr<Handler> handleNotifyNumConnectionsChanged(
             NotifyNumConnectionsChangedFn fn) override {

src/qt/bitcoingui.cpp

@@ -685,10 +685,12 @@ void BitcoinGUI::addWallet(WalletModel *walletModel) {
     if (!walletFrame) {
         return;
     }
+    if (!walletFrame->addWallet(walletModel)) {
+        return;
+    }
     const QString display_name = walletModel->getDisplayName();
     setWalletActionsEnabled(true);
     rpcConsole->addWallet(walletModel);
-    walletFrame->addWallet(walletModel);
     m_wallet_selector->addItem(display_name, QVariant::fromValue(walletModel));
     if (m_wallet_selector->count() == 2) {
         m_wallet_selector_label_action->setVisible(true);

src/ui_interface.cpp

@@ -23,7 +23,6 @@ struct UISignals {
         NotifyNetworkActiveChanged;
     boost::signals2::signal<CClientUIInterface::NotifyAlertChangedSig>
         NotifyAlertChanged;
-    boost::signals2::signal<CClientUIInterface::LoadWalletSig> LoadWallet;
     boost::signals2::signal<CClientUIInterface::ShowProgressSig> ShowProgress;
     boost::signals2::signal<CClientUIInterface::NotifyBlockTipSig>
         NotifyBlockTip;
@@ -46,7 +45,6 @@ ADD_SIGNALS_IMPL_WRAPPER(InitMessage);
 ADD_SIGNALS_IMPL_WRAPPER(NotifyNumConnectionsChanged);
 ADD_SIGNALS_IMPL_WRAPPER(NotifyNetworkActiveChanged);
 ADD_SIGNALS_IMPL_WRAPPER(NotifyAlertChanged);
-ADD_SIGNALS_IMPL_WRAPPER(LoadWallet);
 ADD_SIGNALS_IMPL_WRAPPER(ShowProgress);
 ADD_SIGNALS_IMPL_WRAPPER(NotifyBlockTip);
 ADD_SIGNALS_IMPL_WRAPPER(NotifyHeaderTip);
@@ -75,10 +73,6 @@ void CClientUIInterface::NotifyNetworkActiveChanged(bool networkActive) {
 void CClientUIInterface::NotifyAlertChanged() {
     return g_ui_signals.NotifyAlertChanged();
 }
-void CClientUIInterface::LoadWallet(
-    std::unique_ptr<interfaces::Wallet> &wallet) {
-    return g_ui_signals.LoadWallet(wallet);
-}
 void CClientUIInterface::ShowProgress(const std::string &title, int nProgress,
                                       bool resume_possible) {
     return g_ui_signals.ShowProgress(title, nProgress, resume_possible);

src/ui_interface.h

@@ -17,10 +17,6 @@ namespace signals2 {
 }
 } // namespace boost
 
-namespace interfaces {
-class Wallet;
-} // namespace interfaces
-
 /** General change type (added, updated, removed). */
 enum ChangeType { CT_NEW, CT_UPDATED, CT_DELETED };
 
@@ -118,10 +114,6 @@ class CClientUIInterface {
      */
     ADD_SIGNALS_DECL_WRAPPER(NotifyAlertChanged, void, );
 
-    /** A wallet has been loaded. */
-    ADD_SIGNALS_DECL_WRAPPER(LoadWallet, void,
-                             std::unique_ptr<interfaces::Wallet> &wallet);
-
     /**
      * Show progress e.g. for verifychain.
      * resume_possible indicates shutting down now will result in the current

src/wallet/wallet.cpp

@@ -45,6 +45,7 @@ const std::map<uint64_t, std::string> WALLET_FLAG_CAVEATS{
 
 static RecursiveMutex cs_wallets;
 static std::vector<std::shared_ptr<CWallet>> vpwallets GUARDED_BY(cs_wallets);
+static std::list<LoadWalletFn> g_load_wallet_fns GUARDED_BY(cs_wallets);
 
 bool AddWallet(const std::shared_ptr<CWallet> &wallet) {
     LOCK(cs_wallets);
@@ -92,6 +93,17 @@ std::shared_ptr<CWallet> GetWallet(const std::string &name) {
     return nullptr;
 }
 
+std::unique_ptr<interfaces::Handler>
+HandleLoadWallet(LoadWalletFn load_wallet) {
+    LOCK(cs_wallets);
+    auto it = g_load_wallet_fns.emplace(g_load_wallet_fns.end(),
+                                        std::move(load_wallet));
+    return interfaces::MakeHandler([it] {
+        LOCK(cs_wallets);
+        g_load_wallet_fns.erase(it);
+    });
+}
+
 static Mutex g_wallet_release_mutex;
 static std::condition_variable g_wallet_release_cv;
 static std::set<std::string> g_unloading_wallet_set;
@@ -4370,7 +4382,12 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(
         }
     }
 
-    chain.loadWallet(interfaces::MakeWallet(walletInstance));
+    {
+        LOCK(cs_wallets);
+        for (auto &load_wallet : g_load_wallet_fns) {
+            load_wallet(interfaces::MakeWallet(walletInstance));
+        }
+    }
 
     // Register with the validation interface. It's ok to do this after rescan
     // since we're still holding locked_chain.

src/wallet/wallet.h

@@ -37,6 +37,9 @@
 
 #include <boost/signals2/signal.hpp>
 
+using LoadWalletFn =
+    std::function<void(std::unique_ptr<interfaces::Wallet> wallet)>;
+
 //! Explicitly unload and delete the wallet.
 //! Blocks the current thread after signaling the unload intent so that all
 //! wallet clients release the wallet.
@@ -54,6 +57,7 @@ std::shared_ptr<CWallet> LoadWallet(const CChainParams &chainParams,
                                     const WalletLocation &location,
                                     std::string &error,
                                     std::vector<std::string> &warnings);
+std::unique_ptr<interfaces::Handler> HandleLoadWallet(LoadWalletFn load_wallet);
 
 enum class WalletCreationStatus { SUCCESS, CREATION_FAILED, ENCRYPTION_FAILED };
 
@@ -64,7 +68,6 @@ WalletCreationStatus CreateWallet(const CChainParams &params,
                                   const std::string &name, std::string &error,
                                   std::vector<std::string> &warnings,
                                   std::shared_ptr<CWallet> &result);
-
 //! -paytxfee default
 constexpr Amount DEFAULT_PAY_TX_FEE = Amount::zero();
 //! -fallbackfee default