rust-secp256k1-zkp

rust-secp256k1-zkp copied to clipboard

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

Marked as ready for review.

~Resolved~

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 :)!

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.

The MuSig implementation in libsecp256k1-zkp has significantly changed while it was updated to the BIP version 1. Therefore, we should update the libsecp256k1-zkp version we use here.

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

Well that escalated quickly.. I'm super excited about this though, guys!

It would be nice to have serde serialization for pubnonces and partial signatures. (I may provide a patch for that)

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

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

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?

Are you planning serde serialization for the types?

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.

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

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

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

Damn we missing @sanket1729 here..

Given https://github.com/bitcoin-core/secp256k1/pull/1479 was merged, does it make sense to focus on https://github.com/rust-bitcoin/rust-secp256k1/pull/716 ?

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