diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 9096bf352a5da1..f5307ac0dbfa8a 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -853,6 +853,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/coinjoin/coinjoin.h b/src/coinjoin/coinjoin.h index b31a3d09cbcc1d..51208f5f32b19e 100644 --- a/src/coinjoin/coinjoin.h +++ b/src/coinjoin/coinjoin.h @@ -365,15 +365,15 @@ class CDSTXManager public: CDSTXManager() = default; - void AddDSTX(const CCoinJoinBroadcastTx& dstx) LOCKS_EXCLUDED(cs_mapdstx); - CCoinJoinBroadcastTx GetDSTX(const uint256& hash) LOCKS_EXCLUDED(cs_mapdstx); + void AddDSTX(const CCoinJoinBroadcastTx& dstx) EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx); + CCoinJoinBroadcastTx GetDSTX(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx); void UpdatedBlockTip(const CBlockIndex* pindex, const llmq::CChainLocksHandler& clhandler, const CMasternodeSync& mn_sync); void NotifyChainLock(const CBlockIndex* pindex, const llmq::CChainLocksHandler& clhandler, const CMasternodeSync& mn_sync); - void TransactionAddedToMempool(const CTransactionRef& tx) LOCKS_EXCLUDED(cs_mapdstx); - void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindex) LOCKS_EXCLUDED(cs_mapdstx); - void BlockDisconnected(const std::shared_ptr& pblock, const CBlockIndex*) LOCKS_EXCLUDED(cs_mapdstx); + void TransactionAddedToMempool(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx); + void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx); + void BlockDisconnected(const std::shared_ptr& pblock, const CBlockIndex*)EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx); private: void CheckDSTXes(const CBlockIndex* pindex, const llmq::CChainLocksHandler& clhandler); diff --git a/src/sync.cpp b/src/sync.cpp index 7849ba9d84daa8..47ab4995157a4a 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -292,12 +292,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 111bf61f01be21..034e1a977c6b1d 100644 --- a/src/sync.h +++ b/src/sync.h @@ -54,8 +54,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(); @@ -71,8 +72,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 c87eb9d7d847d0..ca537b6c80f328 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -525,7 +525,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan //! 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 882d883b1fbb69..388a8f06efe040 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1303,7 +1303,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati * 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); @@ -1405,7 +1405,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati void SetActiveScriptPubKeyMan(uint256 id, bool internal, bool memonly = false); //! 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;