From 042e8a4101d8776b5eaf75f559ac705404245205 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 7 Oct 2024 00:21:21 +0700 Subject: [PATCH 01/10] fix: drop unneded lock assert in net.cpp for m_nodes_mutex --- src/net.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 474fbec3e701..49e9ca2c82db 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -643,8 +643,6 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo void CNode::CloseSocketDisconnect(CConnman* connman) { - AssertLockHeld(connman->m_nodes_mutex); - fDisconnect = true; LOCK2(connman->cs_mapSocketToNode, m_sock_mutex); if (!m_sock) { From a219a8be154be6d31873de2985dcf4509e19b62b Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 26 Sep 2024 00:47:28 +0700 Subject: [PATCH 02/10] partial Merge #29040: refactor: Remove pre-C++20 code, fs::path cleanup It fixes return reference to const variable under mutex which is not a good idea --- src/util/system.cpp | 4 ++-- src/util/system.h | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/util/system.cpp b/src/util/system.cpp index b0af7eb30b6b..9ecadeabce70 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -420,7 +420,7 @@ std::optional ArgsManager::GetArgFlags(const std::string& name) co return std::nullopt; } -const fs::path& ArgsManager::GetBlocksDirPath() const +const fs::path ArgsManager::GetBlocksDirPath() const { LOCK(cs_args); fs::path& path = m_cached_blocks_path; @@ -445,7 +445,7 @@ const fs::path& ArgsManager::GetBlocksDirPath() const return path; } -const fs::path& ArgsManager::GetDataDir(bool net_specific) const +const fs::path ArgsManager::GetDataDir(bool net_specific) const { LOCK(cs_args); fs::path& path = net_specific ? m_cached_network_datadir_path : m_cached_datadir_path; diff --git a/src/util/system.h b/src/util/system.h index 1ae230d4f80e..4ab19054596a 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -288,7 +288,7 @@ class ArgsManager * * @return Blocks path which is network specific */ - const fs::path& GetBlocksDirPath() const; + const fs::path GetBlocksDirPath() const; /** * Get data directory path @@ -296,7 +296,7 @@ class ArgsManager * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned * @post Returned directory path is created unless it is empty */ - const fs::path& GetDataDirBase() const { return GetDataDir(false); } + const fs::path GetDataDirBase() const { return GetDataDir(false); } /** * Get data directory path with appended network identifier @@ -304,7 +304,7 @@ class ArgsManager * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned * @post Returned directory path is created unless it is empty */ - const fs::path& GetDataDirNet() const { return GetDataDir(true); } + const fs::path GetDataDirNet() const { return GetDataDir(true); } fs::path GetBackupsDirPath(); @@ -483,7 +483,7 @@ class ArgsManager * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned * @post Returned directory path is created unless it is empty */ - const fs::path& GetDataDir(bool net_specific) const; + const fs::path GetDataDir(bool net_specific) const; // Helper function for LogArgs(). void logArgsPrefix( From 502d6ae8efd024827afbb93db5bb10f50f485505 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 26 Sep 2024 00:20:01 +0700 Subject: [PATCH 03/10] fix: add multiple missing annotation about locks for dash specific code --- src/coinjoin/client.cpp | 9 +++++++-- src/coinjoin/client.h | 13 +++++++++---- src/coinjoin/coinjoin.h | 4 ++-- src/evo/deterministicmns.cpp | 12 +++++++----- src/evo/simplifiedmns.h | 7 ++++--- src/evo/specialtxman.cpp | 7 ++++--- src/governance/governance.h | 9 +++++---- src/governance/object.h | 3 ++- src/llmq/snapshot.h | 2 +- src/masternode/meta.h | 6 +++++- src/net_processing.cpp | 2 +- src/rpc/evo.cpp | 5 +++-- src/txmempool.cpp | 4 ++-- src/wallet/wallet.cpp | 2 +- src/wallet/wallet.h | 18 +++++++++++------- 15 files changed, 64 insertions(+), 39 deletions(-) diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp index 9739727243ed..4c15f98f4a51 100644 --- a/src/coinjoin/client.cpp +++ b/src/coinjoin/client.cpp @@ -1448,11 +1448,16 @@ bool CCoinJoinClientSession::MakeCollateralAmounts() }); // First try to use only non-denominated funds - if (std::any_of(vecTally.begin(), vecTally.end(), [&](const auto& item) { return MakeCollateralAmounts(item, false); })) { + if (ranges::any_of(vecTally, [&](const auto& item) EXCLUSIVE_LOCKS_REQUIRED(m_wallet.cs_wallet) { + return MakeCollateralAmounts(item, false); + })) { return true; } + // There should be at least some denominated funds we should be able to break in pieces to continue mixing - if (std::any_of(vecTally.begin(), vecTally.end(), [&](const auto& item) { return MakeCollateralAmounts(item, true); })) { + if (ranges::any_of(vecTally, [&](const auto& item) EXCLUSIVE_LOCKS_REQUIRED(m_wallet.cs_wallet) { + return MakeCollateralAmounts(item, true); + })) { return true; } diff --git a/src/coinjoin/client.h b/src/coinjoin/client.h index a0deebdbd24b..c4e22b71230c 100644 --- a/src/coinjoin/client.h +++ b/src/coinjoin/client.h @@ -161,13 +161,16 @@ class CCoinJoinClientSession : public CCoinJoinBaseSession /// Create denominations bool CreateDenominated(CAmount nBalanceToDenominate); - bool CreateDenominated(CAmount nBalanceToDenominate, const CompactTallyItem& tallyItem, bool fCreateMixingCollaterals); + bool CreateDenominated(CAmount nBalanceToDenominate, const CompactTallyItem& tallyItem, bool fCreateMixingCollaterals) + EXCLUSIVE_LOCKS_REQUIRED(m_wallet.cs_wallet); /// Split up large inputs or make fee sized inputs bool MakeCollateralAmounts(); - bool MakeCollateralAmounts(const CompactTallyItem& tallyItem, bool fTryDenominated); + bool MakeCollateralAmounts(const CompactTallyItem& tallyItem, bool fTryDenominated) + EXCLUSIVE_LOCKS_REQUIRED(m_wallet.cs_wallet); - bool CreateCollateralTransaction(CMutableTransaction& txCollateral, std::string& strReason); + bool CreateCollateralTransaction(CMutableTransaction& txCollateral, std::string& strReason) + EXCLUSIVE_LOCKS_REQUIRED(m_wallet.cs_wallet); bool JoinExistingQueue(CAmount nBalanceNeedsAnonymized, CConnman& connman); bool StartNewQueue(CAmount nBalanceNeedsAnonymized, CConnman& connman); @@ -175,7 +178,9 @@ class CCoinJoinClientSession : public CCoinJoinBaseSession /// step 0: select denominated inputs and txouts bool SelectDenominate(std::string& strErrorRet, std::vector& vecTxDSInRet); /// step 1: prepare denominated inputs and outputs - bool PrepareDenominate(int nMinRounds, int nMaxRounds, std::string& strErrorRet, const std::vector& vecTxDSIn, std::vector >& vecPSInOutPairsRet, bool fDryRun = false); + bool PrepareDenominate(int nMinRounds, int nMaxRounds, std::string& strErrorRet, const std::vector& vecTxDSIn, + std::vector>& vecPSInOutPairsRet, bool fDryRun = false) + EXCLUSIVE_LOCKS_REQUIRED(m_wallet.cs_wallet); /// step 2: send denominated inputs and outputs prepared in step 1 bool SendDenominate(const std::vector >& vecPSInOutPairsIn, CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin); diff --git a/src/coinjoin/coinjoin.h b/src/coinjoin/coinjoin.h index 6bf8e93e112b..5680b8abc20f 100644 --- a/src/coinjoin/coinjoin.h +++ b/src/coinjoin/coinjoin.h @@ -398,8 +398,8 @@ class CDSTXManager private: void CheckDSTXes(const CBlockIndex* pindex, const llmq::CChainLocksHandler& clhandler) EXCLUSIVE_LOCKS_REQUIRED(!cs_mapdstx); - void UpdateDSTXConfirmedHeight(const CTransactionRef& tx, std::optional nHeight); - + void UpdateDSTXConfirmedHeight(const CTransactionRef& tx, std::optional nHeight) + EXCLUSIVE_LOCKS_REQUIRED(cs_mapdstx); }; bool ATMPIfSaneFee(ChainstateManager& chainman, const CTransactionRef& tx, bool test_accept = false) diff --git a/src/evo/deterministicmns.cpp b/src/evo/deterministicmns.cpp index ac167a298bb3..49912b888061 100644 --- a/src/evo/deterministicmns.cpp +++ b/src/evo/deterministicmns.cpp @@ -1067,11 +1067,13 @@ CDeterministicMNList CDeterministicMNManager::GetListForBlockInternal(gsl::not_n mnListsCache.emplace(snapshot.GetBlockHash(), snapshot); } else { // keep snapshots for yet alive quorums - if (ranges::any_of(Params().GetConsensus().llmqs, [&snapshot, this](const auto& params){ - AssertLockHeld(cs); - return (snapshot.GetHeight() % params.dkgInterval == 0) && - (snapshot.GetHeight() + params.dkgInterval * (params.keepOldConnections + 1) >= tipIndex->nHeight); - })) { + if (ranges::any_of(Params().GetConsensus().llmqs, + [&snapshot, this](const auto& params) EXCLUSIVE_LOCKS_REQUIRED(cs) { + AssertLockHeld(cs); + return (snapshot.GetHeight() % params.dkgInterval == 0) && + (snapshot.GetHeight() + params.dkgInterval * (params.keepOldConnections + 1) >= + tipIndex->nHeight); + })) { mnListsCache.emplace(snapshot.GetBlockHash(), snapshot); } } diff --git a/src/evo/simplifiedmns.h b/src/evo/simplifiedmns.h index 9999a5f35205..0cfd5603d9bb 100644 --- a/src/evo/simplifiedmns.h +++ b/src/evo/simplifiedmns.h @@ -171,8 +171,9 @@ class CSimplifiedMNListDiff [[nodiscard]] UniValue ToJson(bool extended = false) const; }; -bool BuildSimplifiedMNListDiff(CDeterministicMNManager& dmnman, const ChainstateManager& chainman, const llmq::CQuorumBlockProcessor& qblockman, - const llmq::CQuorumManager& qman, const uint256& baseBlockHash, const uint256& blockHash, - CSimplifiedMNListDiff& mnListDiffRet, std::string& errorRet, bool extended = false); +bool BuildSimplifiedMNListDiff(CDeterministicMNManager& dmnman, const ChainstateManager& chainman, + const llmq::CQuorumBlockProcessor& qblockman, const llmq::CQuorumManager& qman, + const uint256& baseBlockHash, const uint256& blockHash, CSimplifiedMNListDiff& mnListDiffRet, + std::string& errorRet, bool extended = false) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); #endif // BITCOIN_EVO_SIMPLIFIEDMNS_H diff --git a/src/evo/specialtxman.cpp b/src/evo/specialtxman.cpp index e20a9e0d4f7b..c8c84e84bed4 100644 --- a/src/evo/specialtxman.cpp +++ b/src/evo/specialtxman.cpp @@ -19,9 +19,10 @@ #include #include -static bool CheckSpecialTxInner(CDeterministicMNManager& dmnman, const ChainstateManager& chainman, const llmq::CQuorumManager& qman, const CTransaction& tx, - const CBlockIndex* pindexPrev, const CCoinsViewCache& view, const std::optional& indexes, bool check_sigs, - TxValidationState& state) +static bool CheckSpecialTxInner(CDeterministicMNManager& dmnman, const ChainstateManager& chainman, + const llmq::CQuorumManager& qman, const CTransaction& tx, const CBlockIndex* pindexPrev, + const CCoinsViewCache& view, const std::optional& indexes, bool check_sigs, + TxValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { AssertLockHeld(cs_main); diff --git a/src/governance/governance.h b/src/governance/governance.h index e5b8e077a8ee..adc25bb5431c 100644 --- a/src/governance/governance.h +++ b/src/governance/governance.h @@ -347,9 +347,9 @@ class CGovernanceManager : public GovernanceStore * - Track governance objects which are triggers * - After triggers are activated and executed, they can be removed */ - std::vector> GetActiveTriggers() const; - bool AddNewTrigger(uint256 nHash); - void CleanAndRemoveTriggers(); + std::vector> GetActiveTriggers() const EXCLUSIVE_LOCKS_REQUIRED(cs); + bool AddNewTrigger(uint256 nHash) EXCLUSIVE_LOCKS_REQUIRED(cs); + void CleanAndRemoveTriggers() EXCLUSIVE_LOCKS_REQUIRED(cs); // Superblocks related: @@ -373,7 +373,8 @@ class CGovernanceManager : public GovernanceStore private: void ExecuteBestSuperblock(const CDeterministicMNList& tip_mn_list, int nBlockHeight); - bool GetBestSuperblock(const CDeterministicMNList& tip_mn_list, CSuperblock_sptr& pSuperblockRet, int nBlockHeight); + bool GetBestSuperblock(const CDeterministicMNList& tip_mn_list, CSuperblock_sptr& pSuperblockRet, int nBlockHeight) + EXCLUSIVE_LOCKS_REQUIRED(cs); std::optional CreateSuperblockCandidate(int nHeight) const; std::optional CreateGovernanceTrigger(const std::optional& sb_opt, PeerManager& peerman, diff --git a/src/governance/object.h b/src/governance/object.h index e58da6a8a1be..8ced4950f939 100644 --- a/src/governance/object.h +++ b/src/governance/object.h @@ -235,7 +235,8 @@ class CGovernanceObject /// Check the collateral transaction for the budget proposal/finalized budget bool IsCollateralValid(const ChainstateManager& chainman, std::string& strError, bool& fMissingConfirmations) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); - void UpdateLocalValidity(const CDeterministicMNList& tip_mn_list, const ChainstateManager& chainman); + void UpdateLocalValidity(const CDeterministicMNList& tip_mn_list, const ChainstateManager& chainman) + EXCLUSIVE_LOCKS_REQUIRED(cs_main); void UpdateSentinelVariables(const CDeterministicMNList& tip_mn_list); diff --git a/src/llmq/snapshot.h b/src/llmq/snapshot.h index b9ca9ac985dd..1551789f3779 100644 --- a/src/llmq/snapshot.h +++ b/src/llmq/snapshot.h @@ -209,7 +209,7 @@ class CQuorumRotationInfo bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, const ChainstateManager& chainman, const CQuorumManager& qman, const CQuorumBlockProcessor& qblockman, const CGetQuorumRotationInfo& request, - CQuorumRotationInfo& response, std::string& errorRet); + CQuorumRotationInfo& response, std::string& errorRet) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); uint256 GetLastBaseBlockHash(Span baseBlockIndexes, const CBlockIndex* blockIndex); class CQuorumSnapshotManager diff --git a/src/masternode/meta.h b/src/masternode/meta.h index 0388905c4462..14870afa636c 100644 --- a/src/masternode/meta.h +++ b/src/masternode/meta.h @@ -73,7 +73,11 @@ class CMasternodeMetaInfo UniValue ToJson() const; public: - const uint256& GetProTxHash() const { LOCK(cs); return proTxHash; } + const uint256 GetProTxHash() const + { + LOCK(cs); + return proTxHash; + } int64_t GetLastDsq() const { return nLastDsq; } int GetMixingTxCount() const { return nMixingTxCount; } diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 9536b8fd07b6..453775423117 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -5771,7 +5771,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) peer->m_blocks_for_inv_relay.clear(); } - auto queueAndMaybePushInv = [this, pto, peer, &vInv, &msgMaker](const CInv& invIn) { + auto queueAndMaybePushInv = [this, pto, peer, &vInv, &msgMaker](const CInv& invIn) EXCLUSIVE_LOCKS_REQUIRED(peer->m_tx_relay->m_tx_inventory_mutex) { AssertLockHeld(peer->m_tx_relay->m_tx_inventory_mutex); peer->m_tx_relay->m_tx_inventory_known_filter.insert(invIn.hash); LogPrint(BCLog::NET, "SendMessages -- queued inv: %s index=%d peer=%d\n", invIn.ToString(), vInv.size(), pto->GetId()); diff --git a/src/rpc/evo.cpp b/src/rpc/evo.cpp index 30fdab04b734..5fa7506cdaa3 100644 --- a/src/rpc/evo.cpp +++ b/src/rpc/evo.cpp @@ -213,8 +213,9 @@ static bool ValidatePlatformPort(const int32_t port) #ifdef ENABLE_WALLET -template -static void FundSpecialTx(CWallet& wallet, CMutableTransaction& tx, const SpecialTxPayload& payload, const CTxDestination& fundDest) +template +static void FundSpecialTx(CWallet& wallet, CMutableTransaction& tx, const SpecialTxPayload& payload, + const CTxDestination& fundDest) EXCLUSIVE_LOCKS_REQUIRED(!wallet.cs_wallet) { // Make sure the results are valid at least up to the most recent block // the user could have gotten from another RPC command prior to now diff --git a/src/txmempool.cpp b/src/txmempool.cpp index cb16e2ddfc7b..291599aea201 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -955,7 +955,7 @@ void CTxMemPool::removeProTxSpentCollateralConflicts(const CTransaction &tx) assert(m_dmnman); // Remove TXs that refer to a MN for which the collateral was spent - auto removeSpentCollateralConflict = [&](const uint256& proTxHash) { + auto removeSpentCollateralConflict = [&](const uint256& proTxHash) EXCLUSIVE_LOCKS_REQUIRED(cs) { // Can't use equal_range here as every call to removeRecursive might invalidate iterators AssertLockHeld(cs); while (true) { @@ -1353,7 +1353,7 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const { LOCK(cs); - auto hasKeyChangeInMempool = [&](const uint256& proTxHash) { + auto hasKeyChangeInMempool = [&](const uint256& proTxHash) EXCLUSIVE_LOCKS_REQUIRED(cs) { AssertLockHeld(cs); for (auto its = mapProTxRefs.equal_range(proTxHash); its.first != its.second; ++its.first) { auto txit = mapTx.find(its.first->second); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index edc7b41c9ef8..b6ac4595a810 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3681,7 +3681,7 @@ bool CWallet::CreateTransactionInternal( txNew.vin.emplace_back(coin.outpoint, CScript(), CTxIn::SEQUENCE_FINAL - 1); } - auto calculateFee = [&](CAmount& nFee) -> bool { + auto calculateFee = [&](CAmount& nFee) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) -> bool { AssertLockHeld(cs_wallet); nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); if (nBytes < 0) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 11824d79ee5e..411367ac2bf1 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -516,8 +516,12 @@ class CWalletTx CAmount GetImmatureWatchOnlyCredit(const bool fUseCache = true) const; CAmount GetChange() const; - CAmount GetAnonymizedCredit(const CCoinControl* coinControl = nullptr) const; - CAmount GetDenominatedCredit(bool unconfirmed, bool fUseCache=true) const; + // TODO: Remove "NO_THREAD_SAFETY_ANALYSIS" and replace it with the correct + // annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The + // annotation "NO_THREAD_SAFETY_ANALYSIS" was temporarily added to avoid + // having to resolve the issue of member access into incomplete type CWallet. + CAmount GetAnonymizedCredit(const CCoinControl* coinControl = nullptr) const NO_THREAD_SAFETY_ANALYSIS; + CAmount GetDenominatedCredit(bool unconfirmed, bool fUseCache=true) const NO_THREAD_SAFETY_ANALYSIS; /** Get the marginal bytes if spending the specified output from this transaction */ int GetSpendSize(unsigned int out, bool use_max_sig = false) const @@ -569,7 +573,7 @@ class CWalletTx int GetDepthInMainChain() const NO_THREAD_SAFETY_ANALYSIS; bool IsInMainChain() const { return GetDepthInMainChain() > 0; } bool IsLockedByInstantSend() const; - bool IsChainLocked() const; + bool IsChainLocked() const NO_THREAD_SAFETY_ANALYSIS; /** * @return number of blocks to maturity for this transaction: @@ -1289,7 +1293,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati std::set GetConflicts(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Check if a given transaction has any of its outputs spent by another transaction in the wallet - bool HasWalletSpend(const uint256& txid) const; + bool HasWalletSpend(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Flush wallet (bitdb flush) void Flush(); @@ -1386,11 +1390,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati void notifyChainLock(const CBlockIndex* pindexChainLock, const std::shared_ptr& clsig) override; /** Load a CGovernanceObject into m_gobjects. */ - bool LoadGovernanceObject(const Governance::Object& obj); + bool LoadGovernanceObject(const Governance::Object& obj) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** Store a CGovernanceObject in the wallet database. This should only be used by governance objects that are created by this wallet via `gobject prepare`. */ - bool WriteGovernanceObject(const Governance::Object& obj); + bool WriteGovernanceObject(const Governance::Object& obj) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** Returns a vector containing pointers to the governance objects in m_gobjects */ - std::vector GetGovernanceObjects(); + std::vector GetGovernanceObjects() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** * Blocks until the wallet state is up-to-date to /at least/ the current From 002580c42d8fc2d7f91ec0ed27876464db0771d2 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Sun, 6 Oct 2024 23:54:32 +0700 Subject: [PATCH 04/10] fix: add ignoring safety-annotation for llmq/signing_shares due to template lambdas --- src/llmq/signing_shares.cpp | 56 +++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 21 deletions(-) diff --git a/src/llmq/signing_shares.cpp b/src/llmq/signing_shares.cpp index 0b157f218ab5..47a70430fa68 100644 --- a/src/llmq/signing_shares.cpp +++ b/src/llmq/signing_shares.cpp @@ -565,24 +565,29 @@ void CSigSharesManager::CollectPendingSigSharesToVerify( // other nodes would be able to poison us with a large batch with N-1 valid shares and the last one being // invalid, making batch verification fail and revert to per-share verification, which in turn would slow down // the whole verification process - std::unordered_set, StaticSaltedHasher> uniqueSignHashes; - IterateNodesRandom(nodeStates, [&]() { - return uniqueSignHashes.size() < maxUniqueSessions; - }, [&](NodeId nodeId, CSigSharesNodeState& ns) { - if (ns.pendingIncomingSigShares.Empty()) { - return false; - } - const auto& sigShare = *ns.pendingIncomingSigShares.GetFirst(); + IterateNodesRandom( + nodeStates, + [&]() { + return uniqueSignHashes.size() < maxUniqueSessions; + // TODO: remove NO_THREAD_SAFETY_ANALYSIS + // using here template IterateNodesRandom makes impossible to use lock annotation + }, + [&](NodeId nodeId, CSigSharesNodeState& ns) NO_THREAD_SAFETY_ANALYSIS { + if (ns.pendingIncomingSigShares.Empty()) { + return false; + } + const auto& sigShare = *ns.pendingIncomingSigShares.GetFirst(); - AssertLockHeld(cs); - if (const bool alreadyHave = this->sigShares.Has(sigShare.GetKey()); !alreadyHave) { - uniqueSignHashes.emplace(nodeId, sigShare.GetSignHash()); - retSigShares[nodeId].emplace_back(sigShare); - } - ns.pendingIncomingSigShares.Erase(sigShare.GetKey()); - return !ns.pendingIncomingSigShares.Empty(); - }, rnd); + AssertLockHeld(cs); + if (const bool alreadyHave = this->sigShares.Has(sigShare.GetKey()); !alreadyHave) { + uniqueSignHashes.emplace(nodeId, sigShare.GetSignHash()); + retSigShares[nodeId].emplace_back(sigShare); + } + ns.pendingIncomingSigShares.Erase(sigShare.GetKey()); + return !ns.pendingIncomingSigShares.Empty(); + }, + rnd); if (retSigShares.empty()) { return; @@ -1020,7 +1025,10 @@ void CSigSharesManager::CollectSigSharesToAnnounce(std::unordered_map, std::unordered_set, StaticSaltedHasher> quorumNodesMap; - sigSharesQueuedToAnnounce.ForEach([this, &quorumNodesMap, &sigSharesToAnnounce](const SigShareKey& sigShareKey, bool) { + // TODO: remove NO_THREAD_SAFETY_ANALYSIS + // using here template ForEach makes impossible to use lock annotation + sigSharesQueuedToAnnounce.ForEach([this, &quorumNodesMap, &sigSharesToAnnounce](const SigShareKey& sigShareKey, + bool) NO_THREAD_SAFETY_ANALYSIS { AssertLockHeld(cs); const auto& signHash = sigShareKey.first; auto quorumMember = sigShareKey.second; @@ -1076,7 +1084,7 @@ bool CSigSharesManager::SendMessages() std::unordered_map> sigSharesToAnnounce; std::unordered_map> sigSessionAnnouncements; - auto addSigSesAnnIfNeeded = [&](NodeId nodeId, const uint256& signHash) { + auto addSigSesAnnIfNeeded = [&](NodeId nodeId, const uint256& signHash) EXCLUSIVE_LOCKS_REQUIRED(cs) { AssertLockHeld(cs); auto& nodeState = nodeStates[nodeId]; auto* session = nodeState.GetSessionBySignHash(signHash); @@ -1357,7 +1365,9 @@ void CSigSharesManager::Cleanup() continue; } // remove global requested state to force a re-request from another node - it->second.requestedSigShares.ForEach([this](const SigShareKey& k, bool) { + // TODO: remove NO_THREAD_SAFETY_ANALYSIS + // using here template ForEach makes impossible to use lock annotation + it->second.requestedSigShares.ForEach([this](const SigShareKey& k, bool) NO_THREAD_SAFETY_ANALYSIS { AssertLockHeld(cs); sigSharesRequested.Erase(k); }); @@ -1390,7 +1400,9 @@ void CSigSharesManager::RemoveBannedNodeStates() for (auto it = nodeStates.begin(); it != nodeStates.end();) { if (Assert(m_peerman)->IsBanned(it->first)) { // re-request sigshares from other nodes - it->second.requestedSigShares.ForEach([this](const SigShareKey& k, int64_t) { + // TODO: remove NO_THREAD_SAFETY_ANALYSIS + // using here template ForEach makes impossible to use lock annotation + it->second.requestedSigShares.ForEach([this](const SigShareKey& k, int64_t) NO_THREAD_SAFETY_ANALYSIS { AssertLockHeld(cs); sigSharesRequested.Erase(k); }); @@ -1419,7 +1431,9 @@ void CSigSharesManager::BanNode(NodeId nodeId) auto& nodeState = it->second; // Whatever we requested from him, let's request it from someone else now - nodeState.requestedSigShares.ForEach([this](const SigShareKey& k, int64_t) { + // TODO: remove NO_THREAD_SAFETY_ANALYSIS + // using here template ForEach makes impossible to use lock annotation + nodeState.requestedSigShares.ForEach([this](const SigShareKey& k, int64_t) NO_THREAD_SAFETY_ANALYSIS { AssertLockHeld(cs); sigSharesRequested.Erase(k); }); From 846ebab6e06601105a3eb22eb6b3c6ff68426adb Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 7 Oct 2024 00:35:30 +0700 Subject: [PATCH 05/10] fix: fixes for orphange's locks and cs_main --- src/net_processing.cpp | 12 +++++++----- src/txorphanage.cpp | 2 +- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 453775423117..803f3d51aa73 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1930,12 +1930,14 @@ void PeerManagerImpl::StartScheduledTasks(CScheduler& scheduler) */ void PeerManagerImpl::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindex) { - LOCK2(::cs_main, g_cs_orphans); + { + LOCK2(::cs_main, g_cs_orphans); - auto orphanWorkSet = m_orphanage.GetCandidatesForBlock(*pblock); - while (!orphanWorkSet.empty()) { - LogPrint(BCLog::MEMPOOL, "Trying to process %d orphans\n", orphanWorkSet.size()); - ProcessOrphanTx(orphanWorkSet); + auto orphanWorkSet = m_orphanage.GetCandidatesForBlock(*pblock); + while (!orphanWorkSet.empty()) { + LogPrint(BCLog::MEMPOOL, "Trying to process %d orphans\n", orphanWorkSet.size()); + ProcessOrphanTx(orphanWorkSet); + } } m_orphanage.EraseForBlock(*pblock); diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 84835e0aad61..ce9d6435ce9f 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -197,7 +197,7 @@ std::set TxOrphanage::GetCandidatesForBlock(const CBlock& block) void TxOrphanage::EraseForBlock(const CBlock& block) { - AssertLockHeld(g_cs_orphans); + LOCK(g_cs_orphans); std::vector vOrphanErase; From 6d452845dc8caf2c77231218a508676e35180bdf Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 7 Oct 2024 01:09:14 +0700 Subject: [PATCH 06/10] fix: add missing lock annotation for ContextualCheckBlock and related TODO --- src/validation.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 38a030ace1c6..afd909c993d3 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3644,9 +3644,10 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidatio * in ConnectBlock(). * Note that -reindex-chainstate skips the validation that happens here! */ -static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev) +static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { - AssertLockHeld(cs_main); + // TODO: validate - why do we need this cs_main ? + AssertLockHeld(::cs_main); const int nHeight = pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1; // Enforce BIP113 (Median Time Past). From 898282d6206381532ae78ade0b47f614afbe7c60 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 23 Jan 2024 14:50:58 -0500 Subject: [PATCH 07/10] Merge bitcoin/bitcoin#28774: wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it (Vasil Dimov) Pull request description: `CWallet::GetEncryptionKey()` would return a reference to the internal `CWallet::vMasterKey`, guarded by `CWallet::cs_wallet`, which is unsafe. Returning a copy would be a shorter solution, but could have security implications of the master key remaining somewhere in the memory even after `CWallet::Lock()` (the current calls to `CWallet::GetEncryptionKey()` are safe, but that is not future proof). So, instead of `EncryptSecret(m_storage.GetEncryptionKey(), ...)` change the `GetEncryptionKey()` method to provide the encryption key to a given callback: `m_storage.WithEncryptionKey([](const CKeyingMaterial& k) { EncryptSecret(k, ...); })` This silences the following (clang 18): ``` wallet/wallet.cpp:3520:12: error: returning variable 'vMasterKey' by reference requires holding mutex 'cs_wallet' [-Werror,-Wthread-safety-reference-return] 3520 | return vMasterKey; | ^ ``` --- _Previously this PR modified both ArgsManager and wallet code. But the ArgsManager commit https://github.com/bitcoin/bitcoin/commit/856c88776f8486446602476a1c9e133ac0cff510 was merged in https://github.com/bitcoin/bitcoin/pull/29040 so now this only affects wallet code. The previous PR description was:_ Avoid this unsafe pattern from `ArgsManager` and `CWallet`: ```cpp class A { Mutex mutex; Foo member GUARDED_BY(mutex); const Foo& Get() { LOCK(mutex); return member; } // callers of `Get()` will have access to `member` without owning the mutex. ``` ACKs for top commit: achow101: ACK 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b ryanofsky: Code review ACK 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b. This seems like a potentially real race condition, and the fix here is pretty simple. furszy: ACK 32a9f13c Tree-SHA512: 133da84691642afc1a73cf14ad004a7266cb4be1a6a3ec634d131dca5dbcdef52522c1d5eb04f5b6c4e06e1fc3e6ac57315f8fe1e207b464ca025c2b4edefdc1 --- src/wallet/scriptpubkeyman.cpp | 36 ++++++++++++++++++++++++++-------- src/wallet/scriptpubkeyman.h | 4 +++- src/wallet/wallet.cpp | 5 +++-- src/wallet/wallet.h | 3 ++- 4 files changed, 36 insertions(+), 12 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 9d84a04b6a32..52d07ff7fa50 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -342,8 +342,11 @@ void LegacyScriptPubKeyMan::UpgradeKeyMetadata() CHDChain hdChainCurrent; if (!GetHDChain(hdChainCurrent)) throw std::runtime_error(std::string(__func__) + ": GetHDChain failed"); - if (!DecryptHDChain(m_storage.GetEncryptionKey(), hdChainCurrent)) + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptHDChain(encryption_key, hdChainCurrent); + })) { throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed"); + } CExtKey masterKey; SecureVector vchSeed = hdChainCurrent.GetSeed(); @@ -474,8 +477,11 @@ bool LegacyScriptPubKeyMan::GetDecryptedHDChain(CHDChain& hdChainRet) return false; } - if (!DecryptHDChain(m_storage.GetEncryptionKey(), hdChainTmp)) + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptHDChain(encryption_key, hdChainTmp); + })) { return false; + } // make sure seed matches this chain if (hdChainTmp.GetID() != hdChainTmp.GetSeedHash()) @@ -911,7 +917,9 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyInner(const CKey& key, const CPubKey &pu std::vector vchCryptedSecret; CKeyingMaterial vchSecret(key.begin(), key.end()); - if (!EncryptSecret(m_storage.GetEncryptionKey(), vchSecret, pubkey.GetHash(), vchCryptedSecret)) { + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return EncryptSecret(encryption_key, vchSecret, pubkey.GetHash(), vchCryptedSecret); + })) { return false; } @@ -933,7 +941,9 @@ bool LegacyScriptPubKeyMan::GetKeyInner(const CKeyID &address, CKey& keyOut) con { const CPubKey &vchPubKey = (*mi).second.first; const std::vector &vchCryptedSecret = (*mi).second.second; - return DecryptKey(m_storage.GetEncryptionKey(), vchCryptedSecret, vchPubKey, keyOut); + return m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptKey(encryption_key, vchCryptedSecret, vchPubKey, keyOut); + }); } return false; } @@ -1164,8 +1174,11 @@ bool LegacyScriptPubKeyMan::GetKey(const CKeyID &address, CKey& keyOut) const CHDChain hdChainCurrent; if (!GetHDChain(hdChainCurrent)) throw std::runtime_error(std::string(__func__) + ": GetHDChain failed"); - if (!DecryptHDChain(m_storage.GetEncryptionKey(), hdChainCurrent)) + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptHDChain(encryption_key, hdChainCurrent); + })) { throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed"); + } // make sure seed matches this chain if (hdChainCurrent.GetID() != hdChainCurrent.GetSeedHash()) throw std::runtime_error(std::string(__func__) + ": Wrong HD chain!"); @@ -1276,8 +1289,11 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata& throw std::runtime_error(std::string(__func__) + ": GetHDChain failed"); } - if (!DecryptHDChain(m_storage.GetEncryptionKey(), hdChainTmp)) + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptHDChain(encryption_key, hdChainTmp); + })) { throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed"); + } // make sure seed matches this chain if (hdChainTmp.GetID() != hdChainTmp.GetSeedHash()) throw std::runtime_error(std::string(__func__) + ": Wrong HD chain!"); @@ -1897,7 +1913,9 @@ std::map DescriptorScriptPubKeyMan::GetKeys() const const CPubKey& pubkey = key_pair.second.first; const std::vector& crypted_secret = key_pair.second.second; CKey key; - DecryptKey(m_storage.GetEncryptionKey(), crypted_secret, pubkey, key); + m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptKey(encryption_key, crypted_secret, pubkey, key); + }); keys[pubkey.GetID()] = key; } return keys; @@ -2010,7 +2028,9 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const std::vector crypted_secret; CKeyingMaterial secret(key.begin(), key.end()); - if (!EncryptSecret(m_storage.GetEncryptionKey(), secret, pubkey.GetHash(), crypted_secret)) { + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return EncryptSecret(encryption_key, secret, pubkey.GetHash(), crypted_secret); + })) { return false; } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 0d6450d671a7..05f2452d2fc8 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -21,6 +21,7 @@ #include +#include #include // Wallet storage things that ScriptPubKeyMans need in order to be able to store things to the wallet database. @@ -38,7 +39,8 @@ class WalletStorage virtual void UnsetBlankWalletFlag(WalletBatch&) = 0; virtual bool CanSupportFeature(enum WalletFeature) const = 0; virtual void SetMinVersion(enum WalletFeature, WalletBatch* = nullptr) = 0; - virtual const CKeyingMaterial& GetEncryptionKey() const = 0; + //! Pass the encryption key to cb(). + virtual bool WithEncryptionKey(std::function cb) const = 0; virtual bool HasEncryptionKeys() const = 0; virtual bool IsLocked(bool fForMixing) const = 0; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b6ac4595a810..00d872582a5f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -5743,9 +5743,10 @@ void CWallet::SetupLegacyScriptPubKeyMan() m_spk_managers[spk_manager->GetID()] = std::move(spk_manager); } -const CKeyingMaterial& CWallet::GetEncryptionKey() const +bool CWallet::WithEncryptionKey(std::function cb) const { - return vMasterKey; + LOCK(cs_wallet); + return cb(vMasterKey); } bool CWallet::HasEncryptionKeys() const diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 411367ac2bf1..72c4c747ba9a 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1468,7 +1468,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati //! Make a LegacyScriptPubKeyMan and set it for all types, internal, and external. void SetupLegacyScriptPubKeyMan(); - const CKeyingMaterial& GetEncryptionKey() const override; + bool WithEncryptionKey(std::function cb) const override; + bool HasEncryptionKeys() const override; /** Get last block processed height */ From 55114a682e52ecba0ee27c0cb6f739d09b3f62df Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 1 Sep 2020 08:18:20 +0200 Subject: [PATCH 08/10] Merge #19668: Do not hide compile-time thread safety warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ea74e10acf17903e44c85e3678853414653dd4e1 doc: Add best practice for annotating/asserting locks (Hennadii Stepanov) 2ee7743fe723227f2ea1b031eddb14fc6863f4c8 sync.h: Make runtime lock checks require compile-time lock checks (Anthony Towns) 23d71d171e6e22ba5e4a909d597a54595b2a2c1f Do not hide compile-time thread safety warnings (Hennadii Stepanov) 3ddc150857178bfb1c854c05bf9b526777876f56 Add missed thread safety annotations (Hennadii Stepanov) af9ea55a72c94678b343f5dd98dc78f3a3ac58cb Use LockAssertion utility class instead of AssertLockHeld() (Hennadii Stepanov) Pull request description: On the way of transit from `RecursiveMutex` to `Mutex` (see #19303) it is crucial to have run-time `AssertLockHeld()` assertion that does _not_ hide compile-time Clang Thread Safety Analysis warnings. On master (65e4ecabd5b4252154640c7bac38c92a3f3a7018) using `AssertLockHeld()` could hide Clang Thread Safety Analysis warnings, e.g., with the following patch applied: ```diff --- a/src/txmempool.h +++ b/src/txmempool.h @@ -607,7 +607,7 @@ public: void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs); - void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); + void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main); void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs); void removeForBlock(const std::vector& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs); ``` Clang compiles the code without any thread safety warnings. See "Add missed thread safety annotations" commit for the actual thread safety warnings that are fixed in this PR. ACKs for top commit: MarcoFalke: ACK ea74e10acf 🎙 jnewbery: ACK ea74e10acf17903e44c85e3678853414653dd4e1 ajtowns: ACK ea74e10acf17903e44c85e3678853414653dd4e1 Tree-SHA512: 8cba996e526751a1cb0e613c0cc1b10f027a3e9945fbfb4bd30f6355fd36b9f9c2e1e95ed3183fc254b42df7c30223278e18e5bdb5e1ef85db7fef067595d447 --- doc/developer-notes.md | 66 ++++++++++++++++++++++++++++++++++++ src/sync.cpp | 6 +++- src/sync.h | 10 +++--- src/wallet/scriptpubkeyman.h | 2 +- src/wallet/wallet.h | 4 +-- 5 files changed, 80 insertions(+), 8 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 248557919a1e..22785e734987 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -899,6 +899,72 @@ the upper cycle, etc. Threads and synchronization ---------------------------- +- Prefer `Mutex` type to `RecursiveMutex` one + +- Consistently use [Clang Thread Safety Analysis](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) annotations to + get compile-time warnings about potential race conditions in code. Combine annotations in function declarations with + run-time asserts in function definitions: + +```C++ +// txmempool.h +class CTxMemPool +{ +public: + ... + mutable RecursiveMutex cs; + ... + void UpdateTransactionsFromBlock(...) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, cs); + ... +} + +// txmempool.cpp +void CTxMemPool::UpdateTransactionsFromBlock(...) +{ + AssertLockHeld(::cs_main); + AssertLockHeld(cs); + ... +} +``` + +```C++ +// validation.h +class ChainstateManager +{ +public: + ... + bool ProcessNewBlock(...) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main); + ... +} + +// validation.cpp +bool ChainstateManager::ProcessNewBlock(...) +{ + AssertLockNotHeld(::cs_main); + ... + LOCK(::cs_main); + ... +} +``` + +- When Clang Thread Safety Analysis is unable to determine if a mutex is locked, use `LockAssertion` class instances: + +```C++ +// net_processing.h +void RelayTransaction(...) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + +// net_processing.cpp +void RelayTransaction(...) +{ + AssertLockHeld(::cs_main); + + connman.ForEachNode([&txid, &wtxid](CNode* pnode) { + LockAssertion lock(::cs_main); + ... + }); +} + +``` + - Build and run tests with `-DDEBUG_LOCKORDER` to verify that no potential deadlocks are introduced. diff --git a/src/sync.cpp b/src/sync.cpp index 35d3ffaf120d..754081926253 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -285,12 +285,16 @@ template void AssertLockHeldInternal(const char*, const char*, int, Mutex*); template void AssertLockHeldInternal(const char*, const char*, int, RecursiveMutex*); template void AssertLockHeldInternal(const char*, const char*, int, SharedMutex*); -void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) +template +void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) { if (!LockHeld(cs)) return; tfm::format(std::cerr, "Assertion failed: lock %s held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld()); abort(); } +template void AssertLockNotHeldInternal(const char*, const char*, int, Mutex*); +template void AssertLockNotHeldInternal(const char*, const char*, int, RecursiveMutex*); +template void AssertLockNotHeldInternal(const char*, const char*, int, SharedMutex*); void DeleteLock(void* cs) { diff --git a/src/sync.h b/src/sync.h index 3712bd9824e5..8d02830726d8 100644 --- a/src/sync.h +++ b/src/sync.h @@ -57,8 +57,9 @@ void LeaveCritical(); void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line); std::string LocksHeld(); template -void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) ASSERT_EXCLUSIVE_LOCK(cs); -void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs); +void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs); +template +void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs); void DeleteLock(void* cs); bool LockStackEmpty(); @@ -74,8 +75,9 @@ inline void EnterCritical(const char* pszName, const char* pszFile, int nLine, M inline void LeaveCritical() {} inline void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) {} template -inline void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) ASSERT_EXCLUSIVE_LOCK(cs) {} -inline void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {} +inline void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs) {} +template +void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) {} inline void DeleteLock(void* cs) {} inline bool LockStackEmpty() { return true; } #endif diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 05f2452d2fc8..e3f1916272c8 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -528,7 +528,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan //! keeps track of whether Unlock has run a thorough check before bool m_decryption_thoroughly_checked = false; - bool AddDescriptorKeyWithDB(WalletBatch& batch, const CKey& key, const CPubKey &pubkey); + bool AddDescriptorKeyWithDB(WalletBatch& batch, const CKey& key, const CPubKey &pubkey) EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); KeyMap GetKeys() const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 72c4c747ba9a..88ec3db2f12f 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1402,7 +1402,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati * Obviously holding cs_main/cs_wallet when going into this call may cause * deadlock */ - void BlockUntilSyncedToCurrentChain() const LOCKS_EXCLUDED(cs_main, cs_wallet); + void BlockUntilSyncedToCurrentChain() const EXCLUSIVE_LOCKS_REQUIRED(!::cs_main, !cs_wallet); /** set a single wallet flag */ void SetWalletFlag(uint64_t flags); @@ -1515,7 +1515,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati void DeactivateScriptPubKeyMan(uint256 id, bool internal); //! Create new DescriptorScriptPubKeyMans and add them to the wallet - void SetupDescriptorScriptPubKeyMans(); + void SetupDescriptorScriptPubKeyMans() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Return the DescriptorScriptPubKeyMan for a WalletDescriptor if it is already in the wallet DescriptorScriptPubKeyMan* GetDescriptorScriptPubKeyMan(const WalletDescriptor& desc) const; From d0a4198166d0670ce814213d77c39b77377cd373 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 6 Apr 2021 07:54:07 +0200 Subject: [PATCH 09/10] Merge #21598: refactor: Remove negative lock annotations from globals fa5eabe72117f6e3704858e8d5b2c57a120258ed refactor: Remove negative lock annotations from globals (MarcoFalke) Pull request description: They only make sense for mutexes that are private members. Until cs_main is a private member the negative annotations should be replaced by excluded annotations, which are optional. ACKs for top commit: sipa: utACK fa5eabe72117f6e3704858e8d5b2c57a120258ed ajtowns: ACK fa5eabe72117f6e3704858e8d5b2c57a120258ed hebasto: ACK fa5eabe72117f6e3704858e8d5b2c57a120258ed vasild: ACK fa5eabe72117f6e3704858e8d5b2c57a120258ed Tree-SHA512: 06f8a200304f81533010efcc42d9f59b8c4d0ae355920c0a28efb6fa161a3e3e68f2dfffb0c009afd9c2501e6a293c6e5a419a64d718f1f4e79668ab2ab1fcdc --- doc/developer-notes.md | 2 +- src/index/base.h | 2 +- src/net_processing.cpp | 2 +- src/sync.h | 4 ++-- src/txorphanage.h | 2 +- src/wallet/wallet.h | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 22785e734987..9e9fb162e9a9 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -932,7 +932,7 @@ class ChainstateManager { public: ... - bool ProcessNewBlock(...) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main); + bool ProcessNewBlock(...) LOCKS_EXCLUDED(::cs_main); ... } diff --git a/src/index/base.h b/src/index/base.h index 403c8c87b1b2..e1f2a9b82d9d 100644 --- a/src/index/base.h +++ b/src/index/base.h @@ -118,7 +118,7 @@ class BaseIndex : public CValidationInterface /// sync once and only needs to process blocks in the ValidationInterface /// queue. If the index is catching up from far behind, this method does /// not block and immediately returns false. - bool BlockUntilSyncedToCurrentChain() const; + bool BlockUntilSyncedToCurrentChain() const LOCKS_EXCLUDED(::cs_main); void Interrupt(); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 803f3d51aa73..688a01d19dc1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -944,7 +944,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) LOCKS_EXCLUDED(cs_main) EXCLUSIVE_LOCKS_REQUIRED(peer.m_getdata_requests_mutex); + void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(peer.m_getdata_requests_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/sync.h b/src/sync.h index 8d02830726d8..cab9d559d543 100644 --- a/src/sync.h +++ b/src/sync.h @@ -59,7 +59,7 @@ std::string LocksHeld(); template void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs); template -void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs); +void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) LOCKS_EXCLUDED(cs); void DeleteLock(void* cs); bool LockStackEmpty(); @@ -77,7 +77,7 @@ inline void CheckLastCritical(void* cs, std::string& lockname, const char* guard template inline void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs) {} template -void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) {} +void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) LOCKS_EXCLUDED(cs) {} inline void DeleteLock(void* cs) {} inline bool LockStackEmpty() { return true; } #endif diff --git a/src/txorphanage.h b/src/txorphanage.h index 6fc043eda607..d8ef7a5c44b9 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -41,7 +41,7 @@ class TxOrphanage { void EraseForPeer(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); /** Erase all orphans included in or invalidated by a new block */ - void EraseForBlock(const CBlock& block) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); + void EraseForBlock(const CBlock& block) LOCKS_EXCLUDED(::g_cs_orphans); /** Limit the orphanage to the given maximum */ unsigned int LimitOrphans(unsigned int max_orphans_size) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 88ec3db2f12f..af80efb535bc 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1402,7 +1402,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati * Obviously holding cs_main/cs_wallet when going into this call may cause * deadlock */ - void BlockUntilSyncedToCurrentChain() const EXCLUSIVE_LOCKS_REQUIRED(!::cs_main, !cs_wallet); + void BlockUntilSyncedToCurrentChain() const LOCKS_EXCLUDED(::cs_main) EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet); /** set a single wallet flag */ void SetWalletFlag(uint64_t flags); From 1db9e6ac76d895d5e80aba7bf0f0a42d13a904e6 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 23 Sep 2020 16:36:52 +0200 Subject: [PATCH 10/10] Merge #19979: Replace LockAssertion with AssertLockHeld, remove LockAssertion 0bd1184adf6610c0bd14f4e9a25c0a200e65ae25 Remove unused LockAssertion struct (Hennadii Stepanov) ab2a44297fd0796bf5797ae2a477e8e56d9c3c12 Replace LockAssertion with a proper thread safety annotations (Hennadii Stepanov) 73f71e19965e07534eb47701f2b23c9ed59ef475 refactor: Use explicit function type instead of template (Hennadii Stepanov) Pull request description: This PR replaces `LockAssertion` with `AssertLockHeld`, and removes `LockAssertion`. This PR is compared with alternatives in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/AssertLockHeld-PRs ACKs for top commit: MarcoFalke: ACK 0bd1184adf6610c0bd14f4e9a25c0a200e65ae25 ajtowns: ACK 0bd1184adf6610c0bd14f4e9a25c0a200e65ae25 vasild: ACK 0bd1184ad Tree-SHA512: ef7780dd689faf0bb479fdb97c49bc652e2dd10c148234bb95502dfbb676442d8565ee37864d923ca21a25f9dc2a335bf46ee82c095e387b59a664ab05c0ae41 --- doc/developer-notes.md | 19 ------------------- src/net.cpp | 8 ++++---- src/net.h | 12 ++++++------ src/net_processing.cpp | 12 ++++++------ src/sync.h | 14 -------------- 5 files changed, 16 insertions(+), 49 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 9e9fb162e9a9..162547f1dd97 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -946,25 +946,6 @@ bool ChainstateManager::ProcessNewBlock(...) } ``` -- When Clang Thread Safety Analysis is unable to determine if a mutex is locked, use `LockAssertion` class instances: - -```C++ -// net_processing.h -void RelayTransaction(...) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - -// net_processing.cpp -void RelayTransaction(...) -{ - AssertLockHeld(::cs_main); - - connman.ForEachNode([&txid, &wtxid](CNode* pnode) { - LockAssertion lock(::cs_main); - ... - }); -} - -``` - - Build and run tests with `-DDEBUG_LOCKORDER` to verify that no potential deadlocks are introduced. diff --git a/src/net.cpp b/src/net.cpp index 49e9ca2c82db..2b6fd4608bb4 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3510,8 +3510,8 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, MasternodeProbeConn isProbe = MasternodeProbeConn::IsNotConnection; - const auto getPendingQuorumNodes = [&]() { - LockAssertion lock(cs_vPendingMasternodes); + const auto getPendingQuorumNodes = [&]() EXCLUSIVE_LOCKS_REQUIRED(cs_vPendingMasternodes) { + AssertLockHeld(cs_vPendingMasternodes); std::vector ret; for (const auto& group : masternodeQuorumNodes) { for (const auto& proRegTxHash : group.second) { @@ -3549,8 +3549,8 @@ void CConnman::ThreadOpenMasternodeConnections(CDeterministicMNManager& dmnman, return ret; }; - const auto getPendingProbes = [&]() { - LockAssertion lock(cs_vPendingMasternodes); + const auto getPendingProbes = [&]() EXCLUSIVE_LOCKS_REQUIRED(cs_vPendingMasternodes) { + AssertLockHeld(cs_vPendingMasternodes); std::vector ret; for (auto it = masternodePendingProbes.begin(); it != masternodePendingProbes.end(); ) { auto dmn = mnList.GetMN(*it); diff --git a/src/net.h b/src/net.h index 248a8a6b0401..c34c87104a6c 100644 --- a/src/net.h +++ b/src/net.h @@ -1275,6 +1275,8 @@ friend class CNode; return ForNode(id, FullyConnectedOnly, func); } + using NodeFn = std::function; + bool IsConnected(const CService& addr, std::function cond) { return ForNode(addr, cond, [](CNode* pnode){ @@ -1331,10 +1333,9 @@ friend class CNode; } }; - template - void ForEachNode(Callable&& func) + void ForEachNode(const NodeFn& fn) { - ForEachNode(FullyConnectedOnly, func); + ForEachNode(FullyConnectedOnly, fn); } template @@ -1347,10 +1348,9 @@ friend class CNode; } }; - template - void ForEachNode(Callable&& func) const + void ForEachNode(const NodeFn& fn) const { - ForEachNode(FullyConnectedOnly, func); + ForEachNode(FullyConnectedOnly, fn); } template diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 688a01d19dc1..d05eb8917e1c 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2000,8 +2000,8 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha m_most_recent_compact_block = pcmpctblock; } - m_connman.ForEachNode([this, pindex, &lazy_ser, &hashBlock](CNode* pnode) { - LockAssertion lock(::cs_main); + m_connman.ForEachNode([this, pindex, &lazy_ser, &hashBlock](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); // TODO: Avoid the repeated-serialization here if (pnode->fDisconnect) return; @@ -5275,8 +5275,8 @@ void PeerManagerImpl::EvictExtraOutboundPeers(std::chrono::seconds now) // We want to prevent recently connected to Onion peers from being disconnected here, protect them as long as // there are more non_onion nodes than onion nodes so far size_t onion_count = 0; - m_connman.ForEachNode([&](CNode* pnode) { - LockAssertion lock(::cs_main); + m_connman.ForEachNode([&](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); if (pnode->addr.IsTor() && ++onion_count <= m_connman.GetMaxOutboundOnionNodeCount()) return; // Don't disconnect masternodes just because they were slow in block announcement if (pnode->m_masternode_connection) return; @@ -5293,8 +5293,8 @@ void PeerManagerImpl::EvictExtraOutboundPeers(std::chrono::seconds now) } }); if (worst_peer != -1) { - bool disconnected = m_connman.ForNode(worst_peer, [&](CNode *pnode) { - LockAssertion lock(::cs_main); + bool disconnected = m_connman.ForNode(worst_peer, [&](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); // Only disconnect a peer that has been connected to us for // some reasonable fraction of our check-frequency, to give diff --git a/src/sync.h b/src/sync.h index cab9d559d543..078ee4f6e8a5 100644 --- a/src/sync.h +++ b/src/sync.h @@ -452,18 +452,4 @@ class CSemaphoreGrant } }; -// Utility class for indicating to compiler thread analysis that a mutex is -// locked (when it couldn't be determined otherwise). -struct SCOPED_LOCKABLE LockAssertion -{ - template - explicit LockAssertion(Mutex& mutex) EXCLUSIVE_LOCK_FUNCTION(mutex) - { -#ifdef DEBUG_LOCKORDER - AssertLockHeld(mutex); -#endif - } - ~LockAssertion() UNLOCK_FUNCTION() {} -}; - #endif // BITCOIN_SYNC_H