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

[maflcko]

This restores the check removed in #17785 (comment)

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

[practicalswift]

Concept ACK

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

[maflcko]

DABORT_ON_FAILED_ASSUME is taken from #16136 by @practicalswift

[naumenkogs]

utACK fa1bcec

[practicalswift]

ACK fab9363: patch looks correct

[naumenkogs]

ACK fab9363

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

[jnewbery]

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.

[maflcko]

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.

[hebasto]

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

[jnewbery]

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.

[maflcko]

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

Please review #20255 first

[practicalswift]

cr ACK fa0f415: patch looks correct

[jnewbery]

utACK fa0f415