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.

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
Changes from 1 commit
Commits
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
12 changes: 12 additions & 0 deletions src/musig.rs
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,10 @@ impl KeyAggCache {
/// let _agg_pk = key_agg_cache.agg_pk();
/// # }
/// ```
///
/// # Panics
///
/// Panics if an empty slice of pubkeys is provided.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It still passes a danging pointer into the C code. Is it a valid operation? I think we ought to assert!(!pubkeys.is_empty()).

Copy link
Member Author

@apoelstra apoelstra Jun 25, 2025

Choose a reason for hiding this comment

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

In a later commit 40a8b65 I add this assertion. Sorry, these changes wound up in a bad order. I think originally they were next to each other and then I moved "docs only" stuff near the start to help reviewers understand where I was going with the PR.

pub fn new<C: Verification>(secp: &Secp256k1<C>, pubkeys: &[&PublicKey]) -> Self {
let cx = secp.ctx().as_ptr();

Expand Down Expand Up @@ -729,6 +733,10 @@ impl AggregatedNonce {
/// let aggnonce = AggregatedNonce::new(&secp, &[&pub_nonce1, &pub_nonce2]);
/// # }
/// ```
/// # Panics
///
/// Panics if an empty slice of nonces is provided.
///
pub fn new<C: Signing>(secp: &Secp256k1<C>, nonces: &[&PublicNonce]) -> Self {
if nonces.is_empty() {
panic!("Cannot aggregate an empty slice of nonces");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I swear there is a clippy lint to rewrite this to assert, weird that it didn't fire. (Of course, this is unrelated.)

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, it's manual_assert and it's allow by default. We turn it on in our giant bundle of clippy lints (that we've done on hex-conservative and a few other places but not here). We'll get to this whenever we bring those lints over here.

Expand Down Expand Up @@ -1140,6 +1148,10 @@ impl Session {
/// assert!(aggregated_signature.verify(&secp, &agg_pk, &msg_bytes).is_ok());
/// # }
/// ```
///
/// # Panics
///
/// Panics if an empty slice of partial signatures is provided.
pub fn partial_sig_agg(&self, partial_sigs: &[&PartialSignature]) -> AggregatedSignature {
if partial_sigs.is_empty() {
panic!("Cannot aggregate an empty slice of partial signatures");
Expand Down