net: Assume that SetCommonVersion is called at most once per peer #20138

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:

• It logs unconditionally to the debug log
• It doesn't abort the program when the error is hit in tests

Concept ACK

While touching src/util/check.h, would you mind cherry-picking in 91bdd439987fb3f24f9b841fe88b3cf416b38f48 from #20122 to make Assert(…) usable in all contexts?

DABORT_ON_FAILED_ASSUME is taken from #16136 by @practicalswift

utACK fa1bcec498074de6f6780dc14e8cacaa6f951bb2

ACK fab93637867217266a810794d179487bb1eb8c69: patch looks correct

ACK fab9363

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #20477 (test/net: Add unit testing of node eviction logic by practicalswift)
• #19972 ([draft] fuzz/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable. by practicalswift)
• #18261 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)

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 terminate
• Assume() 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!

• What's the long-term plan for assert?

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).

• Is it ok for Assert/Assume condition to have side effects?

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.

• Perhaps it'd be a good idea to document this in the developer notes.

Will look into this.

• 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.

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.

ACK fab93637867217266a810794d179487bb1eb8c69, I have reviewed the code and it looks OK, I agree it can be merged.

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.

Thanks! Added dev docs and split out, as requested by @jnewbery

Please review #20255 first

cr ACK fa0f4157098ea68169ced44730986d0ed2c3a5aa: patch looks correct

utACK fa0f4157098ea68169ced44730986d0ed2c3a5aa