Skip to content
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

Add Musig2 module #716

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

jlest01
Copy link
Contributor

@jlest01 jlest01 commented Jul 29, 2024

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.

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

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.

src/musig.rs Outdated
// - Key agg cache is valid
// - extra input is 32 bytes
// This can only happen when the session id is all zeros
Err(MusigNonceGenError::ZeroSession)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

src/musig.rs Outdated Show resolved Hide resolved
src/musig.rs Outdated Show resolved Hide resolved
src/musig.rs Outdated Show resolved Hide resolved
@jlest01 jlest01 force-pushed the musig2-module branch 6 times, most recently from 447a94c to e730b8b Compare July 31, 2024 18:20
@jlest01 jlest01 force-pushed the musig2-module branch 3 times, most recently from a91d293 to 8bbd0d2 Compare August 29, 2024 11:39
@tcharding
Copy link
Member

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

@apoelstra
Copy link
Member

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

@jlest01
Copy link
Contributor Author

jlest01 commented Aug 29, 2024

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
Copy link
Member

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
Copy link
Collaborator

Kixunil commented Aug 30, 2024

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

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

jlest01 commented Aug 31, 2024

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as here: #716 (comment)

MusigSecNonce([0; MUSIG_SECNONCE_LEN])
}

/// Don't use this. Refer to the documentation of wrapper APIs in the crate.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks.

impl_raw_debug!(MusigPubNonce);

impl MusigPubNonce {
pub fn new() -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks also broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as here: #716 (comment)

fn default() -> Self {
Self::new()
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, provide a function that constructs initialized types only.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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);
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification.
Done in 2ea5674

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think they should be changed?

src/musig.rs Outdated
#[derive(Debug, Clone, Copy, Eq, PartialEq, PartialOrd, Ord, Hash)]
pub enum ParseError {
/// Length mismatch
ArgLenMismatch {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We usually name these InvalidLength.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks.

@sanket1729
Copy link
Member

Upstream was released yesterday

@apoelstra
Copy link
Member

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

@jlest01
Copy link
Contributor Author

jlest01 commented Nov 7, 2024

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

Yes, done. Thanks.

@tcharding
Copy link
Member

tcharding commented Nov 11, 2024

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

@jlest01
Copy link
Contributor Author

jlest01 commented Jan 20, 2025

@stevenroose thanks for the review.
Unlike this project, rust-secp256k1-zkp supports Vec, which makes it compatible with the use of &[T].
You can do let pubkey_ptrs = pubkeys.iter().map(|k| k.as_c_ptr()).collect::<Vec<_>>(); there, for example.

As far as I know, there is no way to add &[T] or &[impl Borrow<T>] here because the compiler needs to know the exact size of types at compile time.

If I change to:
pub fn new<C: Verification, P: Borrow<PublicKey>>(secp: &Secp256k1<C>, pubkeys: &[P]) -> Self {,
it causes SIGSEGV (Address boundary error).

If I change to:
pub fn new<C: Verification>(secp: &Secp256k1<C>, pubkeys: &[Borrow<PublicKey>]) -> Self {,
the error is:
the size for values of type 'dyn std::borrow::Borrow<key::PublicKey>' cannot be known at compilation time the trait 'Sized' is not implemented for 'dyn std::borrow::Borrow<key::PublicKey>'

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Not too deep review, I mainly looked at the API. It still needs polishing, I think. Also some of my earlier comments were not addressed.

fn default() -> Self {
Self::new()
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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);
    }
}

src/musig.rs Outdated

/// Cached data related to a key aggregation.
#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct MusigKeyAggCache(ffi::MusigKeyAggCache, XOnlyPublicKey);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer named fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I can change the Musig structs to have named fields, but they will be inconsistent with other structs in the src folder.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't worry about inconsistencies too much if there's a clearly better way to do things. The library is old and some parts were not written perfectly or had to obey old MSRV and nobody had the time to fix it yet. We should improve them eventually but that doesn't mean we should make the new code intentionally worse just to stay consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Named fields added in ddae0f0

#[derive(Copy, Clone, PartialEq, Eq)]
pub struct MusigSecNonce(pub(crate) [c_uchar; MUSIG_SECNONCE_LEN]);
impl_array_newtype!(MusigSecNonce, c_uchar, MUSIG_SECNONCE_LEN);
impl_raw_debug!(MusigSecNonce);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This leaks the nonce. We need to hide it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Done in 2ea5674

src/musig.rs Outdated
/// or [`new_musig_nonce_pair`].
#[allow(missing_copy_implementations)]
#[derive(Debug, Eq, PartialEq)]
pub struct MusigSecRand([u8; 32]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this called MusigSecRand and not SessionId?

Copy link
Member

Choose a reason for hiding this comment

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

If this is the value I'm thinking of, the name "session ID" was leading people to store and reuse this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, OK. Then we should also change the parameter names. However it's unclear to me how it's different from secret nonce other than different purpose. MusigSecRand isn't readable to me. If the thing must not be reused and kept secret and is per-session perhaps SecretSessionNonce would be more readable. (But honestly, I'm not sure if I got the properties right.)

Copy link
Member

Choose a reason for hiding this comment

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

Please file an issue upstream if you want to rename this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@apoelstra I've noticed that it's already named differntly - session_secrand32 and reading the discussion, it doesn't have to be a nonce. So it's already named much better in upstream. However, I'd rather spell out the whole word "secret" ("does 'sec' mean 'second'?") and drop the 32 which is pointless in Rust since we carry that information in the type. If you think I should still suggest this upstream, I will.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with expanding upstream's sec to Secret here. Glad they've added session -- I agree that this is a better name -- but I didn't want to add it here if upstream hadn't.

So I think we're in agreement here now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed MusigSecRand to MusigSessionSecRand in 7a13c8f to match upstream.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given we are already in the musig module, I find the Musig prefix redundant and potentially annoying. I'd rather spend those characters on spelling out the word Secret.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prefix Musig removed from structs and functions in src\musig.rs in 3204792 as below:

. MusigSecRand -> SessionSecretRand
. MusigKeyAggCache -> KeyAggCache
. MusigPartialSignature -> PartialSignature
. MusigSecNonce -> SecretNonce
. MusigPubNonce -> PublicNonce
. MusigAggNonce -> AggregatedNonce
. MusigSession -> Session
. new_musig_nonce_pair(...) -> new_nonce_pair(...)

src/musig.rs Outdated
///
/// Each call to this nonce generation APIs must have a UNIQUE session_id. This must NOT BE
/// REUSED in subsequent calls to nonce generation APIs such as [`MusigKeyAggCache::nonce_gen`]
/// or [`new_musig_nonce_pair`].
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doc is confusing. The struct provides generation API that already guarantees uniqueness (by taking an RNG).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Updated in 499326e.

src/musig.rs Outdated
MusigTweakErr::InvalidTweak => write!(
f,
"Invalid Tweak: This only happens when
tweak is negation of secret key"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the message could be just "the tweak is negation of secret key".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 499326e

src/musig.rs Outdated
pub enum MusigNonceGenError {
/// Supplied a zero session id
ZeroSession,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should have statically typed API that prevents this from happening so the type shouldn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't quite understand this suggestion.
Could you elaborate further?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The ID is generated by RNG which means zero is impossible (but OK to panic there to catch broken RNGs) and all functions that accept it accept the strong type. Thus the errors they return are unreachable, so we should panic instead to catch implementation breakage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification.
Done in 4bf5abd.

src/musig.rs Outdated
/// structure in memory can use the provided API functions for a safe standard
/// workflow.
///
/// Signers that pre-computes and saves these nonces are not yet supported. Users
Copy link
Collaborator

Choose a reason for hiding this comment

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

Grammar. Also it's unclear if it's a limitation of the upstream or the high-level library (looks like the latter in which case I'd very much like to have this).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 499326e

src/musig.rs Outdated
///
/// - ArgLenMismatch: If the [`MusigPubNonce`] is not 132 bytes
/// - MalformedArg: If the [`MusigPubNonce`] is 132 bytes, but out of curve order
pub fn from_slice(data: &[u8]) -> Result<Self, ParseError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we've decided that we want to construct things from arrays rather than slices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 499326e

Also updated the other slices parameters to use array.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to also update the name to from_byte_array, since it's no longer a slice (@apoelstra did we agree on this naming in this crate?)

let sig64 = session.partial_sig_agg(partial_sigs_ref);

assert!(secp.verify_schnorr(&sig64, &msg_bytes, &agg_pk).is_ok());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The way this example is written makes it difficult to understand how one would use the API in a real-world application where the signers are not in a single binary but distributed and need to exchange messages. I suggest restructuring the code such that operations of each signer are separated. Maybe it'd be even better to use threads and channels to simulate message transmission unless it makes the code too complicated (I think it shouldn't since it should be just a few additional lines.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Looking into this.

@jlest01 jlest01 force-pushed the musig2-module branch 3 times, most recently from 0c900f5 to 073e02c Compare January 24, 2025 20:05
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Somewhat deeper review but still not super-deep.

src/musig.rs Outdated
///
/// 32-byte array
pub fn serialize(&self) -> [u8; 32] {
let mut data = [0; 32];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be MaybeUninit as well.

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest that we avoid microoptimizations on a first pass. I don't mind using MaybeUninit but it would be better to add it in a followup PR that could be reviewed independently of the rest of the API and logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 44feb9f

Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally consider MaybeUninit as improving semantics and optimization is just a side effect. (Theoretically return value is even better for semantics but we can't do that in C API.) That being said, I said "could" to mean "not important, would be nice", I would've said "should" or "must" otherwise. :)

src/musig.rs Outdated
///
/// # Errors:
///
/// - MalformedArg: If the signature [`MusigPartialSignature`] is 32 bytes, but out of curve order
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mentioning 32 bytes here is redundant, the type enforces it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 668c296

src/musig.rs Outdated

unsafe {
let pubkeys: &[*const ffi::PublicKey] =
transmute::<&[&PublicKey], &[*const ffi::PublicKey]>(pubkey_ptrs);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This transmute is invalid. You either have to use the as pointer cast or core::slice::from_raw_parts. But even that shouldn't be needed, just cast the thin pointers when passing arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 668c296

src/musig.rs Outdated
let pubkeys: &[*const ffi::PublicKey] =
transmute::<&[&PublicKey], &[*const ffi::PublicKey]>(pubkey_ptrs);

if secp256k1_ec_pubkey_sort(cx, pubkeys.as_ptr(), pubkeys.len()) == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at it again, the sort is wrong. It can be used but doesn't have to and you're forcing it. Thus this code makes it impossible to use the library with implementations in other languages that sort the keys differently.

The sort needs to be removed here.

src/musig.rs Outdated
/// let aggnonce = MusigAggNonce::new(&secp, &[pub_nonce1, pub_nonce2]);
/// # }
/// ```
pub fn new<C: Signing>(secp: &Secp256k1<C>, nonce_ptrs: &[&MusigPubNonce]) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are not pointers in Rust. Just name the argument nones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 668c296

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sort function was removed from KeyAggCache::new(...) in 9b1f7cb

A new commit was added to make the sort function available d5b4e53

Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be interesting to benchmark this against core::sort_unstable* methods to see which is better but I suspect C impl will have advantage at least in non-LTO builds.

src/musig.rs Outdated

unsafe {
let pubnonces: &[*const ffi::MusigPubNonce] =
transmute::<&[&MusigPubNonce], &[*const ffi::MusigPubNonce]>(nonce_ptrs);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Invalid transmute again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 668c296

src/musig.rs Outdated
// Only fails on cryptographically unreachable codes or if the args are invalid.
// None of which can occur in safe rust.
unreachable!("Impossible to construct invalid arguments in safe rust.
Also reaches here if R1 + R2*b == point at infinity, but only occurs with 1/1^128 probability")
Copy link
Collaborator

Choose a reason for hiding this comment

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

2^128

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 668c296

src/musig.rs Outdated
{
// Since the arguments in rust are always session_valid, the only reason
// this will fail if the nonce was reused.
Err(MusigSignError::NonceReuse)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is impossible because we consume the nonce and drop it.

Copy link
Member

Choose a reason for hiding this comment

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

It took me a moment to understand this comment. The trick is that secp256k1_musig_partial_sign can't actually detect "nonce reuse" in general. What it does do is zero out the nonce that is passed to it, and if the user then tries to pass that nonce back in, it'll see all zeroes and error out.

But Kix is correct that this pattern is impossible in the Rust code because of the ownership pattern of these methods. So this error return should be an assertion instead.

If the user manages to reuse a nonce in a different way (by reusing a session randomness when generating nonces, or just abusing unsafe code to duplicate an existing one) then we can't detect this and the user will just be screwed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Very interesting. Done in d743a21

src/musig.rs Outdated
/// assert!(secp.verify_schnorr(&schnorr_sig, &msg, &agg_pk).is_ok())
/// # }
/// ```
pub fn partial_sig_agg(&self, partial_sigs: &[&MusigPartialSignature]) -> schnorr::Signature {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This API may be a bit misleading. As opposed to normal signatures, the result of this function is not guaranteed to be valid (e.g. if one signer produced an invalid partial signature). This could be a footgun in protocols that require signature validity before signing (such as LN requiring commitment transaction to be signed before signing opening transaction)

I suggest returning a new type AggregatedSignature with two methods: assume_valid(self) -> shnorr::Signature and verify(self, aggregate_key: XOnlyPublicKey, message: &[u8]) -> Result<schnorr::Signature, Error>. The consumers can then verify the signature or explicitly acknowledge they don't need to verify it.

Copy link
Member

Choose a reason for hiding this comment

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

I like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 49dd1c1

@jlest01 jlest01 force-pushed the musig2-module branch 7 times, most recently from 690b72f to 2d8b4a3 Compare January 26, 2025 02:48
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Still not deep review, just some problems I noticed when I was digging into the API for other reasons.

#[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_11_ec_pubkey_sort")]
pub fn secp256k1_ec_pubkey_sort(
ctx: *const Context,
pubkeys: *const *const PublicKey,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This signature is invalid. It's supposed to be a mut pointer pointing to a const pointer. IIUC this is unsound.

@apoelstra this is exactly why I advocate for using bindgen. Bugs like this can happen and without it we have to spend time to audit every single function to make sure the signature is correct.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, ok, we should use bindgen. But (at least initially) I would like to commit the generated code and verify it in CI rather than putting it as part of the build process, to minimize the number of dependencies that downstream users need.

We would also need to figure out what to do about secp256k1-sys/src/types.rs. Will bindgen require some sort of tweaking to use these types? Or will we need a post-processing step? Or maybe we should just bump our MSRV to 1.64 so we don't need this file anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed in ff77611

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, it is interesting that Rust does not enforce the const modifier in this case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But (at least initially) I would like to commit the generated code and verify it in CI rather than putting it as part of the build process, to minimize the number of dependencies that downstream users need.

Yes, but maybe use xtask pattern for that? Rather than committing, we would have a subcrate that generates it and the developers would run it when contributing or publishing the crate (so the code would still be on crates.io). The upside is no CI job needed, the downside is that every contributor has to run the xtask even for unrelated contributions.

Rust does not enforce the const modifier in this case.

The compiler has no way of understanding the C code.

Copy link
Member

Choose a reason for hiding this comment

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

Moved bindgen discussion to #775

src/key.rs Outdated
/// # pubkey_sort(&secp, pubkeys_ref);
/// # }
/// ```
pub fn pubkey_sort<C: Verification>(secp: &Secp256k1<C>, pubkeys: &[&PublicKey]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is definitely unsound. The function is mutating the argument but it's not using mut.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 2755c15

src/musig.rs Outdated

/// Musig tweaking related errors.
#[derive(Debug, Clone, Copy, Eq, PartialEq, PartialOrd, Ord, Hash)]
pub enum MusigTweakErr {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given this has only one variant, either it should be a struct or it should have #[non_exhaustive] if we anticipate new errors in the future (personally I don't believe we have a reason to think there will be new ones).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 404a93b

src/musig.rs Outdated
/// # Errors:
///
/// - MalformedArg: If the signature [`PartialSignature`] is out of curve order
pub fn from_slice(data: &[u8; ffi::MUSIG_PART_SIG_LEN]) -> Result<Self, ParseError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To deserialize(...) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I saw it in the comment above.
Updated to from_byte_array() in 3e9d296

src/musig.rs Outdated
#[allow(missing_copy_implementations)]
#[derive(Debug)]
pub struct SecretNonce {
data: ffi::MusigSecNonce
Copy link
Collaborator

Choose a reason for hiding this comment

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

Damn, I was unclear. I only meant to have named fields in structs that have more than one field. This could've been tuple struct. You don't have to change it back if you don't want to but you can if you wish.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. Reverted to the tuple struct those with only one field in 5cf37bd.

src/musig.rs Outdated
/// A Musig partial signature.
#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[repr(transparent)]
pub struct PartialSignature{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 5cf37bd

}

impl AggregatedSignature {
/// Returns the aggregated signature [`schnorr::Signature`] assuming it is valid.
Copy link
Collaborator

@Kixunil Kixunil Jan 26, 2025

Choose a reason for hiding this comment

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

Thanks for doing this! I'd like the doc toe be more in-depth.

/// Returns the aggregated signature [`schnorr::Signature`] assuming it is valid.
///
/// The `partial_sig_agg` function cannot guarantee that the produced signature is valid because participants
/// may send invalid signatures. In some applications this doesn't matter because the invalid message is simply
/// dropped with no consequences. These can simply call this function to obtain the resulting signature. However
/// in applications that require having valid signatures before continuing (e.g. presigned transactions in Bitcoin Lightning Network) this would be exploitable. Such applications MUST verify the resulting signature using the
/// [`verify`](Self::verify) method.
///
/// Note that while an alternative approach of verifying partial signatures is valid, verifying the aggregated
/// signature is more performant. Thus it should be generally better to verify the signature using this function first
/// and fall back to detection of violators if it fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this. Updated in 982e877.

src/musig.rs Outdated
///
/// MuSig differs from regular Schnorr signing in that implementers _must_ take
/// special care to not reuse a nonce. If you cannot provide a `sec_key`, `session_secrand`
/// UNIFORMLY RANDOM AND KEPT SECRET (even from other signers). Refer to libsecp256k1-zkp

Choose a reason for hiding this comment

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

The references to libsecp256k1-zkp and secp256k1-zkp in the comments below seem to be left over from Sanket's original PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. Good catch.
Updated in 4012ed5

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.