Skip to content

validation: set BLOCK_FAILED_CHILD correctly

Placeholder stratospher requested to merge github/fork/stratospher/2025_02_block_failed_child into master

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.
  1. 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 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!

Merge request reports