Attempt to evict nodes to meet the max conn count

The algorithm here is important and took some time to get right. Instead
of comparing whether the current number of connected nodes minus the
number of unevictable nodes is greater than the number of max
connections, check that:

 * there are any evictable nodes (connected nodes minus unevictable
 nodes)
 * there are more nodes connected than requested (connected nodes minus
 max connections)

While we could wait for nodes to disconnect organically, it's more
important to run the eviction logic frequently enough that we can tell
when it will have an effect.

Whitelisted connections and protected inbound connections are
unevictable, and max connections should account for inbound connections.

Because the evictor will never evict protected inbound connections, the
maximum connection count should always be at least as large as the
protected connection count.

Note that the tests for this use a delay and test that the delay has not
expired. This helps improve determinism in the testing. Otherwise, a
strict test for a fixed number of disconnections is susceptible to
things like CPU jitter, especially when running through CI.

Patrick ran this test for 1000 runs on busy CPUs and saw no failures.

qa/rpc-tests/setmaxconnections.py

@@ -6,8 +6,11 @@
 #
 # Exercise setmaxconnections RPC command
 #
-# from the constant MAX_ADDNODE_CONNECTIONS in src/net.h
-MINIMUM_CONNECTIONS = 8
+# from the constants MAX_ADDNODE_CONNECTIONS and PROTECTED_INBOUND_PEERS in src/net.h
+MAX_ADDNODE_CONNECTIONS = 8
+PROTECTED_INBOUND_PEERS = 4 + 8 + 4 + 4
+MINIMUM_CONNECTIONS = MAX_ADDNODE_CONNECTIONS
+MINIMUM_CONNECTIONS_INTERNAL = MAX_ADDNODE_CONNECTIONS + PROTECTED_INBOUND_PEERS
 
 from test_framework.mininode import *
 from test_framework.test_framework import BitcoinTestFramework
@@ -65,9 +68,10 @@ class SetMaxConnectionCountTest (BitcoinTestFramework):
     def run_test(self):
         self.test_rpc_argument_validation()
 
-        self.test_node_connection_changes(12)
+        # these numbers must meet or exceed PROTECTED_INBOUND_CONNECTIONS
+        # otherwise there aren't enough nodes to disconnect
         self.test_node_connection_changes(20)
-        self.test_node_connection_changes(9)
+        self.test_node_connection_changes(30)
 
         # max_count has to be at least 20
         # min_count can be closer to 20
@@ -106,30 +110,41 @@ class SetMaxConnectionCountTest (BitcoinTestFramework):
         except JSONRPCException as e:
             raise AssertionError(f"Must allow parameter value >= {MINIMUM_CONNECTIONS}")
 
+    def wait_for_n_disconnections(self, nodes, count, timeout):
+        def disconnected():
+            closed_conns = [node.peer_disconnected for node in nodes]
+            print(f'Len {len(closed_conns)}, waiting for {count} of {len(nodes)} => {len(closed_conns) >= count}')
+            return len(closed_conns) >= count
+        return wait_until(disconnected, timeout=timeout)
+
     def test_node_connection_changes(self, extras):
         first_node = self.nodes[0]
 
         # MINIMUM_CONNECTIONS outgoing connections plus 1 feeler
-        first_node.setmaxconnections(1 + MINIMUM_CONNECTIONS + extras)
+        max_connections = 1 + MINIMUM_CONNECTIONS + extras
+        first_node.setmaxconnections(max_connections)
         client_nodes = []
 
         self.connect_nodes(client_nodes, extras)
         x = first_node.getconnectioncount()
-        assert(x == extras)
+        assert(x <= extras)
 
         # attempt to add more nodes
         self.connect_nodes(client_nodes, 3)
 
         # the new nodes should not increase the connection count
         x = first_node.getconnectioncount()
-        assert(x == extras)
-
-        for node in client_nodes:
-            node.close()
+        assert(x <= extras)
 
         first_node.setmaxconnections(MINIMUM_CONNECTIONS)
-        x = first_node.getconnectioncount()
-        assert(x <= MINIMUM_CONNECTIONS)
+
+
+        disconnectable_connections=max_connections - MINIMUM_CONNECTIONS_INTERNAL
+        assert(self.wait_for_n_disconnections(client_nodes, count=disconnectable_connections, timeout=30))
+
+        # disconnect to clean up for the next test
+        for node in client_nodes:
+            node.close()
 
     def test_node_disconnections(self, max_count, min_count):
         first_node = self.nodes[0]
@@ -137,12 +152,13 @@ class SetMaxConnectionCountTest (BitcoinTestFramework):
         attempted_nodes = []
 
         # MINIMUM_CONNECTIONS outgoing connections plus 1 feeler
-        first_node.setmaxconnections(1 + MINIMUM_CONNECTIONS + max_count)
+        # plus 20 connections protected from eviction
+        first_node.setmaxconnections(20 + 1 + MINIMUM_CONNECTIONS + max_count)
         client_nodes = []
 
         self.connect_nodes(client_nodes, max_count)
         x = first_node.getconnectioncount()
-        assert(x == max_count)
+        assert(x <= max_count)
 
         first_node.setmaxconnections(min_count)
 
@@ -158,13 +174,15 @@ class SetMaxConnectionCountTest (BitcoinTestFramework):
             if disc_count < max_count - min_count:
                 return False
             return True
-        wait_until(nodes_disconnected, timeout=10)
+        wait_until(nodes_disconnected, timeout=30)
 
         # try asserting this two ways, for debugging the test
         x = first_node.getconnectioncount()
         assert(x < max_count)
-        assert(x == min_count)
+        actual_min = max(min_count, MINIMUM_CONNECTIONS_INTERNAL)
+        assert(x == actual_min)
 
+        # disconnect to clean up for the next test
         for node in attempted_nodes:
             node.close()
 

src/net.cpp

@@ -1160,8 +1160,12 @@ void CConnman::DeleteDisconnectedNodes()
 void CConnman::ThreadSocketHandler()
 {
     unsigned int nPrevNodeCount = 0;
+    const unsigned int nUnevictableConnections = std::max(0, std::max(MAX_OUTBOUND_CONNECTIONS, MAX_ADDNODE_CONNECTIONS) + PROTECTED_INBOUND_PEERS);
+
     while (!interruptNet)
     {
+        unsigned int nWhitelistedConnections = 0;
+
         //
         // Disconnect nodes
         //
@@ -1286,6 +1290,9 @@ void CConnman::ThreadSocketHandler()
             if (interruptNet)
                 return;
 
+            if (pnode->fWhitelisted)
+                nWhitelistedConnections++;
+
             //
             // Receive
             //
@@ -1406,12 +1413,19 @@ void CConnman::ThreadSocketHandler()
         //
         // Reduce number of connections, if needed
         //
-        if (vNodesCopy.size() > (size_t)nMaxConnections)
+
+        long unsigned int nKeepConnections = nUnevictableConnections + nWhitelistedConnections;
+        long unsigned int nNodesCopy       = vNodesCopy.size();
+
+        if (nNodesCopy > nKeepConnections && (nNodesCopy > (long unsigned int)nMaxConnections))
         {
-            LogPrintf("%s: attempting to reduce connections: max=%u current=%u\n", __func__, nMaxConnections, vNodesCopy.size());
+            LogPrintf("%s: attempting to reduce connections: max=%u current=%u keep=%u\n", __func__, nMaxConnections, nNodesCopy, nKeepConnections);
             DisconnectUnusedNodes();
             DeleteDisconnectedNodes();
-            AttemptToEvictConnection();
+
+            if (!AttemptToEvictConnection()) {
+                LogPrint("net", "Failed to evict connections\n");
+            }
         }
 
         {
@@ -1424,8 +1438,9 @@ void CConnman::ThreadSocketHandler()
 
 void CConnman::SetMaxConnections(int newMaxConnections)
 {
-    newMaxConnections = std::max(newMaxConnections, MAX_ADDNODE_CONNECTIONS);
+    newMaxConnections = std::max(newMaxConnections, MAX_ADDNODE_CONNECTIONS + PROTECTED_INBOUND_PEERS);
     nMaxConnections = std::min(newMaxConnections, nAvailableFds);
+
     if (nMaxConnections != newMaxConnections) {
         LogPrintf("%s: capped new maxconnections request of %d to %d\n", __func__, newMaxConnections, nMaxConnections);
     }

src/net.h

@@ -63,6 +63,8 @@ static const unsigned int MAX_SUBVERSION_LENGTH = 256;
 static const int MAX_OUTBOUND_CONNECTIONS = 8;
 /** Maximum number of addnode outgoing nodes */
 static const int MAX_ADDNODE_CONNECTIONS = 8;
+/** Number of peers protected from eviction: 4 random, 8 with lowest ping, 4 that sent recent tx, 4 that sent recent blocks */
+static const int PROTECTED_INBOUND_PEERS = 4 + 8 + 4 + 4;
 /** -listen default */
 static const bool DEFAULT_LISTEN = true;
 /** -upnp default */