util: Add Assume() identity function #20255

MarcoFalke commented at 6:29 PM on October 27, 2020: member

This is needed for #20138. Please refer to the added documentation for motivation.

laanwj commented at 7:09 PM on October 27, 2020: member

Concept ACK. It's mildly interesting that we're sort of reinventing the original assert() here, which gets disabled on NDEBUG, which we had to always force on, but I think it's fine to reinvent the wheel here instead of assuming any specific compiler semantics.

MarcoFalke commented at 7:14 PM on October 27, 2020: member

Indeed, this Assume is like the traditional assert.

jnewbery commented at 7:29 PM on October 27, 2020: member

Concept ACK. If you're willing to entertain paint color requests, I prefer the name Check(), as was originally proposed in #16136.

MarcoFalke commented at 7:36 PM on October 27, 2020: member

I think @ryanofsky also likes check, so I might change to that

jnewbery commented at 7:40 PM on October 27, 2020: member

I think Russ doesn't care about the name, but would prefer a pair of check/dcheck macros: #16136 (comment)

sipa commented at 7:43 PM on October 27, 2020: member

FWIW, libsecp256k1 has CHECK() (always enabled) and VERIFY_CHECK() functions (enabled with -DVERIFY, used in test builds).

DrahtBot added the label Build system on Oct 27, 2020

DrahtBot added the label Docs on Oct 27, 2020

DrahtBot added the label Utils/log/libs on Oct 27, 2020

practicalswift commented at 9:41 PM on October 27, 2020: contributor

Concept ACK

in doc/developer-notes.md:298 in fa96726035 outdated

 293 | +   - For example, a nullptr dereference or any other logic bug in RPC code
 294 | +     means that the RPC code is faulty and can not be executed. However, the
 295 | +     logic bug can be shown to the user and the program can continue to run.
 296 | +* `Assume` can be used for nonfatal internal logic bugs. In debug builds it
 297 | +  behaves like Assert()/assert() to notify developers and testers about
 298 | +  non-fatal errors. In production it doesn't warn or log anything.

ajtowns commented at 6:08 AM on October 28, 2020:

Might be worth clarifying that the expression is still evaluated in production builds?

MarcoFalke commented at 9:12 AM on November 24, 2020:

thx, done

in doc/developer-notes.md:304 in fa96726035 outdated

 297 | +  behaves like Assert()/assert() to notify developers and testers about
 298 | +  non-fatal errors. In production it doesn't warn or log anything.
 299 | +   - For example it can be assumed that a variable is only initialized once,
 300 | +     but a failed assumption does not result in a fatal bug. A failed
 301 | +     assumption may or may not result in a slightly degraded user experience,
 302 | +     but it is safe to continue program execution.

ajtowns commented at 7:01 AM on October 28, 2020:

Maybe rephrase it as a directive: "Assert() should be used to document assumptions when any violation would mean that it is not safe to continue program execution; Assume() should be used to document assumptions when program execution can safely continue even if the assumption is violated." ?

If we're going to stick with "always evaluate assertions" and not support NDEBUG-like optimisation even for a new, debug-only check, maybe we should deprecate assert() too (and perhaps not use it to implement Assert()).

MarcoFalke commented at 9:12 AM on November 24, 2020:

Maybe rephrase it as a directive

thx, done

maybe we should deprecate assert()

I'll leave that for a follow-up

MarcoFalke removed the label Build system on Oct 28, 2020

MarcoFalke removed the label Docs on Oct 28, 2020

MarcoFalke commented at 8:16 AM on October 28, 2020: member

FWIW, libsecp256k1 has CHECK() (always enabled) and VERIFY_CHECK() functions (enabled with -DVERIFY, used in test builds).

That is good to know, because that means we can't use that name due to name-clashing.

ajtowns commented at 4:44 AM on October 29, 2020: member

ACK fa9672603560320bc9f1017a7de5e85de10d9561

I think this is a good choice of behaviour that should be easy to use correctly. Would like to see the ability to turn on logging of violated Assumptions even in production builds in a followup.

practicalswift commented at 6:37 AM on October 29, 2020: contributor

ACK fa9672603560320bc9f1017a7de5e85de10d9561: patch looks correct

hebasto commented at 10:13 PM on October 30, 2020: member

Approach ACK fa9672603560320bc9f1017a7de5e85de10d9561

Are there any cases when assert is preferable to Assert. If "yes", it would be nice to have an example in the docs. If "no", why do we mention assert in the docs?

util: Allow Assert(...) to be used in all contexts

Fixes the compile error when used inside operator[]:

./chain.h:404:23: error: C++11 only allows consecutive left square brackets when introducing an attribute
        return (*this)[Assert(pindex)->nHeight] == pindex;
                      ^

fa861569dc

util: Add Assume() identity function fac5efe730

util: Remove probably misleading TODO

The TODO has been added by me, but I don't remember how to solve it. The
current code works fine, so just remove the TODO.

faa05854f8

MarcoFalke force-pushed on Nov 24, 2020

MarcoFalke commented at 9:13 AM on November 24, 2020: member

Are there any cases when assert is preferable to Assert

No

If "no", why do we mention assert in the docs?

It documents how the assert is used in existing code

practicalswift commented at 9:25 AM on November 24, 2020: contributor

cr ACK faa05854f832405231c9198787a4eafe2cd4c5f0

jnewbery commented at 9:35 AM on November 24, 2020: member

utACK faa05854f832405231c9198787a4eafe2cd4c5f0

in doc/developer-notes.md:294 in faa05854f8

 289 | +     code means the program code is faulty and must terminate immediately.
 290 | +* `CHECK_NONFATAL` should be used for recoverable internal logic bugs. On
 291 | +  failure, it will throw an exception, which can be caught to recover from the
 292 | +  error.
 293 | +   - For example, a nullptr dereference or any other logic bug in RPC code
 294 | +     means that the RPC code is faulty and can not be executed. However, the

jonatack commented at 9:42 AM on November 24, 2020:

s/can not/cannot/

in doc/developer-notes.md:301 in faa05854f8

 296 | +* `Assume` should be used to document assumptions when program execution can
 297 | +  safely continue even if the assumption is violated. In debug builds it
 298 | +  behaves like `Assert`/`assert` to notify developers and testers about
 299 | +  nonfatal errors. In production it doesn't warn or log anything, though the
 300 | +  expression is always evaluated.
 301 | +   - For example it can be assumed that a variable is only initialized once,

jonatack commented at 9:43 AM on November 24, 2020:

s/For example it can/For example, it may/

jonatack commented at 9:43 AM on November 24, 2020: member

Approach ACK, some doc nits if you retouch

MarcoFalke commented at 12:54 PM on November 25, 2020: member

Will leave style nits alone to preserve ACKs

MarcoFalke commented at 8:10 AM on December 4, 2020: member

I think this is ready, but I am waiting on one more (re?)ACK before merge

hebasto approved

hebasto commented at 10:05 AM on December 4, 2020: member

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

MarcoFalke merged this on Dec 4, 2020

MarcoFalke closed this on Dec 4, 2020

MarcoFalke deleted the branch on Dec 4, 2020

sidhujag referenced this in commit c67c618af7 on Dec 4, 2020

Fabcien referenced this in commit 12031e53db on Jun 15, 2021

DrahtBot locked this on Feb 15, 2022