Merge #19347: [net] Make cs_inventory nonrecursive

e8a2822119233ade0de84f791a9e92918a3d6896 [net] Don't try to take cs_inventory before deleting CNode (John Newbery)
3556227ddd3365cfac43b307204d73058b2943f0 [net] Make cs_inventory a non-recursive mutex (John Newbery)
344e831de54f7b864f03a90f6cb19692eafcd463 [net processing] Remove PushBlockInventory and PushBlockHash (John Newbery)

Pull request description:

  - Remove PushBlockInventory() and PushBlockHash(). These are one-line functions that can easy be inlined into the calling code. Doing so also allows us to eliminate the one place that cs_inventory is recursively locked.
  - Make cs_inventory a nonrecursive mutex
  - Remove a redundant TRY_LOCK of cs_inventory when deleting CNode.

ACKs for top commit:
  sipa:
    utACK e8a2822119233ade0de84f791a9e92918a3d6896
  MarcoFalke:
    ACK e8a2822119233ade0de84f791a9e92918a3d6896 🍬
  hebasto:
    re-ACK e8a2822119233ade0de84f791a9e92918a3d6896

Tree-SHA512: dbc721d102cdef7b5827a8f2549daf8b54f543050266999a7ea56c9f36618565b71e31ce0beb1209ba2db43d15388be173355a03fb6db8ad24e2475b145050bd

3 files changed, 9 insertions, 23 deletions

diff --git a/src/net.cpp b/src/net.cpp
index 0233b0fc2f..21d340b516 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1110,12 +1110,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) {
diff --git a/src/net.h b/src/net.h
index b461470f1f..3c23763954 100644
--- a/src/net.h
+++ b/src/net.h
@@ -803,7 +803,7 @@ public:
     // There is no final sorting before sending, as they are always sent immediately
     // and in the order requested.
     std::vector<uint256> vInventoryBlockToSend GUARDED_BY(cs_inventory);
-    RecursiveMutex cs_inventory;
+    Mutex cs_inventory;
 
     struct TxRelay {
         mutable RecursiveMutex cs_filter;
@@ -982,18 +982,6 @@ public:
         }
     }
 
-    void PushBlockInventory(const uint256& hash)
-    {
-        LOCK(cs_inventory);
-        vInventoryBlockToSend.push_back(hash);
-    }
-
-    void PushBlockHash(const uint256 &hash)
-    {
-        LOCK(cs_inventory);
-        vBlockHashesToAnnounce.push_back(hash);
-    }
-
     void CloseSocketDisconnect();
 
     void copyStats(CNodeStats &stats, const std::vector<bool> &m_asmap);
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index d0c3c943fc..f1a89a8936 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1325,9 +1325,10 @@ void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CB
         }
         // Relay inventory, but don't relay old inventory during initial block download.
         connman->ForEachNode([nNewHeight, &vHashes](CNode* pnode) {
+            LOCK(pnode->cs_inventory);
             if (nNewHeight > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : 0)) {
                 for (const uint256& hash : reverse_iterate(vHashes)) {
-                    pnode->PushBlockHash(hash);
+                    pnode->vBlockHashesToAnnounce.push_back(hash);
                 }
             }
         });
@@ -1604,7 +1605,7 @@ void static ProcessGetBlockData(CNode& pfrom, const CChainParams& chainparams, c
         // Trigger the peer node to send a getblocks request for the next batch of inventory
         if (inv.hash == pfrom.hashContinue)
         {
-            // Bypass PushBlockInventory, this must send even if redundant,
+            // Send immediately. This must send even if redundant,
             // and we want it right after the last block so they don't
             // wait for other stuff first.
             std::vector<CInv> vInv;
@@ -2664,7 +2665,7 @@ void ProcessMessage(
                 LogPrint(BCLog::NET, " getblocks stopping, pruned or too old block at %d %s\n", pindex->nHeight, pindex->GetBlockHash().ToString());
                 break;
             }
-            pfrom.PushBlockInventory(pindex->GetBlockHash());
+            WITH_LOCK(pfrom.cs_inventory, pfrom.vInventoryBlockToSend.push_back(pindex->GetBlockHash()));
             if (--nLimit <= 0)
             {
                 // When this block is requested, we'll send an inv that'll
@@ -4082,7 +4083,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
 
                     // If the peer's chain has this block, don't inv it back.
                     if (!PeerHasHeader(&state, pindex)) {
-                        pto->PushBlockInventory(hashToAnnounce);
+                        pto->vInventoryBlockToSend.push_back(hashToAnnounce);
                         LogPrint(BCLog::NET, "%s: sending inv peer=%d hash=%s\n", __func__,
                             pto->GetId(), hashToAnnounce.ToString());
                     }