Merge bitcoin-core/secp256k1#1393: Implement new policy for VERIFY_CH…

…ECK and #ifdef VERIFY (issue bitcoin#1381)

bb46723 remove VERIFY_SETUP define (Sebastian Falbesoner)
a3a3e11 remove unneeded VERIFY_SETUP uses in ECMULT_CONST_TABLE_GET_GE macro (Sebastian Falbesoner)
a0fb68a introduce and use SECP256K1_SCALAR_VERIFY macro (Sebastian Falbesoner)
cf25c86 introduce and use SECP256K1_{FE,GE,GEJ}_VERIFY macros (Sebastian Falbesoner)
5d89bc0 remove superfluous `#ifdef VERIFY`/`#endif` preprocessor conditions (Sebastian Falbesoner)
c2688f8 redefine VERIFY_CHECK to empty in production (non-VERIFY) mode (Sebastian Falbesoner)

Pull request description:

  As suggested in bitcoin#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 (bitcoin#1381 didn't get any feedback about the renaming idea yet).

ACKs for top commit:
  stratospher:
    ACK bb46723.
  real-or-random:
    utACK bb46723

Tree-SHA512: 226ca609926dea638aa3bb537d29d4fac8b8302dcd9da35acf767ba9573e5221d2dae04ea26c15d80a50ed70af1ab0dca10642c21df7dbdda432fa237a5ef2cc

src/ecmult_const_impl.h

@@ -87,8 +87,6 @@ static void secp256k1_ecmult_const_odd_multiples_table_globalz(secp256k1_ge *pre
     secp256k1_fe neg_y; \
     VERIFY_CHECK((n) < (1U << ECMULT_CONST_GROUP_SIZE)); \
     VERIFY_CHECK(index < (1U << (ECMULT_CONST_GROUP_SIZE - 1))); \
-    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; \
@@ -349,9 +347,7 @@ static int secp256k1_ecmult_const_xonly(secp256k1_fe* r, const secp256k1_fe *n,
     secp256k1_fe_mul(&g, &g, n);
     if (d) {
         secp256k1_fe b;
-#ifdef VERIFY
         VERIFY_CHECK(!secp256k1_fe_normalizes_to_zero(d));
-#endif
         secp256k1_fe_sqr(&b, d);
         VERIFY_CHECK(SECP256K1_B <= 8); /* magnitude of b will be <= 8 after the next call */
         secp256k1_fe_mul_int(&b, SECP256K1_B);
@@ -384,13 +380,9 @@ static int secp256k1_ecmult_const_xonly(secp256k1_fe* r, const secp256k1_fe *n,
     p.infinity = 0;
 
     /* Perform x-only EC multiplication of P with q. */
-#ifdef VERIFY
     VERIFY_CHECK(!secp256k1_scalar_is_zero(q));
-#endif
     secp256k1_ecmult_const(&rj, &p, q);
-#ifdef VERIFY
     VERIFY_CHECK(!secp256k1_gej_is_infinity(&rj));
-#endif
 
     /* The resulting (X, Y, Z) point on the effective-affine isomorphic curve corresponds to
      * (X, Y, Z*v) on the secp256k1 curve. The affine version of that has X coordinate

src/field.h

@@ -345,8 +345,10 @@ static int secp256k1_fe_is_square_var(const secp256k1_fe *a);
 
 /** Check invariants on a field element (no-op unless VERIFY is enabled). */
 static void secp256k1_fe_verify(const secp256k1_fe *a);
+#define SECP256K1_FE_VERIFY(a) secp256k1_fe_verify(a)
 
 /** Check that magnitude of a is at most m (no-op unless VERIFY is enabled). */
 static void secp256k1_fe_verify_magnitude(const secp256k1_fe *a, int m);
+#define SECP256K1_FE_VERIFY_MAGNITUDE(a, m) secp256k1_fe_verify_magnitude(a, m)
 
 #endif /* SECP256K1_FIELD_H */

src/field_10x26_impl.h

@@ -403,11 +403,7 @@ void secp256k1_fe_sqr_inner(uint32_t *r, const uint32_t *a);
 
 #else
 
-#ifdef VERIFY
 #define VERIFY_BITS(x, n) VERIFY_CHECK(((x) >> (n)) == 0)
-#else
-#define VERIFY_BITS(x, n) do { } while(0)
-#endif
 
 SECP256K1_INLINE static void secp256k1_fe_mul_inner(uint32_t *r, const uint32_t *a, const uint32_t * SECP256K1_RESTRICT b) {
     uint64_t c, d;

src/field_5x52_int128_impl.h

@@ -12,13 +12,8 @@
 #include "int128.h"
 #include "util.h"
 
-#ifdef VERIFY
 #define VERIFY_BITS(x, n) VERIFY_CHECK(((x) >> (n)) == 0)
 #define VERIFY_BITS_128(x, n) VERIFY_CHECK(secp256k1_u128_check_bits((x), (n)))
-#else
-#define VERIFY_BITS(x, n) do { } while(0)
-#define VERIFY_BITS_128(x, n) do { } while(0)
-#endif
 
 SECP256K1_INLINE static void secp256k1_fe_mul_inner(uint64_t *r, const uint64_t *a, const uint64_t * SECP256K1_RESTRICT b) {
     secp256k1_uint128 c, d;