validation: Improve, document and test logic for chains building on invalid blocks #30207
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
mzumsande
force-pushed
the
202405_invalid_chains
branch
from
c779959 to
994d673
Compare
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
mzumsande
force-pushed
the
202405_invalid_chains
branch
from
994d673 to
2976f3a
Compare
|
Concept ACK |
There are known situations in which these can be wrong (but without any serious consequences). Documenting these limitations should help prevent future bugs. In order to make these fields always consistent, we'd need to walk over the entire blockindex more often and/or introduce additional block header tree tracking.
…OCK_FAILED_CHILD to help the getchaintips rpc detect invalid chains better. Marking these blocks as BLOCK_FAILED_CHILD is computationally inexpensive. This will usually help, but not always: In practice, it is often the case that m_best_header will be at the end of the chain we just failed to connect, considering that we only attempt to connect the block in the first place if it has more work than our tip. However, in a situation where m_best_header is on yet another chain, getchaintips behavior won't be improved.
Remove whitespace that doesn't conform with pep8 and turn some comments into log messages.
mzumsande
force-pushed
the
202405_invalid_chains
branch
from
2976f3a to
d7409a7
Compare
If we have accepted the block header but failed to accept the block (because ContextualCheckBlock failed) we mark the header as BLOCK_FAILED_VALID. Now, we also add this header to m_failed_blocks, so that we now don't accept any headers building on a chain that includes the failed block (and therefore will never be valid). Before, we'd only do that if we accepted the block but failed while connecting it.
mzumsande
force-pushed
the
202405_invalid_chains
branch
from
d7409a7 to
04be77b
Compare
|
🚧 At least one of the CI tasks failed. HintsMake sure to run all tests locally, according to the documentation. The failure may happen due to a number of reasons, for example:
Leave a comment here, if you need help tracking down a confusing failure. |
|
CI failure seems unrelated (#29897) |
4 participants
Some improvements with respect to invalid blocks and blocks building on them:
1.) The fields
m_best_header(best header of a chain that is not known to be invalid, irrespective on whether we have the full block or not) andBLOCK_FAILED_CHILD(BlockStatus saying that a header descends from an invalid block) are populated on a best-effort basis and can sometimes be off. This is not too bad because critical consensus code doesn't rely on either of these, but it is kind of brittle, especially because this is not clearly documented. Document these limitations to prevent future bugs.In order to solve these issues completely, it would be necessary to add more places in which we walk over the entire block index (computationally expensive), and/or introduce a header-tip tracking similar to #12138. This PR attempts neither of these, but I'd be interested in opinions whether we should.
2.) The
getchaintipsRPC can fail to recognize chains building on invalid blocks (#8050) as invalid. Add a test for this and fix it for most cases (a comprehensive fix would require one of the larger changes listed above).3.) Behavior for chains building on invalid blocks depends on the point in validation at which the block was found to be invalid (which in turn depends on what's wrong with the block):
CheckBlock()we never mark the block as permanently invalid (rationale)AcceptBlock()fail, we mark the block as permanently invalid (BLOCK_FAILED_VALID) and don't save it to disk (but we still have the header!). We won't accept headers building directly on it, but will accept headers building indirectly (i.e. with other headers in between), which is not consistent.ConnectBlock()we mark the block as permanently invalid and won't accept any more headers building on it, directly or indirectly.Add test coverage for this behavior and change it such that for blocks found invalid during
AcceptBlock(), we won't accept indirect headers that build on them either.