aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--doc/developer-notes.md66
-rw-r--r--src/net_processing.cpp12
-rw-r--r--src/sync.cpp5
-rw-r--r--src/sync.h10
-rw-r--r--src/validation.h2
-rw-r--r--src/wallet/scriptpubkeyman.h2
-rw-r--r--src/wallet/wallet.h4
7 files changed, 86 insertions, 15 deletions
diff --git a/doc/developer-notes.md b/doc/developer-notes.md
index 6ae7e770e8..ef9ecbb085 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`.
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index adad428413..e2efbd8804 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -663,13 +663,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;
});
@@ -1371,7 +1370,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)
@@ -1513,7 +1512,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);
@@ -3993,7 +3993,7 @@ void PeerLogicValidation::EvictExtraOutboundPeers(int64_t time_in_seconds)
int64_t oldest_block_announcement = std::numeric_limits<int64_t>::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;
@@ -4010,7 +4010,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
diff --git a/src/sync.cpp b/src/sync.cpp
index 4be13a3c48..322198a852 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 <typename MutexType>
+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 05ff2ee8a9..7b397a8003 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 <typename MutexType>
-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 <typename MutexType>
+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 <typename MutexType>
-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 <typename MutexType>
+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 bb59b57f6b..cac9473c7a 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/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index a96d971734..14fb1fa89f 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 111e00f646..42622c9312 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -1178,7 +1178,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);
@@ -1284,7 +1284,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;
generated by cgit v1.2.3 (git 2.26.2) at 2024-07-19 00:03:25 +0000