Merge #6319: backport: bitcoin#19668, bitcoin#21598, bitcoin#19979, b…

…itcoin#28774 - lock annotations

1db9e6a Merge bitcoin#19979: Replace LockAssertion with AssertLockHeld, remove LockAssertion (MarcoFalke)
d0a4198 Merge bitcoin#21598: refactor: Remove negative lock annotations from globals (MarcoFalke)
55114a6 Merge bitcoin#19668: Do not hide compile-time thread safety warnings (MarcoFalke)
898282d Merge bitcoin#28774: wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it (Ava Chow)
6d45284 fix: add missing lock annotation for ContextualCheckBlock and related TODO (Konstantin Akimov)
846ebab fix: fixes for orphange's locks and cs_main (Konstantin Akimov)
002580c fix: add ignoring safety-annotation for llmq/signing_shares due to template lambdas (Konstantin Akimov)
502d6ae fix: add multiple missing annotation about locks for dash specific code (Konstantin Akimov)
a219a8b partial Merge bitcoin#29040: refactor: Remove pre-C++20 code, fs::path cleanup (Konstantin Akimov)
042e8a4 fix: drop unneded lock assert in net.cpp for m_nodes_mutex (Konstantin Akimov)

Pull request description:

  ## What was done?
  These 4 backports improve noticeable implementation of thread-safety analysis by moving many possible warnings to compilation level.
  There's a lot of fixes for annotations after that!

  ## How Has This Been Tested?
  Build with clang:
  ```
  CC=clang CXX=clang++ ./configure --prefix=$(pwd)/depends/x86_64-pc-linux-gnu --enable-suppress-external-warnings   --enable-debug  --enable-stacktraces --enable-werror --enable-crash-hooks --enable-maintainer-mode
  ```

  ## Breaking Changes
  n/a

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK 1db9e6a
  PastaPastaPasta:
    utACK 1db9e6a

Tree-SHA512: 42a73e423f311689babc366b58a92f024556cf63af93357cfa0715c471efc1491d02ed9e7ba19b23efcdf423ab5bd051a24da2ad64c32e4b6db132a05f6669c1

doc/developer-notes.md

@@ -899,6 +899,53 @@ 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(...) LOCKS_EXCLUDED(::cs_main);
+    ...
+}
+
+// validation.cpp
+bool ChainstateManager::ProcessNewBlock(...)
+{
+    AssertLockNotHeld(::cs_main);
+    ...
+    LOCK(::cs_main);
+    ...
+}
+```
+
 - Build and run tests with `-DDEBUG_LOCKORDER` to verify that no potential
   deadlocks are introduced.
 

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;
     }
 

src/coinjoin/client.h

@@ -161,21 +161,26 @@ 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);
 
     /// step 0: select denominated inputs and txouts
     bool SelectDenominate(std::string& strErrorRet, std::vector<CTxDSIn>& vecTxDSInRet);
     /// step 1: prepare denominated inputs and outputs
-    bool PrepareDenominate(int nMinRounds, int nMaxRounds, std::string& strErrorRet, const std::vector<CTxDSIn>& vecTxDSIn, std::vector<std::pair<CTxDSIn, CTxOut> >& vecPSInOutPairsRet, bool fDryRun = false);
+    bool PrepareDenominate(int nMinRounds, int nMaxRounds, std::string& strErrorRet, const std::vector<CTxDSIn>& vecTxDSIn,
+                           std::vector<std::pair<CTxDSIn, CTxOut>>& 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<std::pair<CTxDSIn, CTxOut> >& vecPSInOutPairsIn, CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin);
 

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<int> nHeight);
-
+    void UpdateDSTXConfirmedHeight(const CTransactionRef& tx, std::optional<int> nHeight)
+        EXCLUSIVE_LOCKS_REQUIRED(cs_mapdstx);
 };
 
 bool ATMPIfSaneFee(ChainstateManager& chainman, const CTransactionRef& tx, bool test_accept = false)

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);
             }
         }

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

src/evo/specialtxman.cpp

@@ -19,9 +19,10 @@
 #include <primitives/block.h>
 #include <validation.h>
 
-static bool CheckSpecialTxInner(CDeterministicMNManager& dmnman, const ChainstateManager& chainman, const llmq::CQuorumManager& qman, const CTransaction& tx,
-                                const CBlockIndex* pindexPrev, const CCoinsViewCache& view, const std::optional<CRangesSet>& 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<CRangesSet>& indexes, bool check_sigs,
+                                TxValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
 {
     AssertLockHeld(cs_main);
 

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<std::shared_ptr<CSuperblock>> GetActiveTriggers() const;
-    bool AddNewTrigger(uint256 nHash);
-    void CleanAndRemoveTriggers();
+    std::vector<std::shared_ptr<CSuperblock>> 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<const CSuperblock> CreateSuperblockCandidate(int nHeight) const;
     std::optional<const CGovernanceObject> CreateGovernanceTrigger(const std::optional<const CSuperblock>& sb_opt, PeerManager& peerman,

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);
 

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();
 

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<std::pair<NodeId, uint256>, 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<NodeId, st
 
     std::unordered_map<std::pair<Consensus::LLMQType, uint256>, std::unordered_set<NodeId>, 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<NodeId, std::unordered_map<uint256, CSigSharesInv, StaticSaltedHasher>> sigSharesToAnnounce;
     std::unordered_map<NodeId, std::vector<CSigSesAnn>> 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);
     });

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<const CBlockIndex*> baseBlockIndexes, const CBlockIndex* blockIndex);
 
 class CQuorumSnapshotManager

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; }
 

src/net.cpp

@@ -647,8 +647,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) {
@@ -3587,8 +3585,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<CDeterministicMNCPtr> ret;
             for (const auto& group : masternodeQuorumNodes) {
                 for (const auto& proRegTxHash : group.second) {
@@ -3626,8 +3624,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<CDeterministicMNCPtr> ret;
             for (auto it = masternodePendingProbes.begin(); it != masternodePendingProbes.end(); ) {
                 auto dmn = mnList.GetMN(*it);

src/net.h

@@ -1316,6 +1316,8 @@ friend class CNode;
         return ForNode(id, FullyConnectedOnly, func);
     }
 
+    using NodeFn = std::function<void(CNode*)>;
+
     bool IsConnected(const CService& addr, std::function<bool(const CNode* pnode)> cond)
     {
         return ForNode(addr, cond, [](CNode* pnode){
@@ -1372,10 +1374,9 @@ friend class CNode;
         }
     };
 
-    template<typename Callable>
-    void ForEachNode(Callable&& func)
+    void ForEachNode(const NodeFn& fn)
     {
-        ForEachNode(FullyConnectedOnly, func);
+        ForEachNode(FullyConnectedOnly, fn);
     }
 
     template<typename Condition, typename Callable>
@@ -1388,10 +1389,9 @@ friend class CNode;
         }
     };
 
-    template<typename Callable>
-    void ForEachNode(Callable&& func) const
+    void ForEachNode(const NodeFn& fn) const
     {
-        ForEachNode(FullyConnectedOnly, func);
+        ForEachNode(FullyConnectedOnly, fn);
     }
 
     template<typename Condition, typename Callable, typename CallableAfter>