Merge cfadc8038c into b510893d00

Add a test for block index transitioning from legacy
BLOCK_FAILED_CHILD to BLOCK_FAILED_VALID behavior.

In the scenario where a valid block has a BLOCK_FAILED_CHILD
parent and a BLOCK_FAILED_VALID grandparent, ensure that all
three blocks are correctly marked as BLOCK_FAILED_VALID
after reloading the block index.

src/test/blockchain_tests.cpp

@@ -124,6 +124,11 @@ BOOST_FIXTURE_TEST_CASE(invalidate_block, TestChain100Setup)
     // Check BlockStatus when doing InvalidateBlock()
     BlockValidationState state;
     auto* orig_tip = active.Tip();
+
+    CBlockIndex* block_100 = active[100];
+    CBlockIndex* block_99 = active[99];
+    CBlockIndex* block_98 = active[98];
+
     int height_to_invalidate = orig_tip->nHeight - 10;
     auto* tip_to_invalidate = active[height_to_invalidate];
     m_node.chainman->ActiveChainstate().InvalidateBlock(state, tip_to_invalidate);
@@ -145,6 +150,26 @@ BOOST_FIXTURE_TEST_CASE(invalidate_block, TestChain100Setup)
         WITH_LOCK(::cs_main, assert(pindex->nStatus & BLOCK_FAILED_VALID));
         pindex = pindex->pprev;
     }
+
+    {
+        // consider the chain of blocks block_98 <- block_99 <- block_100
+        // intentionally mark:
+        //   - block_98: BLOCK_FAILED_VALID (already marked as BLOCK_FAILED_VALID from previous test)
+        //   - block_99: BLOCK_FAILED_CHILD (clear BLOCK_FAILED_VALID from previous test and mark as BLOCK_FAILED_CHILD)
+        //   - block_100: not invalid (clear BLOCK_FAILED_VALID from previous test)
+        LOCK(::cs_main);
+        assert(block_98->nStatus & BLOCK_FAILED_VALID);
+        block_99->nStatus = (block_99->nStatus & ~BLOCK_FAILED_VALID) | BLOCK_FAILED_CHILD;
+        block_100->nStatus = (block_100->nStatus & ~BLOCK_FAILED_VALID);
+
+        // Reload block index to recompute block status validity flags from disk.
+        m_node.chainman->LoadBlockIndex();
+
+        // check block_98, block_99, block_100 is marked as BLOCK_FAILED_VALID after reloading from disk
+        assert(block_98->nStatus & BLOCK_FAILED_VALID);
+        assert(block_99->nStatus & BLOCK_FAILED_VALID);
+        assert(block_100->nStatus & BLOCK_FAILED_VALID);
+    }
 }
 
 BOOST_AUTO_TEST_SUITE_END()

src/validation.cpp

@@ -3619,10 +3619,6 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
     assert(pindex);
     if (pindex->nHeight == 0) return false;
 
-    CBlockIndex* invalid_walk_tip = pindex;
-    bool pindex_was_in_chain = false;
-    int disconnected = 0;
-
     // We do not allow ActivateBestChain() to run while InvalidateBlock() is
     // running, as that could cause the tip to change while we disconnect
     // blocks.
@@ -3654,6 +3650,9 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
         }
     }
 
+    bool pindex_was_in_chain = false;
+    int disconnected = 0;
+
     // Disconnect (descendants of) pindex, and mark them invalid.
     while (true) {
         if (m_chainman.m_interrupt) break;
@@ -3667,8 +3666,8 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
         LOCK(MempoolMutex());
         if (!m_chain.Contains(pindex)) break;
         pindex_was_in_chain = true;
-        invalid_walk_tip = m_chain.Tip();
 
+        CBlockIndex* disconnected_tip{m_chain.Tip()};
         // ActivateBestChain considers blocks already in m_chain
         // unconditionally valid already, so force disconnect away from it.
         DisconnectedBlockTransactions disconnectpool{MAX_DISCONNECTED_TX_POOL_BYTES};
@@ -3680,32 +3679,33 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
         // keeping the mempool up to date is probably futile anyway).
         MaybeUpdateMempoolForReorg(disconnectpool, /* fAddToMempool = */ (++disconnected <= 10) && ret);
         if (!ret) return false;
-        assert(invalid_walk_tip->pprev == m_chain.Tip());
+        CBlockIndex* new_tip{m_chain.Tip()};
+        assert(disconnected_tip->pprev == new_tip);
 
         // We immediately mark the disconnected blocks as invalid.
         // This prevents a case where pruned nodes may fail to invalidateblock
         // and be left unable to start as they have no tip candidates (as there
         // are no blocks that meet the "have data and are not invalid per
         // nStatus" criteria for inclusion in setBlockIndexCandidates).
-        invalid_walk_tip->nStatus |= BLOCK_FAILED_VALID;
-        m_blockman.m_dirty_blockindex.insert(invalid_walk_tip);
-        setBlockIndexCandidates.erase(invalid_walk_tip);
-        setBlockIndexCandidates.insert(invalid_walk_tip->pprev);
+        disconnected_tip->nStatus |= BLOCK_FAILED_VALID;
+        m_blockman.m_dirty_blockindex.insert(disconnected_tip);
+        setBlockIndexCandidates.erase(disconnected_tip);
+        setBlockIndexCandidates.insert(new_tip);
 
         // Mark out-of-chain descendants of the invalidated block as invalid
         // Add any equal or more work headers that are not invalidated to setBlockIndexCandidates
         // Recalculate m_best_header if it became invalid.
-        auto candidate_it = highpow_outofchain_headers.lower_bound(invalid_walk_tip->pprev->nChainWork);
+        auto candidate_it = highpow_outofchain_headers.lower_bound(new_tip->nChainWork);
 
-        const bool best_header_needs_update{m_chainman.m_best_header->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip};
+        const bool best_header_needs_update{m_chainman.m_best_header->GetAncestor(disconnected_tip->nHeight) == disconnected_tip};
         if (best_header_needs_update) {
             // pprev is definitely still valid at this point, but there may be better ones
-            m_chainman.m_best_header = invalid_walk_tip->pprev;
+            m_chainman.m_best_header = new_tip;
         }
 
         while (candidate_it != highpow_outofchain_headers.end()) {
             CBlockIndex* candidate{candidate_it->second};
-            if (candidate->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip) {
+            if (candidate->GetAncestor(disconnected_tip->nHeight) == disconnected_tip) {
                 // Children of failed blocks are marked as BLOCK_FAILED_VALID.
                 candidate->nStatus |= BLOCK_FAILED_VALID;
                 m_blockman.m_dirty_blockindex.insert(candidate);
@@ -3714,7 +3714,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
                 candidate_it = highpow_outofchain_headers.erase(candidate_it);
                 continue;
             }
-            if (!CBlockIndexWorkComparator()(candidate, invalid_walk_tip->pprev) &&
+            if (!CBlockIndexWorkComparator()(candidate, new_tip) &&
                 candidate->IsValid(BLOCK_VALID_TRANSACTIONS) &&
                 candidate->HaveNumChainTxs()) {
                 setBlockIndexCandidates.insert(candidate);
@@ -3733,7 +3733,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
 
     {
         LOCK(cs_main);
-        if (m_chain.Contains(invalid_walk_tip)) {
+        if (m_chain.Contains(pindex)) {
             // If the to-be-marked invalid block is in the active chain, something is interfering and we can't proceed.
             return false;
         }
@@ -3758,7 +3758,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
             }
         }
 
-        InvalidChainFound(invalid_walk_tip);
+        InvalidChainFound(pindex);
     }
 
     // Only notify about a new block tip if the active chain was modified.
@@ -3772,8 +3772,8 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
         // changes.
         (void)m_chainman.GetNotifications().blockTip(
             /*state=*/GetSynchronizationState(m_chainman.IsInitialBlockDownload(), m_chainman.m_blockman.m_blockfiles_indexed),
-            /*index=*/*invalid_walk_tip->pprev,
-            /*verification_progress=*/WITH_LOCK(m_chainman.GetMutex(), return m_chainman.GuessVerificationProgress(invalid_walk_tip->pprev)));
+            /*index=*/*pindex->pprev,
+            /*verification_progress=*/WITH_LOCK(m_chainman.GetMutex(), return m_chainman.GuessVerificationProgress(pindex->pprev)));
 
         // Fire ActiveTipChange now for the current chain tip to make sure clients are notified.
         // ActivateBestChain may call this as well, but not necessarily.