Add Musig2 module #716

jlest01 · 2024-07-29T00:41:46Z

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.

This modification allows running the command `./vendor-libsecp.sh -f pull/1479/head` to obtain MuSig2 components from `bitcoin-core/secp256k1`. This commit may be discarded after merging PR #1479.

Kixunil

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.

Kixunil · 2024-07-29T06:36:27Z

src/musig.rs

+            // - Key agg cache is valid
+            // - extra input is 32 bytes
+            // This can only happen when the session id is all zeros
+            Err(MusigNonceGenError::ZeroSession)


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

src/musig.rs

tcharding · 2024-08-29T19:46:08Z

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

apoelstra · 2024-08-29T21:22:20Z

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

jlest01 · 2024-08-29T22:12:56Z

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 · 2024-08-29T22:59:56Z

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 · 2024-08-30T04:38:36Z

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

jlest01 · 2024-08-31T14:22:15Z

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.

Kixunil · 2024-08-31T18:11:50Z

secp256k1-sys/src/lib.rs

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


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

Same as here: #716 (comment)

Kixunil · 2024-08-31T18:12:45Z

secp256k1-sys/src/lib.rs

+        MusigSecNonce([0; MUSIG_SECNONCE_LEN])
+    }
+
+    /// Don't use this. Refer to the documentation of wrapper APIs in the crate.


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.

Done. Thanks.

Kixunil · 2024-08-31T18:13:07Z

secp256k1-sys/src/lib.rs

+impl_raw_debug!(MusigPubNonce);
+
+impl MusigPubNonce {
+    pub fn new() -> Self {


Looks also broken.

Same as here: #716 (comment)

Kixunil · 2024-08-31T18:14:24Z

secp256k1-sys/src/lib.rs

+    fn default() -> Self {
+        Self::new()
+    }
+}


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

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.

Kixunil · 2024-08-31T18:28:09Z

secp256k1-sys/src/lib.rs

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


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

Do you think they should be changed?

Kixunil · 2024-08-31T18:31:24Z

src/musig.rs

+#[derive(Debug, Clone, Copy, Eq, PartialEq, PartialOrd, Ord, Hash)]
+pub enum ParseError {
+    /// Length mismatch
+    ArgLenMismatch {


We usually name these InvalidLength.

Done. Thanks.

jlest01 added 2 commits July 27, 2024 11:17

Modify the vendor-libsecp file to enable checkout of a pull request

8f0dd73

This modification allows running the command `./vendor-libsecp.sh -f pull/1479/head` to obtain MuSig2 components from `bitcoin-core/secp256k1`. This commit may be discarded after merging PR #1479.

Enable musig module in the secp256k1-sys/build.rs

46d5ef8

Kixunil reviewed Jul 29, 2024

View reviewed changes

jlest01 force-pushed the musig2-module branch 4 times, most recently from ee040cf to db43d01 Compare July 31, 2024 17:15

Update codebase to 0_10_0 to include secp256k1 musig module

685112b

jlest01 force-pushed the musig2-module branch 2 times, most recently from 447a94c to e730b8b Compare July 31, 2024 18:20

jlest01 mentioned this pull request Aug 11, 2024

[WIP] Add a Musig2 example file rust-bitcoin/rust-bitcoin#3158

Open

jlest01 force-pushed the musig2-module branch 3 times, most recently from a91d293 to 8bbd0d2 Compare August 29, 2024 11:39

jlest01 force-pushed the musig2-module branch from 8bbd0d2 to c313387 Compare August 30, 2024 01:40

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

Kixunil reviewed Aug 31, 2024

View reviewed changes

jlest01 added 3 commits September 16, 2024 23:00

Add Rust FFI for Musig2 module

047b85d

Add Musig2 module

4096b5d

Add Musig2 example file

071ac15

jlest01 force-pushed the musig2-module branch from 86e2b28 to 071ac15 Compare September 17, 2024 02:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Musig2 module #716

Add Musig2 module #716

jlest01 commented Jul 29, 2024

Kixunil left a comment

Kixunil Jul 29, 2024

tcharding commented Aug 29, 2024

apoelstra commented Aug 29, 2024

jlest01 commented Aug 29, 2024

tcharding commented Aug 29, 2024

Kixunil commented Aug 30, 2024

jlest01 commented Aug 31, 2024

Kixunil Aug 31, 2024

jlest01 Sep 17, 2024

Kixunil Aug 31, 2024

jlest01 Sep 17, 2024

Kixunil Aug 31, 2024

jlest01 Sep 17, 2024

Kixunil Aug 31, 2024

jlest01 Sep 17, 2024

Kixunil Aug 31, 2024

jlest01 Sep 17, 2024

Kixunil Aug 31, 2024

jlest01 Sep 17, 2024

Add Musig2 module #716

Are you sure you want to change the base?

Add Musig2 module #716

Conversation

jlest01 commented Jul 29, 2024

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Aug 29, 2024

apoelstra commented Aug 29, 2024

jlest01 commented Aug 29, 2024

tcharding commented Aug 29, 2024

Kixunil commented Aug 30, 2024

jlest01 commented Aug 31, 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

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

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