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) and BLOCK_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 getchaintips RPC 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):
- During the initial context-free
CheckBlock()we never mark the block as permanently invalid (rationale) - If the contextual checks in
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. - During
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.
