diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 248557919a..22785e7349 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -899,6 +899,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. diff --git a/src/sync.cpp b/src/sync.cpp index 35d3ffaf12..7540819262 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -285,12 +285,16 @@ template void AssertLockHeldInternal(const char*, const char*, int, Mutex*); template void AssertLockHeldInternal(const char*, const char*, int, RecursiveMutex*); template void AssertLockHeldInternal(const char*, const char*, int, SharedMutex*); -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*); +template void AssertLockNotHeldInternal(const char*, const char*, int, SharedMutex*); void DeleteLock(void* cs) { diff --git a/src/sync.h b/src/sync.h index 3712bd9824..8d02830726 100644 --- a/src/sync.h +++ b/src/sync.h @@ -57,8 +57,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) ASSERT_EXCLUSIVE_LOCK(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(); @@ -74,8 +75,9 @@ inline void EnterCritical(const char* pszName, const char* pszFile, int nLine, M 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 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/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 05f2452d2f..e3f1916272 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -528,7 +528,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 72c4c747ba..88ec3db2f1 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1402,7 +1402,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); @@ -1515,7 +1515,7 @@ public: void DeactivateScriptPubKeyMan(uint256 id, 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;