Replace MuSig(1) module with MuSig2

[jonasnick]

The main commit comprises 905 insertions(+), 1253 deletions(-). The diff isn't as small as I had hoped, but that's mostly because it was possible to simplify the API quite substantially which required rewriting large parts. Sorry, almost all of the changes are in one big commit which makes the diff very hard to read. Perhaps best to re-review most parts from scratch.

A few key changes:

• Obviously no commitment round. No big session struct and no verifier sessions. No signer struct.
• There's a new secnonce struct that is the output of musig_nonce_gen and derived from a uniformly random session_id32. The derivation can be strengthened by adding whatever session parameters (combined_pk, msg) are available. The nonce function is my ad-hoc construction that allows for these optional inputs. Please have a look at that.
• The secnonce is made invalid after being used in partial_sign.
• Adaptor signatures basically work as before, according to Add MuSig2 adaptor signatures scriptless-scripts#24 (with the exception that they operate on aggregate instead of partial sigs)
• To avoid making this PR overly complex I did not consider how this implementation interacts with nested-MuSig, sign-to-contract, and antiklepto.
• Testing should be close to complete. There's no reachable line or branch that isn't exercised by the tests.
• In the current implementation when a signer sends an invalid nonce (i.e. some garbage that can't be mapped to a group element), it is ignored when combining nonces. Only after receiving the signers partial signature and running partial_sig_verify will we notice that the signer misbehaved. The reason for this is that 1) this makes the API simpler and 2) malicious peers don't gain any additional powers because they can always interrupt the protocol by refusing to sign. However, this is up for discussion. EDIT: this is not the case anymore since invalid nonces are rejected when they're parsed.
• For every partial signature we verify we have to parse the pubnonce (two compressed points), despite having parsed it in process_nonces already. This is not great. process_nonces could optionally output the array of parsed pubnonces. EDIT: fixed by having a dedicated type for nonces.
• I left src/modules/musig/musig.md unchanged for now. Perhaps we should merge it with the musig-spec EDIT: musig.md is updated
• partial verification should use multiexp to compute R1 + b*R2 + c*P, but this can be done in a separate PR
• renaming wishlist
  • pre_session -> keyagg_cache (because there is no session anymore)
  • pubkey_combine, nonce_combine, partial_sig_combine -> pubkey_agg, nonce_agg, partial_sig_agg (shorter, matches terminology in musig2)
  • musig_session_init -> musig_start (shorter, simpler) or musig_generate_nonce or musig_prepare
  • musig_partial_signature to musig_partial_sig (shorter)
• perhaps remove pubnonces and n_pubnonces argument from process_nonces (and then also add a opaque type for the combined nonce?)
• write the combined_pubkey into the pre_session struct (as suggested below: then 1) session_init and process_nonces don't need a combined_pk argument (and there can't be mix up between tweaked and untweaked keys) and 2) pubkey_tweak doesn't need an input_pubkey and the output_pubkey can be written directly into the pre_session (reducing frustration such as Replace MuSig(1) module with MuSig2 Replace MuSig(1) module with MuSig2 #131 (comment))
• perhaps allow adapting both partial sigs (partial_sig struct) and aggregate partial sigs (64 raw bytes) as suggested below.

Based on #120.

[NicolasDorier]

I think this is not well named. Should be secp256k1_musig_generate_nonce.

[jonasnick]

Added suggestion to OP.

[jonasnick]

How do you like my suggestion musig_start (shorter, simpler)?

[NicolasDorier]

Question about the API: why secp256k1_musig_process_nonces is separate from secp256k1_musig_partial_sign ?

Is there a situation where the consumer would want to call them separately? if not, can we just merge those two functions together?

[NicolasDorier]

This line https://github.com/ElementsProject/secp256k1-zkp/blob/fe02ea61437752357351e268c2ed0d782231711b/src/modules/musig/main_impl.h#L155

seems to change the state of the session, even if the tweak add operation fails. I think the line should be moved after the tweak.

[jonasnick]

@NicolasDorier

Question about the API: why secp256k1_musig_process_nonces is separate from secp256k1_musig_partial_sign ?

It is separate because

1. it allows a non-signer to verify partial sigs and adaptor sigs (see session_cache output)
2. it allows a non-signer to aggregate the partial sigs (see sig_template output)

seems to change the state of the session, even if the tweak add operation fails. I think the line should be moved after the tweak.

If the function fails the output (pre_session) is unspecified anyway but ok I changed this.

[NicolasDorier]

To make the API easier to understand, I would advice to remove pubnonces and n_pubnonces, and only accept combined_pubnonce.

[jonasnick]

That's fair, but then everyone has to call musig_nonces_combine instead of allowing signers without a third-party nonce aggregator to skip this function altogether.

[GeneFerneau]

If you decide to keep pubnonces here, consider using a strong type

[NicolasDorier]

I would advice to make process_nonces only returns a single data structure rather than two separate one.
session_cache is used for partial_sign/verify and sig_template is used for partial_sig_combine.

But since those are:

• opaque data structure for the user,
• there is no parsing functions related to this,
• They are both created by process_nonces

It does not make sense for the user of the API to manipulate them as different objects.

[NicolasDorier]

https://github.com/ElementsProject/secp256k1-zkp/blob/f7a27e6cca0116f60c5d1a6d686e262e8fe1fbf7/include/secp256k1_musig.h#L105

musig_session_init doesn't use pre_session.

[NicolasDorier]

Since partial_sign and process_nonces always need pre_session to sign, and that pre_session can't be serialized,
I think the type pre_session should be removed, and process_nonces/partial_sign just accept the pubkeys.

My guess is that you are doing this for performance reason.
But if you do, since partial_sign need process_nonces to be called before, you could just add the data from pre_session in the new data structure I suggest in #131 (comment)

By doing what I suggest sig_template, session_cache and pre_session would be replaced by a single opaque type processed_nonces.

[NicolasDorier]

I am creating some dependency graph before/after to better explain how I would refactor the API. I may be missing something.

[NicolasDorier]

Trying to make sense of the API here is what exist now in term of dependencies:

Here is what I suggest:

Notably, I tried to remove sig_template, session_cache and pre_session behind an opaque processed_nonces.
And removed the need to call pubkey_combine, by making process_nonce accept pubkeys directly.
Then process_nonce only accept the combined nonce.

[NicolasDorier]

I would checking that it does not overflow.

https://github.com/ElementsProject/secp256k1-zkp/blob/f7a27e6cca0116f60c5d1a6d686e262e8fe1fbf7/src/modules/musig/main_impl.h#L475

[jonasnick]

Thanks again for having a detailed look @NicolasDorier.

I would advice to make process_nonces only returns a single data structure rather than two separate one.

This is a fair suggestion. I don't remember the specific reason why I did this. Perhaps has to do with extensibility.

there is no parsing functions related to this,

We will likely add them though.

musig_session_init doesn't use pre_session.

Thanks, fixed.

My guess is that you are doing this for performance reason.

Indeed. Unless I'm missing something, if you remove the pre_session as you suggest, then entities who need to both get the aggregate key and need to call process_nonces are required to compute the aggregate key twice.

I am creating some dependency graph

That looks very helpful!

I would checking that it does not overflow.

What do you suggest doing exactly?

[NicolasDorier]

I would checking that it does not overflow.

In several place in the code, you actively check if the scalar of the partial signature is overflowing.
I think you can remove those branches by just checking if the scalar is overflowing when signature is getting parsed, such that overflow partial sigs can't even be parsed. (especially that the parse function return a int to indicate parsing result already)

Indeed. Unless I'm missing something, if you remove the pre_session as you suggest, then entities who need to both get the aggregate key and need to call process_nonces are required to compute the aggregate key twice.

The only case I think it is useful, is when you got all the partial signatures, combined them, and verify the combined schnorr sig against the combined pubkey.

But if this is the case, it means you already called process_nonces before. So you could just add the combined pubkey in the output data structure of process_nonces (the processed_nonces in the second graph).

I see that also the pre_session is used to add tweak. I don't really understand the purpose of it, so maybe it would prevent getting rid of pre_session...

[real-or-random]

I would checking that it does not overflow.

In several place in the code, you actively check if the scalar of the partial signature is overflowing.
I think you can remove those branches by just checking if the scalar is overflowing when signature is getting parsed, such that overflow partial sigs can't even be parsed. (especially that the parse function return a int to indicate parsing result already)

I think the question is whether we need the parse/serialize at all. Our philosophy when writing BIP340 and implementing was to consider signatures simply as fixed-sized byte arrays. Could we do this here? On the other hand, having types makes sure the user can't mess up things so easily, and there are a lot of data structures here.

[NicolasDorier]

@real-or-random I think this is nice to have strong type. Crypto library are already complicated enough for users, that it is better to have strong type so they have better idea on how things are tied up.

But I think it would be even better if the parse function was doing some safety check.
That said, thinking about it, in C there is no way to prevent somebody to pass an uninitialized multi sig to the function, so I may be wrong and the function should still do some check when converting to scalar.

Maybe using some magic value, but I don't know how willing you are to do it, since nothing is really doing it yet. And still then, would need to check it. :(

[real-or-random]

@real-or-random I think this is nice to have strong type. Crypto library are already complicated enough for users, that it is better to have strong type so they have better idea on how things are tied up.

Yep, I think that makes sense.

But I think it would be even better if the parse function was doing some safety check.

But the scalar overflow check is not a safety check. Now we have two places where we have checks, parse and verify. I think it's much simpler to have them all in one place.

On the other hand all our other parse functions do some basic checks.

That said, thinking about it, in C there is no way to prevent somebody to pass an uninitialized multi sig to the function, so I may be wrong and the function should still do some check when converting to scalar.

Maybe using some magic value, but I don't know how willing you are to do it, since nothing is really doing it yet. And still then, would need to check it. :(

Hm, we have magic values in the scratch space API... What types do you have in mind concretely?

[NicolasDorier]

@real-or-random musig_partial_sig. My intention was to prevent the creation of an invalid object early (in the parse) so we don't have to check the overflow in the code later on.

But that's not possible since the user might screw up by passing uninitialized object anyway, so the check in the code is still necessary. The magic would prevent this, but then you'd have to check the magic flag rather than the overflow, which is just moving the problem.

It's still nice to have the check in the parse if some other classes are doing it. Not big deal, if you don't though.

[jonasnick]

In several place in the code, you actively check if the scalar of the partial signature is overflowing.
I think you can remove those branches by just checking if the scalar is overflowing when signature is getting parsed, such that overflow partial sigs can't even be parsed. (especially that the parse function return a int to indicate parsing result already)

@NicolasDorier Ah I see what you mean now. Agree that it's fine either way.

I think the question is whether we need the parse/serialize at all.

@real-or-random The reasoning in the BIP340 implementation is that after creating the signature, you aren't really able to do anything but serializing and verifying it. But with partial_sigs you can do more: verify, combine, adapt and (adaptor-) extract.

The only case I think it is useful, is when you got all the partial signatures, combined them, and verify the combined schnorr sig against the combined pubkey.
But if this is the case, it means you already called process_nonces before. So you could just add the combined pubkey in the output data structure of process_nonces (the processed_nonces in the second graph).

@NicolasDorier There are other scenarios. Perhaps you already computed the aggregate pubkey because you created an output, and only later design to sign? Also, we want to make it easy to compute the aggregate pubkey (if available) to feed it into session_init as defense in depth against session_id reuse.

I see that also the pre_session is used to add tweak. I don't really understand the purpose of it, so maybe it would prevent getting rid of pre_session...

In theory, we could add a tweak argument to process nonces (and then remove musig_pubkey_tweak_add altogether) but it has the downside mentioned above.

[robot-dreams]

ACK 7ad1220 based on 3 latest fixup commits

[robot-dreams]

Rechecked tests_impl.h and test vectors as well, still looks good.

Another public API question: right now secp256k1_musig_adapt takes sig64 as both an input and an output. However in working with the tests/examples, it felt a little bit awkward to keep track of pre_sig vs. sig without making mistakes.

What are your thoughts on changing the API to this:

int secp256k1_musig_adapt(
    const secp256k1_context* ctx,
    unsigned char *sig64,
    const unsigned char *pre_sig64,
    const unsigned char *sec_adaptor32,
    int nonce_parity
)

Feel free to respond to that (as well as the comment in #131 (review) about nonce_process vs. nonce_finalize) in a later PR. I'm only commenting now in case the public API becomes much harder to change after this PR is merged.

[jonasnick]

@robot-dreams

should musig_nonce_process be renamed to musig_nonce_finalize since it's supposed to create a final nonce?

The user may not know that they need to create a finalized nonce and the finalized nonce isn't output of the function. But they want to know what to do with the received nonce (process). Either way is probably fine but I slightly prefer nonce_process.

What are your thoughts on changing the API to this:

I probably went back and forth on this a couple of times and I can't remember my reasoning anymore. Note that in the real world (outside of tests I mean) you don't need to keep track of the pre_sig after it is adapted. I checked the WIP rust bindings and noticed that they clone the pre_sig as well which is awkward indeed. So I changed this to your suggestion.

[robot-dreams]

ACK 73059c8 hopefully final? 🤞

Thanks for considering the API updates! I agree with your reasoning about nonce_process and I think keeping it as-is makes sense.

[robot-dreams]

So much for const 🤣

[real-or-random]

Well yeah, const in a function sig is just a promise not to write through that pointer. And we keep that promise.

On a more serious note, I think now we should document in the API docs that sig and pre_sig may overlap/alias.

[real-or-random]

On a more serious note, I think now we should document in the API docs that sig and pre_sig may overlap/alias.

This part is still unresolved (if you agree).

[jonasnick]

Added your suggested sentence

[real-or-random]

ACK mod nits :))

I think this is ready to squash then.

[real-or-random]

Link to unresolved discussion that may not be shown by stupid GitHub: #131 (comment)

[jonasnick]

Ok, I squashed the fixup commits.

To the people who are already depending on this PR in some way (@NicolasDorier, @GeneFerneau, ...):

• There were no security issues that we discovered and had to fix.
• I've pushed a "half-squashed" version to this branch that makes it easy to see what exactly the API and "spec" changes that affected the static test vectors were.

[real-or-random]

ACK ac1e367

[robot-dreams]

ACK ac1e367

[jonasnick]

Just noticed that I had an API design FAQ in my notes. Posting that here for better discoverability:

• Couldn't we merge the secnonce struct into the session struct?
  • We could, but we don't because the session struct can be serialized in principle. On the other hand, we want to avoid serializing the secnonce.
• Couldn't we we copy the keyagg cache into the session struct to avoid having to deal with both?
  • When signing multiple times with the same key (and therefore the same keyagg) we'd have the same keyagg cache copied in all the sessions which is wasteful.
• Couldn't we remove the tweak function and instead add a tweak argument to keyagg?
  • In that case, if you'd want to use a different key you'd have to reaggregate the keys which is costly.

[LLFourn]

Post-merge ACK.

The implementation was really good (so is the paper!). Thanks. I've done a rust implementation in secp256kfun. I tried to do it without looking at this too much. It's compatible though (passes the signing tests here). Here's a few notes.

One minor difference between the two is when the aggregate nonce is 0 I just return None when trying to start the signing session. Here you set it to G so the session can continue. I felt that it'd be better just to let the caller decide what should happen in this odd case. Having an easy way of continuing is a good idea though.

Just noticed that I had an API design FAQ in my notes. Posting that here for better discoverability:

* Couldn't we merge the secnonce struct into the session struct?
  
  * We could, but we don't because the session struct can be serialized in principle. On the other hand, we want to avoid serializing the secnonce.

I ended up just serializing the secret nonces along with the signing session. In the case where you are serializing and storing the secret nonces anyway, it might as well go along with the session.

But come to think of it maybe this is the wrong headed. Since you don't need the secnonce to verify an easy mistake to make is to keep nonces around unnecessarily after you've already signed. Maybe I need a way of clearing secret data once it's been used...

A minor nit I had with this impl was:

            pk_hash: H::default().tagged(b"KeyAgg list"),
            coeff_hash: H::default().tagged(b"KeyAgg coefficient"),
            nonce_coeff_hash: H::default().tagged(b"MuSig/noncecoef"),

The tag names are a bit inconsistent. Perhaps come up with a proper naming convention? I like <protocol>/<use>.

A nano-nit was that the order of the agg nonces in the hashing to get b changed from the paper to the implementation (in the paper the X goes first).

[jonasnick]

Thanks for the feedback @LLFourn! And great to see more implementations. By the way, did you notice our on-going work on the spec (#157)?

One minor difference between the two is when the aggregate nonce is 0 I just return None when trying to start the signing session.

How does the caller typically proceed in this case? Do you also support signing for aggregate nonce None (= infinity)?

In the case where you are serializing and storing the secret nonces anyway, it might as well go along with the session.

Right, but the converse is not true. You could imagine that a signer immediately signs, throws away the secnonces and keeps the session to be able to verify and aggregate partial sigs.

The tag names are a bit inconsistent. Perhaps come up with a proper naming convention? I like /.

Agree that this is not nice. Would be mostly remedied imo by simply replacing the space with "/".

[LLFourn]

Thanks for the feedback @LLFourn! And great to see more implementations. By the way, did you notice our on-going work on the spec (#157)?

Yes. Is this going to become a full spec (not just key agg). If so I'll move further comments over there!

One minor difference between the two is when the aggregate nonce is 0 I just return None when trying to start the signing session.

How does the caller typically proceed in this case? Do you also support signing for aggregate nonce None (= infinity)?

At the moment you can't proceed. Perhaps a better thing to do is to return a dud session (like you do here) but indicate it to the caller that this happened. In the case of a 2-of-2 it should be obvious whose at fault for example!

In the case where you are serializing and storing the secret nonces anyway, it might as well go along with the session.

Right, but the converse is not true. You could imagine that a signer immediately signs, throws away the secnonces and keeps the session to be able to verify and aggregate partial sigs.

Yeah I think the way I've done it is wrong. I imagine two scenarios:

1. You are the one collecting the signatures in which case you don't sign until you've got all the others (or maybe you don't sign at all until some later condition e.g. lightning commit tx). So here you just want a secret free session.
2. You sign with the session right away, in which case you should clear all secret data once you have your sig.

In any case you never really want the secrets in the session :)

[real-or-random]

Thanks for the review and the comments!

At the moment you can't proceed. Perhaps a better thing to do is to return a dud session (like you do here) but indicate it to the caller that this happened. In the case of a 2-of-2 it should be obvious whose at fault for example!

The reason why we don't abort is that you need the second round to tell who's to blame. (I assume you're aware of this point because you mention the 2-of-2 as an exception to this observation.)

My argument for not indicating the failure early (if you continue the sessions) is that the resulting API is simpler. The cost of indicating the failure is one more return value to check (= branch) for the caller, and I believe the gain is literally zero: A disruptive signer is anyway able to force you to do move on to the second round (and then send a wrong signature, or just nothing). Also, this "soft" failure is difficult to explain to users, and they won't know what to do if the first round fails. (Abort? Not abort? Print a warning?)

The tag names are a bit inconsistent. Perhaps come up with a proper naming convention? I like /.

I like that, too but practically speaking, it's easier to change the nonce function, so maybe " " is better than "/". Who knows who's using the code already. Plus, the MuSig1 code that got replaced also used "KeyAgg list" and "KeyAgg coefficient".

A nano-nit was that the order of the agg nonces in the hashing to get b changed from the paper to the implementation (in the paper the X goes first).

I had complained about this, too. But Jonas convinced me that it makes sense to put the agg nonces first: When you compute this hash, the agg nonces are always determined, whereas the public key may not be determined. We're pretty sure it's okay to use MuSig2 in a multi-key setting where you exchange nonces without knowing for which public key you want to sign. We haven't formalized the security of this variant in the paper because it adds complexity to the EUF-CMA game (= we were too lazy to do it).

[robot-dreams]

We haven't formalized the security of this variant

But intuitively it's OK because b depends on the aggregate public key, right?

(Same reason why it's OK to exchange nonces without knowing the message, and this attack no longer works?)

[real-or-random]

Yes, indeed.

If you want a tight reduction, you'd probably modify our reduction as follows. Instead of receiving a single challenge from OMDL, receive many and sell them to the attacker as pubkeys X_1, ... X_k for some polynomial k in lambda. Then if the attacker forges under one of these pks, say X_i, you extract the DL of X_i. For all X_j with j !=i, you simply give them back by asking the DL oracle. Then you have the DLs of all challenges. Also observe that the reduction doesn't use the aggregate public key ~X when simulating first-round signing queries. (This is not immediately implied by the syntax of multisig schemes as defined in the paper. The honest first-round algorithm cannot possibly make use of ~X as it does not receive it as input. But any reduction knows ~X upfront, so it could make use of it. But ours doesn't.)

hint: You could alternatively exploit the "random self-reducibility" of (OM)DL: Receive a single challenge C from OMDL and convert it to polynomially many random looking pubkeys X_i = C^d_i for random factors d_i. Idea: if the attacker forges under one of these pks, say X_i, you extract the DL of X_i and raise it to 1/d_i to obtain the DL of C. This is a standard method for pure DL proofs. But here you don't need it as the ability to have many target group elements is "built-in" in the stronger OMDL already.

[sanket1729]

Note that the function secp256k1_musig_nonce_process already has computed the secp256k1_ge values for aggnonce, it might be worth passing them directly to the _internal function instead of computing them again.

[real-or-random]

Indeed. The [0] value may have changed though when there's an aggregator. But we could still pass the ge values to _internal. Do you want to open a PR?