From 9fcaf25be7ccd96f6c4da9b292daf6ff4cf10e3b Mon Sep 17 00:00:00 2001
From: Jesse Posner <jesse.posner@gmail.com>
Date: Wed, 28 Aug 2024 16:04:10 -0700
Subject: [PATCH] Update group element helpers

Based on:
57de68994ae18d20b0b6e1a9e4531c3d88b5ec81 and 3f9bb4d868a2a27caacdaf19b08ce91ce73c1fb4

Responds to:
https://github.com/BlockstreamResearch/secp256k1-zkp/pull/278#discussion_r1716567383
---
 src/group.h                      | 14 ++++++++-----
 src/group_impl.h                 | 36 ++++++++++++++++----------------
 src/modules/frost/keygen_impl.h  |  4 ++--
 src/modules/frost/session_impl.h |  4 ++--
 src/modules/musig/keyagg_impl.h  |  4 ++--
 src/modules/musig/session_impl.h |  4 ++--
 src/tests.c                      | 22 +++++++++++++++----
 7 files changed, 53 insertions(+), 35 deletions(-)

diff --git a/src/group.h b/src/group.h
index 43cf57fd7..11bfc6262 100644
--- a/src/group.h
+++ b/src/group.h
@@ -185,12 +185,20 @@ static void secp256k1_gej_rescale(secp256k1_gej *r, const secp256k1_fe *b);
 
 /** Convert a group element that is not infinity to a 64-byte array. The output
  *  array is platform-dependent. */
-static void secp256k1_ge_to_bytes(unsigned char *buf, secp256k1_ge *a);
+static void secp256k1_ge_to_bytes(unsigned char *buf, const secp256k1_ge *a);
 
 /** Convert a 64-byte array into group element. This function assumes that the
  *  provided buffer correctly encodes a group element. */
 static void secp256k1_ge_from_bytes(secp256k1_ge *r, const unsigned char *buf);
 
+/** Convert a group element (that is allowed to be infinity) to a 64-byte
+ *  array. The output array is platform-dependent. */
+static void secp256k1_ge_to_bytes_ext(unsigned char *data, const secp256k1_ge *ge);
+
+/** Convert a 64-byte array into a group element. This function assumes that the
+ *  provided buffer is the output of secp256k1_ge_to_bytes_ext. */
+static void secp256k1_ge_from_bytes_ext(secp256k1_ge *ge, const unsigned char *data);
+
 /** Determine if a point (which is assumed to be on the curve) is in the correct (sub)group of the curve.
  *
  * In normal mode, the used group is secp256k1, which has cofactor=1 meaning that every point on the curve is in the
@@ -202,10 +210,6 @@ static void secp256k1_ge_from_bytes(secp256k1_ge *r, const unsigned char *buf);
  */
 static int secp256k1_ge_is_in_correct_subgroup(const secp256k1_ge* ge);
 
-static void secp256k1_point_save_ext(unsigned char *data, secp256k1_ge *ge);
-
-static void secp256k1_point_load_ext(secp256k1_ge *ge, const unsigned char *data);
-
 /** Check invariants on an affine group element (no-op unless VERIFY is enabled). */
 static void secp256k1_ge_verify(const secp256k1_ge *a);
 #define SECP256K1_GE_VERIFY(a) secp256k1_ge_verify(a)
diff --git a/src/group_impl.h b/src/group_impl.h
index fb475217d..f073b5059 100644
--- a/src/group_impl.h
+++ b/src/group_impl.h
@@ -914,23 +914,6 @@ static int secp256k1_gej_has_quad_y_var(const secp256k1_gej *a) {
     return secp256k1_fe_is_square_var(&yz);
 }
 
-static void secp256k1_point_save_ext(unsigned char *data, secp256k1_ge *ge) {
-    if (secp256k1_ge_is_infinity(ge)) {
-        memset(data, 0, 64);
-    } else {
-        secp256k1_ge_to_bytes(data, ge);
-    }
-}
-
-static void secp256k1_point_load_ext(secp256k1_ge *ge, const unsigned char *data) {
-    unsigned char zeros[64] = { 0 };
-    if (secp256k1_memcmp_var(data, zeros, sizeof(zeros)) == 0) {
-        secp256k1_ge_set_infinity(ge);
-    } else {
-        secp256k1_ge_from_bytes(ge, data);
-    }
-}
-
 static int secp256k1_ge_is_in_correct_subgroup(const secp256k1_ge* ge) {
 #ifdef EXHAUSTIVE_TEST_ORDER
     secp256k1_gej out;
@@ -982,7 +965,7 @@ static int secp256k1_ge_x_frac_on_curve_var(const secp256k1_fe *xn, const secp25
      return secp256k1_fe_is_square_var(&r);
 }
 
-static void secp256k1_ge_to_bytes(unsigned char *buf, secp256k1_ge *a) {
+static void secp256k1_ge_to_bytes(unsigned char *buf, const secp256k1_ge *a) {
     secp256k1_ge_storage s;
 
     /* We require that the secp256k1_ge_storage type is exactly 64 bytes.
@@ -1002,4 +985,21 @@ static void secp256k1_ge_from_bytes(secp256k1_ge *r, const unsigned char *buf) {
     secp256k1_ge_from_storage(r, &s);
 }
 
+static void secp256k1_ge_to_bytes_ext(unsigned char *data, const secp256k1_ge *ge) {
+    if (secp256k1_ge_is_infinity(ge)) {
+        memset(data, 0, 64);
+    } else {
+        secp256k1_ge_to_bytes(data, ge);
+    }
+}
+
+static void secp256k1_ge_from_bytes_ext(secp256k1_ge *ge, const unsigned char *data) {
+    unsigned char zeros[64] = { 0 };
+    if (secp256k1_memcmp_var(data, zeros, sizeof(zeros)) == 0) {
+        secp256k1_ge_set_infinity(ge);
+    } else {
+        secp256k1_ge_from_bytes(ge, data);
+    }
+}
+
 #endif /* SECP256K1_GROUP_IMPL_H */
diff --git a/src/modules/frost/keygen_impl.h b/src/modules/frost/keygen_impl.h
index 1c3b90182..84c884109 100644
--- a/src/modules/frost/keygen_impl.h
+++ b/src/modules/frost/keygen_impl.h
@@ -34,7 +34,7 @@ static void secp256k1_tweak_cache_save(secp256k1_frost_tweak_cache *cache, secp2
     unsigned char *ptr = cache->data;
     memcpy(ptr, secp256k1_frost_tweak_cache_magic, 4);
     ptr += 4;
-    secp256k1_point_save_ext(ptr, &cache_i->pk);
+    secp256k1_ge_to_bytes_ext(ptr, &cache_i->pk);
     ptr += 64;
     *ptr = cache_i->parity_acc;
     ptr += 1;
@@ -45,7 +45,7 @@ static int secp256k1_tweak_cache_load(const secp256k1_context* ctx, secp256k1_tw
     const unsigned char *ptr = cache->data;
     ARG_CHECK(secp256k1_memcmp_var(ptr, secp256k1_frost_tweak_cache_magic, 4) == 0);
     ptr += 4;
-    secp256k1_point_load_ext(&cache_i->pk, ptr);
+    secp256k1_ge_from_bytes_ext(&cache_i->pk, ptr);
     ptr += 64;
     cache_i->parity_acc = *ptr & 1;
     ptr += 1;
diff --git a/src/modules/frost/session_impl.h b/src/modules/frost/session_impl.h
index 125e07a2b..c22a4d0b4 100644
--- a/src/modules/frost/session_impl.h
+++ b/src/modules/frost/session_impl.h
@@ -58,7 +58,7 @@ static void secp256k1_frost_pubnonce_save(secp256k1_frost_pubnonce* nonce, secp2
     int i;
     memcpy(&nonce->data[0], secp256k1_frost_pubnonce_magic, 4);
     for (i = 0; i < 2; i++) {
-        secp256k1_point_save_ext(nonce->data + 4+64*i, &ge[i]);
+        secp256k1_ge_to_bytes_ext(nonce->data + 4+64*i, &ge[i]);
     }
 }
 
@@ -69,7 +69,7 @@ static int secp256k1_frost_pubnonce_load(const secp256k1_context* ctx, secp256k1
 
     ARG_CHECK(secp256k1_memcmp_var(&nonce->data[0], secp256k1_frost_pubnonce_magic, 4) == 0);
     for (i = 0; i < 2; i++) {
-        secp256k1_point_load_ext(&ge[i], nonce->data + 4+64*i);
+        secp256k1_ge_from_bytes_ext(&ge[i], nonce->data + 4+64*i);
     }
     return 1;
 }
diff --git a/src/modules/musig/keyagg_impl.h b/src/modules/musig/keyagg_impl.h
index 992fd9a35..1c221d77b 100644
--- a/src/modules/musig/keyagg_impl.h
+++ b/src/modules/musig/keyagg_impl.h
@@ -35,7 +35,7 @@ static void secp256k1_keyagg_cache_save(secp256k1_musig_keyagg_cache *cache, sec
     ptr += 4;
     secp256k1_ge_to_bytes(ptr, &cache_i->pk);
     ptr += 64;
-    secp256k1_point_save_ext(ptr, &cache_i->second_pk);
+    secp256k1_ge_to_bytes_ext(ptr, &cache_i->second_pk);
     ptr += 64;
     memcpy(ptr, cache_i->pk_hash, 32);
     ptr += 32;
@@ -50,7 +50,7 @@ static int secp256k1_keyagg_cache_load(const secp256k1_context* ctx, secp256k1_k
     ptr += 4;
     secp256k1_ge_from_bytes(&cache_i->pk, ptr);
     ptr += 64;
-    secp256k1_point_load_ext(&cache_i->second_pk, ptr);
+    secp256k1_ge_from_bytes_ext(&cache_i->second_pk, ptr);
     ptr += 64;
     memcpy(cache_i->pk_hash, ptr, 32);
     ptr += 32;
diff --git a/src/modules/musig/session_impl.h b/src/modules/musig/session_impl.h
index ff87f2fd3..cab3376f9 100644
--- a/src/modules/musig/session_impl.h
+++ b/src/modules/musig/session_impl.h
@@ -84,7 +84,7 @@ static void secp256k1_musig_aggnonce_save(secp256k1_musig_aggnonce* nonce, secp2
     int i;
     memcpy(&nonce->data[0], secp256k1_musig_aggnonce_magic, 4);
     for (i = 0; i < 2; i++) {
-        secp256k1_point_save_ext(&nonce->data[4 + 64*i], &ge[i]);
+        secp256k1_ge_to_bytes_ext(&nonce->data[4 + 64*i], &ge[i]);
     }
 }
 
@@ -93,7 +93,7 @@ static int secp256k1_musig_aggnonce_load(const secp256k1_context* ctx, secp256k1
 
     ARG_CHECK(secp256k1_memcmp_var(&nonce->data[0], secp256k1_musig_aggnonce_magic, 4) == 0);
     for (i = 0; i < 2; i++) {
-        secp256k1_point_load_ext(&ge[i], &nonce->data[4 + 64*i]);
+        secp256k1_ge_from_bytes_ext(&ge[i], &nonce->data[4 + 64*i]);
     }
     return 1;
 }
diff --git a/src/tests.c b/src/tests.c
index 00eb3bfe6..2ebdab828 100644
--- a/src/tests.c
+++ b/src/tests.c
@@ -4075,13 +4075,27 @@ static void test_add_neg_y_diff_x(void) {
 static void test_ge_bytes(void) {
     int i;
 
-    for (i = 0; i < COUNT; i++) {
+    for (i = 0; i < COUNT + 1; i++) {
         unsigned char buf[64];
         secp256k1_ge p, q;
 
-        random_group_element_test(&p);
-        secp256k1_ge_to_bytes(buf, &p);
-        secp256k1_ge_from_bytes(&q, buf);
+        if (i == 0) {
+            secp256k1_ge_set_infinity(&p);
+        } else {
+            random_group_element_test(&p);
+        }
+
+        if (!secp256k1_ge_is_infinity(&p)) {
+            secp256k1_ge_to_bytes(buf, &p);
+
+            secp256k1_ge_from_bytes(&q, buf);
+            CHECK(secp256k1_ge_eq_var(&p, &q));
+
+            secp256k1_ge_from_bytes_ext(&q, buf);
+            CHECK(secp256k1_ge_eq_var(&p, &q));
+        }
+        secp256k1_ge_to_bytes_ext(buf, &p);
+        secp256k1_ge_from_bytes_ext(&q, buf);
         CHECK(secp256k1_ge_eq_var(&p, &q));
     }
 }