validation: stricter internal handling of invalid blocks #31405
Conversation
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.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31405. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
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.
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.
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.
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.
mzumsande
force-pushed
the
202411_stricter_invalidblock_handling
branch
from
ce3dc73 to
b2136da
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
Some fields in validation are set opportunistically by "best effort":
BLOCK_FAILED_CHILDstatus (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:
This PR continues the work from #30666 to ensure that
BLOCK_FAILED_CHILDstatus andm_best_headerare always correct:InvalidChainFound()inAcceptBlock().BLOCK_FAILED_CHILDandm_best_headertoCheckBlockIndex(). In order to be able to do this, two more caches were added to the RPC-onlyInvalidateBlock()similar to the existingcandidate_blocks_by_workforsetBlockIndexCandidates. These are performance optimizations with the goal of avoiding having a call ofInvalidChainFound()/ 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.m_failed_blocksset, 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.