validation: fix misleading checkblockindex comments #29299
Conversation
These exceptions are not related to situations specific to tests, but are required in general: Without the first check CheckBlockindex could fail for blocks where we only know the header. Without the second, it could fail when blocks are received out of order.
There is a designated section meant for the actual consistency checks, marked by a comment.
|
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. |
There was a problem hiding this comment.
Code review ACK 9819db4. Thanks for figuring this issue out and fixing it. Would suggest changing pr name from "improve comments" to "fix misleading comments" since previous comments were wrong about the reasons the conditions are needed.
I also think it would be good to split up the assert and make it stricter, so feel free to use the code from #29261 (comment) to use in the second commit, if that helps. (Would be good to keep the first commit unchanged because it's a direct and straightforward fix.)
|
ACK 9819db4 🌦 Show signatureSignature: |
|
Changed the title as suggested.
The suggestion looks good to me just by reading the code, but I want to test it more before including it. Will either update the PR in a few days, or alternatively (I don't mind either way) that could be a follow-up to this trivial doc-only PR. |
|
ACK 9819db4 |
The two assumptions there were described as test-only, which has led to confusion whether they should exist.
However, they are necessary in general, as the changed comment explains - without them, the check would fail everywhere where it is enabled.
The second commit moves this assert down to the other checks.
Closes #29261