From 81ea66c30e2953dee24d5b127c28daa0d9452a28 Mon Sep 17 00:00:00 2001
From: Russell Yanofsky <russ@yanofsky.org>
Date: Fri, 27 Sep 2019 07:31:44 -0400
Subject: [PATCH 1/2] Drop signal CClientUIInterface::LoadWallet

---
 src/dummywallet.cpp        | 10 ++++++++--
 src/interfaces/chain.cpp   |  1 -
 src/interfaces/chain.h     |  5 +----
 src/interfaces/handler.cpp | 14 ++++++++++++++
 src/interfaces/handler.h   |  4 ++++
 src/interfaces/node.cpp    |  5 ++---
 src/ui_interface.cpp       |  3 ---
 src/ui_interface.h         |  7 -------
 src/wallet/wallet.cpp      | 15 ++++++++++++++-
 src/wallet/wallet.h        |  3 +++
 10 files changed, 46 insertions(+), 21 deletions(-)

diff --git a/src/dummywallet.cpp b/src/dummywallet.cpp
index 0edcb0286dc..100f2d5dde9 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 b2c20573fb6..9d54e64ec26 100644
--- a/src/interfaces/chain.cpp
+++ b/src/interfaces/chain.cpp
@@ -342,7 +342,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 73a78e21fb3..894a7c433ec 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).
@@ -213,9 +213,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 92601fc4e99..4e235688fee 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 c4c674cac5a..46918bc22e7 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 227ac9f7b91..2c24b02f236 100644
--- a/src/interfaces/node.cpp
+++ b/src/interfaces/node.cpp
@@ -42,11 +42,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
@@ -282,7 +281,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/ui_interface.cpp b/src/ui_interface.cpp
index d310637145b..3968464efc5 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 9efc2db391f..f47b56c281d 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 159d4f78c6d..e19ed95b9da 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;
@@ -4562,7 +4570,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 85c277ff506..954e74df05e 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,

From 6d6a7a8403ae923f189812edebdd95761de0e7f2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= <joao.paulo.barbosa@gmail.com>
Date: Thu, 26 Sep 2019 21:43:44 +0100
Subject: [PATCH 2/2] 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);
```
---
 src/qt/bitcoingui.cpp  | 2 +-
 src/qt/walletframe.cpp | 8 +++++---
 src/qt/walletframe.h   | 2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
index 2aeba6d82c3..bbf1faa13cf 100644
--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -632,10 +632,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 d7f0617315a..704a85ccc7a 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);
@@ -67,6 +67,8 @@ void WalletFrame::addWallet(WalletModel *walletModel)
     });
 
     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 156653f47df..20fad08b0ef 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();