[Bundle 3/n] Prune remaining g_chainman usage in validation functions #21055

In commit "validation: Move LoadBlockIndexDB to CChainState" (fa730f7a871c49072e2e899cefbc8275e5f67057)

It seems chainman variable wasn't really needed here, just the chainman.m_blockman member, so this might make more sense as a BlockManager method instead of CChainState method

Unfortunately we need setBlockIndexCandidates which is a member off CChainState... If you think LoadBlockIndexDB makes more sense logically as a method of BlockManager rather than CChainState we can pass in the setBlockIndexCandidates. Let me know!

re: #21055 (review)

Unfortunately we need setBlockIndexCandidates which is a member off CChainState... If you think LoadBlockIndexDB makes more sense logically as a method of BlockManager rather than CChainState we can pass in the setBlockIndexCandidates. Let me know!

Thanks, I didn't recognize the setBlockIndexCandidates access. Proposed change sounds reasonable, but I don't actually know too much about it and it would seem good to tackle as a followup not changing here.

In commit "validation: Pass in spendheight to CTxMemPool::check" (a765747415aebd432d5feffb6cb609be62e2592b)

I think it would be nice to delete the BlockManager::GetSpendHeight method entirely instead of adding new calls to it. It's basically a one-line function returning height of active_coins_tip + 1, and it would seem more straightforward to pass active_coins_tip and active_coins_tip height together as arguments to the mempool check method (assuming the check method can't access the height directly) instead of the tip and an offset height.

Not entirely sure what you mean here by an "offset height"... Looking at all the uses of spendheight in CTxMemPool::check it seems that this "offset height" is exactly what CheckInputsAndUpdateCoins wants?

I agree with @ryanofsky that the use of BlockManager::GetSpendHeight() is quite ugly. We can get the height directly from ::ChainstateActive().m_chain.Height() + 1.

I also think it'd be preferable to not have net_processing reach inside validation for the various active chainstate fields, just to call a check() function (which defaults to a no-op with default -mempoolcheck). Could we perhaps move this call to CTxMemPool.check() into AcceptToMemoryPool(), so that it's done consistently for all callers, and net_processing doesn't need to concern itself with validation internals?

I'm not sure I can easily convince myself that moving this check into ATMP is completely safe. However, I do see how this is ugly.

Do you think the following patch would be an acceptable solution?

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 614a3e0791..022f2e0574 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2301,10 +2301,7 @@ void PeerManagerImpl::ProcessOrphanTx(std::set<uint256>& orphan_work_set)
             break;
         }
     }
-    CChainState& active_chainstate = m_chainman.ActiveChainstate();
-    CCoinsViewCache& active_coins_tip = active_chainstate.CoinsTip();
-    assert(std::addressof(::ChainstateActive().CoinsTip()) == std::addressof(active_coins_tip));
-    m_mempool.check(&active_coins_tip, active_chainstate.m_blockman.GetSpendHeight(active_coins_tip));
+    m_mempool.check(m_chainman.ActiveChainstate());
 }
 
 /**
@@ -3265,10 +3262,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         const TxValidationState& state = result.m_state;
 
         if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) {
-            CChainState& active_chainstate = m_chainman.ActiveChainstate();
-            CCoinsViewCache& active_coins_tip = active_chainstate.CoinsTip();
-            assert(std::addressof(::ChainstateActive().CoinsTip()) == std::addressof(active_coins_tip));
-            m_mempool.check(&active_coins_tip, active_chainstate.m_blockman.GetSpendHeight(active_coins_tip));
+            m_mempool.check(m_chainman.ActiveChainstate());
 
             // As this version of the transaction was acceptable, we can forget about any
             // requests for it.
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 4df7017c47..05902a55c3 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -619,7 +619,7 @@ static void CheckInputsAndUpdateCoins(const CTransaction& tx, CCoinsViewCache& m
     UpdateCoins(tx, mempoolDuplicate, std::numeric_limits<int>::max());
 }
 
-void CTxMemPool::check(const CCoinsViewCache *pcoins, const int64_t spendheight) const
+void CTxMemPool::check(CChainState& active_chainstate) const
 {
     if (m_check_ratio == 0) return;
 
@@ -633,7 +633,10 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins, const int64_t spendheight)
     CAmount check_total_fee{0};
     uint64_t innerUsage = 0;
 
-    CCoinsViewCache mempoolDuplicate(const_cast<CCoinsViewCache*>(pcoins));
+    CCoinsViewCache& active_coins_tip = active_chainstate.CoinsTip();
+    assert(std::addressof(::ChainstateActive().CoinsTip()) == std::addressof(active_coins_tip)); // TODO: REVIEW-ONLY, REMOVE IN FUTURE COMMIT
+    CCoinsViewCache mempoolDuplicate(const_cast<CCoinsViewCache*>(&active_coins_tip));
+    const int64_t spendheight = active_chainstate.m_chain.Height() + 1;
     assert(g_chainman.m_blockman.GetSpendHeight(mempoolDuplicate) == spendheight); // TODO: REVIEW-ONLY, REMOVE IN FUTURE COMMIT
 
     std::list<const CTxMemPoolEntry*> waitingOnDependants;
@@ -655,7 +658,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins, const int64_t spendheight)
                 fDependsWait = true;
                 setParentCheck.insert(*it2);
             } else {
-                assert(pcoins->HaveCoin(txin.prevout));
+                assert(active_coins_tip.HaveCoin(txin.prevout));
             }
             // Check whether its inputs are marked in mapNextTx.
             auto it3 = mapNextTx.find(txin.prevout);
diff --git a/src/txmempool.h b/src/txmempool.h
index dd82d61fc6..001d856e43 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -604,7 +604,7 @@ public:
      * all inputs are in the mapNextTx array). If sanity-checking is turned off,
      * check does nothing.
      */
-    void check(const CCoinsViewCache *pcoins, const int64_t spendheight) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+    void check(CChainState& active_chainstate) 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
diff --git a/src/validation.cpp b/src/validation.cpp
index 325edc5b6a..ac79dce52e 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2801,8 +2801,7 @@ bool CChainState::ActivateBestChainStep(BlockValidationState& state, const CChai
         UpdateMempoolForReorg(*this, m_mempool, disconnectpool, true);
     }
 
-    CCoinsViewCache& active_coins_tip = CoinsTip();
-    m_mempool.check(&active_coins_tip, m_blockman.GetSpendHeight(active_coins_tip));
+    m_mempool.check(*this);
 
     CheckForkWarningConditions();
 

I'm not sure I can easily convince myself that moving this check into ATMP is completely safe.

I think you're right to be wary. The call to ATMP from UpdateMempoolForReorg probably can't call check().

I like your proposed change.

Fixed in 4744efc9ba

This global was introduced by d23f6c6. At that commit, the globalVersionBitsTipState's unique callerBIP9SoftForkDesc didn't require cs_main locking.

Those methods were refactored by 3862e47, where the new BIP9SoftForkDescPushBack did require the lock. I think since then this lock has been useless and could have been substituted by an annotation.

So not a concern to get rid of them.

Shouldn't an assert be added here and before the following invocations ? The CCoinsViewCache pointer is used throughout `CTxMemPool::check.

Fixed in: 52c29d9aedcf8d141b75a6d923a5d5bea53f4079

Don't split { into a new line.

Fixed in 4bada76237

Perhaps change this function to be annotated with EXCLUSIVE_LOCKS_REQUIRED(cs_main) and assert that cs_main is held, rather than locking cs_main in the first line of the function.

This function is always called with cs_main held. Without that guarantee, there's the possibility that one thread calls it with ::ChainstateActive(), another thread updates the active chainstate, and then this thread asserts on the std::addressof check.

Fixed in ef83bb8550

Perhaps also add AssertLockHeld(cs_main) to the top of the function body?

Fixed in e11b649650

This function no longer exists. Remove the declaration.

Fixed in: 8b99efbcc0

Remove EXCLUSIVE_LOCKS_REQUIRED(cs_main) (which is now on the declaration in validation.h)

Fixed in 8cdb2f7e58