Add group.h ge/gej equality functions #1450

1. 
   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…).

2. sipa cross-referenced this on Dec 1, 2023 from issue Signed-digit multi-comb ecmult_gen algorithm by sipa
3. 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.)
4. 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

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

   ---

   

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

   Duh. Done.
5. 
   real-or-random commented at 8:16 pm on December 1, 2023: contributor

   Concept ACK
6. real-or-random added the label refactor/smell on Dec 1, 2023
7. Add group.h ge/gej equality functions a47cd97d51
8. Add unit tests for group.h equality functions 60525f6c14
9. Replace ge_equals_ge[,j] calls with group.h equality calls 04af0ba162
10. sipa force-pushed on Dec 1, 2023
11. 
    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)

12. 
    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).

13. 
    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.

14. real-or-random approved
15. 
    real-or-random commented at 9:42 pm on December 1, 2023: contributor

    utACK 04af0ba162b152073455a5ccbb2c5833ae6d1d57
16. 
    stratospher commented at 5:21 am on December 2, 2023: contributor

    ACK 04af0ba.
17. real-or-random merged this on Dec 2, 2023
18. real-or-random closed this on Dec 2, 2023

    ---

Contributors
sipa real-or-random stratospher

Labels
refactor/smell