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

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:

$ git grep -e "\.infinity" -- src ':(exclude)src/group_impl.h'
src/ecmult_const_impl.h:    p.infinity = 0;
src/tests.c:    SECP256K1_CHECKMEM_CHECK(&ge.infinity, sizeof(ge.infinity));
src/tests_exhaustive.c:            zless_gej.infinity = groupj[j].infinity;
src/tests_exhaustive.c:                CHECK(group[i].infinity == 0);
src/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:

git 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:
git 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
```
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
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

real-or-random merged this on Jan 6, 2026

real-or-random closed this on Jan 6, 2026

real-or-random commented at 9:13 AM on January 6, 2026: contributor

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

Agreed.

I changed my mind here. There's also some benefit in keeping test-only code in the test files. That means moving the function to group_impl.h is not clearly better.

I took the freedom to merge this as-is because it has two ACKs.

fanquake referenced this in commit c4c4e04ca1 on Jan 26, 2026

fanquake referenced this in commit 2fccbea3c8 on Jan 27, 2026

fjahr referenced this in commit 182197f98a on Jan 29, 2026

fjahr referenced this in commit ea9a84ab3c on Jan 31, 2026

fjahr referenced this in commit 23c2527f12 on Feb 8, 2026

Sjors referenced this in commit d5660d3a13 on Feb 16, 2026

mllwchrry referenced this in commit 34edcef037 on Feb 26, 2026

github-actions[bot] referenced this in commit c3f80fff5f on Mar 1, 2026

github-actions[bot] referenced this in commit 4aeff8400e on Mar 1, 2026

github-actions[bot] referenced this in commit 5f15eb0c55 on Mar 1, 2026

0x000000000019d6689c085ae165831e934ff76 referenced this in commit d54574beca on Mar 2, 2026

csjones referenced this in commit fb3e16af04 on Mar 2, 2026

mllwchrry referenced this in commit 036b454f21 on Mar 2, 2026

mllwchrry referenced this in commit fe48cc9fa5 on Mar 3, 2026

real-or-random referenced this in commit 459eab20f2 on Mar 3, 2026

vmta referenced this in commit 1ddc2f947f on Apr 26, 2026

vmta referenced this in commit 56c40fe100 on Apr 27, 2026