field: remove secp256k1_fe_equal_var

`secp256k1_fe_equal_var` was removed since it hits a fast path only when the inputs are unequal. This unequal inputs condition is not common among its callers (public key parsing, ECDSA verify).
bitcoin-core · Jan 30, 2022 · ea1225b · ea1225b
1 parent d40ea5f
commit ea1225b
Show file tree

Hide file tree

Showing 7 changed files with 31 additions and 44 deletions.
diff --git a/src/field.h b/src/field.h
@@ -64,13 +64,12 @@ static int secp256k1_fe_is_zero(const secp256k1_fe *a);
 /** Check the "oddness" of a field element. Requires the input to be normalized. */
 static int secp256k1_fe_is_odd(const secp256k1_fe *a);
 
-/** Compare two field elements. Requires the first input to have magnitude equal to 1. */
+/** Compare two field elements. Requires the first input to have magnitude equal to 1. 
+ *  The variable time counterpart of this function `secp256k1_fe_equal_var` was removed 
+ *  since it hits fast path only when the inputs are unequal. This unequal inputs 
+ *  condition is not common among its callers (public key parsing, ECDSA verify). */
 static int secp256k1_fe_equal(const secp256k1_fe *a, const secp256k1_fe *b);
 
-/** Same as secp256k1_fe_equal, but may be variable time.
- *  Requires the first input to have magnitude equal to 1. */
-static int secp256k1_fe_equal_var(const secp256k1_fe *a, const secp256k1_fe *b);
-
 /** Compare two field elements. Requires both inputs to be normalized */
 static int secp256k1_fe_cmp_var(const secp256k1_fe *a, const secp256k1_fe *b);
 

diff --git a/src/field_impl.h b/src/field_impl.h
@@ -33,18 +33,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
-    VERIFY_CHECK(a->magnitude == 1);
-    secp256k1_fe_verify(a);
-    secp256k1_fe_verify(b);
-#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 *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.

diff --git a/src/group_impl.h b/src/group_impl.h
@@ -242,7 +242,7 @@ static int secp256k1_gej_eq_x_var(const secp256k1_fe *x, const secp256k1_gej *a)
     VERIFY_CHECK(!a->infinity);
     secp256k1_fe_sqr(&r, &a->z); secp256k1_fe_mul(&r, &r, x);
     r2 = a->x;
-    return secp256k1_fe_equal_var(&r, &r2);
+    return secp256k1_fe_equal(&r, &r2);
 }
 
 static void secp256k1_gej_neg(secp256k1_gej *r, const secp256k1_gej *a) {
@@ -267,7 +267,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(&x3, &secp256k1_fe_const_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
@@ -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(&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
@@ -257,7 +257,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
@@ -2424,7 +2424,7 @@ int check_fe_equal(const secp256k1_fe *a, const secp256k1_fe *b) {
     secp256k1_fe bn = *b;
     secp256k1_fe_normalize_weak(&an);
     secp256k1_fe_normalize_var(&bn);
-    return secp256k1_fe_equal_var(&an, &bn);
+    return secp256k1_fe_equal(&an, &bn);
 }
 
 void run_field_convert(void) {
@@ -2447,9 +2447,9 @@ void run_field_convert(void) {
     secp256k1_fe_storage fes2;
     /* Check conversions to fe. */
     CHECK(secp256k1_fe_set_b32(&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);
@@ -2482,7 +2482,7 @@ void run_field_misc(void) {
         random_fe_non_zero(&y);
         /* 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. */
@@ -2501,7 +2501,7 @@ void run_field_misc(void) {
         CHECK(fe_identical(&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
@@ -2996,8 +2996,8 @@ 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. */
@@ -3035,8 +3035,8 @@ 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));
 }
 
 void test_ge(void) {
@@ -3103,7 +3103,7 @@ 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);
 
@@ -3112,7 +3112,7 @@ 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). */
@@ -3141,7 +3141,7 @@ 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);
@@ -3436,8 +3436,8 @@ 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));
@@ -3495,12 +3495,12 @@ 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;
     }
@@ -3727,7 +3727,7 @@ 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));
 }
 
 void ecmult_const_random_mult(void) {

diff --git a/src/tests_exhaustive.c b/src/tests_exhaustive.c
@@ -33,8 +33,8 @@ 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));
 }
 
 void ge_equals_gej(const secp256k1_ge *a, const secp256k1_gej *b) {
@@ -50,8 +50,8 @@ void ge_equals_gej(const secp256k1_ge *a, const secp256k1_gej *b) {
     u2 = b->x; secp256k1_fe_normalize_weak(&u2);
     secp256k1_fe_mul(&s1, &a->y, &z2s); secp256k1_fe_mul(&s1, &s1, &b->z);
     s2 = b->y; secp256k1_fe_normalize_weak(&s2);
-    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));
 }
 
 void random_fe(secp256k1_fe *x) {
@@ -426,8 +426,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));
             }
         }