Merge #1774: refactor: split up internal pubkey serialization function into compressed/uncompressed variants

f5e815f remove secp256k1_eckey_pubkey_serialize function (Sebastian Falbesoner)
0d3659c use new `_eckey_pubkey_serialize{33,65}` functions in modules (ellswift,musig) (Sebastian Falbesoner)
adb76f8 use new `_eckey_pubkey_serialize{33,65}` functions in public API (Sebastian Falbesoner)
fc7458c introduce `secp256k1_eckey_pubkey_serialize{33,65}` functions (Sebastian Falbesoner)

Pull request description:

  This PR splits up the pubkey serialization function `secp256k1_eckey_pubkey_serialize` into two variants for the compressed (33 bytes) and uncompressed (65 bytes) public key output format each, where only non-infinity group elements as input are allowed. The motivation is to simplify call-sites significantly, as they currently need to introduce two variables and a VERIFY_CHECKs on the return value and the in/out size parameter within a pre-processor block, typically leading to 8 lines of code. By using the new functions, the code is reduced to a single line of code that just calls the function (see #1773). This is helpful for already existing modules on master (ellswift, musig) and upcoming ones (silentpayments, see #1765).

  One drawback is that the public API function `secp256k1_ec_pubkey_serialize` is now slightly more complex (we now call one of two functions instead of a single one, depending on whether the compressed flag is set or not), but that should hopefully not be a problem.

  The commits are intentionally kept small to ease review, happy to squash them if that is preferred.

  (Kudos to w0xlt for the initial idea (#1765 (review)) and to real-or-random for the suggestion to split the already existing function (#1773 (comment)).)

ACKs for top commit:
  real-or-random:
    utACK f5e815f
  w0xlt:
    ACK f5e815f

Tree-SHA512: da576bbeae477f31ba76c0001f8df08b51fe5e31d67b422a238348ead3341bf37f0c1509ad9d0a93b63e6d61c152707c85beabd02f4eac3b3bdcff129e0ea750

Author: real-or-random

src/eckey.h

@@ -15,7 +15,10 @@
 #include "ecmult_gen.h"
 
 static int secp256k1_eckey_pubkey_parse(secp256k1_ge *elem, const unsigned char *pub, size_t size);
-static int secp256k1_eckey_pubkey_serialize(secp256k1_ge *elem, unsigned char *pub, size_t *size, int compressed);
+/** Serialize a group element (that is not allowed to be infinity) to a compressed public key (33 bytes). */
+static void secp256k1_eckey_pubkey_serialize33(secp256k1_ge *elem, unsigned char *pub33);
+/** Serialize a group element (that is not allowed to be infinity) to an uncompressed public key (65 bytes). */
+static void secp256k1_eckey_pubkey_serialize65(secp256k1_ge *elem, unsigned char *pub65);
 
 static int secp256k1_eckey_privkey_tweak_add(secp256k1_scalar *key, const secp256k1_scalar *tweak);
 static int secp256k1_eckey_pubkey_tweak_add(secp256k1_ge *key, const secp256k1_scalar *tweak);

src/eckey_impl.h

@@ -35,24 +35,23 @@ static int secp256k1_eckey_pubkey_parse(secp256k1_ge *elem, const unsigned char
     }
 }
 
-static int secp256k1_eckey_pubkey_serialize(secp256k1_ge *elem, unsigned char *pub, size_t *size, int compressed) {
-    VERIFY_CHECK(compressed == 0 || compressed == 1);
+static void secp256k1_eckey_pubkey_serialize33(secp256k1_ge *elem, unsigned char *pub33) {
+    VERIFY_CHECK(!secp256k1_ge_is_infinity(elem));
 
-    if (secp256k1_ge_is_infinity(elem)) {
-        return 0;
-    }
     secp256k1_fe_normalize_var(&elem->x);
     secp256k1_fe_normalize_var(&elem->y);
-    secp256k1_fe_get_b32(&pub[1], &elem->x);
-    if (compressed) {
-        *size = 33;
-        pub[0] = secp256k1_fe_is_odd(&elem->y) ? SECP256K1_TAG_PUBKEY_ODD : SECP256K1_TAG_PUBKEY_EVEN;
-    } else {
-        *size = 65;
-        pub[0] = SECP256K1_TAG_PUBKEY_UNCOMPRESSED;
-        secp256k1_fe_get_b32(&pub[33], &elem->y);
-    }
-    return 1;
+    pub33[0] = secp256k1_fe_is_odd(&elem->y) ? SECP256K1_TAG_PUBKEY_ODD : SECP256K1_TAG_PUBKEY_EVEN;
+    secp256k1_fe_get_b32(&pub33[1], &elem->x);
+}
+
+static void secp256k1_eckey_pubkey_serialize65(secp256k1_ge *elem, unsigned char *pub65) {
+    VERIFY_CHECK(!secp256k1_ge_is_infinity(elem));
+
+    secp256k1_fe_normalize_var(&elem->x);
+    secp256k1_fe_normalize_var(&elem->y);
+    pub65[0] = SECP256K1_TAG_PUBKEY_UNCOMPRESSED;
+    secp256k1_fe_get_b32(&pub65[1], &elem->x);
+    secp256k1_fe_get_b32(&pub65[33], &elem->y);
 }
 
 static int secp256k1_eckey_privkey_tweak_add(secp256k1_scalar *key, const secp256k1_scalar *tweak) {

src/modules/ellswift/main_impl.h

@@ -406,19 +406,12 @@ int secp256k1_ellswift_encode(const secp256k1_context *ctx, unsigned char *ell64
     if (secp256k1_pubkey_load(ctx, &p, pubkey)) {
         secp256k1_fe t;
         unsigned char p64[64] = {0};
-        size_t ser_size;
-        int ser_ret;
         secp256k1_sha256 hash;
 
         /* Set up hasher state; the used RNG is H(pubkey || "\x00"*31 || rnd32 || cnt++), using
          * BIP340 tagged hash with tag "secp256k1_ellswift_encode". */
         secp256k1_ellswift_sha256_init_encode(&hash);
-        ser_ret = secp256k1_eckey_pubkey_serialize(&p, p64, &ser_size, 1);
-#ifdef VERIFY
-        VERIFY_CHECK(ser_ret && ser_size == 33);
-#else
-        (void)ser_ret;
-#endif
+        secp256k1_eckey_pubkey_serialize33(&p, p64);
         secp256k1_sha256_write(&hash, p64, sizeof(p64));
         secp256k1_sha256_write(&hash, rnd32, 32);
 

src/modules/musig/keyagg_impl.h

@@ -124,18 +124,11 @@ static void secp256k1_musig_keyaggcoef_internal(secp256k1_scalar *r, const unsig
     } else {
         secp256k1_sha256 sha;
         unsigned char buf[33];
-        size_t buflen = sizeof(buf);
-        int ret;
         secp256k1_musig_keyaggcoef_sha256(&sha);
         secp256k1_sha256_write(&sha, pks_hash, 32);
-        ret = secp256k1_eckey_pubkey_serialize(pk, buf, &buflen, 1);
-#ifdef VERIFY
         /* Serialization does not fail since the pk is not the point at infinity
          * (according to this function's precondition). */
-        VERIFY_CHECK(ret && buflen == sizeof(buf));
-#else
-        (void) ret;
-#endif
+        secp256k1_eckey_pubkey_serialize33(pk, buf);
         secp256k1_sha256_write(&sha, buf, sizeof(buf));
         secp256k1_sha256_finalize(&sha, buf);
         secp256k1_scalar_set_b32(r, buf, NULL);

src/modules/musig/session_impl.h

@@ -25,15 +25,8 @@ static void secp256k1_musig_ge_serialize_ext(unsigned char *out33, secp256k1_ge*
     if (secp256k1_ge_is_infinity(ge)) {
         memset(out33, 0, 33);
     } else {
-        int ret;
-        size_t size = 33;
-        ret = secp256k1_eckey_pubkey_serialize(ge, out33, &size, 1);
-#ifdef VERIFY
         /* Serialize must succeed because the point is not at infinity */
-        VERIFY_CHECK(ret && size == 33);
-#else
-        (void) ret;
-#endif
+        secp256k1_eckey_pubkey_serialize33(ge, out33);
     }
 }
 
@@ -224,15 +217,8 @@ int secp256k1_musig_pubnonce_serialize(const secp256k1_context* ctx, unsigned ch
         return 0;
     }
     for (i = 0; i < 2; i++) {
-        int ret;
-        size_t size = 33;
-        ret = secp256k1_eckey_pubkey_serialize(&ges[i], &out66[33*i], &size, 1);
-#ifdef VERIFY
         /* serialize must succeed because the point was just loaded */
-        VERIFY_CHECK(ret && size == 33);
-#else
-        (void) ret;
-#endif
+        secp256k1_eckey_pubkey_serialize33(&ges[i], &out66[33*i]);
     }
     return 1;
 }
@@ -398,11 +384,9 @@ static int secp256k1_musig_nonce_gen_internal(const secp256k1_context* ctx, secp
     secp256k1_gej nonce_ptj[2];
     int i;
     unsigned char pk_ser[33];
-    size_t pk_ser_len = sizeof(pk_ser);
     unsigned char aggpk_ser[32];
     unsigned char *aggpk_ser_ptr = NULL;
     secp256k1_ge pk;
-    int pk_serialize_success;
     int ret = 1;
 
     ARG_CHECK(pubnonce != NULL);
@@ -429,15 +413,8 @@ static int secp256k1_musig_nonce_gen_internal(const secp256k1_context* ctx, secp
     if (!secp256k1_pubkey_load(ctx, &pk, pubkey)) {
         return 0;
     }
-    pk_serialize_success = secp256k1_eckey_pubkey_serialize(&pk, pk_ser, &pk_ser_len, 1);
-
-#ifdef VERIFY
     /* A pubkey cannot be the point at infinity */
-    VERIFY_CHECK(pk_serialize_success);
-    VERIFY_CHECK(pk_ser_len == sizeof(pk_ser));
-#else
-    (void) pk_serialize_success;
-#endif
+    secp256k1_eckey_pubkey_serialize33(&pk, pk_ser);
 
     secp256k1_nonce_function_musig(k, input_nonce, msg32, seckey, pk_ser, aggpk_ser_ptr, extra_input32);
     VERIFY_CHECK(!secp256k1_scalar_is_zero(&k[0]));

src/secp256k1.c

@@ -280,10 +280,14 @@ int secp256k1_ec_pubkey_serialize(const secp256k1_context* ctx, unsigned char *o
     ARG_CHECK(pubkey != NULL);
     ARG_CHECK((flags & SECP256K1_FLAGS_TYPE_MASK) == SECP256K1_FLAGS_TYPE_COMPRESSION);
     if (secp256k1_pubkey_load(ctx, &Q, pubkey)) {
-        ret = secp256k1_eckey_pubkey_serialize(&Q, output, &len, !!(flags & SECP256K1_FLAGS_BIT_COMPRESSION));
-        if (ret) {
-            *outputlen = len;
+        if (flags & SECP256K1_FLAGS_BIT_COMPRESSION) {
+            secp256k1_eckey_pubkey_serialize33(&Q, output);
+            *outputlen = 33;
+        } else {
+            secp256k1_eckey_pubkey_serialize65(&Q, output);
+            *outputlen = 65;
         }
+        ret = 1;
     }
     return ret;
 }

src/tests.c

@@ -4315,8 +4315,6 @@ static void test_point_times_order(const secp256k1_gej *point) {
     secp256k1_scalar nx;
     secp256k1_gej res1, res2;
     secp256k1_ge res3;
-    unsigned char pub[65];
-    size_t psize = 65;
     testutil_random_scalar_order_test(&x);
     secp256k1_scalar_negate(&nx, &x);
     secp256k1_ecmult(&res1, point, &x, &x); /* calc res1 = x * point + x * G; */
@@ -4326,9 +4324,6 @@ static void test_point_times_order(const secp256k1_gej *point) {
     secp256k1_ge_set_gej(&res3, &res1);
     CHECK(secp256k1_ge_is_infinity(&res3));
     CHECK(secp256k1_ge_is_valid_var(&res3) == 0);
-    CHECK(secp256k1_eckey_pubkey_serialize(&res3, pub, &psize, 0) == 0);
-    psize = 65;
-    CHECK(secp256k1_eckey_pubkey_serialize(&res3, pub, &psize, 1) == 0);
     /* check zero/one edge cases */
     secp256k1_ecmult(&res1, point, &secp256k1_scalar_zero, &secp256k1_scalar_zero);
     secp256k1_ge_set_gej(&res3, &res1);
@@ -5435,7 +5430,6 @@ static void test_ecmult_accumulate(secp256k1_sha256* acc, const secp256k1_scalar
     secp256k1_gej rj1, rj2, rj3, rj4, rj5, rj6, gj, infj;
     secp256k1_ge r;
     unsigned char bytes[65];
-    size_t size = 65;
     secp256k1_gej_set_ge(&gj, &secp256k1_ge_const_g);
     secp256k1_gej_set_infinity(&infj);
     secp256k1_ecmult_gen(&CTX->ecmult_gen_ctx, &rj1, x);
@@ -5456,9 +5450,8 @@ static void test_ecmult_accumulate(secp256k1_sha256* acc, const secp256k1_scalar
         secp256k1_sha256_write(acc, zerobyte, 1);
     } else {
         /* Store other points using their uncompressed serialization. */
-        secp256k1_eckey_pubkey_serialize(&r, bytes, &size, 0);
-        CHECK(size == 65);
-        secp256k1_sha256_write(acc, bytes, size);
+        secp256k1_eckey_pubkey_serialize65(&r, bytes);
+        secp256k1_sha256_write(acc, bytes, sizeof(bytes));
     }
 }
 
@@ -6543,16 +6536,18 @@ static void test_random_pubkeys(void) {
         size_t size = len;
         firstb = in[0];
         /* If the pubkey can be parsed, it should round-trip... */
-        CHECK(secp256k1_eckey_pubkey_serialize(&elem, out, &size, len == 33));
-        CHECK(size == len);
+        if (len == 33) {
+            secp256k1_eckey_pubkey_serialize33(&elem, out);
+        } else {
+            secp256k1_eckey_pubkey_serialize65(&elem, out);
+        }
         CHECK(secp256k1_memcmp_var(&in[1], &out[1], len-1) == 0);
         /* ... except for the type of hybrid inputs. */
         if ((in[0] != 6) && (in[0] != 7)) {
             CHECK(in[0] == out[0]);
         }
         size = 65;
-        CHECK(secp256k1_eckey_pubkey_serialize(&elem, in, &size, 0));
-        CHECK(size == 65);
+        secp256k1_eckey_pubkey_serialize65(&elem, in);
         CHECK(secp256k1_eckey_pubkey_parse(&elem2, in, size));
         CHECK(secp256k1_ge_eq_var(&elem2, &elem));
         /* Check that the X9.62 hybrid type is checked. */
@@ -6567,7 +6562,7 @@ static void test_random_pubkeys(void) {
         }
         if (res) {
             CHECK(secp256k1_ge_eq_var(&elem, &elem2));
-            CHECK(secp256k1_eckey_pubkey_serialize(&elem, out, &size, 0));
+            secp256k1_eckey_pubkey_serialize65(&elem, out);
             CHECK(secp256k1_memcmp_var(&in[1], &out[1], 64) == 0);
         }
     }