Implement new policy for VERIFY_CHECK and #ifdef VERIFY (issue #1381)

[theStack]

As suggested in #1381, this PR reworks the policy for VERIFY_CHECK and when to use #ifdef VERIFY, by:

• redefining VERIFY_CHECK to empty in production (non-VERIFY) mode
• removing many then superflous #ifdef VERIFY blocks (if they exclusively contained VERIFY_CHECKs)
• introducing uppercase macros around verify_ functions and using them for better readabiliy

What is not included yet is the proposed renaming from "_check" to "_assert":

And while we're touching this anyway, we could consider renaming "check" to "assert", which is a more precise term. (In fact, if we redefine VERIFY_CHECK to be empty in production, we have almost reimplemented assert.h...)

This should be easy to achieve with simple search-and-replace (e.g. using sed), but I was hesitant as this would probably case annoying merge conflicts on some of the open PRs. Happy to add this if the rename if desired (#1381 didn't get any feedback about the renaming idea yet).

[theStack]

Rebased on master (since #1348 has been merged) and updated the PR description accordingly.

[theStack]

Rebased on master again and introduced an uppercase macro for secp256k1_scalar_verify as well (introduced in recently merged #1373).

[theStack]

Rebased on master (CI was failing previously).

[stratospher]

c723b85: what does VERIFY_SETUP do? it looks really similar to VERIFY_CHECK and used in only 2 places in the codebase.

[real-or-random]

The only two uses trace back to 72ae443 where the macro was introduced.

My thinking is that the purpose of VERIFY_SETUP is to wrap code whose only purpose is to perform side effects ("setup") for further VERIFY_CHECKs. But assuming this interpretation, I don't really understand 72ae443.

The effect of the two existing VERIFY_SETUP uses is that they initialize r->x and r->y but that's done anyway in the current code, immediately afterward:

secp256k1/src/ecmult_const_impl.h

Lines 90 to 95 in 5814d84

	VERIFY_SETUP(secp256k1_fe_clear(&(r)->x)); \
	VERIFY_SETUP(secp256k1_fe_clear(&(r)->y)); \
	/* Unconditionally set r->x = (pre)[m].x. r->y = (pre)[m].y. because it's either the correct one
	* or will get replaced in the later iterations, this is needed to make sure `r` is initialized. */ \
	(r)->x = (pre)[m].x; \
	(r)->y = (pre)[m].y; \

@sipa Is there a reason why you kept these in #1184?

Anyway, I suggest:

• Drop the two uses of VERIFY_SETUP.
• Drop VERIFY_SETUP. (Our implicit convention for such code is to nest it in an #ifdef VERIFY block.)

[sipa]

The VERIFY_SETUP calls in ecmult_const_impl.h were made obsolete long before that, in #754, but somehow survived since. They can clearly be removed, indeed.

[stratospher]

33314ca: can we remove this since it's already done inside secp256k1_scalar_verify?

[real-or-random]

This will be resolved by a rebase because it has already been done in #1184, see #1184 (comment).

[theStack]

33314ca: can we remove this since it's already done inside secp256k1_scalar_verify?

Oops, no idea why I added this line in a commit which is supposed to only remove lines (and only add blank ones for better readability). Removed now.

This will be resolved by a rebase because it has already been done in #1184, see #1184 (comment).

@real-or-random: The comment link refers to the same expression, but in a different function. The one in the review comment (being in secp256k1_scalar_cadd_bit) was indeed still there after the rebase.

[theStack]

Rebased on master (taking the new function _scalar_half into account, introduced in #1184), fixed #1393 (comment) and added commits to drop VERIFY_SETUP as suggested.

[stratospher]

ACK bb46723.

[real-or-random]

utACK bb46723

It looks a bit strange that the new uppercase macros have a SECP256K1_ prefix, but our existing macros don't. If you ask me, we should probably add a prefix to everything, but that could be controversial since it touches many lines. Let's not hold this PR up. edit: Tracked in #1449