bitcoin
bitcoin copied to clipboard
bitcoin
validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment
m_best_header (the most-work header not known to be on an invalid chain) can be wrong in the context of invalidation / reconsideration of blocks. This can happen naturally (a valid header is received and stored in our block tree db; when the full block arrives, it is found to be invalid) or triggered by the user with the invalidateblock / reconsiderblock rpc.
We don't currently use m_best_header for any critical things (see OP of #16974 for a list that still seems up-to-date), so it being wrong affects mostly rpcs.
This PR proposes to recalculate it if necessary by looping over the block index and finding the best header. It also suggest to mark headers between an invalidatetd block and the previous m_best_header as invalid, so they won't be considered in the recalculation.
It adds tests to rpc_invalidateblock.py and rpc_getchaintips.py that fail on master.
One alternative to this suggested in the past would be to introduce a continuous tracking of header tips (#12138). While this might be more performant, it is also more complicated, and situations where we need this data are only be remotely triggerable by paying the cost of creating a valid PoW header for an invalid block. Therefore I think it isn't necessary to optimise for performance here, plus the solution in this PR doesn't perform any extra steps in the normal node operation where no invalidated blocks are encountered.
Fixes #26245
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Code Coverage & Benchmarks
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30666.
Reviews
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | fjahr, TheCharlatan, achow101 |
| Approach ACK | jonatack |
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:
- #29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
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.
Thanks for the reviews so far! Ill address feedback soon, plus I want to rework 7d42b0e4c2722b398c73695d5547fbf8bd2ae01c, see comment above. I'm currently on holidays, so it might take a few days until I make the changes.
One more issue I've noticed while writing the tests this PR is that if we connect an invalid block received via p2p, InvalidChainFound() is called twice:
The first time in ConnectTip(), via InvalidBlockFound() here
The second time in ActivateBestChainStep() after ConnectTip() fails due to an invalid block (here).
Does anyone know if there is a reason for this? It seems to me that the call in ActivateBestChainStep() is unnecessary and could be dropped.
It also suggest to mark headers between an invalidatetd block and the previous m_best_header as invalid, so they won't be considered in the recalculation.
You mean if you invalidateblock, then reconsiderblock, the rest of the child blocks also need to be reconsidered individually? That seems like a conceptual bug IMO, backward incompatible, and quite annoying.
You mean if you invalidateblock, then reconsiderblock, the rest of the child blocks also need to be reconsidered individually?
That's already happening, reconsiderblock already goes over the entire block index db on master, looking at each index individually, see Chainstate::ResetBlockFailureFlags (this also has a bug, #30479, but that is unrelated to the failure flags).
- rebased
- addressed feedback
- reworked commit f44a403ad27297c10256df11c3876b27c58dc1e4 (fix m_best_header tracking and BLOCK_FAILED_CHILD ) to set
BLOCK_FAILED_CHILDfor all blocks building on the invalid block, instead of just the ancestors ofm_best_header
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30214184316
Hints
Make sure to run all tests locally, according to the documentation.
The failure may happen due to a number of reasons, for example:
-
Possibly 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.
-
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
-
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
actually, that didn't work because NotifyHeaderTip can't be called when cs_main is held. Reverting this, I'll think about this a bit more.
Is this ready for review or should we wait until you have addressed this @mzumsande ?
