group: Avoid using infinity field directly in other modules

real-or-random commented at 7:54 pm on October 28, 2025: contributor

Minor refactoring to make the abstraction cleaner

real-or-random added the label tweak/refactor on Oct 28, 2025

hebasto commented at 2:17 pm on November 3, 2025: member

Concept ACK.

Some other places still remain:

0$ git grep -e "\.infinity" -- src ':(exclude)src/group_impl.h'
1src/ecmult_const_impl.h:    p.infinity = 0;
2src/tests.c:    SECP256K1_CHECKMEM_CHECK(&ge.infinity, sizeof(ge.infinity));
3src/tests_exhaustive.c:            zless_gej.infinity = groupj[j].infinity;
4src/tests_exhaustive.c:                CHECK(group[i].infinity == 0);
5src/tests_exhaustive.c:                CHECK(generated.infinity == 0);

theStack commented at 5:15 pm on November 4, 2025: contributor

Concept ACK

group: Avoid using infinity field directly in other modules 2f73e5281d

real-or-random force-pushed on Nov 6, 2025

real-or-random commented at 4:54 pm on November 6, 2025: contributor

Updated. There are three lines left in the tests, but all of them are justified. Check:

0git grep -E "(\->|\.)infinity" -- src ':(exclude)src/group_impl.h'

By the way, one could think that this PR is a bit arbitrary because we also access the x, y, z fields everywhere. Perhaps this could also be improved. And this would actually have an immediate benefit because we could check that they’re only accessed if the point is not infinity. But this should go to a separate PR, I think.

hebasto commented at 3:18 pm on November 7, 2025: member

Updated. There are three lines left in the tests, but all of them are justified. Check:
0git grep -E "(\->|\.)infinity" -- src ':(exclude)src/group_impl.h'
By the way, one could think that this PR is a bit arbitrary because we also access the x, y, z fields everywhere. Perhaps this could also be improved. And this would actually have an immediate benefit because we could check that they’re only accessed if the point is not infinity. But this should go to a separate PR, I think.

I still think that secp256k1_gej_is_infinity could be used here:https://github.com/bitcoin-core/secp256k1/blob/2f73e5281de44e2c2bcf60ebc7b113440e13b07f/src/tests.c#L3653-L3654

If I’m wrong, maybe add an explanatory comment there?

in src/testutil.h:99 in 2f73e5281d

95@@ -96,17 +96,13 @@ static void testutil_random_ge_test(secp256k1_ge *ge) {
96             break;
97         }
98     } while(1);
99-    ge->infinity = 0;

hebasto commented at 4:04 pm on November 7, 2025:

Why is it correct to drop this line?

real-or-random commented at 8:56 pm on November 7, 2025:

secp256k1_ge_set_xo_var sets this already. https://github.com/bitcoin-core/secp256k1/blob/c8206b1ce60704bb1030d079ea9b3d99d8906220/src/group_impl.h#L355

in src/testutil.h:105 in 2f73e5281d

108-    secp256k1_fe_mul(&gej->y, &ge->y, &z3);
109-    gej->infinity = ge->infinity;
110+    secp256k1_fe z;
111+    testutil_random_fe_non_zero_test(&z);
112+    secp256k1_gej_set_ge(gej, ge);
113+    secp256k1_gej_rescale(gej, &z);

hebasto commented at 4:12 pm on November 7, 2025:

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

real-or-random commented at 9:10 pm on November 7, 2025:

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:

convert ge to gej
rescale the gej

More details:

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
```
0ge.x = gej.x / gej.z^2
1ge.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
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.

real-or-random commented at 9:19 pm on November 7, 2025: contributor

I still think that secp256k1_gej_is_infinity could be used here:

https://github.com/bitcoin-core/secp256k1/blob/2f73e5281de44e2c2bcf60ebc7b113440e13b07f/src/tests.c#L3653-L3654

Yeah, it could be used. My thinking is that gej_xyz_equals_gej is conceptually a “group” function. It would be in group_impl.h if it wasn’t needed only in the tests. It will need raw access to x, y, z too, so it can also have raw access to the infinity flag. I could rename it and move it to group_impl.h. I mean, there’s nothing wrong with having functions there that are used only in the tests.

hebasto commented at 9:32 pm on November 7, 2025: member

I still think that secp256k1_gej_is_infinity could be used here: https://github.com/bitcoin-core/secp256k1/blob/2f73e5281de44e2c2bcf60ebc7b113440e13b07f/src/tests.c#L3653-L3654

Yeah, it could be used. My thinking is that gej_xyz_equals_gej is conceptually a “group” function.

Indeed. I was looking for secp256k1_gej_xyz_equals_gej in group.h :)

It would be in group_impl.h if it wasn’t needed only in the tests. It will need raw access to x, y, z too, so it can also have raw access to the infinity flag. I could rename it and move it to group_impl.h.

That sounds good.

I mean, there’s nothing wrong with having functions there that are used only in the tests.

Agreed.

hebasto approved

hebasto commented at 9:43 pm on November 7, 2025: member

ACK 2f73e5281de44e2c2bcf60ebc7b113440e13b07f, I have reviewed the code and it looks OK.

theStack approved

theStack commented at 7:52 pm on November 18, 2025: contributor

ACK 2f73e5281de44e2c2bcf60ebc7b113440e13b07f

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