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

practicalswift commented at 8:38 pm on June 2, 2019: contributor

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:

0int main(void) {
1    ASSUME(IsFoo());
2     ...
3}

If !IsFoo() and -DABORT_ON_FAILED_ASSUME, then:

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

Otherwise the execution continues.

Resolves #4576.

fanquake added the label Build system on Jun 2, 2019

ajtowns commented at 1:40 am on June 3, 2019: member

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 commented at 7:23 am on June 3, 2019: contributor

@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 commented at 3:51 pm on June 6, 2019: member

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 commented at 12:17 pm on June 7, 2019: member

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.

MarcoFalke commented at 12:31 pm on June 7, 2019: member

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 commented at 2:27 am on June 10, 2019: member

Concept ACK

in src/net_processing.cpp:1695 in d720388339 outdated

1691@@ -1691,6 +1692,7 @@ bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::ve
1692                 }
1693                 pindexWalk = pindexWalk->pprev;
1694             }
1695+            CHECK(pindexWalk);

MarcoFalke commented at 4:03 pm on June 13, 2019:

Looks like you are using this wrong. This should be an assert, since it is dereferenced later on.

practicalswift commented at 6:36 pm on June 13, 2019:

I agree it is not an optimal example: assert(…); would obviously be the more appropriate choice for cases where we want an unconditional abort if the assumption does not hold. I’ll remove this example.

in src/check.h:17 in d720388339 outdated

12+// CHECK(expression)
13+// =================
14+// * If compiled with -DABORT_ON_FAILED_CHECK (set by --enable-debug):
15+//   Evaluate expression and abort if expression is falsy.
16+//
17+// * If compiled without -DABORT_ON_FAILED_CHECK:

MarcoFalke commented at 4:04 pm on June 13, 2019:

Shorter without loss of precision:

0// * otherwise:

practicalswift commented at 8:21 pm on June 13, 2019:

Good point! Fixed!

in src/check.h:58 in d720388339 outdated

38+    } while (false)
39+#else
40+#define CHECK_FAILURE(expression, file, line, func) ((void)0)
41+#endif
42+
43+#define CHECK(expression)                                             \

MarcoFalke commented at 4:06 pm on June 13, 2019:

This is missing an explanation when to use it. How is it different from assert or std::logic_error? Where should it be used in net or in consensus/validation code?

practicalswift commented at 9:23 pm on June 13, 2019:

Added additional documentation. Please re-review :-)

MarcoFalke commented at 4:07 pm on June 13, 2019: member

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?

ryanofsky commented at 4:36 pm on June 13, 2019: member

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.

practicalswift force-pushed on Jun 13, 2019

practicalswift renamed this:
~~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(...)
on Jun 13, 2019

practicalswift force-pushed on Jun 13, 2019

in src/check.h:16 in a32e448b00 outdated

11+
12+// DCHECK(expression)
13+// ==================
14+// DCHECK(...) is used to document assumptions.
15+//
16+// Bitcoin is always compiled with assertions enabled. assert(...)

MarcoFalke commented at 9:07 pm on June 13, 2019:

0// Bitcoin Core is always compiled with assertions enabled. assert(...)

practicalswift commented at 9:35 pm on June 13, 2019:

Fixed!

in src/check.h:49 in a32e448b00 outdated

44+// Otherwise the execution continues.
45+
46+#ifdef ABORT_ON_FAILED_DCHECK
47+#define DCHECK_FAILURE(expression, file, line, func)       \
48+    do {                                                  \
49+        fprintf(stderr, "%s:%d: %s: DCHECK(%s) failed.\n", \

MarcoFalke commented at 9:08 pm on June 13, 2019:

0        tfm::format(std::cerr, "%s:%d: %s: DCHECK(%s) failed.\n", \

practicalswift commented at 9:35 pm on June 13, 2019:

Changed!

in src/check.h:60 in a32e448b00 outdated

54+#define DCHECK_FAILURE(expression, file, line, func) ((void)0)
55+#endif
56+
57+#define DCHECK(expression)                                             \
58+    do {                                                              \
59+        if (!(expression)) {                                          \

ryanofsky commented at 9:23 pm on June 13, 2019:

It’s possible I misunderstood part of the intent of this PR, but I don’t think the expression should be evaluated in non-debug builds. Few reasons:

It would encourage putting code that has external side effects in DCHECK macros, which would make it easier not to notice the effects of invoking code, and be bad for readability.
It would encourage general misuse of the DCHECK macro. Like I wrote in my earlier comment I think the best use of DCHECK is just to state basic assumptions that code makes so it is more readable, and to help catch simple bugs during development. It shouldn’t be used as lazy way to handle errors in release builds and avoid having to write readable error messages that put errors into context.
It could potentially make release code less efficient.
It would be inconsistent with other DCHECK implementations. Neither the glog nor folly DCHECK macros have side effects in release builds (see #16136 (comment)) .

My suggestion would be to rename ABORT_ON_FAILED_DCHECK to ENABLE_DCHECK and when it’s not defined either make DCHECK a no-op like folly, or make it compile the condition expression without executing it like glog.

practicalswift commented at 9:31 pm on June 13, 2019:

It seemed like the consensus in #4576 was to make the expression evaluated also in non-debug builds. FWIW, that is consistent with how CHECK(…) works in secp256k1.

I don’t have a strong opinion here: I’ll let others chime in and adjust the code to meet consensus.

ajtowns commented at 10:47 am on June 15, 2019:

Seems like clarifying the intent of the PR would be a good first step…

I think there’s four things we could have:

assert(chk) – always evaluate, always abort if it failes
CHECK(chk) – always evaluate, but only abort if in regtest or similar; maybe log errors if not aborting.
debug_assert(chk) – evaluate condition only when invoked with a special option like -debugchecks=1, which is always off by default, and can be disabled at compile time, allowing the code to dropped out. these checks could be slow and heavyweight. they probably wouldn’t be run in travis.
traditional_assert(chk) – evaluate for test builds, but don’t evaluate in release builds

I think the traditional_assert style probably makes test and release builds different in ways that are hard to ensure are correct (in Haskell or Rust maybe we could ensure that chk is side effect free via the type-system, but that doesn’t seem plausible in C++), so I don’t think that’s a good idea.

I could see any of the others being reasonable though.

practicalswift force-pushed on Jun 13, 2019

in src/check.h:51 in d7406f1d27 outdated

46+// Otherwise the execution continues.
47+
48+#ifdef ABORT_ON_FAILED_DCHECK
49+#define DCHECK_FAILURE(expression, file, line, func)              \
50+    do {                                                          \
51+        tfm::format(std::cerr, "%s:%d: %s: DCHECK(%s) failed.\n", \

MarcoFalke commented at 6:35 pm on June 14, 2019:

0        tfm::format(std::cerr, "%s:%d: %s: DCHECK(%s) failed. You may report this issue here: %s.\n", \
1        ...
2        PACKAGE_BUGREPORT)

practicalswift commented at 9:15 pm on June 14, 2019:

Added!

practicalswift force-pushed on Jun 14, 2019

practicalswift closed this on Sep 19, 2019

MarcoFalke commented at 6:06 pm on October 10, 2019: member

Why was this closed?

practicalswift reopened this on Oct 10, 2019

practicalswift commented at 6:28 pm on October 10, 2019: contributor

@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 force-pushed on Oct 10, 2019

practicalswift renamed this:
~~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(...)
on Oct 10, 2019

practicalswift commented at 6:48 pm on October 10, 2019: contributor

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 commented at 3:34 pm on October 24, 2019: member

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 b1eba5575b53dda9bb7765701d4a6305d07faa74 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 commented at 3:43 pm on October 24, 2019: contributor

@ryanofsky What about ASSUME? :)

ryanofsky commented at 4:18 pm on October 24, 2019: member

@ryanofsky What about ASSUME? :)

Seems fine, and I personally don’t care about the name. I’m trying to say that if you add a macro that aborts and reports errors in debug builds only, it would be good to have a corresponding macro that does the same thing in all builds, and that naming I’ve seen before for this is CHECK/DCHECK.

Taking a step back, the only reason I have interest in this PR is that I think current assert macro sucks, and that it’s be nice to have a better macro that can do things like log to debug.log, capture stack traces, suggest bug reporting, etc.

practicalswift force-pushed on Oct 24, 2019

practicalswift renamed this:
~~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(...)
on Oct 24, 2019

in src/assume.h:25 in 7ab1498652 outdated

20+// and is side-effect safe.
21+//
22+// More specifically:
23+//
24+// * If compiled with -DABORT_ON_FAILED_ASSUME (set by --enable-debug):
25+//   Evaluate expression and abort if expression is falsy.

ajtowns commented at 1:23 am on October 25, 2019:

“false”

in src/assume.h:53 in 7ab1498652 outdated

48+            file, line, func, expression, PACKAGE_BUGREPORT);                                        \
49+        std::abort();                                                                                \
50+    } while (false)
51+#else
52+#define ASSUME_FAILED(expression, file, line, func) ((void)0)
53+#endif

ajtowns commented at 1:33 am on October 25, 2019:

I’m still not sure it wouldn’t be better unconditionally log (independent of compile flags) something like %s:%d: ASSUME(%s) failed and change the testing harnesses to emit a failure if that happens? If we continue fine when running release versions anyway, wouldn’t it make more sense to capture all the failures while testing to reduce the number of restarts needed?

My inclination would be more along the lines of:

0if (!(expression)) {
1    LogPrintf("Assumption failed %s:%d: %s", __FILE__,__LINE__, #expression);
2    if (!Params().IsTestChain()) std::abort();
3}

(or have the std::abort() always happen if a compile time flag is set, if that’s helpful for static analysis)

MarcoFalke commented at 7:43 pm on October 29, 2019:

has been fixed, I believe

in src/assume.h:17 in 7ab1498652 outdated

12+// ASSUME(...) is used to document assumptions.
13+//
14+// Bitcoin Core is always compiled with assertions enabled. assert(...)
15+// is thus only appropriate for documenting critical assumptions
16+// where a failed assumption should lead to immediate abortion also
17+// in production environments.

ajtowns commented at 1:43 am on October 25, 2019:

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?

ajtowns commented at 1:45 am on October 25, 2019: member

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

MarcoFalke commented at 4:35 pm on October 28, 2019: member

Should be moved to the existing util/check.h

practicalswift force-pushed on Oct 28, 2019

practicalswift commented at 11:21 pm on October 28, 2019: contributor

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

MarcoFalke commented at 12:52 pm on October 29, 2019: member

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 commented at 1:05 pm on October 29, 2019: contributor

@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 :)

Add ASSUME(expr). Set -ABORT_ON_ASSUME_FAIL when configured with --enable-debug or --enable-fuzz. 674f9d59ba

in src/util/check.h:83 in 2812b0decf outdated

78+
79+#ifdef ABORT_ON_FAILED_ASSUME
80+#define ASSUME_FAILED(expression, file, line, func)                                                  \
81+    do {                                                                                             \
82+        tfm::format(std::cerr, "%s:%d: %s: ASSUME(%s) failed. You may report this issue here: %s\n", \
83+            file, line, func, expression, PACKAGE_BUGREPORT);                                        \

MarcoFalke commented at 1:13 pm on October 29, 2019:

Shouldn’t this be logged even if the debug flag is not set at compile time?

practicalswift commented at 1:23 pm on October 29, 2019:

That’s not how I intended it to work, but now that you suggest it there is perhaps no downside in logging it also in non-debug/non-fuzz mode? Since we’re only writing to stderr (and not to disk) it should be safe. I’ll update.

practicalswift force-pushed on Oct 29, 2019

MarcoFalke commented at 7:46 pm on October 29, 2019: member

@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 commented at 10:08 pm on October 29, 2019: contributor

@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 :)

MarcoFalke commented at 0:45 am on October 30, 2019: member

ACK 674f9d59ba4c1a1d4662e79467cc417a2988cf15, didn’t look at the configure changes

practicalswift closed this on Mar 10, 2020

practicalswift commented at 8:12 am on March 10, 2020: contributor

Closing due to lack of interest :)

ajtowns commented at 7:11 pm on March 12, 2020: member

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

MarcoFalke commented at 7:45 pm on March 12, 2020: member

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.

MarcoFalke commented at 9:06 pm on October 12, 2020: member

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

hebasto commented at 10:03 pm on October 30, 2020: member

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

Actually, #20255 :)

practicalswift deleted the branch on Apr 10, 2021

MarcoFalke locked this on Aug 15, 2022

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