Merge #20927: [refactor] [net] Clean up InactivityCheck()

bf100f8170770544fb39ae6802175c564cde532f [net] Cleanup InactivityChecks() and add commenting about time (John Newbery)
06fa85cd50b718fecd69f0481740d2b8714a1397 [net] InactivityCheck() takes a CNode reference (John Newbery)

Pull request description:

  This is a pure refactor and should not change any behavior. It clarifies and documents the InactivityCheck() function

  This makes #20721 easier to review. In particular, this function uses a mixture of (unmockable) system time and mockable time. It's important to understand where those are being used when reviewing #20721.

  #20721 doesn't require this change, so if others don't agree that it's useful and makes review easier, then I'm happy to close this and just do #20721 directly.

ACKs for top commit:
  fanquake:
    ACK bf100f8170770544fb39ae6802175c564cde532f
  MarcoFalke:
    review ACK bf100f8170770544fb39ae6802175c564cde532f 💫

Tree-SHA512: 7b001de2a5fbe8a6dc37baeae930db5775290afb2e8a6aecdf13161f1e5b06ef813bc6291d8ee5cefcf1e430c955ea702833a8db84192eebe6e6acf0b9304cb2

src/net.cpp

@@ -1487,37 +1487,47 @@ void CConnman::CalculateNumConnectionsChangedStats()
     statsClient.gauge("peers.torConnections", torNodes, 1.0f);
 }
 
-void CConnman::InactivityCheck(CNode *pnode) const
+bool CConnman::InactivityCheck(const CNode& node) const
 {
-    int64_t nTime = GetSystemTimeInSeconds();
+    // Use non-mockable system time (otherwise these timers will pop when we
-    if (nTime - pnode->nTimeConnected > m_peer_connect_timeout)
+    // use setmocktime in the tests).
-    {
+    int64_t now = GetSystemTimeInSeconds();
-        if (pnode->nLastRecv == 0 || pnode->nLastSend == 0)
+
-        {
+    if (now <= node.nTimeConnected + m_peer_connect_timeout) {
-            LogPrint(BCLog::NET, "socket no message in first %i seconds, %d %d from %d\n", m_peer_connect_timeout, pnode->nLastRecv != 0, pnode->nLastSend != 0, pnode->GetId());
+        // Only run inactivity checks if the peer has been connected longer
-            pnode->fDisconnect = true;
+        // than m_peer_connect_timeout.
-        }
+        return false;
-        else if (nTime - pnode->nLastSend > TIMEOUT_INTERVAL)
-        {
-            LogPrintf("socket sending timeout: %is\n", nTime - pnode->nLastSend);
-            pnode->fDisconnect = true;
-        }
-        else if (nTime - pnode->nLastRecv > TIMEOUT_INTERVAL)
-        {
-            LogPrintf("socket receive timeout: %is\n", nTime - pnode->nLastRecv);
-            pnode->fDisconnect = true;
-        }
-        else if (pnode->nPingNonceSent && pnode->nPingUsecStart + TIMEOUT_INTERVAL * 1000000 < GetTimeMicros())
-        {
-            LogPrintf("ping timeout: %fs\n", 0.000001 * (GetTimeMicros() - pnode->nPingUsecStart));
-            pnode->fDisconnect = true;
-        }
-        else if (!pnode->fSuccessfullyConnected)
-        {
-            LogPrint(BCLog::NET, "version handshake timeout from %d\n", pnode->GetId());
-            pnode->fDisconnect = true;
-        }
     }
+
+    if (node.nLastRecv == 0 || node.nLastSend == 0) {
+        LogPrint(BCLog::NET, "socket no message in first %i seconds, %d %d from %d\n", m_peer_connect_timeout, node.nLastRecv != 0, node.nLastSend != 0, node.GetId());
+        return true;
+    }
+
+    if (now > node.nLastSend + TIMEOUT_INTERVAL) {
+        LogPrintf("socket sending timeout: %is\n", now - node.nLastSend);
+        return true;
+    }
+
+    if (now > node.nLastRecv + TIMEOUT_INTERVAL) {
+        LogPrintf("socket receive timeout: %is\n", now - node.nLastRecv);
+        return true;
+    }
+
+    if (node.nPingNonceSent && node.nPingUsecStart.load() + TIMEOUT_INTERVAL * 1000000 < GetTimeMicros()) {
+        // We use mockable time for ping timeouts. This means that setmocktime
+        // may cause pings to time out for peers that have been connected for
+        // longer than m_peer_connect_timeout.
+        LogPrintf("ping timeout: %fs\n", 0.000001 * (GetTimeMicros() - node.nPingUsecStart));
+        return true;
+    }
+
+    if (!node.fSuccessfullyConnected) {
+        LogPrint(BCLog::NET, "version handshake timeout from %d\n", node.GetId());
+        return true;
+    }
+
+    return false;
 }
 
 bool CConnman::GenerateSelectSet(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_set, std::set<SOCKET> &error_set)
@@ -2036,7 +2046,7 @@ void CConnman::ThreadSocketHandler()
         SocketHandler();
         if (GetTimeMillis() - nLastCleanupNodes > 1000) {
             ForEachNode(AllNodes, [&](CNode* pnode) {
-                InactivityCheck(pnode);
+                if (InactivityCheck(*pnode)) pnode->fDisconnect = true;
             });
             nLastCleanupNodes = GetTimeMillis();
         }

src/net.h

@@ -600,7 +600,8 @@ private:
     void DisconnectNodes();
     void NotifyNumConnectionsChanged();
     void CalculateNumConnectionsChangedStats();
-    void InactivityCheck(CNode *pnode) const;
+    /** Return true if the peer is inactive and should be disconnected. */
+    bool InactivityCheck(const CNode& node) const;
     bool GenerateSelectSet(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_set, std::set<SOCKET> &error_set);
 #ifdef USE_KQUEUE
     void SocketEventsKqueue(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_set, std::set<SOCKET> &error_set, bool fOnlyPoll);