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

thomaseizinger · 2021-09-09T10:06:44Z

This patch-set tries to re-structure the library a bit. What we currently have seems to have been mostly driven by historical growth. For example, with the addition of Schnorr signatures, just exposing secp256k1::Signature is ambiguous.

This PR only contains renames and moving around of code. I've tried to structure the patches in such a way that makes this reasonably easy to review. Feedback welcome!

thomaseizinger · 2021-09-09T10:17:12Z

I had an idea that might make review easier: I could move all tests to tests/ in an initial commit. That would at least showcase that none of the tests change after moving stuff around?

real-or-random · 2021-09-10T16:21:59Z

Hm, yeah, in fact we should do something like this in upstream... But don't assume it will happen, so I'm indifferent to the changes here. The resulting structure is certainly more natural but also not so aligned with upstream, which may make it harder to maintain.

One comment: I really prefer "schnorrsig" instead of "schnorr". Schnorr is not a signature scheme but a living human being.

thomaseizinger · 2021-09-10T20:47:50Z

Thanks for the feedback!

Hm, yeah, in fact we should do something like this in upstream... But don't assume it will happen, so I'm indifferent to the changes here. The resulting structure is certainly more natural but also not so aligned with upstream, which may make it harder to maintain.

I think aligning -sys with upstream makes sense. Everything on top should IMO be as Rust idiomatic as possible and hide underlying details / structures.

One comment: I really prefer "schnorrsig" instead of "schnorr". Schnorr is not a signature scheme but a living human being.

I agree with your reasoning. The question is, do the modules necessarily have to be named after the scheme or is it good enough (maybe better) to find a name that acts as an unambiguous and clear qualifier?

Bear in mind that the whole point of this is to allow users to import the module and qualify the types inside:

schnorr::Signature
schnorrsig::Signature

I prefer the first one out of these two.

real-or-random · 2021-09-13T11:48:35Z

* `schnorr::Signature`

* `schnorrsig::Signature`
I prefer the first one out of these two.

In this special case, I do prefer the first one. But this is a somewhat extreme case. In general, I prefer schnorrsig over schnorr, e.g., I don't like schnorr::Verify.

thomaseizinger · 2021-09-13T12:04:26Z

I would be okay with the module being named schnorr but all functions on Secp256k1 using schnorrsig!

dr-orlovsky · 2021-09-15T04:22:36Z

Just did a PR doing the same: #329. I concept ACK this PR and the fact that I independently proposed the same API proves that these changes are intuitive and required.

sanket1729 · 2021-09-24T01:03:41Z

I think aligning -sys with upstream makes sense. Everything on top should IMO be as Rust idiomatic as possible and hide underlying details / structures.

+1 . Concept ACK, will review this in the near future.

sanket1729

Thanks for this. I really like this API. In fact, I have been using pub type schnorr::PublicKey as XOnlyKey in downstream projects.

Can't find anything worth complaining about, just left the two comments.

sanket1729 · 2021-09-24T20:54:40Z

src/lib.rs

@@ -772,7 +492,18 @@ impl<C: Signing> Secp256k1<C> {
    /// signature implementation of bitcoin core. In average, this function
    /// will perform two signing operations.
    /// Requires a signing capable context.
-    pub fn sign_low_r(&self, msg: &Message, sk: &key::SecretKey) -> Signature {
+    #[deprecated(since = "0.21.0", note = "Use sign_ecdsa_grind_r instead.")]


I don't understand grinding APIs well. But this should be Use sign_ecdsa_sign_low instead

Ups totally! This is a copy-paste error 😅

sanket1729 · 2021-09-24T20:55:46Z

src/lib.rs

-    pub fn sign_low_r(&self, msg: &Message, sk: &key::SecretKey) -> Signature {
+    #[deprecated(since = "0.21.0", note = "Use sign_ecdsa_grind_r instead.")]
+    pub fn sign_low_r(&self, msg: &Message, sk: &SecretKey) -> ecdsa::Signature {
+        return self.sign_grind_with_check(msg, sk, compact_sig_has_zero_first_bit)


nit: Even though the implementation is the same, using self.sign_ecdsa_low_r makes for easier review

Also a copy paste error, thanks for spotting it! :)

dr-orlovsky · 2021-09-27T12:08:16Z

Can someone of maintainer review this PR pls?

Tibo-lg

I think the refactor looks nice, though maybe I feel like putting everything into the key module makes it not so clear that PublicKey is meant to be used for ECDSA while KeyPair and XOnlyPublicKey are meant to be used for Schnorr signatures. Could be worth at least emphasizing this in the docs for PublicKey and KeyPair (XOnlyPublicKey already mentions it).
Also, maybe it's not a valid concern, but if at some point we wanted to make either ECDSA or Schnorr sig optional it would make things a bit more tedious than if the types were in separated modules.

Otherwise just had two tiny nits.

src/key.rs

thomaseizinger · 2021-10-22T07:45:36Z

I think the refactor looks nice, though maybe I feel like putting everything into the key module makes it not so clear that PublicKey is meant to be used for ECDSA while KeyPair and XOnlyPublicKey are meant to be used for Schnorr signatures. Could be worth at least emphasizing this in the docs for PublicKey and KeyPair (XOnlyPublicKey already mentions it).

I think I agree but I wanted to limit this PR to not-so-controversial changes because it is already big enough !

If we can get this in, then I think it will be much easier to send smaller follow-up PRs that make the situation even better :)

Also, maybe it's not a valid concern, but if at some point we wanted to make either ECDSA or Schnorr sig optional it would make things a bit more tedious than if the types were in separated modules.

It might be worth it to create two submodules under key although I wouldn't know what to name them from the top of my head. I know that @sanket1729 has opinions about not referring to keys via the signature scheme they are used in (for example ecdsa::PublicKey) :)

Tibo-lg · 2021-10-22T12:19:32Z

I think I agree but I wanted to limit this PR to not-so-controversial changes because it is already big enough !

If we can get this in, then I think it will be much easier to send smaller follow-up PRs that make the situation even better :)

SGTM.

It might be worth it to create two submodules under key although I wouldn't know what to name them from the top of my head. I know that @sanket1729 has opinions about not referring to keys via the signature scheme they are used in (for example ecdsa::PublicKey) :)

I guess a point could be made that the keys are not tight to the signature scheme per se. I don't have that strong of an opinion tbh and if my concern is unfounded I'll be happy with everything under key (at least I'll be able to remove some aliases from my code :) ).

sanket1729

ACK 975c212

apoelstra

ACK 975c212

apoelstra · 2021-11-02T18:18:40Z

Happy to merge this today, but it needs a rebase.

thomaseizinger · 2021-11-03T09:49:16Z

Happy to merge this today, but it needs a rebase.

I will only be able to do this from the 9th as I am currently on vacation without a computer. Will rebase once I am back!

thomaseizinger · 2021-11-03T09:49:40Z

Also, great news! I am excited for this to get merged :)

dr-orlovsky

Two nits, but I think this is good to go. utACK 975c212

src/key.rs

dr-orlovsky · 2021-11-08T12:56:48Z

src/schnorr.rs

@@ -7,13 +7,12 @@ use rand::thread_rng;
 #[cfg(any(test, feature = "rand"))]
 use rand::{CryptoRng, Rng};

-use super::Error::{InvalidPublicKey, InvalidSecretKey, InvalidSignature};
+use super::Error::InvalidSignature;


I believe better name for this file is bip340, since (a) we need a name of the signing scheme like ECDSA, and the scheme is defined in BIP-340 by P.Wuille (using Schnorr algorithm) and not by Schnorr

Just FYI, but there was a similar discussion at the time of adding the schnorr module and it was decided to use the same name as upstream at the end: #237 (comment)
It was also decided to use schnorrsig instead of schnorr: #237 (comment)
(I don't personally have a strong opinion)

We re-export all structs residing in that module. There is no reason to expose the internal module structure of the library publicly.

Tests are usually placed next to the code they are testing. As such, importing `super::*` is a good starting point.

With the introduction of Schnorr signatures, exporting a `Signature` type without any further qualification is ambiguous. To minimize the ambiguity, the `ecdsa` module is public which should encourage users to refer to its types as `ecdsa::Signature` and `ecdsa::SerializedSignature`. To reduce ambiguity in the APIs on `Secp256k1`, we deprecate several fucntions and introduce new variants that explicitly mention the use of the ECDSA signature algorithm. Due to the move of `Signature` and `SerializedSignature` to a new module, this patch is a breaking change. The impact is minimal though and fixing the compile errors encourages a qualified naming of the type.

The `KeyPair` type is semantically unrelated to the schnorr signature algorithm.

The public key is unrelated to the signature algorithm. It will be moved out of the module in another commit. For ease of review, the renamed is kept separate.

Schnorr is commenly known as a signature algorithm, we don't need to restate that in the name of the module.

The naming scheme we employ is `{sign,verify, ...}_{ecdsa,schnorr}`.

This is not C89. We can declare the more important things first.

thomaseizinger · 2021-11-11T02:46:14Z

@dr-orlovsky @Tibo-lg Nits have been addressed as part of rebase.
@apoelstra Fixed merge conflicts with latest master.

dr-orlovsky

utACK

sanket1729

ACK d244b4d

tcharding

Just found one legacy typo.

This is not C89. We can declare the more important things first.

Laughed at this commit message :)

tcharding · 2021-11-11T23:45:58Z

src/ecdsa/mod.rs

+
+    /// Constructs a signature for `msg` using the secret key `sk`, RFC6979 nonce
+    /// and "grinds" the nonce by passing extra entropy if necessary to produce
+    /// a signature that is less than 71 - bytes_to_grund bytes. The number


Not introduced by this PR but there is a typo here, I think this should be 'bytes_to_grind' not 'grund'. There are two occurrences of 'grund' in this file.

To avoid having to get re-ACKs, I propose this to be patched in a follow-up PR.

apoelstra

ACK d244b4d

thomaseizinger marked this pull request as draft September 9, 2021 10:14

thomaseizinger force-pushed the ecdsa-schnorr branch from c9f0034 to c88dedf Compare September 9, 2021 10:16

thomaseizinger force-pushed the ecdsa-schnorr branch 2 times, most recently from 0c4854a to c7c21be Compare September 9, 2021 14:16

real-or-random mentioned this pull request Sep 14, 2021

Refactoring schnorrsig #329

Closed

thomaseizinger marked this pull request as ready for review September 20, 2021 03:16

sanket1729 reviewed Sep 24, 2021

View reviewed changes

Tibo-lg reviewed Oct 22, 2021

View reviewed changes

src/key.rs Outdated Show resolved Hide resolved

src/key.rs Outdated Show resolved Hide resolved

sanket1729 previously approved these changes Oct 22, 2021

View reviewed changes

apoelstra previously approved these changes Nov 2, 2021

View reviewed changes

dr-orlovsky added this to Ready for merge in Taproot Nov 8, 2021

dr-orlovsky previously approved these changes Nov 8, 2021

View reviewed changes

thomaseizinger mentioned this pull request Nov 11, 2021

Use explicit u8 when assigning a byte slice #334

Merged

thomaseizinger added 3 commits November 11, 2021 13:38

Make key module private

8e96aba

We re-export all structs residing in that module. There is no reason to expose the internal module structure of the library publicly.

Fully qualify Error to simplify imports

52d0554

Prefer use super::* import over manually picking items

49c7e21

Tests are usually placed next to the code they are testing. As such, importing `super::*` is a good starting point.

thomaseizinger dismissed dr-orlovsky’s stale review via 7eceb18 November 11, 2021 02:43

thomaseizinger dismissed stale reviews from apoelstra and sanket1729 via 7eceb18 November 11, 2021 02:43

thomaseizinger force-pushed the ecdsa-schnorr branch from 975c212 to 7eceb18 Compare November 11, 2021 02:43

Taproot automation moved this from Ready for merge to In review Nov 11, 2021

thomaseizinger added 9 commits November 11, 2021 13:43

Move KeyPair to key module

2e0e731

The `KeyPair` type is semantically unrelated to the schnorr signature algorithm.

Rename schnorr::PublicKey to schnorr::XOnlyPublicKey

87d936a

The public key is unrelated to the signature algorithm. It will be moved out of the module in another commit. For ease of review, the renamed is kept separate.

Move XOnlyPublicKey to key module

d4fb819

Rename schnorrsig module to schnorr

760559c

Schnorr is commenly known as a signature algorithm, we don't need to restate that in the name of the module.

Rename schnorr functions on Secp256k1 to match naming of ecdsa

e0c3bb2

The naming scheme we employ is `{sign,verify, ...}_{ecdsa,schnorr}`.

Move ECDSA functionality into ECDSA module

ce44277

Move helper function below usage

c5c9551

This is not C89. We can declare the more important things first.

Fix typo in docs

d244b4d

thomaseizinger force-pushed the ecdsa-schnorr branch from 7eceb18 to d244b4d Compare November 11, 2021 02:44

dr-orlovsky approved these changes Nov 11, 2021

View reviewed changes

sanket1729 approved these changes Nov 11, 2021

View reviewed changes

tcharding reviewed Nov 11, 2021

View reviewed changes

apoelstra approved these changes Nov 12, 2021

View reviewed changes

Taproot automation moved this from In review to Ready for merge Nov 12, 2021

apoelstra merged commit 48683d8 into rust-bitcoin:master Nov 12, 2021

Taproot automation moved this from Ready for merge to Done Nov 12, 2021

sanket1729 mentioned this pull request Jan 8, 2022

Add lints to catch missing traits #364

Merged

real-or-random mentioned this pull request Mar 25, 2022

doc: Clarify configure flags for optional modules bitcoin-core/secp256k1#1094

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

thomaseizinger commented Sep 9, 2021

thomaseizinger commented Sep 9, 2021

real-or-random commented Sep 10, 2021

thomaseizinger commented Sep 10, 2021 •

edited

real-or-random commented Sep 13, 2021

thomaseizinger commented Sep 13, 2021

dr-orlovsky commented Sep 15, 2021

sanket1729 commented Sep 24, 2021

sanket1729 left a comment

sanket1729 Sep 24, 2021

thomaseizinger Sep 24, 2021

sanket1729 Sep 24, 2021

thomaseizinger Sep 24, 2021

dr-orlovsky commented Sep 27, 2021

Tibo-lg left a comment

thomaseizinger commented Oct 22, 2021

Tibo-lg commented Oct 22, 2021

sanket1729 left a comment

apoelstra left a comment

apoelstra commented Nov 2, 2021

thomaseizinger commented Nov 3, 2021

thomaseizinger commented Nov 3, 2021

dr-orlovsky left a comment •

edited

dr-orlovsky Nov 8, 2021

Tibo-lg Nov 9, 2021

thomaseizinger commented Nov 11, 2021

dr-orlovsky left a comment

sanket1729 left a comment

tcharding left a comment

tcharding Nov 11, 2021

thomaseizinger Nov 12, 2021

apoelstra left a comment

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

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

Conversation

thomaseizinger commented Sep 9, 2021

thomaseizinger commented Sep 9, 2021

real-or-random commented Sep 10, 2021

thomaseizinger commented Sep 10, 2021 • edited

real-or-random commented Sep 13, 2021

thomaseizinger commented Sep 13, 2021

dr-orlovsky commented Sep 15, 2021

sanket1729 commented Sep 24, 2021

sanket1729 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

Choose a reason for hiding this comment

dr-orlovsky commented Sep 27, 2021

Tibo-lg left a comment

Choose a reason for hiding this comment

thomaseizinger commented Oct 22, 2021

Tibo-lg commented Oct 22, 2021

sanket1729 left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Nov 2, 2021

thomaseizinger commented Nov 3, 2021

thomaseizinger commented Nov 3, 2021

dr-orlovsky left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger commented Nov 11, 2021

dr-orlovsky left a comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

tcharding left a 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

thomaseizinger commented Sep 10, 2021 •

edited

dr-orlovsky left a comment •

edited