validation: remove BLOCK_FAILED_CHILD #32950

stratospher · 2025-07-11T15:04:52Z

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 (comment), #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)

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

DrahtBot · 2025-07-11T15:04:58Z

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
Concept ACK	TheCharlatan, Prabhat1308, mzumsande, yuvicc, sipa, stringintech
Approach ACK	stickies-v
Stale ACK	naiyoma

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:

#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
#30214 (refactor: Improve assumeutxo state representation by ryanofsky)
#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:

// check grand_parent, parent, child is marked as BLOCK_FAILED_VALID after reloading the block index -> is -> are [subjects "grand_parent, parent, child" are plural, so use "are"]
// This prevents a case where pruned nodes may fail to invalidateblock -> invalidateblock -> InvalidateBlock() [likely intended to refer to the function name; "invalidateblock" is unclear/incorrect casing]

^{drahtbot_id_5_m}

sedited · 2025-07-11T15:32:24Z

Concept ACK

Prabhat1308

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.

src/node/blockstorage.cpp

mzumsande

Concept ACK

naiyoma

Concept ACK,

Wondering if the CI failure is related to this issue. ->#32855

yuvicc

Concept ACK

The CI failure looks like due to #32855

stickies-v

Concept ACK for removing it.

src/node/blockstorage.cpp

naiyoma

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(#32173 (comment)) 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.

after removing BLOCK_FAILED_CHILD:

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

mzumsande

Looks good - just some minor comments.

src/validation.cpp

src/chain.h

src/validation.cpp

sipa

Concept ACK

stickies-v

Approach ACK 2e040d8b3c861c96c536754a82f4352b635c4d01

Intuitively it seems helpful to track something like BLOCK_FAILED_CHILD, but since it's been unused for so long it seems the use case isn't really there. Block validation logic is complex and critical and simplifying it is worth the effort, so I think this is the way to go.

src/node/blockstorage.cpp

src/validation.cpp

src/node/blockstorage.cpp

stratospher · 2025-10-08T15:27:11Z

thanks @stickies-v! I've addressed your review comments.

added a test in cfadc80
used the {disconnected,new}_tip variable tracking suggestions in validation: remove BLOCK_FAILED_CHILD #32950 (comment)

src/test/blockchain_tests.cpp

src/node/blockstorage.cpp

stringintech · 2025-11-04T10:58:51Z

Concept ACK. Left two comments: 1 and 2

Improve upon the variable name for `invalid_walk_tip` to make the InvalidateBlock logic easier to read. Block tip before disconnection is now tracked directly via `disconnected_tip`, and `new_tip` is the tip after the disconnect. Co-authored-by: stickies-v <[email protected]>

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.

since it's the same as BLOCK_FAILED_VALID now

…ng 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.

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.

stratospher · 2025-11-22T06:22:32Z

thanks for the great catches @stringintech and the nicer tests @stickies-v! I've pushed an update.

DrahtBot · 2025-12-16T15:36:16Z

🐙 This pull request conflicts with the target branch and needs rebase.

validation: don't update BLOCK_FAILED_VALID to BLOCK_FAILED_CHILD in …

d121579

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

DrahtBot added the Validation label Jul 11, 2025

DrahtBot added the CI failed label Jul 11, 2025

DrahtBot mentioned this pull request Jul 11, 2025

doc: invalid block handling followups #32843

Open

Prabhat1308 reviewed Jul 12, 2025

View reviewed changes

maflcko reviewed Jul 14, 2025

View reviewed changes

src/node/blockstorage.cpp Outdated Show resolved Hide resolved

mzumsande reviewed Jul 15, 2025

View reviewed changes

naiyoma reviewed Jul 17, 2025

View reviewed changes

yuvicc reviewed Jul 17, 2025

View reviewed changes

stickies-v reviewed Jul 17, 2025

View reviewed changes

src/node/blockstorage.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 1, 2025

kernel, refactor: return error status on all fatal errors #29700

Draft

naiyoma reviewed Aug 7, 2025

View reviewed changes

DrahtBot requested review from Prabhat1308, mzumsande, sedited, stickies-v and yuvicc August 7, 2025 20:59

DrahtBot removed the CI failed label Aug 20, 2025

mzumsande reviewed Sep 24, 2025

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/chain.h Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

DrahtBot requested a review from mzumsande September 24, 2025 18:34

stratospher force-pushed the 2025_02_remove_block_failed_child branch from 06db700 to 2e040d8 Compare October 1, 2025 10:46

sipa reviewed Oct 1, 2025

View reviewed changes

stickies-v reviewed Oct 6, 2025

View reviewed changes

src/node/blockstorage.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/node/blockstorage.cpp Show resolved Hide resolved

stratospher force-pushed the 2025_02_remove_block_failed_child branch from 2e040d8 to cfadc80 Compare October 8, 2025 15:03

DrahtBot mentioned this pull request Oct 16, 2025

kernel, logging: Pass Logger instances to kernel objects #30342

Draft

stickies-v reviewed Oct 17, 2025

View reviewed changes

src/test/blockchain_tests.cpp Outdated Show resolved Hide resolved

src/node/blockstorage.cpp Outdated Show resolved Hide resolved

src/node/blockstorage.cpp Show resolved Hide resolved

stratospher and others added 5 commits November 22, 2025 11:33

validation: remove BLOCK_FAILED_MASK

aea50dd

since it's the same as BLOCK_FAILED_VALID now

stratospher force-pushed the 2025_02_remove_block_failed_child branch from cfadc80 to b22d8b8 Compare November 22, 2025 06:20

DrahtBot mentioned this pull request Nov 23, 2025

refactor: Improve assumeutxo state representation #30214

Merged

DrahtBot added the Needs rebase label Dec 16, 2025

kevkevinpal mentioned this pull request Dec 16, 2025

refactor: Improve assumeutxo state representation kevkevinpal/bitcoin#231

Closed

maflcko mentioned this pull request Dec 17, 2025

fuzz: Add fuzz target for block index tree and related validation events #31533

Merged

validation: remove BLOCK_FAILED_CHILD #32950

Are you sure you want to change the base?

validation: remove BLOCK_FAILED_CHILD #32950

Uh oh!

Conversation

stratospher commented Jul 11, 2025

Uh oh!

DrahtBot commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

sedited commented Jul 11, 2025

Uh oh!

Prabhat1308 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

naiyoma left a comment

Choose a reason for hiding this comment

Uh oh!

yuvicc left a comment

Choose a reason for hiding this comment

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

naiyoma left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stratospher commented Oct 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stringintech commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stratospher commented Nov 22, 2025

Uh oh!

DrahtBot commented Dec 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

DrahtBot commented Jul 11, 2025 •

edited

Loading

naiyoma left a comment •

edited

Loading

stringintech commented Nov 4, 2025 •

edited

Loading