From 73f71e19965e07534eb47701f2b23c9ed59ef475 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sat, 19 Sep 2020 11:52:27 +0300 Subject: [PATCH 1/3] refactor: Use explicit function type instead of template --- src/net.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/net.h b/src/net.h index 60c3dc6aefc..b0294d5b153 100644 --- a/src/net.h +++ b/src/net.h @@ -258,8 +258,8 @@ public: void PushMessage(CNode* pnode, CSerializedNetMsg&& msg); - template - void ForEachNode(Callable&& func) + using NodeFn = std::function; + void ForEachNode(const NodeFn& func) { LOCK(cs_vNodes); for (auto&& node : vNodes) { @@ -268,8 +268,7 @@ public: } }; - template - void ForEachNode(Callable&& func) const + void ForEachNode(const NodeFn& func) const { LOCK(cs_vNodes); for (auto&& node : vNodes) { From ab2a44297fd0796bf5797ae2a477e8e56d9c3c12 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sat, 19 Sep 2020 11:56:28 +0300 Subject: [PATCH 2/3] Replace LockAssertion with a proper thread safety annotations --- src/net_processing.cpp | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 166adc05bb8..7cf6da270ee 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -662,8 +662,8 @@ static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman& connma return; } } - connman.ForNode(nodeid, [&connman](CNode* pfrom){ - LockAssertion lock(::cs_main); + connman.ForNode(nodeid, [&connman](CNode* pfrom) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); uint64_t nCMPCTBLOCKVersion = (pfrom->GetLocalServices() & NODE_WITNESS) ? 2 : 1; if (lNodesAnnouncingHeaderAndIDs.size() >= 3) { // As per BIP152, we only get 3 of our peers to announce @@ -1355,8 +1355,8 @@ void PeerManager::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ fWitnessesPresentInMostRecentCompactBlock = fWitnessEnabled; } - m_connman.ForEachNode([this, &pcmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) { - LockAssertion lock(::cs_main); + m_connman.ForEachNode([this, &pcmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); // TODO: Avoid the repeated-serialization here if (pnode->nVersion < INVALID_CB_NO_BAN_VERSION || pnode->fDisconnect) @@ -1489,9 +1489,8 @@ bool static AlreadyHaveBlock(const uint256& block_hash) EXCLUSIVE_LOCKS_REQUIRED void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman) { - connman.ForEachNode([&txid, &wtxid](CNode* pnode) - { - LockAssertion lock(::cs_main); + connman.ForEachNode([&txid, &wtxid](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); CNodeState* state = State(pnode->GetId()); if (state == nullptr) return; @@ -3979,8 +3978,8 @@ void PeerManager::EvictExtraOutboundPeers(int64_t time_in_seconds) NodeId worst_peer = -1; int64_t oldest_block_announcement = std::numeric_limits::max(); - m_connman.ForEachNode([&](CNode* pnode) { - LockAssertion lock(::cs_main); + m_connman.ForEachNode([&](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); // Ignore non-outbound peers, or nodes marked for disconnect already if (!pnode->IsOutboundOrBlockRelayConn() || pnode->fDisconnect) return; @@ -3996,8 +3995,8 @@ void PeerManager::EvictExtraOutboundPeers(int64_t time_in_seconds) } }); 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 From 0bd1184adf6610c0bd14f4e9a25c0a200e65ae25 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sat, 19 Sep 2020 12:06:36 +0300 Subject: [PATCH 3/3] Remove unused LockAssertion struct --- doc/developer-notes.md | 19 ------------------- src/sync.h | 14 -------------- 2 files changed, 33 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index ef9ecbb0857..fa188dbcd60 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -793,25 +793,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. As of 0.12, this is defined by default when configuring with `--enable-debug`. diff --git a/src/sync.h b/src/sync.h index 7b397a80039..41f4e43bdd5 100644 --- a/src/sync.h +++ b/src/sync.h @@ -352,18 +352,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