Followups to #716 (add musig2 API) #794

apoelstra · 2025-05-15T23:50:42Z

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.

tcharding · 2025-05-16T00:23:52Z

bacfb03 has a bunch of formatting changes in it.

apoelstra · 2025-05-16T00:30:46Z

@tcharding oh, yeah, lemme separate those out into an initial commit.. This crate is supposed to always be formatted but #716 was not.

tcharding

I like all the patch separation. I did not review closely all the unit tests. I'm not keen on the test_ prefix that Claude used.

tcharding · 2025-05-16T00:27:01Z

src/musig.rs

+/// # # [cfg(feature = "std")]
+/// # # [cfg(feature = "rand")] {


In 48d5c8f this syntax is odd, is it meant to eb #![cfg(...)]?

Yeah, something is messed up with my rustfmt setup..

Oh I bet that is why CI is failing, I didn't look though.

No, there shouldn't be a !. But there shouldn't be a space eithr.

CI was failing because some of these doctests don't work when the secp256k1_fuzz "break all the crypto" flag is enabled. The spacing was fine.

Also, this weird spacing originated in #716 and I preserved it because I was copy/pasting lines to add more gates. Fixed now.

tcharding · 2025-05-16T00:34:13Z

Reviewed aa0f303

apoelstra · 2025-05-16T00:39:33Z

Will revisit this tomorrow. cargo +nightly fmt is a no-op for some reason. I'm also unsure where those format changes even originated.

apoelstra · 2025-05-16T00:40:58Z

Ohhh I see. We have ignore = [ "secp256k1-sys" ] in our rustfmt.toml, but with jj I am just running rustfmt directly on all the rs files instead of going through cargo fmt, for stupid reasons ... sigh. Anyway will fix this tomorrow.

And also investigate why CI is complaining about failures with the fuzz cfg set, when there are existing failures with that cfg and CI wasn't complaining..

Claude identified a couple of the FFI bindings to MuSig that were incorrect.

Copy the list of whitelisted lints from BlockstreamResearch/simfony. The "uninlined format args" one was the only one that was currently triggering here.

The compiler doesn't like shared references to static mut variables, because it is worried that we'll share a reference while a mutation happens which is UB. We are only sharing references after using Once to do a one-time initialization, so this is fine, but in recent versions of the compiler there is an encapsulated form of this pattern called `OnceLock` and a variant called `LazyLock`. Neither of these are available on our MSRV so just whitelist the lint.

In rand 0.9 `thread_rng` has been renamed to `rng`. The recent Musig2 PR predates the transition to rand 0.9.

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.

This is just a convenience wrapper around SessionSecretRand::from_rng(&mut rng()). The name `new` disguises the fact that this is an impure function and actually calls a RNG every time it is called. It is also unused, in the docs or in the tests. Finally, clippy complains about it because it feels that a `new` method should always be accompanied by a `Default` impl...which is fair, but the impurity is veen more surprising for `default` than for `new`. Since `rand` renamed `thread_rng` to just `rng` the keystroke-saving value of this method is less. So just delete it.

This feels more natural as `secp.musig_sort_pubkeys` rather than `musig::pubkey_sort.

I decided not to rename the `serialize` functions to `to_byte_array`. Maybe we should do that, but we use the name `serialize` *all over* this library so we should do it in a separate PR that changes everything. Along the way we'll have to decide what to call the methods that produce e.g. a SerializedSignature; this is conceptually a byte array but it's actually not one.

…gate The existing code panics somewhere in ffi.rs trying to do pointer alignment stuff, and is only not UB by accident. Guard this with an explicit panic.

I asked Claude to create an initial set of unit tests, which it did. I then manually cleaned up a lot of its repeated logic (though not all of it, as you can tell) and added a whole bunch more failure cases. For example it did not bother trying to repeat or swap keys/nonces. When it generated the tests, the empty-pubkey-list bug (fixed in the previous commit) was still present. It failed to find it, I think because it was reading the code looking for panics to trigger, and there wasn't one. In future I will try giving it only the API, and try telling it to be more adversarial. We'll see.

apoelstra · 2025-05-16T12:34:51Z

WASM and cross were both failing before this PR.

apoelstra · 2025-05-16T13:37:45Z

cc @Kixunil can you take a look at this?

storopoli · 2025-05-16T15:51:10Z

Cargo.toml

+[lints.clippy]
+# Exclude lints we don't think are valuable.
+large_enum_variant = "allow" # docs say "measure before paying attention to this"; why is it on by default??
+similar_names = "allow" # Too many (subjectively) false positives.
+uninlined_format_args = "allow" # This is a subjective style choice.
+indexing_slicing = "allow" # Too many false positives ... would be cool though
+match_bool = "allow" # Adds extra indentation and LOC.
+match_same_arms = "allow" # Collapses things that are conceptually unrelated to each other.
+must_use_candidate = "allow" # Useful for audit but many false positives.


Can't this lints be whitelisted in the modules as needed rather than globally here.
Also if whitelisting locally (either in modules or specific code) I would advocate to using expect than allow.

These lints are bad and I would rather not use clippy than deal with them. Global whitelisting is the right approach.

Using expect would require I know whether or not they trigger.

storopoli · 2025-05-16T15:51:35Z

secp256k1-sys/Cargo.toml

+large_enum_variant = "allow" # docs say "measure before paying attention to this"; why is it on by default??
+similar_names = "allow" # Too many (subjectively) false positives.
+uninlined_format_args = "allow" # This is a subjective style choice.
+indexing_slicing = "allow" # Too many false positives ... would be cool though
+match_bool = "allow" # Adds extra indentation and LOC.
+match_same_arms = "allow" # Collapses things that are conceptually unrelated to each other.
+must_use_candidate = "allow" # Useful for audit but many false positives.


Can't this lints be whitelisted in the modules as needed rather than globally here.
Also if whitelisting locally (either in modules or specific code) I would advocate to using expect than allow.

apoelstra

On 8a43317 successfully ran local tests

These warnings were repetitive and a bit overwrought. "Will leak your key in two signatures" is clear enough. The text also said some out-of-date things about whether it's possible to reuse a nonce -- you can do it directly now with dangerous_into_bytes, and also you could always do it indirectly by just constructing the same nonce twice by using a bad rng.

apoelstra · 2025-05-26T14:34:53Z

cc @jonasnick are you able to take a look at this PR? It has a gazillion commits but they're all pretty small.

apoelstra

On d611a4f successfully ran local tests

jlest01

ACK d611a4f

Clear improvements and good test cases.

tcharding · 2025-05-29T05:55:38Z

Please ping me if you want me to review @apoelstra.

apoelstra · 2025-05-30T03:30:32Z

@tcharding yeah, that'd be appreciated.

Ademan · 2025-06-01T15:34:45Z

src/musig.rs

    ///
-    ///  We repeat, copying this data structure can result in nonce reuse which will
-    ///  leak the secret signing key.
+    /// Storing and re-creating this structure may leak to nonce reuse, which will leak


~~leak~~ lead to nonce reuse

Will fix in a followup-to-the-followup.

jonasnick

I just had a look at the musig/libsecp-relevant bits. Looks good besides my two comments.

jonasnick · 2025-06-10T12:47:00Z

src/key.rs

+    /// # secp.musig_sort_pubkeys(pubkeys_ref);
+    /// # }
+    /// ```
+    pub fn musig_sort_pubkeys(&self, pubkeys: &mut [&PublicKey]) {


Why not just sort_pubkeys? This function is orthogonal to musig.

I went back and forth on this. It's not quite orthogonal, because there are a few ways it makes sense to sort pubkeys and this is the standard way to do it when using musig.

I wanted the function to be discoverable to musig users who wanted to be sure that they were using the right function.

Yes, there are multiple ways to sort pubkeys, but this is the only way that is implemented in upstream libsecp and it is not part of the musig module. Could also mention this function in the musig docs to make it discoverable. Anyway, this is just a nit.

Ok, nice. I'll rename it and add it to the musig2 docs (in a followup).

jonasnick · 2025-06-10T12:49:21Z

src/musig.rs

+    /// a random number generator, or if that is not available, the output of a
+    /// stable monotonic counter.
+    pub fn assume_unique_per_nonce_gen(inner: [u8; 32]) -> Self {
+        assert_ne!(inner, [0; 32], "session secrets may not be all zero");


Doesn't this introduce secret-depdendent branches?

Hmm, crap, yes.

Can I fix this in a followup-to-the-followup?

jonasnick

ACK d611a4f modulo my comments on the PR (secret dependent branches) and that I only looked at the musig/libsecp-relevant bits.

480370c ci: disable broken WASM and cross tests (Andrew Poelstra) 98bfb48 musig: make zero-check in SessionSecretRand::assume_unique constant time (Andrew Poelstra) d318169 musig: rename `musig_sort_pubkeys` to just `sort_pubkeys` (Andrew Poelstra) fdae0fd typo: leak -> lead (Andrew Poelstra) Pull request description: Addresses review comments from #794, and also disables a couple broken CI jobs. ACKs for top commit: jonasnick: ACK 480370c Tree-SHA512: 33e4849389fcefb9916a1dcb5234497a42aa9759f81b4954cac85002a26f1d763ddc5b5c38c62795be339b84ffdb3544c6b3e7408ef8d95b67289fd697d5f1e1

apoelstra force-pushed the 2025-05_musig2-followups branch 3 times, most recently from eecd9ab to aa0f303 Compare May 16, 2025 00:17

tcharding reviewed May 16, 2025

View reviewed changes

fmt: stop blacklisting secp256k1-sys; just fmt whole crate

f5f90af

apoelstra force-pushed the 2025-05_musig2-followups branch 2 times, most recently from 1de91d1 to e03b54f Compare May 16, 2025 12:24

apoelstra added 13 commits May 16, 2025 12:27

musig: fix a couple FFI bindings

07922fd

Claude identified a couple of the FFI bindings to MuSig that were incorrect.

clippy: whitelist a bunch of lints

7c56bcc

Copy the list of whitelisted lints from BlockstreamResearch/simfony. The "uninlined format args" one was the only one that was currently triggering here.

stop using deprecated thread_rng

4dd861f

In rand 0.9 `thread_rng` has been renamed to `rng`. The recent Musig2 PR predates the transition to rand 0.9.

musig: fix all the doctests

3b0232a

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.

musig: remove outdated doc references to ZeroSession error

00c8c75

musig: add missing Panics sections to docs

6d938d3

key: move pubkey_sort to method on Secp256k1; rename

c492c75

This feels more natural as `secp.musig_sort_pubkeys` rather than `musig::pubkey_sort.

musig: clarify doc comment about aggregate nonce proxy

ebdaec7

musig: explicitly panic when given an empty slice of pubkeys to aggre…

40a8b65

…gate The existing code panics somewhere in ffi.rs trying to do pointer alignment stuff, and is only not UB by accident. Guard this with an explicit panic.

apoelstra force-pushed the 2025-05_musig2-followups branch from e03b54f to 8a43317 Compare May 16, 2025 12:27

apoelstra mentioned this pull request May 16, 2025

Add Musig2 module #716

Merged

storopoli reviewed May 16, 2025

View reviewed changes

apoelstra commented May 16, 2025

View reviewed changes

luckysori mentioned this pull request May 19, 2025

Release version 0.7.0 arkade-os/rust-sdk#67

Open

apoelstra commented May 26, 2025

View reviewed changes

jlest01 approved these changes May 29, 2025

View reviewed changes

Ademan mentioned this pull request Jun 1, 2025

musig::PublicNonce doesn't support serde de/serialization but probably should #796

Closed

Ademan reviewed Jun 1, 2025

View reviewed changes

apoelstra mentioned this pull request Jun 9, 2025

Add musig string and serde support #797

Merged

jonasnick reviewed Jun 10, 2025

View reviewed changes

jonasnick approved these changes Jun 11, 2025

View reviewed changes

apoelstra merged commit e77f33f into rust-bitcoin:master Jun 11, 2025
28 of 30 checks passed

apoelstra mentioned this pull request Jun 11, 2025

More Musig2 followups #798

Merged

apoelstra deleted the 2025-05_musig2-followups branch June 11, 2025 19:18

		/// # # [cfg(feature = "std")]
		/// # # [cfg(feature = "rand")] {

Followups to #716 (add musig2 API) #794

Followups to #716 (add musig2 API) #794

Uh oh!

Conversation

apoelstra commented May 15, 2025

Uh oh!

tcharding commented May 16, 2025

Uh oh!

apoelstra commented May 16, 2025

Uh oh!

tcharding left a comment

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!

tcharding commented May 16, 2025

Uh oh!

apoelstra commented May 16, 2025

Uh oh!

apoelstra commented May 16, 2025

Uh oh!

apoelstra commented May 16, 2025

Uh oh!

apoelstra commented May 16, 2025

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!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented May 26, 2025

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

jlest01 left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented May 29, 2025

Uh oh!

apoelstra commented May 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasnick left a comment

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!

jonasnick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!