Musig2 support #48

sanket1729 · 2022-03-21T20:16:46Z

This was taken by #29 , but significantly changed. In particular, I was super strict about adding unreachable! to make the APIs not return a result when possible. Each API has a doc test with guidelines of using it.

Things to pay special attention to:

Cross-check the use of unreachable! in the FFI calls to make sure they cannot panic for any other reasons.
Review the documentation about the usage of session_id and nonce re-use.

Edit: I wanted to add a extra-rand with time of day example, but doing so in the current MSRV is non-trivial. Will add an example after MSRV update

sanket1729 · 2022-04-15T20:03:02Z

Marked as ready for review.

sanket1729 · 2022-04-15T20:06:51Z

~~Resolved~~

jonasnick

I have only read through half the code yet but looks nice so far.

contrib/test.sh

secp256k1-zkp-sys/src/zkp.rs

src/zkp/musig.rs

jonasnick · 2022-04-22T20:07:55Z

src/zkp/musig.rs

+    ///
+    /// MuSig differs from regular Schnorr signing in that implementers _must_ take
+    /// special care to not reuse a nonce. If you cannot provide `session_id`, or `extra_rand`
+    /// UNIFORMLY at random, refer to libsecp256k1-zkp documentation for additional considerations.


Fwiw extra_rand generally does not need to be uniformly random which makes this sound a bit confusing. Also, what is missing is that in the mode that you're recommending here (session_id uniformly random), the session_id must also be kept secret.

Should we also rename extra_rand? the libsecp implement calls it extra_input, the spec draft extra_in.

src/zkp/musig.rs

jonasnick

First review pass done. I wonder if it is worth having so much redundancy in the doc examples versus just having a single example that shows an entire signing session.

src/zkp/musig.rs

jonasnick · 2022-04-25T09:16:33Z

src/zkp/musig.rs

+            ) == 0
+            {
+                // This can only crash if the individual nonces are invalid which is not possible is rust.
+                // Note that even if aggregate nonce is point at infinity, the musig spec sets it as `G`
+                unreachable!("Public key nonces are well-formed and valid in rust typesystem")


nonce_agg will also fail if none_ptrs.len() == 0. The reason is that there's no aggnonce to return in that case.

Good catch. I think it is okay to panic in this case.

Followup: The term "Public key nonces" is confusing, perhaps just "nonces"?

src/zkp/musig.rs

- secp256k1_zkp is missing BlockstreamResearch/rust-secp256k1-zkp#48 - bitcoin_hashes is missing rust-bitcoin/bitcoin_hashes@f1084bf

sanket1729

Sorry for the delay @jonasnick. Addressed the reviews.

contrib/test.sh

src/zkp/musig.rs

sanket1729 · 2022-06-14T01:41:17Z

src/zkp/musig.rs

+            ) == 0
+            {
+                // This can only crash if the individual nonces are invalid which is not possible is rust.
+                // Note that even if aggregate nonce is point at infinity, the musig spec sets it as `G`
+                unreachable!("Public key nonces are well-formed and valid in rust typesystem")


Good catch. I think it is okay to panic in this case.

src/zkp/musig.rs

Tibo-lg · 2022-06-23T02:48:33Z

Thanks for providing this! I played a bit with it and I'm curious about the reason for having separate Error enums. It feels a bit impractical to me as for example if I want to easily convert them to my library error type I need to implement From many times. Or maybe there is a specific reason behind this choice?

I also spotted quite some typos in the docs but not sure if it's the right time to be nitpicking (if it is let me know I can point them out).

Otherwise it's really neat :)!

sanket1729 · 2022-06-24T04:35:28Z

Or maybe there is a specific reason behind this choice?

@Tibo-lg Indeed. IMO, the return type of the function should only return the corresponding error variants that are expected from the function. If we collect all of them into a single enum, it is would impossible for the caller to know which variants are reachable for the corresponding function. This is handy for callers that want to act on an error instead of propagating forward.

match key_agg_cache.pubkey_ec_tweak_add(&secp, tweak) {
	Ok(_) => {},
	Err(MusigTweakError::InvalidTweak) => panic!("not possible in bip32 tweaking"),
}

If there is a single variant with everything, the caller has no choice but to propagate it forward. Unfortunately, there is a tradeoff that the implementors that want to forward the error instead of dealing with it would have more code to deal with. But overall, I think the tradeoff is clear as it clearly expresses the conditions the function errors with and allows sanely dealing with errors.

I also spotted quite some typos in the docs but not sure if it's the right time to be nitpicking (if it is let me know I can point them out).

Corrections welcome.

ssantos21 · 2023-09-05T02:44:18Z

Exposing the inner bytes of ffi::MusigSecNonce and ffi::MusigSession would allow to serialize and store them.

secp256k1-zkp-sys/src/zkp.rs

 #[repr(C)]
 #[derive(Copy, Clone)]
-pub struct MusigSecNonce([c_uchar; MUSIG_SECNONCE_LEN]);
+pub struct MusigSecNonce(pub [c_uchar; MUSIG_SECNONCE_LEN]);
 impl_array_newtype!(MusigSecNonce, c_uchar, MUSIG_SECNONCE_LEN);
 impl_raw_debug!(MusigSecNonce);
 
@@ -1008,7 +1008,7 @@ impl MusigAggNonce {
 
 #[repr(C)]
 #[derive(Copy, Clone)]
-pub struct MusigSession([c_uchar; MUSIG_SESSION_LEN]);
+pub struct MusigSession(pub [c_uchar; MUSIG_SESSION_LEN]);
 impl_array_newtype!(MusigSession, c_uchar, MUSIG_SESSION_LEN);
 impl_raw_debug!(MusigSession);

src/zkp/musig.rs

 use crate::{Signing, Verification};
+use ffi::{MUSIG_SECNONCE_LEN, MUSIG_SESSION_LEN};
 use secp256k1::Parity;

impl MusigSecNonce {
     pub fn as_mut_ptr(&mut self) -> *mut ffi::MusigSecNonce {
         &mut self.0
     }
+
+    /// Function to return a copy of the internal array
+    pub fn serialize(&self) -> [u8; MUSIG_SECNONCE_LEN] {
+        let ffi::MusigSecNonce(array) = &self.0;
+        *array
+    }
+
+    /// Function to create a new MusigKeyAggCoef from a 32 byte array
+    pub fn from_slice(array: [u8; MUSIG_SECNONCE_LEN]) -> Self {
+        MusigSecNonce(ffi::MusigSecNonce(array))
+    }
 }
 
 impl CPtr for MusigSession {
     type Target = ffi::MusigSession;
@@ -1671,6 +1695,17 @@ impl MusigSession {
     pub fn as_mut_ptr(&mut self) -> *mut ffi::MusigSession {
         &mut self.0
     }
+
+    /// Function to return a copy of the internal array
+    pub fn serialize(&self) -> [u8; MUSIG_SESSION_LEN] {
+        let ffi::MusigSession(array) = &self.0;
+        *array
+    }
+
+    /// Function to create a new MusigKeyAggCoef from a 32 byte array
+    pub fn from_slice(array: [u8; MUSIG_SESSION_LEN]) -> Self {
+        MusigSession(ffi::MusigSession(array))
+    }
 }

ssantos21

The functions pubkey_xonly_tweak_add and secp256k1_musig_pubkey_xonly_tweak_add have the wrong parameter (XOnlyPublicKey instead of PublicKey).

This causes an error when verifying a signature of a tweaked key. Changing them to use the expected parameter fixes this.

ssantos21 · 2023-09-10T05:45:32Z

secp256k1-zkp-sys/src/zkp.rs

+    )]
+    pub fn secp256k1_musig_pubkey_xonly_tweak_add(
+        cx: *const Context,
+        output_pubkey: *mut XOnlyPublicKey,


secp256k1_musig_pubkey_xonly_tweak_add expects a PublicKey, not XOnlyPublicKey.
This is causing signature verification to fail.

Suggested change

output_pubkey: *mut XOnlyPublicKey,

output_pubkey: *mut PublicKey,

Good catch.

src/zkp/musig.rs

sanket1729 · 2024-01-07T16:37:08Z

Hello everyone. Sorry for the delay in working on this.

@ssantos21, there was a good security reason to disallow serialization APIs for SecNonce. I have prefix dangerous to those in the name and doc-comment explaining the reason. I am yet sure the use-case of serializing sessions, but we can do that as a follow-up PR.

I suggest we review this PR for correctness, any incomplete serialization features can be added as a followup.

Ready for review. cc @stevenroose @jonasnick @Ademan @ssantos21 @apoelstra

Signed-off-by: Sanket Kanjalkar <[email protected]>

philipr-za · 2024-01-16T17:00:07Z

src/zkp/musig.rs

+    /// Refer to libsecp256k1-zkp documentation for additional considerations.
+    ///
+    /// Musig2 nonces can be precomputed without knowing the aggregate public key, the message to sign.
+    /// See the `new_nonce_pair` method that allows generating [`MusigSecNonce`] and [`MusigPubNonce`]


The new_nonce_pair method doesn't seem to exist?

Additionally, such a method should probably include the the optional extra_rand field as in this case you might well want to use it for extra protection against accidental misuse?

ahh I see it's actually called new_musig_nonce_pair now so just a doc-comment issue then

stevenroose · 2024-01-16T18:52:49Z

Not actively reviewing, but trying to use this.

The new_musig_nonce_pair function has this sentence in the documentation that is not correct english and confuses me:

If you cannot provide a sec_key, session_id UNIFORMLY RANDOM AND KEPT SECRET (even from other signers).

This makes me wonder, the name "SessionId" somehow made me thing it had to be shared between everyone in the same signing session. But from that sentence maybe it seems that it should be locally random and not shared?

stevenroose · 2024-01-16T18:56:03Z

src/zkp/musig.rs

+        secp: &Secp256k1<C>,
+        mut secnonce: MusigSecNonce,
+        keypair: &Keypair,
+        key_agg_cache: &MusigKeyAggCache,


Why does this need to be provided twice? It's already provided in the constructor..

stevenroose · 2024-01-16T19:19:23Z

src/zkp/musig.rs

+    /// #
+    /// let key_agg_cache = MusigKeyAggCache::new(&secp, &[pub_key1, pub_key2]);
+    /// // The session id must be sampled at random. Read documentation for more details.
+    /// let session_id = MusigSessionId::new(&mut thread_rng());


This method doesn't exist anymore and seems to be renamed to assume_unique_per_nonce_gen.

stevenroose · 2024-01-16T19:19:56Z

src/zkp/musig.rs

+    pub fn nonce_gen<C: Signing>(
+        &self,
+        secp: &Secp256k1<C>,
+        session_id: MusigSessionId,


MusigSessionId is not Copy and it has to be kept around for partial signature generation, right? So this API should take a ref, I suppose.

Hmm, I seem to be mistaken. The session ID is not needed to create a MusigSession at all. So probably ignore this, but maybe double check :)

stevenroose · 2024-01-17T00:41:25Z

Are you planning serde serialization for the types?

stevenroose · 2024-01-18T18:05:19Z

Suggestion: change the MusigKeyAggCache pubkey argument to an iterator so that the user doesn't necessarily have to allocate.

    pub fn new<C: Verification>(secp: &Secp256k1<C>, pubkeys: &[PublicKey]) -> Self {

EDIT: Same goes for nonces. I've been using this for a bit and it seems that a common situation is that you have "all other pubkeys" or "all other nonces", plus your own one and then you either have to re-allocate just to add your own, where with iterators, you could just .iter().chain(Some(&mine)).

So the argument would be impl IntoIterator<Item = PublicKey> and MusigPubNonce respectively. They are both Copy, so no need to take a ref iter.

sanket1729 · 2024-01-29T09:56:05Z

@stevenroose, thanks for the testing this and providing valuable user feedback. Working on suggested changes.

jlest01 · 2024-07-07T02:00:08Z

Is this PR still active? What can be done for the merge ?

apoelstra · 2024-07-29T14:07:33Z

cc @jonasnick can you take a fresh look at this?

jonasnick

I went throught the PR and marked all comments resolved that appeared to be resolved. There are still a few unaddressed review comments (e.g., a panic, an incorrect type in the FFI, unfinished sentences). I think the PR also needs to be rebased to use the new v0_10_0 symbols.

@stevenroose

This makes me wonder, the name "SessionId" somehow made me thing it had to be shared between everyone in the same signing session. But from that sentence maybe it seems that it should be locally random and not shared?

Argh, no, sorry. SessionId is absotlutely not to be shared. It must be kept secret from other signers. We renamed this session_secrand in the latest version of the module (PR'd to libsecp256k1, not in libsecp256k1-zkp yet).

jonasnick · 2024-07-29T20:08:00Z

src/zkp/musig.rs

+    /// UNIFORMLY RANDOM AND KEPT SECRET (even from other signers).
+    /// Refer to libsecp256k1-zkp documentation for additional considerations.
+    ///
+    /// Musig2 nonces can be precomputed without knowing the aggregate public key, the message to sign.


Musig2 -> MuSig2, also below in the error enum doc. And sometimes it's just called MuSig. Should be MuSig2 consistently.

stevenroose · 2024-10-06T23:03:34Z

Damn we missing @sanket1729 here..

Ademan · 2024-10-07T20:24:37Z

Given bitcoin-core/secp256k1#1479 was merged, does it make sense to focus on rust-bitcoin/rust-secp256k1#716 ?

sanket1729 · 2024-10-09T08:41:16Z

Closing this in favor of upstream PR linked in the comment above

sanket1729 changed the title ~~Comment out Wasm build on CI~~ Musig2 support Mar 21, 2022

sanket1729 force-pushed the pr29 branch 3 times, most recently from 3eef309 to 6eb040e Compare March 21, 2022 22:34

sanket1729 mentioned this pull request Mar 23, 2022

Is there a way of having aggregate signature in first Tr descriptor parameter? rust-bitcoin/rust-miniscript#319

Open

sanket1729 force-pushed the pr29 branch 9 times, most recently from 7fcd5b6 to 651c0de Compare April 15, 2022 19:56

sanket1729 marked this pull request as ready for review April 15, 2022 20:02

sanket1729 force-pushed the pr29 branch 2 times, most recently from 9694636 to a923782 Compare April 15, 2022 20:43

jonasnick reviewed Apr 22, 2022

View reviewed changes

jonasnick reviewed Apr 25, 2022

View reviewed changes

justinmoon added a commit to justinmoon/fedimint that referenced this pull request May 7, 2022

A few issues remaining

51b3450

- secp256k1_zkp is missing BlockstreamResearch/rust-secp256k1-zkp#48 - bitcoin_hashes is missing rust-bitcoin/bitcoin_hashes@f1084bf

justinmoon mentioned this pull request May 7, 2022

Check if we can move off forked deps fedimint/fedimint#65

Closed

sanket1729 commented Jun 14, 2022

View reviewed changes

sanket1729 force-pushed the pr29 branch 4 times, most recently from 288f52e to 7ac3464 Compare June 14, 2022 02:08

ssantos21 suggested changes Sep 10, 2023

View reviewed changes

stevenroose reviewed Nov 18, 2023

View reviewed changes

src/zkp/musig.rs Outdated Show resolved Hide resolved

sanket1729 force-pushed the pr29 branch 2 times, most recently from 158ff81 to bcd5419 Compare January 7, 2024 15:21

sanket1729 force-pushed the pr29 branch from bcd5419 to 4a3a703 Compare January 7, 2024 16:40

secp256k1-zkp-sys: Add Rust FFI for Musig2 module

163186a

sanket1729 force-pushed the pr29 branch 2 times, most recently from b54aa8d to 8b956f7 Compare January 7, 2024 17:17

zkp: Add Musig2 module

60e631c

Signed-off-by: Sanket Kanjalkar <[email protected]>

sanket1729 force-pushed the pr29 branch from 8b956f7 to 60e631c Compare January 7, 2024 17:19

philipr-za reviewed Jan 16, 2024

View reviewed changes

stevenroose reviewed Jan 16, 2024

View reviewed changes

rajarshimaitra mentioned this pull request Mar 30, 2024

[SOB]: Swap HTLC via Taproot+Musig2 citadel-tech/coinswap#121

Closed

007harshmahajan mentioned this pull request May 23, 2024

swaping HTLC via Taproot+Musig2 tracking issue citadel-tech/coinswap#147

Open

6 tasks

jlest01 mentioned this pull request Jul 29, 2024

Add Musig2 module rust-bitcoin/rust-secp256k1#716

Open

jonasnick reviewed Jul 29, 2024

View reviewed changes

sanket1729 closed this Oct 9, 2024

	output_pubkey: *mut XOnlyPublicKey,
	output_pubkey: *mut PublicKey,

Musig2 support #48

Musig2 support #48

Conversation

sanket1729 commented Mar 21, 2022 • edited Loading

sanket1729 commented Apr 15, 2022

sanket1729 commented Apr 15, 2022 • edited Loading

jonasnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tibo-lg commented Jun 23, 2022

sanket1729 commented Jun 24, 2022

ssantos21 commented Sep 5, 2023 • edited Loading

ssantos21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 commented Jan 7, 2024

philipr-za Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenroose commented Jan 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenroose commented Jan 17, 2024

stevenroose commented Jan 18, 2024 • edited Loading

sanket1729 commented Jan 29, 2024 • edited Loading

jlest01 commented Jul 7, 2024 • edited Loading

apoelstra commented Jul 29, 2024

jonasnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenroose commented Oct 6, 2024

Ademan commented Oct 7, 2024

sanket1729 commented Oct 9, 2024

sanket1729 commented Mar 21, 2022 •

edited

Loading

sanket1729 commented Apr 15, 2022 •

edited

Loading

ssantos21 commented Sep 5, 2023 •

edited

Loading

philipr-za Jan 16, 2024 •

edited

Loading

stevenroose commented Jan 18, 2024 •

edited

Loading

sanket1729 commented Jan 29, 2024 •

edited

Loading

jlest01 commented Jul 7, 2024 •

edited

Loading