Skip to content

Add musig string and serde support #797

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 11, 2025
Merged

Conversation

Ademan
Copy link
Contributor

@Ademan Ademan commented Jun 9, 2025

This addresses #796 to add serialization and deserialization for musig types that make sense to transmit as part of a protocol.

There might be an argument to also include every type with a serialize() but IMHO this change implements serialization for the types that that make the most sense.

The test vectors come from https://github.com/Ademan/rust-secp256k1/tree/musig-serde-vectors because I wasn't sure if/what the licensing implications are of taking the vectors from bip-0327 (which is BSD licensed). However, they could be trivially dropped in if desired (convert to lowercase first).

The actual implementations are heavily "borrowed" from the serialization+deserialization of schnorr::Signature

@Ademan
Copy link
Contributor Author

Ademan commented Jun 9, 2025

It also occurred to me that the serialize() methods make constants like MUSIG_KEYAGG_LEN arguably part of the public interface, so maybe they should be re-exported in the musig module? (probably off-topic for this PR)

@apoelstra
Copy link
Member

Can you rebase on current master and also run cargo +nightly fmt on the result (or on every commit, whatever makes you happy)?

Otherwise this looks great. TIL that nonces and aggnonces are encoded as full uncompressed keys. It's a good idea that wouldn't have occurred to me. Agreed that we ought to expose these constants as part of the public API, but also agreed that it doesn't have to be part of this PR.

@Ademan
Copy link
Contributor Author

Ademan commented Jun 9, 2025

I think I'm already on top of current master, 4d36fef right?

reformatted. Only the commit containing the test changed. I'll probably be out until this evening so if there's more feedback I probably won't be as responsive.

RE: nonces, I always forget the specifics of MuSig2 after a week so I could be misremembering, but aren't those two compressed points?

@apoelstra
Copy link
Member

I think I'm already on top of current master, 4d36fef right?

Oh, yes, you are. There is a missing CI fix but it's part of #794 which is still not merged :/. No problem (or rather, not a problem with this PR).

RE: nonces, I always forget the specifics of MuSig2 after a week so I could be misremembering, but aren't those two compressed points?

Ah, yep! They are two compressed points. Currently the doccomments for these lengths are

/// Serialized length (in bytes) of the aggregated nonce.
/// This is the compact form (typically using a compressed representation) used for
/// transmitting or storing the aggregated nonce.
pub const MUSIG_AGGNONCE_SERIALIZED_LEN: usize = 66;

/// Serialized length (in bytes) of an individual public nonce.
/// This compact serialized form is what gets exchanged between signers.
pub const MUSIG_PUBNONCE_SERIALIZED_LEN: usize = 66;

which IMO is confusing -- it should emphasize that 66 is the correct value (the length of two compressed points in bytes); it is not the length of a single compressed point in hex (also 66 bytes) nor is it the length of a single uncompressed point (65 bytes). The term "compact form" isn't standard.

But ok, that can also wait for a followup PR.

Otherwise this looks great. TIL that nonces and aggnonces are encoded as full uncompressed keys. It's a good idea that wouldn't have occurred to me.

It still would've been a good idea, since it improves CPU efficiency at the cost of off-chain bandwidth, but I guess it didn't occur to anybody :P.

@apoelstra
Copy link
Member

I believe this is good to go. The WASM and cross CI failures are known, and probably we should just disable those jobs. The lint failure is fixed by #794 which I can rebase on top of this.

@apoelstra
Copy link
Member

Sorry, my local CI isn't passing on this (for the same reasons the real CI isn't passing). I'm going to merge #794 first and ask you to rebase.

Ademan added 3 commits June 11, 2025 12:17
ParseError is used across multiple musig types.
Add serialization and deserialization for
`secp256k1::musig::PublicNonce`, `secp256k1::musig::AggregatedNonce`,
and `secp256k1::musig::PartialSignature`.
@Ademan
Copy link
Contributor Author

Ademan commented Jun 11, 2025

NP, rebased and reran cargo +nightly fmt (no changes). I renamed my test in musig.rs to drop the test_ prefix to match the rest, but everything else should be the same.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 1d8b862; successfully ran local tests

@apoelstra apoelstra merged commit 2d6c4db into rust-bitcoin:master Jun 11, 2025
28 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants