Skip to content

validation: stricter internal handling of invalid blocks

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.

Merge request reports

Loading