[net] Make cs_inventory nonrecursive

Summary:
> [net processing] Remove PushBlockInventory and PushBlockHash
>
> PushBlockInventory() and PushBlockHash() are functions that can
> be replaced with single-line statements. This also eliminates
> the single place that cs_inventory is taken recursively.

> [net] Make cs_inventory a non-recursive mutex
>
> cs_inventory is never taken recursively. Make it a non-recursive mutex.

> [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.

This is a backport of [[bitcoin/bitcoin#19347 | core#19347]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D9433

src/net.cpp

@@ -1214,12 +1214,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) {

src/net.h

@@ -959,7 +959,7 @@ class CNode {
     // There is no final sorting before sending, as they are always sent
     // immediately and in the order requested.
     std::vector<BlockHash> vInventoryBlockToSend GUARDED_BY(cs_inventory);
-    RecursiveMutex cs_inventory;
+    Mutex cs_inventory;
 
     struct TxRelay {
         mutable RecursiveMutex cs_filter;
@@ -1161,16 +1161,6 @@ class CNode {
         }
     }
 
-    void PushBlockInventory(const BlockHash &blockhash) {
-        LOCK(cs_inventory);
-        vInventoryBlockToSend.push_back(blockhash);
-    }
-
-    void PushBlockHash(const BlockHash &hash) {
-        LOCK(cs_inventory);
-        vBlockHashesToAnnounce.push_back(hash);
-    }
-
     void CloseSocketDisconnect();
 
     void copyStats(CNodeStats &stats, const std::vector<bool> &m_asmap);

src/net_processing.cpp

@@ -1654,11 +1654,12 @@ void PeerManager::UpdatedBlockTip(const CBlockIndex *pindexNew,
         // Relay inventory, but don't relay old inventory during initial block
         // download.
         m_connman.ForEachNode([nNewHeight, &vHashes](CNode *pnode) {
+            LOCK(pnode->cs_inventory);
             if (nNewHeight > (pnode->nStartingHeight != -1
                                   ? pnode->nStartingHeight - 2000
                                   : 0)) {
                 for (const BlockHash &hash : reverse_iterate(vHashes)) {
-                    pnode->PushBlockHash(hash);
+                    pnode->vBlockHashesToAnnounce.push_back(hash);
                 }
             }
         });
@@ -1961,7 +1962,7 @@ static void ProcessGetBlockData(const Config &config, CNode &pfrom,
         // Trigger the peer node to send a getblocks request for the next batch
         // of inventory.
         if (hash == pfrom.hashContinue) {
-            // Bypass PushBlockInventory, this must send even if redundant, and
+            // 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;
@@ -3230,7 +3231,8 @@ void PeerManager::ProcessMessage(const Config &config, CNode &pfrom,
                     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
                 // trigger the peer to getblocks the next batch of inventory.
@@ -5131,7 +5133,7 @@ bool PeerManager::SendMessages(const Config &config, 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());