Add group.h ge/gej equality functions #1450

sipa commented at 4:43 PM on December 1, 2023: contributor

This pull requests removes the test-only functions ge_equals_ge and ge_equals_gej, and replaces them with proper group.h functions secp256k1_ge_eq_var and secp256k1_gej_eq_ge_var (mimicking the existing secp256k1_gej_eq_var function).

This drops some of the arbitrary and undocumented magnitude restristrictions these functions have, makes them properly tested on their own, and makes their semantics cleaner (I'm always left checking whether ge_equals_ge does a CHECK internally or whether it returns a value...).

in src/group_impl.h:377 in 8a5854a529 outdated

 372 | +    if (a->infinity != b->infinity) return 0;
 373 | +    if (a->infinity) return 1;
 374 | +
 375 | +    tmp = a->x;
 376 | +    secp256k1_fe_normalize_weak(&tmp);
 377 | +    if (!secp256k1_fe_equal(&tmp, &b->x)) return 0;

real-or-random commented at 8:01 PM on December 1, 2023:

We know that the magnitude of a->x is at most SECP256K1_GE_X_MAGNITUDE_MAX, so normalization shouldn't be necessary. And then tmp is not necessary.

Same for y below.

sipa commented at 9:06 PM on December 1, 2023:

secp256k1_fe_equal requires its first argument to have magnitude 1.

real-or-random commented at 9:28 PM on December 1, 2023:

Nevermind. (Duh, too.)

in src/tests.c:3738 in d1c973aa8f outdated

3730 | @@ -3730,6 +3731,22 @@ static void test_ge(void) {
3731 |              random_gej_y_magnitude(&gej[1 + j + 4 * i]);
3732 |              random_gej_z_magnitude(&gej[1 + j + 4 * i]);
3733 |          }
3734 | +
3735 | +        for (j = 0; j < 4; ++j) {
3736 | +            for (k = 0; k < 4; ++k) {
3737 | +                if ((j >> 1) == (k >> 1)) {
3738 | +                    CHECK(secp256k1_ge_eq_var(&ge[1 + j + 4 * i], &ge[1 + k + 4 * i]));

real-or-random commented at 8:08 PM on December 1, 2023:

nit: could rewrite to

int expect_equal = ((j >> 1) == (k >> 1));
CHECK(secp256k1_ge_eq_var(&ge[1 + j + 4 * i], &ge[1 + k + 4 * i]) == except_equal);
...

sipa commented at 9:10 PM on December 1, 2023:

Duh. Done.

real-or-random commented at 8:16 PM on December 1, 2023: contributor

Concept ACK

real-or-random added the label refactor/smell on Dec 1, 2023

Add group.h ge/gej equality functions a47cd97d51

Add unit tests for group.h equality functions 60525f6c14

Replace ge_equals_ge[,j] calls with group.h equality calls 04af0ba162

sipa force-pushed on Dec 1, 2023

real-or-random commented at 9:36 PM on December 1, 2023: contributor

Naming nit: the field equality function has a suffix _equal instead of _eq. It would be good to make this consistent.

(Sorry for bringing this up now after you addressed the other review... utACK otherwise)

sipa commented at 9:38 PM on December 1, 2023: contributor

Naming nit: the field equality function has a suffix _equal instead of _eq. It would be good to make this consistent.

I noticed that too, but that's an existing inconsistency (there is also a secp256k1_gej_eq_var function already).

real-or-random commented at 9:41 PM on December 1, 2023: contributor

Naming nit: the field equality function has a suffix _equal instead of _eq. It would be good to make this consistent.

I noticed that too, but that's an existing inconsistency (there is also a secp256k1_gej_eq_var function already).

Oh, indeed. I suggest renaming the fe function to _eq but this can be done in a separate PR.

real-or-random approved

real-or-random commented at 9:42 PM on December 1, 2023: contributor

utACK 04af0ba162b152073455a5ccbb2c5833ae6d1d57

stratospher commented at 5:21 AM on December 2, 2023: contributor

ACK 04af0ba.

real-or-random merged this on Dec 2, 2023

real-or-random closed this on Dec 2, 2023

fanquake referenced this in commit 41e1b677ca on Jan 3, 2024

fanquake referenced this in commit 29fde0223a on Jan 4, 2024

janus referenced this in commit 5fe435c9b2 on Apr 6, 2024

hebasto referenced this in commit b6de625950 on May 11, 2024

delta1 referenced this in commit 6089844b3c on Apr 2, 2025

div72 referenced this in commit af627d47c3 on Apr 12, 2025

str4d referenced this in commit 136aa9fe62 on Jun 4, 2025

oskarszoon referenced this in commit 0d5c9260f4 on Jul 1, 2025