From e8a2822119233ade0de84f791a9e92918a3d6896 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Sat, 20 Jun 2020 23:23:14 -0400
Subject: [net] Don't try to take cs_inventory before deleting CNode

The TRY_LOCK(cs_inventory) in DisconnectNodes() is taken after the CNode
object has been removed from vNodes and when the CNode's nRefCount is
zero.

The only other places that cs_inventory can be taken are:

- In ProcessMessages() or SendMessages(), when the CNode's nRefCount
must be >0 (see ThreadMessageHandler(), where the refcount is
incremented before calling ProcessMessages() and SendMessages()).
- In a ForEachNode() lambda in PeerLogicValidation::UpdatedBlockTip().
ForEachNode() locks cs_vNodes and calls the function on the CNode
objects in vNodes.

Therefore, cs_inventory is never locked by another thread when the
TRY_LOCK(cs_inventory) is reached in DisconnectNodes(). Since the
only purpose of this TRY_LOCK is to ensure that the lock is not
taken by another thread, this always succeeds. Remove the check.
---
 src/net.cpp | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

(limited to 'src/net.cpp')

diff --git a/src/net.cpp b/src/net.cpp
index 371fbeed59..391391a7da 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1103,12 +1103,9 @@ void CConnman::DisconnectNodes()
             if (pnode->GetRefCount() <= 0) {
                 bool fDelete = false;
                 {
-                    TRY_LOCK(pnode->cs_inventory, lockInv);
-                    if (lockInv) {
-                        TRY_LOCK(pnode->cs_vSend, lockSend);
-                        if (lockSend) {
-                            fDelete = true;
-                        }
+                    TRY_LOCK(pnode->cs_vSend, lockSend);
+                    if (lockSend) {
+                        fDelete = true;
                     }
                 }
                 if (fDelete) {
-- 
cgit v1.2.3