doc: invalid block handling followups #32843

mzumsande · 2025-06-30T22:10:44Z

Some follow-ups from #31405:

make it clearer that all block indexes with a CBlockIndexWorkComparator score at least as good as the tip (and no others) are expected in setBlockIndexCandidates.
People think of nChainWork when the term work is used and not of the (slightly arbitrary) CBlockIndexWorkComparator tiebreaker rules, which makes the current documentation imprecise ( validation: stricter internal handling of invalid blocks #31405 (comment) )
add a comment that nChainWork, not CBlockIndexWorkComparator is used for m_best_header (validation: stricter internal handling of invalid blocks #31405 (comment))

DrahtBot · 2025-06-30T22:10:49Z

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/32843.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	stratospher, ryanofsky

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30214 (refactor: Improve assumeutxo state representation 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.

stratospher · 2025-07-08T06:38:43Z

In InvalidateBlock, both the block failure flags and m_best_header are kept up to date with each block that is disconnected, so it's not necessary to recalculate them at the end of this function.

maybe I'm missing something but even without the calc_flags_and_header=false flag, this means that the condition cannot hit true and RecalculateBestHeader() in InvalidChainFound() cannot be called right?

to_mark_failed essentially tracks the last disconnected block which used to be part of current chain (m_chain)
the possible values to_mark_failed can take are only the values which invalid_walk_tip can take (this line).
we have very judiciously set m_best_header when invalid_walk_tip is invalid using related logic in:

best_header_needs_update{m_chainman.m_best_header->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip}

so when we call InvalidChainFound() on to_mark_failed, even without passing the false flag, this condition will be false: (because to_mark_failed can only be invalid_walk_tip and we already handled that in 3.)

if (m_chainman.m_best_header != nullptr && m_chainman.m_best_header->GetAncestor(pindexNew->nHeight) == pindexNew) {

mzumsande · 2025-07-08T19:26:03Z

maybe I'm missing something but even without the calc_flags_and_header=false flag, this means that the condition cannot hit true and RecalculateBestHeader() in InvalidChainFound() cannot be called right?

Yes, that's true, good point! To phrase it in my words, the condition that checks whether m_best_header needs update will not be fulfilled, because m_best_header was already successfully updated.

I guess that means we could, instead of calc_flags_and_header, just use a bool calc_flags (for which we can't do a similar check). Do you think that would be clearer?

stratospher · 2025-07-09T17:35:03Z

I guess that means we could, instead of calc_flags_and_header, just use a bool calc_flags (for which we can't do a similar check). Do you think that would be clearer?

yes sounds good! (whenever you retouch next)

Ah missed the fact that iteration of entire block index is done in SetBlockFailureFlags() as well. I originally thought that iteration of entire block index is done only for best header calculation and calc_flags_and_header wasn't useful. Hence the confusion.

stratospher

ACK e32e720.

stickies-v · 2025-10-24T12:30:08Z

src/validation.cpp


 // Called both upon regular invalid block discovery *and* InvalidateBlock
-void Chainstate::InvalidChainFound(CBlockIndex* pindexNew)
+void Chainstate::InvalidChainFound(CBlockIndex* pindexNew, bool calc_flags_and_header = true)


Default values are best defined in the header file

stickies-v · 2025-10-24T13:16:49Z

src/validation.cpp


 // Called both upon regular invalid block discovery *and* InvalidateBlock
-void Chainstate::InvalidChainFound(CBlockIndex* pindexNew)
+void Chainstate::InvalidChainFound(CBlockIndex* pindexNew, bool calc_flags_and_header = true)


Apologies for mostly coming with stop energy here, but I think I'm approach NACK on 30b190e, and I would prefer dropping the commit. While the duplicated header calculation and flag setting is wasteful, I don't think we should complicate our validation logic further for the very rare case that is invalidateblock.

I don't have a suggested alternative yet, but it seems to me that reorganizing this code into different, more natural functions might be an alternative worth exploring. I don't conceptually understand why we have different functions for InvalidChainFound and InvalidBlockFound, and in practice it seems they exist for reasons unrelated to their difference in name. Similarly, SetBlockFailureFlags is a confusing function that doesn't actually set that block's failure flags.

Even without an alternative, I would rather make no change than this change, I don't think anything actually needs fixing here.

In commit "validation: Don't repeat work in InvalidateBlock" (30b190e)

re: #32843 (comment)

Apologies for mostly coming with stop energy here, but I think I'm approach NACK on 30b190e

I tend to disagree and think the code is clearer with this change than without it. Without it, intent is not possible to discern. The code is doing the same work twice without an indication of why it is doing that. With this change, behavior actually makes sense and intent is clear.

I do agree code could be reorganized better and made less fragile, but I don't see the new code here as being more fragile than current code, just more explicit.

I do share some concern about the default value, though, and I think it would be better if InvalidChainFound was not commenting on invalidateblock behavior, which could change at some point. So might suggest:

diff

--- a/src/validation.cpp +++ b/src/validation.cpp @@ -2064,13 +2064,12 @@ void Chainstate::CheckForkWarningConditions() } // Called both upon regular invalid block discovery *and* InvalidateBlock -void Chainstate::InvalidChainFound(CBlockIndex* pindexNew, bool calc_flags_and_header = true) +void Chainstate::InvalidChainFound(CBlockIndex* pindexNew, bool calc_flags_and_header) { AssertLockHeld(cs_main); if (!m_chainman.m_best_invalid || pindexNew->nChainWork > m_chainman.m_best_invalid->nChainWork) { m_chainman.m_best_invalid = pindexNew; } - // Unnecessary in the invalidateblock case, which has its own accounting for the failure flags and best header. if (calc_flags_and_header) { SetBlockFailureFlags(pindexNew); if (m_chainman.m_best_header != nullptr && m_chainman.m_best_header->GetAncestor(pindexNew->nHeight) == pindexNew) { @@ -2098,7 +2097,7 @@ void Chainstate::InvalidBlockFound(CBlockIndex* pindex, const BlockValidationSta pindex->nStatus |= BLOCK_FAILED_VALID; m_blockman.m_dirty_blockindex.insert(pindex); setBlockIndexCandidates.erase(pindex); - InvalidChainFound(pindex); + InvalidChainFound(pindex, /*calc_flags_and_header=*/true); } } @@ -3376,7 +3375,7 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex* if (state.IsInvalid()) { // The block violates a consensus rule. if (state.GetResult() != BlockValidationResult::BLOCK_MUTATED) { - InvalidChainFound(vpindexToConnect.front()); + InvalidChainFound(vpindexToConnect.front(), /*calc_flags_and_header=*/true); } state = BlockValidationState(); fInvalidFound = true; @@ -3809,6 +3808,8 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde } } + // Record invalid chain, avoiding recomputing m_best_header and applying + // BLOCK_FAILED_CHILD flags since these things were done above. InvalidChainFound(to_mark_failed, /*calc_flags_and_header=*/false); }

Or if this commit will be dropped, I think there should at least be a comment added before the InvalidChainFound call in InvalidateBlock saying InvalidChainFound will repeat some work that was already before cs_main was released, to make it clear the duplication was at least not an accident.

After this discussion in which we discussed the separate topic that the current InvalidChainFound() call in ActivateBestChainStep() often does repeated work and logging, I decided to drop the commit, and will work on a PR to reorder things instead. Current rough plan is to move most things from InvalidChainFound to InvalidBlockFound and rename the former to UpdateBestInvalid().

For this PR, I have instead added a comment, so that it is doc-only now. I might also close this and add the commit to the other PR to avoid churn, I'll check that option next week.

ryanofsky

Code review ACK e32e720. Thanks for these followups, I think they should make it easier to remember what this code is trying to do.

ryanofsky · 2025-10-27T17:11:25Z

src/validation.cpp


 // Called both upon regular invalid block discovery *and* InvalidateBlock
-void Chainstate::InvalidChainFound(CBlockIndex* pindexNew)
+void Chainstate::InvalidChainFound(CBlockIndex* pindexNew, bool calc_flags_and_header = true)


In commit "validation: Don't repeat work in InvalidateBlock" (30b190e)

re: #32843 (comment)

Apologies for mostly coming with stop energy here, but I think I'm approach NACK on 30b190e

I tend to disagree and think the code is clearer with this change than without it. Without it, intent is not possible to discern. The code is doing the same work twice without an indication of why it is doing that. With this change, behavior actually makes sense and intent is clear.

I do agree code could be reorganized better and made less fragile, but I don't see the new code here as being more fragile than current code, just more explicit.

I do share some concern about the default value, though, and I think it would be better if InvalidChainFound was not commenting on invalidateblock behavior, which could change at some point. So might suggest:

diff

--- a/src/validation.cpp +++ b/src/validation.cpp @@ -2064,13 +2064,12 @@ void Chainstate::CheckForkWarningConditions() } // Called both upon regular invalid block discovery *and* InvalidateBlock -void Chainstate::InvalidChainFound(CBlockIndex* pindexNew, bool calc_flags_and_header = true) +void Chainstate::InvalidChainFound(CBlockIndex* pindexNew, bool calc_flags_and_header) { AssertLockHeld(cs_main); if (!m_chainman.m_best_invalid || pindexNew->nChainWork > m_chainman.m_best_invalid->nChainWork) { m_chainman.m_best_invalid = pindexNew; } - // Unnecessary in the invalidateblock case, which has its own accounting for the failure flags and best header. if (calc_flags_and_header) { SetBlockFailureFlags(pindexNew); if (m_chainman.m_best_header != nullptr && m_chainman.m_best_header->GetAncestor(pindexNew->nHeight) == pindexNew) { @@ -2098,7 +2097,7 @@ void Chainstate::InvalidBlockFound(CBlockIndex* pindex, const BlockValidationSta pindex->nStatus |= BLOCK_FAILED_VALID; m_blockman.m_dirty_blockindex.insert(pindex); setBlockIndexCandidates.erase(pindex); - InvalidChainFound(pindex); + InvalidChainFound(pindex, /*calc_flags_and_header=*/true); } } @@ -3376,7 +3375,7 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex* if (state.IsInvalid()) { // The block violates a consensus rule. if (state.GetResult() != BlockValidationResult::BLOCK_MUTATED) { - InvalidChainFound(vpindexToConnect.front()); + InvalidChainFound(vpindexToConnect.front(), /*calc_flags_and_header=*/true); } state = BlockValidationState(); fInvalidFound = true; @@ -3809,6 +3808,8 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde } } + // Record invalid chain, avoiding recomputing m_best_header and applying + // BLOCK_FAILED_CHILD flags since these things were done above. InvalidChainFound(to_mark_failed, /*calc_flags_and_header=*/false); }

Or if this commit will be dropped, I think there should at least be a comment added before the InvalidChainFound call in InvalidateBlock saying InvalidChainFound will repeat some work that was already before cs_main was released, to make it clear the duplication was at least not an accident.

ryanofsky · 2025-10-27T17:34:47Z

src/validation.cpp

                // Do not remove candidate from the highpow_outofchain_headers cache, because it might be a descendant of the block being invalidated
                // which needs to be marked failed later.
            }
+            // Best header updating currently doesn't use CBlockIndexWorkComparator, so nChainWork is used for consistency.


In commit "doc: Improve doc for candidate blocks" (e32e720)

Would maybe s/nChainWork/only nChainWork/ since using nChainWork shouldn't be surprising here, but only using nChainWork might be.

ryanofsky · 2025-10-27T17:40:51Z

src/validation.cpp

+            // Best header updating currently doesn't use CBlockIndexWorkComparator, so nChainWork is used for consistency.
            if (best_header_needs_update &&
                m_chainman.m_best_header->nChainWork < candidate->nChainWork) {
                m_chainman.m_best_header = candidate;


In commit "doc: Improve doc for candidate blocks" (e32e720)

Note for reviewers: best description I've seen for why it's not currently safe to assume m_best_header is consistent with CBlockIndexWorkComparator ordering is here #31405 (comment)

ryanofsky · 2025-10-27T17:52:43Z

src/validation.cpp

                // Do not remove candidate from the highpow_outofchain_headers cache, because it might be a descendant of the block being invalidated
                // which needs to be marked failed later.
            }
+            // Best header updating currently doesn't use CBlockIndexWorkComparator, so nChainWork is used for consistency.


In commit "doc: Improve doc for candidate blocks" (e32e720)

To be complete, this could also mention that when there is a tie, this will set m_best_header to whichever block happens to come first in m_block_index std::unordered_map iteration order. This ordering is consistent with the ordering used in RecalculateBestHeader, but inconsistent with the ordering used in AddToBlockIndex, which is based on the order headers are received.

I'm not sure how much detail is necessary here, so also just making these notes for myself.

I added a sentence with the basics (didn't include the describe the different orderings).

It is not true in general that all blocks with the same work (as defined via nChainWork) as the tip are candidates expected to be in setBlockIndexCandidates - only ones with a better tiebreak than the tip. Make that clear in various spots. Also explain that we don't use CBlockIndexWorkComparator tiebreak rules for m_best_header.

DrahtBot · 2025-12-16T15:37:06Z

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

DrahtBot added the Validation label Jun 30, 2025

mzumsande force-pushed the 202506_invalid_blocks_followup branch from ad6cdc8 to e32e720 Compare June 30, 2025 22:12

DrahtBot mentioned this pull request Jul 1, 2025

refactor: Improve assumeutxo state representation #30214

Merged

stratospher approved these changes Jul 10, 2025

View reviewed changes

DrahtBot mentioned this pull request Jul 11, 2025

validation: remove BLOCK_FAILED_CHILD #32950

Open

mzumsande mentioned this pull request Jul 14, 2025

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

Merged

achow101 requested review from ryanofsky, sipa and stickies-v October 22, 2025 15:29

stickies-v reviewed Oct 24, 2025

View reviewed changes

ryanofsky approved these changes Oct 27, 2025

View reviewed changes

mzumsande force-pushed the 202506_invalid_blocks_followup branch from e32e720 to f745092 Compare December 12, 2025 21:07

mzumsande changed the title ~~validation: invalid block handling followups~~ doc: invalid block handling followups Dec 12, 2025

mzumsande force-pushed the 202506_invalid_blocks_followup branch from f745092 to c34e44d Compare December 12, 2025 21:11

DrahtBot added CI failed Needs rebase and removed CI failed labels Dec 12, 2025

kevkevinpal mentioned this pull request Dec 16, 2025

refactor: Improve assumeutxo state representation kevkevinpal/bitcoin#231

Closed

doc: invalid block handling followups #32843

Are you sure you want to change the base?

doc: invalid block handling followups #32843

Uh oh!

Conversation

mzumsande commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

stratospher commented Jul 8, 2025

Uh oh!

mzumsande commented Jul 8, 2025

Uh oh!

stratospher commented Jul 9, 2025

Uh oh!

stratospher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 27, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 27, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Dec 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mzumsande commented Jun 30, 2025 •

edited

Loading

DrahtBot commented Jun 30, 2025 •

edited

Loading

ryanofsky Oct 27, 2025 •

edited

Loading

ryanofsky Oct 27, 2025 •

edited

Loading