Skip to content

group: Avoid using infinity field directly in other modules #1764

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+25 −28
Open

group: Avoid using infinity field directly in other modules #1764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 10 additions & 9 deletions src/ecmult_const_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,17 +87,15 @@ 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))); \
/* Unconditionally set r->x = (pre)[m].x. r->y = (pre)[m].y. because it's either the correct one
/* Unconditionally set r->x = (pre)[m].x and 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; \
(r)->y = (pre)[m].y; \
secp256k1_ge_set_xy((r), &(pre)[m].x, &(pre)[m].y); \
for (m = 1; m < ECMULT_CONST_TABLE_SIZE; m++) { \
/* This loop is used to avoid secret data in array indices. See
* the comment in ecmult_gen_impl.h for rationale. */ \
secp256k1_fe_cmov(&(r)->x, &(pre)[m].x, m == index); \
secp256k1_fe_cmov(&(r)->y, &(pre)[m].y, m == index); \
} \
(r)->infinity = 0; \
secp256k1_fe_negate(&neg_y, &(r)->y, 1); \
secp256k1_fe_cmov(&(r)->y, &neg_y, negative); \
} while(0)
Expand Down Expand Up @@ -375,11 +373,14 @@ static int secp256k1_ecmult_const_xonly(secp256k1_fe* r, const secp256k1_fe *n,

SECP256K1_FE_VERIFY_MAGNITUDE(&g, 2);

/* Compute base point P = (n*g, g^2), the effective affine version of (n*g, g^2, v), which has
* corresponding affine X coordinate n/d. */
secp256k1_fe_mul(&p.x, &g, n);
secp256k1_fe_sqr(&p.y, &g);
p.infinity = 0;
/* Compute base point P = (n*g, g^2), the effective affine version of
* (n*g, g^2, v), which has corresponding affine X coordinate n/d. */
{
secp256k1_fe x, y;
secp256k1_fe_mul(&x, &g, n);
secp256k1_fe_sqr(&y, &g);
secp256k1_ge_set_xy(&p, &x, &y);
}

/* Perform x-only EC multiplication of P with q. */
VERIFY_CHECK(!secp256k1_scalar_is_zero(q));
Expand Down
4 changes: 2 additions & 2 deletions src/ecmult_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ static void secp256k1_ecmult_odd_multiples_table(int n, secp256k1_ge *pre_a, sec
secp256k1_ge d_ge;
int i;

VERIFY_CHECK(!a->infinity);
VERIFY_CHECK(!secp256k1_gej_is_infinity(a));

secp256k1_gej_double_var(&d, a, NULL);

Expand Down Expand Up @@ -341,7 +341,7 @@ static void secp256k1_ecmult_strauss_wnaf(const struct secp256k1_strauss_state *
}
}

if (!r->infinity) {
if (!secp256k1_gej_is_infinity(r)) {
secp256k1_fe_mul(&r->z, &r->z, &Z);
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/tests.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4152,8 +4152,8 @@ static void test_group_decompress(const secp256k1_fe* x) {
secp256k1_fe_normalize_var(&ge_even.y);

/* No infinity allowed. */
CHECK(!ge_even.infinity);
CHECK(!ge_odd.infinity);
CHECK(!secp256k1_ge_is_infinity(&ge_even));
CHECK(!secp256k1_ge_is_infinity(&ge_odd));

/* Check that the x coordinates check out. */
CHECK(secp256k1_fe_equal(&ge_even.x, x));
Expand Down
14 changes: 7 additions & 7 deletions src/tests_exhaustive.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,11 @@ static void test_exhaustive_addition(const secp256k1_ge *group, const secp256k1_
secp256k1_gej_add_ge_var(&tmp, &groupj[i], &group[j], NULL);
CHECK(secp256k1_gej_eq_ge_var(&tmp, &group[(i + j) % EXHAUSTIVE_TEST_ORDER]));
/* add_zinv_var */
zless_gej.infinity = groupj[j].infinity;
zless_gej.x = groupj[j].x;
zless_gej.y = groupj[j].y;
if (secp256k1_gej_is_infinity(&groupj[j])) {
secp256k1_ge_set_infinity(&zless_gej);
} else {
secp256k1_ge_set_xy(&zless_gej, &groupj[j].x, &groupj[j].y);
}
secp256k1_gej_add_zinv_var(&tmp, &groupj[i], &zless_gej, &fe_inv);
CHECK(secp256k1_gej_eq_ge_var(&tmp, &group[(i + j) % EXHAUSTIVE_TEST_ORDER]));
}
Expand Down Expand Up @@ -422,10 +424,8 @@ int main(int argc, char** argv) {
secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &generatedj, &scalar_i);
secp256k1_ge_set_gej(&generated, &generatedj);

CHECK(group[i].infinity == 0);
CHECK(generated.infinity == 0);
CHECK(secp256k1_fe_equal(&generated.x, &group[i].x));
CHECK(secp256k1_fe_equal(&generated.y, &group[i].y));
CHECK(!secp256k1_ge_is_infinity(&group[i]));
CHECK(secp256k1_ge_eq_var(&group[i], &generated));
}
}

Expand Down
12 changes: 4 additions & 8 deletions src/testutil.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,17 +96,13 @@ static void testutil_random_ge_test(secp256k1_ge *ge) {
break;
}
} while(1);
ge->infinity = 0;
Copy link
Member

@hebasto hebasto Nov 7, 2025

Choose a reason for hiding this comment

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

Why is it correct to drop this line?

Copy link
Contributor Author

@real-or-random real-or-random Nov 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

secp256k1_ge_set_xo_var sets this already.

secp256k1/src/group_impl.h

Line 355 in c8206b1

r->infinity = 0;

}

static void testutil_random_ge_jacobian_test(secp256k1_gej *gej, const secp256k1_ge *ge) {
secp256k1_fe z2, z3;
testutil_random_fe_non_zero_test(&gej->z);
secp256k1_fe_sqr(&z2, &gej->z);
secp256k1_fe_mul(&z3, &z2, &gej->z);
secp256k1_fe_mul(&gej->x, &ge->x, &z2);
secp256k1_fe_mul(&gej->y, &ge->y, &z3);
gej->infinity = ge->infinity;
secp256k1_fe z;
testutil_random_fe_non_zero_test(&z);
secp256k1_gej_set_ge(gej, ge);
secp256k1_gej_rescale(gej, &z);
Comment on lines -103 to +105
Copy link
Member

@hebasto hebasto Nov 7, 2025

Choose a reason for hiding this comment

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

I had a hard time following these changes. Could you shed a bit more light on them?

Copy link
Contributor Author

@real-or-random real-or-random Nov 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What this function does is take a ge and convert it to a gej with a random z coordinate. This can be done in two steps:

  1. convert ge to gej
  2. rescale the gej

More details:

  1. ge is a "normal" affine point with x and y coordinates. gej (Jacobian coordinates) represent ge.x and ge.y as gej.x gej.y gej.z with 
    ge.x = gej.x / gej.z^2
ge.y = gej.y / gej.z^3
    Thus, the canonical way to ge to gej is to set gej.z = 1 and then just gej.x = ge.x and gej.y = ge.y because z^2 = 1 and z^3 = 1
  2. Rescaling a gej means changing the representation without changing the effective x and y. This can be multiplying the existing z coordinate with a given field element. To accommodate, we'll need to multiply x with z^2 and y with z^3.

Or another answer: If you look into secp256k1_gej_set_ge and secp256k1_gej_rescale you'll be able to check that there are no semantic changes.

}

static void testutil_random_gej_test(secp256k1_gej *gej) {
Expand Down
Loading