Wednesday, March 15, 2017

Assert() in the hands of bad coders

Using assert() creates better code, as programmers double-check assumptions. But only if used correctly. Unfortunately, bad programmers tend to use them badly, making code worse than if no asserts were used at all. They are a nuanced concept that most programmers don't really understand.

We saw this recently with the crash of "Bitcoin Unlimited", a version of Bitcoin that allows more transactions. They used an assert() to check the validity of input, and when they received bad input, most of the nodes in the network crashed.

The Bitcoin Classic/Unlimited code is full of bad uses of assert. The following examples are all from the file main.cpp.



Example #1this line of code:

  1.     if (nPos >= coins->vout.size() || coins->vout[nPos].IsNull())
  2.         assert(false); 

This use of assert is silly. The code should look like this:

  1.     assert(nPos < coins->vout.size());
  2.     assert(!coins->vout[nPos].IsNull());

This is the least of their problems. It understandable that as code ages, and things are added/changed, that odd looking code like this appears. But still, it's an example of wrong thinking about asserts. Among the problems this would cause is that if asserts were ever turned off, you'd have to deal with dead code elimination warnings in static analyzers.


Example #2line of code:

  1.     assert(view.Flush());

The code within assert is supposed to only read values, not change values. In this example, the Flush function changes things. Normally, asserts are only compiled into debug versions of the code, and removed for release versions. However, doing so for Bitcoin will cause the program to behave incorrectly, as things like the Flush() function are no longer called. That's why they put at the top of this code, to inform people that debug must be left on.

  1. #if defined(NDEBUG)
  2. # error "Bitcoin cannot be compiled without assertions."
  3. #endif


Example #3: line of code:

  1.     CBlockIndex* pindexNew = new CBlockIndex(block);
  2.     assert(pindexNew);

The new operator never returns NULL, but throws its own exception instead. Not only is this a misconception about what new does, it's also a misconception about assert. The assert is supposed to check for bad code, not check errors.


Example #4: line of code

  1.     BlockMap::iterator mi = mapBlockIndex.find(inv.hash);
  2.     CBlock block;
  3.     const Consensus::Params& consensusParams = Params().GetConsensus();
  4.     if (!ReadBlockFromDisk(block, (*mi).second, consensusParams))
  5.         assert(!"cannot load block from disk");

This is the feature that crashed Bitcoin Unlimited, and would also crash main Bitcoin nodes that use the "XTHIN" feature. The problem comes from parsing input (inv.hash). If the parsed input is bad, then the block won't exist on the disk, and the assert will fail, and the program will crash.

Again, assert is for checking for bad code that leads to impossible conditions, not checking errors in input, or checking errors in system functions.


Conclusion

The above examples were taken from only one file in the Bitcoin Classic source code. They demonstrate the typically wrong ways bad programmers use asserts. It'd be a great example to show students of programming how not to write bad code.

More generally, though, it shows why there's a difference between 1x and 10x programmers. 1x programmers, like those writing Bitcoin code, make the typical mistake of treating assert() as error checking. The nuance of assert is lost on them.




Updated to reflect that I'm refering to the "Bitcoin Classic" source code, which isn't the "Bitcoin Core" source code. However, all the problems above appear to also be problems in the Bitcoin Core source code.


8 comments:

Marcel Jamin said...

AFAIK there is no XTHIN feature in bitcoin core.

I'm pretty sure all serious code issues you find are unique to bitcoin forks like XT, Classic or now Unlimited.

IlPorticoDipinto said...

You are mistaken: the mentioned bad code practice is found in main.cpp of Bitcoin Core.

Alphonse Pace said...

Mistake #3 traces back to Satoshi's original commit:

https://sourceforge.net/p/bitcoin/code/1/tree//trunk/main.cpp

It's clear Satoshi was not an expert C++ programmer. But in this case, your already running out of memory and have bigger problems than trying to catch the wrong exception.

Gregory Maxwell said...
This comment has been removed by the author.
Gregory Maxwell said...
This comment has been removed by the author.
Gregory Maxwell said...

There is no Xthin in Bitcoin Core and never has been.. There is no main.cpp in Bitcoin Core anymore, in fact. If what you were looking at had 'xthin' is was no version of Bitcoin Core ever, and if it had a main.cpp it isn't a current version.

In your example #1:

I believe proposed change introduces a bug.. Assume nPos < coins->vout.size() false, then your next line accesses vout[nPos].

The content of the if could, instead, be moved into the assert. I expect if we look at the history of the code we'd see that the current structure follows logically. The current form is hardly unclear or harmful and your proposed change shows the risk in 'optimizing' well tested code.

As an aside-- this is not an example of treating an assert as error handling, either, as it is triggered on an irrecoverably corrupted state.

Example #2 view.Flush() is not an assert that currently exist in Bitcoin Core.

The warning does exist however-- for good reason: Though a serious effort is made to not do dumb things like you've described, the code is not tested that way. This software is security critical and a falsely firing assert is the least of things that could go wrong. Users shouldn't get a non-tested setup as a side effect of using an unusual build system. The rationale was given in the commit introducing the warning. https://github.com/bitcoin/bitcoin/commit/9b59e3bda8c137bff885db5b1f9150346e36e076

It specifically calls out some of the same points you make about assert misuse, and fixes a number of historical assert misuses.

Example #3: assert(pindexNew); -- indeed, that is unnecessary, but it's also harmless and goes back to the original codebase where it instead drove a pointless branch. It is also not an example of treating asserts as error handling: This is an impossible condition that would indicate an irrecoverable state.

Example #4: is Xthin only and there is nothing like that in Bitcoin Core.

I think it would be more correct of you to not attribute these errors to the developers of Bitcoin:

Bitcoin Unlimited is a recently created adversarial fork of the software created by people with no prior affiliation with the project. They have been widely criticized by Bitcoin developers by for their risky development practices.

Gregory Maxwell said...

I just noticed you have links to Bitcoin "Classic", there is nothing classic about the deceptively names Bitcoin Classic, it's another adversarial fork created a bit over a year ago which has now mostly been abandoned.

The Bitcoin project is at https://github.com/bitcoin

Siddharth Garg said...

Great post. Check my website on hindi stories at afsaana . Thanks!