validation: invalid block handling followups #32843

1. 
   mzumsande commented at 10:10 pm on June 30, 2025: contributor

   Some follow-ups from #31405:

   • avoid duplicate recalculation of failure flags and m_best_header in InvalidateBlock(), which already does the accounting for these quantities while disconnecting blocks, so recalcuation is not needed in the final InvalidChainFound call (https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2138368309)
   • 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 ( #31405 (review) )
   • add a comment that nChainWork, not CBlockIndexWorkComparator is used for m_best_header (https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2138368441)
2. validation: Don't repeat work in InvalidateBlock

   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.

   30b190eeba
3. DrahtBot added the label Validation on Jun 30, 2025
4. 
   DrahtBot commented at 10:10 pm on June 30, 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/32843.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	stratospher

   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.

5. doc: Improve doc for candidate blocks

   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.

   e32e72039f
6. mzumsande force-pushed on Jun 30, 2025
7. 
   stratospher commented at 6:38 am on July 8, 2025: contributor

   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:

   0best_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.)

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

8. 
   mzumsande commented at 7:26 pm on July 8, 2025: contributor

   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?

9. 
   stratospher commented at 5:35 pm on July 9, 2025: contributor

   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.

10. stratospher approved
11. 
    stratospher commented at 7:17 am on July 10, 2025: contributor

    ACK e32e72039f02ea0b565e0ef5ab7fa40faeb65e4c.

Contributors
mzumsande DrahtBot stratospher

Labels
Validation