Merge bitcoin/bitcoin#31835: validation: set BLOCK_FAILED_CHILD correctly

3c3548a70e validation: clarify final |= BLOCK_FAILED_VALID in InvalidateBlock (Matt Corallo)
aac5488909 validation: correctly update BlockStatus for invalid block descendants (stratospher)
9e29653b42 test: check BlockStatus when InvalidateBlock is used (stratospher)
c99667583d validation: fix traversal condition to mark BLOCK_FAILED_CHILD (stratospher)

Pull request description:

  This PR addresses 3 issues related to how `BLOCK_FAILED_CHILD` is set:
  1. In `InvalidateBlock()`
  - Previously, `BLOCK_FAILED_CHILD` was not being set when it should have been.
  - This was due to an incorrect traversal condition, which is fixed in this PR.

  2. In `SetBlockFailure()`
  - `BLOCK_FAILED_VALID` is now cleared before setting `BLOCK_FAILED_CHILD`.

  3. In `InvalidateBlock()`
  - if block is already marked as `BLOCK_FAILED_CHILD`, don't mark it as `BLOCK_FAILED_VALID` again.

  Also adds a unit test to check `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` status in `InvalidateBlock()`.

  <details>
  <summary><h3>looking for feedback on an alternate approach</h3></summary>
  <br>

  An alternate approach could be removing `BLOCK_FAILED_CHILD` since even though we have a distinction between
  `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` in the codebase, we don't use it for anything. Whenever we check for BlockStatus, we use `BLOCK_FAILED_MASK` which encompasses both of them. See  similar discussion in https://github.com/bitcoin/bitcoin/pull/16856.

  I have a branch with this approach in https://github.com/stratospher/bitcoin/commits/2025_02_remove_block_failed_child/.
  Compared to the version in #16856, it also resets `BLOCK_FAILED_CHILD` already on disk to `BLOCK_FAILED_VALID` when loading from disk so that we won't be in a dirty state in a no-`BLOCK_FAILED_CHILD`-world.

  I'm not sure if it's a good idea to remove `BLOCK_FAILED_CHILD` though. would be curious to hear what others think of this approach.

  thanks @ mzumsande for helpful discussion regarding this PR!
  </details>

ACKs for top commit:
  achow101:
    ACK 3c3548a70e
  TheCharlatan:
    Re-ACK 3c3548a70e
  mzumsande:
    re-ACK 3c3548a70e

Tree-SHA512: 83e0d29dea95b97519d4868135c965b86f6f43be50b15c0bd8f998b3476388fc7cc22b49c0c54ec532ae8222e57dfc436438f0c8e98f54757b384f220488b6a6

src/test/blockchain_tests.cpp

@@ -117,4 +117,41 @@ BOOST_AUTO_TEST_CASE(num_chain_tx_max)
     BOOST_CHECK_EQUAL(block_index.m_chain_tx_count, std::numeric_limits<uint64_t>::max());
 }
 
+BOOST_FIXTURE_TEST_CASE(invalidate_block, TestChain100Setup)
+{
+    const CChain& active{*WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return &Assert(m_node.chainman)->ActiveChain())};
+
+    // Check BlockStatus when doing InvalidateBlock()
+    BlockValidationState state;
+    auto* orig_tip = active.Tip();
+    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);
+
+    // tip_to_invalidate just got invalidated, so it's BLOCK_FAILED_VALID
+    WITH_LOCK(::cs_main, assert(tip_to_invalidate->nStatus & BLOCK_FAILED_VALID));
+    WITH_LOCK(::cs_main, assert((tip_to_invalidate->nStatus & BLOCK_FAILED_CHILD) == 0));
+
+    // check all ancestors of the invalidated block are validated up to BLOCK_VALID_TRANSACTIONS and are not invalid
+    auto pindex = tip_to_invalidate->pprev;
+    while (pindex) {
+        WITH_LOCK(::cs_main, assert(pindex->IsValid(BLOCK_VALID_TRANSACTIONS)));
+        WITH_LOCK(::cs_main, assert((pindex->nStatus & BLOCK_FAILED_MASK) == 0));
+        pindex = pindex->pprev;
+    }
+
+    // check all descendants of the invalidated block are BLOCK_FAILED_CHILD
+    pindex = orig_tip;
+    while (pindex && pindex != tip_to_invalidate) {
+        WITH_LOCK(::cs_main, assert((pindex->nStatus & BLOCK_FAILED_VALID) == 0));
+        WITH_LOCK(::cs_main, assert(pindex->nStatus & BLOCK_FAILED_CHILD));
+        pindex = pindex->pprev;
+    }
+
+    // don't mark already invalidated block (orig_tip is BLOCK_FAILED_CHILD) with BLOCK_FAILED_VALID again
+    m_node.chainman->ActiveChainstate().InvalidateBlock(state, orig_tip);
+    WITH_LOCK(::cs_main, assert(orig_tip->nStatus & BLOCK_FAILED_CHILD));
+    WITH_LOCK(::cs_main, assert((orig_tip->nStatus & BLOCK_FAILED_VALID) == 0));
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/validation.cpp

@@ -3747,7 +3747,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
         m_blockman.m_dirty_blockindex.insert(invalid_walk_tip);
         setBlockIndexCandidates.erase(invalid_walk_tip);
         setBlockIndexCandidates.insert(invalid_walk_tip->pprev);
-        if (invalid_walk_tip->pprev == to_mark_failed && (to_mark_failed->nStatus & BLOCK_FAILED_VALID)) {
+        if (invalid_walk_tip == to_mark_failed->pprev && (to_mark_failed->nStatus & BLOCK_FAILED_VALID)) {
             // We only want to mark the last disconnected block as BLOCK_FAILED_VALID; its children
             // need to be BLOCK_FAILED_CHILD instead.
             to_mark_failed->nStatus = (to_mark_failed->nStatus ^ BLOCK_FAILED_VALID) | BLOCK_FAILED_CHILD;
@@ -3779,11 +3779,13 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
             return false;
         }
 
-        // Mark pindex (or the last disconnected block) as invalid, even when it never was in the main chain
-        to_mark_failed->nStatus |= BLOCK_FAILED_VALID;
-        m_blockman.m_dirty_blockindex.insert(to_mark_failed);
-        setBlockIndexCandidates.erase(to_mark_failed);
-        m_chainman.m_failed_blocks.insert(to_mark_failed);
+        // Mark pindex as invalid if it never was in the main chain
+        if (!pindex_was_in_chain && !(pindex->nStatus & BLOCK_FAILED_MASK)) {
+            pindex->nStatus |= BLOCK_FAILED_VALID;
+            m_blockman.m_dirty_blockindex.insert(pindex);
+            setBlockIndexCandidates.erase(pindex);
+            m_chainman.m_failed_blocks.insert(pindex);
+        }
 
         // If any new blocks somehow arrived while we were disconnecting
         // (above), then the pre-calculation of what should go into
@@ -3826,8 +3828,9 @@ void Chainstate::SetBlockFailureFlags(CBlockIndex* invalid_block)
     AssertLockHeld(cs_main);
 
     for (auto& [_, block_index] : m_blockman.m_block_index) {
-        if (block_index.GetAncestor(invalid_block->nHeight) == invalid_block && !(block_index.nStatus & BLOCK_FAILED_MASK)) {
-            block_index.nStatus |= BLOCK_FAILED_CHILD;
+        if (invalid_block != &block_index && block_index.GetAncestor(invalid_block->nHeight) == invalid_block) {
+            block_index.nStatus = (block_index.nStatus & ~BLOCK_FAILED_VALID) | BLOCK_FAILED_CHILD;
+            m_blockman.m_dirty_blockindex.insert(&block_index);
         }
     }
 }