Add Musig2 module #716

jlest01 · 2024-07-29T00:41:46Z

This PR adds a musig module based on bitcoin-core/secp256k1#1479.
The structure is based on sanket1729's BlockstreamResearch/rust-secp256k1-zkp#48, but I removed the code related to adaptor signatures.

There is an example file in examples/musig.rs and can be run with cargo run --example musig --features "rand std".
The ffi functions were added to secp256k1-sys/src/lib.rs and the API level functions to the new src/musig.rs file.

Kixunil

Awesome!!! I would wait until the upstream PR merges (and releases) before merging this but I'm looking forward to it. I gave it a quick look anyway.

Kixunil · 2024-07-29T06:36:27Z

src/musig.rs

+            // - Key agg cache is valid
+            // - extra input is 32 bytes
+            // This can only happen when the session id is all zeros
+            Err(MusigNonceGenError::ZeroSession)


IMO this should be just panic. It can only happen if someone passes wrong value to dangerous ID creation function.

src/musig.rs

tcharding · 2024-08-29T19:46:08Z

This is a 10 thousand line diff, is something commited that shouldn't be?

apoelstra · 2024-08-29T21:22:20Z

It updates the vendored library to bring in the upstream MuSig PR.

jlest01 · 2024-08-29T22:12:56Z

It updates the vendored library to bring in the upstream MuSig PR.

Yes. For now, only the last three commits matter for review purposes.
The others will be discarded when the upstream MuSig PR is merged.

tcharding · 2024-08-29T22:59:56Z

Cool, thanks. To clarify this is going to wait till upstream merges before being considered for merge, right? What sort of review are you chasing?

Kixunil · 2024-08-30T04:38:36Z

@tcharding I will definitely not ack this until it's upstream is released. However I appreciate the experiment/demo.

jlest01 · 2024-08-31T14:22:15Z

To clarify this is going to wait till upstream merges before being considered for merge, right? What sort of review are you chasing?

Yes, the idea is to wait for the upstream PR to be merged.
Regarding the review, I mean that the last three commits are the ones that are intended to be merged.

Kixunil · 2024-08-31T18:11:50Z

secp256k1-sys/src/lib.rs

+impl MusigSecNonce {
+    pub fn new() -> Self {
+        MusigSecNonce([0; MUSIG_SECNONCE_LEN])
+    }


Isn't this highly misleading? If it's all-zeros it's not a nonce and thus broken. Where would one need it?

Same as here: #716 (comment)

This seems like a huge footgun.

It's also not present in the current version of the PR.

Kixunil · 2024-08-31T18:12:45Z

secp256k1-sys/src/lib.rs

+        MusigSecNonce([0; MUSIG_SECNONCE_LEN])
+    }
+
+    /// Don't use this. Refer to the documentation of wrapper APIs in the crate.


The documentation of these methods is intended for the higher-level API implementors not for for end consumers so it should rather properly describe what's going on here.

Done. Thanks.

Kixunil · 2024-08-31T18:13:07Z

secp256k1-sys/src/lib.rs

+impl_raw_debug!(MusigPubNonce);
+
+impl MusigPubNonce {
+    pub fn new() -> Self {


Looks also broken.

Same as here: #716 (comment)

Kixunil · 2024-08-31T18:14:24Z

secp256k1-sys/src/lib.rs

+    fn default() -> Self {
+        Self::new()
+    }
+}


It looks to me that none of these Defaults should exist. People should just use arrays or MaybeUninit<T> to represent the uninitialized state.

Are you suggesting something like this ?

let key_agg_cache = MaybeUninit::<ffi::MusigKeyAggCache>::uninit(); let mut key_agg_cache = key_agg_cache.assume_init();

This will cause UB (without MaybeUninit::write).
The reason for pub fn new() is that the internal array is private (ex: pub struct MusigKeyAggCache([c_uchar; MUSIG_KEYAGG_LEN]);), which is consistent with the other structs in the code.

No, provide a function that constructs initialized types only.

Oh, now I see that I was confused because these are the FFI structs. However, I still maintain they are highly confusing.

The correct usage (inside secp256k1::musig::MusigKeyAggCache::new) is this:

let mut key_agg_cache = MaybeUninit::<ffi::MusigKeyAggCache>::uninit(); let mut agg_pk = MaybeUninit::<ffi::XOnlyPublicKey>::uninit(); unsafe { if ffi::secp256k1_musig_pubkey_agg( cx, agg_pk.as_mut_ptr(), key_agg_cache.as_mut_ptr(), pubkeys.as_ptr(), pubkey_ptrs.len(), ) == 0 { panic!(...); } else { // secp256k1_musig_pubkey_agg overwrites the cache and the key so this is sound. let key_agg_cache = key_agg_cache.assume_init(); let agg_pk = agg_pk.assume_init(); MusigKeyAggCache(key_agg_cache, pk); } }

Thanks for the clarification.
Done in 2ea5674

I've also applied the same approach to the other structs.

Kixunil · 2024-08-31T18:28:09Z

secp256k1-sys/src/lib.rs

+
+#[repr(C)]
+#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct MusigPartialSignature([c_uchar; MUSIG_PART_SIG_LEN]);


FTR these struct declarations looked wrong but are indeed correct based on the current API.

Do you think they should be changed?

Kixunil · 2024-08-31T18:31:24Z

src/musig.rs

+#[derive(Debug, Clone, Copy, Eq, PartialEq, PartialOrd, Ord, Hash)]
+pub enum ParseError {
+    /// Length mismatch
+    ArgLenMismatch {


We usually name these InvalidLength.

Done. Thanks.

sanket1729 · 2024-11-06T00:45:51Z

Upstream was released yesterday

apoelstra · 2024-11-06T15:07:51Z

Can you rebase and format each commit with the nightly formatter? That should fix CI.

jlest01 · 2024-11-07T16:57:14Z

Can you rebase and format each commit with the nightly formatter? That should fix CI.

Yes, done. Thanks.

tcharding · 2024-11-11T03:58:19Z

Patch 1 can be removed now, right? Then your shellcheck CI fail should disappear.

apoelstra · 2025-05-14T19:36:26Z

src/musig.rs

+///
+/// # Errors:
+///
+/// * `ZeroSession`: if the `session_secrand` is supplied is all zeros.


In dd26c30:

(Will fix in followup) there is no such error anymore. (Same below.)

apoelstra · 2025-05-14T19:40:23Z

src/musig.rs

+    ///
+    /// In rand-std environment, [`SessionSecretRand::new`] can be used to generate a random
+    /// session id using thread rng.
+    pub fn assume_unique_per_nonce_gen(inner: [u8; 32]) -> Self { SessionSecretRand(inner) }


In dd26c30:

I think you should panic here if the user provides all 0s. Will probably be easier to diagnose than them getting a panic later when signing. (I wouldn't bother returning a Result or something ... this function is supposed to be passed the result of a cryptographic hash or RNG or something.) (Will do in followup.)

Probably OK to do it later but this kind of thing tends to come up in cryptography more often so maybe we should have a newtype for it.

Hard to say. This function is pretty-explicitly "I got these bytes from somewhere and they're legit, promise" so it feels both paternalistic and insufficient to provide a newtype that does a single validity check on it.

Since the upstream library will do bad things if you provide all zeros, I think we should catch that, but I think that immediately panicking is probably the least bad thing to do.

I mean we could just rename the function to be less scary and make the constructor of the newtype scary with the purpose that various RNG libraries can construct it. But maybe it's not worth it here.

I think in this case we should just keep it as-is. We sorta tried this with the ThirtyTwoByteHash trait and it wound up just causing a lot of unnecessary and frustrating coupling between libraries, and also the "per-nonce" uniqueness constraint here is stronger than what a rng library can necessarily produce.

apoelstra · 2025-05-14T19:44:52Z

src/musig.rs

+    /// # Returns
+    ///
+    /// 32-byte array
+    pub fn serialize(&self) -> [u8; 32] {


In dd26c30:

+1 to using the constant rather than just 32. Also I would like to rename this to to_byte_array for symmetry. We should do this across the codebase, but for now let's just do it with new code. (Will do in followup.)

Same rename for PublicNonce.

apoelstra · 2025-05-14T19:50:49Z

src/musig.rs

+    /// let aggnonce = AggregatedNonce::new(&secp, &[pub_nonce1, pub_nonce2]);
+    /// # }
+    /// ```
+    pub fn new<C: Signing>(secp: &Secp256k1<C>, nonces: &[&PublicNonce]) -> Self {


In dd26c30:

Needs a Panics section of the docs (will do in followup).

apoelstra · 2025-05-14T19:52:28Z

src/musig.rs

+    /// This is useful to reduce the communication between signers, because instead
+    /// of everyone sending nonces to everyone else, there can be one party
+    /// receiving all nonces, combining the nonces with this function and then
+    /// sending only the combined nonce back to the signers.


In dd26c30:

In the upstream docs there's an extra line clarifying that if the aggregator lies, the signature will just be invalid. We should copy that here. (Will do in followup.)

apoelstra · 2025-05-14T21:23:04Z

src/musig.rs

+    /// let _agg_pk = key_agg_cache.agg_pk();
+    /// # }
+    /// ```
+    pub fn new<C: Verification>(secp: &Secp256k1<C>, pubkeys: &[&PublicKey]) -> Self {


In dd26c30:

What if you pass an empty array of pubkeys? I'll play with this. Probably I'll just add an explict panic, though given that we have the same issue with partial sigs, I'm tempted to add an error variant for this, or maybe a nonemptyarray newtype. Will address in followup, at least to add a panic (I suspect the existing code will do weird potentially-UB things) but I might wind up just filing an issue.

For your curiosity, what happens is that we try to pass a bad pointer to the C code, but our alignment trickery in secp256k1-sys catches it and panics (though I doubt the panic is very reliable). So on my system it's not actually UB, but it's pretty close.

apoelstra · 2025-05-14T23:06:43Z

src/musig.rs

+    /// Generates a new session ID using thread RNG.
+    #[cfg(all(feature = "rand", feature = "std"))]
+    pub fn new() -> Self {
+        Self::from_rng(&mut rand::thread_rng())


In dd26c30:

Clippy is complaining about there being new but not Default. I think it's got a point, and that we should rename this function from_thread_rng to make it more obvious that it's not a pure function. Will do in a followup.

I believe your comments have been addressed, and your review is blocking merge. I went through them all even though Github has literally hidden them and marked every one as "outdated". If I'm wrong I'd appreciate a heads up and for you to re-open your comments, maybe on my followup PR.

apoelstra

ACK c6fc3d7; successfully ran local tests

apoelstra · 2025-05-15T01:44:51Z

Will open a followup PR tomorrow.

Pretty-much all of the doctests included in rust-bitcoin#716 corresponded to an old version of the API. I'm unsure why it is that my local CI accepted this. The Github CI did not.

apoelstra · 2025-05-16T15:21:39Z

#794

dpc · 2025-05-26T05:56:00Z

src/musig.rs

+/// structure in memory can use the provided API functions for a safe standard
+/// workflow.
+///
+/// Signers that pre-compute and save these nonces are not yet supported. Users


Stale? They can just call dangerous_into_bytes and dangerous_from_bytes.

dpc · 2025-05-26T06:04:14Z

src/musig.rs

+    /// Function to return a copy of the internal array. See WARNING before using this function.
+    ///
+    /// # Warning:
+    ///  This structure MUST NOT be copied or read or written to directly. A


I find this wording too paranoid and a lie(?). Not implementing Copy and Clone, seems reasonable, as it will lead the user to correct usage and reading the docs, great. But once they are using something named dangerous_* it's clear that they should pay attention.

At that point seems unnecessary to tell them scary lies about MUST NOT be copied? They must not reuse the nonce, that's all, right? They can copy it a hundred times, save it, restore it, etc., as long as they not re-use it. So they have been warned and I think misinformation will just potentially confuse them.

Maybe "The nonce must not be reused, so to minimize the chance of it happening by accident this type doesn't implement Copy or Clone."

Yeah, I'm happy to tone this down (as well as the "not supported" language, which if interpreted as "you can't do it with the library" is false and if interpreted as "we won't help you" is meaningless because the software is provided as-is with no warranty).

Updated #794. If you have further comments about the text please comment there.

Kixunil · 2025-05-26T10:05:35Z

@apoelstra yeah, IIRC I've reviewed all updates that they address my comments in the past. Sorry for not marking it clearly. (I've been traveling also, so was unable to do anything past week.)

…ICIPANT_PUBKEYS 2481695 Add tests for BIP-373 PSBT_{IN,OUT}_MUSIG2_PARTICIPANT_PUBKEYS serialization and deserialization (Daniel Roberts) 3e8e6d9 Add BIP-373 PSBT_{IN,OUT}_MUSIG2_PARTICIPANT_PUBKEYS serialization and deserialization (Daniel Roberts) Pull request description: This change adds support for serializing and deserializing two PSBT keys from BIP-373: `PSBT_IN_MUSIG2_PARTICIPANT_PUBKEYS` and `PSBT_OUT_MUSIG2_PARTICIPANT_PUBKEYS` This is a part of #4207 that can be implemented independently of the rest (which depends on rust-bitcoin/rust-secp256k1#716). I believe this satisfactorily avoids changing things multiple times on end users, *however* it's not *completely* transparent to end users, since any code that currently accesses these fields through `unknown` will need to be updated. Later, when `PSBT_IN_MUSIG2_PUB_NONCE` and `PSBT_IN_MUSIG2_PARTIAL_SIG` are supported, code will need to be updated a second time to retrieve them from the correct place instead of `unknown`. I'm of the opinion that this imposes a very minor maintenance burden, only consisting of *removing* deserialization code. ### Notes/Requests for feedback - For the most part I used my judgement rather than `cargo fmt` since `cargo fmt` already had a lot of other complaints, but of course I'll update if I need to. - To satisfy the requirement that every commit pass tests, the commit updating the psbt serde regression test should probably be squashed into the first commit, but I just wanted to confirm that before I did it. I suppose similarly, the test commit could be squashed as well? - I waffled between `musig2_participants` and `musig2_participant_pubkeys`, but I've decided to go with `musig2_participant_pubkeys` because that is consistent with Bitcoin Core ACKs for top commit: tcharding: ACK 2481695 apoelstra: ACK 2481695; successfully ran local tests Tree-SHA512: af884923593c9cbb24ff3f1f08219458538592fabde85d5d65bc2d9bc7bf0b1a73dac38d2c56303b4f3162088db129ea7e879c3d4b324e965933c121ef939a07

d611a4f musig: weaken/simplify warnings about nonce reuse (Andrew Poelstra) 8a43317 musig: add a bunch of unit tests (Andrew Poelstra) 40a8b65 musig: explicitly panic when given an empty slice of pubkeys to aggregate (Andrew Poelstra) ebdaec7 musig: clarify doc comment about aggregate nonce proxy (Andrew Poelstra) dc04575 musig: a couple small improvements of byte array APIs (Andrew Poelstra) c492c75 key: move pubkey_sort to method on Secp256k1; rename (Andrew Poelstra) ec66003 musig: remove SessionSecretRand::new constructor (Andrew Poelstra) 6d938d3 musig: add missing Panics sections to docs (Andrew Poelstra) 00c8c75 musig: remove outdated doc references to ZeroSession error (Andrew Poelstra) 3b0232a musig: fix all the doctests (Andrew Poelstra) 4dd861f stop using deprecated thread_rng (Andrew Poelstra) 9615ec8 context: whitelist new compiler warning (Andrew Poelstra) 7c56bcc clippy: whitelist a bunch of lints (Andrew Poelstra) 07922fd musig: fix a couple FFI bindings (Andrew Poelstra) f5f90af fmt: stop blacklisting secp256k1-sys; just fmt whole crate (Andrew Poelstra) Pull request description: This PR needs to be merged before the next release because the existing code has one instance of UB when passing an empty array to the aggregate nonce function. (Ok, there's a rust panic in our alignment code so maybe no bad pointers make it across the C boundary and we're ok. But it's near-UB.) This PR is the first one I created using jujutsu. One thing I notice is that the tool encourages you to produce way more commits than you would with git. Most of these are small. Let me know if you want me to squash any. ACKs for top commit: jlest01: ACK d611a4f jonasnick: ACK d611a4f modulo my comments on the PR (secret dependent branches) and that I only looked at the musig/libsecp-relevant bits. Tree-SHA512: a504912639bcb6296bd6fdf7a0533464ce9e9064d1c2bf06bf142b7749b4aaf75ed4bae10f9912b92191c64a453bcf56bbd001e3c99ab383e02de4676c7c6a69

Ademan · 2025-06-16T06:23:30Z

secp256k1-sys/src/lib.rs

+/// Serialized length (in bytes) of a partial signature.
+/// This is the compact form (typically just the 32-byte scalar) that is used when communicating
+/// partial signatures to be combined into the final signature.
+pub const MUSIG_PART_SIG_SERIALIZED_LEN: usize = 32;


These musig constants are ~the only ones in rust-libsecp256k1 to use a _LEN suffix (as opposed to _SIZE) is there a reason for that? I'm working on a PR that touches them, and I think it makes sense to rename them to _SIZE before the next release if that wasn't an intentional decision.

Not intentional. Please rename them!

Kixunil reviewed Jul 29, 2024

View reviewed changes

jlest01 force-pushed the musig2-module branch 6 times, most recently from 447a94c to e730b8b Compare July 31, 2024 18:20

jlest01 mentioned this pull request Aug 11, 2024

[WIP] Add a Musig2 example file rust-bitcoin/rust-bitcoin#3158

Open

jlest01 force-pushed the musig2-module branch 3 times, most recently from a91d293 to 8bbd0d2 Compare August 29, 2024 11:39

jlest01 force-pushed the musig2-module branch from 8bbd0d2 to c313387 Compare August 30, 2024 01:40

jlest01 force-pushed the musig2-module branch 3 times, most recently from 0a2361b to 86e2b28 Compare August 30, 2024 22:13

Kixunil reviewed Aug 31, 2024

View reviewed changes

jlest01 force-pushed the musig2-module branch from 86e2b28 to 071ac15 Compare September 17, 2024 02:01

Ademan mentioned this pull request Oct 7, 2024

Musig2 support BlockstreamResearch/rust-secp256k1-zkp#48

Closed

ekrembal mentioned this pull request Oct 14, 2024

Use musig2 from libsecp256k1 chainwayxyz/clementine#339

Closed

jlest01 force-pushed the musig2-module branch from 071ac15 to 7f76102 Compare November 7, 2024 16:55

apoelstra reviewed May 14, 2025

View reviewed changes

run cargo fmt

c6fc3d7

apoelstra approved these changes May 15, 2025

View reviewed changes

apoelstra merged commit 4d36fef into rust-bitcoin:master May 15, 2025
27 of 30 checks passed

lorenzolfm mentioned this pull request May 15, 2025

Topicos Maio/25 saopaulobitdevs/saopaulobitdevs.org#49

Closed

luisschwab mentioned this pull request May 15, 2025

Maio 2025 BitDevsBSB/BitDevsBSB#20

Closed

apoelstra mentioned this pull request May 15, 2025

Release 1.0-rc.0 rust-bitcoin/hex-conservative#172

Closed

apoelstra mentioned this pull request May 16, 2025

Followups to #716 (add musig2 API) #794

Merged

edilmedeiros mentioned this pull request May 20, 2025

Maio/2025 poabitdevs/poabitdevs.org#7

Closed

dpc reviewed May 26, 2025

View reviewed changes

douglaz mentioned this pull request Jun 9, 2025

#3 bitdevsasuncion/bitdevsasuncion.github.io#1

Open

Ademan reviewed Jun 16, 2025

View reviewed changes

Add Musig2 module #716

Add Musig2 module #716

Uh oh!

Conversation

jlest01 commented Jul 29, 2024 • edited by apoelstra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tcharding commented Aug 29, 2024

Uh oh!

apoelstra commented Aug 29, 2024

Uh oh!

jlest01 commented Aug 29, 2024

Uh oh!

tcharding commented Aug 29, 2024

Uh oh!

Kixunil commented Aug 30, 2024

Uh oh!

jlest01 commented Aug 31, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanket1729 commented Nov 6, 2024

Uh oh!

apoelstra commented Nov 6, 2024

Uh oh!

jlest01 commented Nov 7, 2024

Uh oh!

tcharding commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlest01 commented Jul 29, 2024 •

edited by apoelstra

Loading

tcharding commented Nov 11, 2024 •

edited

Loading

dpc May 26, 2025 •

edited

Loading