Merge bitcoin-core/gui#801: Fix nullptr clientModel access during shutdown

b7aa717cdd3f6af266c244fec6d775e917cf8d0c refactor: gui, simplify boost signals disconnection (furszy)
f3a612f9016fe1f59c73d6059274bea8025b8940 gui: guard accessing a nullptr 'clientModel' (furszy)

Pull request description:

  Fixing #800.

  During shutdown, already queue events dispatched from the backend such
  'numConnectionsChanged' and 0networkActiveChanged' could try to access
  the clientModel object, which might not exist because we manually delete
  it inside 'BitcoinApplication::requestShutdown()'.

  This happen because boost does not clears the queued events when they arise
  concurrently with the signal disconnection (see https://www.boost.org/doc/libs/1_55_0/doc/html/signals2/thread-safety.html).
  From the docs:
  1) "Note that since we unlock the connection's mutex before executing its associated slot, it is possible a slot will still be executing after it has been disconnected by a [connection::disconnect](https://www.boost.org/doc/libs/1_55_0/doc/html/boost/signals2/connection.html#idp89761576-bb)(), if the disconnect was called concurrently with signal invocation."
  2)  "The fact that concurrent signal invocations use the same combiner object means you need to insure any custom combiner you write is thread-safe"

  So, we need to guard `clientModel` before accessing it at the handler side.

ACKs for top commit:
  hebasto:
    re-ACK b7aa717cdd3f6af266c244fec6d775e917cf8d0c

Tree-SHA512: f1a21d69248628f6a13556a9438c9e4ea9f0a3678aab09ddfe836e78e4eee405a6730d37d39f1445068ada3a110b655b619cf0e090fc2d0cdf99bed061364aeb

5 files changed, 31 insertions, 29 deletions

diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index 33c305f0d4..b1a8461d02 100644
--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -372,6 +372,11 @@ void BitcoinApplication::requestShutdown()
     // Request node shutdown, which can interrupt long operations, like
     // rescanning a wallet.
     node().startShutdown();
+    // Prior to unsetting the client model, stop listening backend signals
+    if (clientModel) {
+        clientModel->stop();
+    }
+
     // Unsetting the client model can cause the current thread to wait for node
     // to complete an operation, like wait for a RPC execution to complete.
     window->setClientModel(nullptr);
diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
index ad80922c8b..5f132b817e 100644
--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -989,6 +989,7 @@ void BitcoinGUI::gotoLoadPSBT(bool from_clipboard)
 
 void BitcoinGUI::updateNetworkState()
 {
+    if (!clientModel) return;
     int count = clientModel->getNumConnections();
     QString icon;
     switch(count)
diff --git a/src/qt/clientmodel.cpp b/src/qt/clientmodel.cpp
index c31e06e88e..05172cfbd2 100644
--- a/src/qt/clientmodel.cpp
+++ b/src/qt/clientmodel.cpp
@@ -70,7 +70,7 @@ ClientModel::ClientModel(interfaces::Node& node, OptionsModel *_optionsModel, QO
     subscribeToCoreSignals();
 }
 
-ClientModel::~ClientModel()
+void ClientModel::stop()
 {
     unsubscribeFromCoreSignals();
 
@@ -78,6 +78,11 @@ ClientModel::~ClientModel()
     m_thread->wait();
 }
 
+ClientModel::~ClientModel()
+{
+    stop();
+}
+
 int ClientModel::getNumConnections(unsigned int flags) const
 {
     ConnectionDirection connections = ConnectionDirection::None;
@@ -238,47 +243,41 @@ void ClientModel::TipChanged(SynchronizationState sync_state, interfaces::BlockT
 
 void ClientModel::subscribeToCoreSignals()
 {
-    m_handler_show_progress = m_node.handleShowProgress(
+    m_event_handlers.emplace_back(m_node.handleShowProgress(
         [this](const std::string& title, int progress, [[maybe_unused]] bool resume_possible) {
             Q_EMIT showProgress(QString::fromStdString(title), progress);
-        });
-    m_handler_notify_num_connections_changed = m_node.handleNotifyNumConnectionsChanged(
+        }));
+    m_event_handlers.emplace_back(m_node.handleNotifyNumConnectionsChanged(
         [this](int new_num_connections) {
             Q_EMIT numConnectionsChanged(new_num_connections);
-        });
-    m_handler_notify_network_active_changed = m_node.handleNotifyNetworkActiveChanged(
+        }));
+    m_event_handlers.emplace_back(m_node.handleNotifyNetworkActiveChanged(
         [this](bool network_active) {
             Q_EMIT networkActiveChanged(network_active);
-        });
-    m_handler_notify_alert_changed = m_node.handleNotifyAlertChanged(
+        }));
+    m_event_handlers.emplace_back(m_node.handleNotifyAlertChanged(
         [this]() {
             qDebug() << "ClientModel: NotifyAlertChanged";
             Q_EMIT alertsChanged(getStatusBarWarnings());
-        });
-    m_handler_banned_list_changed = m_node.handleBannedListChanged(
+        }));
+    m_event_handlers.emplace_back(m_node.handleBannedListChanged(
         [this]() {
             qDebug() << "ClienModel: Requesting update for peer banlist";
             QMetaObject::invokeMethod(banTableModel, [this] { banTableModel->refresh(); });
-        });
-    m_handler_notify_block_tip = m_node.handleNotifyBlockTip(
+        }));
+    m_event_handlers.emplace_back(m_node.handleNotifyBlockTip(
         [this](SynchronizationState sync_state, interfaces::BlockTip tip, double verification_progress) {
             TipChanged(sync_state, tip, verification_progress, SyncType::BLOCK_SYNC);
-        });
-    m_handler_notify_header_tip = m_node.handleNotifyHeaderTip(
+        }));
+    m_event_handlers.emplace_back(m_node.handleNotifyHeaderTip(
         [this](SynchronizationState sync_state, interfaces::BlockTip tip, bool presync) {
             TipChanged(sync_state, tip, /*verification_progress=*/0.0, presync ? SyncType::HEADER_PRESYNC : SyncType::HEADER_SYNC);
-        });
+        }));
 }
 
 void ClientModel::unsubscribeFromCoreSignals()
 {
-    m_handler_show_progress->disconnect();
-    m_handler_notify_num_connections_changed->disconnect();
-    m_handler_notify_network_active_changed->disconnect();
-    m_handler_notify_alert_changed->disconnect();
-    m_handler_banned_list_changed->disconnect();
-    m_handler_notify_block_tip->disconnect();
-    m_handler_notify_header_tip->disconnect();
+    m_event_handlers.clear();
 }
 
 bool ClientModel::getProxyInfo(std::string& ip_port) const
diff --git a/src/qt/clientmodel.h b/src/qt/clientmodel.h
index 493e18a07d..624056b5df 100644
--- a/src/qt/clientmodel.h
+++ b/src/qt/clientmodel.h
@@ -58,6 +58,8 @@ public:
     explicit ClientModel(interfaces::Node& node, OptionsModel *optionsModel, QObject *parent = nullptr);
     ~ClientModel();
 
+    void stop();
+
     interfaces::Node& node() const { return m_node; }
     OptionsModel *getOptionsModel();
     PeerTableModel *getPeerTableModel();
@@ -95,13 +97,7 @@ public:
 
 private:
     interfaces::Node& m_node;
-    std::unique_ptr<interfaces::Handler> m_handler_show_progress;
-    std::unique_ptr<interfaces::Handler> m_handler_notify_num_connections_changed;
-    std::unique_ptr<interfaces::Handler> m_handler_notify_network_active_changed;
-    std::unique_ptr<interfaces::Handler> m_handler_notify_alert_changed;
-    std::unique_ptr<interfaces::Handler> m_handler_banned_list_changed;
-    std::unique_ptr<interfaces::Handler> m_handler_notify_block_tip;
-    std::unique_ptr<interfaces::Handler> m_handler_notify_header_tip;
+    std::vector<std::unique_ptr<interfaces::Handler>> m_event_handlers;
     OptionsModel *optionsModel;
     PeerTableModel* peerTableModel{nullptr};
     PeerTableSortProxy* m_peer_table_sort_proxy{nullptr};
diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp
index 4ef45490d9..d2b184ebdf 100644
--- a/src/qt/rpcconsole.cpp
+++ b/src/qt/rpcconsole.cpp
@@ -966,6 +966,7 @@ void RPCConsole::message(int category, const QString &message, bool html)
 
 void RPCConsole::updateNetworkState()
 {
+    if (!clientModel) return;
     QString connections = QString::number(clientModel->getNumConnections()) + " (";
     connections += tr("In:") + " " + QString::number(clientModel->getNumConnections(CONNECTIONS_IN)) + " / ";
     connections += tr("Out:") + " " + QString::number(clientModel->getNumConnections(CONNECTIONS_OUT)) + ")";