Merge bitcoin#19979: Replace LockAssertion with AssertLockHeld, remov…

…e LockAssertion

0bd1184 Remove unused LockAssertion struct (Hennadii Stepanov)
ab2a442 Replace LockAssertion with a proper thread safety annotations (Hennadii Stepanov)
73f71e1 refactor: Use explicit function type instead of template (Hennadii Stepanov)

Pull request description:

  This PR replaces `LockAssertion` with `AssertLockHeld`, and removes `LockAssertion`.

  This PR is compared with alternatives in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/AssertLockHeld-PRs

ACKs for top commit:
  MarcoFalke:
    ACK 0bd1184
  ajtowns:
    ACK 0bd1184
  vasild:
    ACK 0bd1184

Tree-SHA512: ef7780dd689faf0bb479fdb97c49bc652e2dd10c148234bb95502dfbb676442d8565ee37864d923ca21a25f9dc2a335bf46ee82c095e387b59a664ab05c0ae41

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.
 

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<CDeterministicMNCPtr> 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<CDeterministicMNCPtr> ret;
             for (auto it = masternodePendingProbes.begin(); it != masternodePendingProbes.end(); ) {
                 auto dmn = mnList.GetMN(*it);

src/net.h

@@ -1275,6 +1275,8 @@ friend class CNode;
         return ForNode(id, FullyConnectedOnly, func);
     }
 
+    using NodeFn = std::function<void(CNode*)>;
+
     bool IsConnected(const CService& addr, std::function<bool(const CNode* pnode)> cond)
     {
         return ForNode(addr, cond, [](CNode* pnode){
@@ -1331,10 +1333,9 @@ friend class CNode;
         }
     };
 
-    template<typename Callable>
-    void ForEachNode(Callable&& func)
+    void ForEachNode(const NodeFn& fn)
     {
-        ForEachNode(FullyConnectedOnly, func);
+        ForEachNode(FullyConnectedOnly, fn);
     }
 
     template<typename Condition, typename Callable>
@@ -1347,10 +1348,9 @@ friend class CNode;
         }
     };
 
-    template<typename Callable>
-    void ForEachNode(Callable&& func) const
+    void ForEachNode(const NodeFn& fn) const
     {
-        ForEachNode(FullyConnectedOnly, func);
+        ForEachNode(FullyConnectedOnly, fn);
     }
 
     template<typename Condition, typename Callable, typename CallableAfter>

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

src/sync.h

@@ -452,18 +452,4 @@ class CSemaphoreGrant
     }
 };
 
-// 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 <typename Mutex>
-    explicit LockAssertion(Mutex& mutex) EXCLUSIVE_LOCK_FUNCTION(mutex)
-    {
-#ifdef DEBUG_LOCKORDER
-        AssertLockHeld(mutex);
-#endif
-    }
-    ~LockAssertion() UNLOCK_FUNCTION() {}
-};
-
 #endif // BITCOIN_SYNC_H