Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Removes _fe_equal_var, and unwanted _fe_normalize_weak calls (in tests) #1062

Merged
merged 2 commits into from
Aug 17, 2023
Merged

Removes _fe_equal_var, and unwanted _fe_normalize_weak calls (in tests) #1062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb4efd6
tests: remove unwanted `secp256k1_fe_normalize_weak` call
siv2r Jan 4, 2022
54058d1
field: remove `secp256k1_fe_equal_var`
siv2r Jan 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions src/field.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
15 changes: 1 addition & 14 deletions src/field_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions src/group_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion src/modules/extrakeys/tests_exhaustive_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/modules/schnorrsig/main_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
45 changes: 22 additions & 23 deletions src/tests.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2991,8 +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);
secp256k1_fe_normalize_var(&bn);
Copy link
Contributor Author

@siv2r siv2r Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why normalize_var was used on bn. I assumed it intended to set bn's magnitude to 1 for the fe_equal call. So, I removed it. Please let me know if that's not the case, and I'll revert this.

All test cases passed with this change.

return secp256k1_fe_equal_var(&an, &bn);
return secp256k1_fe_equal(&an, &bn);
}

static void run_field_convert(void) {
Expand All @@ -3015,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);
Expand Down Expand Up @@ -3174,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. */
Expand All @@ -3199,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
Expand Down Expand Up @@ -3704,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. */
Expand Down Expand Up @@ -3740,11 +3739,11 @@ 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);
CHECK(secp256k1_fe_equal_var(&u1, &u2));
CHECK(secp256k1_fe_equal_var(&s1, &s2));
s2 = b->y;
CHECK(secp256k1_fe_equal(&u1, &u2));
CHECK(secp256k1_fe_equal(&s1, &s2));
}

static void test_ge(void) {
Expand Down Expand Up @@ -3812,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);

Expand All @@ -3821,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). */
Expand Down Expand Up @@ -3850,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);
Expand Down Expand Up @@ -3933,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. */
Expand Down Expand Up @@ -4173,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));
Expand Down Expand Up @@ -4226,18 +4225,18 @@ 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));

/* 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;
}
Expand Down Expand Up @@ -4456,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) {
Expand Down
20 changes: 10 additions & 10 deletions src/tests_exhaustive.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -52,11 +52,11 @@ 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);
CHECK(secp256k1_fe_equal_var(&u1, &u2));
CHECK(secp256k1_fe_equal_var(&s1, &s2));
s2 = b->y;
CHECK(secp256k1_fe_equal(&u1, &u2));
CHECK(secp256k1_fe_equal(&s1, &s2));
}

static void random_fe(secp256k1_fe *x) {
Expand Down Expand Up @@ -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));
}
}
}
Expand Down Expand Up @@ -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));
}
}

Expand Down