net: a node could only ever watch 63 peers (#78)

5 files changed, 120 insertions, 9 deletions

diff --git a/bitflash-node-1.2.10-x86_64 b/bitflash-node-1.2.10-x86_64
new file mode 100644
index 0000000..a279608
--- /dev/null
+++ b/bitflash-node-1.2.10-x86_64
diff --git a/bitflash-node-1.2.9-x86_64 b/bitflash-node-1.2.9-x86_64
new file mode 100644
index 0000000..561bc22
--- /dev/null
+++ b/bitflash-node-1.2.9-x86_64
diff --git a/src/makefile.mingw b/src/makefile.mingw
index 01f7702..ae533ef 100644
--- a/src/makefile.mingw
+++ b/src/makefile.mingw
@@ -14,8 +14,13 @@ 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 \
-           -DWIN32 -DNOPCH \
+           -DWIN32 -DNOPCH -DFD_SETSIZE=1024 \
            -DSECP256K1_STATIC \
            -I$(DEPS)/include -I$(RANDOMX)/src \
            -I$(DEPS)/include/nlohmann \
diff --git a/src/net.cpp b/src/net.cpp
index 37c081f..5ef8316 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -954,8 +954,27 @@ void ThreadSocketHandler2(void* parg)
             vector<CNode*> vNodesCopy = vNodes;
             foreach(CNode* pnode, vNodesCopy)
             {
-                if (pnode->ReadyToDisconnect() && pnode->vRecv.empty() && pnode->vSend.empty())
+                // Wait for the buffers to drain before letting a node go, so a
+                // last message still gets out -- but not forever. vSend never
+                // drains through a socket whose far end is gone, and a node
+                // stuck that way stayed in vNodes for the life of the process,
+                // holding its socket and counting as a peer.
+                if (!pnode->ReadyToDisconnect())
+                    pnode->nDisconnectSince = 0;
+                else if (pnode->nDisconnectSince == 0)
+                    pnode->nDisconnectSince = GetTime();
+
+                bool fDrained = pnode->vRecv.empty() && pnode->vSend.empty();
+                bool fStuck   = pnode->nDisconnectSince != 0 &&
+                                GetTime() - pnode->nDisconnectSince > DISCONNECT_DRAIN_SECS;
+
+                if (pnode->ReadyToDisconnect() && (fDrained || fStuck))
                 {
+                    if (!fDrained)
+                        printf("dropping stuck node %s after %d s with %d bytes unsent\n",
+                               pnode->addr.ToString().c_str(),
+                               (int)(GetTime() - pnode->nDisconnectSince),
+                               (int)pnode->vSend.size());
                     // remove from vNodes
                     vNodes.erase(remove(vNodes.begin(), vNodes.end(), pnode), vNodes.end());
                     pnode->Disconnect();
@@ -1010,16 +1029,35 @@ void ThreadSocketHandler2(void* parg)
         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
         CRITICAL_BLOCK(cs_vNodes)
         {
             foreach(CNode* pnode, vNodes)
             {
+                if (nWatched >= FD_SETSIZE)
+                    break;
                 FD_SET(pnode->hSocket, &fdsetRecv);
+                nWatched++;
                 hSocketMax = max(hSocketMax, pnode->hSocket);
                 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);
+                }
+            }
         }
 
         vfThreadRunning[0] = false;
@@ -1028,12 +1066,37 @@ void ThreadSocketHandler2(void* parg)
         CheckForShutdown(0);
         if (nSelect == 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);
-            for (int i = 0; i <= hSocketMax; i++)
+            FD_ZERO(&fdsetRecv);
+            FD_ZERO(&fdsetSend);
+            CRITICAL_BLOCK(cs_vNodes)
             {
-                FD_SET(i, &fdsetRecv);
-                FD_SET(i, &fdsetSend);
+                foreach(CNode* pnode, vNodes)
+                {
+                    int nType = 0;
+#ifdef _WIN32
+                    int nTypeLen = sizeof(nType);
+#else
+                    socklen_t nTypeLen = sizeof(nType);
+#endif
+                    if (getsockopt(pnode->hSocket, SOL_SOCKET, SO_TYPE, (char*)&nType, &nTypeLen) != 0)
+                    {
+                        printf("dropping node %s: its socket is no longer valid\n",
+                               pnode->addr.ToString().c_str());
+                        pnode->fDisconnect = true;
+                    }
+                }
             }
             Sleep(timeout.tv_usec/1000);
         }
@@ -1068,11 +1131,28 @@ void ThreadSocketHandler2(void* parg)
             }
             else
             {
-                LogPrint("net", "accepted connection from %s\n", addr.ToString().c_str());
-                CNode* pnode = new CNode(hSocket, addr, true);
-                pnode->AddRef();
+                // 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
+                // limit sat open and unread, which fed on itself: the deafer
+                // the node got, the more connections it collected.
+                unsigned int nNodes = 0;
                 CRITICAL_BLOCK(cs_vNodes)
-                    vNodes.push_back(pnode);
+                    nNodes = (unsigned int)vNodes.size();
+                if (nNodes >= MAX_CONNECTIONS)
+                {
+                    LogPrint("net", "refusing connection from %s, already at %u\n",
+                             addr.ToString().c_str(), nNodes);
+                    closesocket(hSocket);
+                }
+                else
+                {
+                    LogPrint("net", "accepted connection from %s\n", addr.ToString().c_str());
+                    CNode* pnode = new CNode(hSocket, addr, true);
+                    pnode->AddRef();
+                    CRITICAL_BLOCK(cs_vNodes)
+                        vNodes.push_back(pnode);
+                }
             }
         }
 
diff --git a/src/net.h b/src/net.h
index 5e828c3..01f314d 100644
--- a/src/net.h
+++ b/src/net.h
@@ -67,6 +67,30 @@ static const int          BTF_RECV_TIMEOUT_SECS    = 30 * 60;
 static const int          BTF_SEND_STALL_SECS      = 10 * 60;
 static const int          BTF_HANDSHAKE_GRACE_SECS = 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.
+//
+// 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.
+static const unsigned int MAX_CONNECTIONS = 125;
+
+// A node marked for disconnect is normally held until its buffers drain, so
+// a last message still goes out. But vSend cannot drain through a socket
+// whose far end is gone, and that made such a node immortal: it stayed in
+// vNodes, kept its socket open, and kept being counted. Force it out after
+// this long regardless of what is still buffered.
+static const int64        DISCONNECT_DRAIN_SECS    = 60;
+
 // Send a ping after this long with nothing to say, so a peer running the
 // inactivity check above does not mistake a quiet node for a dead one. Must
 // stay well under BTF_RECV_TIMEOUT_SECS.
@@ -520,6 +544,7 @@ protected:
     int nRefCount;
 public:
     int64 nReleaseTime;
+    int64 nDisconnectSince;
     map<uint256, CRequestTracker> mapRequests;
     CCriticalSection cs_mapRequests;
 
@@ -572,6 +597,7 @@ public:
         fDisconnect = false;
         nRefCount = 0;
         nReleaseTime = 0;
+        nDisconnectSince = 0;
         nLastPexRecv = 0;
         pindexLastGetBlocksBegin = NULL;
         hashLastGetBlocksEnd = 0;