This restores the check removed in #17785 (review)
Instead of using error, which was used previously, it uses a newly introduced Assume(). error had several issues:
Concept ACK
While touching src/util/check.h, would you mind cherry-picking in 91bdd439987fb3f24f9b841fe88b3cf416b38f48 from #20122 to make Assert(…) usable in all contexts?
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
What’s the long-term plan for assert? Should all asserts eventually be changed to:
Assert() for fatal errors - always causes the software to terminateAssume() for recoverable errors - only causes the software to terminate if compiled with -enable-debug?Is it ok for Assert/Assume condition to have side effects? The reason NDEBUG builds are not allowed is that some of the assert conditions had side effects (#3344)
Perhaps it’d be a good idea to document this in the developer notes.
Should Assume() log to debug log (perhaps in a category) if hit in in non-debug build? It means there’s a logic bug which should be reported and fixed.
Excellent questions!
assert and Assert do almost the exact same thing and they can be used interchangeably, as long as the code compiles.
Existing asserts should not be replaced with Assume, because asserts documents fatal checks. For example, the program should not continue when running into UB. (Assert(pindex)->nHeight can not continue if the assert fails, so Assume would be inappropriate).
Yes, I’d say so. I can’t imagine anyone would want to run with asserts disabled, even if none of them had side-effects. For example, the consensus code uses asserts to catch internal logic errors. With those disabled, you might run out of consensus without noticing.
Will look into this.
Correct, there is likely a non-fatal logic bug which should be reported. Though, care should be taken to not turn it into a fatal one. E.g. a remote peer could trigger the Assume and cause an out-of-disk error by writing to the debug log. Also, we should take care to not induce anxiety into users when they hit a non-fatal issue, that generally doesn’t affect node operation. A specific debug log category for this could make sense, but I’ll leave that for a follow up.
The title of this PR should be changed to reflect that fact that most of the changes here are to introduce the Assume() macro, and it looks like the SetCommonVersion change is just an example of how to use Assume().
I think we should try to get agreement on how assert/Assert/Assume should be used, and document that before merging this.