# error "Bitcoin cannot be compiled without assertions." <<<<NOT
13 messages, 3 to 7 Jun 2014. Started by Ron.
Mike Hearn (3), Wladimir J. van der Laan (3), Greg Maxwell (2), Jeff Garzik (2), Ron, Jannis Froese, Pieter Wuille
Hello
What is the issue with the Bitcoin code for 0.9.x with regard to assertions that isn't in 0.8.6 or previous releases?
on April 18th, I offered
https://github.com/bc4-old-c-coder/bitcoin/commit/f0d221e56a12947b67b9c8f43cc5832b665052c8
this commit and code with all side effects removed from the assertions.
Then on the 28th,
https://github.com/bc4-old-c-coder/bitcoin/tree/0.8.6
this code with unit tests working.
And if that isn't enough, I did a video series on building Bitcoind.exe and the static libraries (on MSVC++) all in NDEBUG (release) mode.
See https://www.youtube.com/playlist?list=PLFnWb0ttBBMLyUuniLp3PJ5Mn4tVUlliZ Notice that the NDEBUG release mode is featured, and I even run it!
Lastly what does that say about building Bitcoin-qt in release mode? Should one or not??
There is also a video on building an alternate coin-qt.exe in release mode (gcc version) and running it! See https://www.youtube.com/watch?v=C8GvHpjbAnM
assert() should have no side effects, that is the problem.
a great book, BTW. Everyone who thinks they know what they are doing when they write C++ should read this book! They will realize that they don't know Jack
Why weren't these and all the other examples of amateur, i.e., non-professional, software fixed way back in version 0.3.0 in 2010, before any more releases were done? And why were these and other sub-standard coding practices continued and expanded in later releases, right up until the present?
Ron
Hi Ron,
FYI your mail is being spamfoldered due to Yahoo's DMARC policy and the brokenness of the SF.net mailing list software. I would not expect to get replies reliably whilst this is the case. I think we should move away from SF.net for hosting mailing lists personally, because it's this list that's at fault not Yahoo, but until then you may wish to send to the list with a different email address.
As to your question,
assert() should have *no* side effects, that is the problem.
Quoted text (15 lines)
> > See > > http://books.google.com/books?id=L5ZbzVnpkXAC&pg=PA72&lpg=PA72&dq=Gotcha+%2328+Side+Effects&source=bl&ots=Rn15TlPmje&sig=tymHqta0aSANwaM2GaXC-1Di_tk&hl=en&sa=X&ei=uVKNU47fCcvTsAT6goHIBA&ved=0CCAQ6AEwAA#v=onepage&q=Gotcha%20%2328%20Side%20Effects&f=false > > a great book, BTW. Everyone who thinks they know what they are doing when > they write C++ should read this book! They will realize that they don't > know Jack [image: Roll Eyes] > > Why weren't these and all the other examples of amateur, i.e., > non-professional, software fixed way back in version 0.3.0 in 2010, before > any more releases were done? And why were these and other sub-standard > coding practices continued and expanded in later releases, right up until > the present? >
Back in 2010 most code was still being written by Satoshi so perhaps you should ask him. Regardless, it's very common for professional codebases to require assertions be enabled. For example the entire Google C++ codebase uses always-on assertions that have side effects liberally: it's convenient and safe, when you have the guarantee the code will always run, and the performance benefits of compiling out assertions are usually non-existent.
So for this reason I think Bitcoin Core currently will fail to build if assertions are disabled, and that seems OK to me.
Quoted text (5 lines)
> > > assert() should have *no* side effects, that is the problem. >> >
I'm pretty sure that all the side effects of assertions have been removed before 0.9.0.
However, the assertion checks are extremely important to the proper sanity of the client and network, so IMHO it's fair to still require building with them enabled.
Wladimir
On Wed, Jun 4, 2014 at 2:51 AM, Mike Hearn <mike@plan99•net> wrote:
Quoted text (38 lines)
> Hi Ron, > > FYI your mail is being spamfoldered due to Yahoo's DMARC policy and the > brokenness of the SF.net mailing list software. I would not expect to get > replies reliably whilst this is the case. I think we should move away from > SF.net for hosting mailing lists personally, because it's this list that's > at fault not Yahoo, but until then you may wish to send to the list with a > different email address. > > As to your question, > > assert() should have *no* side effects, that is the problem. >> >> See >> >> http://books.google.com/books?id=L5ZbzVnpkXAC&pg=PA72&lpg=PA72&dq=Gotcha+%2328+Side+Effects&source=bl&ots=Rn15TlPmje&sig=tymHqta0aSANwaM2GaXC-1Di_tk&hl=en&sa=X&ei=uVKNU47fCcvTsAT6goHIBA&ved=0CCAQ6AEwAA#v=onepage&q=Gotcha%20%2328%20Side%20Effects&f=false >> >> a great book, BTW. Everyone who thinks they know what they are doing >> when they write C++ should read this book! They will realize that they >> don't know Jack [image: Roll Eyes] >> >> Why weren't these and all the other examples of amateur, i.e., >> non-professional, software fixed way back in version 0.3.0 in 2010, before >> any more releases were done? And why were these and other sub-standard >> coding practices continued and expanded in later releases, right up until >> the present? >> > > Back in 2010 most code was still being written by Satoshi so perhaps you > should ask him. Regardless, it's very common for professional codebases to > require assertions be enabled. For example the entire Google C++ codebase > uses always-on assertions that have side effects liberally: it's convenient > and safe, when you have the guarantee the code will always run, and the > performance benefits of compiling out assertions are usually non-existent. > > So for this reason I think Bitcoin Core currently will fail to build if > assertions are disabled, and that seems OK to me. >
As a matter of procedure we do not use assertions with side effects— the codebase did at one point, but have cleaned them up. In an abundance of caution we also made it refuse to compile without assertions enabled: A decision who's wisdom was clearly demonstrated when not long after, some additional side-effect having assert was contributed. In the real world errors happen here and there, and making robust software involves defense in depth.
Considering the normal criticality of the software it should always be with the assertions. Without them is an untested configuration. It would probably be superior to use our own assertion macros (for one, they can give some better reporting…) that don't have the baggage ordinary assertions have, but as a the codebase is a production thing, making larger changes all at once to satisfy aesthetics would be unwise... simply refusing to compile in that untested, unsupported configuration is prudent, for the time being.
Quoted text (9 lines)
> > As a matter of procedure we do not use assertions with side effects— the > codebase did at one point, but have cleaned them up. In an abundance of > caution we also made it refuse to compile without assertions enabled: A > decision who's wisdom was clearly demonstrated when not long after, some > additional side-effect having assert was contributed. In the real world > errors happen here and there, and making robust software involves defense > in depth. >
I think this class of errors could be removed entirely by just saying it's OK for assertions to have side effects and requiring them to be enabled, as is currently done.
The glog library:
http://google-glog.googlecode.com/svn/trunk/doc/glog.html
provides CHECK macros that print stack traces when they fail. Using them would also be good.
On Wed, Jun 4, 2014 at 3:20 AM, Mike Hearn <mike@plan99•net> wrote:
Quoted text (19 lines)
>> As a matter of procedure we do not use assertions with side effects— the >> codebase did at one point, but have cleaned them up. In an abundance of >> caution we also made it refuse to compile without assertions enabled: A >> decision who's wisdom was clearly demonstrated when not long after, some >> additional side-effect having assert was contributed. In the real world >> errors happen here and there, and making robust software involves defense in >> depth. > > > I think this class of errors could be removed entirely by just saying it's > OK for assertions to have side effects and requiring them to be enabled, as > is currently done. > > The glog library: > > http://google-glog.googlecode.com/svn/trunk/doc/glog.html > > provides CHECK macros that print stack traces when they fail. Using them > would also be good.
Yes... it takes only about 10 lines of code to have a nicer assert than the posix one, all my own software does... and with the noreturn attribute on the failure path it behaves the same for most static analysis tools as a regular assert does. I would have just dropped one in, but an IFDEF seemed more prudent at the time.
There are reasons to have assertions enabled by default in software like Bitcoin Core, where incorrect behaviour can be costly. But this comes at a prize: our assertions have to satisfy certain performance requirements. It's no longer possible to do expensive, redundant checks in performance critical code, which is one of the main advantages of asserts. Imho, asserts are not intended for small range checks etc, but are meant for checks that a hash hasn't changed, that a tree structure is still a tree, that data is still sorted, or that data structures are in sync.
I think most concerns about the current use of asserts would be resolved if the currently used asserts would be changed to a nicer definition which is independent of NDEBUG, and a second class of debugging asserts would be introduced, which is exclusively for expensive, redundant checks and is disabled by NDEBUG.
Am 2014-06-04 12:15, schrieb Gregory Maxwell:
Quoted text (75 lines)
> On Wed, Jun 4, 2014 at 2:51 AM, Mike Hearn <mike@plan99•net > <mailto:mike@plan99.net>> wrote: > > Hi Ron, > > FYI your mail is being spamfoldered due to Yahoo's DMARC policy > and the brokenness of the SF.net mailing list software. I would > not expect to get replies reliably whilst this is the case. I > think we should move away from SF.net for hosting mailing lists > personally, because it's this list that's at fault not Yahoo, but > until then you may wish to send to the list with a different email > address. > > As to your question, > > assert() should have *no* side effects, that is the problem. > > See > http://books.google.com/books?id=L5ZbzVnpkXAC&pg=PA72&lpg=PA72&dq=Gotcha+%2328+Side+Effects&source=bl&ots=Rn15TlPmje&sig=tymHqta0aSANwaM2GaXC-1Di_tk&hl=en&sa=X&ei=uVKNU47fCcvTsAT6goHIBA&ved=0CCAQ6AEwAA#v=onepage&q=Gotcha%20%2328%20Side%20Effects&f=false > > a great book, BTW. Everyone who thinks they know what they > are doing when they write C++ should read this book! They > will realize that they don't know Jack Roll Eyes > > Why weren't these and all the other examples of amateur, i.e., > non-professional, software fixed way back in version 0.3.0 in > 2010, before any more releases were done? And why were these > and other sub-standard coding practices continued and expanded > in later releases, right up until the present? > > > Back in 2010 most code was still being written by Satoshi so > perhaps you should ask him. Regardless, it's very common for > professional codebases to require assertions be enabled. For > example the entire Google C++ codebase uses always-on assertions > that have side effects liberally: it's convenient and safe, when > you have the guarantee the code will always run, and the > performance benefits of compiling out assertions are usually > non-existent. > > So for this reason I think Bitcoin Core currently will fail to > build if assertions are disabled, and that seems OK to me. > > > As a matter of procedure we do not use assertions with side effects--- > the codebase did at one point, but have cleaned them up. In an > abundance of caution we also made it refuse to compile without > assertions enabled: A decision who's wisdom was clearly demonstrated > when not long after, some additional side-effect having assert was > contributed. In the real world errors happen here and there, and > making robust software involves defense in depth. > > Considering the normal criticality of the software it should always be > with the assertions. Without them is an untested configuration. It > would probably be superior to use our own assertion macros (for one, > they can give some better reporting...) that don't have the baggage > ordinary assertions have, but as a the codebase is a production thing, > making larger changes all at once to satisfy aesthetics would be > unwise... simply refusing to compile in that untested, unsupported > configuration is prudent, for the time being. > > > > ------------------------------------------------------------------------------ > Learn Graph Databases - Download FREE O'Reilly Book > "Graph Databases" is the definitive new guide to graph databases and their > applications. Written by three acclaimed leaders in the field, > this first edition is now available. Download your free book today! > http://p.sf.net/sfu/NeoTech > > > _______________________________________________ > Bitcoin-development mailing list > Bitcoin-development@lists•sourceforge•net > https://lists.sourceforge.net/lists/listinfo/bitcoin-development
Currently expensive checks are guarded with command line flags. It'd be nice if there could be one unified command line flag -expensivechecks that subsumes -checkmempool and so on.
On Wed, Jun 4, 2014 at 6:42 PM, Jannis Froese <s9jafroe@stud•uni-saarland•de
Quoted text (107 lines)
> wrote: > There are reasons to have assertions enabled by default in software like > Bitcoin Core, where incorrect behaviour can be costly. But this comes at a > prize: our assertions have to satisfy certain performance requirements. > It's no longer possible to do expensive, redundant checks in performance > critical code, which is one of the main advantages of asserts. Imho, > asserts are not intended for small range checks etc, but are meant for > checks that a hash hasn't changed, that a tree structure is still a tree, > that data is still sorted, or that data structures are in sync. > > I think most concerns about the current use of asserts would be resolved > if the currently used asserts would be changed to a nicer definition which > is independent of NDEBUG, and a second class of debugging asserts would be > introduced, which is exclusively for expensive, redundant checks and is > disabled by NDEBUG. > > > > Am 2014-06-04 12:15, schrieb Gregory Maxwell: > > On Wed, Jun 4, 2014 at 2:51 AM, Mike Hearn <mike@plan99•net> wrote: > >> Hi Ron, >> >> FYI your mail is being spamfoldered due to Yahoo's DMARC policy and the >> brokenness of the SF.net mailing list software. I would not expect to get >> replies reliably whilst this is the case. I think we should move away from >> SF.net for hosting mailing lists personally, because it's this list that's >> at fault not Yahoo, but until then you may wish to send to the list with a >> different email address. >> >> As to your question, >> >> assert() should have *no* side effects, that is the problem. >>> >>> See >>> >>> http://books.google.com/books?id=L5ZbzVnpkXAC&pg=PA72&lpg=PA72&dq=Gotcha+%2328+Side+Effects&source=bl&ots=Rn15TlPmje&sig=tymHqta0aSANwaM2GaXC-1Di_tk&hl=en&sa=X&ei=uVKNU47fCcvTsAT6goHIBA&ved=0CCAQ6AEwAA#v=onepage&q=Gotcha%20%2328%20Side%20Effects&f=false >>> >>> a great book, BTW. Everyone who thinks they know what they are doing >>> when they write C++ should read this book! They will realize that they >>> don't know Jack [image: Roll Eyes] >>> >>> Why weren't these and all the other examples of amateur, i.e., >>> non-professional, software fixed way back in version 0.3.0 in 2010, before >>> any more releases were done? And why were these and other sub-standard >>> coding practices continued and expanded in later releases, right up until >>> the present? >>> >> >> Back in 2010 most code was still being written by Satoshi so perhaps >> you should ask him. Regardless, it's very common for professional codebases >> to require assertions be enabled. For example the entire Google C++ >> codebase uses always-on assertions that have side effects liberally: it's >> convenient and safe, when you have the guarantee the code will always run, >> and the performance benefits of compiling out assertions are usually >> non-existent. >> >> So for this reason I think Bitcoin Core currently will fail to build if >> assertions are disabled, and that seems OK to me. >> > > As a matter of procedure we do not use assertions with side effects— the > codebase did at one point, but have cleaned them up. In an abundance of > caution we also made it refuse to compile without assertions enabled: A > decision who's wisdom was clearly demonstrated when not long after, some > additional side-effect having assert was contributed. In the real world > errors happen here and there, and making robust software involves defense > in depth. > > Considering the normal criticality of the software it should always be > with the assertions. Without them is an untested configuration. It would > probably be superior to use our own assertion macros (for one, they can > give some better reporting…) that don't have the baggage ordinary > assertions have, but as a the codebase is a production thing, making larger > changes all at once to satisfy aesthetics would be unwise... simply > refusing to compile in that untested, unsupported configuration is prudent, > for the time being. > > > > ------------------------------------------------------------------------------ > Learn Graph Databases - Download FREE O'Reilly Book > "Graph Databases" is the definitive new guide to graph databases and their > applications. Written by three acclaimed leaders in the field, > this first edition is now available. Download your free book today!http://p.sf.net/sfu/NeoTech > > > > _______________________________________________ > Bitcoin-development mailing listBitcoin-development@lists•sourceforge•nethttps://lists.sourceforge.net/lists/listinfo/bitcoin-development > > > > > ------------------------------------------------------------------------------ > Learn Graph Databases - Download FREE O'Reilly Book > "Graph Databases" is the definitive new guide to graph databases and their > applications. Written by three acclaimed leaders in the field, > this first edition is now available. Download your free book today! > http://p.sf.net/sfu/NeoTech > _______________________________________________ > Bitcoin-development mailing list > Bitcoin-development@lists•sourceforge•net > https://lists.sourceforge.net/lists/listinfo/bitcoin-development > >
On Wed, Jun 4, 2014 at 12:42 PM, Jannis Froese < s9jafroe@stud•uni-saarland•de> wrote:
Quoted text (6 lines)
> I think most concerns about the current use of asserts would be resolved > if the currently used asserts would be changed to a nicer definition which > is independent of NDEBUG, and a second class of debugging asserts would be > introduced, which is exclusively for expensive, redundant checks and is > disabled by NDEBUG. >
Sounds good to me.
Wladimir
Yes, check macros like that can be useful.
I like the kernel's policy, which parallels our direction: 1) Enable and use lightweight assertions for most users. 2) No assertions with side effects
If you want to compile them out, that's fine, but they should always be present in production software.
On Wed, Jun 4, 2014 at 6:20 AM, Mike Hearn <mike@plan99•net> wrote:
Quoted text (31 lines)
>> As a matter of procedure we do not use assertions with side effects— the >> codebase did at one point, but have cleaned them up. In an abundance of >> caution we also made it refuse to compile without assertions enabled: A >> decision who's wisdom was clearly demonstrated when not long after, some >> additional side-effect having assert was contributed. In the real world >> errors happen here and there, and making robust software involves defense in >> depth. > > > I think this class of errors could be removed entirely by just saying it's > OK for assertions to have side effects and requiring them to be enabled, as > is currently done. > > The glog library: > > http://google-glog.googlecode.com/svn/trunk/doc/glog.html > > provides CHECK macros that print stack traces when they fail. Using them > would also be good. > > ------------------------------------------------------------------------------ > Learn Graph Databases - Download FREE O'Reilly Book > "Graph Databases" is the definitive new guide to graph databases and their > applications. Written by three acclaimed leaders in the field, > this first edition is now available. Download your free book today! > http://p.sf.net/sfu/NeoTech > _______________________________________________ > Bitcoin-development mailing list > Bitcoin-development@lists•sourceforge•net > https://lists.sourceforge.net/lists/listinfo/bitcoin-development >
On Wed, Jun 4, 2014 at 12:42 PM, Jannis Froese < s9jafroe@stud•uni-saarland•de> wrote:
I think most concerns about the current use of asserts would be resolved if
Quoted text (5 lines)
> the currently used asserts would be changed to a nicer definition which is > independent of NDEBUG, and a second class of debugging asserts would be > introduced, which is exclusively for expensive, redundant checks and is > disabled by NDEBUG. >
Also, most assertion errors that happen to people running Bitcoin Core are not caused by software bugs but database corruption errors (usually due to unclean shutdown).
For example in case we detect missing/truncated block files or UTXO db consistency we should, instead of raising an assertion error, propose a -reindex - see also https://github.com/bitcoin/bitcoin/issues/2202 .
So instead of using assertions we need a fatal error function for those problems which are probably recoverable in a certain specific way. In principle starting a reindex wouldn't even need to take down the entire process (though that's easier for implementation due to cleanup and assumptions made).
Wladimir
On Fri, Jun 6, 2014 at 10:29 AM, Wladimir <laanwj@gmail•com> wrote:
Quoted text (8 lines)
> On Wed, Jun 4, 2014 at 12:42 PM, Jannis Froese > <s9jafroe@stud•uni-saarland•de> wrote: > >> I think most concerns about the current use of asserts would be resolved >> if the currently used asserts would be changed to a nicer definition which >> is independent of NDEBUG, and a second class of debugging asserts would be >> introduced, which is exclusively for expensive, redundant checks and is >> disabled by NDEBUG.
There are a few examples of things that would classify as expensive/redundant checks: * addrman consistency checks (only enabled with -DDEBUG_ADDRMAN). * mempool consistency checks (only enabled with -checkmempool). * deadlock detection (only enabled with -DDEBUG_LOCKORDER).
I'm not sure all of these make sense to put under a single runtime flag. For example, addrman consistency is unlikely to be affected unless you're working on addrman code, and is pretty expensive.
Still, I do like the idea of optional consistency checks, that help guarantee the software always has a consistency state.
Speaking very generally, the Linux kernel wisdom on this tends to be,
* Compile in as many cheap, compiler-predictable asserts as possible into the production runtime. * Debug builds are of limited value. Users do not recompile software, just to provide better bug reports/diagnostics. * Make it as easy as possible for users to send reports that are useful to programmers. * Expensive diagnostics are fine. Compile in, but disable by default at runtime (and make sure these features, when turned off, do not slow down the system). * Make sure the assert/dump provides a high level of diagnostics. Stack trace of each thread + multi-threaded core dump are a good start.