From 6ed62b323c51d20359b53ce5ac3af76db756c5f0 Mon Sep 17 00:00:00 2001
From: MarcoFalke <falke.marco@gmail.com>
Date: Sun, 25 Apr 2021 10:08:52 +0200
Subject: [PATCH] Merge bitcoin/bitcoin#21563: net: Restrict period when
 cs_vNodes mutex is locked
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

8c8237a4a10feb2ac9ce46f67b5d14bf879b670f net, refactor: Fix style in CConnman::StopNodes (Hennadii Stepanov)
229ac1892d807a1eea5a7c24ae0fe27dc913b1bd net: Combine two loops into one, and update comments (Hennadii Stepanov)
a3d090d1103cd6c25daf07afdf4e65febca6d3f7 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 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f
  vasild:
    ACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f
  ajtowns:
    utACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f - logic seems sound
  MarcoFalke:
    review ACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f 👢

Tree-SHA512: a8277924339622b188b12d260a100adf5d82781634cf974320cf6007341f946a7ff40351137c2f5369aed0d318f38aac2d32965c9b619432440d722a4e78bb73
---
 src/init.cpp                       | 15 +--------------
 src/net.cpp                        | 20 ++++++++------------
 src/test/fuzz/process_message.cpp  |  1 -
 src/test/fuzz/process_messages.cpp |  1 -
 4 files changed, 9 insertions(+), 28 deletions(-)

diff --git a/src/init.cpp b/src/init.cpp
index e92742be1d..cbae172cb2 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -254,20 +254,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();
 
diff --git a/src/net.cpp b/src/net.cpp
index c884ed264c..67dee39d44 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -4301,13 +4301,15 @@ void CConnman::StopNodes()
         }
     }
 
-    {
-        LOCK(m_nodes_mutex);
-
-        // Close sockets
-        for (CNode *pnode : m_nodes)
-            pnode->CloseSocketDisconnect(this);
+    // Delete peer connections.
+    std::vector<CNode*> nodes;
+    WITH_LOCK(m_nodes_mutex, nodes.swap(m_nodes));
+    for (CNode *pnode : nodes) {
+        pnode->CloseSocketDisconnect(this);
+        DeleteNode(pnode);
     }
+
+    // Close listening sockets.
     for (ListenSocket& hListenSocket : vhListenSocket) {
         if (hListenSocket.sock) {
             if (m_edge_trig_events && !m_edge_trig_events->RemoveSocket(hListenSocket.sock->Get())) {
@@ -4316,12 +4318,6 @@ void CConnman::StopNodes()
         }
     }
 
-    // clean up some globals (to help leak detection)
-    std::vector<CNode*> nodes;
-    WITH_LOCK(m_nodes_mutex, nodes.swap(m_nodes));
-    for (CNode* pnode : nodes) {
-        DeleteNode(pnode);
-    }
     for (CNode* pnode : m_nodes_disconnected) {
         DeleteNode(pnode);
     }
diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp
index 7c042b5cb0..a5a59e3154 100644
--- a/src/test/fuzz/process_message.cpp
+++ b/src/test/fuzz/process_message.cpp
@@ -103,7 +103,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 938f189db1..5eb6e14925 100644
--- a/src/test/fuzz/process_messages.cpp
+++ b/src/test/fuzz/process_messages.cpp
@@ -76,6 +76,5 @@ FUZZ_TARGET_INIT(process_messages, initialize_process_messages)
         g_setup->m_node.peerman->SendMessages(&random_node);
     }
     SyncWithValidationInterfaceQueue();
-    LOCK2(::cs_main, g_cs_orphans); // See init.cpp for rationale for implicit locking order requirement
     g_setup->m_node.connman->StopNodes();
 }