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 #1: this 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 #2: line 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.