tests: Don't assert(...) with side effects #14088

Don't assert(...) with side effects.

From the developer notes:

Assertions should not have side-effects

Rationale: Even though the source code is set to refuse to compile with assertions disabled, having side-effects in assertions is unexpected and makes the code harder to understand

These assertions were introduced quite recently (in #14069 which was merged two days ago) and since this is a recurring thing (see #13534 – "Don't assert(foo()) where foo() has side effects" from May) I added a simple regression test for the most obvious common side effect.

utACK 1916b4e

Thanks :) utACK 1916b4e I don't know how I feel about the linter though - it is too narrowly scoped to ++|-- and won't catch anything inside of a BOOST_* style assert either.

If you're worried about somebody trying to run a node with NDEBUG - maybe add a more explanatory warning message https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L51 about why this could dangerous - instead of the very vague one that's there now?

@skeees Yes, the regression test will only catch this specific type of side effect. The regression test is not meant to be exhaustive (regression tests seldom are! :-)).

FWIW, this is the third time this specific type of side effect is fixed during the past few months so having a check for this is better than no checking :-)

@skeees Updated version based on your feedback with improved comments. Now catching also accidental assignment operations in assertions :-)

From PRE31-C (SEI CERT C Coding Standard):

Assertions should not contain assignments, increment, or decrement operators.

Now catching all three.

@skeees Regarding BOOST_ASSERT(...) – we're not using it are we? :-)

Cool - utACK - I think this change makes things strictly better. Regarding BOOST_CHECK_* stuff - used in unit tests but not production code I believe

I do think its worth changing the #error warning message (or adding a comment nearby) to something that explains why bitcoin cannot be compiled without assertions. Right now it just states so without any reason. Perhaps:

In addition to providing important sanity checks on the internal state of the system, is possible that assert statements throughout bitcoin may contain side-effecting code that is consensus critical, therefore bitcoin may not be compiled without assertions

@skeees BOOST_CHECK_* is ever used outside of the tests:

$ git grep BOOST_CHECK_ ":(exclude)*/test/*"
$

And I don't see any scenario where BOOST_CHECK_* could be removed at run-time like assert(…) under NDEBUG. Do you? :-)

utACK ca1a093127c11bb2aea10bf96c38dbfb40f8d170

And I don't see any scenario where BOOST_CHECK_* could be removed at run-time like assert(…) under NDEBUG. Do you? :-)

No—the problem does not exist for BOOST_CHECK_*, leave those alone please.