Skip to content

Commit 07687e8

Browse files
Merge #1393: Implement new policy for VERIFY_CHECK and #ifdef VERIFY (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
2 parents 5814d84 + bb46723 commit 07687e8

17 files changed

+349
-376
lines changed

src/ecmult_const_impl.h

-8
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,6 @@ static void secp256k1_ecmult_const_odd_multiples_table_globalz(secp256k1_ge *pre
8787
secp256k1_fe neg_y; \
8888
VERIFY_CHECK((n) < (1U << ECMULT_CONST_GROUP_SIZE)); \
8989
VERIFY_CHECK(index < (1U << (ECMULT_CONST_GROUP_SIZE - 1))); \
90-
VERIFY_SETUP(secp256k1_fe_clear(&(r)->x)); \
91-
VERIFY_SETUP(secp256k1_fe_clear(&(r)->y)); \
9290
/* Unconditionally set r->x = (pre)[m].x. r->y = (pre)[m].y. because it's either the correct one
9391
* or will get replaced in the later iterations, this is needed to make sure `r` is initialized. */ \
9492
(r)->x = (pre)[m].x; \
@@ -349,9 +347,7 @@ static int secp256k1_ecmult_const_xonly(secp256k1_fe* r, const secp256k1_fe *n,
349347
secp256k1_fe_mul(&g, &g, n);
350348
if (d) {
351349
secp256k1_fe b;
352-
#ifdef VERIFY
353350
VERIFY_CHECK(!secp256k1_fe_normalizes_to_zero(d));
354-
#endif
355351
secp256k1_fe_sqr(&b, d);
356352
VERIFY_CHECK(SECP256K1_B <= 8); /* magnitude of b will be <= 8 after the next call */
357353
secp256k1_fe_mul_int(&b, SECP256K1_B);
@@ -384,13 +380,9 @@ static int secp256k1_ecmult_const_xonly(secp256k1_fe* r, const secp256k1_fe *n,
384380
p.infinity = 0;
385381

386382
/* Perform x-only EC multiplication of P with q. */
387-
#ifdef VERIFY
388383
VERIFY_CHECK(!secp256k1_scalar_is_zero(q));
389-
#endif
390384
secp256k1_ecmult_const(&rj, &p, q);
391-
#ifdef VERIFY
392385
VERIFY_CHECK(!secp256k1_gej_is_infinity(&rj));
393-
#endif
394386

395387
/* The resulting (X, Y, Z) point on the effective-affine isomorphic curve corresponds to
396388
* (X, Y, Z*v) on the secp256k1 curve. The affine version of that has X coordinate

src/field.h

+2
Original file line numberDiff line numberDiff line change
@@ -345,8 +345,10 @@ static int secp256k1_fe_is_square_var(const secp256k1_fe *a);
345345

346346
/** Check invariants on a field element (no-op unless VERIFY is enabled). */
347347
static void secp256k1_fe_verify(const secp256k1_fe *a);
348+
#define SECP256K1_FE_VERIFY(a) secp256k1_fe_verify(a)
348349

349350
/** Check that magnitude of a is at most m (no-op unless VERIFY is enabled). */
350351
static void secp256k1_fe_verify_magnitude(const secp256k1_fe *a, int m);
352+
#define SECP256K1_FE_VERIFY_MAGNITUDE(a, m) secp256k1_fe_verify_magnitude(a, m)
351353

352354
#endif /* SECP256K1_FIELD_H */

src/field_10x26_impl.h

-4
Original file line numberDiff line numberDiff line change
@@ -403,11 +403,7 @@ void secp256k1_fe_sqr_inner(uint32_t *r, const uint32_t *a);
403403

404404
#else
405405

406-
#ifdef VERIFY
407406
#define VERIFY_BITS(x, n) VERIFY_CHECK(((x) >> (n)) == 0)
408-
#else
409-
#define VERIFY_BITS(x, n) do { } while(0)
410-
#endif
411407

412408
SECP256K1_INLINE static void secp256k1_fe_mul_inner(uint32_t *r, const uint32_t *a, const uint32_t * SECP256K1_RESTRICT b) {
413409
uint64_t c, d;

src/field_5x52_int128_impl.h

-5
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,8 @@
1212
#include "int128.h"
1313
#include "util.h"
1414

15-
#ifdef VERIFY
1615
#define VERIFY_BITS(x, n) VERIFY_CHECK(((x) >> (n)) == 0)
1716
#define VERIFY_BITS_128(x, n) VERIFY_CHECK(secp256k1_u128_check_bits((x), (n)))
18-
#else
19-
#define VERIFY_BITS(x, n) do { } while(0)
20-
#define VERIFY_BITS_128(x, n) do { } while(0)
21-
#endif
2217

2318
SECP256K1_INLINE static void secp256k1_fe_mul_inner(uint64_t *r, const uint64_t *a, const uint64_t * SECP256K1_RESTRICT b) {
2419
secp256k1_uint128 c, d;

0 commit comments

Comments
 (0)