Add module "musig" that implements MuSig2 multi-signatures (BIP 327)

[jonasnick]

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 Rework or get rid of scratch space  #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/.

[jonasnick]

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.

[real-or-random]

needs rebase :)

[t-bast]

FWIW, we have JVM bindings on top of this branch in ACINQ/secp256k1-kmp#93 and an implementation of swap-in-potentiam (musig2 key-path with alternative delayed script path) in ACINQ/bitcoin-kmp#107 and everything is working fine, and the API is easy enough to use!

[jonasnick]

Rebased.

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

[siv2r]

Attaching a visualization for the API flow.

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

[siv2r]

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]

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]

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]

Added sorting and a comment.

[josibake]

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.

[josibake]

in 26dde29 ("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]

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.

[theStack]

Light code-review ACK 398051c

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.

[theStack]

any reason why storing the result into ret and calling the _CHECKMEM_DEFINE macro isn't needed/done here?

[jonasnick]

The return value doesn't depend on secret data.

[theStack]

Light re-ACK 5dccc7b

[josibake]

ACK 5dccc7b

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.

[josibake]

in 2288e88:

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]

Ok, I should have replaced all the singulars with plurals.

[t-bast]

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 👍

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

[theStack]

this should be moved up to the "Unreleased" section now

[jonasnick]

Oh, done!

[theStack]

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

Suggested change

#include "../../field.h"

[jonasnick]

done

[theStack]

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

Suggested change

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

[jonasnick]

Yes, done.

[theStack]

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

Suggested change

	* Returns: 0 if the arguments are invalid or if some signer sent invalid
	* pubnonces, 1 otherwise
	* Returns: 0 if the arguments are invalid, 1 otherwise

[jonasnick]

Right, forgot about that. We can even make the type of nonce_process_internal void.

[theStack]

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

Suggested change

secp256k1_gej_set_infinity(&fin_nonce_ptj);

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: theStack@419c05c
Not sure if its worth it to pick this up, but if yes, it probably needs a better name for the helper function 😅

[jonasnick]

Yes, done. Also added your helper function.

[Kixunil]

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]

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]

Someone coming from Rust and being paranoid about everything. :D Thanks for clarifying, though I think a documentation comment would still be nice.

[Kixunil]

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!

[real-or-random]

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...

[Kixunil]

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?

[jonasnick]

@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.

[Kixunil]

@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?

[Kixunil]

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]

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]

That makes sense. Might be nice to change all of them at some point unless there's a problem I'm missing.

[sipa]

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

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

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]

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

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

And it generated this code:

extern "C" {
    pub fn foo(arg: *const ::std::os::raw::c_uchar);
}
extern "C" {
    pub fn bar(arg: *const ::std::os::raw::c_uchar);
}
extern "C" {
    pub fn baz(arg: *const [::std::os::raw::c_uchar; 32usize]);
}

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]

Interesting!

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

[Kixunil]

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.)

[sipa]

Mostly looked through the interface and documentation.

[sipa]

Nit: the public nonces.

[jonasnick]

fixed

[sipa]

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]

Agree, improved the wording.

[sipa]

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]

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]

Well they can just store the raw bytes on disk, which works, but risks platform-dependency issues.

[jonasnick]

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]

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]

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?

[sipa]

If provided, what happens if the seckey does not match the pubkey argument? (also applies to secp256k1_musig_nonce_gen_counter).

[jonasnick]

The musig_nonce_gen functions do not check if the seckey matches the given pubkey.

[sipa]

But will for example the resulting signature be invalid, or does doing this risk leaking private keys?

[jonasnick]

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.

[sipa]

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]

Yeah, done.

[sipa]

If both seckey and pubkey are mandatory here, would it make sense to instead take a secp256k1_keypair as argument?

[jonasnick]

Yeah, makes sense. I implemented that

[sipa]

So one could say it actually corresponds to (1-gacc[v])/2 in the spec?

[jonasnick]

Yes, done.

[sipa]

Maybe call the argument pks_hash32 (following the style of putting the size of array pointers in the name).

[jonasnick]

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.

[theStack]

changelog nit: this line accidentally slipped into the "Unreleased" part now, though it was already in the 0.5.1 release

[jonasnick]

🤮 ... thanks ... should be fixed for real now