From 344e831de54f7b864f03a90f6cb19692eafcd463 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Sat, 20 Jun 2020 23:16:20 -0400 Subject: [PATCH 1/3] [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. --- src/net.h | 12 ------------ src/net_processing.cpp | 9 +++++---- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/src/net.h b/src/net.h index b461470f1fc..ddb185763f3 100644 --- a/src/net.h +++ b/src/net.h @@ -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 &m_asmap); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index d7fbf6941d3..f678b73a42d 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1328,9 +1328,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); } } }); @@ -1607,7 +1608,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 vInv; @@ -2666,7 +2667,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 @@ -4083,7 +4084,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()); } From 3556227ddd3365cfac43b307204d73058b2943f0 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Sat, 20 Jun 2020 23:17:29 -0400 Subject: [PATCH 2/3] [net] Make cs_inventory a non-recursive mutex cs_inventory is never taken recursively. Make it a non-recursive mutex. --- src/net.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net.h b/src/net.h index ddb185763f3..3c237639545 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 vInventoryBlockToSend GUARDED_BY(cs_inventory); - RecursiveMutex cs_inventory; + Mutex cs_inventory; struct TxRelay { mutable RecursiveMutex cs_filter; From e8a2822119233ade0de84f791a9e92918a3d6896 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Sat, 20 Jun 2020 23:23:14 -0400 Subject: [PATCH 3/3] [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(-) diff --git a/src/net.cpp b/src/net.cpp index 371fbeed593..391391a7da0 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) {