Merge bitcoin/bitcoin#29299: validation: fix misleading checkblockindex comments

9819db4ccaa03519a78d4d9ecce9f89f5be669e5 validation: move nChainTx assert down in CheckBlockIndex (Martin Zumsande)
033477dba65151b1863214606d5a49686ab6f559 doc: fix checkblockindex comments (Martin Zumsande)

Pull request description:

  The two assumptions there were described as test-only, which has led to confusion whether they should exist.
  However, they are necessary in general, as the changed comment explains - without them, the check would fail everywhere where it is enabled.
  The second commit moves this assert down to the other checks.

  Closes #29261

ACKs for top commit:
  maflcko:
    ACK 9819db4ccaa03519a78d4d9ecce9f89f5be669e5 🌦
  naumenkogs:
    ACK 9819db4ccaa03519a78d4d9ecce9f89f5be669e5
  ryanofsky:
    Code review ACK 9819db4ccaa03519a78d4d9ecce9f89f5be669e5. Thanks for figuring this issue out and fixing it. Would suggest changing pr name from "improve comments" to "fix misleading comments" since previous comments were wrong about the reasons the conditions are needed.

Tree-SHA512: 3f77791253eb0c97f8153dd8ae1c567f43f6387ea7a53efea94817463c672a4e11d548aa7eff62235346ff0713ff4d6fe08f9ec50d0c30a1e6b6d27b9918b419

1 files changed, 9 insertions, 10 deletions

diff --git a/src/validation.cpp b/src/validation.cpp
index 8c583c586c..4dc3a0f512 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4862,16 +4862,6 @@ void ChainstateManager::CheckBlockIndex()
     CBlockIndex* pindexFirstAssumeValid = nullptr; // Oldest ancestor of pindex which has BLOCK_ASSUMED_VALID
     while (pindex != nullptr) {
         nNodes++;
-        // Make sure nChainTx sum is correctly computed.
-        unsigned int prev_chain_tx = pindex->pprev ? pindex->pprev->nChainTx : 0;
-        assert((pindex->nChainTx == pindex->nTx + prev_chain_tx)
-               // For testing, allow transaction counts to be completely unset.
-               || (pindex->nChainTx == 0 && pindex->nTx == 0)
-               // For testing, allow this nChainTx to be unset if previous is also unset.
-               || (pindex->nChainTx == 0 && prev_chain_tx == 0 && pindex->pprev)
-               // Transaction counts prior to snapshot are unknown.
-               || pindex->IsAssumedValid());
-
         if (pindexFirstAssumeValid == nullptr && pindex->nStatus & BLOCK_ASSUMED_VALID) pindexFirstAssumeValid = pindex;
         if (pindexFirstInvalid == nullptr && pindex->nStatus & BLOCK_FAILED_VALID) pindexFirstInvalid = pindex;
         if (pindexFirstMissing == nullptr && !(pindex->nStatus & BLOCK_HAVE_DATA)) {
@@ -4954,6 +4944,15 @@ void ChainstateManager::CheckBlockIndex()
             // Checks for not-invalid blocks.
             assert((pindex->nStatus & BLOCK_FAILED_MASK) == 0); // The failed mask cannot be set for blocks without invalid parents.
         }
+        // Make sure nChainTx sum is correctly computed.
+        unsigned int prev_chain_tx = pindex->pprev ? pindex->pprev->nChainTx : 0;
+        assert((pindex->nChainTx == pindex->nTx + prev_chain_tx)
+               // Transaction may be completely unset - happens if only the header was accepted but the block hasn't been processed.
+               || (pindex->nChainTx == 0 && pindex->nTx == 0)
+               // nChainTx may be unset, but nTx set (if a block has been accepted, but one of its predecessors hasn't been processed yet)
+               || (pindex->nChainTx == 0 && prev_chain_tx == 0 && pindex->pprev)
+               // Transaction counts prior to snapshot are unknown.
+               || pindex->IsAssumedValid());
         // Chainstate-specific checks on setBlockIndexCandidates
         for (auto c : GetAll()) {
             if (c->m_chain.Tip() == nullptr) continue;