fixup! address robot-dreams' and real-or-random's comments

examples/musig.c

@@ -28,7 +28,7 @@ struct signer {
 
  /* Number of public keys involved in creating the aggregate signature */
 #define N_SIGNERS 3
- /* Create a key pair and store it in seckey and pubkey */
+/* Create a key pair, store it in signer_secrets->keypair and signer->pubkey */
 int create_keypair(const secp256k1_context* ctx, struct signer_secrets *signer_secrets, struct signer *signer) {
     unsigned char seckey[32];
     FILE *frand = fopen("/dev/urandom", "r");
@@ -89,7 +89,8 @@ int sign(const secp256k1_context* ctx, struct signer_secrets *signer_secrets, st
         pubkeys[i] = &signer[i].pubkey;
         pubnonces[i] = &signer[i].pubnonce;
     }
-    /* Communication round 1: Exchange nonces */
+    /* Communication round 1: A production system would exchange public nonces
+     * here before moving on. */
     for (i = 0; i < N_SIGNERS; i++) {
         secp256k1_musig_aggnonce agg_pubnonce;
 
@@ -112,7 +113,8 @@ int sign(const secp256k1_context* ctx, struct signer_secrets *signer_secrets, st
         }
         partial_sigs[i] = &signer[i].partial_sig;
     }
-    /* Communication round 2: Exchange partial signatures */
+    /* Communication round 2: A production system would exchange
+     * partial signatures here before moving on. */
     for (i = 0; i < N_SIGNERS; i++) {
         /* To check whether signing was successful, it suffices to either verify
          * the aggregate signature with the aggregate public key using

include/secp256k1_musig.h

@@ -24,7 +24,7 @@ extern "C" {
  *
  * You may know that the MuSig2 scheme uses two "nonces" instead of one. This
  * is not wrong, but only a technical detail we don't want to bother the user
- * with. Therefore, the API only uses to the singular term "nonce".
+ * with. Therefore, the API only uses the singular term "nonce".
  *
  * Since the first version of MuSig is essentially replaced by MuSig2, when
  * writing MuSig or musig here we mean MuSig2.
@@ -121,7 +121,7 @@ SECP256K1_API int secp256k1_musig_pubnonce_parse(
  *
  *  Returns: 1 when the nonce could be serialized, 0 otherwise
  *  Args:    ctx: a secp256k1 context object
- *  Out:   out32: pointer to a 66-byte array to store the serialized nonce
+ *  Out:   out66: pointer to a 66-byte array to store the serialized nonce
  *  In:    nonce: pointer to the nonce
  */
 SECP256K1_API int secp256k1_musig_pubnonce_serialize(
@@ -147,7 +147,7 @@ SECP256K1_API int secp256k1_musig_aggnonce_parse(
  *
  *  Returns: 1 when the nonce could be serialized, 0 otherwise
  *  Args:    ctx: a secp256k1 context object
- *  Out:   out32: pointer to a 66-byte array to store the serialized nonce
+ *  Out:   out66: pointer to a 66-byte array to store the serialized nonce
  *  In:    nonce: pointer to the nonce
  */
 SECP256K1_API int secp256k1_musig_aggnonce_serialize(
@@ -260,7 +260,7 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_musig_pubkey_tweak_add(
     secp256k1_pubkey *output_pubkey,
     secp256k1_musig_keyagg_cache *keyagg_cache,
     const unsigned char *tweak32
-) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4);
+) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4);
 
 /** Starts a signing session by generating a nonce
  *
@@ -299,7 +299,8 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_musig_pubkey_tweak_add(
  *              msg32: the 32-byte message that will later be signed, if already known
  *                     (can be NULL)
  *       keyagg_cache: pointer to the keyagg_cache that was used to create the aggregate
- *                     (and tweaked) public key if already known (can be NULL)
+ *                     (and potentially tweaked) public key if already known
+ *                     (can be NULL)
  *      extra_input32: an optional 32-byte array that is input to the nonce
  *                     derivation function (can be NULL)
  */
@@ -337,7 +338,7 @@ SECP256K1_API int secp256k1_musig_nonce_agg(
     size_t n_pubnonces
 ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);
 
-/** Takes the public nonces of all signers and computes a session cache that is
+/** Takes the public nonces of all signers and computes a session that is
  *  required for signing and verification of partial signatures.
  *
  *  If the adaptor argument is non-NULL, then the output of
@@ -353,7 +354,7 @@ SECP256K1_API int secp256k1_musig_nonce_agg(
  *                      output of musig_nonce_agg
  *              msg32:  the 32-byte message to sign
  *       keyagg_cache:  pointer to the keyagg_cache that was used to create the
- *                      aggregate (and tweaked) pubkey
+ *                      aggregate (and potentially tweaked) pubkey
  *            adaptor:  optional pointer to an adaptor point encoded as a public
  *                      key if this signing session is part of an adaptor
  *                      signature protocol (can be NULL)

src/modules/musig/adaptor_impl.h

@@ -44,6 +44,16 @@ int secp256k1_musig_adapt(const secp256k1_context* ctx, unsigned char *sig64, co
     secp256k1_scalar_set_b32(&t, sec_adaptor32, &overflow);
     ret &= !overflow;
 
+    /* Determine if the secret adaptor should be negated.
+     *
+     * The musig_session stores the X-coordinate and the parity of the "final nonce"
+     * (r + t)*G, where r*G is the aggregate public nonce and t is the secret adaptor.
+     *
+     * Since a BIP340 signature requires an x-only public nonce, in the case where
+     * (r + t)*G has odd Y-coordinate (i.e. nonce_parity == 1), the x-only public nonce
+     * corresponding to the signature is actually (-r - t)*G. Thus adapting a
+     * presignature requires negating t in this case.
+     */
     if (nonce_parity) {
         secp256k1_scalar_negate(&t, &t);
     }

src/modules/musig/keyagg_impl.h

@@ -204,11 +204,11 @@ int secp256k1_musig_pubkey_agg(const secp256k1_context* ctx, secp256k1_scratch_s
     /* No point on the curve has an X coordinate equal to 0 */
     secp256k1_fe_set_int(&ecmult_data.second_pk_x, 0);
     for (i = 1; i < n_pubkeys; i++) {
-        secp256k1_ge pt;
-        if (!secp256k1_xonly_pubkey_load(ctx, &pt, pubkeys[i])) {
-            return 0;
-        }
         if (secp256k1_memcmp_var(pubkeys[0], pubkeys[i], sizeof(*pubkeys[0])) != 0) {
+            secp256k1_ge pt;
+            if (!secp256k1_xonly_pubkey_load(ctx, &pt, pubkeys[i])) {
+                return 0;
+            }
             ecmult_data.second_pk_x = pt.x;
             break;
         }

src/modules/musig/musig.md

@@ -49,7 +49,7 @@ Similarly, the API supports an alternative protocol flow where generating the ag
 
 # Verification
 
-A participant who wants to verify the partial signatures, but does not sign itself may do so using the above instructions except that the verifier does _not_ generate a nonce with `secp256k1_musig_nonce_gen`.
+A participant who wants to verify the partial signatures, but does not sign itself may do so using the above instructions except that the verifier skips steps 1, 4 and 7.
 
 # Atomic Swaps
 

src/modules/musig/session_impl.h

@@ -87,7 +87,7 @@ static const unsigned char secp256k1_musig_session_cache_magic[4] = { 0x9d, 0xed
 /* A session consists of
  * - 4 byte session cache magic
  * - 1 byte the parity of the final nonce
- * - 32 byte final nonce
+ * - 32 byte serialized x-only final nonce
  * - 32 byte nonce coefficient b
  * - 32 byte signature challenge hash e
  * - 32 byte scalar s that is added to the partial signatures of the signers
@@ -386,7 +386,7 @@ int secp256k1_musig_nonce_agg(const secp256k1_context* ctx, secp256k1_musig_aggn
     return 1;
 }
 
-/* hash(aggnonce[0], aggnonce[1], agg_pk, msg) */
+/* tagged_hash(aggnonce[0], aggnonce[1], agg_pk, msg) */
 static int secp256k1_musig_compute_noncehash(unsigned char *noncehash, secp256k1_ge *aggnonce, const unsigned char *agg_pk32, const unsigned char *msg) {
     unsigned char buf[33];
     secp256k1_sha256 sha;
@@ -542,7 +542,7 @@ int secp256k1_musig_partial_sign(const secp256k1_context* ctx, secp256k1_musig_p
      *   P_agg := mu[0]*|P[0]| + ... + mu[n-1]*|P[n-1]|
      * - P_tweak[i] is the tweaked public key after the i-th tweaking operation
      *   P_tweak[0] := P_agg
-     *   P_tweak[i] := |P_tweak[i-1]| + t[i]*G for i = 1, ..., m-1
+     *   P_tweak[i] := |P_tweak[i-1]| + t[i]*G for i = 1, ..., m
      *
      * Note that our goal is to produce a partial signature corresponding to
      * the final public key after m tweaking operations P_final = |P_tweak[m]|.
@@ -644,7 +644,7 @@ int secp256k1_musig_partial_sig_verify(const secp256k1_context* ctx, const secp2
     }
 
     /* Compute "effective" nonce rj = aggnonce[0] + b*aggnonce[1] */
-    /* TODO: use multiexp to compute -s*G + e*pubkey + aggnonce[0] + b*aggnonce[1] */
+    /* TODO: use multiexp to compute -s*G + e*mu*pubkey + aggnonce[0] + b*aggnonce[1] */
     if (!secp256k1_musig_pubnonce_load(ctx, nonce_pt, pubnonce)) {
         return 0;
     }
@@ -670,6 +670,43 @@ int secp256k1_musig_partial_sig_verify(const secp256k1_context* ctx, const secp2
      * negated exactly when the aggregate key parity is odd. If the aggregate
      * key is tweaked, then negation happens when the aggregate key has an odd Y
      * coordinate XOR the internal key has an odd Y coordinate.*/
+
+    /* When producing a partial signature, signer i uses a possibly
+     * negated secret key:
+     *
+     *   sk[i] = (d_tweak*d_agg*d[i])*x[i]
+     *
+     * to ensure that the aggregate signature will correspond to
+     * an aggregate public key with even Y coordinate (see the
+     * notation and explanation in musig_partial_sign).
+     *
+     * We use the following additional notation:
+     * - e is the (Schnorr signature) challenge
+     * - r[i] is the i-th signer's secret nonce
+     * - R[i] = r[i]*G is the i-th signer's public nonce
+     * - R is the aggregated public nonce
+     * - d_nonce is chosen so that |R| = d_nonce*R
+     *
+     * The i-th partial signature is:
+     *
+     *   s[i] = d_nonce*r[i] + mu[i]*e*sk[i]
+     *
+     * In order to verify this partial signature, we need to check:
+     *
+     *   s[i]*G = d_nonce*R[i] + mu[i]*e*sk[i]*G
+     *
+     * The verifier doesn't have access to sk[i]*G, but can construct
+     * it using the xonly public key |P[i]| as follows:
+     *
+     *   sk[i]*G = d_tweak*d_agg*d[i]*x[i]*G
+     *           = d_tweak*d_agg*d[i]*P[i]
+     *           = d_tweak*d_agg*|P[i]|
+     *
+     * The if condition is below is true whenever d_tweak*d_agg is
+     * negative (again, see the explanation in musig_partial_sign). In
+     * this case, the verifier negates e which will have the same end
+     * result as negating |P[i]|, since they are multiplied later anyway.
+     */
     if (secp256k1_fe_is_odd(&cache_i.pk.y)
             != cache_i.internal_key_parity) {
         secp256k1_scalar_negate(&e, &e);
@@ -678,7 +715,7 @@ int secp256k1_musig_partial_sig_verify(const secp256k1_context* ctx, const secp2
     if (!secp256k1_musig_partial_sig_load(ctx, &s, partial_sig)) {
         return 0;
     }
-    /* Compute -s*G + e*pkj + rj */
+    /* Compute -s*G + e*pkj + rj (e already includes the keyagg coefficient mu) */
     secp256k1_scalar_negate(&s, &s);
     secp256k1_gej_set_ge(&pkj, &pkp);
     secp256k1_ecmult(&tmp, &pkj, &e, &s);