diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 9e9fb162e9..162547f1dd 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -946,25 +946,6 @@ bool ChainstateManager::ProcessNewBlock(...) } ``` -- When Clang Thread Safety Analysis is unable to determine if a mutex is locked, use `LockAssertion` class instances: - -```C++ -// net_processing.h -void RelayTransaction(...) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - -// net_processing.cpp -void RelayTransaction(...) -{ - AssertLockHeld(::cs_main); - - connman.ForEachNode([&txid, &wtxid](CNode* pnode) { - LockAssertion lock(::cs_main); - ... - }); -} - -``` - - Build and run tests with `-DDEBUG_LOCKORDER` to verify that no potential deadlocks are introduced. diff --git a/src/net.cpp b/src/net.cpp index 49e9ca2c82..2b6fd4608b 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3510,8 +3510,8 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, MasternodeProbeConn isProbe = MasternodeProbeConn::IsNotConnection; - const auto getPendingQuorumNodes = [&]() { - LockAssertion lock(cs_vPendingMasternodes); + const auto getPendingQuorumNodes = [&]() EXCLUSIVE_LOCKS_REQUIRED(cs_vPendingMasternodes) { + AssertLockHeld(cs_vPendingMasternodes); std::vector ret; for (const auto& group : masternodeQuorumNodes) { for (const auto& proRegTxHash : group.second) { @@ -3549,8 +3549,8 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, return ret; }; - const auto getPendingProbes = [&]() { - LockAssertion lock(cs_vPendingMasternodes); + const auto getPendingProbes = [&]() EXCLUSIVE_LOCKS_REQUIRED(cs_vPendingMasternodes) { + AssertLockHeld(cs_vPendingMasternodes); std::vector ret; for (auto it = masternodePendingProbes.begin(); it != masternodePendingProbes.end(); ) { auto dmn = mnList.GetMN(*it); diff --git a/src/net.h b/src/net.h index 248a8a6b04..c34c87104a 100644 --- a/src/net.h +++ b/src/net.h @@ -1275,6 +1275,8 @@ public: return ForNode(id, FullyConnectedOnly, func); } + using NodeFn = std::function; + bool IsConnected(const CService& addr, std::function cond) { return ForNode(addr, cond, [](CNode* pnode){ @@ -1331,10 +1333,9 @@ public: } }; - template - void ForEachNode(Callable&& func) + void ForEachNode(const NodeFn& fn) { - ForEachNode(FullyConnectedOnly, func); + ForEachNode(FullyConnectedOnly, fn); } template @@ -1347,10 +1348,9 @@ public: } }; - template - void ForEachNode(Callable&& func) const + void ForEachNode(const NodeFn& fn) const { - ForEachNode(FullyConnectedOnly, func); + ForEachNode(FullyConnectedOnly, fn); } template diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 688a01d19d..d05eb8917e 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2000,8 +2000,8 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha m_most_recent_compact_block = pcmpctblock; } - m_connman.ForEachNode([this, pindex, &lazy_ser, &hashBlock](CNode* pnode) { - LockAssertion lock(::cs_main); + m_connman.ForEachNode([this, pindex, &lazy_ser, &hashBlock](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); // TODO: Avoid the repeated-serialization here if (pnode->fDisconnect) return; @@ -5275,8 +5275,8 @@ void PeerManagerImpl::EvictExtraOutboundPeers(std::chrono::seconds now) // We want to prevent recently connected to Onion peers from being disconnected here, protect them as long as // there are more non_onion nodes than onion nodes so far size_t onion_count = 0; - m_connman.ForEachNode([&](CNode* pnode) { - LockAssertion lock(::cs_main); + m_connman.ForEachNode([&](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); if (pnode->addr.IsTor() && ++onion_count <= m_connman.GetMaxOutboundOnionNodeCount()) return; // Don't disconnect masternodes just because they were slow in block announcement if (pnode->m_masternode_connection) return; @@ -5293,8 +5293,8 @@ void PeerManagerImpl::EvictExtraOutboundPeers(std::chrono::seconds now) } }); if (worst_peer != -1) { - bool disconnected = m_connman.ForNode(worst_peer, [&](CNode *pnode) { - LockAssertion lock(::cs_main); + bool disconnected = m_connman.ForNode(worst_peer, [&](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); // Only disconnect a peer that has been connected to us for // some reasonable fraction of our check-frequency, to give diff --git a/src/sync.h b/src/sync.h index cab9d559d5..078ee4f6e8 100644 --- a/src/sync.h +++ b/src/sync.h @@ -452,18 +452,4 @@ public: } }; -// Utility class for indicating to compiler thread analysis that a mutex is -// locked (when it couldn't be determined otherwise). -struct SCOPED_LOCKABLE LockAssertion -{ - template - explicit LockAssertion(Mutex& mutex) EXCLUSIVE_LOCK_FUNCTION(mutex) - { -#ifdef DEBUG_LOCKORDER - AssertLockHeld(mutex); -#endif - } - ~LockAssertion() UNLOCK_FUNCTION() {} -}; - #endif // BITCOIN_SYNC_H