Add an optional extra level of checking: ASSUME(...) - an opt-in side-effect safe assert(...) #16136

practicalswift · 2019-06-02T20:38:21Z

As suggested by sipa in the open issue #4576 (2014) -- "[discussion] Dealing with assertions and optional consistency checking" -- as an alternative to assert(…) in situations where assert(…) is not appropriate:

…
What I want is more of these checks, more as a way for the programmer to say "this is what I assume here", more than "if this doesn't hold here, we're in BIG trouble". It makes the code clearer, and simultaneously verifies that such assumptions hold. But only in cases where we're not at risk of hurting the network by dying.
…

ASSUME(expression) works like this:

If compiled with -DABORT_ON_FAILED_ASSUME (set by --enable-debug and/or --enable-fuzz): Evaluate expression and abort if expression is false.
If compiled without -DABORT_ON_FAILED_ASSUME: Evaluate expression and continue execution.

Example:

int main(void) {
    ASSUME(IsFoo());
     ...
}

If !IsFoo() and -DABORT_ON_FAILED_ASSUME, then:

    filename.cpp:123: main: ASSUME(IsFoo()) failed.
    Aborted

Otherwise the execution continues.

Resolves #4576.

ajtowns · 2019-06-03T01:40:48Z

If you're running the CHECK no matter what, could consider making the failure configurable at runtime (could default to not abort on mainnet, but to abort on testnet/regtest maybe?). Something like:

do {
     if (g_abort_on_check_fail) {
          fprintf(stderr, ...); std::abort();
     } else {
          LogPrintf(...);
     }
} while(0)

practicalswift · 2019-06-03T07:23:51Z

@ajtowns I like the idea of making the default depend on the network. I'll let others chime in regarding how to choose the defaults and how to override it and then update based on the feedback :-)

sdaftuar · 2019-06-06T15:51:39Z

Concept ACK -- I often want something that's like an assert but wouldn't cause cascading failure on the network if I'm wrong.

morcos · 2019-06-07T12:17:10Z

Concept ACK, but I also really like the idea of LogPrinting these (either no matter what or at least if some level of debug logging is on). Part of the benefit would be in catching rare conditions that might not occur in regular testing but would show up occasionally among thousands of real world users. The LogPrint could direct the user to report the error on GitHub.

maflcko · 2019-06-07T12:31:45Z

The link could even prefill all the additional debug information. E.g. https://github.com/bitcoin/bitcoin/issues/new?title=file_name.cpp:LINE_NR%20CHECK%20failed%20bla&body=more%20bla

Empact · 2019-06-10T02:27:00Z

Concept ACK

maflcko

Concept ACK if done right (with proper examples and documentation), but currently it seems to be confusing assert with CHECK.

And maybe it should be called ASSUME?

src/net_processing.cpp

src/check.h

ryanofsky · 2019-06-13T16:36:17Z

This change seems good, but I'd suggest calling the macro DCHECK instead of CHECK because:

It would suggest that the macro has different behavior in debug vs release builds.
It would allow us to create a corresponding CHECK macro in the future to replace our current non-standard usages of assert (in standard c++, asserts are compiled out of release builds, but not in bitcoin c++).
It would be more consistent with other support libraries (glog, folly) which use CHECK to abort unconditionally and DCHECK to only abort in debug builds.

For reference:

	Condition is compiled	Condition is evaluated	Program is aborted
C++ standard `assert`	if `NDEBUG` is unset	if `NDEBUG` is unset	if `NDEBUG` is unset
Bitcoin `assert`	always	always	always
glog `CHECK`	always	always	always
glog `DCHECK`	always	if `NDEBUG` is unset	if `NDEBUG` is unset
folly `XCHECK`	always	always	always
folly `XDCHECK`	if `NDEBUG` is unset	if `NDEBUG` is unset	if `NDEBUG` is unset

Re: #16136 (comment) and #16136 (comment) I think what Marco and Alex are asking for is useful but out of scope for an assert or check macro. I think asserts and checks are best for succinctly conveying assumptions made by the code author that in no cases should ever be violated, and conveying at a very coarse level how crucial those assumptions are (abort-worthy or not). The main point of asserts and checks should be to make code more readable and communicate how to think about it.

Once you're in the realm of conditions that you think should never happen, but are more complicated and might end up happening anyway, you are really better off writing an actual log print with text that would put the error into context. You may also want to abort in these cases, which is why logging frameworks support FATAL and DFATAL severity levels.

src/check.h

maflcko · 2019-10-10T18:06:00Z

Why was this closed?

practicalswift · 2019-10-10T18:28:42Z

@MarcoFalke I try to limit the number of PR:s I have opened and it seemed like this had a low probability of being merged. Seems I was wrong: now re-opened :)

practicalswift · 2019-10-10T18:48:47Z

Rebased and renamed back from DCHECK to the originally suggested name CHECK (as first suggested by @sipa in #4576).

I find DCHECK somewhat unintuitive: I prefer CHECK or ASSUME. Let's bikeshed! :)

ryanofsky · 2019-10-24T15:34:53Z

Rebased and renamed back from DCHECK to the originally suggested name CHECK (as first suggested by @sipa in #4576).

I find DCHECK somewhat unintuitive: I prefer CHECK or ASSUME. Let's bikeshed! :)

Not interested in bikeshedding and this is fine if other reviewers want it, but I'll just point out that as of b1eba55 this PR makes assert and CHECK mean the exact opposite things they mean in other C++ codebases, where CHECK aborts in all builds and assert aborts in debug builds only (examples in table above #16136 (comment)).

A good way to provide more clarity, I think, would be to follow convention used in other libraries and add a pair of CHECK/DCHECK macros instead of a single macro, avoiding use of any assert. An advantage of doing this is that both macros could do extra things like log to debug.log or capture stack traces before aborting, which aren't so easily possible with assert.

practicalswift · 2019-10-24T15:43:28Z

@ryanofsky What about ASSUME? :)

ajtowns

I like this, but am not sure it makes sense :)

src/assume.h

ajtowns · 2019-10-25T01:43:43Z

src/assume.h

+// Bitcoin Core is always compiled with assertions enabled. assert(...)
+// is thus only appropriate for documenting critical assumptions
+// where a failed assumption should lead to immediate abortion also
+// in production environments.


It's not really clear to me when you should choose assert vs ASSUME -- neither is any quicker because both always run the test, and if the assumptions you had when writing the code were wrong, how confident can you really be that continuing won't cause horrible corruption to decide that continuing is okay?

maflcko · 2019-10-28T16:35:16Z

Should be moved to the existing util/check.h

practicalswift · 2019-10-28T23:21:34Z

@MarcoFalke Good idea: addressed! Please re-review.

maflcko · 2019-10-29T12:52:39Z

Would it make sense to add a wrapper around our current assert, which always aborts when false (even in production, with debug disabled)?

We have vague error reports like #17298, that obviously lack details in the debug log, because logging wasn't enabled. Maybe an wrapper around our current assert that still aborts the program, but also dumps a full -debug=1 log for the last n (lets say 10 or 100) lines of debug log?

practicalswift · 2019-10-29T13:05:51Z

@MarcoFalke That could be nifty to have. I would like to keep this PR and ASSUME(…) as simple as possible, so I'll leave that for a future PR.

My suggestion is that we start with something super simple in and then we can iterate from that as we gain experience with the different use cases :)

src/util/check.h

…able-debug or --enable-fuzz.

maflcko · 2019-10-29T19:46:22Z

@MarcoFalke That could be nifty to have. I would like to keep this PR and ASSUME(…) as simple as possible, so I'll leave that for a future PR.

Fine, but at the very least we should decide on naming. I guess, given that this patch does not use the CHECK/DCHECK naming, your proposed naming would be ASSERT/ASSUME?

practicalswift · 2019-10-29T22:08:20Z

@MarcoFalke I think ASSERT and ASSUME are fine, but I'm happy to change that to whatever the consensus opinion is :)

I've encountered a few places where ASSUME would be handy when fuzzing: places where I want the program to abort when running under a fuzzer but where the error condition is not severe enough to warrant aborting in production. So for me getting the macro in is the important thing -- I can live with any name :)

maflcko · 2019-10-30T00:45:15Z

ACK 674f9d5, didn't look at the configure changes

practicalswift · 2020-03-10T08:12:14Z

Closing due to lack of interest :)

ajtowns · 2020-03-12T19:11:43Z

We have CHECK_NONFATAL(cond) in util/check.h now, which is always compiled, always evaluated, but only throws an exception rather than aborting the program. Probably best to see how that works for a while

maflcko · 2020-03-12T19:45:49Z

I think ASSUME serves a different use case than CHECK_NONFATAL, but it might be best to introduce it when it is actually used in the code.

maflcko · 2020-10-12T21:06:03Z

Revived in #20138 with an actual use-case (I hope)

hebasto · 2020-10-30T22:03:31Z

Revived in #20138 with an actual use-case (I hope)

Actually, #20255 :)

fanquake added the Build system label Jun 2, 2019

practicalswift mentioned this pull request Jun 2, 2019

[discussion] Dealing with assertions and optional consistency checking #4576

Closed

maflcko reviewed Jun 13, 2019

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/check.h Outdated Show resolved Hide resolved

src/check.h Outdated Show resolved Hide resolved

practicalswift force-pushed the check branch from d720388 to 00acb50 Compare June 13, 2019 20:20

practicalswift changed the title ~~Add an optional extra level of checking: CHECK(...) - an opt-in side-effect safe assert(...)~~ Add an optional extra level of checking: DCHECK(...) - an opt-in side-effect safe assert(...) Jun 13, 2019

practicalswift force-pushed the check branch 2 times, most recently from 02fd0cc to a32e448 Compare June 13, 2019 20:43

maflcko reviewed Jun 13, 2019

View reviewed changes

src/check.h Outdated Show resolved Hide resolved

src/check.h Outdated Show resolved Hide resolved

ryanofsky reviewed Jun 13, 2019

View reviewed changes

src/check.h Outdated Show resolved Hide resolved

practicalswift force-pushed the check branch from a32e448 to d7406f1 Compare June 13, 2019 21:34

maflcko reviewed Jun 14, 2019

View reviewed changes

src/check.h Outdated Show resolved Hide resolved

practicalswift force-pushed the check branch from d7406f1 to b2ac90b Compare June 14, 2019 21:15

ajtowns mentioned this pull request Jul 25, 2019

Add package acceptance logic to mempool #16401

Closed

practicalswift closed this Sep 19, 2019

maflcko mentioned this pull request Oct 10, 2019

util: Filter control characters out of log messages #17095

Merged

practicalswift reopened this Oct 10, 2019

practicalswift force-pushed the check branch from b2ac90b to b1eba55 Compare October 10, 2019 18:33

practicalswift changed the title ~~Add an optional extra level of checking: DCHECK(...) - an opt-in side-effect safe assert(...)~~ Add an optional extra level of checking: CHECK(...) - an opt-in side-effect safe assert(...) Oct 10, 2019

practicalswift force-pushed the check branch from b1eba55 to 7ab1498 Compare October 24, 2019 21:22

practicalswift changed the title ~~Add an optional extra level of checking: CHECK(...) - an opt-in side-effect safe assert(...)~~ Add an optional extra level of checking: ASSUME(...) - an opt-in side-effect safe assert(...) Oct 24, 2019

ajtowns reviewed Oct 25, 2019

View reviewed changes

practicalswift force-pushed the check branch 2 times, most recently from e006452 to 2812b0d Compare October 28, 2019 23:04

practicalswift mentioned this pull request Oct 29, 2019

Add assertion to randrange that input is not 0 #17293

Merged

maflcko reviewed Oct 29, 2019

View reviewed changes

src/util/check.h Outdated Show resolved Hide resolved

Add ASSUME(expr). Set -ABORT_ON_ASSUME_FAIL when configured with --en…

674f9d5

…able-debug or --enable-fuzz.

practicalswift force-pushed the check branch from 2812b0d to 674f9d5 Compare October 29, 2019 18:52

practicalswift mentioned this pull request Nov 19, 2019

util: Make logging noexcept #17514

Closed

practicalswift closed this Mar 10, 2020

This was referenced Apr 6, 2020

refactor: drop boost::signals2 in validationinterface #18524

Merged

wallet: Abort when assumption fails in debug mode #18555

Closed

maflcko mentioned this pull request Jun 15, 2020

util: Add Assert identity function #19277

Merged

maflcko mentioned this pull request Oct 12, 2020

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

Merged

jnewbery mentioned this pull request Oct 27, 2020

util: Add Assume() identity function #20255

Merged

practicalswift deleted the check branch April 10, 2021 19:39

bitcoin locked and limited conversation to collaborators Aug 15, 2022

Add an optional extra level of checking: ASSUME(...) - an opt-in side-effect safe assert(...) #16136

Add an optional extra level of checking: ASSUME(...) - an opt-in side-effect safe assert(...) #16136

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!