Merge #16963: wallet: Fix unique_ptr usage in boost::signals2

6d6a7a8403ae923f189812edebdd95761de0e7f2 gui: Fix duplicate wallet showing up (João Barbosa)
81ea66c30e2953dee24d5b127c28daa0d9452a28 Drop signal CClientUIInterface::LoadWallet (Russell Yanofsky)

Pull request description:

  This PR includes 2 fixes:
   - prevent GUI LoadWallet handlers from crashing on startup when multiple handlers are attached, because the first handler takes ownership of the wallet unique pointer. Now every handler will receive its own unique pointer;

   - prevent showing a wallet twice in the GUI on startup due to a race with `loadwallet`.

  Fixes #16937

ACKs for top commit:
  fjahr:
    code review ACK 6d6a7a8403ae923f189812edebdd95761de0e7f2
  ryanofsky:
    Code review ACK 6d6a7a8403ae923f189812edebdd95761de0e7f2. No changes since last ACK other than rebase due to #17070
  kallewoof:
    Code review ACK 6d6a7a8403ae923f189812edebdd95761de0e7f2

Tree-SHA512: 7f0658c9011f81dfa176a094c2263448ee1d14fda7dc94e8b55ee9c8b81538bd2d1e4bf8a8dbfcd029ebfc9feb6d3cda9dee3f911122df0a4b1e0ca75f653ba4

13 files changed, 53 insertions, 26 deletions

diff --git a/src/dummywallet.cpp b/src/dummywallet.cpp
index 2fa1ad59da..1bc4e7ef72 100644
--- a/src/dummywallet.cpp
+++ b/src/dummywallet.cpp
@@ -11,6 +11,8 @@ enum class WalletCreationStatus;
 
 namespace interfaces {
 class Chain;
+class Handler;
+class Wallet;
 }
 
 class DummyWalletInit : public WalletInitInterface {
@@ -80,9 +82,13 @@ WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString&
     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)
 {
diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
index ac640aa35a..ed35e19525 100644
--- a/src/interfaces/chain.cpp
+++ b/src/interfaces/chain.cpp
@@ -338,7 +338,6 @@ public:
     void initMessage(const std::string& message) override { ::uiInterface.InitMessage(message); }
     void initWarning(const std::string& message) override { InitWarning(message); }
     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);
diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
index 349af152d5..c42f1a1fa3 100644
--- a/src/interfaces/chain.h
+++ b/src/interfaces/chain.h
@@ -43,7 +43,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).
@@ -209,9 +209,6 @@ public:
     //! 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;
 
diff --git a/src/interfaces/handler.cpp b/src/interfaces/handler.cpp
index 92601fc4e9..4e235688fe 100644
--- a/src/interfaces/handler.cpp
+++ b/src/interfaces/handler.cpp
@@ -22,6 +22,15 @@ public:
     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)
@@ -29,4 +38,9 @@ std::unique_ptr<Handler> MakeHandler(boost::signals2::connection connection)
     return MakeUnique<HandlerImpl>(std::move(connection));
 }
 
+std::unique_ptr<Handler> MakeHandler(std::function<void()> cleanup)
+{
+    return MakeUnique<CleanupHandler>(std::move(cleanup));
+}
+
 } // namespace interfaces
diff --git a/src/interfaces/handler.h b/src/interfaces/handler.h
index c4c674cac5..46918bc22e 100644
--- a/src/interfaces/handler.h
+++ b/src/interfaces/handler.h
@@ -5,6 +5,7 @@
 #ifndef BITCOIN_INTERFACES_HANDLER_H
 #define BITCOIN_INTERFACES_HANDLER_H
 
+#include <functional>
 #include <memory>
 
 namespace boost {
@@ -30,6 +31,9 @@ public:
 //! 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
diff --git a/src/interfaces/node.cpp b/src/interfaces/node.cpp
index f2b00377be..7e022dce7a 100644
--- a/src/interfaces/node.cpp
+++ b/src/interfaces/node.cpp
@@ -43,11 +43,10 @@ std::vector<fs::path> ListWalletDir();
 std::vector<std::shared_ptr<CWallet>> GetWallets();
 std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const std::string& name, std::string& error, std::vector<std::string>& warnings);
 WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, 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
@@ -286,7 +285,7 @@ public:
     }
     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
     {
diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
index 8444984b27..7911784577 100644
--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -634,10 +634,10 @@ void BitcoinGUI::setWalletController(WalletController* wallet_controller)
 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);
diff --git a/src/qt/walletframe.cpp b/src/qt/walletframe.cpp
index 4b2b475883..f363d5ddc5 100644
--- a/src/qt/walletframe.cpp
+++ b/src/qt/walletframe.cpp
@@ -39,11 +39,11 @@ void WalletFrame::setClientModel(ClientModel *_clientModel)
     this->clientModel = _clientModel;
 }
 
-void WalletFrame::addWallet(WalletModel *walletModel)
+bool WalletFrame::addWallet(WalletModel *walletModel)
 {
-    if (!gui || !clientModel || !walletModel) return;
+    if (!gui || !clientModel || !walletModel) return false;
 
-    if (mapWalletViews.count(walletModel) > 0) return;
+    if (mapWalletViews.count(walletModel) > 0) return false;
 
     WalletView *walletView = new WalletView(platformStyle, this);
     walletView->setBitcoinGUI(gui);
@@ -62,6 +62,8 @@ void WalletFrame::addWallet(WalletModel *walletModel)
     mapWalletViews[walletModel] = walletView;
 
     connect(walletView, &WalletView::outOfSyncWarningClicked, this, &WalletFrame::outOfSyncWarningClicked);
+
+    return true;
 }
 
 void WalletFrame::setCurrentWallet(WalletModel* wallet_model)
diff --git a/src/qt/walletframe.h b/src/qt/walletframe.h
index 156653f47d..20fad08b0e 100644
--- a/src/qt/walletframe.h
+++ b/src/qt/walletframe.h
@@ -36,7 +36,7 @@ public:
 
     void setClientModel(ClientModel *clientModel);
 
-    void addWallet(WalletModel *walletModel);
+    bool addWallet(WalletModel *walletModel);
     void setCurrentWallet(WalletModel* wallet_model);
     void removeWallet(WalletModel* wallet_model);
     void removeAllWallets();
diff --git a/src/ui_interface.cpp b/src/ui_interface.cpp
index c51f89d0dc..63ba9b255f 100644
--- a/src/ui_interface.cpp
+++ b/src/ui_interface.cpp
@@ -16,7 +16,6 @@ struct UISignals {
     boost::signals2::signal<CClientUIInterface::NotifyNumConnectionsChangedSig> NotifyNumConnectionsChanged;
     boost::signals2::signal<CClientUIInterface::NotifyNetworkActiveChangedSig> 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;
     boost::signals2::signal<CClientUIInterface::NotifyHeaderTipSig> NotifyHeaderTip;
@@ -36,7 +35,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);
@@ -48,7 +46,6 @@ void CClientUIInterface::InitMessage(const std::string& message) { return g_ui_s
 void CClientUIInterface::NotifyNumConnectionsChanged(int newNumConnections) { return g_ui_signals.NotifyNumConnectionsChanged(newNumConnections); }
 void CClientUIInterface::NotifyNetworkActiveChanged(bool networkActive) { return g_ui_signals.NotifyNetworkActiveChanged(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); }
 void CClientUIInterface::NotifyBlockTip(bool b, const CBlockIndex* i) { return g_ui_signals.NotifyBlockTip(b, i); }
 void CClientUIInterface::NotifyHeaderTip(bool b, const CBlockIndex* i) { return g_ui_signals.NotifyHeaderTip(b, i); }
diff --git a/src/ui_interface.h b/src/ui_interface.h
index 650a29ae3d..65721006ea 100644
--- a/src/ui_interface.h
+++ b/src/ui_interface.h
@@ -17,10 +17,6 @@ class connection;
 }
 } // namespace boost
 
-namespace interfaces {
-class Wallet;
-} // namespace interfaces
-
 /** General change type (added, updated, removed). */
 enum ChangeType
 {
@@ -105,9 +101,6 @@ public:
      */
     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 progress action resuming upon restart.
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index fe6238fc8a..559b26aa84 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -47,6 +47,7 @@ static const size_t OUTPUT_GROUP_MAX_ENTRIES = 10;
 
 static CCriticalSection 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)
 {
@@ -89,6 +90,13 @@ 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;
@@ -3911,7 +3919,12 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
         }
     }
 
-    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.
     walletInstance->handleNotifications();
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 9dad0d780a..c86367dc0f 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -35,6 +35,8 @@
 
 #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.
@@ -48,6 +50,7 @@ bool HasWallets();
 std::vector<std::shared_ptr<CWallet>> GetWallets();
 std::shared_ptr<CWallet> GetWallet(const std::string& name);
 std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocation& location, std::string& error, std::vector<std::string>& warnings);
+std::unique_ptr<interfaces::Handler> HandleLoadWallet(LoadWalletFn load_wallet);
 
 enum class WalletCreationStatus {
     SUCCESS,