From 6f194e3b9839117b1b23c7fbb9626ab6abeb7efa Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 9 Oct 2025 20:28:06 +0700 Subject: [PATCH 1/6] refactor: merge mutexes cs_mapSporksCachedActive and cs_mapSporksCachedValues to cs_cache Motivation: they are captured almost always together. Get one mutex is faster than get 2, there are no performance benefits Example of usage: WITH_LOCK(cs_mapSporksCachedActive, mapSporksCachedActive.erase(spork.nSporkID)); WITH_LOCK(cs_mapSporksCachedValues, mapSporksCachedValues.erase(spork.nSporkID)); --- src/spork.cpp | 21 +++++++++++++-------- src/spork.h | 10 +++++----- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/spork.cpp b/src/spork.cpp index 021ab690e139..0b66222d6911 100644 --- a/src/spork.cpp +++ b/src/spork.cpp @@ -31,7 +31,7 @@ std::optional CSporkManager::SporkValueIfActive(SporkId nSporkID) co if (!mapSporksActive.count(nSporkID)) return std::nullopt; { - LOCK(cs_mapSporksCachedValues); + LOCK(cs_cache); if (auto it = mapSporksCachedValues.find(nSporkID); it != mapSporksCachedValues.end()) { return {it->second}; } @@ -45,7 +45,7 @@ std::optional CSporkManager::SporkValueIfActive(SporkId nSporkID) co // nMinSporkKeys is always more than the half of the max spork keys number, // so there is only one such value and we can stop here { - LOCK(cs_mapSporksCachedValues); + LOCK(cs_cache); mapSporksCachedValues[nSporkID] = spork.nValue; } return {spork.nValue}; @@ -184,8 +184,11 @@ MessageProcessingResult CSporkManager::ProcessSpork(NodeId from, CDataStream& vR mapSporksByHash[hash] = spork; mapSporksActive[spork.nSporkID][keyIDSigner] = spork; // Clear cached values on new spork being processed - WITH_LOCK(cs_mapSporksCachedActive, mapSporksCachedActive.erase(spork.nSporkID)); - WITH_LOCK(cs_mapSporksCachedValues, mapSporksCachedValues.erase(spork.nSporkID)); + { + LOCK(cs_cache); + mapSporksCachedActive.erase(spork.nSporkID); + mapSporksCachedValues.erase(spork.nSporkID); + } } ret.m_inventory.emplace_back(MSG_SPORK, hash); @@ -226,8 +229,10 @@ bool CSporkManager::UpdateSpork(PeerManager& peerman, SporkId nSporkID, SporkVal mapSporksByHash[spork.GetHash()] = spork; mapSporksActive[nSporkID][*opt_keyIDSigner] = spork; // Clear cached values on new spork being processed - WITH_LOCK(cs_mapSporksCachedActive, mapSporksCachedActive.erase(spork.nSporkID)); - WITH_LOCK(cs_mapSporksCachedValues, mapSporksCachedValues.erase(spork.nSporkID)); + + LOCK(cs_cache); + mapSporksCachedActive.erase(spork.nSporkID); + mapSporksCachedValues.erase(spork.nSporkID); } spork.Relay(peerman); @@ -238,7 +243,7 @@ bool CSporkManager::IsSporkActive(SporkId nSporkID) const { // If nSporkID is cached, and the cached value is true, then return early true { - LOCK(cs_mapSporksCachedActive); + LOCK(cs_cache); if (auto it = mapSporksCachedActive.find(nSporkID); it != mapSporksCachedActive.end() && it->second) { return true; } @@ -249,7 +254,7 @@ bool CSporkManager::IsSporkActive(SporkId nSporkID) const bool ret = nSporkValue < GetAdjustedTime(); // Only cache true values if (ret) { - LOCK(cs_mapSporksCachedActive); + LOCK(cs_cache); mapSporksCachedActive[nSporkID] = ret; } return ret; diff --git a/src/spork.h b/src/spork.h index 6f348a50a93e..3841f3a04c1d 100644 --- a/src/spork.h +++ b/src/spork.h @@ -220,11 +220,11 @@ class CSporkManager : public SporkStore const std::unique_ptr m_db; bool is_valid{false}; - mutable Mutex cs_mapSporksCachedActive; - mutable std::unordered_map mapSporksCachedActive GUARDED_BY(cs_mapSporksCachedActive); - - mutable Mutex cs_mapSporksCachedValues; - mutable std::unordered_map mapSporksCachedValues GUARDED_BY(cs_mapSporksCachedValues); + // TODO: drop mutex cs_cache completely so far as sporks are used on testnet only + // and simplify IsSporkActive to avoid any mutex for better mainnet performance + mutable Mutex cs_cache; + mutable std::unordered_map mapSporksCachedActive GUARDED_BY(cs_cache); + mutable std::unordered_map mapSporksCachedValues GUARDED_BY(cs_cache); std::set setSporkPubKeyIDs GUARDED_BY(cs); int nMinSporkKeys GUARDED_BY(cs) {std::numeric_limits::max()}; From badb221fe9ba9aea88ac7eef9a16d2adc7e7f225 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 9 Oct 2025 00:31:45 +0700 Subject: [PATCH 2/6] refactor: add missing thread safety annotations for dash specific code --- src/coinjoin/client.h | 20 +++++------ src/coinjoin/util.h | 6 +++- src/evo/creditpool.h | 9 ++--- src/evo/deterministicmns.h | 35 +++++++++++-------- src/evo/mnhftx.h | 15 ++++---- src/governance/governance.cpp | 2 +- src/instantsend/instantsend.h | 9 ++--- src/llmq/blockprocessor.h | 33 +++++++++++------- src/llmq/debug.h | 19 ++++++---- src/llmq/dkgsession.h | 15 ++++---- src/llmq/dkgsessionhandler.h | 35 +++++++++++-------- src/llmq/dkgsessionmgr.h | 11 ++++-- src/llmq/quorums.h | 53 +++++++++++++++++----------- src/llmq/signing.h | 46 +++++++++++++++---------- src/llmq/signing_shares.h | 7 ++-- src/net.h | 52 ++++++++++++++-------------- src/net_processing.cpp | 20 +++++------ src/spork.h | 14 ++++---- src/stats/client.cpp | 65 ++++++++++++++++++++++++++++------- src/versionbits.h | 2 +- 20 files changed, 285 insertions(+), 183 deletions(-) diff --git a/src/coinjoin/client.h b/src/coinjoin/client.h index 79dccfc938dd..cba6903030b3 100644 --- a/src/coinjoin/client.h +++ b/src/coinjoin/client.h @@ -95,16 +95,16 @@ class CoinJoinWalletManager { } } - void Add(const std::shared_ptr& wallet); - void DoMaintenance(CConnman& connman); + void Add(const std::shared_ptr& wallet) EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map); + void DoMaintenance(CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map); - void Remove(const std::string& name); - void Flush(const std::string& name); + void Remove(const std::string& name) EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map); + void Flush(const std::string& name) EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map); - CCoinJoinClientManager* Get(const std::string& name) const; + CCoinJoinClientManager* Get(const std::string& name) const EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map); template - void ForEachCJClientMan(Callable&& func) + void ForEachCJClientMan(Callable&& func) EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map) { LOCK(cs_wallet_manager_map); for (auto&& [_, clientman] : m_wallet_manager_map) { @@ -113,7 +113,7 @@ class CoinJoinWalletManager { }; template - bool ForAnyCJClientMan(Callable&& func) + bool ForAnyCJClientMan(Callable&& func) EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map) { LOCK(cs_wallet_manager_map); return ranges::any_of(m_wallet_manager_map, [&](auto& pair) { return func(pair.second); }); @@ -251,9 +251,9 @@ class CCoinJoinClientQueueManager : public CCoinJoinBaseManager m_mn_sync(mn_sync), m_is_masternode{is_masternode} {}; - [[nodiscard]] MessageProcessingResult ProcessMessage(NodeId from, CConnman& connman, PeerManager& peerman, std::string_view msg_type, - CDataStream& vRecv) - EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue); + [[nodiscard]] MessageProcessingResult ProcessMessage(NodeId from, CConnman& connman, PeerManager& peerman, + std::string_view msg_type, CDataStream& vRecv) + EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue, !cs_ProcessDSQueue); void DoMaintenance(); }; diff --git a/src/coinjoin/util.h b/src/coinjoin/util.h index ab0122d1bb64..8f1a26b4fc7e 100644 --- a/src/coinjoin/util.h +++ b/src/coinjoin/util.h @@ -117,7 +117,11 @@ class CTransactionBuilder /// Check if an amounts should be considered as dust bool IsDust(CAmount nAmount) const; /// Get the total number of added outputs - int CountOutputs() const { LOCK(cs_outputs); return vecOutputs.size(); } + int CountOutputs() const EXCLUSIVE_LOCKS_REQUIRED(!cs_outputs) + { + LOCK(cs_outputs); + return vecOutputs.size(); + } /// Create and Commit the transaction to the wallet bool Commit(bilingual_str& strResult) EXCLUSIVE_LOCKS_REQUIRED(!cs_outputs); /// Convert to a string diff --git a/src/evo/creditpool.h b/src/evo/creditpool.h index e337c00baea9..5b2bb8cf43d7 100644 --- a/src/evo/creditpool.h +++ b/src/evo/creditpool.h @@ -132,14 +132,15 @@ class CCreditPoolManager * In case if block is invalid the function GetCreditPool throws an exception * it can happen if there limits of withdrawal (unlock) exceed */ - CCreditPool GetCreditPool(const CBlockIndex* block, const Consensus::Params& consensusParams); + CCreditPool GetCreditPool(const CBlockIndex* block, const Consensus::Params& consensusParams) + EXCLUSIVE_LOCKS_REQUIRED(!cache_mutex); private: - std::optional GetFromCache(const CBlockIndex& block_index); - void AddToCache(const uint256& block_hash, int height, const CCreditPool& pool); + std::optional GetFromCache(const CBlockIndex& block_index) EXCLUSIVE_LOCKS_REQUIRED(!cache_mutex); + void AddToCache(const uint256& block_hash, int height, const CCreditPool& pool) EXCLUSIVE_LOCKS_REQUIRED(!cache_mutex); CCreditPool ConstructCreditPool(const gsl::not_null block_index, CCreditPool prev, - const Consensus::Params& consensusParams); + const Consensus::Params& consensusParams) EXCLUSIVE_LOCKS_REQUIRED(!cache_mutex); }; std::optional GetCreditPoolDiffForBlock(CCreditPoolManager& cpoolman, const node::BlockManager& blockman, const llmq::CQuorumManager& qman, diff --git a/src/evo/deterministicmns.h b/src/evo/deterministicmns.h index 1c5f53600a13..e195577a5e3e 100644 --- a/src/evo/deterministicmns.h +++ b/src/evo/deterministicmns.h @@ -162,7 +162,7 @@ class CDeterministicMNList mutable std::shared_ptr m_cached_sml GUARDED_BY(m_cached_sml_mutex); // Private helper method to invalidate SML cache - void InvalidateSMLCache() + void InvalidateSMLCache() EXCLUSIVE_LOCKS_REQUIRED(!m_cached_sml_mutex) { LOCK(m_cached_sml_mutex); m_cached_sml = nullptr; @@ -193,6 +193,7 @@ class CDeterministicMNList // Assignment operator CDeterministicMNList& operator=(const CDeterministicMNList& other) + EXCLUSIVE_LOCKS_REQUIRED(!m_cached_sml_mutex, !other.m_cached_sml_mutex) { if (this != &other) { blockHash = other.blockHash; @@ -229,7 +230,7 @@ class CDeterministicMNList } template - void Unserialize(Stream& s) + void Unserialize(Stream& s) EXCLUSIVE_LOCKS_REQUIRED(!m_cached_sml_mutex) { Clear(); @@ -240,7 +241,7 @@ class CDeterministicMNList } } - void Clear() + void Clear() EXCLUSIVE_LOCKS_REQUIRED(!m_cached_sml_mutex) { blockHash = uint256{}; nHeight = -1; @@ -372,7 +373,7 @@ class CDeterministicMNList * Calculates CSimplifiedMNList for current list and cache it * Thread safety: Uses internal mutex for thread-safe cache access */ - gsl::not_null> to_sml() const; + gsl::not_null> to_sml() const EXCLUSIVE_LOCKS_REQUIRED(!m_cached_sml_mutex); /** * Calculates the maximum penalty which is allowed at the height of this MN list. It is dynamic and might change @@ -393,14 +394,14 @@ class CDeterministicMNList * Penalty scores are only increased when the MN is not already banned, which means that after banning the penalty * might appear lower then the current max penalty, while the MN is still banned. */ - void PoSePunish(const uint256& proTxHash, int penalty, bool debugLogs); + void PoSePunish(const uint256& proTxHash, int penalty, bool debugLogs) EXCLUSIVE_LOCKS_REQUIRED(!m_cached_sml_mutex); - void DecreaseScores(); + void DecreaseScores() EXCLUSIVE_LOCKS_REQUIRED(!m_cached_sml_mutex); /** * Decrease penalty score of MN by 1. * Only allowed on non-banned MNs. */ - void PoSeDecrease(const CDeterministicMN& dmn); + void PoSeDecrease(const CDeterministicMN& dmn) EXCLUSIVE_LOCKS_REQUIRED(!m_cached_sml_mutex); [[nodiscard]] CDeterministicMNListDiff BuildDiff(const CDeterministicMNList& to) const; /** @@ -408,13 +409,17 @@ class CDeterministicMNList * It is more efficient than creating a copy due to heavy copy constructor. * Calculating for old block may require up to {DISK_SNAPSHOT_PERIOD} object copy & destroy. */ - void ApplyDiff(gsl::not_null pindex, const CDeterministicMNListDiff& diff); - - void AddMN(const CDeterministicMNCPtr& dmn, bool fBumpTotalCount = true); - void UpdateMN(const CDeterministicMN& oldDmn, const std::shared_ptr& pdmnState); - void UpdateMN(const uint256& proTxHash, const std::shared_ptr& pdmnState); - void UpdateMN(const CDeterministicMN& oldDmn, const CDeterministicMNStateDiff& stateDiff); - void RemoveMN(const uint256& proTxHash); + void ApplyDiff(gsl::not_null pindex, const CDeterministicMNListDiff& diff) + EXCLUSIVE_LOCKS_REQUIRED(!m_cached_sml_mutex); + + void AddMN(const CDeterministicMNCPtr& dmn, bool fBumpTotalCount = true) EXCLUSIVE_LOCKS_REQUIRED(!m_cached_sml_mutex); + void UpdateMN(const CDeterministicMN& oldDmn, const std::shared_ptr& pdmnState) + EXCLUSIVE_LOCKS_REQUIRED(!m_cached_sml_mutex); + void UpdateMN(const uint256& proTxHash, const std::shared_ptr& pdmnState) + EXCLUSIVE_LOCKS_REQUIRED(!m_cached_sml_mutex); + void UpdateMN(const CDeterministicMN& oldDmn, const CDeterministicMNStateDiff& stateDiff) + EXCLUSIVE_LOCKS_REQUIRED(!m_cached_sml_mutex); + void RemoveMN(const uint256& proTxHash) EXCLUSIVE_LOCKS_REQUIRED(!m_cached_sml_mutex); template [[nodiscard]] bool HasUniqueProperty(const T& v) const @@ -672,7 +677,7 @@ class CDeterministicMNManager // Test if given TX is a ProRegTx which also contains the collateral at index n static bool IsProTxWithCollateral(const CTransactionRef& tx, uint32_t n); - void DoMaintenance() EXCLUSIVE_LOCKS_REQUIRED(!cs); + void DoMaintenance() EXCLUSIVE_LOCKS_REQUIRED(!cs, !cs_cleanup); // Migration support for nVersion-first CDeterministicMNStateDiff format [[nodiscard]] bool IsMigrationRequired() const EXCLUSIVE_LOCKS_REQUIRED(!cs, ::cs_main); diff --git a/src/evo/mnhftx.h b/src/evo/mnhftx.h index 855607b21698..210598086f7a 100644 --- a/src/evo/mnhftx.h +++ b/src/evo/mnhftx.h @@ -110,7 +110,8 @@ class CMNHFManager : public AbstractEHFManager * @pre Caller must ensure that LLMQContext has been initialized and the llmq::CQuorumManager pointer has been * set by calling ConnectManagers() for this CMNHFManager instance */ - std::optional ProcessBlock(const CBlock& block, const CBlockIndex* const pindex, bool fJustCheck, BlockValidationState& state); + std::optional ProcessBlock(const CBlock& block, const CBlockIndex* const pindex, bool fJustCheck, + BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(!cs_cache); /** * Every undo block should be processed when Tip() is updated by calling of CMNHFManager::UndoBlock @@ -120,10 +121,10 @@ class CMNHFManager : public AbstractEHFManager * @pre Caller must ensure that LLMQContext has been initialized and the llmq::CQuorumManager pointer has been * set by calling ConnectManagers() for this CMNHFManager instance */ - bool UndoBlock(const CBlock& block, const CBlockIndex* const pindex); + bool UndoBlock(const CBlock& block, const CBlockIndex* const pindex) EXCLUSIVE_LOCKS_REQUIRED(!cs_cache); // Implements interface - Signals GetSignalsStage(const CBlockIndex* const pindexPrev) override; + Signals GetSignalsStage(const CBlockIndex* const pindexPrev) override EXCLUSIVE_LOCKS_REQUIRED(!cs_cache); /** * Helper that used in Unit Test to forcely setup EHF signal for specific block @@ -145,10 +146,10 @@ class CMNHFManager : public AbstractEHFManager */ void DisconnectManagers(); - bool ForceSignalDBUpdate() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + bool ForceSignalDBUpdate() EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !cs_cache); private: - void AddToCache(const Signals& signals, const CBlockIndex* const pindex); + void AddToCache(const Signals& signals, const CBlockIndex* const pindex) EXCLUSIVE_LOCKS_REQUIRED(!cs_cache); /** * This function returns list of signals available on previous block. @@ -156,13 +157,13 @@ class CMNHFManager : public AbstractEHFManager * until state won't be recovered. * NOTE: that some signals could expired between blocks. */ - Signals GetForBlock(const CBlockIndex* const pindex); + Signals GetForBlock(const CBlockIndex* const pindex) EXCLUSIVE_LOCKS_REQUIRED(!cs_cache); /** * This function access to in-memory cache or to evo db but does not calculate anything * NOTE: that some signals could expired between blocks. */ - std::optional GetFromCache(const CBlockIndex* const pindex); + std::optional GetFromCache(const CBlockIndex* const pindex) EXCLUSIVE_LOCKS_REQUIRED(!cs_cache); }; std::optional extractEHFSignal(const CTransaction& tx); diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index 1ff5ddda0e01..dc09f514e50d 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -718,7 +718,7 @@ std::optional CGovernanceManager::CreateGovernanceTrigg } // Nobody submitted a trigger we'd like to see, so let's do it but only if we are the payee - const CBlockIndex *tip = WITH_LOCK(::cs_main, return m_chainman.ActiveChain().Tip()); + const CBlockIndex* tip = m_chainman.ActiveChain().Tip(); const auto mnList = Assert(m_dmnman)->GetListForBlock(tip); const auto mn_payees = mnList.GetProjectedMNPayees(tip); diff --git a/src/instantsend/instantsend.h b/src/instantsend/instantsend.h index ddeb91c9d144..e9b5c809c197 100644 --- a/src/instantsend/instantsend.h +++ b/src/instantsend/instantsend.h @@ -119,7 +119,7 @@ class CInstantSendManager final : public instantsend::InstantSendSignerParent EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry); void AddNonLockedTx(const CTransactionRef& tx, const CBlockIndex* pindexMined) - EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks); + EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_timingsTxSeen); void RemoveNonLockedTx(const uint256& txid, bool retryChildren) EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingRetry); void RemoveConflictedTx(const CTransaction& tx) @@ -142,13 +142,14 @@ class CInstantSendManager final : public instantsend::InstantSendSignerParent bool IsWaitingForTx(const uint256& txHash) const EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks); instantsend::InstantSendLockPtr GetConflictingLock(const CTransaction& tx) const override; - [[nodiscard]] MessageProcessingResult ProcessMessage(NodeId from, std::string_view msg_type, CDataStream& vRecv); + [[nodiscard]] MessageProcessingResult ProcessMessage(NodeId from, std::string_view msg_type, CDataStream& vRecv) + EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks); void TransactionAddedToMempool(const CTransactionRef& tx) - EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry); + EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry, !cs_timingsTxSeen); void TransactionRemovedFromMempool(const CTransactionRef& tx); void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindex) - EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry); + EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry, !cs_timingsTxSeen); void BlockDisconnected(const std::shared_ptr& pblock, const CBlockIndex* pindexDisconnected); bool AlreadyHave(const CInv& inv) const EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks); diff --git a/src/llmq/blockprocessor.h b/src/llmq/blockprocessor.h index f4f19ae067fe..de026946d7ab 100644 --- a/src/llmq/blockprocessor.h +++ b/src/llmq/blockprocessor.h @@ -58,18 +58,26 @@ class CQuorumBlockProcessor CQuorumSnapshotManager& qsnapman); ~CQuorumBlockProcessor(); - [[nodiscard]] MessageProcessingResult ProcessMessage(const CNode& peer, std::string_view msg_type, CDataStream& vRecv); + [[nodiscard]] MessageProcessingResult ProcessMessage(const CNode& peer, std::string_view msg_type, CDataStream& vRecv) + EXCLUSIVE_LOCKS_REQUIRED(!minableCommitmentsCs); - bool ProcessBlock(const CBlock& block, gsl::not_null pindex, BlockValidationState& state, bool fJustCheck, bool fBLSChecks) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - bool UndoBlock(const CBlock& block, gsl::not_null pindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + bool ProcessBlock(const CBlock& block, gsl::not_null pindex, BlockValidationState& state, + bool fJustCheck, bool fBLSChecks) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs); + bool UndoBlock(const CBlock& block, gsl::not_null pindex) + EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs); //! it returns hash of commitment if it should be relay, otherwise nullopt - std::optional AddMineableCommitment(const CFinalCommitment& fqc); - bool HasMineableCommitment(const uint256& hash) const; - bool GetMineableCommitmentByHash(const uint256& commitmentHash, CFinalCommitment& ret) const; - std::optional> GetMineableCommitments(const Consensus::LLMQParams& llmqParams, int nHeight) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - bool GetMineableCommitmentsTx(const Consensus::LLMQParams& llmqParams, int nHeight, std::vector& ret) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - bool HasMinedCommitment(Consensus::LLMQType llmqType, const uint256& quorumHash) const; + std::optional AddMineableCommitment(const CFinalCommitment& fqc) EXCLUSIVE_LOCKS_REQUIRED(!minableCommitmentsCs); + bool HasMineableCommitment(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(!minableCommitmentsCs); + bool GetMineableCommitmentByHash(const uint256& commitmentHash, CFinalCommitment& ret) const + EXCLUSIVE_LOCKS_REQUIRED(!minableCommitmentsCs); + std::optional> GetMineableCommitments(const Consensus::LLMQParams& llmqParams, + int nHeight) const + EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs); + bool GetMineableCommitmentsTx(const Consensus::LLMQParams& llmqParams, int nHeight, std::vector& ret) const + EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs); + bool HasMinedCommitment(Consensus::LLMQType llmqType, const uint256& quorumHash) const + EXCLUSIVE_LOCKS_REQUIRED(!minableCommitmentsCs); std::pair GetMinedCommitment(Consensus::LLMQType llmqType, const uint256& quorumHash) const; std::vector GetMinedCommitmentsUntilBlock(Consensus::LLMQType llmqType, gsl::not_null pindex, size_t maxCount) const; @@ -82,9 +90,10 @@ class CQuorumBlockProcessor std::optional GetLastMinedCommitmentsByQuorumIndexUntilBlock(Consensus::LLMQType llmqType, const CBlockIndex* pindex, int quorumIndex, size_t cycle) const; private: static bool GetCommitmentsFromBlock(const CBlock& block, gsl::not_null pindex, std::multimap& ret, BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - bool ProcessCommitment(int nHeight, const uint256& blockHash, const CFinalCommitment& qc, - BlockValidationState& state, bool fJustCheck) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - size_t GetNumCommitmentsRequired(const Consensus::LLMQParams& llmqParams, int nHeight) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + bool ProcessCommitment(int nHeight, const uint256& blockHash, const CFinalCommitment& qc, BlockValidationState& state, + bool fJustCheck) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs); + size_t GetNumCommitmentsRequired(const Consensus::LLMQParams& llmqParams, int nHeight) const + EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !minableCommitmentsCs); static uint256 GetQuorumBlockHash(const Consensus::LLMQParams& llmqParams, const CChain& active_chain, int nHeight, int quorumIndex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); }; } // namespace llmq diff --git a/src/llmq/debug.h b/src/llmq/debug.h index b97d6bc25cec..61ba107adfd7 100644 --- a/src/llmq/debug.h +++ b/src/llmq/debug.h @@ -103,13 +103,18 @@ class CDKGDebugManager public: CDKGDebugManager(); - void GetLocalDebugStatus(CDKGDebugStatus& ret) const; - - void ResetLocalSessionStatus(Consensus::LLMQType llmqType, int quorumIndex); - void InitLocalSessionStatus(const Consensus::LLMQParams& llmqParams, int quorumIndex, const uint256& quorumHash, int quorumHeight); - - void UpdateLocalSessionStatus(Consensus::LLMQType llmqType, int quorumIndex, std::function&& func); - void UpdateLocalMemberStatus(Consensus::LLMQType llmqType, int quorumIndex, size_t memberIdx, std::function&& func); + void GetLocalDebugStatus(CDKGDebugStatus& ret) const EXCLUSIVE_LOCKS_REQUIRED(!cs_lockStatus); + + void ResetLocalSessionStatus(Consensus::LLMQType llmqType, int quorumIndex) EXCLUSIVE_LOCKS_REQUIRED(!cs_lockStatus); + void InitLocalSessionStatus(const Consensus::LLMQParams& llmqParams, int quorumIndex, const uint256& quorumHash, + int quorumHeight) EXCLUSIVE_LOCKS_REQUIRED(!cs_lockStatus); + + void UpdateLocalSessionStatus(Consensus::LLMQType llmqType, int quorumIndex, + std::function&& func) + EXCLUSIVE_LOCKS_REQUIRED(!cs_lockStatus); + void UpdateLocalMemberStatus(Consensus::LLMQType llmqType, int quorumIndex, size_t memberIdx, + std::function&& func) + EXCLUSIVE_LOCKS_REQUIRED(!cs_lockStatus); }; } // namespace llmq diff --git a/src/llmq/dkgsession.h b/src/llmq/dkgsession.h index 33b11d811a14..2b1b095b93fe 100644 --- a/src/llmq/dkgsession.h +++ b/src/llmq/dkgsession.h @@ -357,30 +357,31 @@ class CDKGSession void Contribute(CDKGPendingMessages& pendingMessages, PeerManager& peerman); void SendContributions(CDKGPendingMessages& pendingMessages, PeerManager& peerman); bool PreVerifyMessage(const CDKGContribution& qc, bool& retBan) const; - std::optional ReceiveMessage(const CDKGContribution& qc); + std::optional ReceiveMessage(const CDKGContribution& qc) EXCLUSIVE_LOCKS_REQUIRED(!invCs, !cs_pending); void VerifyPendingContributions() EXCLUSIVE_LOCKS_REQUIRED(cs_pending); // Phase 2: complaint - void VerifyAndComplain(CConnman& connman, CDKGPendingMessages& pendingMessages, PeerManager& peerman); + void VerifyAndComplain(CConnman& connman, CDKGPendingMessages& pendingMessages, PeerManager& peerman) + EXCLUSIVE_LOCKS_REQUIRED(!cs_pending); void VerifyConnectionAndMinProtoVersions(CConnman& connman) const; void SendComplaint(CDKGPendingMessages& pendingMessages, PeerManager& peerman); bool PreVerifyMessage(const CDKGComplaint& qc, bool& retBan) const; - std::optional ReceiveMessage(const CDKGComplaint& qc); + std::optional ReceiveMessage(const CDKGComplaint& qc) EXCLUSIVE_LOCKS_REQUIRED(!invCs); // Phase 3: justification - void VerifyAndJustify(CDKGPendingMessages& pendingMessages, PeerManager& peerman); + void VerifyAndJustify(CDKGPendingMessages& pendingMessages, PeerManager& peerman) EXCLUSIVE_LOCKS_REQUIRED(!invCs); void SendJustification(CDKGPendingMessages& pendingMessages, PeerManager& peerman, const std::set& forMembers); bool PreVerifyMessage(const CDKGJustification& qj, bool& retBan) const; - std::optional ReceiveMessage(const CDKGJustification& qj); + std::optional ReceiveMessage(const CDKGJustification& qj) EXCLUSIVE_LOCKS_REQUIRED(!invCs); // Phase 4: commit void VerifyAndCommit(CDKGPendingMessages& pendingMessages, PeerManager& peerman); void SendCommitment(CDKGPendingMessages& pendingMessages, PeerManager& peerman); bool PreVerifyMessage(const CDKGPrematureCommitment& qc, bool& retBan) const; - std::optional ReceiveMessage(const CDKGPrematureCommitment& qc); + std::optional ReceiveMessage(const CDKGPrematureCommitment& qc) EXCLUSIVE_LOCKS_REQUIRED(!invCs); // Phase 5: aggregate/finalize - std::vector FinalizeCommitments(); + std::vector FinalizeCommitments() EXCLUSIVE_LOCKS_REQUIRED(!invCs); // All Phases 5-in-1 for single-node-quorum CFinalCommitment FinalizeSingleCommitment(); diff --git a/src/llmq/dkgsessionhandler.h b/src/llmq/dkgsessionhandler.h index faab21a69b17..ad241a5efc13 100644 --- a/src/llmq/dkgsessionhandler.h +++ b/src/llmq/dkgsessionhandler.h @@ -76,14 +76,15 @@ class CDKGPendingMessages explicit CDKGPendingMessages(size_t _maxMessagesPerNode, uint32_t _invType) : invType(_invType), maxMessagesPerNode(_maxMessagesPerNode) {}; - [[nodiscard]] MessageProcessingResult PushPendingMessage(NodeId from, CDataStream& vRecv); - std::list PopPendingMessages(size_t maxCount); - bool HasSeen(const uint256& hash) const; + [[nodiscard]] MessageProcessingResult PushPendingMessage(NodeId from, CDataStream& vRecv) + EXCLUSIVE_LOCKS_REQUIRED(!cs_messages); + std::list PopPendingMessages(size_t maxCount) EXCLUSIVE_LOCKS_REQUIRED(!cs_messages); + bool HasSeen(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(!cs_messages); void Misbehaving(NodeId from, int score, PeerManager& peerman); - void Clear(); + void Clear() EXCLUSIVE_LOCKS_REQUIRED(!cs_messages); template - void PushPendingMessage(NodeId from, Message& msg, PeerManager& peerman) + void PushPendingMessage(NodeId from, Message& msg, PeerManager& peerman) EXCLUSIVE_LOCKS_REQUIRED(!cs_messages) { CDataStream ds(SER_NETWORK, PROTOCOL_VERSION); ds << msg; @@ -91,8 +92,9 @@ class CDKGPendingMessages } // Might return nullptr messages, which indicates that deserialization failed for some reason - template + template std::vector>> PopAndDeserializeMessages(size_t maxCount) + EXCLUSIVE_LOCKS_REQUIRED(!cs_messages) { auto binaryMessages = PopPendingMessages(maxCount); if (binaryMessages.empty()) { @@ -165,7 +167,7 @@ class CDKGSessionHandler const CSporkManager& sporkman, const Consensus::LLMQParams& _params, int _quorumIndex); ~CDKGSessionHandler(); - void UpdatedBlockTip(const CBlockIndex *pindexNew); + void UpdatedBlockTip(const CBlockIndex* pindexNew) EXCLUSIVE_LOCKS_REQUIRED(!cs_phase_qhash); [[nodiscard]] MessageProcessingResult ProcessMessage(NodeId from, std::string_view msg_type, CDataStream& vRecv); void StartThread(CConnman& connman, PeerManager& peerman); @@ -179,7 +181,7 @@ class CDKGSessionHandler private: bool InitNewQuorum(const CBlockIndex* pQuorumBaseBlockIndex); - std::pair GetPhaseAndQuorumHash() const; + std::pair GetPhaseAndQuorumHash() const EXCLUSIVE_LOCKS_REQUIRED(!cs_phase_qhash); using StartPhaseFunc = std::function; using WhileWaitFunc = std::function; @@ -189,12 +191,17 @@ class CDKGSessionHandler * @param expectedQuorumHash expected QuorumHash, defaults to null * @param shouldNotWait function that returns bool, defaults to function that returns false. If the function returns false, we will wait in the loop, if true, we don't wait */ - void WaitForNextPhase(std::optional curPhase, QuorumPhase nextPhase, const uint256& expectedQuorumHash=uint256(), const WhileWaitFunc& shouldNotWait=[]{return false;}) const; - void WaitForNewQuorum(const uint256& oldQuorumHash) const; - void SleepBeforePhase(QuorumPhase curPhase, const uint256& expectedQuorumHash, double randomSleepFactor, const WhileWaitFunc& runWhileWaiting) const; - void HandlePhase(QuorumPhase curPhase, QuorumPhase nextPhase, const uint256& expectedQuorumHash, double randomSleepFactor, const StartPhaseFunc& startPhaseFunc, const WhileWaitFunc& runWhileWaiting); - void HandleDKGRound(CConnman& connman, PeerManager& peerman); - void PhaseHandlerThread(CConnman& connman, PeerManager& peerman); + void WaitForNextPhase( + std::optional curPhase, QuorumPhase nextPhase, const uint256& expectedQuorumHash = uint256(), + const WhileWaitFunc& shouldNotWait = [] { return false; }) const EXCLUSIVE_LOCKS_REQUIRED(!cs_phase_qhash); + void WaitForNewQuorum(const uint256& oldQuorumHash) const EXCLUSIVE_LOCKS_REQUIRED(!cs_phase_qhash); + void SleepBeforePhase(QuorumPhase curPhase, const uint256& expectedQuorumHash, double randomSleepFactor, + const WhileWaitFunc& runWhileWaiting) const EXCLUSIVE_LOCKS_REQUIRED(!cs_phase_qhash); + void HandlePhase(QuorumPhase curPhase, QuorumPhase nextPhase, const uint256& expectedQuorumHash, + double randomSleepFactor, const StartPhaseFunc& startPhaseFunc, + const WhileWaitFunc& runWhileWaiting) EXCLUSIVE_LOCKS_REQUIRED(!cs_phase_qhash); + void HandleDKGRound(CConnman& connman, PeerManager& peerman) EXCLUSIVE_LOCKS_REQUIRED(!cs_phase_qhash); + void PhaseHandlerThread(CConnman& connman, PeerManager& peerman) EXCLUSIVE_LOCKS_REQUIRED(!cs_phase_qhash); }; } // namespace llmq diff --git a/src/llmq/dkgsessionmgr.h b/src/llmq/dkgsessionmgr.h index 51fb2fdbe8e9..dba7948ec2f7 100644 --- a/src/llmq/dkgsessionmgr.h +++ b/src/llmq/dkgsessionmgr.h @@ -87,7 +87,8 @@ class CDKGSessionManager void StartThreads(CConnman& connman, PeerManager& peerman); void StopThreads(); - void UpdatedBlockTip(const CBlockIndex *pindexNew, bool fInitialDownload); + void UpdatedBlockTip(const CBlockIndex* pindexNew, bool fInitialDownload) + EXCLUSIVE_LOCKS_REQUIRED(!contributionsCacheCs); [[nodiscard]] MessageProcessingResult ProcessMessage(CNode& pfrom, bool is_masternode, std::string_view msg_type, CDataStream& vRecv); @@ -100,7 +101,11 @@ class CDKGSessionManager // Contributions are written while in the DKG void WriteVerifiedVvecContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const BLSVerificationVectorPtr& vvec); void WriteVerifiedSkContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const CBLSSecretKey& skContribution); - bool GetVerifiedContributions(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const std::vector& validMembers, std::vector& memberIndexesRet, std::vector& vvecsRet, std::vector& skContributionsRet) const; + bool GetVerifiedContributions(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, + const std::vector& validMembers, std::vector& memberIndexesRet, + std::vector& vvecsRet, + std::vector& skContributionsRet) const + EXCLUSIVE_LOCKS_REQUIRED(!contributionsCacheCs); /// Write encrypted (unverified) DKG contributions for the member with the given proTxHash to the llmqDb void WriteEncryptedContributions(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const CBLSIESMultiRecipientObjects& contributions); /// Read encrypted (unverified) DKG contributions for the member with the given proTxHash from the llmqDb @@ -109,7 +114,7 @@ class CDKGSessionManager void CleanupOldContributions() const; private: - void CleanupCache() const; + void CleanupCache() const EXCLUSIVE_LOCKS_REQUIRED(!contributionsCacheCs); }; } // namespace llmq diff --git a/src/llmq/quorums.h b/src/llmq/quorums.h index 33acc6f3bbeb..1ba398103739 100644 --- a/src/llmq/quorums.h +++ b/src/llmq/quorums.h @@ -197,25 +197,27 @@ class CQuorum ~CQuorum() = default; void Init(CFinalCommitmentPtr _qc, const CBlockIndex* _pQuorumBaseBlockIndex, const uint256& _minedBlockHash, Span _members); - bool SetVerificationVector(const std::vector& quorumVecIn); - void SetVerificationVector(BLSVerificationVectorPtr vvec_in) { + bool SetVerificationVector(const std::vector& quorumVecIn) EXCLUSIVE_LOCKS_REQUIRED(!cs_vvec_shShare); + void SetVerificationVector(BLSVerificationVectorPtr vvec_in) EXCLUSIVE_LOCKS_REQUIRED(!cs_vvec_shShare) + { LOCK(cs_vvec_shShare); quorumVvec = std::move(vvec_in); } - bool SetSecretKeyShare(const CBLSSecretKey& secretKeyShare, const CActiveMasternodeManager& mn_activeman); + bool SetSecretKeyShare(const CBLSSecretKey& secretKeyShare, const CActiveMasternodeManager& mn_activeman) + EXCLUSIVE_LOCKS_REQUIRED(!cs_vvec_shShare); - bool HasVerificationVector() const LOCKS_EXCLUDED(cs_vvec_shShare); + bool HasVerificationVector() const EXCLUSIVE_LOCKS_REQUIRED(!cs_vvec_shShare); bool IsMember(const uint256& proTxHash) const; bool IsValidMember(const uint256& proTxHash) const; int GetMemberIndex(const uint256& proTxHash) const; - CBLSPublicKey GetPubKeyShare(size_t memberIdx) const; - CBLSSecretKey GetSkShare() const; + CBLSPublicKey GetPubKeyShare(size_t memberIdx) const EXCLUSIVE_LOCKS_REQUIRED(!cs_vvec_shShare); + CBLSSecretKey GetSkShare() const EXCLUSIVE_LOCKS_REQUIRED(!cs_vvec_shShare); private: bool HasVerificationVectorInternal() const EXCLUSIVE_LOCKS_REQUIRED(cs_vvec_shShare); - void WriteContributions(CDBWrapper& db) const; - bool ReadContributions(const CDBWrapper& db); + void WriteContributions(CDBWrapper& db) const EXCLUSIVE_LOCKS_REQUIRED(!cs_vvec_shShare); + bool ReadContributions(const CDBWrapper& db) EXCLUSIVE_LOCKS_REQUIRED(!cs_vvec_shShare); }; /** @@ -242,7 +244,7 @@ class CQuorumManager mutable Mutex cs_map_quorums; mutable std::map> mapQuorumsCache GUARDED_BY(cs_map_quorums); - mutable Mutex cs_scan_quorums; + mutable Mutex cs_scan_quorums; // TODO: merge cs_map_quorums, cs_scan_quorums mutexes mutable std::map>> scanQuorumsCache GUARDED_BY(cs_scan_quorums); mutable Mutex cs_cleanup; @@ -266,11 +268,15 @@ class CQuorumManager void Start(); void Stop(); - void TriggerQuorumDataRecoveryThreads(CConnman& connman, const CBlockIndex* pIndex) const; + void TriggerQuorumDataRecoveryThreads(CConnman& connman, const CBlockIndex* pIndex) const + EXCLUSIVE_LOCKS_REQUIRED(!cs_db, !cs_scan_quorums, !cs_map_quorums); - void UpdatedBlockTip(const CBlockIndex* pindexNew, CConnman& connman, bool fInitialDownload) const; + void UpdatedBlockTip(const CBlockIndex* pindexNew, CConnman& connman, bool fInitialDownload) const + EXCLUSIVE_LOCKS_REQUIRED(!cs_db, !cs_scan_quorums, !cs_map_quorums); - [[nodiscard]] MessageProcessingResult ProcessMessage(CNode& pfrom, CConnman& connman, std::string_view msg_type, CDataStream& vRecv); + [[nodiscard]] MessageProcessingResult ProcessMessage(CNode& pfrom, CConnman& connman, std::string_view msg_type, + CDataStream& vRecv) + EXCLUSIVE_LOCKS_REQUIRED(!cs_map_quorums, !cs_db); static bool HasQuorum(Consensus::LLMQType llmqType, const CQuorumBlockProcessor& quorum_block_processor, const uint256& quorumHash); @@ -278,21 +284,28 @@ class CQuorumManager const uint256& proTxHash = uint256()) const; // all these methods will lock cs_main for a short period of time - CQuorumCPtr GetQuorum(Consensus::LLMQType llmqType, const uint256& quorumHash) const; - std::vector ScanQuorums(Consensus::LLMQType llmqType, size_t nCountRequested) const; + CQuorumCPtr GetQuorum(Consensus::LLMQType llmqType, const uint256& quorumHash) const + EXCLUSIVE_LOCKS_REQUIRED(!cs_db, !cs_map_quorums); + std::vector ScanQuorums(Consensus::LLMQType llmqType, size_t nCountRequested) const + EXCLUSIVE_LOCKS_REQUIRED(!cs_db, !cs_map_quorums, !cs_scan_quorums); // this one is cs_main-free - std::vector ScanQuorums(Consensus::LLMQType llmqType, const CBlockIndex* pindexStart, size_t nCountRequested) const; + std::vector ScanQuorums(Consensus::LLMQType llmqType, const CBlockIndex* pindexStart, + size_t nCountRequested) const + EXCLUSIVE_LOCKS_REQUIRED(!cs_db, !cs_map_quorums, !cs_scan_quorums); private: // all private methods here are cs_main-free - void CheckQuorumConnections(CConnman& connman, const Consensus::LLMQParams& llmqParams, - const CBlockIndex* pindexNew) const; + void CheckQuorumConnections(CConnman& connman, const Consensus::LLMQParams& llmqParams, const CBlockIndex* pindexNew) const + EXCLUSIVE_LOCKS_REQUIRED(!cs_db, !cs_scan_quorums, !cs_map_quorums); - CQuorumPtr BuildQuorumFromCommitment(Consensus::LLMQType llmqType, gsl::not_null pQuorumBaseBlockIndex, bool populate_cache) const; + CQuorumPtr BuildQuorumFromCommitment(Consensus::LLMQType llmqType, + gsl::not_null pQuorumBaseBlockIndex, + bool populate_cache) const EXCLUSIVE_LOCKS_REQUIRED(!cs_db, !cs_map_quorums); bool BuildQuorumContributions(const CFinalCommitmentPtr& fqc, const std::shared_ptr& quorum) const; - CQuorumCPtr GetQuorum(Consensus::LLMQType llmqType, gsl::not_null pindex, bool populate_cache = true) const; + CQuorumCPtr GetQuorum(Consensus::LLMQType llmqType, gsl::not_null pindex, + bool populate_cache = true) const EXCLUSIVE_LOCKS_REQUIRED(!cs_db, !cs_map_quorums); /// Returns the start offset for the masternode with the given proTxHash. This offset is applied when picking data recovery members of a quorum's /// memberlist and is calculated based on a list of all member of all active quorums for the given llmqType in a way that each member /// should receive the same number of request if all active llmqType members requests data from one llmqType quorum. @@ -303,7 +316,7 @@ class CQuorumManager uint16_t nDataMask) const; void StartCleanupOldQuorumDataThread(const CBlockIndex* pIndex) const; - void MigrateOldQuorumDB(CEvoDB& evoDb) const; + void MigrateOldQuorumDB(CEvoDB& evoDb) const EXCLUSIVE_LOCKS_REQUIRED(!cs_db); }; // when selecting a quorum for signing and verification, we use CQuorumManager::SelectQuorum with this offset as diff --git a/src/llmq/signing.h b/src/llmq/signing.h index 2425de093604..e49e76a14329 100644 --- a/src/llmq/signing.h +++ b/src/llmq/signing.h @@ -127,15 +127,15 @@ class CRecoveredSigsDb ~CRecoveredSigsDb(); bool HasRecoveredSig(Consensus::LLMQType llmqType, const uint256& id, const uint256& msgHash) const; - bool HasRecoveredSigForId(Consensus::LLMQType llmqType, const uint256& id) const; - bool HasRecoveredSigForSession(const uint256& signHash) const; - bool HasRecoveredSigForHash(const uint256& hash) const; + bool HasRecoveredSigForId(Consensus::LLMQType llmqType, const uint256& id) const EXCLUSIVE_LOCKS_REQUIRED(!cs_cache); + bool HasRecoveredSigForSession(const uint256& signHash) const EXCLUSIVE_LOCKS_REQUIRED(!cs_cache); + bool HasRecoveredSigForHash(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(!cs_cache); bool GetRecoveredSigByHash(const uint256& hash, CRecoveredSig& ret) const; bool GetRecoveredSigById(Consensus::LLMQType llmqType, const uint256& id, CRecoveredSig& ret) const; - void WriteRecoveredSig(const CRecoveredSig& recSig); - void TruncateRecoveredSig(Consensus::LLMQType llmqType, const uint256& id); + void WriteRecoveredSig(const CRecoveredSig& recSig) EXCLUSIVE_LOCKS_REQUIRED(!cs_cache); + void TruncateRecoveredSig(Consensus::LLMQType llmqType, const uint256& id) EXCLUSIVE_LOCKS_REQUIRED(!cs_cache); - void CleanupOldRecoveredSigs(int64_t maxAge); + void CleanupOldRecoveredSigs(int64_t maxAge) EXCLUSIVE_LOCKS_REQUIRED(!cs_cache); // votes are removed when the recovered sig is written to the db bool HasVotedOnId(Consensus::LLMQType llmqType, const uint256& id) const; @@ -146,7 +146,8 @@ class CRecoveredSigsDb private: bool ReadRecoveredSig(Consensus::LLMQType llmqType, const uint256& id, CRecoveredSig& ret) const; - void RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id, bool deleteHashKey, bool deleteTimeKey); + void RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id, bool deleteHashKey, + bool deleteTimeKey) EXCLUSIVE_LOCKS_REQUIRED(!cs_cache); }; class CRecoveredSigsListener @@ -182,14 +183,16 @@ class CSigningManager CSigningManager(const CActiveMasternodeManager* const mn_activeman, const CChainState& chainstate, const CQuorumManager& _qman, bool fMemory, bool fWipe); - bool AlreadyHave(const CInv& inv) const; + bool AlreadyHave(const CInv& inv) const EXCLUSIVE_LOCKS_REQUIRED(!cs_pending); bool GetRecoveredSigForGetData(const uint256& hash, CRecoveredSig& ret) const; - [[nodiscard]] MessageProcessingResult ProcessMessage(NodeId from, std::string_view msg_type, CDataStream& vRecv); + [[nodiscard]] MessageProcessingResult ProcessMessage(NodeId from, std::string_view msg_type, CDataStream& vRecv) + EXCLUSIVE_LOCKS_REQUIRED(!cs_pending); // This is called when a recovered signature was was reconstructed from another P2P message and is known to be valid // This is the case for example when a signature appears as part of InstantSend or ChainLocks - void PushReconstructedRecoveredSig(const std::shared_ptr& recoveredSig); + void PushReconstructedRecoveredSig(const std::shared_ptr& recoveredSig) + EXCLUSIVE_LOCKS_REQUIRED(!cs_pending); // This is called when a recovered signature can be safely removed from the DB. This is only safe when some other // mechanism prevents possible conflicts. As an example, ChainLocks prevent conflicts in confirmed TXs InstantSend votes @@ -198,22 +201,26 @@ class CSigningManager void TruncateRecoveredSig(Consensus::LLMQType llmqType, const uint256& id); private: - void CollectPendingRecoveredSigsToVerify(size_t maxUniqueSessions, - std::unordered_map>>& retSigShares, - std::unordered_map, CQuorumCPtr, StaticSaltedHasher>& retQuorums); - void ProcessPendingReconstructedRecoveredSigs(PeerManager& peerman); - bool ProcessPendingRecoveredSigs(PeerManager& peerman); // called from the worker thread of CSigSharesManager + void CollectPendingRecoveredSigsToVerify( + size_t maxUniqueSessions, std::unordered_map>>& retSigShares, + std::unordered_map, CQuorumCPtr, StaticSaltedHasher>& retQuorums) + EXCLUSIVE_LOCKS_REQUIRED(!cs_pending); + void ProcessPendingReconstructedRecoveredSigs(PeerManager& peerman) + EXCLUSIVE_LOCKS_REQUIRED(!cs_pending, !cs_listeners); + bool ProcessPendingRecoveredSigs(PeerManager& peerman) + EXCLUSIVE_LOCKS_REQUIRED(!cs_pending, !cs_listeners); // called from the worker thread of CSigSharesManager public: // TODO - should not be public! - void ProcessRecoveredSig(const std::shared_ptr& recoveredSig, PeerManager& peerman); + void ProcessRecoveredSig(const std::shared_ptr& recoveredSig, PeerManager& peerman) + EXCLUSIVE_LOCKS_REQUIRED(!cs_pending, !cs_listeners); private: void Cleanup(); // called from the worker thread of CSigSharesManager public: // public interface - void RegisterRecoveredSigsListener(CRecoveredSigsListener* l); - void UnregisterRecoveredSigsListener(CRecoveredSigsListener* l); + void RegisterRecoveredSigsListener(CRecoveredSigsListener* l) EXCLUSIVE_LOCKS_REQUIRED(!cs_listeners); + void UnregisterRecoveredSigsListener(CRecoveredSigsListener* l) EXCLUSIVE_LOCKS_REQUIRED(!cs_listeners); bool AsyncSignIfMember(Consensus::LLMQType llmqType, CSigSharesManager& shareman, const uint256& id, const uint256& msgHash, const uint256& quorumHash = uint256(), bool allowReSign = false, @@ -229,7 +236,8 @@ class CSigningManager private: std::thread workThread; CThreadInterrupt workInterrupt; - void WorkThreadMain(PeerManager& peerman); + void WorkThreadMain(PeerManager& peerman) EXCLUSIVE_LOCKS_REQUIRED(!cs_pending, !cs_listeners); + ; public: void StartWorkerThread(PeerManager& peerman); diff --git a/src/llmq/signing_shares.h b/src/llmq/signing_shares.h index b47a41c16a1d..258a6f2417a6 100644 --- a/src/llmq/signing_shares.h +++ b/src/llmq/signing_shares.h @@ -433,7 +433,8 @@ class CSigSharesManager : public CRecoveredSigsListener void ProcessMessage(const CNode& pnode, PeerManager& peerman, const CSporkManager& sporkman, const std::string& msg_type, CDataStream& vRecv); - void AsyncSign(const CQuorumCPtr& quorum, const uint256& id, const uint256& msgHash); + void AsyncSign(const CQuorumCPtr& quorum, const uint256& id, const uint256& msgHash) + EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingSigns); std::optional CreateSigShare(const CQuorumCPtr& quorum, const uint256& id, const uint256& msgHash) const; void ForceReAnnouncement(const CQuorumCPtr& quorum, Consensus::LLMQType llmqType, const uint256& id, const uint256& msgHash); @@ -485,8 +486,8 @@ class CSigSharesManager : public CRecoveredSigsListener void CollectSigSharesToAnnounce(const CConnman& connman, std::unordered_map>& sigSharesToAnnounce) EXCLUSIVE_LOCKS_REQUIRED(cs); - void SignPendingSigShares(const CConnman& connman, PeerManager& peerman); - void WorkThreadMain(CConnman& connman, PeerManager& peerman); + void SignPendingSigShares(const CConnman& connman, PeerManager& peerman) EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingSigns); + void WorkThreadMain(CConnman& connman, PeerManager& peerman) EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingSigns); }; } // namespace llmq diff --git a/src/net.h b/src/net.h index 1f86bf5a0199..33740a656b10 100644 --- a/src/net.h +++ b/src/net.h @@ -1027,7 +1027,7 @@ class CNode void CloseSocketDisconnect(CConnman* connman) EXCLUSIVE_LOCKS_REQUIRED(!m_sock_mutex); - void CopyStats(CNodeStats& stats) EXCLUSIVE_LOCKS_REQUIRED(!m_subver_mutex, !m_addr_local_mutex, !cs_vSend, !cs_vRecv); + void CopyStats(CNodeStats& stats) EXCLUSIVE_LOCKS_REQUIRED(!m_subver_mutex, !m_addr_local_mutex, !cs_vSend, !cs_vRecv, !cs_mnauth); std::string ConnectionTypeAsString() const { return ::ConnectionTypeAsString(m_conn_type); } @@ -1041,42 +1041,42 @@ class CNode bool CanRelay() const { return !m_masternode_connection || m_masternode_iqr_connection; } - uint256 GetSentMNAuthChallenge() const { + uint256 GetSentMNAuthChallenge() const EXCLUSIVE_LOCKS_REQUIRED(!cs_mnauth) { LOCK(cs_mnauth); return sentMNAuthChallenge; } - uint256 GetReceivedMNAuthChallenge() const { + uint256 GetReceivedMNAuthChallenge() const EXCLUSIVE_LOCKS_REQUIRED(!cs_mnauth) { LOCK(cs_mnauth); return receivedMNAuthChallenge; } - uint256 GetVerifiedProRegTxHash() const { + uint256 GetVerifiedProRegTxHash() const EXCLUSIVE_LOCKS_REQUIRED(!cs_mnauth) { LOCK(cs_mnauth); return verifiedProRegTxHash; } - uint256 GetVerifiedPubKeyHash() const { + uint256 GetVerifiedPubKeyHash() const EXCLUSIVE_LOCKS_REQUIRED(!cs_mnauth) { LOCK(cs_mnauth); return verifiedPubKeyHash; } - void SetSentMNAuthChallenge(const uint256& newSentMNAuthChallenge) { + void SetSentMNAuthChallenge(const uint256& newSentMNAuthChallenge) EXCLUSIVE_LOCKS_REQUIRED(!cs_mnauth) { LOCK(cs_mnauth); sentMNAuthChallenge = newSentMNAuthChallenge; } - void SetReceivedMNAuthChallenge(const uint256& newReceivedMNAuthChallenge) { + void SetReceivedMNAuthChallenge(const uint256& newReceivedMNAuthChallenge) EXCLUSIVE_LOCKS_REQUIRED(!cs_mnauth) { LOCK(cs_mnauth); receivedMNAuthChallenge = newReceivedMNAuthChallenge; } - void SetVerifiedProRegTxHash(const uint256& newVerifiedProRegTxHash) { + void SetVerifiedProRegTxHash(const uint256& newVerifiedProRegTxHash) EXCLUSIVE_LOCKS_REQUIRED(!cs_mnauth) { LOCK(cs_mnauth); verifiedProRegTxHash = newVerifiedProRegTxHash; } - void SetVerifiedPubKeyHash(const uint256& newVerifiedPubKeyHash) { + void SetVerifiedPubKeyHash(const uint256& newVerifiedPubKeyHash) EXCLUSIVE_LOCKS_REQUIRED(!cs_mnauth) { LOCK(cs_mnauth); verifiedPubKeyHash = newVerifiedPubKeyHash; } @@ -1244,8 +1244,8 @@ friend class CNode; EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !m_added_nodes_mutex, !m_addr_fetches_mutex, !mutexMsgProc); void StopThreads(); - void StopNodes(); - void Stop() + void StopNodes() EXCLUSIVE_LOCKS_REQUIRED(!cs_mapSocketToNode, !cs_sendable_receivable_nodes); + void Stop() EXCLUSIVE_LOCKS_REQUIRED(!cs_mapSocketToNode, !cs_sendable_receivable_nodes) { StopThreads(); StopNodes(); @@ -1274,9 +1274,9 @@ friend class CNode; const char* strDest, ConnectionType conn_type, bool use_v2transport, MasternodeConn masternode_connection = MasternodeConn::IsNotConnection, MasternodeProbeConn masternode_probe_connection = MasternodeProbeConn::IsNotConnection) - EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex, !mutexMsgProc); + EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex, !mutexMsgProc, !cs_mapSocketToNode); void OpenMasternodeConnection(const CAddress& addrConnect, bool use_v2transport, MasternodeProbeConn probe = MasternodeProbeConn::IsConnection) - EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex, !mutexMsgProc); + EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex, !mutexMsgProc, !cs_mapSocketToNode); bool CheckIncomingNonce(uint64_t nonce); // alias for thread safety annotations only, not defined @@ -1477,7 +1477,7 @@ friend class CNode; * - Max connection capacity for type is filled */ bool AddConnection(const std::string& address, ConnectionType conn_type, bool use_v2transport) - EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex, !mutexMsgProc); + EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex, !mutexMsgProc, !cs_mapSocketToNode); bool AddPendingMasternode(const uint256& proTxHash); void SetMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash, const Uint256HashSet& proTxHashes); @@ -1579,16 +1579,16 @@ friend class CNode; bool InitBinds(const Options& options); void ThreadOpenAddedConnections() - EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex, !m_unused_i2p_sessions_mutex, !m_reconnections_mutex, !mutexMsgProc); + EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex, !m_unused_i2p_sessions_mutex, !m_reconnections_mutex, !mutexMsgProc, !cs_mapSocketToNode); void AddAddrFetch(const std::string& strDest) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex); void ProcessAddrFetch() - EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_unused_i2p_sessions_mutex, !mutexMsgProc); + EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_unused_i2p_sessions_mutex, !mutexMsgProc, !cs_mapSocketToNode); void ThreadOpenConnections(const std::vector connect, CDeterministicMNManager& dmnman) - EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex, !m_nodes_mutex, !m_unused_i2p_sessions_mutex, !m_reconnections_mutex, !mutexMsgProc); + EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex, !m_nodes_mutex, !m_unused_i2p_sessions_mutex, !m_reconnections_mutex, !mutexMsgProc, !cs_mapSocketToNode); void ThreadMessageHandler() EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc); - void ThreadI2PAcceptIncoming(CMasternodeSync& mn_sync) EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc); + void ThreadI2PAcceptIncoming(CMasternodeSync& mn_sync) EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc, !cs_mapSocketToNode); void AcceptConnection(const ListenSocket& hListenSocket, CMasternodeSync& mn_sync) - EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc); + EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc, !cs_mapSocketToNode); /** * Create a `CNode` object from a socket that has just been accepted and add the node to @@ -1602,7 +1602,7 @@ friend class CNode; NetPermissionFlags permission_flags, const CAddress& addr_bind, const CAddress& addr, - CMasternodeSync& mn_sync) EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc); + CMasternodeSync& mn_sync) EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc, !cs_mapSocketToNode); void DisconnectNodes() EXCLUSIVE_LOCKS_REQUIRED(!m_reconnections_mutex, !m_nodes_mutex); void NotifyNumConnectionsChanged(CMasternodeSync& mn_sync); @@ -1620,28 +1620,28 @@ friend class CNode; /** * Check connected and listening sockets for IO readiness and process them accordingly. */ - void SocketHandler(CMasternodeSync& mn_sync) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc); + void SocketHandler(CMasternodeSync& mn_sync) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc, !cs_mapSocketToNode, !cs_sendable_receivable_nodes); /** * Do the read/write for connected sockets that are ready for IO. * @param[in] events_per_sock Sockets that are ready for IO. */ void SocketHandlerConnected(const Sock::EventsPerSock& events_per_sock) - EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc); + EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc, !cs_sendable_receivable_nodes, !cs_mapSocketToNode, !cs_sendable_receivable_nodes); /** * Accept incoming connections, one from each read-ready listening socket. * @param[in] events_per_sock Sockets that are ready for IO. */ void SocketHandlerListening(const Sock::EventsPerSock& events_per_sock, CMasternodeSync& mn_sync) - EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc); + EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc, !cs_mapSocketToNode); void ThreadSocketHandler(CMasternodeSync& mn_sync) - EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc, !m_nodes_mutex, !m_reconnections_mutex); + EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc, !m_nodes_mutex, !m_reconnections_mutex, !cs_mapSocketToNode, !cs_sendable_receivable_nodes); void ThreadDNSAddressSeed() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_nodes_mutex); void ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_metaman, CMasternodeSync& mn_sync) - EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_nodes_mutex, !m_unused_i2p_sessions_mutex, !mutexMsgProc); + EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_nodes_mutex, !m_unused_i2p_sessions_mutex, !mutexMsgProc, !cs_mapSocketToNode); uint64_t CalculateKeyedNetGroup(const CAddress& ad) const; @@ -1934,7 +1934,7 @@ friend class CNode; std::list m_reconnections GUARDED_BY(m_reconnections_mutex); /** Attempt reconnections, if m_reconnections non-empty. */ - void PerformReconnections() EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc, !m_reconnections_mutex, !m_unused_i2p_sessions_mutex); + void PerformReconnections() EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc, !m_reconnections_mutex, !m_unused_i2p_sessions_mutex, !cs_mapSocketToNode); /** * Cap on the size of `m_unused_i2p_sessions`, to ensure it does not diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6e104ac8ab5b..e449f31d7b27 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -324,7 +324,7 @@ struct Peer { /** * (Bitcoin) Initializes a TxRelay struct for this peer. Can be called at most once for a peer. * (Dash) Enables the flag that allows GetTxRelay() to return m_tx_relay */ - TxRelay* SetTxRelay() LOCKS_EXCLUDED(m_tx_relay_mutex) + TxRelay* SetTxRelay() EXCLUSIVE_LOCKS_REQUIRED(!m_tx_relay_mutex) { LOCK(m_tx_relay_mutex); Assume(!m_can_tx_relay); @@ -337,12 +337,12 @@ struct Peer { return WITH_LOCK(m_tx_relay_mutex, return m_tx_relay.get()); } - TxRelay* GetTxRelay() LOCKS_EXCLUDED(m_tx_relay_mutex) + TxRelay* GetTxRelay() EXCLUSIVE_LOCKS_REQUIRED(!m_tx_relay_mutex) { LOCK(m_tx_relay_mutex); return m_can_tx_relay ? m_tx_relay.get() : nullptr; }; - const TxRelay* GetTxRelay() const LOCKS_EXCLUDED(m_tx_relay_mutex) + const TxRelay* GetTxRelay() const EXCLUSIVE_LOCKS_REQUIRED(!m_tx_relay_mutex) { LOCK(m_tx_relay_mutex); return m_can_tx_relay ? m_tx_relay.get() : nullptr; @@ -611,15 +611,15 @@ class PeerManagerImpl final : public PeerManager EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void BlockChecked(const CBlock& block, const BlockValidationState& state) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); - void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) override; + void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) override EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex); /** Implement NetEventsInterface */ void InitializeNode(CNode& node, ServiceFlags our_services) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void FinalizeNode(const CNode& node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); bool ProcessMessages(CNode* pfrom, std::atomic& interrupt) override - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, g_msgproc_mutex); + EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, g_msgproc_mutex, !m_most_recent_block_mutex); bool SendMessages(CNode* pto) override - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, g_msgproc_mutex); + EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, g_msgproc_mutex, !m_most_recent_block_mutex); /** Implement PeerManager */ void StartScheduledTasks(CScheduler& scheduler) override; @@ -639,12 +639,12 @@ class PeerManagerImpl final : public PeerManager void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message = "") override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv, const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc) override - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, g_msgproc_mutex); + EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, g_msgproc_mutex, !m_most_recent_block_mutex); void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) override; bool IsBanned(NodeId pnode) override EXCLUSIVE_LOCKS_REQUIRED(cs_main, !m_peer_mutex); size_t GetRequestedObjectCount(NodeId nodeid) const override EXCLUSIVE_LOCKS_REQUIRED(::cs_main); private: - void _RelayTransaction(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + void _RelayTransaction(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(cs_main, !m_peer_mutex); /** Ask peers that have a transaction in their inventory to relay it to us. */ void AskPeersForTransaction(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); @@ -852,7 +852,7 @@ class PeerManagerImpl final : public PeerManager */ bool BlockRequestAllowed(const CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool AlreadyHaveBlock(const uint256& block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - void ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& inv, llmq::CInstantSendManager& isman); + void ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& inv, llmq::CInstantSendManager& isman) EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex); /** * Validation logic for compact filters request handling. @@ -1034,7 +1034,7 @@ class PeerManagerImpl final : public PeerManager /** Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). */ CTransactionRef FindTxForGetData(const CNode* peer, const uint256& txid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) LOCKS_EXCLUDED(cs_main); - void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(peer.m_getdata_requests_mutex) LOCKS_EXCLUDED(::cs_main); + void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(peer.m_getdata_requests_mutex, !m_most_recent_block_mutex) LOCKS_EXCLUDED(::cs_main); /** Process a new block. Perform any post-processing housekeeping */ void ProcessBlock(CNode& from, const std::shared_ptr& pblock, bool force_processing); diff --git a/src/spork.h b/src/spork.h index 3841f3a04c1d..5dde49cb4dbe 100644 --- a/src/spork.h +++ b/src/spork.h @@ -234,7 +234,7 @@ class CSporkManager : public SporkStore * SporkValueIfActive is used to get the value agreed upon by the majority * of signed spork messages for a given Spork ID. */ - std::optional SporkValueIfActive(SporkId nSporkID) const EXCLUSIVE_LOCKS_REQUIRED(cs); + std::optional SporkValueIfActive(SporkId nSporkID) const EXCLUSIVE_LOCKS_REQUIRED(cs, !cs_cache); public: CSporkManager(); @@ -257,7 +257,8 @@ class CSporkManager : public SporkStore /** * ProcessMessage is used to call ProcessSpork and ProcessGetSporks. See below */ - [[nodiscard]] MessageProcessingResult ProcessMessage(CNode& peer, CConnman& connman, std::string_view msg_type, CDataStream& vRecv); + [[nodiscard]] MessageProcessingResult ProcessMessage(CNode& peer, CConnman& connman, std::string_view msg_type, + CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs_cache); /** * ProcessSpork is used to handle the 'spork' p2p message. @@ -265,7 +266,8 @@ class CSporkManager : public SporkStore * For 'spork', it validates the spork and adds it to the internal spork storage and * performs any necessary processing. */ - [[nodiscard]] MessageProcessingResult ProcessSpork(NodeId from, CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs); + [[nodiscard]] MessageProcessingResult ProcessSpork(NodeId from, CDataStream& vRecv) + EXCLUSIVE_LOCKS_REQUIRED(!cs, !cs_cache); /** * ProcessGetSporks is used to handle the 'getsporks' p2p message. @@ -278,7 +280,7 @@ class CSporkManager : public SporkStore * UpdateSpork is used by the spork RPC command to set a new spork value, sign * and broadcast the spork message. */ - bool UpdateSpork(PeerManager& peerman, SporkId nSporkID, SporkValue nValue) EXCLUSIVE_LOCKS_REQUIRED(!cs); + bool UpdateSpork(PeerManager& peerman, SporkId nSporkID, SporkValue nValue) EXCLUSIVE_LOCKS_REQUIRED(!cs, !cs_cache); /** * IsSporkActive returns a bool for time-based sporks, and should be used @@ -288,13 +290,13 @@ class CSporkManager : public SporkStore * instead, and therefore this method doesn't make sense and should not be * used. */ - bool IsSporkActive(SporkId nSporkID) const; + bool IsSporkActive(SporkId nSporkID) const EXCLUSIVE_LOCKS_REQUIRED(!cs_cache); /** * GetSporkValue returns the spork value given a Spork ID. If no active spork * message has yet been received by the node, it returns the default value. */ - SporkValue GetSporkValue(SporkId nSporkID) const EXCLUSIVE_LOCKS_REQUIRED(!cs); + SporkValue GetSporkValue(SporkId nSporkID) const EXCLUSIVE_LOCKS_REQUIRED(!cs, !cs_cache); /** * GetSporkIDByName returns the internal Spork ID given the spork name. diff --git a/src/stats/client.cpp b/src/stats/client.cpp index d0e3a938e1e1..973fcb266941 100644 --- a/src/stats/client.cpp +++ b/src/stats/client.cpp @@ -48,24 +48,63 @@ class StatsdClientImpl final : public StatsdClient ~StatsdClientImpl() = default; public: - bool dec(std::string_view key, float sample_rate) override { return count(key, -1, sample_rate); } - bool inc(std::string_view key, float sample_rate) override { return count(key, 1, sample_rate); } - bool count(std::string_view key, int64_t delta, float sample_rate) override { return _send(key, delta, STATSD_METRIC_COUNT, sample_rate); } - bool gauge(std::string_view key, int64_t value, float sample_rate) override { return _send(key, value, STATSD_METRIC_GAUGE, sample_rate); } - bool gaugeDouble(std::string_view key, double value, float sample_rate) override { return _send(key, value, STATSD_METRIC_GAUGE, sample_rate); } - bool timing(std::string_view key, uint64_t ms, float sample_rate) override { return _send(key, ms, STATSD_METRIC_TIMING, sample_rate); } - - bool send(std::string_view key, double value, std::string_view type, float sample_rate) override { return _send(key, value, type, sample_rate); } - bool send(std::string_view key, int32_t value, std::string_view type, float sample_rate) override { return _send(key, value, type, sample_rate); } - bool send(std::string_view key, int64_t value, std::string_view type, float sample_rate) override { return _send(key, value, type, sample_rate); } - bool send(std::string_view key, uint32_t value, std::string_view type, float sample_rate) override { return _send(key, value, type, sample_rate); } - bool send(std::string_view key, uint64_t value, std::string_view type, float sample_rate) override { return _send(key, value, type, sample_rate); } + bool dec(std::string_view key, float sample_rate) override EXCLUSIVE_LOCKS_REQUIRED(!cs) + { + return count(key, -1, sample_rate); + } + bool inc(std::string_view key, float sample_rate) override EXCLUSIVE_LOCKS_REQUIRED(!cs) + { + return count(key, 1, sample_rate); + } + bool count(std::string_view key, int64_t delta, float sample_rate) override EXCLUSIVE_LOCKS_REQUIRED(!cs) + { + return _send(key, delta, STATSD_METRIC_COUNT, sample_rate); + } + bool gauge(std::string_view key, int64_t value, float sample_rate) override EXCLUSIVE_LOCKS_REQUIRED(!cs) + { + return _send(key, value, STATSD_METRIC_GAUGE, sample_rate); + } + bool gaugeDouble(std::string_view key, double value, float sample_rate) override EXCLUSIVE_LOCKS_REQUIRED(!cs) + { + return _send(key, value, STATSD_METRIC_GAUGE, sample_rate); + } + bool timing(std::string_view key, uint64_t ms, float sample_rate) override EXCLUSIVE_LOCKS_REQUIRED(!cs) + { + return _send(key, ms, STATSD_METRIC_TIMING, sample_rate); + } + + bool send(std::string_view key, double value, std::string_view type, float sample_rate) override + EXCLUSIVE_LOCKS_REQUIRED(!cs) + { + return _send(key, value, type, sample_rate); + } + bool send(std::string_view key, int32_t value, std::string_view type, float sample_rate) override + EXCLUSIVE_LOCKS_REQUIRED(!cs) + { + return _send(key, value, type, sample_rate); + } + bool send(std::string_view key, int64_t value, std::string_view type, float sample_rate) override + EXCLUSIVE_LOCKS_REQUIRED(!cs) + { + return _send(key, value, type, sample_rate); + } + bool send(std::string_view key, uint32_t value, std::string_view type, float sample_rate) override + EXCLUSIVE_LOCKS_REQUIRED(!cs) + { + return _send(key, value, type, sample_rate); + } + bool send(std::string_view key, uint64_t value, std::string_view type, float sample_rate) override + EXCLUSIVE_LOCKS_REQUIRED(!cs) + { + return _send(key, value, type, sample_rate); + } bool active() const override { return m_sender != nullptr; } private: template - inline bool _send(std::string_view key, T1 value, std::string_view type, float sample_rate); + inline bool _send(std::string_view key, T1 value, std::string_view type, float sample_rate) + EXCLUSIVE_LOCKS_REQUIRED(!cs); private: /* Mutex to protect PRNG */ diff --git a/src/versionbits.h b/src/versionbits.h index 19232ba61977..036fba26b423 100644 --- a/src/versionbits.h +++ b/src/versionbits.h @@ -85,7 +85,7 @@ class VersionBitsCache public: /** Get the numerical statistics for a given deployment for the signalling period that includes the block after pindexPrev. */ - BIP9Stats Statistics(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos); + BIP9Stats Statistics(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); static uint32_t Mask(const Consensus::Params& params, Consensus::DeploymentPos pos); From b70c995171c0fa5b66ee3a731d35722bce606a6a Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 22 Feb 2021 09:47:11 +0100 Subject: [PATCH 3/6] Merge #21202: [validation] Two small clang lock annotation improvements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 25c57d640992255ed67964a44b17afbfd4bed0cf [doc] Add a note about where lock annotations should go. (Amiti Uttarwar) ad5f01b96045f304b6cf9100879592b835c49c40 [validation] Move the lock annotation from function definition to declaration (Amiti Uttarwar) Pull request description: Based on reviewing #21188 the first commit switches the lock annotations on `CheckInputScripts` to be on the function declaration instead of on the function definition. this ensures that all call sites are checked, not just ones that come after the definition. the second commit adds a note to the developer-notes section to clarify where the annotations should be applied. ACKs for top commit: MarcoFalke: ACK 25c57d640992255ed67964a44b17afbfd4bed0cf 🥘 promag: Code review ACK 25c57d640992255ed67964a44b17afbfd4bed0cf. Tree-SHA512: 61b6ef856bf6c6016d535fbdd19daf57b9e59fe54a1f30d47282a071b9b9d60b2466b044ee57929e0320cb1bdef52e7a1687cacaa27031bbc43d058ffffe22ba --- doc/developer-notes.md | 5 +++++ src/test/txvalidationcache_tests.cpp | 5 ++++- src/validation.cpp | 11 +++++++++-- 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 67f377b200e1..4c0d3deea7b1 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -952,6 +952,11 @@ Threads and synchronization called unconditionally after it because `LOCK()` does the same check as `AssertLockNotHeld()` internally, for non-recursive mutexes): + - In functions that are declared separately from where they are defined, the + thread safety annotations should be added exclusively to the function + declaration. Annotations on the definition could lead to false positives + (lack of compile failure) at call sites between the two. + ```C++ // txmempool.h class CTxMemPool diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp index 708c4686e7e6..62eea17e34a0 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -17,7 +17,10 @@ struct Dersig100Setup : public TestChain100Setup { : TestChain100Setup{CBaseChainParams::REGTEST, {"-testactivationheight=dersig@102"}} {} }; -bool CheckInputScripts(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks); +bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, + const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, + bool cacheFullScriptStore, PrecomputedTransactionData& txdata, + std::vector* pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main); BOOST_AUTO_TEST_SUITE(txvalidationcache_tests) diff --git a/src/validation.cpp b/src/validation.cpp index 63f7109b83f9..967fe2d716b3 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -161,7 +161,11 @@ const CBlockIndex* CChainState::FindForkInGlobalIndex(const CBlockLocator& locat return m_chain.Genesis(); } -bool CheckInputScripts(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks = nullptr); +bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, + const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, + bool cacheFullScriptStore, PrecomputedTransactionData& txdata, + std::vector* pvChecks = nullptr) + EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool CheckFinalTxAtTip(const CBlockIndex& active_chain_tip, const CTransaction& tx) { @@ -1793,7 +1797,10 @@ void InitScriptExecutionCache() { * * Non-static (and re-declared) in src/test/txvalidationcache_tests.cpp */ -bool CheckInputScripts(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main) +bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, + const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, + bool cacheFullScriptStore, PrecomputedTransactionData& txdata, + std::vector* pvChecks) { auto start = Now(); if (tx.IsCoinBase()) return true; From 4e37f90c642decdb978306df6f4d29cd49aacbee Mon Sep 17 00:00:00 2001 From: MacroFake Date: Fri, 10 Jun 2022 16:42:26 +0200 Subject: [PATCH 4/6] Merge bitcoin/bitcoin#24931: Strengthen thread safety assertions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ce893c0497fc9b8ab9752153dfcc77c9f427545e doc: Update developer notes (Anthony Towns) d2852917eecad6ab422a7b2c9892d351a7f0cc96 sync.h: Imply negative assertions when calling LOCK (Anthony Towns) bba87c0553780eacf0317fbfec7330ea27aa02f8 scripted-diff: Convert global Mutexes to GlobalMutexes (Anthony Towns) a559509a0b8cade27199740212d7b589f71a0e3b sync.h: Add GlobalMutex type (Anthony Towns) be6aa72f9f8d50b6b5b19b319a74abe7ab4099ff qt/clientmodel: thread safety annotation for m_cached_tip_mutex (Anthony Towns) f24bd45b37e1b2d19e5a053dbfefa30306c1d41a net_processing: thread safety annotation for m_tx_relay_mutex (Anthony Towns) Pull request description: This changes `LOCK(mutex)` for non-global, non-recursive mutexes to be annotated with the negative capability for the mutex it refers to, to prevent . clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions. This can't reasonably be used for globals, because clang would require every function to be annotated with `EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)` for each global mutex; so this introduces a trivial `GlobalMutex` subclass of `Mutex`, and reduces the annotations for both `GlobalMutex` to `LOCKS_EXCLUDED` which only catches trivial errors (eg (`LOCK(x); LOCK(x);`). ACKs for top commit: MarcoFalke: review ACK ce893c0497fc9b8ab9752153dfcc77c9f427545e 🐦 hebasto: ACK ce893c0497fc9b8ab9752153dfcc77c9f427545e Tree-SHA512: 5c35e8c7677ce3d994a7e3774f4344adad496223a51b3a1d1d3b5f20684b2e1d5cff688eb3fbc8d33e1b9940dfa76e515f9434e21de6f3ce3c935e29a319f529 --- doc/developer-notes.md | 41 ++++++++++++++++++++++++++--------- src/httpserver.cpp | 2 +- src/init.cpp | 2 +- src/net.cpp | 2 +- src/net.h | 2 +- src/net_processing.cpp | 2 +- src/netbase.cpp | 2 +- src/qt/clientmodel.h | 2 +- src/rpc/blockchain.cpp | 2 +- src/rpc/server.cpp | 4 ++-- src/sync.h | 49 ++++++++++++++++++++++++++++++++++-------- src/timedata.cpp | 2 +- src/util/system.cpp | 2 +- src/validation.cpp | 2 +- src/validation.h | 2 +- src/wallet/wallet.cpp | 4 ++-- src/warnings.cpp | 2 +- 17 files changed, 88 insertions(+), 36 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 4c0d3deea7b1..ea7d9c1fd526 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -947,16 +947,21 @@ 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 (`AssertLockNotHeld()` can be omitted if `LOCK()` is - called unconditionally after it because `LOCK()` does the same check as - `AssertLockNotHeld()` internally, for non-recursive mutexes): + get compile-time warnings about potential race conditions or deadlocks in code. - In functions that are declared separately from where they are defined, the thread safety annotations should be added exclusively to the function declaration. Annotations on the definition could lead to false positives (lack of compile failure) at call sites between the two. + - Prefer locks that are in a class rather than global, and that are + internal to a class (private or protected) rather than public. + + - Combine annotations in function declarations with run-time asserts in + function definitions (`AssertLockNotHeld()` can be omitted if `LOCK()` is + called unconditionally after it because `LOCK()` does the same check as + `AssertLockNotHeld()` internally, for non-recursive mutexes): + ```C++ // txmempool.h class CTxMemPool @@ -980,21 +985,37 @@ void CTxMemPool::UpdateTransactionsFromBlock(...) ```C++ // validation.h -class ChainstateManager +class CChainState { +protected: + ... + Mutex m_chainstate_mutex; + ... public: ... - bool ProcessNewBlock(...) LOCKS_EXCLUDED(::cs_main); + bool ActivateBestChain( + BlockValidationState& state, + std::shared_ptr pblock = nullptr) + EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex) + LOCKS_EXCLUDED(::cs_main); + ... + bool PreciousBlock(BlockValidationState& state, CBlockIndex* pindex) + EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex) + LOCKS_EXCLUDED(::cs_main); ... } // validation.cpp -bool ChainstateManager::ProcessNewBlock(...) +bool CChainState::PreciousBlock(BlockValidationState& state, CBlockIndex* pindex) { + AssertLockNotHeld(m_chainstate_mutex); AssertLockNotHeld(::cs_main); - ... - LOCK(::cs_main); - ... + { + LOCK(cs_main); + ... + } + + return ActivateBestChain(state, std::shared_ptr()); } ``` diff --git a/src/httpserver.cpp b/src/httpserver.cpp index fe8c8879ea43..7cdf8cd072cb 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -159,7 +159,7 @@ static std::unique_ptr> g_work_queue{nullptr}; //! List of 'external' RPC users (global variable, used by httprpc) std::vector g_external_usernames; //! Handlers for (sub)paths -static Mutex g_httppathhandlers_mutex; +static GlobalMutex g_httppathhandlers_mutex; static std::vector pathHandlers GUARDED_BY(g_httppathhandlers_mutex); //! Bound listening sockets static std::vector boundSockets; diff --git a/src/init.cpp b/src/init.cpp index 25b2ca72090c..bfa35a053cc1 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -800,7 +800,7 @@ void SetupServerArgs(ArgsManager& argsman) } static bool fHaveGenesis = false; -static Mutex g_genesis_wait_mutex; +static GlobalMutex g_genesis_wait_mutex; static std::condition_variable g_genesis_wait_cv; static void BlockNotifyGenesisWait(const CBlockIndex* pBlockIndex) diff --git a/src/net.cpp b/src/net.cpp index b339dd451460..ea96b1162a40 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -128,7 +128,7 @@ static const uint64_t RANDOMIZER_ID_ADDRCACHE = 0x1cf2e4ddd306dda9ULL; // SHA256 // bool fDiscover = true; bool fListen = true; -Mutex g_maplocalhost_mutex; +GlobalMutex g_maplocalhost_mutex; std::map mapLocalHost GUARDED_BY(g_maplocalhost_mutex); std::string strSubVersion; diff --git a/src/net.h b/src/net.h index 33740a656b10..bbbc260a1e11 100644 --- a/src/net.h +++ b/src/net.h @@ -199,7 +199,7 @@ struct LocalServiceInfo { uint16_t nPort; }; -extern Mutex g_maplocalhost_mutex; +extern GlobalMutex g_maplocalhost_mutex; extern std::map mapLocalHost GUARDED_BY(g_maplocalhost_mutex); extern const std::string NET_MESSAGE_TYPE_OTHER; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e449f31d7b27..284b7942ef75 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -332,7 +332,7 @@ struct Peer { return m_tx_relay.get(); }; - TxRelay* GetInvRelay() LOCKS_EXCLUDED(m_tx_relay_mutex) + TxRelay* GetInvRelay() EXCLUSIVE_LOCKS_REQUIRED(!m_tx_relay_mutex) { return WITH_LOCK(m_tx_relay_mutex, return m_tx_relay.get()); } diff --git a/src/netbase.cpp b/src/netbase.cpp index 64034b919b50..7ac84ad4e3d0 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -29,7 +29,7 @@ #endif // Settings -static Mutex g_proxyinfo_mutex; +static GlobalMutex g_proxyinfo_mutex; static Proxy proxyInfo[NET_MAX] GUARDED_BY(g_proxyinfo_mutex); static Proxy nameProxy GUARDED_BY(g_proxyinfo_mutex); int nConnectTimeout = DEFAULT_CONNECT_TIMEOUT; diff --git a/src/qt/clientmodel.h b/src/qt/clientmodel.h index f4f39a4f61f1..241639e2cd14 100644 --- a/src/qt/clientmodel.h +++ b/src/qt/clientmodel.h @@ -123,7 +123,7 @@ class ClientModel : public QObject std::unique_ptr mnListCached GUARDED_BY(cs_mnlist){}; const CBlockIndex* mnListTip{nullptr}; - void TipChanged(SynchronizationState sync_state, interfaces::BlockTip tip, double verification_progress, bool header); + void TipChanged(SynchronizationState sync_state, interfaces::BlockTip tip, double verification_progress, bool header) EXCLUSIVE_LOCKS_REQUIRED(!m_cached_tip_mutex); void subscribeToCoreSignals(); void unsubscribeFromCoreSignals(); diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 35e4d54c1f25..ac07d6a5a029 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -77,7 +77,7 @@ struct CUpdatedBlock int height; }; -static Mutex cs_blockchange; +static GlobalMutex cs_blockchange; static std::condition_variable cond_blockchange; static CUpdatedBlock latestblock GUARDED_BY(cs_blockchange); diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 80e07ad9b1d5..1932268917ae 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -23,14 +23,14 @@ #include #include -static Mutex g_rpc_warmup_mutex; +static GlobalMutex g_rpc_warmup_mutex; static std::atomic g_rpc_running{false}; static bool fRPCInWarmup GUARDED_BY(g_rpc_warmup_mutex) = true; static std::string rpcWarmupStatus GUARDED_BY(g_rpc_warmup_mutex) = "RPC server started"; /* Timer-creating functions */ static RPCTimerInterface* timerInterface = nullptr; /* Map of name to timer. */ -static Mutex g_deadline_timers_mutex; +static GlobalMutex g_deadline_timers_mutex; static std::map > deadlineTimers GUARDED_BY(g_deadline_timers_mutex); static bool ExecuteCommand(const CRPCCommand& command, const JSONRPCRequest& request, UniValue& result, bool last_handler); diff --git a/src/sync.h b/src/sync.h index 6d10e10637ce..8ca945cca76f 100644 --- a/src/sync.h +++ b/src/sync.h @@ -144,6 +144,17 @@ using RecursiveMutex = AnnotatedMixin; /** Wrapped mutex: supports waiting but not recursive locking */ using Mutex = AnnotatedMixin; +/** Different type to mark Mutex at global scope + * + * Thread safety analysis can't handle negative assertions about mutexes + * with global scope well, so mark them with a separate type, and + * eventually move all the mutexes into classes so they are not globally + * visible. + * + * See: https://github.com/bitcoin/bitcoin/pull/20272#issuecomment-720755781 + */ +class GlobalMutex : public Mutex { }; + /** Wrapped shared mutex: supports read locking via .shared_lock, exclusive locking via .lock; * does not support recursive locking */ using SharedMutex = SharedAnnotatedMixin; @@ -152,6 +163,7 @@ using SharedMutex = SharedAnnotatedMixin; inline void AssertLockNotHeldInline(const char* name, const char* file, int line, Mutex* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) { AssertLockNotHeldInternal(name, file, line, cs); } inline void AssertLockNotHeldInline(const char* name, const char* file, int line, RecursiveMutex* cs) LOCKS_EXCLUDED(cs) { AssertLockNotHeldInternal(name, file, line, cs); } +inline void AssertLockNotHeldInline(const char* name, const char* file, int line, GlobalMutex* cs) LOCKS_EXCLUDED(cs) { AssertLockNotHeldInternal(name, file, line, cs); } inline void AssertLockNotHeldInline(const char* name, const char* file, int line, SharedMutex* cs) LOCKS_EXCLUDED(cs) { AssertLockNotHeldInternal(name, file, line, cs); } #define AssertLockNotHeld(cs) AssertLockNotHeldInline(#cs, __FILE__, __LINE__, &cs) @@ -308,14 +320,33 @@ using DebugLock = UniqueLock using ReadLock = SharedLock::type>::type>; -#define LOCK(cs) DebugLock UNIQUE_NAME(criticalblock)(cs, #cs, __FILE__, __LINE__) -#define READ_LOCK(cs) ReadLock UNIQUE_NAME(criticalblock)(cs, #cs, __FILE__, __LINE__) +// When locking a Mutex, require negative capability to ensure the lock +// is not already held +inline Mutex& MaybeCheckNotHeld(Mutex& cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) LOCK_RETURNED(cs) { return cs; } +inline Mutex* MaybeCheckNotHeld(Mutex* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) LOCK_RETURNED(cs) { return cs; } + +// When locking a GlobalMutex, just check it is not locked in the surrounding scope +inline GlobalMutex& MaybeCheckNotHeld(GlobalMutex& cs) LOCKS_EXCLUDED(cs) LOCK_RETURNED(cs) { return cs; } +inline GlobalMutex* MaybeCheckNotHeld(GlobalMutex* cs) LOCKS_EXCLUDED(cs) LOCK_RETURNED(cs) { return cs; } + +// When locking a RecursiveMutex, it's okay to already hold the lock +// but check that it is not known to be locked in the surrounding scope anyway +inline RecursiveMutex& MaybeCheckNotHeld(RecursiveMutex& cs) LOCKS_EXCLUDED(cs) LOCK_RETURNED(cs) { return cs; } +inline RecursiveMutex* MaybeCheckNotHeld(RecursiveMutex* cs) LOCKS_EXCLUDED(cs) LOCK_RETURNED(cs) { return cs; } + +// When locking a SharedMutex, it's okay to already hold the lock +// but check that it is not known to be locked in the surrounding scope anyway +inline SharedMutex& MaybeCheckNotHeld(SharedMutex& cs) LOCKS_EXCLUDED(cs) LOCK_RETURNED(cs) { return cs; } +inline SharedMutex* MaybeCheckNotHeld(SharedMutex* cs) LOCKS_EXCLUDED(cs) LOCK_RETURNED(cs) { return cs; } + +#define LOCK(cs) DebugLock UNIQUE_NAME(criticalblock)(MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__) +#define READ_LOCK(cs) ReadLock UNIQUE_NAME(criticalblock)(MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__) #define LOCK2(cs1, cs2) \ - DebugLock criticalblock1(cs1, #cs1, __FILE__, __LINE__); \ - DebugLock criticalblock2(cs2, #cs2, __FILE__, __LINE__) -#define TRY_LOCK(cs, name) DebugLock name(cs, #cs, __FILE__, __LINE__, true) -#define TRY_READ_LOCK(cs, name) ReadLock name(cs, #cs, __FILE__, __LINE__, true) -#define WAIT_LOCK(cs, name) DebugLock name(cs, #cs, __FILE__, __LINE__) + DebugLock criticalblock1(MaybeCheckNotHeld(cs1), #cs1, __FILE__, __LINE__); \ + DebugLock criticalblock2(MaybeCheckNotHeld(cs2), #cs2, __FILE__, __LINE__) +#define TRY_LOCK(cs, name) DebugLock name(MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__, true) +#define TRY_READ_LOCK(cs, name) ReadLock name(MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__, true) +#define WAIT_LOCK(cs, name) DebugLock name(MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__) #define ENTER_CRITICAL_SECTION(cs) \ { \ @@ -354,8 +385,8 @@ using ReadLock = SharedLock decltype(auto) { LOCK(cs); code; }() -#define WITH_READ_LOCK(cs, code) [&]() -> decltype(auto) { READ_LOCK(cs); code; }() +#define WITH_LOCK(cs, code) (MaybeCheckNotHeld(cs), [&]() -> decltype(auto) { LOCK(cs); code; }()) +#define WITH_READ_LOCK(cs, code) (MaybeCheckNotHeld(cs), [&]() -> decltype(auto) { READ_LOCK(cs); code; }()) /** An implementation of a semaphore. * diff --git a/src/timedata.cpp b/src/timedata.cpp index 8debca706562..ceee08e68c4e 100644 --- a/src/timedata.cpp +++ b/src/timedata.cpp @@ -16,7 +16,7 @@ #include #include -static Mutex g_timeoffset_mutex; +static GlobalMutex g_timeoffset_mutex; static int64_t nTimeOffset GUARDED_BY(g_timeoffset_mutex) = 0; /** diff --git a/src/util/system.cpp b/src/util/system.cpp index e92324817c23..efa58bbd22cf 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -97,7 +97,7 @@ const char * const BITCOIN_SETTINGS_FILENAME = "settings.json"; ArgsManager gArgs; /** Mutex to protect dir_locks. */ -static Mutex cs_dir_locks; +static GlobalMutex cs_dir_locks; /** A map that contains all the currently held directory locks. After * successful locking, these will be held here until the global destructor diff --git a/src/validation.cpp b/src/validation.cpp index 967fe2d716b3..5354f8909c88 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -125,7 +125,7 @@ static constexpr int PRUNE_LOCK_BUFFER{10}; */ RecursiveMutex cs_main; -Mutex g_best_block_mutex; +GlobalMutex g_best_block_mutex; std::condition_variable g_best_block_cv; uint256 g_best_block; bool g_parallel_script_checks{false}; diff --git a/src/validation.h b/src/validation.h index 5da4e69f203f..31318029cf8e 100644 --- a/src/validation.h +++ b/src/validation.h @@ -111,7 +111,7 @@ enum class SynchronizationState { }; extern RecursiveMutex cs_main; -extern Mutex g_best_block_mutex; +extern GlobalMutex g_best_block_mutex; extern std::condition_variable g_best_block_cv; /** Used to notify getblocktemplate RPC of new tips. */ extern uint256 g_best_block; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e9c09e8186e0..1caf30747a3b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -197,8 +197,8 @@ void NotifyWalletLoaded(WalletContext& context, const std::shared_ptr& } } -static Mutex g_loading_wallet_mutex; -static Mutex g_wallet_release_mutex; +static GlobalMutex g_loading_wallet_mutex; +static GlobalMutex g_wallet_release_mutex; static std::condition_variable g_wallet_release_cv; static std::set g_loading_wallet_set GUARDED_BY(g_loading_wallet_mutex); static std::set g_unloading_wallet_set GUARDED_BY(g_wallet_release_mutex); diff --git a/src/warnings.cpp b/src/warnings.cpp index a4ba98333c0f..85719d2dcdb1 100644 --- a/src/warnings.cpp +++ b/src/warnings.cpp @@ -13,7 +13,7 @@ #include -static Mutex g_warnings_mutex; +static GlobalMutex g_warnings_mutex; static bilingual_str g_misc_warnings GUARDED_BY(g_warnings_mutex); static bool fLargeWorkInvalidChainFound GUARDED_BY(g_warnings_mutex) = false; From e87226c90d11f7fd3b2f7a59f2df20146ad450b7 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Sat, 11 Oct 2025 10:46:29 +0700 Subject: [PATCH 5/6] fix: removed duplicated annotations for masternode/meta --- src/masternode/meta.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/masternode/meta.cpp b/src/masternode/meta.cpp index 93dda0796f13..adb2aaa30259 100644 --- a/src/masternode/meta.cpp +++ b/src/masternode/meta.cpp @@ -66,7 +66,7 @@ void CMasternodeMetaInfo::RemoveGovernanceObject(const uint256& nGovernanceObjec mapGovernanceObjectsVotedOn.erase(nGovernanceObjectHash); } -CMasternodeMetaInfoPtr CMasternodeMetaMan::GetMetaInfo(const uint256& proTxHash, bool fCreate) EXCLUSIVE_LOCKS_REQUIRED(!cs) +CMasternodeMetaInfoPtr CMasternodeMetaMan::GetMetaInfo(const uint256& proTxHash, bool fCreate) { LOCK(cs); auto it = metaInfos.find(proTxHash); @@ -115,7 +115,7 @@ bool CMasternodeMetaMan::AddGovernanceVote(const uint256& proTxHash, const uint2 return true; } -void CMasternodeMetaMan::RemoveGovernanceObject(const uint256& nGovernanceObjectHash) EXCLUSIVE_LOCKS_REQUIRED(!cs) +void CMasternodeMetaMan::RemoveGovernanceObject(const uint256& nGovernanceObjectHash) { LOCK(cs); for(const auto& p : metaInfos) { @@ -123,20 +123,20 @@ void CMasternodeMetaMan::RemoveGovernanceObject(const uint256& nGovernanceObject } } -std::vector CMasternodeMetaMan::GetAndClearDirtyGovernanceObjectHashes() EXCLUSIVE_LOCKS_REQUIRED(!cs) +std::vector CMasternodeMetaMan::GetAndClearDirtyGovernanceObjectHashes() { std::vector vecTmp; WITH_LOCK(cs, vecTmp.swap(vecDirtyGovernanceObjectHashes)); return vecTmp; } -bool CMasternodeMetaMan::AlreadyHavePlatformBan(const uint256& inv_hash) const EXCLUSIVE_LOCKS_REQUIRED(!cs) +bool CMasternodeMetaMan::AlreadyHavePlatformBan(const uint256& inv_hash) const { LOCK(cs); return m_seen_platform_bans.exists(inv_hash); } -std::optional CMasternodeMetaMan::GetPlatformBan(const uint256& inv_hash) const EXCLUSIVE_LOCKS_REQUIRED(!cs) +std::optional CMasternodeMetaMan::GetPlatformBan(const uint256& inv_hash) const { LOCK(cs); PlatformBanMessage ret; @@ -147,13 +147,13 @@ std::optional CMasternodeMetaMan::GetPlatformBan(const uint2 return ret; } -void CMasternodeMetaMan::RememberPlatformBan(const uint256& inv_hash, PlatformBanMessage&& msg) EXCLUSIVE_LOCKS_REQUIRED(!cs) +void CMasternodeMetaMan::RememberPlatformBan(const uint256& inv_hash, PlatformBanMessage&& msg) { LOCK(cs); m_seen_platform_bans.insert(inv_hash, std::move(msg)); } -std::string MasternodeMetaStore::ToString() const EXCLUSIVE_LOCKS_REQUIRED(!cs) +std::string MasternodeMetaStore::ToString() const { LOCK(cs); return strprintf("Masternodes: meta infos object count: %d, nDsqCount: %d", metaInfos.size(), nDsqCount); From 8d142aaec0d30db1a20edcaa5b7fce1a51b07d6d Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Fri, 10 Oct 2025 17:19:40 +0300 Subject: [PATCH 6/6] fix: missing asserts for masternode/meta.h after rebase --- src/masternode/meta.h | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/masternode/meta.h b/src/masternode/meta.h index 8d8df8d0df92..217b9d800287 100644 --- a/src/masternode/meta.h +++ b/src/masternode/meta.h @@ -70,18 +70,26 @@ class CMasternodeMetaInfo { } - SERIALIZE_METHODS(CMasternodeMetaInfo, obj) + template + void Serialize(Stream& s) const EXCLUSIVE_LOCKS_REQUIRED(!cs) { - LOCK(obj.cs); - READWRITE(obj.proTxHash, obj.nLastDsq, obj.nMixingTxCount, obj.mapGovernanceObjectsVotedOn, - obj.outboundAttemptCount, obj.lastOutboundAttempt, obj.lastOutboundSuccess, obj.m_platform_ban, - obj.m_platform_ban_updated); + LOCK(cs); + s << proTxHash << nLastDsq << nMixingTxCount << mapGovernanceObjectsVotedOn << outboundAttemptCount + << lastOutboundAttempt << lastOutboundSuccess << m_platform_ban << m_platform_ban_updated; + } + + template + void Unserialize(Stream& s) EXCLUSIVE_LOCKS_REQUIRED(!cs) + { + LOCK(cs); + s >> proTxHash >> nLastDsq >> nMixingTxCount >> mapGovernanceObjectsVotedOn >> outboundAttemptCount >> + lastOutboundAttempt >> lastOutboundSuccess >> m_platform_ban >> m_platform_ban_updated; } - UniValue ToJson() const; + UniValue ToJson() const EXCLUSIVE_LOCKS_REQUIRED(!cs); public: - const uint256 GetProTxHash() const + const uint256 GetProTxHash() const EXCLUSIVE_LOCKS_REQUIRED(!cs) { LOCK(cs); return proTxHash; @@ -92,16 +100,16 @@ class CMasternodeMetaInfo bool IsValidForMixingTxes() const { return GetMixingTxCount() <= MASTERNODE_MAX_MIXING_TXES; } // KEEP TRACK OF EACH GOVERNANCE ITEM IN CASE THIS NODE GOES OFFLINE, SO WE CAN RECALCULATE THEIR STATUS - void AddGovernanceVote(const uint256& nGovernanceObjectHash); + void AddGovernanceVote(const uint256& nGovernanceObjectHash) EXCLUSIVE_LOCKS_REQUIRED(!cs); - void RemoveGovernanceObject(const uint256& nGovernanceObjectHash); + void RemoveGovernanceObject(const uint256& nGovernanceObjectHash) EXCLUSIVE_LOCKS_REQUIRED(!cs); bool OutboundFailedTooManyTimes() const { return outboundAttemptCount > MASTERNODE_MAX_FAILED_OUTBOUND_ATTEMPTS; } void SetLastOutboundAttempt(int64_t t) { lastOutboundAttempt = t; ++outboundAttemptCount; } int64_t GetLastOutboundAttempt() const { return lastOutboundAttempt; } void SetLastOutboundSuccess(int64_t t) { lastOutboundSuccess = t; outboundAttemptCount = 0; } int64_t GetLastOutboundSuccess() const { return lastOutboundSuccess; } - bool SetPlatformBan(bool is_banned, int height) + bool SetPlatformBan(bool is_banned, int height) EXCLUSIVE_LOCKS_REQUIRED(!cs) { LOCK(cs); if (height < m_platform_ban_updated) { @@ -114,7 +122,7 @@ class CMasternodeMetaInfo m_platform_ban_updated = height; return true; } - bool IsPlatformBanned() const + bool IsPlatformBanned() const EXCLUSIVE_LOCKS_REQUIRED(!cs) { LOCK(cs); return m_platform_ban;