Merge pull request 'net: replace peer select loop with poll' (#29) from poll-socket-handler into main

4 files changed, 97 insertions, 82 deletions

diff --git a/README.md b/README.md
index 978aba5..ef4f39b 100644
--- a/README.md
+++ b/README.md
@@ -342,7 +342,7 @@ probes disabled. Use `-tor=HOST:PORT` when Tor listens somewhere else.
 ### When something looks wrong
 
 **Menu > Diagnostics**, and the same report in `debug.log` every ten minutes:
-peers held against peers `select()` is actually watching, blocks received and how
+peers held against peers the socket loop is actually watching, blocks received and how
 many arrived without a parent, the proof-of-work mode with the live miner thread
 count, sockets by the part of the program that opened them, and per peer how long
 since the last message each way. There is a copy button — if you open an issue,
diff --git a/src/makefile.mingw b/src/makefile.mingw
index 28e925c..4e36a5c 100644
--- a/src/makefile.mingw
+++ b/src/makefile.mingw
@@ -14,14 +14,14 @@ WARNFLAGS = -Wformat -Werror=format \
             -Wno-deprecated-declarations -Wno-write-strings \
             -Wno-narrowing -Wno-unused-result
 
-# FD_SETSIZE has to be defined before winsock2.h and it has to be the same
-# number in every translation unit -- fd_set carries an array of that length,
-# so disagreeing about it means disagreeing about sizeof(fd_set). That is why
-# it lives here as a -D and not as a #define in a header. Windows defaults to
-# 64, which capped this node at 63 watchable peers no matter how many it had.
 CXXFLAGS = -std=gnu++14 -O2 $(WARNFLAGS) -fpermissive -pthread \
+<<<<<<< HEAD
+           -DWIN32 -DNOPCH \
+           -DSECP256K1_STATIC \
+=======
            -DWIN32 -DNOPCH -DFD_SETSIZE=1024 \
            -DSECP256K1_STATIC -DBITFLASH_NODE_BUILD \
+>>>>>>> origin/main
            -I$(DEPS)/include -I$(RANDOMX)/src \
            -I$(DEPS)/include/nlohmann \
            -IC:/msys64/ucrt64/include \
diff --git a/src/net.cpp b/src/net.cpp
index fe9964e..c24547e 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -13,6 +13,8 @@
 #include <thread>          // hardware_concurrency, for the miner thread count
 #ifdef _WIN32
 #include <winsock2.h>
+#else
+#include <poll.h>
 #endif
 #include <openssl/rand.h>
 #include "btfaddr.h"
@@ -23,6 +25,20 @@ void ThreadReconnectCachedBtfPeers(void* parg);
 void ThreadMessageHandler2(void* parg);
 void ThreadSocketHandler2(void* parg);
 
+#ifdef _WIN32
+typedef WSAPOLLFD BtfPollFd;
+static int BtfPoll(BtfPollFd* pfd, unsigned int nfd, int nTimeoutMs)
+{
+    return WSAPoll(pfd, nfd, nTimeoutMs);
+}
+#else
+typedef struct pollfd BtfPollFd;
+static int BtfPoll(BtfPollFd* pfd, unsigned int nfd, int nTimeoutMs)
+{
+    return poll(pfd, (nfds_t)nfd, nTimeoutMs);
+}
+#endif
+
 
 
 
@@ -275,8 +291,8 @@ string GetDiagnosticsText()
 
     // The number that would have made the deafness obvious. Anything held but
     // not watched is a socket this node will never read again.
-    str += strprintf("  peers watched     %d of %d that select() can hold%s\n",
-                     nPeersWatched, (int)FD_SETSIZE - 1,
+    str += strprintf("  peers watched     %d of %d held by poll%s\n",
+                     nPeersWatched, nHeld,
                      nHeld > nPeersWatched ? "   <-- NOT ALL PEERS ARE BEING READ" : "");
 
     if (nBlocksReceived > 0)
@@ -1202,7 +1218,7 @@ void CNode::Disconnect()
 
     // Invalidate before anything else can look at it. This used to close the
     // socket and leave the handle number sitting in the object, so ~CNode
-    // closed it a second time and the select loop kept arming it in the
+    // closed it a second time and the socket loop kept arming it in the
     // meantime. See the note on ~CNode in net.h.
     BtfCloseSocket(hSocket);
     hSocket = INVALID_SOCKET;
@@ -1363,52 +1379,49 @@ void ThreadSocketHandler2(void* parg)
         //
         // Find which sockets have data to receive
         //
-        struct timeval timeout;
-        timeout.tv_sec  = 0;
-        timeout.tv_usec = 50000; // frequency to poll pnode->vSend
-
-        fd_set fdsetRecv;
-        fd_set fdsetSend;
-        FD_ZERO(&fdsetRecv);
-        FD_ZERO(&fdsetSend);
-        SOCKET hSocketMax = 0;
-        FD_SET(hListenSocket, &fdsetRecv);
-        hSocketMax = max(hSocketMax, hListenSocket);
-        // FD_SET drops silently once the set is full, so count what goes in
-        // and say so. A peer that never makes it into the set is never read,
-        // and that is indistinguishable from a peer with nothing to say --
-        // the failure this whole path used to hide.
-        unsigned int nWatched = 1; // the listen socket
+        static const int nPollTimeoutMs = 50; // frequency to poll pnode->vSend
+
+        struct NodePollResult
+        {
+            CNode* pnode;
+            SOCKET hSocket;
+            bool fRecv;
+            bool fSend;
+            bool fInvalid;
+            NodePollResult(CNode* pnodeIn, SOCKET hSocketIn)
+                : pnode(pnodeIn), hSocket(hSocketIn), fRecv(false), fSend(false), fInvalid(false) {}
+        };
+
+        vector<BtfPollFd> vPoll;
+        vector<NodePollResult> vPollNodes;
+
+        BtfPollFd listenPoll;
+        memset(&listenPoll, 0, sizeof(listenPoll));
+        listenPoll.fd = hListenSocket;
+        listenPoll.events = POLLIN;
+        vPoll.push_back(listenPoll);
+
         CRITICAL_BLOCK(cs_vNodes)
         {
             foreach(CNode* pnode, vNodes)
             {
-                if (nWatched >= FD_SETSIZE)
-                    break;
                 // A disconnected node stays in vNodes until its references go.
-                // Arming its closed handle asks select() to watch a number the
-                // operating system may already have given to somebody else.
+                // Do not watch a closed handle number: the OS may already have
+                // handed it to a different connection.
                 if (pnode->hSocket == INVALID_SOCKET)
                     continue;
-                FD_SET(pnode->hSocket, &fdsetRecv);
-                nWatched++;
-                hSocketMax = max(hSocketMax, pnode->hSocket);
+
+                BtfPollFd pfd;
+                memset(&pfd, 0, sizeof(pfd));
+                pfd.fd = pnode->hSocket;
+                pfd.events = POLLIN;
                 TRY_CRITICAL_BLOCK(pnode->cs_vSend)
                     if (!pnode->vSend.empty())
-                        FD_SET(pnode->hSocket, &fdsetSend);
-            }
-            if (vNodes.size() + 1 > (size_t)FD_SETSIZE)
-            {
-                static int64 nLastWarned = 0;
-                if (GetTime() - nLastWarned > 60)
-                {
-                    nLastWarned = GetTime();
-                    printf("WARNING: %d peers but select() can only watch %d -- %d are not being read\n",
-                           (int)vNodes.size(), (int)FD_SETSIZE - 1,
-                           (int)vNodes.size() - (int)FD_SETSIZE + 1);
-                }
+                        pfd.events |= POLLOUT;
+                vPoll.push_back(pfd);
+                vPollNodes.push_back(NodePollResult(pnode, pnode->hSocket));
             }
-            nPeersWatched = (int)nWatched - 1;   // less the listen socket
+            nPeersWatched = (int)vPollNodes.size();
         }
 
         // Say the state of the node out loud now and then, so a log pulled off
@@ -1429,32 +1442,17 @@ void ThreadSocketHandler2(void* parg)
         }
 
         vfThreadRunning[0] = false;
-        int nSelect = select(hSocketMax + 1, &fdsetRecv, &fdsetSend, NULL, &timeout);
+        int nPoll = BtfPoll(&vPoll[0], (unsigned int)vPoll.size(), nPollTimeoutMs);
         vfThreadRunning[0] = true;
         CheckForShutdown(0);
-        if (nSelect == SOCKET_ERROR)
+        if (nPoll == SOCKET_ERROR)
         {
-            // This used to answer a failed select() by marking every
-            // descriptor from 0 to hSocketMax ready, which is code written for
-            // POSIX, where a descriptor is a small integer. A Windows SOCKET is
-            // a kernel handle in the thousands, so the loop iterated over
-            // numbers that are not sockets and told the rest of the function
-            // that every peer had data. One bad handle in the set makes
-            // select() fail on every pass, so a single closed socket put the
-            // whole thread into that state permanently.
-            //
-            // Find the bad handle instead and let it be dropped.
             int nErr = WSAGetLastError();
-            LogPrint("net", "select failed: %d\n", nErr);
-            FD_ZERO(&fdsetRecv);
-            FD_ZERO(&fdsetSend);
+            LogPrint("net", "poll failed: %d\n", nErr);
             CRITICAL_BLOCK(cs_vNodes)
             {
                 foreach(CNode* pnode, vNodes)
                 {
-                    // Already disconnected: its handle is gone on purpose, and
-                    // reporting it here would name the wrong node for a select()
-                    // failure it did not cause.
                     if (pnode->hSocket == INVALID_SOCKET)
                         continue;
                     int nType = 0;
@@ -1471,7 +1469,17 @@ void ThreadSocketHandler2(void* parg)
                     }
                 }
             }
-            Sleep(timeout.tv_usec/1000);
+            Sleep(nPollTimeoutMs);
+        }
+        else
+        {
+            for (size_t i = 0; i < vPollNodes.size(); i++)
+            {
+                short revents = vPoll[i + 1].revents; // poll[0] is the listener
+                vPollNodes[i].fRecv = (revents & (POLLIN | POLLERR | POLLHUP)) != 0;
+                vPollNodes[i].fSend = (revents & POLLOUT) != 0;
+                vPollNodes[i].fInvalid = (revents & POLLNVAL) != 0;
+            }
         }
         RandAddSeed();
 
@@ -1487,7 +1495,7 @@ void ThreadSocketHandler2(void* parg)
         //
         // Accept new connections
         //
-        if (FD_ISSET(hListenSocket, &fdsetRecv))
+        if (nPoll != SOCKET_ERROR && (vPoll[0].revents & POLLIN))
         {
             struct sockaddr_in sockaddr;
 #ifdef _WIN32
@@ -1506,7 +1514,7 @@ void ThreadSocketHandler2(void* parg)
             {
                 // Refuse rather than accept a connection this node cannot
                 // watch. Without this the listen socket kept taking peers --
-                // it is always in the select set -- while everything past the
+                // it is always watched -- while everything past the
                 // limit sat open and unread, which fed on itself: the deafer
                 // the node got, the more connections it collected.
                 unsigned int nNodes = 0;
@@ -1533,18 +1541,28 @@ void ThreadSocketHandler2(void* parg)
         //
         // Service each socket
         //
-        vector<CNode*> vNodesCopy;
-        CRITICAL_BLOCK(cs_vNodes)
-            vNodesCopy = vNodes;
-        foreach(CNode* pnode, vNodesCopy)
+        for (size_t i = 0; i < vPollNodes.size(); i++)
         {
             CheckForShutdown(0);
-            SOCKET hSocket = pnode->hSocket;
+            NodePollResult& poll = vPollNodes[i];
+            CNode* pnode = poll.pnode;
+            SOCKET hSocket = poll.hSocket;
+
+            if (hSocket == INVALID_SOCKET || pnode->hSocket != hSocket)
+                continue;
+
+            if (poll.fInvalid)
+            {
+                LogPrint("net", "poll reported invalid socket for %s\n",
+                         pnode->addr.ToString().c_str());
+                pnode->fDisconnect = true;
+                continue;
+            }
 
             //
             // Receive
             //
-            if (FD_ISSET(hSocket, &fdsetRecv))
+            if (poll.fRecv)
             {
                 TRY_CRITICAL_BLOCK(pnode->cs_vRecv)
                 {
@@ -1584,7 +1602,7 @@ void ThreadSocketHandler2(void* parg)
             //
             // Send
             //
-            if (FD_ISSET(hSocket, &fdsetSend))
+            if (poll.fSend)
             {
                 TRY_CRITICAL_BLOCK(pnode->cs_vSend)
                 {
diff --git a/src/net.h b/src/net.h
index bcd0b2a..feecf96 100644
--- a/src/net.h
+++ b/src/net.h
@@ -98,19 +98,16 @@ static const int          BTF_INCOMPLETE_MESSAGE_TIMEOUT_SECS = 2 * 60;
 
 // Ceiling on simultaneous connections.
 //
-// ThreadSocketHandler watches every peer through one select(), and select()
-// cannot watch more descriptors than FD_SETSIZE. On Windows FD_SET simply
-// stops adding once the set is full -- no error, no log -- so every socket
-// past the limit stays open and is never read again. Nothing capped vNodes,
-// so a node accepted connections it had no way to service, and because new
-// entries go on the end of vNodes it was always the newest connection that
-// starved, including the ones dialed to fix connectivity.
+// ThreadSocketHandler watches every peer in one socket loop. This used to be a
+// select() loop, which made FD_SETSIZE the real limit and silently starved
+// sockets past that bitmap. It is now poll()/WSAPoll(), so the ceiling below is
+// a deliberate resource bound rather than a descriptor-set accident.
 //
 // Measured on two nodes before this ceiling existed: 277 and 90 CNode objects
 // against 93 and 90 live sockets, and a status bar reporting hundreds of
 // peers for a node that was really talking to thirty.
 //
-// 125 is Bitcoin's number and sits far below FD_SETSIZE on both platforms.
+// 125 is Bitcoin's number and remains a conservative public-node default.
 static const unsigned int MAX_CONNECTIONS = 125;
 
 // A node marked for disconnect is normally held until its buffers drain, so
@@ -142,7 +139,7 @@ static const int          BTF_BEHIND_WARN_INTERVAL_SECS = 5 * 60;
 //
 // These counters exist so it can. Cheap to keep, and they name the failures
 // that actually happened rather than the ones that sound impressive.
-extern int   nPeersWatched;        // peers in the last select() set
+extern int   nPeersWatched;        // peers in the last poll() set
 extern int64 nBlocksReceived;      // blocks handed to ProcessBlock
 extern int64 nBlocksWithoutParent; // ...of those, how many arrived orphaned