group: Avoid using infinity field directly in other modules

[real-or-random]

Minor refactoring to make the abstraction cleaner

[hebasto]

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]

Concept ACK

[real-or-random]

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]

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:

secp256k1/src/tests.c

Lines 3653 to 3654 in 2f73e52

	ret &= a->infinity == b->infinity;
	if (ret && !a->infinity) {

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

[hebasto]

Why is it correct to drop this line?

[real-or-random]

secp256k1_ge_set_xo_var sets this already.

secp256k1/src/group_impl.h

Line 355 in c8206b1

r->infinity = 0;

[hebasto]

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

[real-or-random]

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.

[real-or-random]

I still think that secp256k1_gej_is_infinity could be used here:

secp256k1/src/tests.c

Lines 3653 to 3654 in 2f73e52

	ret &= a->infinity == b->infinity;
	if (ret && !a->infinity) {

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]

I still think that secp256k1_gej_is_infinity could be used here:

secp256k1/src/tests.c

Lines 3653 to 3654 in 2f73e52

	ret &= a->infinity == b->infinity;
	if (ret && !a->infinity) {

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]

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