From bb4efd6404960f9e8f93c15d7d001af068e5b5a4 Mon Sep 17 00:00:00 2001 From: siv2r Date: Wed, 5 Jan 2022 04:23:30 +0530 Subject: [PATCH 1/2] tests: remove unwanted `secp256k1_fe_normalize_weak` call It is not neccessary for the second argument in `secp256k1_fe_equal_var` (or `secp256k1_fe_equal`) to have magnitude = 1. Hence, removed the `secp256k1_fe_normalize_weak` call for those argument. --- src/tests.c | 9 ++++----- src/tests_exhaustive.c | 4 ++-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/tests.c b/src/tests.c index 7a76b27ae0..2af5db4e58 100644 --- a/src/tests.c +++ b/src/tests.c @@ -2991,7 +2991,6 @@ static int check_fe_equal(const secp256k1_fe *a, const secp256k1_fe *b) { secp256k1_fe an = *a; secp256k1_fe bn = *b; secp256k1_fe_normalize_weak(&an); - secp256k1_fe_normalize_var(&bn); return secp256k1_fe_equal_var(&an, &bn); } @@ -3740,9 +3739,9 @@ static void ge_equals_gej(const secp256k1_ge *a, const secp256k1_gej *b) { /* Check a.x * b.z^2 == b.x && a.y * b.z^3 == b.y, to avoid inverses. */ secp256k1_fe_sqr(&z2s, &b->z); secp256k1_fe_mul(&u1, &a->x, &z2s); - u2 = b->x; secp256k1_fe_normalize_weak(&u2); + u2 = b->x; secp256k1_fe_mul(&s1, &a->y, &z2s); secp256k1_fe_mul(&s1, &s1, &b->z); - s2 = b->y; secp256k1_fe_normalize_weak(&s2); + s2 = b->y; CHECK(secp256k1_fe_equal_var(&u1, &u2)); CHECK(secp256k1_fe_equal_var(&s1, &s2)); } @@ -4226,8 +4225,8 @@ static void test_pre_g_table(const secp256k1_ge_storage * pre_g, size_t n) { secp256k1_ge_from_storage(&q, &pre_g[i]); CHECK(secp256k1_ge_is_valid_var(&q)); - secp256k1_fe_negate(&dqx, &q.x, 1); secp256k1_fe_add(&dqx, &gg.x); secp256k1_fe_normalize_weak(&dqx); - dqy = q.y; secp256k1_fe_add(&dqy, &gg.y); secp256k1_fe_normalize_weak(&dqy); + secp256k1_fe_negate(&dqx, &q.x, 1); secp256k1_fe_add(&dqx, &gg.x); + dqy = q.y; secp256k1_fe_add(&dqy, &gg.y); /* Check that -q is not equal to gg */ CHECK(!secp256k1_fe_normalizes_to_zero_var(&dqx) || !secp256k1_fe_normalizes_to_zero_var(&dqy)); diff --git a/src/tests_exhaustive.c b/src/tests_exhaustive.c index dbb6b7eb46..b38e0c49cf 100644 --- a/src/tests_exhaustive.c +++ b/src/tests_exhaustive.c @@ -52,9 +52,9 @@ static void ge_equals_gej(const secp256k1_ge *a, const secp256k1_gej *b) { /* Check a.x * b.z^2 == b.x && a.y * b.z^3 == b.y, to avoid inverses. */ secp256k1_fe_sqr(&z2s, &b->z); secp256k1_fe_mul(&u1, &a->x, &z2s); - u2 = b->x; secp256k1_fe_normalize_weak(&u2); + u2 = b->x; secp256k1_fe_mul(&s1, &a->y, &z2s); secp256k1_fe_mul(&s1, &s1, &b->z); - s2 = b->y; secp256k1_fe_normalize_weak(&s2); + s2 = b->y; CHECK(secp256k1_fe_equal_var(&u1, &u2)); CHECK(secp256k1_fe_equal_var(&s1, &s2)); } From 54058d16feaa431520029335e2d56252859d3260 Mon Sep 17 00:00:00 2001 From: siv2r Date: Mon, 31 Jan 2022 04:43:57 +0530 Subject: [PATCH 2/2] field: remove `secp256k1_fe_equal_var` `fe_equal_var` hits a fast path only when the inputs are unequal, which is uncommon among its callers (public key parsing, ECDSA verify). --- src/field.h | 6 ---- src/field_impl.h | 15 +------- src/group_impl.h | 4 +-- src/modules/extrakeys/tests_exhaustive_impl.h | 2 +- src/modules/schnorrsig/main_impl.h | 2 +- src/tests.c | 36 +++++++++---------- src/tests_exhaustive.c | 16 ++++----- 7 files changed, 31 insertions(+), 50 deletions(-) diff --git a/src/field.h b/src/field.h index c1775912f8..ccd228e1ae 100644 --- a/src/field.h +++ b/src/field.h @@ -176,12 +176,6 @@ static int secp256k1_fe_is_odd(const secp256k1_fe *a); */ static int secp256k1_fe_equal(const secp256k1_fe *a, const secp256k1_fe *b); -/** Determine whether two field elements are equal, without constant-time guarantee. - * - * Identical in behavior to secp256k1_fe_equal, but not constant time in either a or b. - */ -static int secp256k1_fe_equal_var(const secp256k1_fe *a, const secp256k1_fe *b); - /** Compare the values represented by 2 field elements, without constant-time guarantee. * * On input, a and b must be valid normalized field elements. diff --git a/src/field_impl.h b/src/field_impl.h index 29b54c74bc..80d34b9ef2 100644 --- a/src/field_impl.h +++ b/src/field_impl.h @@ -31,19 +31,6 @@ SECP256K1_INLINE static int secp256k1_fe_equal(const secp256k1_fe *a, const secp return secp256k1_fe_normalizes_to_zero(&na); } -SECP256K1_INLINE static int secp256k1_fe_equal_var(const secp256k1_fe *a, const secp256k1_fe *b) { - secp256k1_fe na; -#ifdef VERIFY - secp256k1_fe_verify(a); - secp256k1_fe_verify(b); - secp256k1_fe_verify_magnitude(a, 1); - secp256k1_fe_verify_magnitude(b, 31); -#endif - secp256k1_fe_negate(&na, a, 1); - secp256k1_fe_add(&na, b); - return secp256k1_fe_normalizes_to_zero_var(&na); -} - static int secp256k1_fe_sqrt(secp256k1_fe * SECP256K1_RESTRICT r, const secp256k1_fe * SECP256K1_RESTRICT a) { /** Given that p is congruent to 3 mod 4, we can compute the square root of * a mod p as the (p+1)/4'th power of a. @@ -151,7 +138,7 @@ static int secp256k1_fe_sqrt(secp256k1_fe * SECP256K1_RESTRICT r, const secp256k if (!ret) { secp256k1_fe_negate(&t1, &t1, 1); secp256k1_fe_normalize_var(&t1); - VERIFY_CHECK(secp256k1_fe_equal_var(&t1, a)); + VERIFY_CHECK(secp256k1_fe_equal(&t1, a)); } #endif return ret; diff --git a/src/group_impl.h b/src/group_impl.h index dd8a5d0ca9..b9542ce8ae 100644 --- a/src/group_impl.h +++ b/src/group_impl.h @@ -367,7 +367,7 @@ static int secp256k1_gej_eq_x_var(const secp256k1_fe *x, const secp256k1_gej *a) #endif secp256k1_fe_sqr(&r, &a->z); secp256k1_fe_mul(&r, &r, x); - return secp256k1_fe_equal_var(&r, &a->x); + return secp256k1_fe_equal(&r, &a->x); } static void secp256k1_gej_neg(secp256k1_gej *r, const secp256k1_gej *a) { @@ -400,7 +400,7 @@ static int secp256k1_ge_is_valid_var(const secp256k1_ge *a) { secp256k1_fe_sqr(&y2, &a->y); secp256k1_fe_sqr(&x3, &a->x); secp256k1_fe_mul(&x3, &x3, &a->x); secp256k1_fe_add_int(&x3, SECP256K1_B); - return secp256k1_fe_equal_var(&y2, &x3); + return secp256k1_fe_equal(&y2, &x3); } static SECP256K1_INLINE void secp256k1_gej_double(secp256k1_gej *r, const secp256k1_gej *a) { diff --git a/src/modules/extrakeys/tests_exhaustive_impl.h b/src/modules/extrakeys/tests_exhaustive_impl.h index d3d817a131..645bae2d47 100644 --- a/src/modules/extrakeys/tests_exhaustive_impl.h +++ b/src/modules/extrakeys/tests_exhaustive_impl.h @@ -48,7 +48,7 @@ static void test_exhaustive_extrakeys(const secp256k1_context *ctx, const secp25 /* Compare the xonly_pubkey bytes against the precomputed group. */ secp256k1_fe_set_b32_mod(&fe, xonly_pubkey_bytes[i - 1]); - CHECK(secp256k1_fe_equal_var(&fe, &group[i].x)); + CHECK(secp256k1_fe_equal(&fe, &group[i].x)); /* Check the parity against the precomputed group. */ fe = group[i].y; diff --git a/src/modules/schnorrsig/main_impl.h b/src/modules/schnorrsig/main_impl.h index 4e7b45a045..26727e4651 100644 --- a/src/modules/schnorrsig/main_impl.h +++ b/src/modules/schnorrsig/main_impl.h @@ -261,7 +261,7 @@ int secp256k1_schnorrsig_verify(const secp256k1_context* ctx, const unsigned cha secp256k1_fe_normalize_var(&r.y); return !secp256k1_fe_is_odd(&r.y) && - secp256k1_fe_equal_var(&rx, &r.x); + secp256k1_fe_equal(&rx, &r.x); } #endif diff --git a/src/tests.c b/src/tests.c index 2af5db4e58..5b7c914f8c 100644 --- a/src/tests.c +++ b/src/tests.c @@ -2991,7 +2991,7 @@ static int check_fe_equal(const secp256k1_fe *a, const secp256k1_fe *b) { secp256k1_fe an = *a; secp256k1_fe bn = *b; secp256k1_fe_normalize_weak(&an); - return secp256k1_fe_equal_var(&an, &bn); + return secp256k1_fe_equal(&an, &bn); } static void run_field_convert(void) { @@ -3014,9 +3014,9 @@ static void run_field_convert(void) { secp256k1_fe_storage fes2; /* Check conversions to fe. */ CHECK(secp256k1_fe_set_b32_limit(&fe2, b32)); - CHECK(secp256k1_fe_equal_var(&fe, &fe2)); + CHECK(secp256k1_fe_equal(&fe, &fe2)); secp256k1_fe_from_storage(&fe2, &fes); - CHECK(secp256k1_fe_equal_var(&fe, &fe2)); + CHECK(secp256k1_fe_equal(&fe, &fe2)); /* Check conversion from fe. */ secp256k1_fe_get_b32(b322, &fe); CHECK(secp256k1_memcmp_var(b322, b32, 32) == 0); @@ -3173,7 +3173,7 @@ static void run_field_misc(void) { CHECK(check_fe_equal(&q, &z)); /* Test the fe equality and comparison operations. */ CHECK(secp256k1_fe_cmp_var(&x, &x) == 0); - CHECK(secp256k1_fe_equal_var(&x, &x)); + CHECK(secp256k1_fe_equal(&x, &x)); z = x; secp256k1_fe_add(&z,&y); /* Test fe conditional move; z is not normalized here. */ @@ -3198,7 +3198,7 @@ static void run_field_misc(void) { q = z; secp256k1_fe_normalize_var(&x); secp256k1_fe_normalize_var(&z); - CHECK(!secp256k1_fe_equal_var(&x, &z)); + CHECK(!secp256k1_fe_equal(&x, &z)); secp256k1_fe_normalize_var(&q); secp256k1_fe_cmov(&q, &z, (i&1)); #ifdef VERIFY @@ -3703,8 +3703,8 @@ static void ge_equals_ge(const secp256k1_ge *a, const secp256k1_ge *b) { if (a->infinity) { return; } - CHECK(secp256k1_fe_equal_var(&a->x, &b->x)); - CHECK(secp256k1_fe_equal_var(&a->y, &b->y)); + CHECK(secp256k1_fe_equal(&a->x, &b->x)); + CHECK(secp256k1_fe_equal(&a->y, &b->y)); } /* This compares jacobian points including their Z, not just their geometric meaning. */ @@ -3742,8 +3742,8 @@ static void ge_equals_gej(const secp256k1_ge *a, const secp256k1_gej *b) { u2 = b->x; secp256k1_fe_mul(&s1, &a->y, &z2s); secp256k1_fe_mul(&s1, &s1, &b->z); s2 = b->y; - CHECK(secp256k1_fe_equal_var(&u1, &u2)); - CHECK(secp256k1_fe_equal_var(&s1, &s2)); + CHECK(secp256k1_fe_equal(&u1, &u2)); + CHECK(secp256k1_fe_equal(&s1, &s2)); } static void test_ge(void) { @@ -3811,7 +3811,7 @@ static void test_ge(void) { /* Check Z ratio. */ if (!secp256k1_gej_is_infinity(&gej[i1]) && !secp256k1_gej_is_infinity(&refj)) { secp256k1_fe zrz; secp256k1_fe_mul(&zrz, &zr, &gej[i1].z); - CHECK(secp256k1_fe_equal_var(&zrz, &refj.z)); + CHECK(secp256k1_fe_equal(&zrz, &refj.z)); } secp256k1_ge_set_gej_var(&ref, &refj); @@ -3820,7 +3820,7 @@ static void test_ge(void) { ge_equals_gej(&ref, &resj); if (!secp256k1_gej_is_infinity(&gej[i1]) && !secp256k1_gej_is_infinity(&resj)) { secp256k1_fe zrz; secp256k1_fe_mul(&zrz, &zr, &gej[i1].z); - CHECK(secp256k1_fe_equal_var(&zrz, &resj.z)); + CHECK(secp256k1_fe_equal(&zrz, &resj.z)); } /* Test gej + ge (var, with additional Z factor). */ @@ -3849,7 +3849,7 @@ static void test_ge(void) { ge_equals_gej(&ref, &resj); /* Check Z ratio. */ secp256k1_fe_mul(&zr2, &zr2, &gej[i1].z); - CHECK(secp256k1_fe_equal_var(&zr2, &resj.z)); + CHECK(secp256k1_fe_equal(&zr2, &resj.z)); /* Normal doubling. */ secp256k1_gej_double_var(&resj, &gej[i2], NULL); ge_equals_gej(&ref, &resj); @@ -3932,7 +3932,7 @@ static void test_ge(void) { ret_set_xo = secp256k1_ge_set_xo_var(&q, &r, 0); CHECK(ret_on_curve == ret_frac_on_curve); CHECK(ret_on_curve == ret_set_xo); - if (ret_set_xo) CHECK(secp256k1_fe_equal_var(&r, &q.x)); + if (ret_set_xo) CHECK(secp256k1_fe_equal(&r, &q.x)); } /* Test batch gej -> ge conversion with many infinities. */ @@ -4172,8 +4172,8 @@ static void test_group_decompress(const secp256k1_fe* x) { CHECK(!ge_odd.infinity); /* Check that the x coordinates check out. */ - CHECK(secp256k1_fe_equal_var(&ge_even.x, x)); - CHECK(secp256k1_fe_equal_var(&ge_odd.x, x)); + CHECK(secp256k1_fe_equal(&ge_even.x, x)); + CHECK(secp256k1_fe_equal(&ge_odd.x, x)); /* Check odd/even Y in ge_odd, ge_even. */ CHECK(secp256k1_fe_is_odd(&ge_odd.y)); @@ -4231,12 +4231,12 @@ static void test_pre_g_table(const secp256k1_ge_storage * pre_g, size_t n) { CHECK(!secp256k1_fe_normalizes_to_zero_var(&dqx) || !secp256k1_fe_normalizes_to_zero_var(&dqy)); /* Check that -q is not equal to p */ - CHECK(!secp256k1_fe_equal_var(&dpx, &dqx) || !secp256k1_fe_equal_var(&dpy, &dqy)); + CHECK(!secp256k1_fe_equal(&dpx, &dqx) || !secp256k1_fe_equal(&dpy, &dqy)); /* Check that p, -q and gg are colinear */ secp256k1_fe_mul(&dpx, &dpx, &dqy); secp256k1_fe_mul(&dpy, &dpy, &dqx); - CHECK(secp256k1_fe_equal_var(&dpx, &dpy)); + CHECK(secp256k1_fe_equal(&dpx, &dpy)); p = q; } @@ -4455,7 +4455,7 @@ static void run_point_times_order(void) { secp256k1_fe_sqr(&x, &x); } secp256k1_fe_normalize_var(&x); - CHECK(secp256k1_fe_equal_var(&x, &xr)); + CHECK(secp256k1_fe_equal(&x, &xr)); } static void ecmult_const_random_mult(void) { diff --git a/src/tests_exhaustive.c b/src/tests_exhaustive.c index b38e0c49cf..3af8ec1ee5 100644 --- a/src/tests_exhaustive.c +++ b/src/tests_exhaustive.c @@ -38,8 +38,8 @@ static void ge_equals_ge(const secp256k1_ge *a, const secp256k1_ge *b) { if (a->infinity) { return; } - CHECK(secp256k1_fe_equal_var(&a->x, &b->x)); - CHECK(secp256k1_fe_equal_var(&a->y, &b->y)); + CHECK(secp256k1_fe_equal(&a->x, &b->x)); + CHECK(secp256k1_fe_equal(&a->y, &b->y)); } static void ge_equals_gej(const secp256k1_ge *a, const secp256k1_gej *b) { @@ -55,8 +55,8 @@ static void ge_equals_gej(const secp256k1_ge *a, const secp256k1_gej *b) { u2 = b->x; secp256k1_fe_mul(&s1, &a->y, &z2s); secp256k1_fe_mul(&s1, &s1, &b->z); s2 = b->y; - CHECK(secp256k1_fe_equal_var(&u1, &u2)); - CHECK(secp256k1_fe_equal_var(&s1, &s2)); + CHECK(secp256k1_fe_equal(&u1, &u2)); + CHECK(secp256k1_fe_equal(&s1, &s2)); } static void random_fe(secp256k1_fe *x) { @@ -219,14 +219,14 @@ static void test_exhaustive_ecmult(const secp256k1_ge *group, const secp256k1_ge /* Test secp256k1_ecmult_const_xonly with all curve X coordinates, and xd=NULL. */ ret = secp256k1_ecmult_const_xonly(&tmpf, &group[i].x, NULL, &ng, 0); CHECK(ret); - CHECK(secp256k1_fe_equal_var(&tmpf, &group[(i * j) % EXHAUSTIVE_TEST_ORDER].x)); + CHECK(secp256k1_fe_equal(&tmpf, &group[(i * j) % EXHAUSTIVE_TEST_ORDER].x)); /* Test secp256k1_ecmult_const_xonly with all curve X coordinates, with random xd. */ random_fe_non_zero(&xd); secp256k1_fe_mul(&xn, &xd, &group[i].x); ret = secp256k1_ecmult_const_xonly(&tmpf, &xn, &xd, &ng, 0); CHECK(ret); - CHECK(secp256k1_fe_equal_var(&tmpf, &group[(i * j) % EXHAUSTIVE_TEST_ORDER].x)); + CHECK(secp256k1_fe_equal(&tmpf, &group[(i * j) % EXHAUSTIVE_TEST_ORDER].x)); } } } @@ -475,8 +475,8 @@ int main(int argc, char** argv) { CHECK(group[i].infinity == 0); CHECK(generated.infinity == 0); - CHECK(secp256k1_fe_equal_var(&generated.x, &group[i].x)); - CHECK(secp256k1_fe_equal_var(&generated.y, &group[i].y)); + CHECK(secp256k1_fe_equal(&generated.x, &group[i].x)); + CHECK(secp256k1_fe_equal(&generated.y, &group[i].y)); } }