Zcash: Review existing use of assert(); replace with another form of error handling if better suited to the context

[bitcartel]

In both inherited upstream code and our own.

http://blog.erratasec.com/2017/03/assert-in-hands-of-bad-coders.html?m=1

[daira]

The article rubs me up the wrong way because it repeats (and the author appears to uncritically believe) the myth about "1x" and "10x" coders. See https://leanpub.com/leprechauns for a debunking of this myth.

It's unproductive and unhelpful to blame "bad programmers" for uses of asserts that enable DoS or that arise from gaps in error checking. What exactly are programmers supposed to use instead? The answer is not exceptions; exceptions are just as likely to be unhandled and cause a DoS issue as asserts — even if they didn't have horrible interactions with threading. (Releasing locks when throwing out of a locked region is bad, because invariants may be left violated. Not releasing locks is also bad, since then you'll inevitably run into a deadlock the next time you attempt to acquire the lock. There are also more low-level gotchas like the one with throwing out of OpenMP parallel regions we ran into in libsnark. Speaking of which, enforcing a consistent assertion policy across dependencies is really hard.)

It is also easy to say that assertions should be distinct from error handling, but much harder to implement that distinction in practice. Error handling code is almost always less well tested, and it's often the case that errors are not detected immediately but cause state inconsistencies that then (if we're lucky) trigger an assertion. We've had several of those so far in Zcash 1.0.x. It is in practice better to abort than to continue with corrupted state, even if it enables a DoS attack, since the alternative might be vulnerability to a worse attack.

I have my own biases as to how error handling and assertion failures should work – basically: transparent language-level support for rolling back state, as described in https://github.com/noether-lang/noether/blob/master/doc/presentations/StrangeLoop2014/handling.pdf – but that approach is not feasible to implement in C++, even less for a codebase that heavily uses shared-state concurrency.

[daira]

Perhaps we need a topical meeting about error handling and use of assertions.

[bitcartel]

I guess the title of this ticket could really have been 'review existing use of assert() to see if appropriate and replace with another form of error handling if better suited given the context'.