net: Break disconnecting out of Ban()

These are separate events which need to be carried out by separate subsystems. This also cleans up some whitespace and tabs in qt to avoid getting flagged by the linter. Current behavior is preserved.
ElementsProject · Jan 16, 2019 · 7cc2b9f · 7cc2b9f
1 parent f71c2ea
commit 7cc2b9f
Show file tree

Hide file tree

Showing 7 changed files with 55 additions and 25 deletions.
diff --git a/src/interfaces/node.cpp b/src/interfaces/node.cpp
@@ -144,6 +144,13 @@ class NodeImpl : public Node
         }
         return false;
     }
+    bool disconnect(const CNetAddr& net_addr) override
+    {
+        if (g_connman) {
+            return g_connman->DisconnectNode(net_addr);
+        }
+        return false;
+    }
     bool disconnect(NodeId id) override
     {
         if (g_connman) {

diff --git a/src/interfaces/node.h b/src/interfaces/node.h
@@ -113,7 +113,10 @@ class Node
     //! Unban node.
     virtual bool unban(const CSubNet& ip) = 0;
 
-    //! Disconnect node.
+    //! Disconnect node by address.
+    virtual bool disconnect(const CNetAddr& net_addr) = 0;
+
+    //! Disconnect node by id.
     virtual bool disconnect(NodeId id) = 0;
 
     //! Get total bytes recv.

diff --git a/src/net.cpp b/src/net.cpp
@@ -554,13 +554,6 @@ void CConnman::Ban(const CSubNet& subNet, const BanReason &banReason, int64_t ba
     }
     if(clientInterface)
         clientInterface->BannedListChanged();
-    {
-        LOCK(cs_vNodes);
-        for (CNode* pnode : vNodes) {
-            if (subNet.Match(static_cast<CNetAddr>(pnode->addr)))
-                pnode->fDisconnect = true;
-        }
-    }
     if(banReason == BanReasonManuallyAdded)
         DumpBanlist(); //store banlist to disk immediately if user requested ban
 }
@@ -2643,6 +2636,25 @@ bool CConnman::DisconnectNode(const std::string& strNode)
     }
     return false;
 }
+
+bool CConnman::DisconnectNode(const CSubNet& subnet)
+{
+    bool disconnected = false;
+    LOCK(cs_vNodes);
+    for (CNode* pnode : vNodes) {
+        if (subnet.Match(pnode->addr)) {
+            pnode->fDisconnect = true;
+            disconnected = true;
+        }
+    }
+    return disconnected;
+}
+
+bool CConnman::DisconnectNode(const CNetAddr& addr)
+{
+    return DisconnectNode(CSubNet(addr));
+}
+
 bool CConnman::DisconnectNode(NodeId id)
 {
     LOCK(cs_vNodes);

diff --git a/src/net.h b/src/net.h
@@ -282,6 +282,8 @@ class CConnman
     size_t GetNodeCount(NumConnections num);
     void GetNodeStats(std::vector<CNodeStats>& vstats);
     bool DisconnectNode(const std::string& node);
+    bool DisconnectNode(const CSubNet& subnet);
+    bool DisconnectNode(const CNetAddr& addr);
     bool DisconnectNode(NodeId id);
 
     ServiceFlags GetLocalServices() const;

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -2961,14 +2961,14 @@ static bool SendRejectsAndCheckIfBanned(CNode* pnode, CConnman* connman, bool en
             LogPrintf("Warning: not punishing whitelisted peer %s!\n", pnode->addr.ToString());
         else if (pnode->m_manual_connection)
             LogPrintf("Warning: not punishing manually-connected peer %s!\n", pnode->addr.ToString());
-        else {
+        else if (pnode->addr.IsLocal()) {
+            // Disconnect but don't ban _this_ local node
+            LogPrintf("Warning: disconnecting but not banning local peer %s!\n", pnode->addr.ToString());
             pnode->fDisconnect = true;
-            if (pnode->addr.IsLocal())
-                LogPrintf("Warning: not banning local peer %s!\n", pnode->addr.ToString());
-            else
-            {
-                connman->Ban(pnode->addr, BanReasonNodeMisbehaving);
-            }
+        } else {
+            // Disconnect and ban all nodes sharing the address
+            connman->Ban(pnode->addr, BanReasonNodeMisbehaving);
+            connman->DisconnectNode(pnode->addr);
         }
         return true;
     }

diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp
@@ -1216,16 +1216,16 @@ void RPCConsole::banSelectedNode(int bantime)
         // Get currently selected peer address
         NodeId id = nodes.at(i).data().toLongLong();
 
-	// Get currently selected peer address
-	int detailNodeRow = clientModel->getPeerTableModel()->getRowByNodeId(id);
-	if(detailNodeRow < 0)
-	    return;
-
-	// Find possible nodes, ban it and clear the selected node
-	const CNodeCombinedStats *stats = clientModel->getPeerTableModel()->getNodeStats(detailNodeRow);
-	if(stats) {
+        // Get currently selected peer address
+        int detailNodeRow = clientModel->getPeerTableModel()->getRowByNodeId(id);
+        if (detailNodeRow < 0) return;
+
+        // Find possible nodes, ban it and clear the selected node
+        const CNodeCombinedStats *stats = clientModel->getPeerTableModel()->getNodeStats(detailNodeRow);
+        if (stats) {
             m_node.ban(stats->nodeStats.addr, BanReasonManuallyAdded, bantime);
-	}
+            m_node.disconnect(stats->nodeStats.addr);
+        }
     }
     clearSelectedNode();
     clientModel->getBanTableModel()->refresh();

diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
@@ -565,7 +565,13 @@ static UniValue setban(const JSONRPCRequest& request)
         if (request.params[3].isTrue())
             absolute = true;
 
-        isSubnet ? g_connman->Ban(subNet, BanReasonManuallyAdded, banTime, absolute) : g_connman->Ban(netAddr, BanReasonManuallyAdded, banTime, absolute);
+        if (isSubnet) {
+            g_connman->Ban(subNet, BanReasonManuallyAdded, banTime, absolute);
+            g_connman->DisconnectNode(subNet);
+        } else {
+            g_connman->Ban(netAddr, BanReasonManuallyAdded, banTime, absolute);
+            g_connman->DisconnectNode(netAddr);
+        }
     }
     else if(strCommand == "remove")
     {