Skip to content

Followups to #716 (add musig2 API) #794

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.

Sign up for GitHub

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 15 commits into from
Jun 11, 2025
Merged

Followups to #716 (add musig2 API) #794

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f5f90af
fmt: stop blacklisting secp256k1-sys; just fmt whole crate
apoelstra May 16, 2025
07922fd
musig: fix a couple FFI bindings
apoelstra May 15, 2025
7c56bcc
clippy: whitelist a bunch of lints
apoelstra May 15, 2025
9615ec8
context: whitelist new compiler warning
apoelstra May 15, 2025
4dd861f
stop using deprecated thread_rng
apoelstra May 15, 2025
3b0232a
musig: fix all the doctests
apoelstra May 15, 2025
00c8c75
musig: remove outdated doc references to ZeroSession error
apoelstra May 15, 2025
6d938d3
musig: add missing Panics sections to docs
apoelstra May 15, 2025
ec66003
musig: remove SessionSecretRand::new constructor
apoelstra May 15, 2025
c492c75
key: move pubkey_sort to method on Secp256k1; rename
apoelstra May 15, 2025
dc04575
musig: a couple small improvements of byte array APIs
apoelstra May 15, 2025
ebdaec7
musig: clarify doc comment about aggregate nonce proxy
apoelstra May 15, 2025
40a8b65
musig: explicitly panic when given an empty slice of pubkeys to aggre…
apoelstra May 15, 2025
8a43317
musig: add a bunch of unit tests
apoelstra May 15, 2025
d611a4f
musig: weaken/simplify warnings about nonce reuse
apoelstra May 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions secp256k1-sys/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,7 +843,7 @@ extern "C" {
cx: *const Context,
secnonce: *mut MusigSecNonce,
pubnonce: *mut MusigPubNonce,
session_secrand32: *const c_uchar,
session_secrand32: *mut c_uchar,
seckey: *const c_uchar,
pubkey: *const PublicKey,
msg32: *const c_uchar,
Expand Down Expand Up @@ -915,7 +915,7 @@ extern "C" {
#[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: *mut *const PublicKey,
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 looks wrong. How can it sort keys without mutating them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh shit, you're right. I'm not sure what I was thinking here. (Well, probably I saw const rustsecp256k1_v0_11_pubkey **pubkeys and forgot that in C notation, an extra * on the other end of the line will negate const).

No worries, we haven't published this yet, so we can revert this particular change.

Copy link
Member Author

@apoelstra apoelstra Jun 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yikes, this unsoundness goes pretty deep.

In #627 we added an impl of CPtr for &[u8] which implements an as_mut_c_ptr function, which by itself is not unsound, but pretty-much any use of it will be unsound..

This is how the original code (before I changed *mut to *const erroneously) compiled and passed CI. It was using this function to get a *mut pointer from a &[u8].

This made it into 3 releases (secp-sys 0.9, 0.10 and 0.11). You can see it here: https://docs.rs/secp256k1-sys/latest/src/secp256k1_sys/lib.rs.html#1054-1072

Oops.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, untangled the threads. #816

I believe all released versions of our library are technically sound, though we expose a <&[T]>::as_mut_c_ptr() method which invites UB upon anybody who calls it.

n_pubkeys: size_t,
) -> c_int;
}
Expand Down
15 changes: 13 additions & 2 deletions src/musig.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ impl SessionSecretRand {

/// Obtains a reference to the inner bytes of the [`SessionSecretRand`].
pub fn as_bytes(&self) -> &[u8; 32] { &self.0 }

/// Obtains a mutable raw pointer to the beginning of the underlying storage.
///
/// This is a low-level function and not exposed in the public API.
fn as_mut_ptr(&mut self) -> *mut u8 { self.0.as_mut_ptr() }
}

/// Cached data related to a key aggregation.
Expand Down Expand Up @@ -154,7 +159,7 @@ impl fmt::Display for InvalidTweakErr {
/// ```
pub fn new_nonce_pair<C: Signing>(
secp: &Secp256k1<C>,
session_secrand: SessionSecretRand,
mut session_secrand: SessionSecretRand,
key_agg_cache: Option<&KeyAggCache>,
sec_key: Option<SecretKey>,
pub_key: PublicKey,
Expand All @@ -167,13 +172,19 @@ pub fn new_nonce_pair<C: Signing>(
let msg_ptr = msg.as_ref().map(|e| e.as_c_ptr()).unwrap_or(core::ptr::null());
let cache_ptr = key_agg_cache.map(|e| e.as_ptr()).unwrap_or(core::ptr::null());
unsafe {
// The use of a mutable pointer to `session_secrand`, which is a local variable,
// may seem concerning/wrong. It is ok: this pointer is only mutable because the
// behavior of `secp256k1_musig_nonce_gen` on error is to zero out the secret
// nonce. We guarantee this won't happen, but also if it does, it's harmless
// to zero out a local variable without propagating that change back to the
// caller or anything.
let mut sec_nonce = MaybeUninit::<ffi::MusigSecNonce>::uninit();
let mut pub_nonce = MaybeUninit::<ffi::MusigPubNonce>::uninit();
if ffi::secp256k1_musig_nonce_gen(
cx,
sec_nonce.as_mut_ptr(),
pub_nonce.as_mut_ptr(),
session_secrand.as_bytes().as_ptr(),
session_secrand.as_mut_ptr(),
sk_ptr,
pub_key.as_c_ptr(),
msg_ptr,
Expand Down