Add bip340 schnorr #237

Tibo-lg · 2020-09-15T02:24:00Z

As BIP 340 support was merged in bitcoin core and that I had a branch of rust-secp256k1 with BIP 340 support I thought maybe now was a good time to try to get it merged.

Changes:

Update the depend folder to the latest master
Update secp256k1-sys to add Schnorr and extra keys modules, as well as the bindings for the related functions
Add schnorrsig module in the rust-secp256k1

One thing I am unsure is whether types from the extra keys module should be used into the public interfaces (x-only pubkeys and keypairs). For now I implemented to just use PublicKey and SecretKey in public interface, but I can change it.

elichai · 2020-09-15T09:14:04Z

I think we should have a discussion on what API do we want to use here and how.
IMHO we should really discourage people from using the same keys for schnorr and ecdsa, do we do this by having even a separate privkey struct for schnorr? idk.
but at least I think we shouldn't allow verifying schnorr from an ECDSA pubkey without explicitly converting between the keys.

Maybe we can schedule an IRC call on this sometime? @apoelstra @real-or-random

Tibo-lg · 2020-09-15T12:23:29Z

I'm happy to add separate type if that's what you think it's best (and if you're ok with it). I tried to keep it minimal for now. But that kind of feedback is really helpful as I'm maintaining a separate branch for DLC things and the less I diverge from mainstream the easier it is to maintain. Anyway keep me updated!

Tibo-lg · 2020-09-25T01:20:09Z

Updated to add SchnorrPublicKey struct (which corresponds to XOnlyPublicKey at the sys module level) as it makes quite some things easier for me and simplifies the code a little bit.

apoelstra · 2020-10-01T11:38:40Z

src/schnorrsig.rs

+
+/// A Secp256k1 public key, used for verification of signatures
+#[derive(Copy, Clone, PartialEq, Eq, Debug, PartialOrd, Ord, Hash)]
+pub struct SchnorrPublicKey(ffi::XOnlyPublicKey);


This can just be called PublicKey, it's already in the bip340 module, so users can refer to it as bip340::PublicKey

apoelstra · 2020-10-01T11:39:36Z

src/lib.rs

@@ -169,6 +169,7 @@ mod context;
 pub mod constants;
 pub mod ecdh;
 pub mod key;
+pub mod schnorrsig;


I'd like the module to be called bip340 rather than Schnorr. Both as a dig at Schnorr for patenting things, and also to be clear that this is a specific Schnorr-based signature scheme specified in BIP340.

Schnorr has been removed from this module :). Only mentions left are in the sys module as the method calls of secp use it.

apoelstra · 2020-10-01T11:56:32Z

+1 to having a separate SecretKey type in the bip340 module

apoelstra · 2020-10-01T11:57:51Z

src/schnorrsig.rs

+use {Message, Signing};
+
+/// Represents a schnorr signature.
+pub struct SchnorrSignature([u8; constants::SCHNORR_SIGNATURE_SIZE]);


Similarly we can drop the prefix from this type

Tibo-lg · 2020-10-02T05:18:16Z

Changed names from Schnorr to BIP340 wherever possible
Made macros for implementing SecretKey functions and the serde serialization for PublicKey to reduce code duplication. If you don't like that I can replace it with simple copy paste but it felt nicer that way. There might be room for adding some PublicKey functions to macros, though not all due to the uncompressed version for ECDSA.
Added SecretKey in BIP340 module (which has the exact same implementation as the one in the key module through the above mentioned macro).

Also wanted to mention a couple of things I haven't handled:

tweak add for bip340::PublicKey
I just put unimplemented!() in the fuzz_dummy module for the functions I added
I think those can be handled later but if let me know if they are essential.

apoelstra · 2020-10-02T11:32:08Z

I think it's fine to macro-ize the serde stuff. Can you use the same macro to impl serde on the bip340::Signature type?

As for the secret key stuff, it'd be better to move the code into a new key.rs.in file and then include!("key.rs.in"); // define SecretKey in both files. Rather than using macro_rules which adds extra indentation and tends to result in confusing compiler error messages.

real-or-random · 2020-10-02T11:52:15Z

I think bip340 is fine but it may be a good idea to use the same module names as upstream.

Tibo-lg · 2020-10-02T13:06:31Z

@apoelstra

I think it's fine to macro-ize the serde stuff. Can you use the same macro to impl serde on the bip340::Signature type?

I couldn't use the same one as PublicKey but used the one that was already there for SecretKey (it had a harcoded SecretKey I assumed that was a typo: 8735378#diff-e1576943a5907b15388ab726037ebdb3L70).

As for the secret key stuff, it'd be better to move the code into a new key.rs.in file and then include!("key.rs.in"); // define SecretKey in both files. Rather than using macro_rules which adds extra indentation and tends to result in confusing compiler error messages.

Done. That's neat didn't know about this feature.

Pipeline is failing now but AFAICT it seems to be some issue with rustup installation of nightly version.

apoelstra · 2020-10-02T13:36:15Z

secp256k1-sys/src/lib.rs

    pub fn secp256k1_ec_pubkey_tweak_add(cx: *const Context,
                                         pk: *mut PublicKey,
                                         tweak: *const c_uchar)
                                         -> c_int;

-    #[deprecated(since = "0.2.0",note = "Please use the secp256k1_ec_seckey_tweak_mul function instead")]


Did you change this on purpose?

Sorry not sure what I've done there, fixed.

apoelstra · 2020-10-02T13:43:46Z

@Tibo-lg can you change the first commit to include all the 0_2_0→0_2_1 mappings in the .rs files? otherwise it doesn't build.

apoelstra · 2020-10-02T13:49:36Z

Can you also reduce the indentation level in key.rs.in, and also move

/// Secret 256-bit key used as `x` in an ECDSA signature
pub struct SecretKey([u8; constants::SECRET_KEY_SIZE]);

(I guess, update the comment to not say ECDSA) into the file? It makes the diff easier to read.

real-or-random · 2020-10-26T10:31:03Z

Yes, I think we should change the prefix to schnorrsig too (not just schnorr).

Tibo-lg · 2020-10-27T08:28:21Z

Yes, I think we should change the prefix to schnorrsig too (not just schnorr).

Updated

elichai · 2020-11-05T10:25:15Z

We can't use the same SecretKey operations for schnorr secret key.
as it will not produce the same results as the public key will.

(ie schnorrsig::PublicKey.add(tweak) != schnorrsig::SecretKey.add(tweak).PublicKey())
I think we need use the secp256k1_keypair API but it doesn't currently expose the seckey back and it means every seckey tweak is also a EC addition

apoelstra · 2020-11-05T16:28:53Z

Ah, good catch. I'd forgotten about the keypair API. You're right that we cannot use normal secret keys with the schnorrsig module, at least not without nasty surprises for users.

I think the best thing is to replicate the upstream API ... which does mean that you can't tweak secret keys without also tweaking the public ones. The only alternative is that the user keep track of y-parity themselves and this is too fragile and "hand-rolled crypto"-y for this library.

apoelstra · 2020-11-05T16:35:06Z

So @Tibo-lg sorry to jerk you around again, but I think we need to back out the key.rs.in stuff, drop the schnorrsig::SecretKey type, and work only with keypairs and public keys, like upstream does. Trying to provide an independent SecretKey will cause surprising inconsistencies because the keytweaking functions need to be aware of the x-only-key stuff.

Please don't feel obligated to finish this PR btw -- if you drop it, I can pick it up when I find a free moment (though this may not be for a little while).

Tibo-lg · 2020-11-09T07:39:12Z

@apoelstra I don't mind updating the PR if you don't mind reviewing it :)

I removed the key.rs.in file, and switched to have KeyPair instead of SecretKey in the schnorrsig module.

I also added add_assign to tweak a KeyPair, but I'm not really sure on how to test this properly without using a "regular" public key object somewhere. For example, testing schnorrsig::PublicKey.add(tweak) == schnorrsig::SecretKey.add(tweak).PublicKey() as @elichai mentioned would require adding an add function to schnorrsig::PublicKey, but the underlying tweak function returns a "regular" pubkey so I'm not sure how the API should look like. I guess the two options would be to either have two PublicKey types in the schnorrsig module (XOnlyPublicKey and PublicKey), or to return the parity somehow (but IIUC we want to avoid that?). I'm hoping maybe someone has a better suggestion?

elichai · 2020-11-09T09:16:44Z

I also added add_assign to tweak a KeyPair, but I'm not really sure on how to test this properly without using a "regular" public key object somewhere. For example, testing schnorrsig::PublicKey.add(tweak) == schnorrsig::SecretKey.add(tweak).PublicKey() as @elichai mentioned would require adding an add function to schnorrsig::PublicKey, but the underlying tweak function returns a "regular" pubkey so I'm not sure how the API should look like. I guess the two options would be to either have two PublicKey types in the schnorrsig module (XOnlyPublicKey and PublicKey), or to return the parity somehow (but IIUC we want to avoid that?). I'm hoping maybe someone has a better suggestion?

IMO we should only expose the XOnlyPublicKey here, but you can internally convert back and forth between the puckeys where needed, so it will look something like:

secp256k1_xonly_pubkey_tweak_add(ctx, &out_pubkey, &xonlypubkey, &tweak);
secp256k1_xonly_pubkey_from_pubkey(ctx, &xonlypubkey, NULL, &out_pubkey)

apoelstra · 2020-11-09T14:38:46Z

+1 to @elichai's suggestion, though I think it's reasonable to also expose a function for the user to expose conversion functions, with the understanding that ordinary pubkeys are not useful for any other part of the schnorr API.

Also, I hope we don't wind up converting from xonly to regular anywhere internally, since that's expensive.

elichai · 2020-11-09T14:47:57Z

+1 to @elichai's suggestion, though I think it's reasonable to also expose a function for the user to expose conversion functions, with the understanding that ordinary pubkeys are not useful for any other part of the schnorr API.

Also, I hope we don't wind up converting from xonly to regular anywhere internally, since that's expensive.

we shouldn't, only the opposite (from pubkey to xonly), which is cheap

Tibo-lg · 2020-11-10T06:15:46Z

@elichai Thanks for the suggestion. I added add_assign to schnorrsig::PublicKey and a test for addition.

I think it's reasonable to also expose a function for the user to expose conversion functions, with the understanding that ordinary pubkeys are not useful for any other part of the schnorr API.

I added From<::key::PublicKey> for schnorrsig::PublicKey does that fit what you had in mind? Left it in a separate commit for now so it's easier to drop.

apoelstra · 2020-11-27T17:11:08Z

secp256k1-sys/src/lib.rs

+
+impl XOnlyPublicKey {
+    /// Create a new (zeroed) x-only public key usable for the FFI interface
+    pub fn new() -> XOnlyPublicKey { XOnlyPublicKey([0; 64]) }


We shouldn't have this function, as it creates an invalid public key which will trigger assertation failures in upstream.

I'll fix this in a followup commit/PR. It's a bit subtle because we do need (temporarily) uninitialized values to use the FFI interface.

apoelstra · 2020-11-27T17:11:25Z

secp256k1-sys/src/lib.rs

+impl XOnlyPublicKey {
+    /// Create a new (zeroed) x-only public key usable for the FFI interface
+    pub fn new() -> XOnlyPublicKey { XOnlyPublicKey([0; 64]) }
+    pub fn from_array(data: [c_uchar; 64]) -> XOnlyPublicKey {


We should rename this to from_array_unchecked for similar reasons.

apoelstra · 2020-11-27T17:47:54Z

secp256k1-sys/src/lib.rs

+        msg32: *const c_uchar,
+        keypair: *const KeyPair,
+        noncefp: SchnorrNonceFn,
+        noncedata: *const c_void


upstream this is a *mut c_void

apoelstra · 2020-11-27T18:01:47Z

src/schnorrsig.rs

+        &mut self,
+        secp: &Secp256k1<C>,
+        tweak: &[u8],
+    ) -> Result<(), Error> {


This should return the parity as a bool rather than returning ()

apoelstra

ack 0c937d0

Will address nits in a followup PR.

Tibo-lg force-pushed the add-bip340-schnorr branch 6 times, most recently from a0f1800 to 61b3017 Compare September 15, 2020 06:25

Tibo-lg force-pushed the add-bip340-schnorr branch 3 times, most recently from 2a60595 to 7923629 Compare September 25, 2020 00:31

apoelstra reviewed Oct 1, 2020

View reviewed changes

Tibo-lg force-pushed the add-bip340-schnorr branch 2 times, most recently from bb3c4fe to 1eaf097 Compare October 2, 2020 04:44

Tibo-lg force-pushed the add-bip340-schnorr branch 4 times, most recently from 2f99f09 to 8735378 Compare October 2, 2020 12:58

apoelstra reviewed Oct 2, 2020

View reviewed changes

Tibo-lg force-pushed the add-bip340-schnorr branch from 8735378 to d9a83e3 Compare October 5, 2020 01:42

Tibo-lg force-pushed the add-bip340-schnorr branch 2 times, most recently from cccdb80 to afebc54 Compare October 27, 2020 05:37

Tibo-lg added 2 commits November 9, 2020 16:00

Update secp256k1-sys to add schnorr/extra-keys

45a4459

Add serde macros for public keys

47b3382

Tibo-lg force-pushed the add-bip340-schnorr branch 2 times, most recently from 34aad13 to 5ba9d5f Compare November 9, 2020 07:25

Add schnorrsig module

09b0456

Tibo-lg force-pushed the add-bip340-schnorr branch from 5ba9d5f to 09b0456 Compare November 10, 2020 04:43

Add key::PublicKey to schnorrsig::PublicKey conversion

0c937d0

apoelstra reviewed Nov 27, 2020

View reviewed changes

apoelstra approved these changes Nov 27, 2020

View reviewed changes

apoelstra merged commit be9a78f into rust-bitcoin:master Nov 27, 2020

apoelstra mentioned this pull request Nov 27, 2020

BIP 0340 followups #253

Merged

dr-orlovsky mentioned this pull request Feb 18, 2021

Add support for Schnorr signatures to be stored in RGB data RGB-WG/rgb-core#16

Closed

1 task

Tibo-lg mentioned this pull request Apr 23, 2021

Use aux rand for ecdsa adaptor encrypt BlockstreamResearch/rust-secp256k1-zkp#15

Merged

Tibo-lg mentioned this pull request Nov 9, 2021

Re-arrange functionality to make ECDSA and Schnorr equal-ish citizens #327

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bip340 schnorr #237

Add bip340 schnorr #237

Tibo-lg commented Sep 15, 2020

elichai commented Sep 15, 2020

Tibo-lg commented Sep 15, 2020

Tibo-lg commented Sep 25, 2020

apoelstra Oct 1, 2020

Tibo-lg Oct 2, 2020

apoelstra Oct 1, 2020

Tibo-lg Oct 2, 2020

apoelstra commented Oct 1, 2020

apoelstra Oct 1, 2020

Tibo-lg Oct 2, 2020

Tibo-lg commented Oct 2, 2020

apoelstra commented Oct 2, 2020

real-or-random commented Oct 2, 2020

Tibo-lg commented Oct 2, 2020

apoelstra Oct 2, 2020

Tibo-lg Oct 5, 2020

apoelstra commented Oct 2, 2020

apoelstra commented Oct 2, 2020

real-or-random commented Oct 26, 2020

Tibo-lg commented Oct 27, 2020

elichai commented Nov 5, 2020

apoelstra commented Nov 5, 2020

apoelstra commented Nov 5, 2020

Tibo-lg commented Nov 9, 2020

elichai commented Nov 9, 2020

apoelstra commented Nov 9, 2020

elichai commented Nov 9, 2020 •

edited

Loading

Tibo-lg commented Nov 10, 2020

apoelstra Nov 27, 2020

apoelstra Nov 27, 2020

apoelstra Nov 27, 2020

apoelstra Nov 27, 2020

apoelstra Nov 27, 2020

apoelstra left a comment

Add bip340 schnorr #237

Add bip340 schnorr #237

Conversation

Tibo-lg commented Sep 15, 2020

elichai commented Sep 15, 2020

Tibo-lg commented Sep 15, 2020

Tibo-lg commented Sep 25, 2020

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

apoelstra commented Oct 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tibo-lg commented Oct 2, 2020

apoelstra commented Oct 2, 2020

real-or-random commented Oct 2, 2020

Tibo-lg commented Oct 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Oct 2, 2020

apoelstra commented Oct 2, 2020

real-or-random commented Oct 26, 2020

Tibo-lg commented Oct 27, 2020

elichai commented Nov 5, 2020

apoelstra commented Nov 5, 2020

apoelstra commented Nov 5, 2020

Tibo-lg commented Nov 9, 2020

elichai commented Nov 9, 2020

apoelstra commented Nov 9, 2020

elichai commented Nov 9, 2020 • edited Loading

Tibo-lg commented Nov 10, 2020

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

apoelstra left a comment

Choose a reason for hiding this comment

elichai commented Nov 9, 2020 •

edited

Loading