-
Notifications
You must be signed in to change notification settings - Fork 290
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
Conversation
It also occurred to me that the |
Can you rebase on current 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. |
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? |
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).
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.
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. |
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. |
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. |
ParseError is used across multiple musig types.
Add serialization and deserialization for `secp256k1::musig::PublicNonce`, `secp256k1::musig::AggregatedNonce`, and `secp256k1::musig::PartialSignature`.
NP, rebased and reran |
There was a problem hiding this 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
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