more fixes addressing PR comments (docs remain)

Author: siv2r

.github/workflows/ci.yml

@@ -704,7 +704,7 @@ jobs:
           - { WIDEMUL: 'int128', RECOVERY: 'yes' }
           - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes', EXPERIMENTAL: 'yes', ECDSA_S2C: 'yes', RANGEPROOF: 'yes', WHITELIST: 'yes', GENERATOR: 'yes', MUSIG: 'yes', ECDSAADAPTOR: 'yes', BPPP: 'yes', SCHNORRSIG_HALFAGG: 'yes', SCHNORRADAPTOR: 'yes' }
           - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes', EXPERIMENTAL: 'yes', ECDSA_S2C: 'yes', RANGEPROOF: 'yes', WHITELIST: 'yes', GENERATOR: 'yes', MUSIG: 'yes', ECDSAADAPTOR: 'yes', BPPP: 'yes', SCHNORRSIG_HALFAGG: 'yes', SCHNORRADAPTOR: 'yes', CC: 'gcc' }
-          - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes', EXPERIMENTAL: 'yes', ECDSA_S2C: 'yes', RANGEPROOF: 'yes', WHITELIST: 'yes', GENERATOR: 'yes', MUSIG: 'yes', ECDSAADAPTOR: 'yes', BPPP: 'yes', SCHNORRSIG_HALFAGG: 'yes', SCHNORRADAPTOR: 'yes',          WRAPPER_CMD: 'valgrind --error-exitcode=42', SECP256K1_TEST_ITERS: 2 }
+          - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes', EXPERIMENTAL: 'yes', ECDSA_S2C: 'yes', RANGEPROOF: 'yes', WHITELIST: 'yes', GENERATOR: 'yes', MUSIG: 'yes', ECDSAADAPTOR: 'yes', BPPP: 'yes', SCHNORRSIG_HALFAGG: 'yes', SCHNORRADAPTOR: 'yes', WRAPPER_CMD: 'valgrind --error-exitcode=42', SECP256K1_TEST_ITERS: 2 }
           - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes', EXPERIMENTAL: 'yes', ECDSA_S2C: 'yes', RANGEPROOF: 'yes', WHITELIST: 'yes', GENERATOR: 'yes', MUSIG: 'yes', ECDSAADAPTOR: 'yes', BPPP: 'yes', SCHNORRSIG_HALFAGG: 'yes', SCHNORRADAPTOR: 'yes', CC: 'gcc', WRAPPER_CMD: 'valgrind --error-exitcode=42', SECP256K1_TEST_ITERS: 2 }
           - { WIDEMUL: 'int128', RECOVERY: 'yes', ECDH: 'yes', SCHNORRSIG: 'yes', ELLSWIFT: 'yes', EXPERIMENTAL: 'yes', ECDSA_S2C: 'yes', RANGEPROOF: 'yes', WHITELIST: 'yes', GENERATOR: 'yes', MUSIG: 'yes', ECDSAADAPTOR: 'yes', BPPP: 'yes', SCHNORRSIG_HALFAGG: 'yes', SCHNORRADAPTOR: 'yes', CPPFLAGS: '-DVERIFY', CTIMETESTS: 'no' }
           - BUILD: 'distcheck'

ci/ci.sh

@@ -13,8 +13,8 @@ print_environment() {
     # does not rely on bash.
     for var in WERROR_CFLAGS MAKEFLAGS BUILD \
             ECMULTWINDOW ECMULTGENPRECISION ASM WIDEMUL WITH_VALGRIND EXTRAFLAGS \
-            EXPERIMENTAL ECDH RECOVERY SCHNORRSIG SCHNORRSIG_HALFAGG ELLSWIFT \
-            ECDSA_S2C GENERATOR RANGEPROOF WHITELIST MUSIG ECDSAADAPTOR BPPP \
+            EXPERIMENTAL ECDH RECOVERY SCHNORRSIG SCHNORRSIG_HALFAGG SCHNORRADAPTOR \
+            ELLSWIFT ECDSA_S2C GENERATOR RANGEPROOF WHITELIST MUSIG ECDSAADAPTOR BPPP \
             SECP256K1_TEST_ITERS BENCH SECP256K1_BENCH_ITERS CTIMETESTS\
             EXAMPLES \
             HOST WRAPPER_CMD \

include/secp256k1_schnorr_adaptor.h

@@ -66,7 +66,7 @@ extern "C" {
 
 /** A pointer to a function to deterministically generate a nonce.
  *
- *  Same as secp256k1_schnorrsig_nonce function with the exception of using the
+ *  Same as `secp256k1_nonce_function_hardened` with the exception of using the
  *  compressed 33-byte encoding for the adaptor argument.
  *
  *  Returns: 1 if a nonce was successfully generated. 0 will cause signing to
@@ -96,12 +96,19 @@ typedef int (*secp256k1_nonce_function_hardened_schnorr_adaptor)(
     void *data
 );
 
-/** A Schnorr Adaptor nonce generation function. */
+/** A modified BIP-340 nonce generation function. If a data pointer is passed, it is
+ *  assumed to be a pointer to 32 bytes of auxiliary random data as defined in BIP-340.
+ *  If the data pointer is NULL, the nonce derivation procedure uses a zeroed 32-byte
+ *  auxiliary random data. The hash will be tagged with algo after removing all
+ *  terminating null bytes.
+ */
 SECP256K1_API const secp256k1_nonce_function_hardened_schnorr_adaptor secp256k1_nonce_function_schnorr_adaptor;
 
-/** Creates a Schnorr pre-signature.
- *  TODO: this description could be improved & do we really need the
- *  below paragraph?
+/** Creates a pre-signature for a given message and adaptor point.
+ *
+ *  The pre-signature can be converted into a valid BIP-340 Schnorr signature
+ *  (using `schnorr_adaptor_adapt`) by combining it with the discrete logarithm
+ *  of the adaptor point.
  *
  *  This function only signs 32-byte messages. If you have messages of a
  *  different size (or the same size but without a context-specific tag
@@ -111,16 +118,16 @@ SECP256K1_API const secp256k1_nonce_function_hardened_schnorr_adaptor secp256k1_
  *  signatures from being valid in multiple contexts by accident.
  *
  *  Returns 1 on success, 0 on failure.
- *  Args:       ctx: pointer to a context object (not secp256k1_context_static).
- *  Out:   pre_sig65: pointer to a 65-byte array to store the serialized pre-signature.
- *  In:       msg32: the 32-byte message being signed.
- *          keypair: pointer to an initialized keypair.
- *          adaptor: pointer to an adaptor point encoded as a public key.
- *       aux_rand32: pointer to arbitrary data used by the nonce generation
- *                   function (can be NULL). If it is non-NULL and
- *                   secp256k1_nonce_function_schnorr_adaptor is used, then
- *                   aux_rand32 must be a pointer to 32-byte auxiliary randomness
- *                   as per BIP-340.
+ *  Args:        ctx: pointer to a context object (not secp256k1_context_static).
+ *  Out:   pre_sig65: pointer to a 65-byte array to store the pre-signature.
+ *  In:        msg32: the 32-byte message being signed.
+ *           keypair: pointer to an initialized keypair.
+ *           adaptor: pointer to an adaptor point encoded as a public key.
+ *        aux_rand32: pointer to arbitrary data used by the nonce generation
+ *                    function (can be NULL). If it is non-NULL and
+ *                    secp256k1_nonce_function_schnorr_adaptor is used, then
+ *                    aux_rand32 must be a pointer to 32-byte auxiliary randomness
+ *                    as per BIP-340.
  */
 SECP256K1_API int secp256k1_schnorr_adaptor_presign(
     const secp256k1_context *ctx,
@@ -131,12 +138,17 @@ SECP256K1_API int secp256k1_schnorr_adaptor_presign(
     const unsigned char *aux_rand32
 ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4) SECP256K1_ARG_NONNULL(5);
 
-/** Extract an adaptor point from the pre-signature.
+/** Extracts an adaptor point from the pre-signature.
+ *  TODO: Note: returning 1 doesn't neccessarily guarantee
+ *  that the extracted adaptor point is correct.
+ *  If this funtion passes, secp256k1_schnorr_adaptor_adapt called with
+ *  the secret key corresponding to public key T and presig65 outputs a
+ *  valid Schnorr signature.
  *
  *  Returns 1 on success, 0 on failure.
  *  Args:         ctx: pointer to a context object.
- *  Out:      adaptor: pointer to an adaptor point.
- *  In:      pre_sig65: pointer to a 65-byte pre-signature.
+ *  Out:      adaptor: pointer to store the adaptor point.
+ *  In:     pre_sig65: pointer to a 65-byte pre-signature.
  *              msg32: the 32-byte message being signed.
  *             pubkey: pointer to the x-only public key used to
  *                     generate the `pre_sig65`
@@ -150,15 +162,19 @@ SECP256K1_API int secp256k1_schnorr_adaptor_extract(
 ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4) SECP256K1_ARG_NONNULL(5);
 
 /** Creates a signature from a pre-signature and a secret adaptor.
+ *
+ *  TODO: Note: returning 1 doesn't neccessarily guarantee
+ *  that the extracted adaptor point is correct.
  *
  *  If the sec_adaptor32 argument is incorrect, the output signature will be
  *  invalid. This function does not verify the signature.
  *
  *  Returns 1 on success, 0 on failure.
  *  Args:           ctx: pointer to a context object.
- *  Out:          sig64: 64-byte signature. This pointer may point to the same
- *                     memory area as `pre_sig65`.
- *  In:       pre_sig65: 65-byte pre-signature
+ *  Out:          sig64: pointer to a 64-byte array to store the adapted
+ *                       pre-signature. This pointer may point to the same
+ *                       memory area as `pre_sig65`.
+ *  In:       pre_sig65: 65-byte pre-signature corresponding to `sec_adaptor32`.
  *        sec_adaptor32: pointer to a 32-byte secret adaptor.
  */
 SECP256K1_API int secp256k1_schnorr_adaptor_adapt(
@@ -171,12 +187,14 @@ SECP256K1_API int secp256k1_schnorr_adaptor_adapt(
 /** Extracts a secret adaptor from a pre-signature and the corresponding
  *  signature.
  *
+ *  TODO: Note: returning 1 doesn't neccessarily guarantee
+ *  that the extracted adaptor point is correct.
+ *
  *  Returns 1 on success, 0 on failure.
  *  Args:           ctx: pointer to a context object.
- *  Out:  sec_adaptor32: 32-byte secret adaptor.
+ *  Out:  sec_adaptor32: pointer to a 32-byte array to store the secret adaptor.
  *  In:       pre_sig65: the pre-signature corresponding to `sig64`
- *                sig64: complete, valid 64-byte signature.
- * TODO: swap the presig and sig arg order
+ *                sig64: complete, valid 64-byte BIP-340 Schnorr signature.
  */
 SECP256K1_API int secp256k1_schnorr_adaptor_extract_sec(
     const secp256k1_context *ctx,

src/modules/schnorr_adaptor/main_impl.h

@@ -112,6 +112,7 @@ static int secp256k1_schnorr_adaptor_presign_internal(const secp256k1_context *c
     unsigned char seckey[32];
     unsigned char adaptor_buff[33];
     size_t cmprssd_len = 33; /* for serializing `adaptor_ge` and `pre_sig65` */
+    int serialize_ret = 0;
     int ret = 1;
 
     VERIFY_CHECK(ctx != NULL);
@@ -125,6 +126,11 @@ static int secp256k1_schnorr_adaptor_presign_internal(const secp256k1_context *c
         noncefp = secp256k1_nonce_function_schnorr_adaptor;
     }
 
+    /* T := adaptor_ge */
+    if(!secp256k1_pubkey_load(ctx, &adaptor_ge, adaptor)){
+        return 0;
+    }
+
     ret &= secp256k1_keypair_load(ctx, &sk, &pk, keypair);
     /* Because we are signing for a x-only pubkey, the secret key is negated
      * before signing if the point corresponding to the secret key does not
@@ -136,9 +142,8 @@ static int secp256k1_schnorr_adaptor_presign_internal(const secp256k1_context *c
     /* Generate the nonce k */
     secp256k1_scalar_get_b32(seckey, &sk);
     secp256k1_fe_get_b32(pk_buf, &pk.x);
-    /* T := adaptor_ge */
-    ret &= secp256k1_pubkey_load(ctx, &adaptor_ge, adaptor);
-    ret &= secp256k1_eckey_pubkey_serialize(&adaptor_ge, adaptor_buff, &cmprssd_len, 1);
+    serialize_ret = secp256k1_eckey_pubkey_serialize(&adaptor_ge, adaptor_buff, &cmprssd_len, 1);
+    VERIFY_CHECK(serialize_ret);
     ret &= !!noncefp(nonce32, msg32, seckey, adaptor_buff, pk_buf, schnorr_adaptor_algo, sizeof(schnorr_adaptor_algo), ndata);
     secp256k1_scalar_set_b32(&k, nonce32, NULL);
     ret &= !secp256k1_scalar_is_zero(&k);
@@ -176,7 +181,10 @@ static int secp256k1_schnorr_adaptor_presign_internal(const secp256k1_context *c
     if (secp256k1_fe_is_odd(&rp.y)) {
         secp256k1_scalar_negate(&k, &k);
     }
-    ret &= secp256k1_eckey_pubkey_serialize(&rp, pre_sig65, &cmprssd_len, 1);
+    serialize_ret = secp256k1_eckey_pubkey_serialize(&rp, pre_sig65, &cmprssd_len, 1);
+    /* R' is not the point at infinity with overwhelming probability */
+    VERIFY_CHECK(serialize_ret);
+    (void) serialize_ret;
 
     secp256k1_schnorrsig_challenge(&e, &pre_sig65[1], msg32, 32, pk_buf);
     secp256k1_scalar_mul(&e, &e, &sk);
@@ -273,9 +281,11 @@ int secp256k1_schnorr_adaptor_adapt(const secp256k1_context *ctx, unsigned char
     VERIFY_CHECK(ctx != NULL);
     ARG_CHECK(sig64 != NULL);
     ARG_CHECK(pre_sig65 != NULL);
-    ARG_CHECK(pre_sig65[0] == SECP256K1_TAG_PUBKEY_EVEN || pre_sig65[0] == SECP256K1_TAG_PUBKEY_ODD);
     ARG_CHECK(sec_adaptor32 != NULL);
 
+    if (pre_sig65[0] != SECP256K1_TAG_PUBKEY_EVEN && pre_sig65[0] != SECP256K1_TAG_PUBKEY_ODD) {
+        return 0;
+    }
     secp256k1_scalar_set_b32(&s, &pre_sig65[33], &overflow);
     if (overflow) {
         return 0;
@@ -300,10 +310,11 @@ int secp256k1_schnorr_adaptor_adapt(const secp256k1_context *ctx, unsigned char
     if (pre_sig65[0] == SECP256K1_TAG_PUBKEY_ODD) {
         secp256k1_scalar_negate(&t, &t);
     }
-
     secp256k1_scalar_add(&s, &s, &t);
     secp256k1_scalar_get_b32(&sig64[32], &s);
     memmove(sig64, &pre_sig65[1], 32);
+
+    secp256k1_memczero(sig64, 64, !ret);
     secp256k1_scalar_clear(&t);
     return ret;
 }
@@ -317,16 +328,17 @@ int secp256k1_schnorr_adaptor_extract_sec(const secp256k1_context *ctx, unsigned
     VERIFY_CHECK(ctx != NULL);
     ARG_CHECK(sec_adaptor32 != NULL);
     ARG_CHECK(pre_sig65 != NULL);
-    ARG_CHECK(pre_sig65[0] == SECP256K1_TAG_PUBKEY_EVEN || pre_sig65[0] == SECP256K1_TAG_PUBKEY_ODD);
     ARG_CHECK(sig64 != NULL);
 
-    secp256k1_scalar_set_b32(&t, &sig64[32], &overflow);
-    ret &= !overflow;
-
+    if (pre_sig65[0] != SECP256K1_TAG_PUBKEY_EVEN && pre_sig65[0] != SECP256K1_TAG_PUBKEY_ODD) {
+        return 0;
+    }
     secp256k1_scalar_set_b32(&s, &pre_sig65[33], &overflow);
     if (overflow) {
         return 0;
     }
+    secp256k1_scalar_set_b32(&t, &sig64[32], &overflow);
+    ret &= !overflow;
 
     /*TODO: should we parse presig[0:33] & sig[0:32], to make sure the presig &
      * has valid public nonce point?
@@ -348,8 +360,9 @@ int secp256k1_schnorr_adaptor_extract_sec(const secp256k1_context *ctx, unsigned
     if (pre_sig65[0] == SECP256K1_TAG_PUBKEY_ODD) {
         secp256k1_scalar_negate(&t, &t);
     }
-
     secp256k1_scalar_get_b32(sec_adaptor32, &t);
+
+    secp256k1_memczero(sec_adaptor32, 32, !ret);
     secp256k1_scalar_clear(&t);
     return ret;
 }

src/modules/schnorr_adaptor/tests_impl.h

@@ -151,16 +151,16 @@ static void test_schnorr_adaptor_api(void) {
     CHECK(secp256k1_schnorr_adaptor_adapt(CTX, sig, pre_sig, sec_adaptor) == 1);
     CHECK_ILLEGAL(CTX, secp256k1_schnorr_adaptor_adapt(CTX, NULL, pre_sig, sec_adaptor));
     CHECK_ILLEGAL(CTX, secp256k1_schnorr_adaptor_adapt(CTX, sig, NULL, sec_adaptor));
-    /* invokes the ARG_CHECK on pre_sig[0] */
-    CHECK_ILLEGAL(CTX, secp256k1_schnorr_adaptor_adapt(CTX, sig, invalid_pre_sig, sec_adaptor));
+    /* invalid pre_sig[0] byte */
+    CHECK(secp256k1_schnorr_adaptor_adapt(CTX, sig, invalid_pre_sig, sec_adaptor) == 0);
     CHECK_ILLEGAL(CTX, secp256k1_schnorr_adaptor_adapt(CTX, sig, pre_sig, NULL));
 
     CHECK(secp256k1_schnorr_adaptor_adapt(CTX, sig, pre_sig, sec_adaptor) == 1);
     CHECK(secp256k1_schnorr_adaptor_extract_sec(CTX, extracted_sec_adaptor, pre_sig, sig) == 1);
     CHECK_ILLEGAL(CTX, secp256k1_schnorr_adaptor_extract_sec(CTX, NULL, pre_sig, sig));
     CHECK_ILLEGAL(CTX, secp256k1_schnorr_adaptor_extract_sec(CTX, extracted_sec_adaptor, NULL, sig));
-    /* invokes the ARG_CHECK on pre_sig[0] */
-    CHECK_ILLEGAL(CTX, secp256k1_schnorr_adaptor_extract_sec(CTX, extracted_sec_adaptor, invalid_pre_sig, sig));
+    /* invalid pre_sig[0] byte */
+    CHECK(secp256k1_schnorr_adaptor_extract_sec(CTX, extracted_sec_adaptor, invalid_pre_sig, sig) == 0);
     CHECK_ILLEGAL(CTX, secp256k1_schnorr_adaptor_extract_sec(CTX, extracted_sec_adaptor, pre_sig, NULL));
 }