locks: Annotate CTxMemPool::check to require cs_main

dongcarl · dongcarl · commit b3964670537d · 2021-01-20T16:15:03.000-05:00
Currently, CTxMemPool::check locks CTxMemPool's own cs member, then calls GetSpendHeight which locks cs_main. This can potentially cause an undesirable lock invesion since CTxMemPool's cs is supposed to be locked after cs_main. This does not cause us any problems right now because all callers of CTxMemPool already lock cs_main before calling CTxMemPool::check, which means that the LOCK(cs_main) in GetSpendHeight becomes benign. However, it is currently possible for new code to be added which calls CTxMemPool::check without locking cs_main (which would be dangerous). Therefore we should make it explicit that cs_main needs to be held before calling CTxMemPool::check. NOTE: After all review-only assertions are removed in "bitcoin#20158 | tree-wide: De-globalize ChainstateManager", and assuming that we keep the changes in "validation: Pass in spendheight to CTxMemPool::check", we can re-evaluate to see if this annotation is still necessary.
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
@@ -618,6 +618,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
 
     if (GetRand(m_check_ratio) >= 1) return;
 
+    AssertLockHeld(::cs_main);
     LOCK(cs);
     LogPrint(BCLog::MEMPOOL, "Checking mempool with %u transactions and %u inputs\n", (unsigned int)mapTx.size(), (unsigned int)mapNextTx.size());
 
diff --git a/src/txmempool.h b/src/txmempool.h
@@ -602,7 +602,7 @@ class CTxMemPool
      * all inputs are in the mapNextTx array). If sanity-checking is turned off,
      * check does nothing.
      */
-    void check(const CCoinsViewCache *pcoins) const;
+    void check(const CCoinsViewCache *pcoins) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
     // addUnchecked must updated state for all ancestors of a given transaction,
     // to track size/count of descendant transactions.  First version of
