Add module “musig” that implements MuSig2 multi-signatures (BIP 327) #1479

pull jonasnick wants to merge 5 commits into bitcoin-core:master from jonasnick:musig2-module changing 29 files +4493 −28
  1. jonasnick commented at 8:40 pm on January 6, 2024: contributor

    EDIT: based on #1518. Closes #1452. Most of the code is a copy from libsecp256k1-zkp. The API added in this PR is identical with the exception of two modifications:

    1. I removed the unused scratch_space argument from secp256k1_musig_pubkey_agg. This argument was intended to allow using ecmult_multi algorithms for key aggregation in the future. But at this point it’s unclear whether the scratch_space object will remain in its current form (see #1302).
    2. Support for adaptor signatures was removed and therefore the adaptor argument of musig_nonce_process was also removed.

    In contrast to the module in libsecp256k1-zkp, the module is non-experimental. I slightly cleaned up parts of the module, adjusted the code to the new definition of the VERIFY_CHECK macro and applied some simplifications that were possible because the module is now in the upstream repo (ge_from_bytes, ge_to_bytes). You can follow the changes I made to the libsecp256k1-zkp module at https://github.com/jonasnick/secp256k1-zkp/commits/musig2-upstream/.

  2. jonasnick force-pushed on Jan 6, 2024
  3. in include/secp256k1_extrakeys.h:257 in bf1ebb890f outdated
    252+ */
    253+SECP256K1_API int secp256k1_pubkey_cmp(
    254+    const secp256k1_context *ctx,
    255+    const secp256k1_pubkey *pk1,
    256+    const secp256k1_pubkey *pk2
    257+) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);
    


    real-or-random commented at 11:46 am on January 7, 2024:

    Remind me what the difference was to the function secp256k1_ec_pubkey_cmp. Is there no difference and we simply overlooked this? I suspect what happened is this:

    • When we had x-only keys as input to key agg, we had to add a comparison function for x-only keys (to extrakeys module)
    • Then we switched to compressed keys as input, and we changed the comparison function to also take compressed keys, not noticing that there’s one already in secp256k1.h

    jonasnick commented at 1:53 pm on January 7, 2024:
    There is no difference. I removed the commit.
  4. in src/secp256k1.c:247 in 04aa4e7699 outdated
    246@@ -247,8 +247,15 @@ static int secp256k1_pubkey_load(const secp256k1_context* ctx, secp256k1_ge* ge,
    247     } else {
    


    real-or-random commented at 11:51 am on January 7, 2024:
    I think we should simply get rid of this branch entirely, see #1352. I can work on a PR next week.

    real-or-random commented at 3:26 pm on January 8, 2024:
    See #1480

    jonasnick commented at 3:50 pm on January 8, 2024:
    Yes thanks! I considered doing this but didn’t want to make this PR dependent on a separate issue.

    jonasnick commented at 1:59 pm on January 11, 2024:
    Got rid of this by rebasing.
  5. jonasnick force-pushed on Jan 7, 2024
  6. jonasnick force-pushed on Jan 7, 2024
  7. jonasnick force-pushed on Jan 7, 2024
  8. jonasnick force-pushed on Jan 8, 2024
  9. jonasnick force-pushed on Jan 8, 2024
  10. jonasnick force-pushed on Jan 11, 2024
  11. jonasnick commented at 2:01 pm on January 11, 2024: contributor
    Rebased on top of master to get #1480 which allowed dropping a commit. Old state is preserved at https://github.com/jonasnick/secp256k1/tree/musig2-module-backup.
  12. real-or-random added the label feature on Jan 11, 2024
  13. in include/secp256k1_extrakeys.h:257 in e1ba262f16 outdated
    252+SECP256K1_API int secp256k1_pubkey_sort(
    253+    const secp256k1_context *ctx,
    254+    const secp256k1_pubkey **pubkeys,
    255+    size_t n_pubkeys
    256+) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2);
    257+
    


    real-or-random commented at 4:50 pm on January 18, 2024:
    I think this function should be in the main module because it works on secp256k1_pubkey objects, and the comparison function is also there. (Extrakeys would make sense for x-only, I guess.) I don’t think code size is an issue, the heap sort implementation should be tiny.

    jonasnick commented at 7:55 pm on March 8, 2024:
    That’s reasonable. Done.
  14. in src/modules/musig/keyagg.h:34 in fce0857aa0 outdated
    29+
    30+/* musig_ge_to_bytes_ext and musig_ge_from_bytes_ext are identical to ge_save and ge_load
    31+ * except that they allow saving and loading the point at infinity */
    32+static void secp256k1_musig_ge_to_bytes_ext(unsigned char *data, secp256k1_ge *ge);
    33+
    34+static void secp256k1_musig_ge_from_bytes_ext(secp256k1_ge *ge, const unsigned char *data);
    


    real-or-random commented at 4:55 pm on January 18, 2024:

    I think these should be in group.h, even though they’ll be used only by the musig module. Infinity can be helpful in other contexts, and conceptually it’s a group function.

    Anyway, the comment needs to be updated ge_save and ge_load have been renamed.


    jonasnick commented at 7:55 pm on March 8, 2024:
    Agree, done.
  15. real-or-random commented at 4:55 pm on January 18, 2024: contributor
    needs rebase :)
  16. t-bast commented at 2:19 pm on January 23, 2024: none
    FWIW, we have JVM bindings on top of this branch in https://github.com/ACINQ/secp256k1-kmp/pull/93 and an implementation of swap-in-potentiam (musig2 key-path with alternative delayed script path) in https://github.com/ACINQ/bitcoin-kmp/pull/107 and everything is working fine, and the API is easy enough to use!
  17. jonasnick force-pushed on Jan 23, 2024
  18. jonasnick commented at 7:23 pm on January 23, 2024: contributor

    Rebased.

    Thanks @t-bast, that’s good to hear.

  19. siv2r commented at 5:21 am on February 1, 2024: contributor

    Attaching a visualization for the API flow.

    musig2-api-flowchart

    Edit: The above visualization is incorrect. I will update it with the correct one soon.

  20. in examples/musig.c:212 in dd4932b67b outdated
    181+            return 1;
    182+        }
    183+        pubkeys_ptr[i] = &signers[i].pubkey;
    184+    }
    185+    printf("ok\n");
    186+    printf("Combining public keys...");
    


    siv2r commented at 10:23 am on February 5, 2024:

    Do we recommend that users sort their pubkeys before aggregating them? The musig_pubkey_agg API documentation simply says the user “can” do it.

    If we recommend the sorting step, including it in the example file would be helpful.


    real-or-random commented at 10:52 am on February 5, 2024:

    I think there’s no catch-all recommendation. BIP327 says “The aggregate public key produced by KeyAgg (regardless of the type) depends on the order of the individual public keys. If the application does not have a canonical order of the signers, the individual public keys can be sorted with the KeySort algorithm to ensure that the aggregate public key is independent of the order of signers.”

    In other words: If in your application, the collection of pubkeys (or signers represented by them) is conceptually an (ordered) list, then don’t bother with sorting. If in your application, the collection of pubkeys is conceptually an (unordered) set, i.e., the application doesn’t want to care about the order of pubkeys, then sort to make sure the set has a canonical serialization.

    Perhaps we can explain this somewhere in more detail, either in the API docs or in the example.


    josibake commented at 8:14 am on July 25, 2024:

    nit: I think it would be nice to including sorting in the example for the following reasons:

    • It provides a practical example on how to use the sorting API
    • It’s a good hook for adding a comment explaining what @real-or-random just explained above. Feels a bit nicer to have that comment in the example, vs the API docs

    jonasnick commented at 9:10 am on July 26, 2024:
    Added sorting and a comment.
  21. in src/modules/musig/session_impl.h:1 in dd4932b67b


    siv2r commented at 9:13 am on February 6, 2024:
    Is there any specific reason for avoiding sha256 mid-state optimization in the musig_compute_noncehash and nonce_function_musig functions?

    jonasnick commented at 7:54 pm on March 8, 2024:
    Because apparently I had been to lazy so far :D. I added this optimization.
  22. in include/secp256k1_musig.h:325 in dd4932b67b outdated
    320+ *  1. Each call to this function must have a UNIQUE session_id32 that must NOT BE
    321+ *     REUSED in subsequent calls to this function.
    322+ *     If you do not provide a seckey, session_id32 _must_ be UNIFORMLY RANDOM
    323+ *     AND KEPT SECRET (even from other signers). If you do provide a seckey,
    324+ *     session_id32 can instead be a counter (that must never repeat!). However,
    325+ *     it is recommended to always choose session_id32 uniformly at random.
    


    real-or-random commented at 12:18 pm on March 7, 2024:

    We should probably rename this. Despite this big fat warning, session_id itself sounds like a public value.

    Brain dump:

    • session_nonce: too easy to confuse with the sec nonce
    • session_rand: but what if it’s counter
    • session_seed: okayish

    But now that I think about this again, I believe that the confusion stems from the fact that we have these two modes: It’s either random and secret, or a counter, but that’s only okay if you provide a seckey. A single name that fits both scenarios is necessarily imprecise.

    I think a better approach is to provide two different functions, e.g., secp256k1_musig_nonce_gen and secp256k1_musig_nonce_gen_with_counter (like in the BIP where we have CounterNonceGen as a separate algorithm). Then we can have clear argument names, even very verbose ones like session_secret_rand and nonrepeating_counter. Moreover, we can enforce the presence of the seckey in the counter function.


    jonasnick commented at 2:25 pm on March 10, 2024:
    Agreed. I split the two nonce_gen functions.
  23. jonasnick force-pushed on Mar 10, 2024
  24. in include/secp256k1_musig.h:324 in 461970682f outdated
    324- *     If you do not provide a seckey, session_id32 _must_ be UNIFORMLY RANDOM
    325- *     AND KEPT SECRET (even from other signers). If you do provide a seckey,
    326- *     session_id32 can instead be a counter (that must never repeat!). However,
    327- *     it is recommended to always choose session_id32 uniformly at random.
    328+ *     If you do not provide a seckey, session_secrand32 _must_ be UNIFORMLY RANDOM
    329+ *     AND KEPT SECRET (even from other signers).
    


    real-or-random commented at 8:13 am on March 14, 2024:
    We should then drop the condition “If you do not provide a seckey,”

    real-or-random commented at 8:20 am on March 14, 2024:

    We could also add a reference to the new function, maybe below the numbered list.

    something like “If you don’t have access to good randomness, but you have access to a non-repeating counter, then see …”


    jonasnick commented at 6:55 pm on April 16, 2024:
    Added both suggestions.
  25. in include/secp256k1_musig.h:398 in 461970682f outdated
    382+ *  1. The nonrepeating_cnt argument must be a counter value that never
    383+ *     repeats, i.e., you must never call `secp256k1_musig_nonce_gen_counter`
    384+ *     twice with the same seckey and nonrepeating_cnt value.
    385+ *  2. If you already know the seckey, message or aggregate public key
    386+ *     cache, they can be optionally provided to derive the nonce and increase
    387+ *     misuse-resistance. The extra_input32 argument can be used to provide
    


    real-or-random commented at 8:18 am on March 14, 2024:

    seckey is mandatory for this function anyway.

    I think usually I would suggest moving this big explainer at the top of the module, so that it covers both functions and does not need to be repeated. But in this case of a big fat warning, it’s probably better to keep it in the docstring, where it’s harder to miss…


    jonasnick commented at 6:55 pm on April 16, 2024:
    dropped mention of seckey
  26. in include/secp256k1_musig.h:383 in 461970682f outdated
    379+ *  MuSig differs from regular Schnorr signing in that implementers _must_ take
    380+ *  special care to not reuse a nonce. This can be ensured by following these rules:
    381+ *
    382+ *  1. The nonrepeating_cnt argument must be a counter value that never
    383+ *     repeats, i.e., you must never call `secp256k1_musig_nonce_gen_counter`
    384+ *     twice with the same seckey and nonrepeating_cnt value.
    


    real-or-random commented at 8:25 am on March 14, 2024:
    We could expand a bit to mention that this includes cases where the same seckey is used on multiple devices. (Any other precautions to mention?)

    jonasnick commented at 6:56 pm on April 16, 2024:
    done
  27. in src/modules/extrakeys/main_impl.h:309 in 26dde295d0 outdated
    304+
    305+    /* Suppress wrong warning (fixed in MSVC 19.33) */
    306+    #if defined(_MSC_VER) && (_MSC_VER < 1933)
    307+    #pragma warning(push)
    308+    #pragma warning(disable: 4090)
    309+    #endif
    


    josibake commented at 2:41 pm on April 8, 2024:

    in https://github.com/bitcoin-core/secp256k1/pull/1479/commits/26dde295d0ad12a011fc4062ad642852f7ff68fa (“extrakeys: add secp256k1_pubkey_sort”):

    Does it make sense to move this block into the secp256k1_sort function? I ended up copying these lines while writing a secp256k1_silentpayments_recipient_sort function, which made me realize anyone else would also need to copy these lines when writing a sort function for heapsort.


    jonasnick commented at 6:52 pm on April 16, 2024:
    You mean into secp256k1_hsort? I’d guess the wrong warning is emitted when secp256k1_hsort is called and therefore it would be to late when the warning was disabled in secp256k1_hsort.
  28. josibake commented at 2:43 pm on April 8, 2024: member
    I’m also using the heapsort commits in #1471. What do you think about splitting out the sort commits into their own PR? Also fine with cherry-picking for now, but figured I’d mention it since it might simplify both of our PRs.
  29. jonasnick force-pushed on Apr 16, 2024
  30. jonasnick commented at 7:32 pm on April 16, 2024: contributor

    @josibake

    What do you think about splitting out the sort commits into their own PR?

    That’s a good idea. In particular, if more fixups are needed for the sort commits. See #1518.

  31. sipa referenced this in commit bb528cfb08 on May 6, 2024
  32. achow101 commented at 10:17 pm on June 4, 2024: member
    Rebase perhaps? It’d be nice to get this in soon.
  33. jonasnick force-pushed on Jun 5, 2024
  34. in src/modules/musig/keyagg_impl.h:31 in c9362664e6 outdated
    26+ * - 64 byte "second" public key (set to the point at infinity if not present)
    27+ * - 32 byte hash of all public keys
    28+ * - 1 byte the parity of the internal key (if tweaked, otherwise 0)
    29+ * - 32 byte tweak
    30+ */
    31+/* Requires that cache_i->pk is not infinity and cache_i->second_pk_x to be normalized. */
    


    stratospher commented at 4:01 am on June 21, 2024:
    c936266: why does second_pk_x need to be normalized?

    jonasnick commented at 1:50 pm on July 10, 2024:
    it doesn’t anymore, fixed
  35. in src/modules/musig/keyagg_impl.h:148 in c9362664e6 outdated
    143+        secp256k1_sha256_finalize(&sha, buf);
    144+        secp256k1_scalar_set_b32(r, buf, NULL);
    145+    }
    146+}
    147+
    148+/* Assumes both field elements x and second_pk_x are normalized. */
    


    stratospher commented at 6:21 am on June 21, 2024:
    c936266: micro nit: s/x/pk_x

    jonasnick commented at 1:50 pm on July 10, 2024:
    This was a bogus comment anyway. Fixed.
  36. in src/modules/musig/keyagg_impl.h:217 in c9362664e6 outdated
    214+        return 0;
    215+    }
    216+    secp256k1_ge_set_gej(&pkp, &pkj);
    217+    secp256k1_fe_normalize_var(&pkp.y);
    218+    /* The resulting public key is infinity with negligible probability */
    219+    VERIFY_CHECK(!secp256k1_ge_is_infinity(&pkp));
    


    stratospher commented at 8:59 am on June 21, 2024:
    c936266: slightly confused here and in L277. is a VERIFY_CHECK instead of return 0 sufficient for these failures?

    jonasnick commented at 1:50 pm on July 10, 2024:
    I believe so. These conditions are effectively unreachable. If they do become reachable, then we have a serious bug that the tests hopefully notice. Moreover, we cannot exercise the VERIFY_CHECKed conditions with a test, so if we would add a branch we couldn’t cover it.
  37. in src/modules/musig/session_impl.h:481 in c9362664e6 outdated
    476+int secp256k1_musig_nonce_gen_counter(const secp256k1_context* ctx, secp256k1_musig_secnonce *secnonce, secp256k1_musig_pubnonce *pubnonce, uint64_t nonrepeating_cnt, const unsigned char *seckey, const secp256k1_pubkey *pubkey, const unsigned char *msg32, const secp256k1_musig_keyagg_cache *keyagg_cache, const unsigned char *extra_input32) {
    477+    unsigned char buf[32] = { 0 };
    478+    int i;
    479+
    480+    VERIFY_CHECK(ctx != NULL);
    481+    ARG_CHECK((seckey != NULL));
    


    stratospher commented at 8:40 am on June 24, 2024:
    c936266: nit: remove double (()).

    jonasnick commented at 1:50 pm on July 10, 2024:
    done
  38. in CHANGELOG.md:15 in c9362664e6 outdated
     6@@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
     7 
     8 ## [Unreleased]
     9 
    10+#### Added
    11+ - New module `musig` implements the MuSig2 multisignature scheme according to the [BIP 327 specification](https://github.com/bitcoin/bips/blob/master/bip-0327.mediawiki). See:
    12+   - Header file `include/secp256k1_musig.h` which defines the new API.
    13+   - Document `doc/musig.md` for further notes on API usage.
    14+   - Usage example `examples/musig.c`.
    15+ - Added `secp256k1_ec_pubkey_sort` which sorts an array of public keys (see `include/secp256k1.h`).
    


    theStack commented at 12:17 pm on June 24, 2024:

    This line can be removed, as the function was already added in the last release 0.5.0


    jonasnick commented at 1:48 pm on July 10, 2024:
    fixed
  39. in configure.ac:450 in c9362664e6 outdated
    443@@ -432,6 +444,10 @@ if test x"$enable_module_ecdh" = x"yes"; then
    444   SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DENABLE_MODULE_ECDH=1"
    445 fi
    446 
    447+if test x"$enable_module_ellswift" = x"yes"; then
    448+  SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DENABLE_MODULE_ELLSWIFT=1"
    449+fi
    450+
    


    theStack commented at 12:19 pm on June 24, 2024:
    This code block already exists about 30 lines above (was it intended to be moved from top to bottom?).

    real-or-random commented at 5:47 pm on June 24, 2024:
    I think the duplication was just introduced as a mistake when rebasing after #1482

    jonasnick commented at 1:48 pm on July 10, 2024:
    Yes, fixed
  40. in examples/musig.c:37 in c9362664e6 outdated
    32+    secp256k1_musig_pubnonce pubnonce;
    33+    secp256k1_musig_partial_sig partial_sig;
    34+};
    35+
    36+ /* Number of public keys involved in creating the aggregate signature */
    37+#define N_SIGNERS 3
    


    theStack commented at 1:14 pm on June 24, 2024:
    Verified that the example also succeeds with larger values of N_SIGNERS, like e.g. 15000. At some point if the limit is increased further (e.g. 20000), segmentation faults occur on my machine, I guess this happens due to some stack limits being exceeded.

    josibake commented at 9:38 am on July 25, 2024:

    I replaced the stack allocations with heap allocations here: https://github.com/josibake/secp256k1/commit/35ac3b194aefc064e8cee94e93498be224cc0d69

    and ran the example with 100,000 signers. Everything up until signing was very fast, whereas signing took 45 mins (ran on a relatively powerful machine). Worth mentioning the time is a somewhat useless number here and the goal was more to stress test. The sign function in the example does both rounds of communication in the same function and the partial signing is done sequentially, whereas in reality the partial signing would be done in parallel


    jonasnick commented at 7:50 pm on July 25, 2024:
    I was a quite confused at first that signing took 45 minutes on your machine, until I realized that all signers in the example effectively do the coordinator’s job of aggregating nonces. I changed that to only aggregate nonces once. Entire example with 100,000 signers takes 12 seconds on my laptop now.

    josibake commented at 12:07 pm on July 26, 2024:
    Ah, nice!
  41. in examples/musig.c:44 in c9362664e6 outdated
    39+static int create_keypair(const secp256k1_context* ctx, struct signer_secrets *signer_secrets, struct signer *signer) {
    40+    unsigned char seckey[32];
    41+    while (1) {
    42+        if (!fill_random(seckey, sizeof(seckey))) {
    43+            printf("Failed to generate randomness\n");
    44+            return 1;
    


    theStack commented at 1:17 pm on June 24, 2024:
    0            return 0;
    

    jonasnick commented at 1:49 pm on July 10, 2024:
    oops, fixed
  42. in examples/musig.c:69 in c9362664e6 outdated
    64+    /* Plain tweaking which, for example, allows deriving multiple child
    65+     * public keys from a single aggregate key using BIP32 */
    66+    if (!secp256k1_musig_pubkey_ec_tweak_add(ctx, NULL, cache, plain_tweak)) {
    67+        return 0;
    68+    }
    69+    /* Note that we did not provided an output_pk argument, because the
    


    theStack commented at 1:23 pm on June 24, 2024:

    nit:

    0    /* Note that we did not provide an output_pk argument, because the
    

    jonasnick commented at 1:49 pm on July 10, 2024:
    done
  43. in examples/musig.c:230 in c9362664e6 outdated
    195+    if (!tweak(ctx, &agg_pk, &cache)) {
    196+        printf("FAILED\n");
    197+        return 1;
    198+    }
    199+    printf("ok\n");
    200+    printf("Signing message.........");
    


    theStack commented at 1:28 pm on June 24, 2024:

    nit: if an operation takes longer, it would be nice to see this message already before the function call following returns

    0    printf("Signing message........."); fflush(stdout);
    

    (noticed this for larger values of N_SIGNERS, for the value of 3 chosen here it shouldn’t matter though even on slow hardware)


    jonasnick commented at 1:49 pm on July 10, 2024:
    Makes sense. Added fflush after every printf without a newline.
  44. theStack commented at 1:47 pm on June 24, 2024: contributor
    Looked mostly at the API header and the example so far, left a few nits. I noticed that the generated test vector headers differs in a few lines when generated with the data from the bips repo (master branch), probably that’s due to the recent bump from 1.0.0 to 1.0.1 (https://github.com/bitcoin/bips/pull/1591).
  45. in src/modules/musig/session_impl.h:808 in c9362664e6 outdated
    791+    for (i = 0; i < n_sigs; i++) {
    792+        secp256k1_scalar term;
    793+        if (!secp256k1_musig_partial_sig_load(ctx, &term, partial_sigs[i])) {
    794+            return 0;
    795+        }
    796+        secp256k1_scalar_add(&session_i.s_part, &session_i.s_part, &term);
    


    stratospher commented at 7:06 am on June 25, 2024:
    c936266: don’t we need to do the e⋅g⋅tacc part of this equation s = s1 + ... + su + e⋅g⋅tacc mod n?

    jonasnick commented at 1:50 pm on July 10, 2024:
    We’ve already done the g*tacc computation in tweak_add and e*g*tacc in nonce_process.
  46. in examples/musig.c:12 in c9362664e6 outdated
     7+ * EXAMPLES_COPYING or https://creativecommons.org/publicdomain/zero/1.0 *
     8+ *************************************************************************/
     9+
    10+/** This file demonstrates how to use the MuSig module to create a
    11+ *  3-of-3 multisignature. Additionally, see the documentation in
    12+ *  include/secp256k1_musig.h and src/modules/musig/musig.md.
    


    theStack commented at 10:57 am on June 25, 2024:
    0 *  include/secp256k1_musig.h and doc/musig.md.
    

    jonasnick commented at 1:51 pm on July 10, 2024:
    done
  47. in src/modules/musig/session_impl.h:302 in c9362664e6 outdated
    273+    VERIFY_CHECK(ctx != NULL);
    274+    ARG_CHECK(out32 != NULL);
    275+    ARG_CHECK(sig != NULL);
    276+    memcpy(out32, &sig->data[4], 32);
    277+    return 1;
    278+}
    


    theStack commented at 4:09 pm on June 26, 2024:
    Shouldn’t this function call _musig_partial_sig_load, in order to check the 4 bytes magic and verify that the scalar doesn’t overflow? (similar to the _musig_{pub,agg}nonce_serialize functions which also call _musig_{pub,agg}nonce_load first)

    jonasnick commented at 1:51 pm on July 10, 2024:
    Added a memcmp. I don’t think parsing the scalar is necessary because if properly initialized (which we ensure by checking the magic), the scalar cannot overflow. _musig_{pub,agg}nonce_serialize call load because they are required to do more work.
  48. in include/secp256k1_musig.h:238 in c9362664e6 outdated
    235+ *  The tweaking method is the same as `secp256k1_ec_pubkey_tweak_add`. So after
    236+ *  the following pseudocode buf and buf2 have identical contents (absent
    237+ *  earlier failures).
    238+ *
    239+ *  secp256k1_musig_pubkey_agg(..., keyagg_cache, pubkeys, ...)
    240+ *  secp256k1_musig_pubkey_get(..., agg_pk, keyagg_cache)
    


    theStack commented at 4:43 pm on June 26, 2024:

    nit: IIUC, agg_pk can be directly retrieved in the _musig_pubkey_agg call:

    0 *  secp256k1_musig_pubkey_agg(..., agg_pk, keyagg_cache, pubkeys, ...)
    

    (that’s how it’s also done in the API description for the x-only tweaking function below)


    jonasnick commented at 1:51 pm on July 10, 2024:
    Yep, fixed
  49. in include/secp256k1_musig.h:244 in c9362664e6 outdated
    239+ *  secp256k1_musig_pubkey_agg(..., keyagg_cache, pubkeys, ...)
    240+ *  secp256k1_musig_pubkey_get(..., agg_pk, keyagg_cache)
    241+ *  secp256k1_musig_pubkey_ec_tweak_add(..., output_pk, tweak32, keyagg_cache)
    242+ *  secp256k1_ec_pubkey_serialize(..., buf, output_pk)
    243+ *  secp256k1_ec_pubkey_tweak_add(..., agg_pk, tweak32)
    244+ *  secp256k1_ec_pubkey_serialize(..., buf2, agg_pk)
    


    theStack commented at 4:49 pm on June 26, 2024:

    nit: not sure how strict we want to be here to match the exact API, but with all parameters considered, it would look like e.g.

    0 *  secp256k1_ec_pubkey_serialize(..., buf2, &outlen, agg_pk, ...)
    

    jonasnick commented at 1:51 pm on July 10, 2024:
    fixed
  50. in include/secp256k1_musig.h:282 in c9362664e6 outdated
    277+ *  The tweaking method is the same as `secp256k1_xonly_pubkey_tweak_add`. So in
    278+ *  the following pseudocode xonly_pubkey_tweak_add_check (absent earlier
    279+ *  failures) returns 1.
    280+ *
    281+ *  secp256k1_musig_pubkey_agg(..., agg_pk, keyagg_cache, pubkeys, ...)
    282+ *  secp256k1_musig_pubkey_xonly_tweak_add(..., output_pk, tweak32, keyagg_cache)
    


    theStack commented at 4:52 pm on June 26, 2024:
    0 *  secp256k1_musig_pubkey_xonly_tweak_add(..., output_pk, keyagg_cache, tweak32)
    

    jonasnick commented at 1:51 pm on July 10, 2024:
    fixed
  51. in src/modules/musig/keyagg_impl.h:185 in c9362664e6 outdated
    182+
    183+    VERIFY_CHECK(ctx != NULL);
    184+    if (agg_pk != NULL) {
    185+        memset(agg_pk, 0, sizeof(*agg_pk));
    186+    }
    187+    ARG_CHECK(pubkeys != NULL);
    


    stratospher commented at 7:55 am on June 27, 2024:
    c936266: what is the difference between doing ARG_CHECKs like this here vs SECP256K1_ARG_NONNULL(4) checks in the function declaration for the same function parameter.

    jonasnick commented at 1:53 pm on July 10, 2024:
    The SECP256K1_ARG_NONNULL attribute relies on a GNU extension that can give a warning at compile time. A failing ARG_CHECK will call the illegal_callback at runtime. They complement each other.
  52. in src/modules/musig/tests_impl.h:233 in c9362664e6 outdated
    228+            secp256k1_pubkey tmp_output_pk;
    229+            secp256k1_musig_keyagg_cache tmp_keyagg_cache = keyagg_cache;
    230+            CHECK((*tweak_func[i])(CTX, &tmp_output_pk, &tmp_keyagg_cache, tweak) == 1);
    231+            /* Reset keyagg_cache */
    232+            tmp_keyagg_cache = keyagg_cache;
    233+            CHECK((*tweak_func[i])(CTX, &tmp_output_pk, &tmp_keyagg_cache, tweak) == 1);
    


    stratospher commented at 9:07 am on June 27, 2024:
    c936266: isn’t this a repetition of L230?

    jonasnick commented at 1:53 pm on July 10, 2024:
    Yes, removed
  53. in src/modules/musig/tests_impl.h:297 in c9362664e6 outdated
    292+    CHECK(secp256k1_musig_nonce_gen_counter(CTX, &secnonce[0], &pubnonce[0], nonrepeating_cnt, sk[0], &pk[0], msg, NULL, max64) == 1);
    293+    CHECK_ILLEGAL(CTX, secp256k1_musig_nonce_gen_counter(CTX, &secnonce[0], &pubnonce[0], nonrepeating_cnt, sk[0], &pk[0], msg, &invalid_keyagg_cache, max64));
    294+    CHECK(memcmp_and_randomize(secnonce[0].data, zeros132, sizeof(secnonce[0].data)) == 0);
    295+    CHECK(secp256k1_musig_nonce_gen_counter(CTX, &secnonce[0], &pubnonce[0], nonrepeating_cnt, sk[0], &pk[0], msg, &keyagg_cache, NULL) == 1);
    296+
    297+    /* Every in-argument except session_secrand, sk and pubkey can be NULL */
    


    stratospher commented at 11:42 am on June 27, 2024:
    c936266: s/session_secrand/nonrepeating_cnt

    jonasnick commented at 1:53 pm on July 10, 2024:
    fixed
  54. in include/secp256k1_musig.h:429 in c9362664e6 outdated
    425+    const unsigned char *seckey,
    426+    const secp256k1_pubkey *pubkey,
    427+    const unsigned char *msg32,
    428+    const secp256k1_musig_keyagg_cache *keyagg_cache,
    429+    const unsigned char *extra_input32
    430+) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(5) SECP256K1_ARG_NONNULL(6);
    


    stratospher commented at 11:44 am on June 27, 2024:
    c936266: SECP256K1_ARG_NONNULL(4) too?

    jonasnick commented at 1:54 pm on July 10, 2024:
    nonrepeating_cnt isn’t a pointer, so it cannot be NULL.
  55. in src/modules/musig/session_impl.h:300 in c9362664e6 outdated
    295+/* Write optional inputs into the hash */
    296+static void secp256k1_nonce_function_musig_helper(secp256k1_sha256 *sha, unsigned int prefix_size, const unsigned char *data, unsigned char len) {
    297+    unsigned char zero[7] = { 0 };
    298+    /* The spec requires length prefixes to be between 1 and 8 bytes
    299+     * (inclusive) */
    300+    VERIFY_CHECK(prefix_size <= 8);
    


    theStack commented at 4:51 pm on June 27, 2024:

    nit, could also check the lower bound of prefix_size:

    0    VERIFY_CHECK(prefix_size >= 1 && prefix_size <= 8);
    

    jonasnick commented at 1:51 pm on July 10, 2024:
    Done
  56. in src/modules/musig/session_impl.h:485 in c9362664e6 outdated
    480+    VERIFY_CHECK(ctx != NULL);
    481+    ARG_CHECK((seckey != NULL));
    482+
    483+    for (i = 0; i < 8; ++i) {
    484+        buf[7 - i] = (nonrepeating_cnt >> (i * 8)) & 0xFF;
    485+    }
    


    theStack commented at 4:54 pm on June 27, 2024:

    could use the _write_be64 helper here:

    0    secp256k1_write_be64(buf, nonrepeating_cnt);
    

    jonasnick commented at 1:52 pm on July 10, 2024:
    nice, done
  57. in src/modules/musig/session_impl.h:361 in c9362664e6 outdated
    356+        }
    357+    } else {
    358+        memcpy(rand, session_secrand, sizeof(rand));
    359+    }
    360+
    361+    /* Subtract one from `sizeof` to avoid hashing the implicit null byte */
    


    theStack commented at 5:01 pm on June 27, 2024:
    Is this comment a leftover? I couldn’t figure out to what sizeof it refers to and where a subtraction happens.

    jonasnick commented at 1:52 pm on July 10, 2024:
    Yes, removed the comment
  58. in src/modules/musig/session_impl.h:613 in c9362664e6 outdated
    608+        return 0;
    609+    }
    610+    secp256k1_gej_set_ge(&aggnonce_ptj[0], &aggnonce_pt[0]);
    611+    secp256k1_gej_set_ge(&aggnonce_ptj[1], &aggnonce_pt[1]);
    612+
    613+    if (!secp256k1_musig_nonce_process_internal(&session_i.fin_nonce_parity, fin_nonce, &session_i.noncecoef, aggnonce_ptj, agg_pk32, msg32)) {
    


    theStack commented at 5:16 pm on June 28, 2024:

    I’ve noticed that the aggregated nonce points are converted from affine to jacobi coordinates here, and then again from jacobi to affine inside secp256k1_musig_nonce_process_internal. Could pass as affine coordinates to the internal function instead in order to do only one conversion there. With the following patch, the tests still pass:

     0diff --git a/src/modules/musig/session_impl.h b/src/modules/musig/session_impl.h
     1index b0ea45d..b073ec0 100644
     2--- a/src/modules/musig/session_impl.h
     3+++ b/src/modules/musig/session_impl.h
     4@@ -557,14 +557,14 @@ static int secp256k1_musig_compute_noncehash(unsigned char *noncehash, secp256k1
     5     return 1;
     6 }
     7 
     8-static int secp256k1_musig_nonce_process_internal(int *fin_nonce_parity, unsigned char *fin_nonce, secp256k1_scalar *b, secp256k1_gej *aggnoncej, const unsigned char *agg_pk32, const unsigned char *msg) {
     9+static int secp256k1_musig_nonce_process_internal(int *fin_nonce_parity, unsigned char *fin_nonce, secp256k1_scalar *b, secp256k1_ge *aggnonce, const unsigned char *agg_pk32, const unsigned char *msg) {
    10     unsigned char noncehash[32];
    11     secp256k1_ge fin_nonce_pt;
    12     secp256k1_gej fin_nonce_ptj;
    13-    secp256k1_ge aggnonce[2];
    14+    secp256k1_gej aggnoncej[2];
    15 
    16-    secp256k1_ge_set_gej(&aggnonce[0], &aggnoncej[0]);
    17-    secp256k1_ge_set_gej(&aggnonce[1], &aggnoncej[1]);
    18+    secp256k1_gej_set_ge(&aggnoncej[0], &aggnonce[0]);
    19+    secp256k1_gej_set_ge(&aggnoncej[1], &aggnonce[1]);
    20     if (!secp256k1_musig_compute_noncehash(noncehash, aggnonce, agg_pk32, msg)) {
    21         return 0;
    22     }
    23@@ -588,7 +588,6 @@ static int secp256k1_musig_nonce_process_internal(int *fin_nonce_parity, unsigne
    24 int secp256k1_musig_nonce_process(const secp256k1_context* ctx, secp256k1_musig_session *session, const secp256k1_musig_aggnonce  *aggnonce, const unsigned char *msg32, const secp256k1_musig_keyagg_cache *keyagg_cache) {
    25     secp256k1_keyagg_cache_internal cache_i;
    26     secp256k1_ge aggnonce_pt[2];
    27-    secp256k1_gej aggnonce_ptj[2];
    28     unsigned char fin_nonce[32];
    29     secp256k1_musig_session_internal session_i;
    30     unsigned char agg_pk32[32];
    31@@ -607,10 +606,8 @@ int secp256k1_musig_nonce_process(const secp256k1_context* ctx, secp256k1_musig_
    32     if (!secp256k1_musig_aggnonce_load(ctx, aggnonce_pt, aggnonce)) {
    33         return 0;
    34     }
    35-    secp256k1_gej_set_ge(&aggnonce_ptj[0], &aggnonce_pt[0]);
    36-    secp256k1_gej_set_ge(&aggnonce_ptj[1], &aggnonce_pt[1]);
    37 
    38-    if (!secp256k1_musig_nonce_process_internal(&session_i.fin_nonce_parity, fin_nonce, &session_i.noncecoef, aggnonce_ptj, agg_pk32, msg32)) {
    39+    if (!secp256k1_musig_nonce_process_internal(&session_i.fin_nonce_parity, fin_nonce, &session_i.noncecoef, aggnonce_pt, agg_pk32, msg32)) {
    40         return 0;
    41     }
    

    jonasnick commented at 1:52 pm on July 10, 2024:
    Cool, implemented your suggestion.
  59. in src/modules/musig/session_impl.h:627 in c9362664e6 outdated
    622+        secp256k1_scalar e_tmp;
    623+        secp256k1_scalar_mul(&e_tmp, &session_i.challenge, &cache_i.tweak);
    624+        if (secp256k1_fe_is_odd(&cache_i.pk.y)) {
    625+            secp256k1_scalar_negate(&e_tmp, &e_tmp);
    626+        }
    627+        secp256k1_scalar_add(&session_i.s_part, &session_i.s_part, &e_tmp);
    


    theStack commented at 5:19 pm on June 28, 2024:

    nit: could assign the result directly rather than adding (session_i.s_part is zero at this point):

    0        session_i.s_part = e_tmp;
    

    jonasnick commented at 1:52 pm on July 10, 2024:
    yes, done
  60. in src/modules/musig/session_impl.h:677 in c9362664e6 outdated
    672+              && secp256k1_fe_equal(&pk.y, &keypair_pk.y));
    673+    if (!secp256k1_keyagg_cache_load(ctx, &cache_i, keyagg_cache)) {
    674+        secp256k1_musig_partial_sign_clear(&sk, k);
    675+        return 0;
    676+    }
    677+    secp256k1_fe_normalize_var(&pk.y);
    


    theStack commented at 5:39 pm on June 28, 2024:
    nit: though it doesn’t hurt, normalizing doesn’t seem to be necessary, considering that pk is created via deserialization (_musig_secnonce_load -> _ge_from_bytes -> _ge_from_storage -> _fe_from_storage), where the group element’s coordinates x and y are always normalized initially. Same for pk.x below.

    jonasnick commented at 1:53 pm on July 10, 2024:
    Removed both calls to fe_normalize
  61. in src/modules/musig/session_impl.h:367 in c9362664e6 outdated
    354+        for (i = 0; i < 32; i++) {
    355+            rand[i] ^= seckey32[i];
    356+        }
    357+    } else {
    358+        memcpy(rand, session_secrand, sizeof(rand));
    359+    }
    


    theStack commented at 12:46 pm on June 30, 2024:
    Considering that rand contains secret data, should it be cleared at the end of the function? (same for buf and sha_tmp in the loop below I guess)

    jonasnick commented at 1:53 pm on July 10, 2024:
    Done
  62. in src/modules/musig/tests_impl.h:624 in c9362664e6 outdated
    619+    CHECK(secp256k1_musig_pubkey_agg(CTX, &P_xonly[0], &keyagg_cache, pk_ptr, 2) == 1);
    620+    musig_tweak_test_helper(&P_xonly[0], sk[0], sk[1], &keyagg_cache);
    621+    CHECK(secp256k1_musig_pubkey_get(CTX, &P[0], &keyagg_cache));
    622+
    623+    /* Compute Pi = f(Pj) + tweaki*G where where j = i-1 and try signing for
    624+     * that key. If xonly is set to true, the function f is normalizes the input
    


    stratospher commented at 4:50 am on July 1, 2024:
    c9362664e: micro grammar nit: s/is normalizes/normalizes
  63. in src/modules/musig/vectors.h:80 in c9362664e6 outdated
    75+    unsigned char aggpk[32];
    76+    int has_msg;
    77+    unsigned char msg[32];
    78+    int has_extra_in;
    79+    unsigned char extra_in[32];
    80+    unsigned char expected_secnonce[97];
    


    stratospher commented at 6:16 am on July 1, 2024:
    c9362664e: micro nit: we could remove the last 33 bytes and make it expected_secnonce[64].

    jonasnick commented at 1:54 pm on July 10, 2024:
    Hm the test vector is 97 bytes though. I changed the test to check that last 33 bytes of expected_secnonce as well, to make sure it’s really the public key as expected.
  64. jonasnick force-pushed on Jul 10, 2024
  65. jonasnick force-pushed on Jul 10, 2024
  66. jonasnick commented at 2:09 pm on July 10, 2024: contributor
    Thanks a lot @stratospher @theStack for the review! I think I addressed all comments and I rebased on master.
  67. jonasnick force-pushed on Jul 11, 2024
  68. jonasnick commented at 7:26 am on July 11, 2024: contributor
    rebased again
  69. in src/modules/musig/vectors.h:2 in 0dd333a919 outdated
    0@@ -0,0 +1,346 @@
    1+/**
    2+ * Automatically generated by ./contrib/musig2-vectors.py.
    


    real-or-random commented at 8:11 am on July 11, 2024:

    I think this was lost in some rebase.

    0 * Automatically generated by tools/test_vectors_musig2_generate_.py.
    

    And just remove contrib/musig2-vectors.py, the PR currently adds the (identical) script to both paths.


    jonasnick commented at 8:58 am on July 11, 2024:
    Not a rebase issue, just confusion: I had thought that I had forgotten to add the python script because it’s called differently in -zkp.

    jonasnick commented at 9:25 am on July 11, 2024:
    fixed
  70. in doc/musig.md:7 in 0dd333a919 outdated
    0@@ -0,0 +1,51 @@
    1+Notes on the musig module API
    2+===========================
    3+
    4+The following sections contain additional notes on the API of the musig module (`include/secp256k1_musig.h`).
    5+A usage example can be found in `examples/musig.c`.
    6+
    7+# API misuse
    


    real-or-random commented at 8:15 am on July 11, 2024:

    nit:

    0## API misuse
    

    (also for all other headings below)

    # is the same level as the heading for the entire document, see the rendered file.


    jonasnick commented at 9:25 am on July 11, 2024:
    fixed
  71. in examples/musig.c:2 in 0dd333a919 outdated
    0@@ -0,0 +1,218 @@
    1+/*************************************************************************
    2+ * Written in 2018 by Jonas Nick                                         *
    


    real-or-random commented at 8:23 am on July 11, 2024:

    (Okay, I’m all in favor of not doing updating years and stuff, but this is just too wrong. :D)

    0 * Written by Jonas Nick                                                 *
    

    jonasnick commented at 9:26 am on July 11, 2024:
    Removed name and year from all headers
  72. real-or-random commented at 8:36 am on July 11, 2024: contributor
    Just left a comment in the silent payments PR about checking test vectors, which is relevant here too: #1519 (review)
  73. jonasnick commented at 9:02 am on July 11, 2024: contributor

    Just left a comment in the silent payments PR about checking test vectors, which is relevant here too: #1519 (comment)

    This would only work if we included the json test vectors from the BIP.

  74. jonasnick force-pushed on Jul 11, 2024
  75. real-or-random commented at 3:47 pm on July 11, 2024: contributor

    This would only work if we included the json test vectors from the BIP.

    Okay yeah, I think this has a non-zero benefit because then CI can test the py script and the repo is self-contained, but it’s not clear if it’s worth the hassle, in particular given that the PR is already in a pretty mature state.

  76. josibake commented at 4:14 pm on July 11, 2024: member

    Okay yeah, I think this has a non-zero benefit because then CI can test the py script and the repo is self-contained, but it’s not clear if it’s worth the hassle, in particular given that the PR is already in a pretty mature state.

    Seems like this could be opened as an issue and left for a follow-up PR? I agree its a non-zero benefit, but not worth delaying this PR for.

  77. in examples/musig.c:70 in f8f4f3c7a1 outdated
    65+    if (!secp256k1_musig_pubkey_ec_tweak_add(ctx, NULL, cache, plain_tweak)) {
    66+        return 0;
    67+    }
    68+    /* Note that we did not provide an output_pk argument, because the
    69+     * resulting pk is also saved in the cache and so if one is just interested
    70+     * in signing the output_pk argument is unnecessary. On the other hand, if
    


    theStack commented at 11:39 am on July 18, 2024:

    nit

    0    /* Note that we did not provide an output_pk argument, because the
    1     * resulting pk is also saved in the cache and so if one is just interested
    2     * in signing, the output_pk argument is unnecessary. On the other hand, if
    

    (for avoiding this to be read as “signing the output_pk argument”)


    jonasnick commented at 9:59 am on July 24, 2024:
    Fixed.
  78. in include/secp256k1_musig.h:242 in f8f4f3c7a1 outdated
    238+ *
    239+ *  secp256k1_musig_pubkey_agg(..., agg_pk, keyagg_cache, pubkeys, ...)
    240+ *  secp256k1_musig_pubkey_ec_tweak_add(..., output_pk, tweak32, keyagg_cache)
    241+ *  secp256k1_ec_pubkey_serialize(..., buf, ..., output_pk, ...)
    242+ *  secp256k1_ec_pubkey_tweak_add(..., agg_pk, tweak32)
    243+ *  secp256k1_ec_pubkey_serialize(..., buf2, ..., agg_pk, ...)
    


    theStack commented at 2:47 pm on July 18, 2024:

    The aggregated agg_pk resulting from the _musig_pubkey_agg is an x-only public key, so it can’t be used for the _ec_pubkey_... functions below. Seems like the earlier version, getting agg_pk from the keyagg_cache via _musig_pubkey_get(...), was correct (sorry, it was me who proposed this “simplification” in #1479 (review) 🤦‍♂️ ).

    0 *  secp256k1_musig_pubkey_agg(..., keyagg_cache, pubkeys, ...)
    1 *  secp256k1_musig_pubkey_get(..., agg_pk, keyagg_cache)
    2 *  secp256k1_musig_pubkey_ec_tweak_add(..., output_pk, tweak32, keyagg_cache)
    3 *  secp256k1_ec_pubkey_serialize(..., buf, ..., output_pk, ...)
    4 *  secp256k1_ec_pubkey_tweak_add(..., agg_pk, tweak32)
    5 *  secp256k1_ec_pubkey_serialize(..., buf2, ..., agg_pk, ...)
    

    jonasnick commented at 9:59 am on July 24, 2024:
    Oops I missed that too, fixed.
  79. in examples/musig.c:188 in f8f4f3c7a1 outdated
    183+    }
    184+    printf("ok\n");
    185+    printf("Combining public keys...");
    186+    fflush(stdout);
    187+    /* If you just want to aggregate and not sign the cache can be NULL */
    188+    if (!secp256k1_musig_pubkey_agg(ctx, &agg_pk, &cache, pubkeys_ptr, N_SIGNERS)) {
    


    theStack commented at 3:24 pm on July 18, 2024:
    nit: the resulting agg_pk is not used here, as it’s overwritten in the tweak call below (maybe it’s also fine to keep passing it for demonstration purposes, I just thought it could cause confusion, in the sense that readers think this result is needed for the tweaking below, in addition to the cache)

    jonasnick commented at 10:00 am on July 24, 2024:
    Set agg_pk argument to NULL and improved comment.
  80. in examples/musig.c:257 in f8f4f3c7a1 outdated
    212+        return 1;
    213+    }
    214+    printf("ok\n");
    215+    secp256k1_context_destroy(ctx);
    216+    return 0;
    217+}
    


    theStack commented at 3:26 pm on July 18, 2024:
    Should the secrets be cleared out (here and in sign), like also done in other examples?

    jonasnick commented at 10:00 am on July 24, 2024:
    Thanks, secrets should be cleared everywhere now.
  81. in src/modules/musig/keyagg.h:21 in f8f4f3c7a1 outdated
    16+typedef struct {
    17+    secp256k1_ge pk;
    18+    /* If there is no "second" public key, second_pk is set to the point at
    19+     * infinity */
    20+    secp256k1_ge second_pk;
    21+    unsigned char pk_hash[32];
    


    theStack commented at 4:01 pm on July 18, 2024:

    nit, feel free to ignore:

    0    unsigned char pks_hash[32];
    

    maybe, to express that this is the hash of multiple pubkeys, not a single one?


    jonasnick commented at 10:00 am on July 24, 2024:
    done
  82. in src/modules/musig/keyagg_impl.h:114 in f8f4f3c7a1 outdated
    109+    sha->s[7] = 0x4484be15ul;
    110+    sha->bytes = 64;
    111+}
    112+
    113+/* Compute KeyAgg coefficient which is constant 1 for the second pubkey and
    114+ * otherwise tagged_hash(pk_hash, x) where pk_hash is the hash of public keys.
    


    theStack commented at 4:03 pm on July 18, 2024:
    0 * otherwise tagged_hash(pk_hash, pk) where pk_hash is the hash of public keys.
    

    jonasnick commented at 10:00 am on July 24, 2024:
    fixed
  83. in src/modules/musig/keyagg_impl.h:229 in f8f4f3c7a1 outdated
    226+    }
    227+
    228+    secp256k1_extrakeys_ge_even_y(&pkp);
    229+    if (agg_pk != NULL) {
    230+        secp256k1_xonly_pubkey_save(agg_pk, &pkp);
    231+    }
    


    theStack commented at 4:10 pm on July 18, 2024:

    nit

    0    if (agg_pk != NULL) {
    1        secp256k1_extrakeys_ge_even_y(&pkp);
    2        secp256k1_xonly_pubkey_save(agg_pk, &pkp);
    3    }
    

    jonasnick commented at 10:00 am on July 24, 2024:
    done
  84. in include/secp256k1_musig.h:25 in f8f4f3c7a1 outdated
    20+ *
    21+ * It is recommended to read the documentation in this include file carefully.
    22+ * Further notes on API usage can be found in doc/musig.md
    23+ *
    24+ * Since the first version of MuSig is essentially replaced by MuSig2, we use
    25+ * MuSig, musig and MuSig2 synonymously unless noted otherwise.
    


    josibake commented at 2:23 pm on July 22, 2024:

    in https://github.com/bitcoin-core/secp256k1/pull/1479/commits/f8f4f3c7a10345295ed72cdae0faecff37a919e5:

    nit: spacing

     0/** This module implements BIP 327 "MuSig2 for BIP340-compatible
     1  * Multi-Signatures"
     2  * (https://github.com/bitcoin/bips/blob/master/bip-0327.mediawiki)
     3  * v1.0.0. You can find an example demonstrating the musig module in
     4  * examples/musig.c.
     5  *
     6  * The module also supports BIP-341 ("Taproot") public key tweaking.
     7  *
     8  * It is recommended to read the documentation in this include file carefully.
     9  * Further notes on API usage can be found in doc/musig.md
    10  *
    11  * Since the first version of MuSig is essentially replaced by MuSig2, we use
    12  * MuSig, musig and MuSig2 synonymously unless noted otherwise.
    

    jonasnick commented at 10:01 am on July 24, 2024:
    I improved this doc comment in a way that’s consistent with the existing doc.
  85. in include/secp256k1_musig.h:53 in f8f4f3c7a1 outdated
    48+ *
    49+ *  WARNING: This structure MUST NOT be copied or read or written to directly. A
    50+ *  signer who is online throughout the whole process and can keep this
    51+ *  structure in memory can use the provided API functions for a safe standard
    52+ *  workflow. See
    53+ *  https://blockstream.com/2019/02/18/musig-a-new-multisignature-standard/ for
    


    josibake commented at 2:27 pm on July 22, 2024:

    in https://github.com/bitcoin-core/secp256k1/commit/f8f4f3c7a10345295ed72cdae0faecff37a919e5:

    Seems brittle to point to a blog for an important warning about the risks of using a struct in this library. What about copying the relevant parts from the blog into doc/musig.md and updating this link to point to the docs?


    jonasnick commented at 10:01 am on July 24, 2024:
    Yeah, I re-read the post and I’m not sure what exactly we wanted to convey here. The link predates the BIP. I think just saying that copying the nonce can leak the secret key is clear enough. I removed the link.
  86. in include/secp256k1_musig.h:161 in f8f4f3c7a1 outdated
    156+ *  Returns: 1 when the signature could be serialized, 0 otherwise
    157+ *  Args:    ctx: pointer to a context object
    158+ *  Out:   out32: pointer to a 32-byte array to store the serialized signature
    159+ *  In:      sig: pointer to the signature
    160+ */
    161+SECP256K1_API int secp256k1_musig_partial_sig_serialize(
    


    josibake commented at 2:31 pm on July 22, 2024:

    in https://github.com/bitcoin-core/secp256k1/commit/f8f4f3c7a10345295ed72cdae0faecff37a919e5:

    micro-nit: for all of the other function pairs, the order in the header is parse, serialize. This one switches the order, which is a bit ocd’ triggering :sweat_smile:


    jonasnick commented at 10:01 am on July 24, 2024:
    Let’s minimize the triggers where reasonable ;P Changed.
  87. in include/secp256k1_musig.h:248 in f8f4f3c7a1 outdated
    243+ *  secp256k1_ec_pubkey_serialize(..., buf2, ..., agg_pk, ...)
    244+ *
    245+ *  This function is required if you want to _sign_ for a tweaked aggregate key.
    246+ *  On the other hand, if you are only computing a public key, but not intending
    247+ *  to create a signature for it, you can just use
    248+ *  `secp256k1_ec_pubkey_tweak_add`.
    


    josibake commented at 2:40 pm on July 22, 2024:

    in https://github.com/bitcoin-core/secp256k1/commit/f8f4f3c7a10345295ed72cdae0faecff37a919e5:

    nit: this reads a bit like a suggestions, perhaps:

    0 *  This function is required if you want to _sign_ for a tweaked aggregate key.
    1 *  If you are only computing a public key but not intending to create a signature 
    2 *  for it, use `secp256k1_ec_pubkey_tweak_add` instead.
    

    jonasnick commented at 10:02 am on July 24, 2024:
    done
  88. in include/secp256k1_musig.h:288 in f8f4f3c7a1 outdated
    283+ *  secp256k1_xonly_pubkey_tweak_add_check(..., buf, ..., agg_pk, tweak32)
    284+ *
    285+ *  This function is required if you want to _sign_ for a tweaked aggregate key.
    286+ *  On the other hand, if you are only computing a public key, but not intending
    287+ *  to create a signature for it, you can just use
    288+ *  `secp256k1_xonly_pubkey_tweak_add`.
    


    josibake commented at 2:42 pm on July 22, 2024:

    in https://github.com/bitcoin-core/secp256k1/commit/f8f4f3c7a10345295ed72cdae0faecff37a919e5:

    nit: same as above:

    0 *  This function is required if you want to _sign_ for a tweaked aggregate key.
    1 *  If you are only computing a public key but not intending to create a signature 
    2 *  for it, use `secp256k1_xonly_pubkey_tweak_add` instead.
    

    jonasnick commented at 10:02 am on July 24, 2024:
    done
  89. in src/modules/musig/keyagg_impl.h:31 in f8f4f3c7a1 outdated
    26+ * - 32 byte hash of all public keys
    27+ * - 1 byte the parity of the internal key (if tweaked, otherwise 0)
    28+ * - 32 byte tweak
    29+ */
    30+/* Requires that cache_i->pk is not infinity. */
    31+static void secp256k1_keyagg_cache_save(secp256k1_musig_keyagg_cache *cache, secp256k1_keyagg_cache_internal *cache_i) {
    


    josibake commented at 3:23 pm on July 22, 2024:

    in https://github.com/bitcoin-core/secp256k1/commit/f8f4f3c7a10345295ed72cdae0faecff37a919e5:

    Any reason *cache_i isn’t const? You’d need to also make the in params for both _ge_to_bytes and _ge_to_bytes_ext to be const, but seems like they should be unless I’m missing something.


    josibake commented at 3:44 pm on July 22, 2024:
    FWIW, I changed these functions to const and was able to compile and run the tests.

    jonasnick commented at 10:02 am on July 24, 2024:
    Thanks, fixed.
  90. in src/modules/musig/keyagg_impl.h:41 in f8f4f3c7a1 outdated
    36+    ptr += 64;
    37+    secp256k1_ge_to_bytes_ext(ptr, &cache_i->second_pk);
    38+    ptr += 64;
    39+    memcpy(ptr, cache_i->pk_hash, 32);
    40+    ptr += 32;
    41+    *ptr = cache_i->parity_acc;
    


    josibake commented at 3:25 pm on July 22, 2024:

    in https://github.com/bitcoin-core/secp256k1/commit/f8f4f3c7a10345295ed72cdae0faecff37a919e5:

    Note to self/other reviewers: this initially tripped me up a bit as I expected something like memcpy(ptr, &cache_i->parity_acc, 1); but after some googling, apparently this is the more idiomatic/performant way of doing this!

  91. in src/modules/musig/keyagg_impl.h:79 in f8f4f3c7a1 outdated
    74+    sha->s[7] = 0xab148a38ul;
    75+    sha->bytes = 64;
    76+}
    77+
    78+/* Computes pk_hash = tagged_hash(pk[0], ..., pk[np-1]) */
    79+static int secp256k1_musig_compute_pk_hash(const secp256k1_context *ctx, unsigned char *pk_hash, const secp256k1_pubkey * const* pk, size_t np) {
    


    josibake commented at 3:28 pm on July 22, 2024:

    in https://github.com/bitcoin-core/secp256k1/commit/f8f4f3c7a10345295ed72cdae0faecff37a919e5:

    nit: slightly more readable if pk is plural:

    0static int secp256k1_musig_compute_pk_hash(const secp256k1_context *ctx, unsigned char *pk_hash, const secp256k1_pubkey * const* pks, size_t n_pks) {
    

    jonasnick commented at 10:02 am on July 24, 2024:
    done
  92. in src/modules/musig/keyagg_impl.h:86 in f8f4f3c7a1 outdated
    81+    size_t i;
    82+
    83+    secp256k1_musig_keyagglist_sha256(&sha);
    84+    for (i = 0; i < np; i++) {
    85+        unsigned char ser[33];
    86+        size_t ser_len = sizeof(ser);
    


    josibake commented at 3:35 pm on July 22, 2024:

    in https://github.com/bitcoin-core/secp256k1/commit/f8f4f3c7a10345295ed72cdae0faecff37a919e5:

    Could move this outside the loop to make this slightly more performant and, imo, more readable. Not sure the performance matters but could make a difference if computing pk_hash for a large number of keys?


    jonasnick commented at 10:02 am on July 24, 2024:
    I think that would be less readable than declaring the variables only in the block where they are needed. The compiler will almost certainly optimize the current version to not cause any overhead.
  93. in src/modules/musig/keyagg_impl.h:125 in f8f4f3c7a1 outdated
    120+
    121+    VERIFY_CHECK(!secp256k1_ge_is_infinity(pk));
    122+
    123+    if (!secp256k1_ge_is_infinity(second_pk)
    124+          && secp256k1_fe_equal(&pk->x, &second_pk->x)
    125+          && secp256k1_fe_is_odd(&pk->y) == secp256k1_fe_is_odd(&second_pk->y)) {
    


    josibake commented at 3:38 pm on July 22, 2024:

    in https://github.com/bitcoin-core/secp256k1/commit/f8f4f3c7a10345295ed72cdae0faecff37a919e5:

    Not sure how important this check is , but I noticed this branch is not covered while checking gcov results. IIUC, the test case to add would be for pk and second_pk to have the same x value but even and odd y values.


    jonasnick commented at 10:03 am on July 24, 2024:
    Good catch! We want everything covered by gcov. I replaced this with ge_equals_ge_var which was added after I wrote this code.
  94. josibake commented at 3:43 pm on July 22, 2024: member
    Working my way through src/modules/musig/keyagg_impl.h, left some suggestions but nothing blocking so feel free to ignore
  95. in src/modules/musig/keyagg_impl.h:46 in 8703fe4807 outdated
    41+    *ptr = cache_i->parity_acc;
    42+    ptr += 1;
    43+    secp256k1_scalar_get_b32(ptr, &cache_i->tweak);
    44+}
    45+
    46+static int secp256k1_keyagg_cache_load(const secp256k1_context* ctx, secp256k1_keyagg_cache_internal *cache_i, const secp256k1_musig_keyagg_cache *cache) {
    


    josibake commented at 10:07 am on July 23, 2024:

    in https://github.com/bitcoin-core/secp256k1/commit/f8f4f3c7a10345295ed72cdae0faecff37a919e5:

    Note to self/reviewers: wasn’t obvious to me how ctx was being used at first but it’s being used in the ARG_CHECK macro.


    josibake commented at 10:29 am on July 23, 2024:
    On second thought, seems a bit strange to be using ARG_CHECK inside an internal function (i.e., not exposed in the musig module API). Why not do an assert or a return here, which would let you simplify the function by removing ctx from the function signature?
  96. in src/modules/musig/session_impl.h:47 in 8703fe4807 outdated
    42+static int secp256k1_musig_ge_parse_ext(secp256k1_ge* ge, const unsigned char *in33) {
    43+    unsigned char zeros[33] = { 0 };
    44+
    45+    if (secp256k1_memcmp_var(in33, zeros, sizeof(zeros)) == 0) {
    46+        secp256k1_ge_set_infinity(ge);
    47+        return 1;
    


    josibake commented at 10:19 am on July 23, 2024:

    in https://github.com/bitcoin-core/secp256k1/commit/f8f4f3c7a10345295ed72cdae0faecff37a919e5:

    Feels like I’m missing some context here. Generally, I’d expect setting the group element to the point at infinity to be a failure (i.e., expect the function to return 0;). But here it seems to be perfectly acceptable? My guess is that this failure mode is handled later on in a different function? It might be worth add some context in the comment for why this function is returning true, despite returning the point at infinity in the output param


    jonasnick commented at 10:03 am on July 24, 2024:

    The point of the serialize_ext and parse_ext functions is to be able to serialize and parse the point at infinity. So this is not a failure case. This is noted in the doc of the function:

    0/* Outputs the point at infinity if the given byte array is all zero, otherwise
    1 * attempts to parse compressed point serialization. */
    
  97. in src/modules/musig/session_impl.h:54 in 8703fe4807 outdated
    49+    return secp256k1_eckey_pubkey_parse(ge, in33, 33);
    50+}
    51+
    52+static const unsigned char secp256k1_musig_secnonce_magic[4] = { 0x22, 0x0e, 0xdc, 0xf1 };
    53+
    54+static void secp256k1_musig_secnonce_save(secp256k1_musig_secnonce *secnonce, const secp256k1_scalar *k, secp256k1_ge *pk) {
    


    josibake commented at 10:22 am on July 23, 2024:

    in https://github.com/bitcoin-core/secp256k1/commit/f8f4f3c7a10345295ed72cdae0faecff37a919e5:

    0static void secp256k1_musig_secnonce_save(secp256k1_musig_secnonce *secnonce, const secp256k1_scalar *k, const secp256k1_ge *pk) {
    

    jonasnick commented at 10:04 am on July 24, 2024:
    fixed (also in a few other places in session_impl.h)
  98. in src/modules/musig/session_impl.h:61 in 8703fe4807 outdated
    56+    secp256k1_scalar_get_b32(&secnonce->data[4], &k[0]);
    57+    secp256k1_scalar_get_b32(&secnonce->data[36], &k[1]);
    58+    secp256k1_ge_to_bytes(&secnonce->data[68], pk);
    59+}
    60+
    61+static int secp256k1_musig_secnonce_load(const secp256k1_context* ctx, secp256k1_scalar *k, secp256k1_ge *pk, secp256k1_musig_secnonce *secnonce) {
    


    josibake commented at 10:24 am on July 23, 2024:

    in https://github.com/bitcoin-core/secp256k1/commit/f8f4f3c7a10345295ed72cdae0faecff37a919e5:

    0static int secp256k1_musig_secnonce_load(const secp256k1_context* ctx, secp256k1_scalar *k, secp256k1_ge *pk, const secp256k1_musig_secnonce *secnonce) {
    

    jonasnick commented at 10:04 am on July 24, 2024:
    fixed
  99. in src/modules/musig/keyagg_impl.h:257 in 8703fe4807 outdated
    254+    VERIFY_CHECK(ctx != NULL);
    255+    if (output_pubkey != NULL) {
    256+        memset(output_pubkey, 0, sizeof(*output_pubkey));
    257+    }
    258+    ARG_CHECK(keyagg_cache != NULL);
    259+    ARG_CHECK(tweak32 != NULL);
    


    josibake commented at 10:31 am on July 23, 2024:

    in https://github.com/bitcoin-core/secp256k1/commit/f8f4f3c7a10345295ed72cdae0faecff37a919e5:

    Generally speaking, seems like the ARG_CHECKS should be happening in the public facing functions, not internal functions? i.e., doing a runtime check inside a function an external caller doesn’t have access to seems a bit strange to me.


    jonasnick commented at 10:04 am on July 24, 2024:
    At the cost of code duplication?

    josibake commented at 6:58 am on July 25, 2024:

    In this case, I think it helps with the clarity to have the arg checking happen in the public function before calling the internal function (even at the cost of code duplication).

    That being said, this is really just a style nit so feel free to resolve this discussion if you prefer the (current) more concise version.


    jonasnick commented at 7:07 pm on July 25, 2024:
    I tried this suggestion and I prefer the more concise version.
  100. in src/modules/musig/keyagg_impl.h:287 in 8703fe4807 outdated
    284+
    285+int secp256k1_musig_pubkey_ec_tweak_add(const secp256k1_context* ctx, secp256k1_pubkey *output_pubkey, secp256k1_musig_keyagg_cache *keyagg_cache, const unsigned char *tweak32) {
    286+    return secp256k1_musig_pubkey_tweak_add_internal(ctx, output_pubkey, keyagg_cache, tweak32, 0);
    287+}
    288+
    289+int secp256k1_musig_pubkey_xonly_tweak_add(const secp256k1_context* ctx, secp256k1_pubkey *output_pubkey, secp256k1_musig_keyagg_cache *keyagg_cache, const unsigned char *tweak32) {
    


    josibake commented at 10:32 am on July 23, 2024:

    in https://github.com/bitcoin-core/secp256k1/commit/f8f4f3c7a10345295ed72cdae0faecff37a919e5:

    per my comment above, this is where I’d expect the ARG_CHECKS to happen, vs inside the internal function

  101. in src/modules/musig/session_impl.h:66 in 8703fe4807 outdated
    58+    secp256k1_ge_to_bytes(&secnonce->data[68], pk);
    59+}
    60+
    61+static int secp256k1_musig_secnonce_load(const secp256k1_context* ctx, secp256k1_scalar *k, secp256k1_ge *pk, secp256k1_musig_secnonce *secnonce) {
    62+    int is_zero;
    63+    ARG_CHECK(secp256k1_memcmp_var(&secnonce->data[0], secp256k1_musig_secnonce_magic, 4) == 0);
    


    josibake commented at 10:36 am on July 23, 2024:

    in https://github.com/bitcoin-core/secp256k1/commit/f8f4f3c7a10345295ed72cdae0faecff37a919e5:

    This function could be simplified to return void if the ARG_CHECK were moved outside the function (seems like we could safely check these conditions before calling _load?)

  102. josibake commented at 10:43 am on July 23, 2024: member

    Don’t want to repeat the same comment in multiple places so I’ll try to summarise here:

    It seems there are quite a few internal functions where ctx is in the signature only for ARG_CHECK and/or functions that could be void return an error for ARG_CHECK. I’ll revisit the public API of the module and take a look at the example to make sure I’m not missing something, but couldn’t the magic bytes be checked before calling these internal functions and other ARG_CHECKS be moved to the public facing functions? The reason I think this is better is it simplifies the internal functions quite a bit and it also feels strange to me to be checking for illegal argument errors this deep in the internals of the module. It seems to me we should only get an illegal argument from a public facing function. Checking for illegal arguments inside internal functions feels indicates to me we aren’t validating inputs earlier on in the public facing functions.

    Again, I could be missing something here, so more just throwing this out for discussion.

  103. jonasnick force-pushed on Jul 24, 2024
  104. jonasnick commented at 10:04 am on July 24, 2024: contributor

    Thanks @josibake @theStack for the review! I should have addressed all comments except for the “don’t use ARG_CHECK in internal functions one”:

    It seems there are quite a few internal functions where ctx is in the signature only for ARG_CHECK and/or functions that could be void return an error for ARG_CHECK.

    I agree that internal functions should avoid ARG_CHECKs where possible. In the case of the magic bytes, I think it’s best to check the bytes where they are used, i.e., in the _load functions. If we’d check the magic bytes in the calling code, the code would be more fragile. A similar ARG_CHECK happens in secp256k1_pubkey_load in secp256k1.c.

  105. josibake commented at 7:07 am on July 25, 2024: member

    I think it’s best to check the bytes where they are used, i.e., in the _load functions

    Thinking about this more (and taking a second look at the code), this is a really good point. Just to convince myself, I started working on a patch that moves the _load functions out of the internal functions wherever possible and instead passes cache_i as a parameter to the internal functions. The idea here was you could reason a cache_i has already been validated, which would simplify the logic of the internal functions and in some cases allows us to remove ctx from the function signatures and make some of the internal functions void. However, this ended up making cache_i an In/Out param and overall did feel more brittle.

    Feel free to resolve my review comments re: ARG_CHECK.

  106. in examples/musig.c:45 in 75ff36d5db outdated
    39+    unsigned char seckey[32];
    40+    while (1) {
    41+        if (!fill_random(seckey, sizeof(seckey))) {
    42+            printf("Failed to generate randomness\n");
    43+            return 0;
    44+        }
    


    josibake commented at 7:38 am on July 25, 2024:
    The while is unnecessary here (see #1570)

    jonasnick commented at 7:45 pm on July 25, 2024:
    Let’s keep it consistent for now with the other examples.
  107. in examples/musig.c:63 in 75ff36d5db outdated
    57+/* Tweak the pubkey corresponding to the provided keyagg cache, update the cache
    58+ * and return the tweaked aggregate pk. */
    59+static int tweak(const secp256k1_context* ctx, secp256k1_xonly_pubkey *agg_pk, secp256k1_musig_keyagg_cache *cache) {
    60+    secp256k1_pubkey output_pk;
    61+    unsigned char plain_tweak[32] = "this could be a BIP32 tweak....";
    62+    unsigned char xonly_tweak[32] = "this could be a taproot tweak..";
    


    josibake commented at 8:05 am on July 25, 2024:

    It might be worth including a warning in the example (or even the API docs) that the tweak functions take generic tweaks and it is the caller’s responsibility to make sure these tweaks are safely and correctly computed, e.g., according to the BIP341 spec in the case of a taproot commitment.

    When reviewing the musig_pubkey_tweak_add_internal function, I was a bit surprised when I didn’t see taptweak logic in the function because I had seen “taproot commitment”, “tap tweak” mentioned a few times in the API docs/examples. Obviously, it makes sense that this would be a generic tweak32 and we wouldn’t want the musig2 module to be implementing logic from other protocols, but I could also see a naive caller passing h to _xonly_tweak_add, assuming it will be computing H_TapTweak(P || h)under the hood.


    jonasnick commented at 9:11 am on July 26, 2024:
    Good point. I updated the include file, example & musig.md to clarify that tweak32 is supposed to be a hash. Also added two sentences to the include file that say that the user is responsible for providing a tweak32 that is compatible with MuSig2 (although it is not known whether a fully attacker-controlled tweak32 breaks unforgeability).
  108. in examples/musig.c:151 in 75ff36d5db outdated
    139+         * you must _never_ reuse the secnonce (or use the same session_secrand to
    140+         * create a secnonce). If you do, you effectively reuse the nonce and
    141+         * leak the secret key. */
    142+        if (!secp256k1_musig_partial_sign(ctx, &signer[i].partial_sig, &signer_secrets[i].secnonce, &signer_secrets[i].keypair, cache, &session)) {
    143+            return 0;
    144+        }
    


    josibake commented at 8:11 am on July 25, 2024:
    nit: perhaps worth having an assert that secnonce is 0 to drive the point home?

    jonasnick commented at 7:46 pm on July 25, 2024:
    Hm, then people will copy the assert into their code…
  109. in examples/musig.c:168 in 75ff36d5db outdated
    163+        }
    164+    }
    165+    return secp256k1_musig_partial_sig_agg(ctx, sig64, &session, partial_sigs, N_SIGNERS);
    166+}
    167+
    168+ int main(void) {
    


    josibake commented at 8:40 am on July 25, 2024:

    nit: spacing

    0int main(void) {
    
  110. in examples/musig.c:20 in 75ff36d5db outdated
    15+#include <assert.h>
    16+#include <string.h>
    17+
    18+#include <secp256k1.h>
    19+#include <secp256k1_schnorrsig.h>
    20+#include <secp256k1_musig.h>
    


    josibake commented at 8:42 am on July 25, 2024:

    nit: ordering, add secp256k1_extrakeys.hsince keypair is used

    0#include <secp256k1.h>
    1#include <secp256k1_extrakeys.h
    2#include <secp256k1_musig.h>
    3#include <secp256k1_schnorrsig.h>
    
  111. josibake commented at 9:40 am on July 25, 2024: member
    The example is great and really helped me understand how the full two round protocol is supposed to work. Left some nits, but nothing blocking
  112. jonasnick force-pushed on Jul 25, 2024
  113. jonasnick force-pushed on Jul 25, 2024
  114. jonasnick force-pushed on Jul 26, 2024
  115. jonasnick force-pushed on Jul 26, 2024
  116. in include/secp256k1_musig.h:160 in aee8456257 outdated
    155+ *  Out:     sig: pointer to a signature object
    156+ *  In:     in32: pointer to the 32-byte signature to be parsed
    157+ *
    158+ *  After the call, sig will always be initialized. If parsing failed or the
    159+ *  encoded numbers are out of range, signature verification with it is
    160+ *  guaranteed to fail for every message and public key.
    


    theStack commented at 10:28 am on July 29, 2024:
    In the API description for _musig_partial_sig_parse: the statement “sig will always be initialized” is currently not true, as the function returns if the scalar encoded in in32 overflows, without setting sig: https://github.com/jonasnick/secp256k1/blob/cf20ad88cd15bd21fcae9e3e7080d310434a21ce/src/modules/musig/session_impl.h#L281-L294

    jonasnick commented at 3:00 pm on July 29, 2024:

    I removed that paragraph.

    What we can provide to the user is the guarantee that even if the return value of partial_sig_parse isn’t checked, then partial_sig_verify will for sure fail. Even though that was already the case due to the magic, I added an extra memset to make this extra clear. I don’t see a reason to mention this to the user (they should just check return values…).

  117. in src/modules/musig/keyagg_impl.h:241 in aee8456257 outdated
    236+    VERIFY_CHECK(ctx != NULL);
    237+    ARG_CHECK(agg_pk != NULL);
    238+    memset(agg_pk, 0, sizeof(*agg_pk));
    239+    ARG_CHECK(keyagg_cache != NULL);
    240+
    241+    if(!secp256k1_keyagg_cache_load(ctx, &cache_i, keyagg_cache)) {
    


    theStack commented at 10:32 am on July 29, 2024:

    whitespace nit

    0    if (!secp256k1_keyagg_cache_load(ctx, &cache_i, keyagg_cache)) {
    
  118. in src/modules/musig/session_impl.h:426 in aee8456257 outdated
    421+        aggpk_ser_ptr = aggpk_ser;
    422+    }
    423+    if (!secp256k1_pubkey_load(ctx, &pk, pubkey)) {
    424+        return 0;
    425+    }
    426+    pk_serialize_success = secp256k1_eckey_pubkey_serialize(&pk, pk_ser, &pk_ser_len, SECP256K1_EC_COMPRESSED);
    


    theStack commented at 10:37 am on July 29, 2024:

    in _musig_nonce_gen_internal: for the internal pubkey serialization function _eckey_pubkey_serialize, the last parameter is a bool, rather than a collection of flags:

    0    pk_serialize_success = secp256k1_eckey_pubkey_serialize(&pk, pk_ser, &pk_ser_len, 1);
    

    (out of scope for this PR, but maybe we should assert compressed == 0 || compressed == 1 in this function, to avoid issues like this that can happen very easily?)

  119. in src/modules/musig/session_impl.h:389 in aee8456257 outdated
    384+    memset(rand, 0, sizeof(rand));
    385+    memset(&sha, 0, sizeof(sha));
    386+}
    387+
    388+int secp256k1_musig_nonce_gen_internal(const secp256k1_context* ctx, secp256k1_musig_secnonce *secnonce, secp256k1_musig_pubnonce *pubnonce, const unsigned char *input_nonce, const unsigned char *seckey, const secp256k1_pubkey *pubkey, const unsigned char *msg32, const secp256k1_musig_keyagg_cache *keyagg_cache, const unsigned char *extra_input32) {
    389+    secp256k1_keyagg_cache_internal cache_i;
    


    theStack commented at 10:41 am on July 29, 2024:
    in _musig_nonce_gen_internal: nit, could limit the scope of cache_i to where it’s needed, i.e inside the if (keyagg_cache != NULL) body

    jonasnick commented at 3:00 pm on July 29, 2024:
    fixed
  120. in include/secp256k1_musig.h:459 in aee8456257 outdated
    454+    const secp256k1_musig_pubnonce * const *pubnonces,
    455+    size_t n_pubnonces
    456+) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);
    457+
    458+/** Takes the public nonces of all signers and computes a session that is
    459+ *  required for signing and verification of partial signatures.
    


    theStack commented at 10:46 am on July 29, 2024:
    in API description for _musig_nonce_process: should the “Takes the public nonces of” part be replaced by something like “Takes the public aggregated nonce of…”?

    jonasnick commented at 3:01 pm on July 29, 2024:
    Wow this sentence was older than nonce aggregation. Fixed.
  121. in include/secp256k1_musig.h:474 in aee8456257 outdated
    469+ *                      aggregate (and potentially tweaked) pubkey
    470+ */
    471+SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_musig_nonce_process(
    472+    const secp256k1_context *ctx,
    473+    secp256k1_musig_session *session,
    474+    const secp256k1_musig_aggnonce  *aggnonce,
    


    theStack commented at 10:48 am on July 29, 2024:

    whitespace nit

    0    const secp256k1_musig_aggnonce *aggnonce,
    
  122. in src/modules/musig/session_impl.h:214 in aee8456257 outdated
    185+    /* Parsed signatures can not overflow */
    186+    VERIFY_CHECK(!overflow);
    187+    return 1;
    188+}
    189+
    190+int secp256k1_musig_pubnonce_serialize(const secp256k1_context* ctx, unsigned char *out66, const secp256k1_musig_pubnonce* nonce) {
    


    theStack commented at 10:54 am on July 29, 2024:
    pedantic mini-nit: the public _parse/_serialize functions appear in a different order in the API description than in the implementation file here (parse, serialize vs serialize, parse)

    jonasnick commented at 3:00 pm on July 29, 2024:
    fixed
  123. theStack commented at 10:56 am on July 29, 2024: contributor
    Went through another review round, LGTM, only found some nits that I left below. Haven’t looked closer at CI changes and ctime-tests yet.
  124. jonasnick force-pushed on Jul 29, 2024
  125. in src/modules/musig/session_impl.h:474 in 7196ac9511 outdated
    468+     * defense-in-depth measure that may protect against a faulty RNG. */
    469+    for (i = 0; i < 32; i++) {
    470+        acc |= session_secrand32[i];
    471+    }
    472+    ret &= !!acc;
    473+    memset(&acc, 0, sizeof(acc));
    


    theStack commented at 10:25 am on August 2, 2024:

    nit, since this is just a single byte:

    0    acc = 0;
    

    (or is this then more likely to be optimized out by the compiler?)


    real-or-random commented at 10:54 pm on August 4, 2024:

    I guess dead store elimination happens at a later compilation stage that does not care whether the store came from an assignment or a memset, so they’re equally likely to be optimized out by the compiler.

    So unfortunately both variants are without effect. The advantage of an explicit memset is that it stands out / is grepable, if something wants to pick up #636. (Feel free to give it a shot :P)

  126. in src/modules/musig/session_impl.h:550 in 7196ac9511 outdated
    545+    sha->s[7] = 0xde7a2500ul;
    546+    sha->bytes = 64;
    547+}
    548+
    549+/* tagged_hash(aggnonce[0], aggnonce[1], agg_pk, msg) */
    550+static int secp256k1_musig_compute_noncehash(unsigned char *noncehash, secp256k1_ge *aggnonce, const unsigned char *agg_pk32, const unsigned char *msg) {
    


    theStack commented at 10:27 am on August 2, 2024:
    this function always succeeds, so it could be declared void instead, and the return value check in _musig_nonce_process_internal below can then be removed as well.

    jonasnick commented at 11:44 am on August 5, 2024:
    fixed
  127. in src/modules/musig/tests_impl.h:399 in 7196ac9511 outdated
    366+    CHECK_ILLEGAL(CTX, secp256k1_musig_nonce_process(CTX, &session, &aggnonce, NULL, &keyagg_cache));
    367+    CHECK_ILLEGAL(CTX, secp256k1_musig_nonce_process(CTX, &session, &aggnonce, msg, NULL));
    368+    CHECK_ILLEGAL(CTX, secp256k1_musig_nonce_process(CTX, &session, &aggnonce, msg, &invalid_keyagg_cache));
    369+    CHECK(secp256k1_musig_nonce_process(CTX, &session, &aggnonce, msg, &keyagg_cache) == 1);
    370+
    371+    CHECK(secp256k1_musig_nonce_process(CTX, &session, &aggnonce, msg, &keyagg_cache) == 1);
    


    theStack commented at 12:39 pm on August 4, 2024:
    nit: duplicate call

    jonasnick commented at 11:44 am on August 5, 2024:
    fixed
  128. in src/ctime_tests.c:229 in 7196ac9511 outdated
    224+        CHECK(ret == 1);
    225+
    226+        CHECK(secp256k1_musig_nonce_agg(ctx, &aggnonce, pubnonce_ptr, 1));
    227+        /* Make sure that previous tests don't undefine msg. It's not used as a secret here. */
    228+        SECP256K1_CHECKMEM_DEFINE(msg, sizeof(msg));
    229+        CHECK(secp256k1_musig_nonce_process(ctx, &session, &aggnonce, msg, &cache) == 1);
    


    theStack commented at 12:43 pm on August 4, 2024:
    any reason why storing the result into ret and calling the _CHECKMEM_DEFINE macro isn’t needed/done here?

    jonasnick commented at 11:44 am on August 5, 2024:
    The return value doesn’t depend on secret data.
  129. in src/modules/musig/tests_impl.h:529 in 7196ac9511 outdated
    524+    secp256k1_sha256 sha;
    525+    secp256k1_sha256_initialize_tagged(&sha, tag, taglen);
    526+    test_sha256_eq(&sha, sha_tagged);
    527+}
    528+
    529+/* Checks that the initialized tagged hashes initialized have the expected
    


    theStack commented at 12:53 pm on August 4, 2024:

    nit: one initialized too much i guess

    0/* Checks that the initialized tagged hashes have the expected
    

    jonasnick commented at 11:44 am on August 5, 2024:
    fixed
  130. in src/modules/musig/tests_impl.h:536 in 7196ac9511 outdated
    531+static void sha256_tag_test(void) {
    532+    secp256k1_sha256 sha;
    533+    {
    534+        char tag[11] = "KeyAgg list";
    535+        secp256k1_musig_keyagglist_sha256(&sha);
    536+        sha256_tag_test_internal(&sha, (unsigned char*)tag, sizeof(tag));
    


    theStack commented at 12:57 pm on August 4, 2024:

    nit: not sure if it’s worth to change it, but could either use strlen or sizeof(tag)-1 to avoid specifying the exact size for the array

    0        char tag[] = "KeyAgg list";
    1        secp256k1_musig_keyagglist_sha256(&sha);
    2        sha256_tag_test_internal(&sha, (unsigned char*)tag, sizeof(tag)-1);
    

    jonasnick commented at 11:44 am on August 5, 2024:
    done
  131. theStack approved
  132. theStack commented at 1:04 pm on August 4, 2024: contributor

    Light code-review ACK 398051cba6052c77f801b0aa57f1b0f161b0b12d

    Reviewed the implementation code thoroughly and verified that it matches the BIP327 specification, and that all best practices that I’m aware of are followed (in particular, cleaning of secret data). Spent only a relatively small time looking at test and CI code.

  133. jonasnick force-pushed on Aug 5, 2024
  134. theStack approved
  135. theStack commented at 8:11 pm on August 5, 2024: contributor
    Light re-ACK 5dccc7b4578fb583df60290fc363220cbacb7a03
  136. in src/modules/musig/session_impl.h:90 in 2288e88088 outdated
    85+
    86+static const unsigned char secp256k1_musig_pubnonce_magic[4] = { 0xf5, 0x7a, 0x3d, 0xa0 };
    87+
    88+/* Saves two group elements into a pubnonce. Requires that none of the provided
    89+ * group elements is infinity. */
    90+static void secp256k1_musig_pubnonce_save(secp256k1_musig_pubnonce* nonce, const secp256k1_ge* ge) {
    


    josibake commented at 8:07 am on August 6, 2024:

    in https://github.com/bitcoin-core/secp256k1/pull/1479/commits/2288e8808883556dc27cea6a61f409c08f70b30d:

    nit: took me a second to mentally parse this function since the variable name is singular (ge), but it really is two group elements. Not a big deal since this is an internal function, but I think it would be more clear if the variable name were something like ges or two_ges. There are several other places where an array of secp256k1_ge objects is referred to as just ge, would be nice to make them all plural if you end up taking this suggestion.


    jonasnick commented at 7:09 pm on September 2, 2024:
    Ok, I should have replaced all the singulars with plurals.
  137. in src/modules/musig/session_impl.h:567 in 2288e88088 outdated
    545+    sha->s[7] = 0xde7a2500ul;
    546+    sha->bytes = 64;
    547+}
    548+
    549+/* tagged_hash(aggnonce[0], aggnonce[1], agg_pk, msg) */
    550+static void secp256k1_musig_compute_noncehash(unsigned char *noncehash, secp256k1_ge *aggnonce, const unsigned char *agg_pk32, const unsigned char *msg) {
    


    josibake commented at 8:16 am on August 6, 2024:

    in https://github.com/bitcoin-core/secp256k1/pull/1479/commits/2288e8808883556dc27cea6a61f409c08f70b30d:

    nit: same feedback regarding singular vs plural:

    0static void secp256k1_musig_compute_noncehash(unsigned char *noncehash, secp256k1_ge *aggnonces, const unsigned char *agg_pk32, const unsigned char *msg) {
    
  138. josibake approved
  139. josibake commented at 8:37 am on August 6, 2024: member

    ACK https://github.com/bitcoin-core/secp256k1/commit/5dccc7b4578fb583df60290fc363220cbacb7a03

    Thanks for all the hard work on this, @jonasnick; I realise a lot of my feedback was on style 😅 Verified to the best of my ability that this conforms to the BIP327 spec, and that the module is well-tested (mainly by running and checking the gcov results and manually inspecting the tests). Overall, I found the module to be very well organised and easy to follow. I left one last nit regarding singular vs plural arguments, but not a blocker.

  140. t-bast commented at 8:02 am on August 23, 2024: none

    Hey all, thanks for the hard work on this feature! We’re actively using this in our tests for lightning stuff: we haven’t thoroughly reviewed the code (we don’t have the C skills to give a good enough review) but we’ve found the API simple enough to use so concept ACK :+1:

    Are there any blockers left on this PR or can we expect it to be integrated in the next release?

  141. in CHANGELOG.md:14 in 2288e88088 outdated
    10@@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
    11 
    12 #### Added
    13  - Added usage example for an ElligatorSwift key exchange.
    14+ - New module `musig` implements the MuSig2 multisignature scheme according to the [BIP 327 specification](https://github.com/bitcoin/bips/blob/master/bip-0327.mediawiki). See:
    15+   - Header file `include/secp256k1_musig.h` which defines the new API.
    16+   - Document `doc/musig.md` for further notes on API usage.
    17+   - Usage example `examples/musig.c`.
    


    theStack commented at 10:17 am on August 26, 2024:
    this should be moved up to the “Unreleased” section now

    jonasnick commented at 7:09 pm on September 2, 2024:
    Oh, done!
  142. in src/modules/musig/keyagg.h:12 in 2288e88088 outdated
     7+#define SECP256K1_MODULE_MUSIG_KEYAGG_H
     8+
     9+#include "../../../include/secp256k1.h"
    10+#include "../../../include/secp256k1_musig.h"
    11+
    12+#include "../../field.h"
    


    theStack commented at 12:02 pm on August 26, 2024:

    nit: no field element type is used in this header, so could remove this include


    jonasnick commented at 7:09 pm on September 2, 2024:
    done
  143. in src/modules/musig/session_impl.h:750 in 2288e88088 outdated
    731+
    732+    /* Compute "effective" nonce rj = aggnonce[0] + b*aggnonce[1] */
    733+    /* TODO: use multiexp to compute -s*G + e*mu*pubkey + aggnonce[0] + b*aggnonce[1] */
    734+    if (!secp256k1_musig_pubnonce_load(ctx, nonce_pt, pubnonce)) {
    735+        return 0;
    736+    }
    


    theStack commented at 3:10 pm on August 26, 2024:

    comment nit in function secp256k1_musig_partial_sig_verify: IIUC, the effective nonce here is computed from the individual signer’s public nonce points, not the aggregated ones (the latter we could just fetch from the session instance), according to https://github.com/bitcoin/bips/blob/97012a82064c7247df502a170c03b053825cdd15/bip-0327.mediawiki?plain=1#L497-L499

    0    /* Compute "effective" nonce rj = pubnonce[0] + b*pubnonce[1] */
    1    /* TODO: use multiexp to compute -s*G + e*mu*pubkey + pubnonce[0] + b*pubnonce[1] */
    2    if (!secp256k1_musig_pubnonce_load(ctx, nonce_pt, pubnonce)) {
    3        return 0;
    4    }
    

    jonasnick commented at 7:10 pm on September 2, 2024:
    Yes, done.
  144. bitcoin-core deleted a comment on Aug 26, 2024
  145. in include/secp256k1_musig.h:458 in 2288e88088 outdated
    453+
    454+/** Takes the aggregate nonce and creates a session that is required for signing
    455+ *  and verification of partial signatures.
    456+ *
    457+ *  Returns: 0 if the arguments are invalid or if some signer sent invalid
    458+ *           pubnonces, 1 otherwise
    


    theStack commented at 11:56 am on August 29, 2024:

    in the secp256k1_musig_nonce_process API description: should the “some signer…” part be removed, as suggested in secp256k1-zkp (https://github.com/BlockstreamResearch/secp256k1-zkp/pull/189)? seems like the “sent invalid pubnonce” case would have already been caught previously by the nonce aggregation function

    0 *  Returns: 0 if the arguments are invalid, 1 otherwise
    

    jonasnick commented at 7:10 pm on September 2, 2024:
    Right, forgot about that. We can even make the type of nonce_process_internal void.
  146. in src/modules/musig/session_impl.h:576 in 2288e88088 outdated
    571+    secp256k1_musig_compute_noncehash(noncehash, aggnonce, agg_pk32, msg);
    572+    secp256k1_gej_set_ge(&aggnoncej[0], &aggnonce[0]);
    573+    secp256k1_gej_set_ge(&aggnoncej[1], &aggnonce[1]);
    574+    /* fin_nonce = aggnonce[0] + b*aggnonce[1] */
    575+    secp256k1_scalar_set_b32(b, noncehash, NULL);
    576+    secp256k1_gej_set_infinity(&fin_nonce_ptj);
    


    theStack commented at 1:22 pm on August 29, 2024:

    nit: this line shouldn’t be needed, as fin_nonce_ptj is overwritten in the next line anyways

    fwiw, out of curiosity I tried deduplicating the calculation of $R = R1 + b*R2$ (needed both for calculating the final nonce in _musig_nonce_process and for the “effective” nonce in the partial sig verification _musig_partial_sig_verify) on the following branch: https://github.com/theStack/secp256k1/commit/419c05cce03b2b719fcdc86f734f86a87953bdf1 Not sure if its worth it to pick this up, but if yes, it probably needs a better name for the helper function 😅


    jonasnick commented at 7:11 pm on September 2, 2024:
    Yes, done. Also added your helper function.
  147. in include/secp256k1_musig.h:98 in 5dccc7b457 outdated
    92+ *  Guaranteed to be 36 bytes in size. Serialized and parsed with
    93+ *  `musig_partial_sig_serialize` and `musig_partial_sig_parse`.
    94+ */
    95+typedef struct {
    96+    unsigned char data[36];
    97+} secp256k1_musig_partial_sig;
    


    Kixunil commented at 6:27 pm on August 31, 2024:
    It’s unclear to me if these structs have any alignment needs. On the surface it looks like they don’t but that might make the performance worse on some architectures? If it’s not the case then it could be still nice to explicitly say they are unaligned in the doc. If it’s the case maybe instead align them and say it in the doc?

    real-or-random commented at 0:52 am on September 1, 2024:

    Hm, I don’t see how anyone could get the idea that any type in a public header would have stricter alignment than what C anyway requires (unless otherwise specified in the header, of course). And The implied alignment is 1 in this case.

    As far as I remember, we “read” from these char arrays into our internal data structures (the performance penalty is probably negligible), and so alignment shouldn’t matter for performance either.


    Kixunil commented at 7:12 am on September 1, 2024:
    Someone coming from Rust and being paranoid about everything. :D Thanks for clarifying, though I think a documentation comment would still be nice.
  148. Kixunil commented at 6:56 pm on August 31, 2024: none

    I have a question that’s maybe more about MuSig itself not this PR but it may have implications that could affect the API. But firstly I want to apologize for the question not being well-researched upfront, maybe this was already addressed somewhere, I don’t have that much time right now and I feel like I should speak up before this merges and cements the API.

    From my understanding the reason we have this “toxic data” of making sure nonces are not reused is that the counterparties could send nonces that are computed as sum of their nonce and inverse of our nonce which would allow them to compute private key. However can’t the same be achieved by simply proving that they know the discrete log? Which IIUC could be done by simply single-signing the message they’re going to sign (or some HMAC of it to avoid cryptographic interactions) producing a 64-byte proof. And once the nonces are proven to be random they can be deterministically derived from the message, the set of public keys and the private key. So the tradeoff would be to make all signing messages 64-byte larger to eliminate the risk that the state leaks somehow and with it also the keys or that nonces get reused by accident because of HW failure or a bug.

    For an application I intend to use MuSig for I could easily have a kilobyte proof and it would still be worth the tradeoff. But I don’t see any API here providing this. Sorry again if I missed it or if there’s some serious problem in my reasoning that’s documented somewhere. If this can be refuted with a link, you can just paste the link.

    Thanks for consideration and all the hard work that went into this!

  149. Bigheem approved
  150. real-or-random commented at 0:06 am on September 1, 2024: contributor

    From my understanding the reason we have this “toxic data” of making sure nonces are not reused is that the counterparties could send nonces that are computed as sum of their nonce and inverse of our nonce which would allow them to compute private key.

    I think you’re mixing up two different issues. What you describe doesn’t lead to an attack. The attacker can choose nonces that cancel out a honest signer’s nonces, but the only effect this will have is that the attacker won’t be able to come up with a valid partial signature and thus the protocol will fail.

    This cancellation attack you describe is a concern not with nonces, but with the individual public keys in the key aggregation. Indeed, cancelling a key of an honest signer would work if the aggregate public key was just the sum of the individual keys. (This is called a “rouge key attack” or literally “a key cancellation attack”). But MuSig2 eliminates this attack vector by using a key aggregation function that is not just the sum of the keys, but instead a sum with random coefficients.

    But yes, letting everyone prove knowledge of the discrete logarithm of their public individual public key using a Schnorr signature is a different way to eliminate rogue key attacks. This is used in SpeedyMuSig: https://eprint.iacr.org/2021/1375.pdf The drawback of this method is that it doesn’t suffice to send these additional Schnorr signatures around during the signing protocol. For this to be secure, the signatures need to be there already at the key aggregation stage, which is possibly performed by a different party. In MuSig2, anyone (i.e., not just the involved signers) can combine some keys A and B into an aggregate key and use it, e.g., send money to it, and it will be ensured that A and B can only spend it if they work together. In SpeedyMuSig, the party who performs the key aggregation needs to see the Schnorr signatures valid for A and B before they can securely send money to the aggregate key A+B. This complicates key management and is a potential footgun (people could send to A+B without checking the additional Schnorr sigs).

    edit: For the actual problem with reusing nonces, see either the paper or https://delvingbitcoin.org/t/how-many-nonce-reuse-before-exposing-your-musig2-private-key/217, but both are rather involved…

  151. Kixunil commented at 8:18 am on September 1, 2024: none

    Ah, OK I think I understand better now. Thanks for explaining! What I was really thinking was if all nonces are deterministic then for the same key and same message they produce the same signature so how could an attacker possibly extract additional information from that? What I was missing is there’s no way for a signer to know if the pubnonce sent by another party was generated deterministically except for having some kind of ZKP that would be likely expensive to construct and verify.

    Still it would be nice to not have to store the secnonce on a permanent storage so maybe a viable solution for cases when there’s one “unreliable” signer (random user with a smartphone that could kill the app at any time) and other parties are “reliable” (run on server 24/7, can hold the secnonces in RAM) would be for the “unreliable” signer to collect all nonces first and compute his own as an HMAC(secret_key, message || aggregate_pubkey || all_nonces_except_signers) and be the last one who sends pubnonce. The other signers hold their secnonces in RAM in the meantime. Does that make sense?

  152. jonasnick commented at 4:54 pm on September 2, 2024: contributor

    @Kixunil

    if all nonces are deterministic then for the same key and same message they produce the same signature so how could an attacker possibly extract additional information from that? What I was missing is there’s no way for a signer to know if the pubnonce sent by another party was generated deterministically except for having some kind of ZKP that would be likely expensive to construct and verify.

    Proving that the nonces were generated deterministically from the session parameters is the idea behind deterministic signing in MuSig-DN and Exponent-VRFs and Their Applications.

    Does that make sense?

    It sounds like you’re describing a variant of the “Deterministic and Stateless Signing for a Single Signer”-mode of BIP MuSig. In this mode, one signer is able to derive the nonces deterministically from the session parameters and the pubnonces of all other signers.

  153. group: add ge_to_bytes and ge_from_bytes 85e224dd97
  154. jonasnick force-pushed on Sep 2, 2024
  155. jonasnick force-pushed on Sep 2, 2024
  156. Kixunil commented at 8:55 am on September 3, 2024: none

    @jonasnick thanks for references!

    It sounds like you’re describing a variant of the “Deterministic and Stateless Signing for a Single Signer”-mode of BIP MuSig.

    Yes, that’s what I want! From what I can see it’s not currently supported by the API. Is this planned or can it be implemented some other way?

  157. in include/secp256k1_musig.h:110 in 52cd8e0a7a outdated
    104+ *  In:     in66: pointer to the 66-byte nonce to be parsed
    105+ */
    106+SECP256K1_API int secp256k1_musig_pubnonce_parse(
    107+    const secp256k1_context *ctx,
    108+    secp256k1_musig_pubnonce *nonce,
    109+    const unsigned char *in66
    


    Kixunil commented at 8:59 am on September 3, 2024:
    Wouldn’t it be better to declare this as const unsigned char (*in66)[66]? Aside from clarity it could help static analysis tools or perhaps bindings generators to generate correct bindings.

    jonasnick commented at 1:53 pm on September 3, 2024:
    That’s an interesting point. Right now, we consistently use declarations of the form const unsigned char *in66 consistently in the library. I prefer to have a standard across the whole API and would avoid just changing this in the musig module.

    Kixunil commented at 3:20 pm on September 3, 2024:
    That makes sense. Might be nice to change all of them at some point unless there’s a problem I’m missing.

    sipa commented at 8:04 pm on September 4, 2024:

    You cannot pass arrays as arguments in C; they collapse into pointers. The syntax

    0int some_func(char c[32]) {
    1    ...
    2}
    

    is for all intents and purposes equivalent to int some_func(char *c) { ...}, including the fact that sizeof(c) will be 4 or 8 bytes (same as sizeof(char*)). Because it is so confusing, it’s generally considered misleading to use arrays as arguments in C code.

    EDIT: Oh, you’re not suggesting this, but int some_func(char (*c)[32]) instead. That does work, and equivalent after compilation, but it does mean an additional & at the caller, and an additional * inside the callee. Can you elaborate on the (potential) advantages for static analysis or binding generators?


    Kixunil commented at 5:34 pm on September 5, 2024:

    For instance in Rust we have bindgen, a tool that generates bindings automatically. I’ve tried to pass it this header file:

    0void foo(const unsigned char *arg);
    1void bar(const unsigned char arg[32]);
    2void baz(const unsigned char (*arg)[32]);
    

    And it generated this code:

    0extern "C" {
    1    pub fn foo(arg: *const ::std::os::raw::c_uchar);
    2}
    3extern "C" {
    4    pub fn bar(arg: *const ::std::os::raw::c_uchar);
    5}
    6extern "C" {
    7    pub fn baz(arg: *const [::std::os::raw::c_uchar; 32usize]);
    8}
    

    As you can see, the last one has a different type. And while it’s the same from ABI perspective, there’s a real difference between them in Rust since you can directly pass in &array without any casts because the types match making the code more obviously correct.

    Currently, we aren’t using bidgen in rust-secp256k1 but we might in the future and I’d be surprised if similar tools don’t exist in other languages.

    Also I’m not familiar with the current state of static analysis in C but I think a reasonable static analyzer should be able to see that statements like out[33] = 42 or baz(&twentyone_item_array) are obviously wrong. gcc already complains about the these when compiling with -O2 -W -Wall -Warray-bounds.


    sipa commented at 1:28 am on September 6, 2024:

    Interesting!

    It’s unfortunate that it’s an API break to introduce this (but indeed not an ABI break, I think).


    Kixunil commented at 4:13 am on September 6, 2024:

    Oh, I found that bindgen has an option to generate *const [c_uchar; 32] instead of *const c_uchar for arguments defined as const unsigned char arg[32], so maybe even that is already valuable? (IIUC not API-breaking) I think it has documentation value too.

    It looks like cppcheck is also able to detect mistakes when you use these (I haven’t tested but I found a PR that supposedly does it.)


    real-or-random commented at 4:05 pm on November 18, 2024:
    @Kixunil We discussed this in our IRC meeting today. Can you try if bindgen understands arg[static 32]? This is a special syntax in C99, see https://hamberg.no/erlend/posts/2013-02-18-static-array-indices.html
  158. in doc/musig.md:36 in 52cd8e0a7a outdated
    31+
    32+1. Generate a keypair with `secp256k1_keypair_create` and obtain the public key with `secp256k1_keypair_pub`.
    33+2. Call `secp256k1_musig_pubkey_agg` with the pubkeys of all participants.
    34+3. Optionally add a (Taproot) tweak with `secp256k1_musig_pubkey_xonly_tweak_add` and a plain tweak with `secp256k1_musig_pubkey_ec_tweak_add`.
    35+4. Generate a pair of secret and public nonce with `secp256k1_musig_nonce_gen` and send the public nonce to the other signers.
    36+5. Someone (not necessarily the signer) aggregates the public nonce with `secp256k1_musig_nonce_agg` and sends it to the signers.
    


    sipa commented at 3:34 pm on September 6, 2024:
    Nit: the public nonces.

    jonasnick commented at 2:28 pm on September 12, 2024:
    fixed
  159. in doc/musig.md:44 in 52cd8e0a7a outdated
    39+8. Verify the partial signatures (optional in some scenarios) with `secp256k1_musig_partial_sig_verify`.
    40+9. Someone (not necessarily the signer) obtains all partial signatures and aggregates them into the final Schnorr signature using `secp256k1_musig_partial_sig_agg`.
    41+
    42+The aggregate signature can be verified with `secp256k1_schnorrsig_verify`.
    43+
    44+Note that steps 1 to 5 can happen before the message to be signed is known to the signers.
    


    sipa commented at 3:40 pm on September 6, 2024:

    I think it makes sense to stress that the normal (safer) mode is to construct the nonces after the message is known, rather than only pointing out the alternative here.

    Suggested language:

    Steps 1 through 5 above can happen before or after the message to be signed is known to the signers. Wherever possible, it is recommended to only generate the nonces after the message is known, as it has more defense-in-depth measures, but requires two communication rounds at signing time. The alternative, generating the nonces in a pre-processing step before the message is known, removes those measures, but means signing can happen non-interatively.


    jonasnick commented at 2:28 pm on September 12, 2024:
    Agree, improved the wording.
  160. in include/secp256k1_musig.h:327 in 52cd8e0a7a outdated
    323+ *  2. If you already know the seckey, message or aggregate public key
    324+ *     cache, they can be optionally provided to derive the nonce and increase
    325+ *     misuse-resistance. The extra_input32 argument can be used to provide
    326+ *     additional data that does not repeat in normal scenarios, such as the
    327+ *     current time.
    328+ *  3. Avoid copying (or serializing) the secnonce. This reduces the possibility
    


    sipa commented at 5:52 pm on September 6, 2024:

    In case someone has no way around storing session data on disk somewhere, would the recommendation be:

    • To store/load the secnonce to/from disk (violating rule 3 here, plus risking platform dependence).
    • To store/load the session_secrand32 to/from disk and re-run secp256k1_musig_nonce_gen to obtain the same secnonce (violating rule 1 here, and duplicating computation).

    I suspect some users will have not have the ability to leave the secnonce in volatile memory, so it may be useful to give advice on how to do that (plus reiterate the dangers of not wiping the stored data after signing).


    jonasnick commented at 2:29 pm on September 12, 2024:
    We don’t provide the ability to serialize the secnonce right now, which prevents users from storing/loading from disk. Maybe worth a separate PR?

    sipa commented at 5:10 pm on September 12, 2024:
    Well they can just store the raw bytes on disk, which works, but risks platform-dependency issues.

    jonasnick commented at 9:37 am on September 13, 2024:

    If we provide the on store/load the secnonce from disk, we should also provide de/serialization functions because our API docs instruct users to not read the raw bytes. I had considered this out of scope for this initial PR because it seems dangerous. Especially since so far no one requested this feature, just “don’t store to disk” seems to be a sensible answer.

    Actually one developer asked me about the missing de/serialization functions for the secnonce but further discussion on their motivation for requesting this revealed that they planned to build a very insecure protocol because they had entirely misunderstood what nonce reuse actually is. In the end they agreed that they should not store the secnonce to disk and changed their implementation accordingly.

    Also I had hoped that someone would eventually come up with a more clever API that can protect from misuse better than simple de/serialization functions (at least in some cases). But maybe that just doesn’t exist.


    sipa commented at 6:54 pm on September 13, 2024:
    That’s totally reasonable, but I don’t think you have answered my question. If someone is somehow not able to keep everything in volatile memory, they have two (bad) options (store secnonce, or store session_secrand32), and both options violate a rule. Which of the two would you, as API and protocol designer, recommend people take?

    jonasnick commented at 3:23 pm on September 16, 2024:

    If someone really needs to do this right now, then I would probably recommend option 2. In contrast to option 1, if you reuse session_secrand but you’re signing for a different message or aggregate public key, you’re protected.

    One downside of that method is that session_secrand has no “in-memory protection”, in the sense that the session_secrand buffer is not cleared after nonce_generation. We do wipe the secnonce after signing. Maybe we should change nonce generation to overwrite the session_secrand buffer?


    real-or-random commented at 12:30 pm on September 24, 2024:

    I agree that option 2 is better for the reasons mentioned.

    We do wipe the secnonce after signing. Maybe we should change nonce generation to overwrite the session_secrand buffer?

    That sounds reasonable to me.


    sipa commented at 3:05 pm on September 24, 2024:
    Seems like a good idea, yes.

    jonasnick commented at 7:28 pm on September 25, 2024:
    Done. The secrand buffer is invalidated whenever nonce_gen returns 1. The alternative is to invalidate whenever a session_secrand argument is given, which is a tiny bit more tedious to test and document. Anyway, if nonce_gen returns 0, then the secnonce is invalid as well, so reusing the secrand buffer in that case is not an issue.
  161. in include/secp256k1_musig.h:347 in 52cd8e0a7a outdated
    340+ *  Out:     secnonce: pointer to a structure to store the secret nonce
    341+ *           pubnonce: pointer to a structure to store the public nonce
    342+ *  In:
    343+ *  session_secrand32: a 32-byte session_secrand32 as explained above. Must be unique to this
    344+ *                     call to secp256k1_musig_nonce_gen and must be uniformly random.
    345+ *             seckey: the 32-byte secret key that will later be used for signing, if
    


    sipa commented at 5:54 pm on September 6, 2024:
    If provided, what happens if the seckey does not match the pubkey argument? (also applies to secp256k1_musig_nonce_gen_counter).

    jonasnick commented at 2:29 pm on September 12, 2024:
    The musig_nonce_gen functions do not check if the seckey matches the given pubkey.

    sipa commented at 5:11 pm on September 12, 2024:
    But will for example the resulting signature be invalid, or does doing this risk leaking private keys?

    jonasnick commented at 9:20 am on September 13, 2024:

    If the wrong public key is provided, signing will fail. This works because the public key is appended to the secnonce output of the nonce_gen functions and partial_sign will compare this public key with the public key the signer is trying to sign for.

    Providing the optional seckey argument is a defense-in-depth measure to strengthen nonce generation against a low entropy session_secrand argument. If provided, the seckey is simply hashed into the nonce and does not have an effect on whether the resulting signature is valid.

    So, the idea behind the defense-in-depth measure is that if the seckey does correspond to the pubkey argument and has high entropy, but the session_secrand argument has low entropy (but does not repeat), the generated nonce is secure. On the other hand, if the seckey does not match the pubkey, nonce generation is only insecure if both the provided seckey and secrand have low entropy. Providing a wrong seckey cannot negatively affect nonce generation as long as secrand has enough entropy.


    real-or-random commented at 12:35 pm on September 24, 2024:

    If the wrong public key is provided, signing will fail. This works because the public key is appended to the secnonce output of the nonce_gen functions and partial_sign will compare this public key with the public key the signer is trying to sign for.

    Let me add that the reason for this (and for the need for a pubkey argument in the first place) is that it prevents a vulnerability that could occur when people sign with tweaked individual keys, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-October/021000.html (you should remember the story).


    real-or-random commented at 12:38 pm on September 24, 2024:
    The docs currently just say “Returns: 0 if the arguments are invalid and 1 otherwise” Perhaps we should note that a pubkey/seckey mismatch does not count as invalid, but that it will be caught later during signing.

    jonasnick commented at 7:25 pm on September 25, 2024:

    Perhaps we should note that a pubkey/seckey mismatch does not count as invalid, but that it will be caught later during signing.

    I added a sentence, but it may be confusing. A pubkey/seckey mismatch in nonce generation may not be caught during signing, because the signer can still provide the correct seckey for signing.

  162. in include/secp256k1_musig.h:441 in 52cd8e0a7a outdated
    431+/** Aggregates the nonces of all signers into a single nonce
    432+ *
    433+ *  This can be done by an untrusted party to reduce the communication
    434+ *  between signers. Instead of everyone sending nonces to everyone else, there
    435+ *  can be one party receiving all nonces, aggregating the nonces with this
    436+ *  function and then sending only the aggregate nonce back to the signers.
    


    sipa commented at 5:57 pm on September 6, 2024:
    Worth pointing out what happens when the aggregator misbehaves (presumably, either the partial signatures will be invalid, or the resulting aggregated signature will be invalid)?

    jonasnick commented at 2:29 pm on September 12, 2024:
    Yeah, done.
  163. in include/secp256k1_musig.h:424 in 52cd8e0a7a outdated
    419+SECP256K1_API int secp256k1_musig_nonce_gen_counter(
    420+    const secp256k1_context *ctx,
    421+    secp256k1_musig_secnonce *secnonce,
    422+    secp256k1_musig_pubnonce *pubnonce,
    423+    uint64_t nonrepeating_cnt,
    424+    const unsigned char *seckey,
    


    sipa commented at 6:04 pm on September 6, 2024:
    If both seckey and pubkey are mandatory here, would it make sense to instead take a secp256k1_keypair as argument?

    jonasnick commented at 2:29 pm on September 12, 2024:
    Yeah, makes sense. I implemented that
  164. in src/modules/musig/keyagg.h:23 in 52cd8e0a7a outdated
    18+     * infinity */
    19+    secp256k1_ge second_pk;
    20+    unsigned char pks_hash[32];
    21+    /* tweak is identical to value tacc[v] in the specification. */
    22+    secp256k1_scalar tweak;
    23+    /* parity_acc corresponds to gacc[v] in the spec. If gacc[v] is -1,
    


    sipa commented at 6:06 pm on September 6, 2024:
    So one could say it actually corresponds to (1-gacc[v])/2 in the spec?

    jonasnick commented at 2:29 pm on September 12, 2024:
    Yes, done.
  165. in src/modules/musig/keyagg_impl.h:79 in 52cd8e0a7a outdated
    74+    sha->s[7] = 0xab148a38ul;
    75+    sha->bytes = 64;
    76+}
    77+
    78+/* Computes pks_hash = tagged_hash(pk[0], ..., pk[np-1]) */
    79+static int secp256k1_musig_compute_pks_hash(const secp256k1_context *ctx, unsigned char *pks_hash, const secp256k1_pubkey * const* pks, size_t np) {
    


    sipa commented at 6:10 pm on September 6, 2024:
    Maybe call the argument pks_hash32 (following the style of putting the size of array pointers in the name).

    jonasnick commented at 2:30 pm on September 12, 2024:
    I think we only followed that style for externally facing functions, also in the Schnorr signature and extrakeys module. But it’d be possible to adopt that style and start with the musig module.

    real-or-random commented at 1:14 pm on September 24, 2024:
    I think we sometimes use it internally, but I don’t think it should be a strict rule. And in this case, “32” is almost implied by “hash” already.
  166. sipa commented at 6:13 pm on September 6, 2024: contributor
    Mostly looked through the interface and documentation.
  167. jonasnick force-pushed on Sep 12, 2024
  168. jonasnick force-pushed on Sep 12, 2024
  169. in CHANGELOG.md:11 in b6efaa53cc outdated
     6@@ -7,10 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
     7 
     8 ## [Unreleased]
     9 
    10-## [0.5.1] - 2024-08-01
    11-
    12 #### Added
    13  - Added usage example for an ElligatorSwift key exchange.
    


    theStack commented at 9:32 am on September 13, 2024:
    changelog nit: this line accidentally slipped into the “Unreleased” part now, though it was already in the 0.5.1 release

    jonasnick commented at 9:48 am on September 13, 2024:
    :vomiting_face: … thanks … should be fixed for real now
  170. jonasnick force-pushed on Sep 13, 2024
  171. in src/modules/musig/keyagg_impl.h:119 in 81f1c6ae31 outdated
    114+ * otherwise tagged_hash(pks_hash, pk) where pks_hash is the hash of public keys.
    115+ * second_pk is the point at infinity in case there is no second_pk. Assumes
    116+ * that pk is not the point at infinity and that the Y-coordinates of pk and
    117+ * second_pk are normalized. */
    118+static void secp256k1_musig_keyaggcoef_internal(secp256k1_scalar *r, const unsigned char *pks_hash, secp256k1_ge *pk, const secp256k1_ge *second_pk) {
    119+    secp256k1_sha256 sha;
    


    theStack commented at 12:17 pm on September 19, 2024:
    nit: could move the sha instance into the else-branch, as it’s only needed there

    jonasnick commented at 7:24 pm on September 25, 2024:
    fixed
  172. in src/modules/musig/keyagg_impl.h:87 in 81f1c6ae31 outdated
    82+
    83+    secp256k1_musig_keyagglist_sha256(&sha);
    84+    for (i = 0; i < np; i++) {
    85+        unsigned char ser[33];
    86+        size_t ser_len = sizeof(ser);
    87+        if (!secp256k1_ec_pubkey_serialize(ctx, ser, &ser_len, pks[i], SECP256K1_EC_COMPRESSED)) {
    


    theStack commented at 12:36 pm on September 19, 2024:
    note for other reviewers: it’s intentional to use the public API function for serializing a pubkey here (as opposed to the internal one called _eckey_pubkey_serialize, which is faster and used at all other places in the musig module) , as we want to check the validity of the input pubkeys of secp256k1_musig_pubkey_agg.
  173. theStack approved
  174. theStack commented at 1:19 pm on September 19, 2024: contributor

    Code-review ACK fdc09608036822afc1cebbe0c5b56cebf8ba508d

    Reviewed once again that the code matches the specification in BIP327 and also took a closer look on the tests. Regarding the interface discussion #1479 (review), I wonder if it’s acceptable for now to not support (and not even mention) the use-case where users can’t store the secnonce in volatile memory by now, and leave that for a follow-up? If a suggested way to work around that is described in the API docs (“if you really need to do this…”), that could be seen by users as invitation to break the rules. On the other hand, if nothing is mentioned, users might break the rules anyway and do it in an even worse way, so not sure what is better. Anyways, happy to re-review on changes in that area (e.g. suggestions in #1479 (review)).

  175. in README.md:24 in fdc0960803 outdated
    20@@ -21,6 +21,7 @@ Features:
    21 * Optional module for ECDH key exchange.
    22 * Optional module for Schnorr signatures according to [BIP-340](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki).
    23 * Optional module for ElligatorSwift key exchange according to [BIP-324](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki).
    24+* Optional module for the MuSig2 multi-signature scheme according to [BIP-327](https://github.com/bitcoin/bips/blob/master/bip-0327.mediawiki).
    


    real-or-random commented at 12:09 pm on September 24, 2024:

    nit:

    0* Optional module for the MuSig2 Schnorr multi-signatures according to [BIP-327](https://github.com/bitcoin/bips/blob/master/bip-0327.mediawiki).
    

    because it mentions Schnorr then


    jonasnick commented at 7:24 pm on September 25, 2024:
    fixed
  176. in include/secp256k1_musig.h:275 in fdc0960803 outdated
    270+ *  generator multiplied with `tweak32` to it. This is useful for creating
    271+ *  Taproot outputs where `tweak32` is set to a TapTweak hash as defined in BIP
    272+ *  341.
    273+ *
    274+ *  Callers are responsible for deriving `tweak32` in a way that does not reduce
    275+ *  the security of MuSig2 (for example, by following Taproot BIP 341).
    


    real-or-random commented at 12:22 pm on September 24, 2024:
    0 *  the security of MuSig (for example, by following Taproot BIP 341).
    

    for consistency


    jonasnick commented at 7:24 pm on September 25, 2024:
    done
  177. in include/secp256k1_musig.h:230 in fdc0960803 outdated
    225+ *  the generator multiplied with `tweak32` to it. This is useful for deriving
    226+ *  child keys from an aggregate public key via BIP 32 where `tweak32` is set to
    227+ *  a hash as defined in BIP 32.
    228+ *
    229+ *  Callers are responsible for deriving `tweak32` in a way that does not reduce
    230+ *  the security of MuSig2 (for example, by following BIP 32).
    


    real-or-random commented at 12:23 pm on September 24, 2024:
    0 *  the security of MuSig (for example, by following BIP 32).
    

    for consistency


    jonasnick commented at 7:24 pm on September 25, 2024:
    done
  178. in include/secp256k1_musig.h:326 in fdc0960803 outdated
    322+ *     SECRET (even from other signers).
    323+ *  2. If you already know the seckey, message or aggregate public key
    324+ *     cache, they can be optionally provided to derive the nonce and increase
    325+ *     misuse-resistance. The extra_input32 argument can be used to provide
    326+ *     additional data that does not repeat in normal scenarios, such as the
    327+ *     current time.
    


    real-or-random commented at 12:27 pm on September 24, 2024:
    0 *  2. If the seckey, message or aggregate public key
    1 *     cache is already available at this stage, any of these can be optionally provided, in which case they will
    2 *     be used in the derivation of the nonce and increase
    3 *     misuse-resistance. The extra_input32 argument can be used to provide
    4 *     additional data that does not repeat in normal scenarios, such as the
    5 *     current time.
    

    Coherence: “you know” vs passive voice. Must fix paragraph formatting.


    jonasnick commented at 7:24 pm on September 25, 2024:
    done
  179. in include/secp256k1_musig.h:292 in fdc0960803 outdated
    289+ *
    290+ *  Returns: 0 if the arguments are invalid or the resulting public key would be
    291+ *           invalid (only when the tweak is the negation of the corresponding
    292+ *           secret key). 1 otherwise.
    293+ *  Args:            ctx: pointer to a context object
    294+ *  Out:   output_pubkey: pointer to a public key to store the result. Will be set
    


    real-or-random commented at 12:39 pm on September 24, 2024:
    0 *  Out:   aggpubkey: pointer to a public key to store the result. Will be set
    

    Or agg_pubkey, but we also use aggnonce. (the same in the code example a few lines above. If you accept this, you may also want to change it in the example file and in keyagg_impl.h.)


    jonasnick commented at 7:33 pm on September 25, 2024:
    It’s called output_pubkey because that is the terminology we use for BIP 341 tweaking. People don’t have that context, so I’m ok with changing it. But if we do, then how about agg_pk because that’s what we’re using currently for the aggregation and get_pubkey functions.

    real-or-random commented at 11:53 am on September 26, 2024:

    It’s called output_pubkey because that is the terminology we use for BIP 341 tweaking.

    Oh, sure. Never mind, I mixed things up and thought this is simply the aggregate key. But it’s the tweaked aggregate key, so yep, it’s better to have a separate name for this, and then output_pk is good.

  180. in include/secp256k1_musig.h:534 in fdc0960803 outdated
    529+ *       create the `session` with `musig_nonce_process`.
    530+ *
    531+ *  It is not required to call this function in regular MuSig sessions, because
    532+ *  if any partial signature does not verify, the final signature will not verify
    533+ *  either, so the problem will be caught. But this function allows determining
    534+ *  the specific party who produced an invalid signature.
    


    real-or-random commented at 1:11 pm on September 24, 2024:

    Certain caveats apply for “determining the specific party” (e.g., one need authenticated connections.) Do we want to add a reference to the BIP327 here?

    Or simpler: Rephrase this to “But this function allows determining which of the partial signatures does not verify.”, which doesn’t say that someone produced an invalid signature.


    jonasnick commented at 7:28 pm on September 25, 2024:
    done
  181. in src/modules/musig/session_impl.h:71 in fdc0960803 outdated
    66+    secp256k1_ge_from_bytes(pk, &secnonce->data[68]);
    67+    /* We make very sure that the nonce isn't invalidated by checking the values
    68+     * in addition to the magic. */
    69+    is_zero = secp256k1_scalar_is_zero(&k[0]) & secp256k1_scalar_is_zero(&k[1]);
    70+    secp256k1_declassify(ctx, &is_zero, sizeof(is_zero));
    71+    ARG_CHECK(!is_zero);
    


    real-or-random commented at 1:50 pm on September 24, 2024:
    Would it be preferable to check if secnonce->data[4..68] is all zeroes instead? I think it’s simpler. (And it’s negligible more correct because overflow could also result in a zero scalar.) edit: Hm, okay, we don’t have a function for constant-time byte comparison, but we do another of these below (when checking if session_secrand32 is all zeroes.)

    jonasnick commented at 7:31 pm on September 25, 2024:
    I implemented this by creating a is_zero_array function in util.h and using it from both places in the musig module. I think this is a bit nicer, because is_zero_array is reusable, but the change did not turn out to make the secnonce load function simpler. Maybe you meant something else?
  182. in src/modules/musig/session_impl.h:515 in fdc0960803 outdated
    510+    }
    511+    memset(seckey, 0, sizeof(seckey));
    512+    return 1;
    513+}
    514+
    515+static int secp256k1_musig_sum_nonces(const secp256k1_context* ctx, secp256k1_gej *summed_nonces, const secp256k1_musig_pubnonce * const* pubnonces, size_t n_pubnonces) {
    


    real-or-random commented at 2:02 pm on September 24, 2024:
    0static int secp256k1_musig_sum_pubnonces(const secp256k1_context* ctx, secp256k1_gej *summed_pubnonces, const secp256k1_musig_pubnonce * const* pubnonces, size_t n_pubnonces) {
    

    perhaps a slightly better name


    jonasnick commented at 7:32 pm on September 25, 2024:
    done
  183. real-or-random approved
  184. real-or-random commented at 2:03 pm on September 24, 2024: contributor

    ACK fdc09608036822afc1cebbe0c5b56cebf8ba508d

    I left a bunch of nits, mostly noticed when I looked at @sipa’s comments in the include file, but none of these are blockers.

  185. in src/group_impl.h:975 in 12835f80af outdated
    970+        secp256k1_ge_to_bytes(data, ge);
    971+    }
    972+}
    973+
    974+static void secp256k1_ge_from_bytes_ext(secp256k1_ge *ge, const unsigned char *data) {
    975+    unsigned char zeros[64] = { 0 };
    


    sipa commented at 2:18 pm on September 24, 2024:
    static const?

    jonasnick commented at 7:32 pm on September 25, 2024:
    done

    real-or-random commented at 11:45 am on October 7, 2024:
    nit: you did this, but in the “wrong” commit

    jonasnick commented at 2:05 pm on October 7, 2024:
    fixed
  186. in doc/musig.md:10 in 81f1c6ae31 outdated
     5+A usage example can be found in `examples/musig.c`.
     6+
     7+## API misuse
     8+
     9+The musig API is designed to be as misuse resistant as possible.
    10+However, the MuSig protocol has some additional failure modes (mainly due to interactivity) that do not appear in single-signing.
    


    sipa commented at 2:24 pm on September 24, 2024:
    Nit: reads a bit unnatural. How about “that do not apply to single signatures”?

    jonasnick commented at 7:32 pm on September 25, 2024:
    improved the wording
  187. in doc/musig.md:11 in 81f1c6ae31 outdated
     6+
     7+## API misuse
     8+
     9+The musig API is designed to be as misuse resistant as possible.
    10+However, the MuSig protocol has some additional failure modes (mainly due to interactivity) that do not appear in single-signing.
    11+While the results can be catastrophic (e.g. leaking of the secret key), it is unfortunately not possible for the musig implementation to rule out all such failure modes.
    


    sipa commented at 2:29 pm on September 24, 2024:
    Nit: “ruling out” sounds like something a human does when assessing a protocol. How about “prevent”?

    jonasnick commented at 7:33 pm on September 25, 2024:
    done
  188. in doc/musig.md:25 in 81f1c6ae31 outdated
    20+   Instead, only the provided accessor functions are used.
    21+
    22+## Key Aggregation and (Taproot) Tweaking
    23+
    24+Given a set of public keys, the aggregate public key is computed with `secp256k1_musig_pubkey_agg`.
    25+A plain tweak can be added to the resulting public key with `secp256k1_ec_pubkey_tweak_add` by setting the `tweak32` argument to the hash defined in BIP 32. Similarly, a Taproot tweak can be added with `secp256k1_xonly_pubkey_tweak_add` by setting the `tweak32` argument to the TapTweak hash defined in BIP 341.
    


    sipa commented at 2:31 pm on September 24, 2024:
    (If true) perhaps mention that can be combined and/or invoked multiple times if the setting calls for that?

    jonasnick commented at 7:33 pm on September 25, 2024:
    done
  189. in include/secp256k1_musig.h:38 in 81f1c6ae31 outdated
    33+ *  comparison, use the corresponding serialization and parsing functions.
    34+ */
    35+
    36+/** Opaque data structure that caches information about public key aggregation.
    37+ *
    38+ *  Guaranteed to be 197 bytes in size. It can be safely copied/moved. No
    


    sipa commented at 2:38 pm on September 24, 2024:
    Worth pointing out (here and elsewhere) that despite being copyable/movable, the encoding is platform- and version-dependent?

    jonasnick commented at 7:36 pm on September 25, 2024:
    I moved the “It can be safely copied/moved.” to the above general section on opaque data structures; right after the sentence that the representation is platform dependent (which is the same as the doc for the other opaque data structures in the API).
  190. in include/secp256k1_musig.h:114 in 81f1c6ae31 outdated
    109+    const unsigned char *in66
    110+) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);
    111+
    112+/** Serialize a signer's public nonce
    113+ *
    114+ *  Returns: 1 when the nonce could be serialized, 0 otherwise
    


    sipa commented at 3:03 pm on September 24, 2024:
    I think it’s only possible that 0 is returned when the API is misused? If so, I think it’s better to say “1 always” (in the sense that API-compliant callers do not need to care about the return value). Same below.

    jonasnick commented at 7:36 pm on September 25, 2024:
    fixed
  191. in include/secp256k1_musig.h:384 in 81f1c6ae31 outdated
    373+ *  This function outputs a secret nonce that will be required for signing and a
    374+ *  corresponding public nonce that is intended to be sent to other signers.
    375+ *
    376+ *  This function differs from `secp256k1_musig_nonce_gen` by accepting a
    377+ *  non-repeating counter value instead of a secret random value. This requires
    378+ *  that the seckey argument (which is part of the keypair argument in
    


    sipa commented at 3:14 pm on September 24, 2024:
    This comment seems outdated now.

    jonasnick commented at 7:37 pm on September 25, 2024:
    Outdated in what way?

    sipa commented at 11:39 pm on September 25, 2024:

    There is no seckey argument anymore.

    EDIT: Ok, it does day “which is part of the keypair argument in secp256k1_musig_nonce_gen_counter”, but that sounds a bit confusing given that this is the documentation for that function.

    What about “This requires that a private key is provided to secp256k1_musig_nonce_gen_counter (through the keypair argument), as opposed to secp256k1_musig_nonce_gen where the seckey argument is optional.”?


    real-or-random commented at 11:32 am on September 26, 2024:
    I like the suggestion, s/private key/secret key is more consistent.

    jonasnick commented at 3:11 pm on September 26, 2024:
    Replaced the sentence with the suggestion.
  192. jonasnick force-pushed on Sep 25, 2024
  193. in examples/musig.c:130 in 96c95bfd3a outdated
    125+            return 0;
    126+        }
    127+        pubnonces[i] = &signer[i].pubnonce;
    128+
    129+        secure_erase(seckey, sizeof(seckey));
    130+        secure_erase(session_secrand, sizeof(session_secrand));
    


    theStack commented at 10:43 am on September 26, 2024:
    with the latest change to secp256k1_musig_nonce_gen, this line wouldn’t be needed anymore (doesn’t hurt to keep it though for demonstrating good practices, I presume)

    jonasnick commented at 3:13 pm on September 26, 2024:
    Good catch. I removed that line. Imo explicitly erasaing secrand is more overkill than good practice.
  194. in src/modules/musig/session_impl.h:67 in 55d3dc0a70 outdated
    62+    int is_zero;
    63+    ARG_CHECK(secp256k1_memcmp_var(&secnonce->data[0], secp256k1_musig_secnonce_magic, 4) == 0);
    64+    /* We make very sure that the nonce isn't invalidated by checking the values
    65+     * in addition to the magic. */
    66+    is_zero = secp256k1_is_zero_array(&secnonce->data[4], 32);
    67+    is_zero |= secp256k1_is_zero_array(&secnonce->data[36], 32);
    


    real-or-random commented at 11:43 am on September 26, 2024:

    This is what I meant. :) Yeah, it’s not so much simpler, but I think it’s an improvement overall.

    If you want to make it one line simpler, you could do this (not sure if it’s better):

    0    is_zero = secp256k1_is_zero_array(&secnonce->data[4], 2 * 32);
    

    jonasnick commented at 3:12 pm on September 26, 2024:
    Oh, yes! Replaced the two lines with the suggestion.
  195. jonasnick force-pushed on Sep 26, 2024
  196. in src/modules/musig/session_impl.h:208 in 6f496a0d5f outdated
    200+        if (!secp256k1_eckey_pubkey_parse(&ges[i], &in66[33*i], 33)) {
    201+            return 0;
    202+        }
    203+        if (!secp256k1_ge_is_in_correct_subgroup(&ges[i])) {
    204+            return 0;
    205+        }
    


    theStack commented at 4:45 pm on September 29, 2024:
    curiosity nit: is this check needed, and if yes, does it make sense to also add it in the aggregate nonce parsing function _musig_aggnonce_parse? IIUC, this is only becomes relevant once exhaustive tests are added, and returns always 1 in production (so no big deal if it stays as-is now).

    jonasnick commented at 8:41 am on October 7, 2024:
    Right, added it for consistency.
  197. theStack commented at 5:14 pm on September 29, 2024: contributor
    Minor API question: are there any musig API functions where the SECP256K1_WARN_UNUSED_RESULT attribute should be added? (Probably for the _{pubnonce,aggnonce,partial_sig}_parse functions, given that external data is passed in, and the nonce generation functions?)
  198. in include/secp256k1_musig.h:292 in 6f496a0d5f outdated
    287+ *  This function is required if you want to _sign_ for a tweaked aggregate key.
    288+ *  If you are only computing a public key but not intending to create a
    289+ *  signature for it, use `secp256k1_xonly_pubkey_tweak_add` instead.
    290+ *
    291+ *  Returns: 0 if the arguments are invalid or the resulting public key would be
    292+ *           invalid (only when the tweak is the negation of the corresponding
    


    sipa commented at 2:12 pm on September 30, 2024:
    Depending on the sign of the previous aggregated public key, I believe it is also possible this returns 0 if the tweak equals the corresponding private key?

    jonasnick commented at 8:41 am on October 7, 2024:
    Correct, updated the description.
  199. in src/modules/musig/session_impl.h:441 in 6f496a0d5f outdated
    436+#endif
    437+
    438+    secp256k1_nonce_function_musig(k, input_nonce, msg32, seckey, pk_ser, aggpk_ser_ptr, extra_input32);
    439+    VERIFY_CHECK(!secp256k1_scalar_is_zero(&k[0]));
    440+    VERIFY_CHECK(!secp256k1_scalar_is_zero(&k[1]));
    441+    VERIFY_CHECK(!secp256k1_scalar_eq(&k[0], &k[1]));
    


    sipa commented at 4:30 pm on September 30, 2024:
    Any given individual linear relation between k0 and k1 is of course practically impossible to reach, but what is the reason for this specific one? BIP327 only seems to suggest tests for k0=0 and k1=0 (in NonceGen()).

    jonasnick commented at 8:42 am on October 7, 2024:
    I don’t remember. k[0] == k[1] doesn’t lead to invalid values in contrast to the case where k[i] == 0 (public nonce would be the unserializable point at infinity). Maybe this was added for testing that nonce_function_musig correctly produces two different scalars, but this is now tested explicitly.
  200. in src/modules/musig/session_impl.h:450 in 6f496a0d5f outdated
    443+    secp256k1_musig_secnonce_invalidate(ctx, secnonce, !ret);
    444+
    445+    for (i = 0; i < 2; i++) {
    446+        secp256k1_gej nonce_ptj;
    447+        secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &nonce_ptj, &k[i]);
    448+        secp256k1_ge_set_gej(&nonce_pts[i], &nonce_ptj);
    


    sipa commented at 4:59 pm on September 30, 2024:
    It would be nice if this could use batch inversion, but secp256k1_ge_set_all_gej_var is variable time (which I don’t think is acceptable here?), and a constant-time version would either need a guarantee that neither point is infinity, or some annoying cmov logic to deal with infinities. Perhaps for a follow-up.

    real-or-random commented at 10:35 pm on October 3, 2024:
    nonce_ptj is guaranteed not to be infinity here. But I agree, this seems something for a follow-up.

    jonasnick commented at 8:42 am on October 7, 2024:

    Why would variable time batch inversion not be acceptable here? We could change the code to:

    0  for (i = 0; i < 2; i++) {
    1      secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &nonce_ptsj[i], &k[i]);
    2      secp256k1_declassify(ctx, &nonce_ptsj[i], sizeof(nonce_ptsj));
    3      secp256k1_scalar_clear(&k[i]);
    4  }
    5  secp256k1_ge_set_all_gej_var(nonce_pts, nonce_ptsj, 2);
    

    sipa commented at 11:52 am on October 7, 2024:
    When performing an EC multiplication A = aG for secret a, the resulting affine coordinates of A are presumed to not leak information about a (ECDLP), but the same is not necessarily true for the Jacobian coordinates that come out of our multiplication algorithm. So we shouldn’t treat nonce_ptj as public here.
  201. in src/modules/musig/session_impl.h:546 in 6f496a0d5f outdated
    541+
    542+    if (!secp256k1_musig_sum_pubnonces(ctx, aggnonce_ptsj, pubnonces, n_pubnonces)) {
    543+        return 0;
    544+    }
    545+    for (i = 0; i < 2; i++) {
    546+        secp256k1_ge_set_gej(&aggnonce_pts[i], &aggnonce_ptsj[i]);
    


    sipa commented at 5:00 pm on September 30, 2024:
    I think secp256k1_ge_set_all_gej_var is usable here, as the public key inputs are not secret?

    jonasnick commented at 8:42 am on October 7, 2024:
    done
  202. in src/modules/musig/session_impl.h:704 in 6f496a0d5f outdated
    700+         != cache_i.parity_acc)) {
    701+        secp256k1_scalar_negate(&sk, &sk);
    702+    }
    703+
    704+    /* Multiply KeyAgg coefficient */
    705+    /* TODO Cache mu */
    


    sipa commented at 5:08 pm on September 30, 2024:
    Is there space in the cache data structure to do this caching? Would it make sense to reserve space for it?

    jonasnick commented at 8:42 am on October 7, 2024:

    Looking at this again, I don’t see an easy way to do this. Caching all coefficients would make the size of the cache dependent on the number of public keys aggregated. Right now, the cache is just a fixed size object.

    Maybe caching the MuSig coefficient is overkill:

    • If a signer doesn’t verify partial signatures, caching all coefficients is unnecessary. They just need their own coefficient to sign. It would be possible to cache just that single coefficient, but that would require making key aggregation aware of who is going to sign with the cache later. This would be unnecessary in cases where key aggregation is done by a non-signer.
    • Computing the coefficient is essentially just to_scalar(hash(32_byte_pks_hash, serialize_33(pk)).

    sipa commented at 11:54 am on October 7, 2024:
    In that case, remove the TODO, or clarify the nuance?

    jonasnick commented at 2:05 pm on October 7, 2024:
    Removed the TODO
  203. in src/modules/musig/tests_impl.h:90 in 6f496a0d5f outdated
    84+
    85+    CHECK(secp256k1_musig_partial_sig_agg(CTX, final_sig, &session, partial_sig_ptr, 2) == 1);
    86+    CHECK(secp256k1_schnorrsig_verify(CTX, final_sig, msg, sizeof(msg), &agg_pk) == 1);
    87+}
    88+
    89+static void pubnonce_summing_to_inf(secp256k1_musig_pubnonce *pubnonce) {
    


    sipa commented at 6:11 pm on September 30, 2024:
    Can you add a comment what this function does?

    jonasnick commented at 8:42 am on October 7, 2024:
    done
  204. in src/modules/musig/tests_impl.h:25 in 6f496a0d5f outdated
    20+#include "../../group.h"
    21+#include "../../hash.h"
    22+#include "../../util.h"
    23+
    24+#include "vectors.h"
    25+#include <inttypes.h>
    


    sipa commented at 6:30 pm on September 30, 2024:
    What is inttypes.h needed for?

    real-or-random commented at 12:20 pm on October 1, 2024:
    yes, it seems that stdint.h suffices here

    jonasnick commented at 8:43 am on October 7, 2024:
    Not needed (anymore). Removed.
  205. sipa commented at 6:41 pm on September 30, 2024: contributor

    Code review ACK 5e55f093118c2517f068841f5414f767fb35c7fe

    I have not done a detailed comparison with the BIP spec.

  206. theStack approved
  207. theStack commented at 11:47 am on October 1, 2024: contributor

    re-ACK 5e55f093118c2517f068841f5414f767fb35c7fe

    (as per $ git diff fdc09608036822afc1cebbe0c5b56cebf8ba508d 5e55f093118c2517f068841f5414f767fb35c7fe)

    As far as I can tell, none of the unadressed questions/comments are critical.

  208. jonasnick force-pushed on Oct 7, 2024
  209. jonasnick commented at 8:41 am on October 7, 2024: contributor

    @theStack

    are there any musig API functions where the SECP256K1_WARN_UNUSED_RESULT attribute should be added?

    That’s a good point. I couldn’t discover a consistent pattern we’re currently following (e.g, secp256k1_ec_pubkey_parse has the warning, secp256k1_tagged_sha256 has the warning even though it always returns 1, and signature_parse_der, signature_parse_compact don’t). I think this should be made more consistent treewide with a note to CONTRIBUTING.md.

    There are different rules we could follow:

    1. Just add the warning basically everywhere. Maybe that could be annoying sometimes if the user has data from a trusted source.
    2. Add it to functions returning a boolean result that would be pointless to call without checking for the result (e.g., verify).
    3. Add to all functions that can fail without calling a callback (e.g. they can fail because they are receiving data from untrusted sources – at least in normal scenarios).
    4. Add it to all functions you need to call before verify to make sure that verify doesn’t succeed due to invalid values. However, this would be defense-in-depth because it shouldn’t happen anyway.
    5. Add it to all functions that operate on a seckey and produce a public output (e.g. sign) and all the functions the user needs to call before. This prevents that an invalid output (e.g., signature) accidentally leaks information about the seckey. However, users should already be protected from this because we zeroize the outputs if these functions fail.
    6. Add it to functions which sound particularly scary (nonce generation).

    I added it to a few more functions (following basically rules 2, 3, 4, 6), because removing the warning is backwards compatible.

  210. real-or-random approved
  211. real-or-random commented at 11:59 am on October 7, 2024: contributor
    reACK 352cd3bd04ed4089cf2250b66eb73a16cb53cab3 nit: musig_example needs to be added to .gitignore
  212. group: add ge_to_bytes_ext and ge_from_bytes_ext c8fbdb1b97
  213. util: add constant-time is_zero_array function 0be79660f3
  214. Add module "musig" that implements MuSig2 multi-signatures (BIP 327) f411841a46
  215. build: allow enabling the musig module in cmake 168c92011f
  216. jonasnick force-pushed on Oct 7, 2024
  217. jonasnick commented at 2:06 pm on October 7, 2024: contributor

    nit: musig_example needs to be added to .gitignore

    fixed

  218. sipa commented at 2:40 pm on October 7, 2024: contributor
    reACK 168c92011f5ddae8d7fe28d166b68f126459548a
  219. real-or-random approved
  220. real-or-random commented at 2:53 pm on October 7, 2024: contributor
    reACK 168c92011f5ddae8d7fe28d166b68f126459548a
  221. real-or-random commented at 2:57 pm on October 7, 2024: contributor
    We have an open issue for SECP256K1_WARN_UNUSED_RESULT, I copied @jonasnick’s comment there.
  222. theStack approved
  223. theStack commented at 3:01 pm on October 7, 2024: contributor
    re-ACK 168c92011f5ddae8d7fe28d166b68f126459548a
  224. real-or-random merged this on Oct 7, 2024
  225. real-or-random closed this on Oct 7, 2024

  226. theStack referenced this in commit c9210784f7 on Oct 11, 2024
  227. achow101 referenced this in commit 5a0d3f1db5 on Oct 12, 2024
  228. hebasto referenced this in commit 5c425b5703 on Oct 16, 2024
  229. theStack referenced this in commit b942b9f70a on Oct 22, 2024
  230. achow101 referenced this in commit c24b343141 on Oct 24, 2024
  231. theStack referenced this in commit 765ef53335 on Oct 25, 2024
  232. vmta referenced this in commit 4d1f6d5635 on Oct 29, 2024
  233. achow101 referenced this in commit 378ca17fd1 on Nov 1, 2024
  234. achow101 referenced this in commit 2d46a89386 on Nov 4, 2024
  235. Eunovo referenced this in commit 55a2f7a840 on Nov 12, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-23 15:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me