backport: bitcoin#21202, bitcoin#24931 and multiple thread safety asserion fixes

doc/developer-notes.md

@@ -947,10 +947,20 @@ 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
@@ -975,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<const CBlock> 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<const CBlock>());
 }
 ```
 

src/coinjoin/client.h

@@ -95,16 +95,16 @@ class CoinJoinWalletManager {
         }
     }
 
-    void Add(const std::shared_ptr<wallet::CWallet>& wallet);
-    void DoMaintenance(CConnman& connman);
+    void Add(const std::shared_ptr<wallet::CWallet>& 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 <typename Callable>
-    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 <typename Callable>
-    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();
 };
 

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

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<CCreditPool> GetFromCache(const CBlockIndex& block_index);
-    void AddToCache(const uint256& block_hash, int height, const CCreditPool& pool);
+    std::optional<CCreditPool> 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<const CBlockIndex*> block_index, CCreditPool prev,
-                                    const Consensus::Params& consensusParams);
+                                    const Consensus::Params& consensusParams) EXCLUSIVE_LOCKS_REQUIRED(!cache_mutex);
 };
 
 std::optional<CCreditPoolDiff> GetCreditPoolDiffForBlock(CCreditPoolManager& cpoolman, const node::BlockManager& blockman, const llmq::CQuorumManager& qman,

src/evo/deterministicmns.h

@@ -162,7 +162,7 @@ class CDeterministicMNList
     mutable std::shared_ptr<const CSimplifiedMNList> 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 <typename Stream>
-    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<std::shared_ptr<const CSimplifiedMNList>> to_sml() const;
+    gsl::not_null<std::shared_ptr<const CSimplifiedMNList>> 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,28 +394,32 @@ 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;
     /**
      * Apply Diff modifies current object.
      * 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<const CBlockIndex*> pindex, const CDeterministicMNListDiff& diff);
-
-    void AddMN(const CDeterministicMNCPtr& dmn, bool fBumpTotalCount = true);
-    void UpdateMN(const CDeterministicMN& oldDmn, const std::shared_ptr<const CDeterministicMNState>& pdmnState);
-    void UpdateMN(const uint256& proTxHash, const std::shared_ptr<const CDeterministicMNState>& pdmnState);
-    void UpdateMN(const CDeterministicMN& oldDmn, const CDeterministicMNStateDiff& stateDiff);
-    void RemoveMN(const uint256& proTxHash);
+    void ApplyDiff(gsl::not_null<const CBlockIndex*> 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<const CDeterministicMNState>& pdmnState)
+        EXCLUSIVE_LOCKS_REQUIRED(!m_cached_sml_mutex);
+    void UpdateMN(const uint256& proTxHash, const std::shared_ptr<const CDeterministicMNState>& 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 <typename T>
     [[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);

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<Signals> ProcessBlock(const CBlock& block, const CBlockIndex* const pindex, bool fJustCheck, BlockValidationState& state);
+    std::optional<Signals> 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,24 +146,24 @@ 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.
      * if the signals for previous block is not available in cache it would read blocks from disk
      * 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<Signals> GetFromCache(const CBlockIndex* const pindex);
+    std::optional<Signals> GetFromCache(const CBlockIndex* const pindex) EXCLUSIVE_LOCKS_REQUIRED(!cs_cache);
 };
 
 std::optional<uint8_t> extractEHFSignal(const CTransaction& tx);

src/governance/governance.cpp

@@ -718,7 +718,7 @@ std::optional<const CGovernanceObject> 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);
 

src/httpserver.cpp

@@ -159,7 +159,7 @@ static std::unique_ptr<WorkQueue<HTTPClosure>> g_work_queue{nullptr};
 //! List of 'external' RPC users (global variable, used by httprpc)
 std::vector<std::string> g_external_usernames;
 //! Handlers for (sub)paths
-static Mutex g_httppathhandlers_mutex;
+static GlobalMutex g_httppathhandlers_mutex;
 static std::vector<HTTPPathHandler> pathHandlers GUARDED_BY(g_httppathhandlers_mutex);
 //! Bound listening sockets
 static std::vector<evhttp_bound_socket *> boundSockets;

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)

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<const CBlock>& 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<const CBlock>& pblock, const CBlockIndex* pindexDisconnected);
 
     bool AlreadyHave(const CInv& inv) const EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks);