validation: Improve, document and test logic for chains building on invalid blocks

mzumsande commented at 8:20 pm on May 30, 2024: contributor

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.

doc: Caution against relying on m_best_header and BLOCK_FAILED_CHILD

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.

0bf3be014a

test: add test for getchaintips behavior with invalid chains 8e7877fd43

validation: mark blocks between m_best_header and invalid block as BLOCK_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.

ae4fd3121c

test: cleanup rpc_getchaintips.py

Remove whitespace that doesn't conform with pep8 and turn some
comments into log messages.

d010febbe7

DrahtBot commented at 8:20 pm on May 30, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	furszy, ismaelsadeeq

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29790 ([DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system – WIP by hebasto)

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.

DrahtBot added the label Validation on May 30, 2024

mzumsande force-pushed on May 30, 2024

DrahtBot added the label CI failed on May 30, 2024

DrahtBot commented at 8:25 pm on May 30, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is 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.

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

Debug: https://github.com/bitcoin/bitcoin/runs/25622591015

furszy commented at 8:31 pm on May 30, 2024: member

Concept ACK

test: add test for chains building on invalid blocks f0f6c0b245

validation: add to m_failed_blocks if found invalid during AcceptBlock

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.

2976f3a8dc

mzumsande force-pushed on May 30, 2024

DrahtBot removed the label CI failed on May 31, 2024

ismaelsadeeq commented at 9:43 pm on June 6, 2024: member

Concept ACK

validation: Improve, document and test logic for chains building on invalid blocks #30207

Code Coverage

Reviews

Conflicts