validation: invalid block handling followups #32843

pull mzumsande wants to merge 2 commits into bitcoin:master from mzumsande:202506_invalid_blocks_followup changing 2 files +21 −13
  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}
    
    1. 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.

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-07-26 12:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me