Skip to content

Commit

Permalink
Merge #1393: Implement new policy for VERIFY_CHECK and #ifdef VERIFY …
Browse files Browse the repository at this point in the history
…(issue #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 #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).

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

Tree-SHA512: 226ca609926dea638aa3bb537d29d4fac8b8302dcd9da35acf767ba9573e5221d2dae04ea26c15d80a50ed70af1ab0dca10642c21df7dbdda432fa237a5ef2cc
  • Loading branch information
real-or-random committed Dec 1, 2023
2 parents 5814d84 + bb46723 commit 07687e8
Show file tree
Hide file tree
Showing 17 changed files with 349 additions and 376 deletions.
8 changes: 0 additions & 8 deletions src/ecmult_const_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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; \
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions src/field.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
4 changes: 0 additions & 4 deletions src/field_10x26_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
5 changes: 0 additions & 5 deletions src/field_5x52_int128_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading

0 comments on commit 07687e8

Please sign in to comment.