From af9ea55a72c94678b343f5dd98dc78f3a3ac58cb Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Wed, 5 Aug 2020 13:10:11 +0300 Subject: [PATCH 1/5] Use LockAssertion utility class instead of AssertLockHeld() This change prepares for upcoming commit "Do not hide compile-time thread safety warnings" by replacing AssertLockHeld() with LockAssertion() where needed. --- src/net_processing.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 60bdfbe9f5d..cd1e672c2fe 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -628,13 +628,12 @@ static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman& connma } } connman.ForNode(nodeid, [&connman](CNode* pfrom){ - AssertLockHeld(cs_main); + LockAssertion lock(::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 // blocks using compact encodings. connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [&connman, nCMPCTBLOCKVersion](CNode* pnodeStop){ - AssertLockHeld(cs_main); connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetSendVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/false, nCMPCTBLOCKVersion)); return true; }); @@ -1327,7 +1326,7 @@ void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std: } m_connman.ForEachNode([this, &pcmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) { - AssertLockHeld(cs_main); + LockAssertion lock(::cs_main); // TODO: Avoid the repeated-serialization here if (pnode->nVersion < INVALID_CB_NO_BAN_VERSION || pnode->fDisconnect) @@ -1469,7 +1468,8 @@ void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& { connman.ForEachNode([&txid, &wtxid](CNode* pnode) { - AssertLockHeld(cs_main); + LockAssertion lock(::cs_main); + CNodeState &state = *State(pnode->GetId()); if (state.m_wtxid_relay) { pnode->PushTxInventory(wtxid); @@ -3957,7 +3957,7 @@ void PeerLogicValidation::EvictExtraOutboundPeers(int64_t time_in_seconds) int64_t oldest_block_announcement = std::numeric_limits::max(); m_connman.ForEachNode([&](CNode* pnode) { - AssertLockHeld(cs_main); + LockAssertion lock(::cs_main); // Ignore non-outbound peers, or nodes marked for disconnect already if (!pnode->IsOutboundOrBlockRelayConn() || pnode->fDisconnect) return; @@ -3974,7 +3974,7 @@ void PeerLogicValidation::EvictExtraOutboundPeers(int64_t time_in_seconds) }); if (worst_peer != -1) { bool disconnected = m_connman.ForNode(worst_peer, [&](CNode *pnode) { - AssertLockHeld(cs_main); + LockAssertion lock(::cs_main); // Only disconnect a peer that has been connected to us for // some reasonable fraction of our check-frequency, to give From 3ddc150857178bfb1c854c05bf9b526777876f56 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Wed, 5 Aug 2020 15:00:50 +0300 Subject: [PATCH 2/5] Add missed thread safety annotations This is needed for upcoming commit "sync.h: Make runtime lock checks require compile-time lock checks" to pass. --- src/wallet/scriptpubkeyman.h | 2 +- src/wallet/wallet.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index a96d971734b..14fb1fa89f9 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -535,7 +535,7 @@ private: //! keeps track of whether Unlock has run a thorough check before bool m_decryption_thoroughly_checked = false; - bool AddDescriptorKeyWithDB(WalletBatch& batch, const CKey& key, const CPubKey &pubkey); + bool AddDescriptorKeyWithDB(WalletBatch& batch, const CKey& key, const CPubKey &pubkey) EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); KeyMap GetKeys() const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 2f9d3010007..06e4e7c3a3a 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1285,7 +1285,7 @@ public: void LoadActiveScriptPubKeyMan(uint256 id, OutputType type, bool internal); //! Create new DescriptorScriptPubKeyMans and add them to the wallet - void SetupDescriptorScriptPubKeyMans(); + void SetupDescriptorScriptPubKeyMans() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Return the DescriptorScriptPubKeyMan for a WalletDescriptor if it is already in the wallet DescriptorScriptPubKeyMan* GetDescriptorScriptPubKeyMan(const WalletDescriptor& desc) const; From 23d71d171e6e22ba5e4a909d597a54595b2a2c1f Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Wed, 5 Aug 2020 14:56:20 +0300 Subject: [PATCH 3/5] Do not hide compile-time thread safety warnings --- src/sync.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sync.h b/src/sync.h index 05ff2ee8a9c..c5f68933742 100644 --- a/src/sync.h +++ b/src/sync.h @@ -53,7 +53,7 @@ void LeaveCritical(); void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line); std::string LocksHeld(); template -void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) ASSERT_EXCLUSIVE_LOCK(cs); +void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs); void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs); void DeleteLock(void* cs); bool LockStackEmpty(); @@ -69,7 +69,7 @@ inline void EnterCritical(const char* pszName, const char* pszFile, int nLine, v inline void LeaveCritical() {} inline void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) {} template -inline void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) ASSERT_EXCLUSIVE_LOCK(cs) {} +inline void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) {} inline void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {} inline void DeleteLock(void* cs) {} inline bool LockStackEmpty() { return true; } From 2ee7743fe723227f2ea1b031eddb14fc6863f4c8 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Thu, 6 Aug 2020 02:55:31 +1000 Subject: [PATCH 4/5] sync.h: Make runtime lock checks require compile-time lock checks --- src/sync.cpp | 5 ++++- src/sync.h | 10 ++++++---- src/validation.h | 2 +- src/wallet/wallet.h | 2 +- 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index 4be13a3c48a..322198a852e 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -238,12 +238,15 @@ void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, template void AssertLockHeldInternal(const char*, const char*, int, Mutex*); template void AssertLockHeldInternal(const char*, const char*, int, RecursiveMutex*); -void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) +template +void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) { if (!LockHeld(cs)) return; tfm::format(std::cerr, "Assertion failed: lock %s held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld()); abort(); } +template void AssertLockNotHeldInternal(const char*, const char*, int, Mutex*); +template void AssertLockNotHeldInternal(const char*, const char*, int, RecursiveMutex*); void DeleteLock(void* cs) { diff --git a/src/sync.h b/src/sync.h index c5f68933742..7b397a80039 100644 --- a/src/sync.h +++ b/src/sync.h @@ -53,8 +53,9 @@ void LeaveCritical(); void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line); std::string LocksHeld(); template -void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs); -void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs); +void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs); +template +void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs); void DeleteLock(void* cs); bool LockStackEmpty(); @@ -69,8 +70,9 @@ inline void EnterCritical(const char* pszName, const char* pszFile, int nLine, v inline void LeaveCritical() {} inline void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) {} template -inline void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) {} -inline void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {} +inline void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs) {} +template +void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) {} inline void DeleteLock(void* cs) {} inline bool LockStackEmpty() { return true; } #endif diff --git a/src/validation.h b/src/validation.h index 534162d64aa..b5bf3e41a33 100644 --- a/src/validation.h +++ b/src/validation.h @@ -245,7 +245,7 @@ bool TestLockPointValidity(const LockPoints* lp) EXCLUSIVE_LOCKS_REQUIRED(cs_mai * * See consensus/consensus.h for flag definitions. */ -bool CheckSequenceLocks(const CTxMemPool& pool, const CTransaction& tx, int flags, LockPoints* lp = nullptr, bool useExistingLockPoints = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main); +bool CheckSequenceLocks(const CTxMemPool& pool, const CTransaction& tx, int flags, LockPoints* lp = nullptr, bool useExistingLockPoints = false) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, pool.cs); /** * Closure representing one script verification diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 06e4e7c3a3a..a62e7b18bed 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1179,7 +1179,7 @@ public: * Obviously holding cs_main/cs_wallet when going into this call may cause * deadlock */ - void BlockUntilSyncedToCurrentChain() const LOCKS_EXCLUDED(cs_main, cs_wallet); + void BlockUntilSyncedToCurrentChain() const EXCLUSIVE_LOCKS_REQUIRED(!::cs_main, !cs_wallet); /** set a single wallet flag */ void SetWalletFlag(uint64_t flags); From ea74e10acf17903e44c85e3678853414653dd4e1 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sat, 8 Aug 2020 12:56:28 +0300 Subject: [PATCH 5/5] doc: Add best practice for annotating/asserting locks --- doc/developer-notes.md | 66 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 6ae7e770e8b..ef9ecbb0857 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -746,6 +746,72 @@ the upper cycle, etc. Threads and synchronization ---------------------------- +- Prefer `Mutex` type to `RecursiveMutex` one + +- Consistently use [Clang Thread Safety Analysis](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) annotations to + get compile-time warnings about potential race conditions in code. Combine annotations in function declarations with + run-time asserts in function definitions: + +```C++ +// txmempool.h +class CTxMemPool +{ +public: + ... + mutable RecursiveMutex cs; + ... + void UpdateTransactionsFromBlock(...) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, cs); + ... +} + +// txmempool.cpp +void CTxMemPool::UpdateTransactionsFromBlock(...) +{ + AssertLockHeld(::cs_main); + AssertLockHeld(cs); + ... +} +``` + +```C++ +// validation.h +class ChainstateManager +{ +public: + ... + bool ProcessNewBlock(...) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main); + ... +} + +// validation.cpp +bool ChainstateManager::ProcessNewBlock(...) +{ + AssertLockNotHeld(::cs_main); + ... + LOCK(::cs_main); + ... +} +``` + +- 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`.