Skip to content

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
chain-forgexcr45 added a commit to chain-forgexcr45/rust-secp256k1 that referenced this pull request Sep 28, 2025
…port

1d8b8621f8fc9b15310bf302bf9bc91eff85c82d Add tests for MuSig2 de/serialization (Daniel Roberts)
6100c6d34dba8c15f78cd9a8e9b1f51878f2d329 Add serialization and deserialization for relevant MuSig2 types (Daniel Roberts)
c668f8efc05d808678014d521943f9465cae1b37 Fix `musig::ParseError` docs (Daniel Roberts)

Pull request description:

  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`

ACKs for top commit:
  apoelstra:
    ACK 1d8b8621f8fc9b15310bf302bf9bc91eff85c82d; successfully ran local tests

Tree-SHA512: 85fe69347b73761d1ac3c18a7496d2734d7ca5be9d819e1c0d576bfe183f1be3c1710621beee1df86b1ab78da5437cd798684801d5d1a831806702982a6ff9a3
william2332-limf added a commit to william2332-limf/rust-secp256k1 that referenced this pull request Oct 2, 2025
…port

1d8b8621f8fc9b15310bf302bf9bc91eff85c82d Add tests for MuSig2 de/serialization (Daniel Roberts)
6100c6d34dba8c15f78cd9a8e9b1f51878f2d329 Add serialization and deserialization for relevant MuSig2 types (Daniel Roberts)
c668f8efc05d808678014d521943f9465cae1b37 Fix `musig::ParseError` docs (Daniel Roberts)

Pull request description:

  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`

ACKs for top commit:
  apoelstra:
    ACK 1d8b8621f8fc9b15310bf302bf9bc91eff85c82d; successfully ran local tests

Tree-SHA512: 85fe69347b73761d1ac3c18a7496d2734d7ca5be9d819e1c0d576bfe183f1be3c1710621beee1df86b1ab78da5437cd798684801d5d1a831806702982a6ff9a3
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