validation: remove BLOCK_FAILED_CHILD #32950

1. 
   stratospher commented at 3:04 pm on July 11, 2025: contributor

   Fixes #32173

   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.

   Since there is no functional difference between BLOCK_FAILED_VALID and BLOCK_FAILED_CHILD and it’s added code complexity to correctly categorise them (ex: #31405 (review), #16856 (comment)), we could just remove it.

   Looking for conceptual feedback on whether it’s better to improve handling of BLOCK_FAILED_CHILD in the codebase or remove BLOCK_FAILED_CHILD.

   Of less relevance, but it would also fix a reconsiderblock crash that could happen in the situation mentioned in #32173 (comment)

   Similar attempt in the past in #16856 (comment)

2. validation: don't update BLOCK_FAILED_VALID to BLOCK_FAILED_CHILD in InvalidateBlock

   - there is no functional difference between BLOCK_FAILED_VALID and BLOCK_FAILED_CHILD
   and it's unnecessary code complexity to correctly categorise them.

   d121579abc
3. refactor/validation: remove to_mark_failed

   to_mark_failed was useful to track last disconnected block so that
   it's children can be marked as BLOCK_FAILED_CHILD. since the previous
   commit removes BLOCK_FAILED_CHILD, the existing variable
   invalid_walk_tip is sufficient.
   
   invalid_walk_tip can track the last disconnected block
   since we exit out of the loop before invalid_walk_tip is reset. this
   is needed in code later on to mark last disconnected block as invalid.

   ca2421d527
4. validation: remove BLOCK_FAILED_CHILD

   even though we have a distinction between BLOCK_FAILED_VALID
   and BLOCK_FAILED_CHILD in the codebase, we don't use it for
   anything. since there's no functional difference between them
   and it's unnecessary code complexity to categorise them correctly,
   just mark as BLOCK_FAILED_VALID instead.

   8935af08c9
5. validation: remove BLOCK_FAILED_MASK

   since it's the same as BLOCK_FAILED_VALID now

   4200ecf662
6. validation: reset BLOCK_FAILED_CHILD to BLOCK_FAILED_VALID when loading from disk

   - there maybe existing block indexes stored in disk with
     BLOCK_FAILED_CHILD
   - since they don't exist anymore, clean up block index entries with
     BLOCK_FAILED_CHILD and reset it to BLOCK_FAILED_VALID.

   06db700a13
7. DrahtBot added the label Validation on Jul 11, 2025
8. 
   DrahtBot commented at 3:04 pm on July 11, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32950.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	naiyoma
   Concept ACK	TheCharlatan, Prabhat1308, mzumsande, yuvicc, stickies-v

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #32843 (validation: invalid block handling followups by mzumsande)
   • #29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

   LLM Linter (✨ experimental)

   Possible typos and grammar issues:

   • exists in disk -> exists on disk [“in disk” is nonstandard; use “on disk”]

   No other typos affecting comprehension were found.

   drahtbot_id_4_m

9. 
   TheCharlatan commented at 3:32 pm on July 11, 2025: contributor

   Concept ACK
10. DrahtBot added the label CI failed on Jul 11, 2025
11. 
    Prabhat1308 commented at 3:38 pm on July 12, 2025: contributor

    Concept ACK

    I think removing is a better option. The only weak argument that we could have for not removing is that BLOCK_FAILED_VALID right now marks the start of the invalid block chain and is better for debugging manually. However , the burden of maintaining this distinction seems too high for debugging purposes with no actual benefit functionally.

12. in src/node/blockstorage.cpp:467 in 06db700a13

    462@@ -463,8 +463,10 @@ bool BlockManager::LoadBlockIndex(const std::optional<uint256>& snapshot_blockha
    463                 pindex->m_chain_tx_count = pindex->nTx;
    464             }
    465         }
    466-        if (!(pindex->nStatus & BLOCK_FAILED_MASK) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_MASK)) {
    467-            pindex->nStatus |= BLOCK_FAILED_CHILD;
    468+        if (!(pindex->nStatus & BLOCK_FAILED_VALID) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_VALID)) {
    469+            // if BLOCK_FAILED_CHILD already exists in disk, clear it
    

    ---

    ---

    

    maflcko commented at 10:22 am on July 14, 2025:

    exists in disk -> exists on disk [“in disk” is nonstandard; use “on disk”]
    

    An alternative wording could be “if … was persisted, clear it”

    ---

    

    stickies-v commented at 9:54 pm on July 17, 2025:

    Or:

    // BLOCK_FAILED_CHILD is deprecated, but may still exist on disk. Replace it with BLOCK_FAILED_VALID.

13. 
    mzumsande commented at 9:59 pm on July 15, 2025: contributor

    Concept ACK
14. 
    naiyoma commented at 6:58 am on July 17, 2025: contributor

    Concept ACK,

    Wondering if the CI failure is related to this issue. ->https://github.com/bitcoin/bitcoin/issues/32855

15. 
    yuvicc commented at 1:21 pm on July 17, 2025: contributor

    Concept ACK

    The CI failure looks like due to #32855

16. 
    stickies-v commented at 9:55 pm on July 17, 2025: contributor

    Concept ACK for removing it.
17. 
    naiyoma commented at 8:59 pm on August 7, 2025: contributor

    TestedACK 06db700a1315bb655ac7fa12578c626990a22ea5

                      Block 2 (valid)
                        /         \
    (valid)Block height 3         Block height 3 (BLOCK_FAILED_CHILD)
                        |         |
    (valid) Block height 4        Block height 4(BLOCK_FAILED_CHILD)
                        |         |
    

    Before removing BLOCK_FAILED_CHILD,this test(https://github.com/bitcoin/bitcoin/issues/32173#issue-2960274990) would fail at: self.log.info(self.nodes[0].reconsiderblock(res["bestblock"])) (Reconsidering block at height 3 — which has BLOCK_FAILED_CHILD) This failure happened because:

    This check fails, block 3 has BLOCK_FAILED_CHILD (not BLOCK_FAILED_VALID).

    and so in this assertion -> https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L5363

    pindex->nStatus & BLOCK_FAILED_MASK) != 0.

    Now, after removing BLOCK_FAILED_CHILD:

    block 3 , status is BLOCK_FAILED_VALID, and pindexFirstInvalid is set correctly.

18. DrahtBot requested review from Prabhat1308 on Aug 7, 2025
19. DrahtBot requested review from TheCharlatan on Aug 7, 2025
20. DrahtBot requested review from mzumsande on Aug 7, 2025
21. DrahtBot requested review from yuvicc on Aug 7, 2025
22. DrahtBot requested review from stickies-v on Aug 7, 2025

Contributors
stratospher DrahtBot TheCharlatan Prabhat1308 maflcko stickies-v mzumsande naiyoma yuvicc

Review Requested
TheCharlatan mzumsande stickies-v yuvicc Prabhat1308

Labels
Validation CI failed