From 23b83109eaacb64872be21b460b1bc8758cb4fd2 Mon Sep 17 00:00:00 2001
From: MarcoFalke <falke.marco@gmail.com>
Date: Mon, 3 May 2021 08:13:49 +0200
Subject: [PATCH] Merge bitcoin/bitcoin#21750: net: remove unnecessary check of
 CNode::cs_vSend
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

9096b13a4764873511b65f32a005ce4738b0d81c net: remove unnecessary check of CNode::cs_vSend (Vasil Dimov)

Pull request description:

  It is not possible to have a node in `CConnman::vNodesDisconnected` and
  its reference count to be incremented - all `CNode::AddRef()` are done
  either before the node is added to `CConnman::vNodes` or while holding
  `CConnman::cs_vNodes` and the object being in `CConnman::vNodes`.

  So, the object being in `CConnman::vNodesDisconnected` and its reference
  count being zero means that it is not and will not start to be used by
  other threads.

  So, the lock of `CNode::cs_vSend` in `CConnman::DisconnectNodes()` will
  always succeed and is not necessary.

  Indeed all locks of `CNode::cs_vSend` are done either when the reference
  count is >0 or under the protection of `CConnman::cs_vNodes` and the
  node being in `CConnman::vNodes`.

ACKs for top commit:
  MarcoFalke:
    review ACK 9096b13a4764873511b65f32a005ce4738b0d81c 🏧
  jnewbery:
    utACK 9096b13a4764873511b65f32a005ce4738b0d81c

Tree-SHA512: 910899cdcdc8934642eb0c40fcece8c3b01b7e20a0b023966b9d6972db6a885cb3a9a04e9562bae14d5833967e45e2ecb3687b94d495060c3da4b1f2afb0ac8f
---
 src/net.cpp | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/src/net.cpp b/src/net.cpp
index 6ae5c88943..86605bb5ef 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1566,21 +1566,11 @@ void CConnman::DisconnectNodes()
         for (auto it = m_nodes_disconnected.begin(); it != m_nodes_disconnected.end(); )
         {
             CNode* pnode = *it;
-            // wait until threads are done using it
-            bool fDelete = false;
+            // Destroy the object only after other threads have stopped using it.
             if (pnode->GetRefCount() <= 0) {
-                {
-                    TRY_LOCK(pnode->cs_vSend, lockSend);
-                    if (lockSend) {
-                        fDelete = true;
-                    }
-                }
-                if (fDelete) {
-                    it = m_nodes_disconnected.erase(it);
-                    DeleteNode(pnode);
-                }
-            }
-            if (!fDelete) {
+                it = m_nodes_disconnected.erase(it);
+                DeleteNode(pnode);
+            } else {
                 ++it;
             }
         }