validation: stricter internal handling of invalid blocks #31405

1. 
   mzumsande commented at 7:29 pm on December 2, 2024: contributor

   Some fields in validation are set opportunistically by “best effort”:

   • The BLOCK_FAILED_CHILD status (which means that the block index has an invalid predecessor)
   • m_best_header (the most-work header not known to be invalid).

   This means that there are known situations in which these fields are not set when they should be, or set to wrong values. This is tolerated because the fields are not used for anything consensus-critical and triggering these situations involved creating invalid blocks with valid PoW header, so would have a cost attached. Also, having stricter guarantees for these fields requires iterating over the entire block index, which has some DoS potential, especially with any header above the checkpoint being accepted int he past (see e.g. #11531).

   However, there are reasons to change this now:

   • RPCs use these fields and can report wrong results
   • There is the constant possibility that someone could add code that expects these fields to be correct, especially because it is not well documented that these fields cannot always be relied upon.
   • DoS concerns have become less of an issue after #25717 - now an attacker would need to invest much more work because they can’t fork off the last checkpoint anymore

   This PR continues the work from #30666 to ensure that BLOCK_FAILED_CHILD status and m_best_header are always correct:

   • it adds a call to InvalidChainFound() in AcceptBlock().
   • it adds checks for BLOCK_FAILED_CHILD and m_best_header to CheckBlockIndex(). In order to be able to do this, two more caches were added to the RPC-only InvalidateBlock() similar to the existing candidate_blocks_by_work for setBlockIndexCandidates. These are performance optimizations with the goal of avoiding having a call of InvalidChainFound() / looping over the block index after each disconnected block. I also wrote a fuzz test to find possible edge cases violating CheckBlockIndex, which I will PR separately soon.
   • it removes the m_failed_blocks set, which was a heuristic necessary when we couldn’t be sure if a given block index had an invalid predecessor or not. Now that we have that guarantee, the set is no longer needed.
2. validation: call InvalidChainfound also from AcceptBlock

   In this scenario we have stored a valid header with an invalid (but not
   mutated!) block, so it can only be triggered by doing the
   necessary PoW.
   The added call ensures that descendant blocks failure flags and m_best_header
   will be set correctly - at the cost of looping over the entire block
   index, which, because of the mentioned necessary PoW, is not a DoS
   vector.

   61e8d9631b
3. 
   DrahtBot commented at 7:29 pm on December 2, 2024: 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/31405.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #30479 (validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates by mzumsande)

   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.

4. DrahtBot added the label Validation on Dec 2, 2024
5. validation: in invalidateblock, mark children as invalid right away

   Before, they would be marked as invalid only after disconnecting
   multiple blocks, letting go of cs_main in the meantime.
   This is in preparation for adding a check to
   CheckBlockIndex() requiring that descendants of invalid block indexes
   are always marked as invalid.

   7f88af8b24
6. validation: in invalidateblock, calculate m_best_header right away

   Before, m_best_header would be calculated only after disconnecting
   multiple blocks, letting go of cs_main in the meantime.
   This is in preparation for adding checks to CheckBlockIndex()
   requiring that m_best_header is the most-work header not known to be invalid.

   89ce4b0cdb
7. validation: Add more checks to CheckBlockIndex()

   This adds checks that
   1) Descendants of invalid block indexes are also marked invalid
   2) m_best_header cannot be invalid, and there can be no valid
   block with more work than it.

   528b8cc98d
8. validation: remove m_failed_blocks

   We now mark all blocks that descend from an invalid block
   immediately as the invalid block is encountered
   (by iterating over the entire block index).
   As a result, m_failed_blocks, which was a heuristic to only
   mark descendants of failed blocks as failed when necessary,
   (i.e., when we have do decide whether to add another descendant or not)
   is no longer required.

   b2136da98d
9. mzumsande force-pushed on Dec 2, 2024
10. DrahtBot added the label CI failed on Dec 2, 2024
11. 
    DrahtBot commented at 8:47 pm on December 2, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33808912013

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

12. DrahtBot removed the label CI failed on Dec 2, 2024
13. mzumsande marked this as ready for review on Dec 3, 2024

Contributors
mzumsande DrahtBot

Labels
Validation