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

1. 
   sipa commented at 1:36 pm on July 23, 2014: member

   Currently the Bitcoin source code relies on assertions (because they may have side effects), and some compile-time and runtime settable consistency checks.

   This leads to a few odditiies:

   • Failing to build with -NDEBUG
   • Uncertainty what performance impact consistency checks have.
   • Conflict between the ‘asserts are good because it tests the assumptions you’re relying on hold’/‘better fail than have undefined behavior’ and ‘asserts are bad because if they’re exploitable they’re potentially a massive DoS to the network’.

   This leads to only limited assert usage (because you don’t want them for anything potentially exploitable), and a few very expensive optional consistency checks (-checkmempool, DEBUG_LOCKORDER, …).

   My proposal:

   • Add a -checks command-line flag which enables inexpensive consistency checks.
   • Enable -checks by default in debug builds, but not in release/gitian builds.
   • Add a safe CHECK() macro which
     • always evaluates its arguments
     • is a no-op without -checks but like assert() with -checks.
   • Get rid of all assert()s.
2. 
   laanwj commented at 1:48 pm on July 23, 2014: member

   Sounds like a good idea to add an optional extra level of checking.

   But would the result of this be to disable all current checks by default in gitian builds?

3. 
   sipa commented at 2:12 pm on July 23, 2014: member

   I guess it’s a compromise. If there is a situation where we’d rather crash and risk having it being triggerable by peers, assert() (or some equivalent that does not depend on -checks) would be fine.
4. 
   laanwj commented at 2:15 pm on July 23, 2014: member

   I really don’t feel good about disabling all checks by default. A lot of corruption problems in the database, for example, come to light as exceptions. I think a base level of consistency checks in a program in which a sane state is so important like bitcoin should always be there.
5. 
   sipa commented at 2:18 pm on July 23, 2014: member

   Exceptions remain exceptions. They’re about dealing with exceptions runtime conditions, but about incorrect assumptions the source code makes (which is what assertions are about).
6. 
   sipa commented at 2:18 pm on July 23, 2014: member

   Oh, just to be clear: this whole issue is just about things that are currently assertions.

   I think there should be more of those, but they having them enabled in production systems can be a risk to the network.

7. 
   laanwj commented at 2:26 pm on July 23, 2014: member

   I’m also talking about assertions. A quick search of issues shows that problems (usually with the database) regularly are detected by assertions, not exceptions:

   #2821 Assertion mempool transaction missing input #2631 First time running, assertion failed (assertion in leveldb) #4097 Assertion failed in table/table_builder.cc:97 (assertion in leveldb) #3495 EXCEPTION: St9bad_alloc; assertion “pindex->pprev == view.GetBestBlock()” failed #4066 failed on open 64bit version

   If the current assertion checks are disabled by default the client would keep running with corrupted state, which may be harmful to the network or the user.

8. 
   jgarzik commented at 2:28 pm on July 23, 2014: contributor

   Related observations on assert, from working on other projects:

   • assert() tends to be a major sledgehammer, because it ends the program.
   • Programmers often make mistakes and add an assert for a runtime condition (such as out-of-disk-space). We have even seen this in bitcoin.
   • assert() should only ever be used for “should never happen” type conditions, where a crash or undefined behavior results should the assert otherwise be absent

   Observations on checks:

   • Yes. Yes. Goal should be to turn on as much inexpensive runtime checks as possible, always.
   • Related goal, make checks as inexpensive as possible, so as to not punish the user (and therefore encourage the user to want to turn off the checks).
   • Can use the compiler’s branch prediction hints (likely/unlikely macros)
   • Related, make debugging/diagnostic dumps easy for the user to trigger

   Cheap runtime checks are good. Asserts are bad.

9. 
   laanwj commented at 2:35 pm on July 23, 2014: member

   Related, make debugging/diagnostic dumps easy for the user to trigger

   Good idea, but only after the wallet is split off (or disabled). Currently there is the risk of private key data in memory (or even in registers) and it’s not safe to send dumps around. See also #2551.

10. 
    sipa commented at 2:37 pm on July 23, 2014: member

    Well, leveldb is separate in any case. I don’t suggest changing their assertion behavior.

    I’m very aware of the reasons why asserts exist, and why turning them off is bad. What I’m saying is that in production environments, it may be better to not risk blowing up the process due to an overeager check, as that automatically means a huge risk to the network if that assert is triggerable by a network condition. If it’s based on a relayable message, it may even wipe out the network entirely.

    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.

    I don’t have a good answer for what to do when things are clearly blowing up. Perhaps the checks that we are really sure about indicate irrecoverably bad state and really sure cannot be triggered by the network can remain asserts, but I’d rather see them turned off in production if that means we can have more checks in the code.

11. 
    laanwj commented at 2:42 pm on July 23, 2014: member

    What about - for example - asserts that check bounds? A DoS is bad, but disabling them may result in exploitable behaviour much worse than a DoS.

    So, whereas I agree with adding an extra level of expensive checks that is only enabled in debug builds, I don’t agree with making all current assertions/checks optional.

12. 
    sipa commented at 2:43 pm on July 23, 2014: member

    Fair enough. I guess we can have two levels, optional and mandatory checks.

    Encourage people to fill their code with optional checks, and maybe after a few releases, some optional ones can turn into mandatory ones.

13. 
    jgarzik commented at 2:56 pm on July 23, 2014: contributor

    Asserts are bad, in part, because people never bother to build with -DNDEBUG (the not-default). You do not want a path that is rarely built. Compile-time conditionals, in general, suck.

    You always want to compile as much code as possible. Disable at runtime if its expensive or painful, but always compile. Code that is not always compiled bitrots rapidly.

14. 
    laanwj commented at 3:13 pm on July 23, 2014: member

    @jgarzik Oh yes there is a danger in making ‘release builds’ and ‘debug builds’ diverge too much. I’ve seen that in game development. Everyone will test with the debug build, and then problems appear in the release build which no one has tested before the actual release :-) Also having a build that intentionally strips sanity checks makes troubleshooting harder. So yes, run-time instead of compile-time disabling is preferable where possible.

    So for critical checks we should have an ‘assert’ that ignores NDEBUG.

15. laanwj added the label Brainstorming on Jul 31, 2014
16. 
    practicalswift commented at 8:45 pm on June 2, 2019: contributor

    Suggested implementation submitted in #16136.
17. 
    MarcoFalke commented at 3:10 pm on March 29, 2022: member

    Closing for now, but feel free to continue discussion or reopen.
18. MarcoFalke closed this on Mar 29, 2022

    ---

19. DrahtBot locked this on Mar 29, 2023

Contributors
sipa laanwj jgarzik practicalswift MarcoFalke

Labels
Brainstorming