Merge bitcoin#21563: net: Restrict period when cs_vNodes mutex is locked

8c8237a net, refactor: Fix style in CConnman::StopNodes (Hennadii Stepanov)
229ac18 net: Combine two loops into one, and update comments (Hennadii Stepanov)
a3d090d net: Restrict period when cs_vNodes mutex is locked (Hennadii Stepanov)

Pull request description:

  This PR restricts the period when the `cs_vNodes` mutex is locked, prevents the only case when `cs_vNodes` could be locked before the `::cs_main`.

  This change makes the explicit locking of recursive mutexes in the explicit order redundant.

ACKs for top commit:
  jnewbery:
    utACK 8c8237a
  vasild:
    ACK 8c8237a
  ajtowns:
    utACK 8c8237a - logic seems sound
  MarcoFalke:
    review ACK 8c8237a 👢

Tree-SHA512: a8277924339622b188b12d260a100adf5d82781634cf974320cf6007341f946a7ff40351137c2f5369aed0d318f38aac2d32965c9b619432440d722a4e78bb73

src/init.cpp

@@ -247,20 +247,7 @@ void PrepareShutdown(NodeContext& node)
     // Because these depend on each-other, we make sure that neither can be
     // using the other before destroying them.
     if (node.peerman) UnregisterValidationInterface(node.peerman.get());
-    // Follow the lock order requirements:
-    // * CheckForStaleTipAndEvictPeers locks cs_main before indirectly calling GetExtraFullOutboundCount
-    //   which locks cs_vNodes.
-    // * ProcessMessage locks cs_main and g_cs_orphans before indirectly calling ForEachNode which
-    //   locks cs_vNodes.
-    // * CConnman::Stop calls DeleteNode, which calls FinalizeNode, which locks cs_main and calls
-    //   EraseOrphansFor, which locks g_cs_orphans.
-    //
-    // Thus the implicit locking order requirement is: (1) cs_main, (2) g_cs_orphans, (3) cs_vNodes.
-    if (node.connman) {
-        node.connman->StopThreads();
-        LOCK2(::cs_main, ::g_cs_orphans);
-        node.connman->StopNodes();
-    }
+    if (node.connman) node.connman->Stop();
 
     StopTorControl();
 

src/net.cpp

@@ -3462,37 +3462,23 @@ void CConnman::StopNodes()
         }
     }
 
-    {
-        LOCK(cs_vNodes);
-
-        // Close sockets
-        for (CNode *pnode : vNodes)
+    // Delete peer connections.
+    std::vector<CNode*> nodes;
+    WITH_LOCK(cs_vNodes, nodes.swap(vNodes));
+    for (CNode* pnode : nodes) {
             pnode->CloseSocketDisconnect(this);
+            DeleteNode(pnode);
     }
-    for (ListenSocket& hListenSocket : vhListenSocket)
+
+    // Close listening sockets.
+    for (ListenSocket& hListenSocket : vhListenSocket) {
         if (hListenSocket.socket != INVALID_SOCKET) {
-#ifdef USE_KQUEUE
-            if (socketEventsMode == SOCKETEVENTS_KQUEUE) {
-                struct kevent event;
-                EV_SET(&event, hListenSocket.socket, EVFILT_READ, EV_DELETE, 0, 0, nullptr);
-                kevent(kqueuefd, &event, 1, nullptr, 0, nullptr);
-            }
-#endif
-#ifdef USE_EPOLL
-            if (socketEventsMode == SOCKETEVENTS_EPOLL) {
-                epoll_ctl(epollfd, EPOLL_CTL_DEL, hListenSocket.socket, nullptr);
-            }
-#endif
-            if (!CloseSocket(hListenSocket.socket))
+            if (!CloseSocket(hListenSocket.socket)) {
                 LogPrintf("CloseSocket(hListenSocket) failed with error %s\n", NetworkErrorString(WSAGetLastError()));
+            }
         }
-
-    // clean up some globals (to help leak detection)
-    std::vector<CNode*> nodes;
-    WITH_LOCK(cs_vNodes, nodes.swap(vNodes));
-    for (CNode* pnode : nodes) {
-        DeleteNode(pnode);
     }
+
     for (CNode* pnode : vNodesDisconnected) {
         DeleteNode(pnode);
     }

src/test/fuzz/process_message.cpp

@@ -106,7 +106,6 @@ void fuzz_target(FuzzBufferType buffer, const std::string& LIMIT_TO_MESSAGE_TYPE
         g_setup->m_node.peerman->SendMessages(&p2p_node);
     }
     SyncWithValidationInterfaceQueue();
-    LOCK2(::cs_main, g_cs_orphans); // See init.cpp for rationale for implicit locking order requirement
     g_setup->m_node.connman->StopNodes();
 }
 

src/test/fuzz/process_messages.cpp

@@ -79,6 +79,5 @@ FUZZ_TARGET_INIT(process_messages, initialize_process_messages)
         }
     }
     SyncWithValidationInterfaceQueue();
-    LOCK2(::cs_main, g_cs_orphans); // See init.cpp for rationale for implicit locking order requirement
     g_setup->m_node.connman->StopNodes();
 }