Add a checkfor the previous invalid comparison of invalid_walk_tip

TheBlueMatt · TheBlueMatt · commit 0ae728197ca5 · 2019-09-11T17:09:29.000-04:00
This adds a new test in CheckBlockIndex which tests that any
descendants of a block marked BLOCK_FAILED_VALID will not be marked
BLOCK_FAILED_VALID as they should never have been connected in the
first place.

Note that this check requires further changes in InvalidateBlock as
if you iteratively call InvalidateBlock to walk the chain backwards
(instead of calling InvalidateBlock on an old block and allowing it
to do the walking), the later-in-the-chain blocks will violate this.
diff --git a/src/validation.cpp b/src/validation.cpp
@@ -2792,6 +2792,8 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c
     // work as we go.
     std::multimap<const arith_uint256, CBlockIndex *> candidate_blocks_by_work;
 
+    std::set<CBlockIndex *> failed_valid_descendants;
+
     {
         LOCK(cs_main);
         for (const auto& entry : m_blockman.m_block_index) {
@@ -2801,11 +2803,12 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c
             // as we disconnect.
             // Instead, consider only non-active-chain blocks that have at
             // least as much work as where we expect the new tip to end up.
-            if (!m_chain.Contains(candidate) &&
-                    !CBlockIndexWorkComparator()(candidate, pindex->pprev) &&
-                    candidate->IsValid(BLOCK_VALID_TRANSACTIONS) &&
-                    candidate->HaveTxsDownloaded()) {
-                candidate_blocks_by_work.insert(std::make_pair(candidate->nChainWork, candidate));
+            if (!m_chain.Contains(candidate) && !CBlockIndexWorkComparator()(candidate, pindex->pprev)) {
+                if (candidate->IsValid(BLOCK_VALID_TRANSACTIONS) && candidate->HaveTxsDownloaded()) {
+                    candidate_blocks_by_work.insert(std::make_pair(candidate->nChainWork, candidate));
+                } else if ((candidate->nStatus & BLOCK_FAILED_VALID) && candidate->GetAncestor(pindex->nHeight) == pindex) {
+                    failed_valid_descendants.insert(candidate);
+                }
             }
         }
     }
@@ -2863,6 +2866,15 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c
             }
         }
 
+        for (auto candidate_descendant_it = failed_valid_descendants.begin(); candidate_descendant_it != failed_valid_descendants.end(); ) {
+            if ((*candidate_descendant_it)->nHeight > invalid_walk_tip->nHeight && (*candidate_descendant_it)->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip) {
+                (*candidate_descendant_it)->nStatus = ((*candidate_descendant_it)->nStatus ^ BLOCK_FAILED_VALID) | BLOCK_FAILED_CHILD;
+                candidate_descendant_it = failed_valid_descendants.erase(candidate_descendant_it);
+            } else {
+                ++candidate_descendant_it;
+            }
+        }
+
         // Track the last disconnected block, so we can correct its BLOCK_FAILED_CHILD status in future
         // iterations, or, if it's the last one, call InvalidChainFound on it.
         to_mark_failed = invalid_walk_tip;
@@ -4652,6 +4664,11 @@ void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams)
         if (pindexFirstInvalid == nullptr) {
             // Checks for not-invalid blocks.
             assert((pindex->nStatus & BLOCK_FAILED_MASK) == 0); // The failed mask cannot be set for blocks without invalid parents.
+        } else if (pindexFirstInvalid != pindex) {
+            // If a parent is invalid, we may be BLOCK_FAILED_CHILD (though no guarantee thereof),
+            // but we must not be BLOCK_FAILED_VALID (as we shouldn't have attempted to connect
+            // this block in the first place).
+            assert((pindex->nStatus & BLOCK_FAILED_VALID) == 0);
         }
         if (!CBlockIndexWorkComparator()(pindex, m_chain.Tip()) && pindexFirstNeverProcessed == nullptr) {
             if (pindexFirstInvalid == nullptr) {
