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.

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:

• #32950 (validation: remove BLOCK_FAILED_CHILD by stratospher)

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.

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?

1. to_mark_failed essentially tracks the last disconnected block which used to be part of current chain (m_chain)
2. the possible values to_mark_failed can take are only the values which invalid_walk_tip can take (this line).
3. 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}

4. 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) {

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?

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.