validation: set BLOCK_FAILED_CHILD correctly #31835

1. 
   stratospher commented at 2:59 pm on February 10, 2025: contributor

   This PR addresses 2 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.

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

   looking for feedback on an alternate approach

   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 #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!

2. validation: fix traversal condition to mark BLOCK_FAILED_CHILD

   this block of code is not reached on master since other than
   initialisation, all other iterations have invalid_walk_tip
   and to_mark_failed pointers in some form of this layout.
   
   	invalid_walk_tip
   	  ↓
   1 -> 2 -> 3 -> 4
   	       ↑
   	      to_mark_failed
   
   fix it so that blocks are correctly marked as BLOCK_FAILED_CHILD
   if it's a descendant of BLOCK_FAILED_VALID block.

   27b5abf946
3. test: check BlockStatus when InvalidateBlock is used

   when a block is invalidated using InvalidateBlock, check that:
   1. it's status is BLOCK_FAILED_VALID
   2. it's children's status is BLOCK_FAILED_CHILD
      and not BLOCK_FAILED_VALID
   3. it's ancestors are valid

   c03ff89eba
4. validation: correctly update BlockStatus for invalid block descendants

   invalid_block ----------> block_index
   
   - before this commit, only if block_index is not invalid, it will mark
     block_index as BLOCK_FAILED_CHILD
   - it's possible that block_index encountered is invalid and was marked
     as BLOCK_FAILED_VALID previously
   - in this case, correctly update BlockStatus of block_index by
     clearing BLOCK_FAILED_VALID and then setting it to BLOCK_FAILED_CHILD

   3328bc6245
5. 
   DrahtBot commented at 2:59 pm on February 10, 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/31835.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	mzumsande

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

6. DrahtBot added the label Validation on Feb 10, 2025
7. 
   mzumsande commented at 11:09 pm on February 11, 2025: contributor

   Concept ACK

   This is a straightforward bugfix.

   In general, I’m in favor of removing the BLOCK_FAILED_CHILD / BLOCK_FAILED_VALID distinction as a follow-up, as suggested in #16856 (comment). It’s been introduced in 2012 (#1677) and (as far as I know) has never been used for anything. I also can’t think of a legitimate use - block indexes that are marked as invalid are invalid indefinitely (outside of the reconsiderblock rpc, but that removes both flags from all descendants of a reconsidered block so it doesn’t need the distinction either.) It just adds unnecessary complexity in my opinion.

Contributors
stratospher DrahtBot mzumsande

Labels
Validation