fix: undefined banListModel on click unban

qml/models/nodemodel.cpp

@@ -808,7 +808,7 @@ void NodeModel::ConnectToBannedListChangedSignal()
     m_handler_notify_banned_list_changed = m_node.handleBannedListChanged([this]() {
         QMetaObject::invokeMethod(this, [this] {
             Q_EMIT bannedListChanged();
-        });
+        }, Qt::QueuedConnection);
     });
 }
 

qml/pages/node/BannedPeers.qml

@@ -15,6 +15,13 @@ Page {
     signal back()
     background: null
 
+    function unbanPeer(row) {
+        if (!banListModel.unbanAt(row)) {
+            unbanActionError.message = qsTr("Could not unban peer. The ban list may have changed.")
+            unbanActionError.open()
+        }
+    }
+
     header: NavigationBar2 {
         leftItem: NavButton {
             objectName: "bannedPeersBackButton"
@@ -96,15 +103,7 @@ Page {
                     bold: false
                     horizontalPadding: 24
                     text: qsTr("Unban")
-                    onClicked: {
-                        if (banListModel.unbanAt(index)) {
-                            banListModel.refresh()
-                        } else {
-                            unbanActionError.message = qsTr("Could not unban peer. The ban list may have changed.")
-                            unbanActionError.open()
-                            banListModel.refresh()
-                        }
-                    }
+                    onClicked: root.unbanPeer(index)
                 }
             }
         }

test/qml/qml_tests_main.cpp

@@ -2480,6 +2480,7 @@ class MockBanListModel : public QAbstractListModel
     Q_OBJECT
     Q_PROPERTY(int count READ count NOTIFY countChanged)
     Q_PROPERTY(bool unbanResult MEMBER m_unban_result NOTIFY actionStateChanged)
+    Q_PROPERTY(bool resetOnUnban MEMBER m_reset_on_unban NOTIFY actionStateChanged)
     Q_PROPERTY(int unbanCalls READ unbanCalls NOTIFY actionCallsChanged)
     Q_PROPERTY(int refreshCalls READ refreshCalls NOTIFY refreshCallsChanged)
 
@@ -2524,6 +2525,10 @@ class MockBanListModel : public QAbstractListModel
     {
         ++m_unban_calls;
         Q_EMIT actionCallsChanged();
+        if (m_reset_on_unban) {
+            beginResetModel();
+            endResetModel();
+        }
         return row >= 0 && row < count() && m_unban_result;
     }
 
@@ -2536,6 +2541,7 @@ class MockBanListModel : public QAbstractListModel
     Q_INVOKABLE void resetTestState()
     {
         m_unban_result = true;
+        m_reset_on_unban = false;
         m_unban_calls = 0;
         m_refresh_calls = 0;
         Q_EMIT actionStateChanged();
@@ -2551,6 +2557,7 @@ class MockBanListModel : public QAbstractListModel
 
 private:
     bool m_unban_result{true};
+    bool m_reset_on_unban{false};
     int m_unban_calls{0};
     int m_refresh_calls{0};
 };

test/qml/tst_peeractions.qml

@@ -85,6 +85,15 @@ TestCase {
         verify(message.text.indexOf(expectedText) >= 0)
     }
 
+    function verifyUnbanActionError(page) {
+        const popup = findChild(page, "unbanActionErrorPopup")
+        verify(popup !== null)
+        tryCompare(popup, "opened", true)
+        const message = findChild(popup, "actionErrorMessage")
+        verify(message !== null)
+        verify(message.text.indexOf("Could not unban peer.") >= 0)
+    }
+
     function test_disconnect_success_refreshes_peer_table_without_error() {
         const page = createPeerDetailsPage()
         const button = findChild(page, "peerDisconnectButton")
@@ -151,7 +160,7 @@ TestCase {
         verifyPeerActionError(page, "Could not ban peer.")
     }
 
-    function test_unban_success_refreshes_ban_list_without_error() {
+    function test_unban_success_leaves_refresh_to_the_model_without_error() {
         const page = createBannedPeersPage()
         const button = findChild(page, "unbanButton_0")
         const popup = findChild(page, "unbanActionErrorPopup")
@@ -161,7 +170,7 @@ TestCase {
         button.clicked()
 
         compare(banListModel.unbanCalls, 1)
-        compare(banListModel.refreshCalls, 1)
+        compare(banListModel.refreshCalls, 0)
         compare(popup.opened, false)
     }
 
@@ -174,12 +183,36 @@ TestCase {
         button.clicked()
 
         compare(banListModel.unbanCalls, 1)
-        compare(banListModel.refreshCalls, 1)
+        compare(banListModel.refreshCalls, 0)
+        verifyUnbanActionError(page)
+    }
+
+    function test_unban_survives_synchronous_model_reset() {
+        banListModel.resetOnUnban = true
+        const page = createBannedPeersPage()
+        const button = findChild(page, "unbanButton_0")
         const popup = findChild(page, "unbanActionErrorPopup")
+        verify(button !== null)
         verify(popup !== null)
-        tryCompare(popup, "opened", true)
-        const message = findChild(popup, "actionErrorMessage")
-        verify(message !== null)
-        verify(message.text.indexOf("Could not unban peer.") >= 0)
+
+        button.clicked()
+
+        compare(banListModel.unbanCalls, 1)
+        compare(banListModel.refreshCalls, 0)
+        compare(popup.opened, false)
+    }
+
+    function test_unban_failure_with_synchronous_model_reset_opens_error_popup() {
+        banListModel.resetOnUnban = true
+        banListModel.unbanResult = false
+        const page = createBannedPeersPage()
+        const button = findChild(page, "unbanButton_0")
+        verify(button !== null)
+
+        button.clicked()
+
+        compare(banListModel.unbanCalls, 1)
+        compare(banListModel.refreshCalls, 0)
+        verifyUnbanActionError(page)
     }
 }

test/test_nodemodel.cpp

@@ -117,6 +117,7 @@ private Q_SLOTS:
     void initializationSuccessDuringCoreShutdownSkipsReadyState();
     void destructorUnsubscribesCoreSignalsBeforeStoppingPolling();
     void nodeNotificationHandlersUpdateModelThroughQueuedSignals();
+    void bannedListNotificationFromGuiThreadIsNotDeliveredReentrantly();
     void blockTipUpdatesQueuedAcrossThreadsRetainPayloadValues();
     void blockSyncActiveFollowsInitializationAndBlockTipState();
     void alertNotificationsRefreshWarningList();
@@ -544,6 +545,34 @@ void NodeModelTests::nodeNotificationHandlersUpdateModelThroughQueuedSignals()
     QCOMPARE(model.numOutboundPeers(), 7);
 }
 
+void NodeModelTests::bannedListNotificationFromGuiThreadIsNotDeliveredReentrantly()
+{
+    MockNode node;
+    MempoolState mempool;
+    interfaces::Node::BannedListChangedFn banned_list_changed_fn;
+
+    ConfigureNodeModelDefaults(node);
+    ConfigureMempoolGetters(node, mempool);
+    node.handle_banned_list_changed_fn = [&](interfaces::Node::BannedListChangedFn fn) {
+        banned_list_changed_fn = std::move(fn);
+        return MakeNoopHandler();
+    };
+
+    NodeModel model{node};
+    WaitForInitialMempoolRefresh(mempool);
+    QVERIFY(banned_list_changed_fn);
+
+    QSignalSpy banned_list_spy{&model, &NodeModel::bannedListChanged};
+
+    // A ban list change caused by a GUI action reaches the notification handler
+    // on the GUI thread. Delivering it directly would reset the ban list model
+    // while the QML handler that triggered it is still running.
+    banned_list_changed_fn();
+    QCOMPARE(banned_list_spy.count(), 0);
+
+    QTRY_COMPARE_WITH_TIMEOUT(banned_list_spy.count(), 1, ASYNC_TIMEOUT_MS);
+}
+
 void NodeModelTests::blockTipUpdatesQueuedAcrossThreadsRetainPayloadValues()
 {
     MockNode node;