From a3d090d1103cd6c25daf07afdf4e65febca6d3f7 Mon Sep 17 00:00:00 2001
From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Date: Mon, 12 Apr 2021 14:54:58 +0300
Subject: [PATCH] net: Restrict period when cs_vNodes mutex is locked

---
 src/init.cpp                       | 15 +--------------
 src/net.cpp                        |  8 ++++----
 src/test/fuzz/process_message.cpp  |  1 -
 src/test/fuzz/process_messages.cpp |  1 -
 4 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/src/init.cpp b/src/init.cpp
index 701a7529afa5c..280a0cbda7755 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -200,20 +200,7 @@ void Shutdown(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();
 
diff --git a/src/net.cpp b/src/net.cpp
index ae38acdc3cb2f..65f8b8baecf9b 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2636,8 +2636,9 @@ void CConnman::StopNodes()
     }
 
     // Close sockets
-    LOCK(cs_vNodes);
-    for (CNode* pnode : vNodes)
+    std::vector<CNode*> nodes;
+    WITH_LOCK(cs_vNodes, nodes.swap(vNodes));
+    for (CNode* pnode : nodes)
         pnode->CloseSocketDisconnect();
     for (ListenSocket& hListenSocket : vhListenSocket)
         if (hListenSocket.socket != INVALID_SOCKET)
@@ -2645,13 +2646,12 @@ void CConnman::StopNodes()
                 LogPrintf("CloseSocket(hListenSocket) failed with error %s\n", NetworkErrorString(WSAGetLastError()));
 
     // clean up some globals (to help leak detection)
-    for (CNode* pnode : vNodes) {
+    for (CNode* pnode : nodes) {
         DeleteNode(pnode);
     }
     for (CNode* pnode : vNodesDisconnected) {
         DeleteNode(pnode);
     }
-    vNodes.clear();
     vNodesDisconnected.clear();
     vhListenSocket.clear();
     semOutbound.reset();
diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp
index 96e1cfa08fb81..7b99193ad04f2 100644
--- a/src/test/fuzz/process_message.cpp
+++ b/src/test/fuzz/process_message.cpp
@@ -100,7 +100,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();
 }
 
diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp
index 203c0ef8e1573..11b236c9bdda2 100644
--- a/src/test/fuzz/process_messages.cpp
+++ b/src/test/fuzz/process_messages.cpp
@@ -80,6 +80,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();
 }