build: stop treating schnorrsig, extrakeys modules as experimental

jonasnick commented at 11:54 am on October 20, 2021: contributor

Fixes #992

in README.md:22 in 99cdb5cc62 outdated

16@@ -17,9 +17,7 @@ Features:
17 * Suitable for embedded systems.
18 * Optional module for public key recovery.
19 * Optional module for ECDH key exchange.
20-* Optional module for Schnorr signatures according to [BIP-340](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki) (experimental).
21-
22-Experimental features have not received enough scrutiny to satisfy the standard of quality of this library but are made available for testing and review by the community. The APIs of these features should not be considered stable.

real-or-random commented at 12:47 pm on October 20, 2021:

ARM assembly is still experimental, so I wouldn’t remove this paragraph for now.

I see that we don’t mention it here in the list of features. Maybe you can add it to the list? Strictly speaking this is not in the scope of this PR but opening a separate PR seems overkill.

jonasnick commented at 1:35 pm on October 20, 2021:

It’s in the list of “implementation details”, so I added the sentence there.

real-or-random cross-referenced this on Oct 20, 2021 from issue Scope of the library by real-or-random

sipa commented at 8:42 pm on October 20, 2021: contributor

utACK c1b946cddfde16ab1e485303e4a96331100b6352

t-bast commented at 8:41 am on October 21, 2021: none

Concept ACK, if that could go with a 1.0 release soon afterwards it would be awesome :wink:

siv2r commented at 6:10 am on October 24, 2021: contributor

tACK c1b946c

Before this PR, ./configure --enable-module-extrakeys and ./configure --enable-module-schnorrsig give the error configure: error: extrakeys module is experimental. Use --enable-experimental to allow. - output file

After this PR, both of those commands compile successfully. - output file

Rogelio165 approved

real-or-random commented at 2:51 pm on October 28, 2021: contributor

I plan to review the APIs in the affected modules again before I ack this.

real-or-random cross-referenced this on Dec 21, 2021 from issue ci: Various improvements by real-or-random

real-or-random cross-referenced this on Dec 21, 2021 from issue ci: Optimize build matrix by real-or-random

real-or-random commented at 3:58 pm on February 24, 2022: contributor

Needs trivial rebase. Sorry that I was sitting on this for so long…

jonasnick force-pushed on Feb 24, 2022

real-or-random commented at 3:42 pm on March 1, 2022: contributor

Needs trivial rebase. Sorry that I was sitting on this for so long…

Again :/

jonasnick force-pushed on Mar 1, 2022

real-or-random approved

real-or-random commented at 9:10 pm on March 14, 2022: contributor

ACK cd7285800287563e2a12a56dadee779dbe2330d4

One thing that I noticed is that there’s no “natural counterpart” verification function for the normal signing function. Normal signing assumes 32 bytes but the verification function expects a length argument… But this is not a blocker. It’s at most related to “elegance” of the API, and we may not care at all.

real-or-random commented at 3:03 pm on March 15, 2022: contributor

One thing that I noticed is that there’s no “natural counterpart” verification function for the normal signing function. Normal signing assumes 32 bytes but the verification function expects a length argument… But this is not a blocker. It’s at most related to “elegance” of the API, and we may not care at all.

Ok, I’ve just discussed this with @jonasnick somewhere else and here are our thoughts:

The current API is based on the discussion in https://github.com/sipa/bips/issues/207 and the agreed plan there is to amend the BIP to accept variable length messages, and give recommendations for domain separation. (I think I promised to open a PR to the BIP and I failed – sorry, will move this the top of my list).

So taking this into account, the BIP340 signing and verifications algorithms (will) actually accept variable length messages. This means that for my above ACK comment that not the var-len _verify function is off but actually the _sign function is not perfectly aligned with the standard. A possible enhancement of our API is to rename schnorrsign_sign to schnorrsig_sign32 to emphasize it’s a specialization of BIP340 signing to 32-byte messages, and deprecate schnorrsign_sign. (I think we should not remove it given it’s used widely and perfectly safe. But there’s no need to have more than two (non-deprecated) signing functions. Callers who want variable length messages can call sign_custom with NULL. This is reasonable enough.

And @jonasnick points out that this way of thinking will also simplify advanced signing APIs, e.g., musig: Then the way to go for these is to provide only varlen functionality. This will save code and simplify these APIs by reducing the number of provided functions. For example, a call to musig_nonce_gen or similar functions is complex anyway, passing a “32” does not hurt.

Let me remind people that supporting varlen is almost entirely orthogonal to domain separation. People can get domain separation wrong even with an API that supports only 32-byte messages. @sipa Does this sound reasonable? If yes, do you think we should rename schnorrsign_sign to schnorrsig_sign32 and deprecate the former? If yes, we should do it now (quickly) before stabilization. I just don’t think we should hold this up for another month…

sipa commented at 3:54 pm on March 15, 2022: contributor

I’m happy with either renaming or keeping things as they currently are.

real-or-random cross-referenced this on Mar 16, 2022 from issue Schnorrsig API improvements by real-or-random

real-or-random commented at 10:44 am on March 17, 2022: contributor

See the README issue mentioned in #1091 (comment). Perfectly fine to solve this in a separate PR if you prefer, we just shouldn’t forget about it.

build: stop treating schnorrsig, extrakeys modules as experimental 80cf4eea5f

jonasnick force-pushed on Mar 18, 2022

README: mention that ARM assembly is experimental 7f09d0f311

jonasnick commented at 1:34 pm on March 18, 2022: contributor

Fixed the README issue.

real-or-random approved

real-or-random commented at 1:37 pm on March 18, 2022: contributor

ACK 7f09d0f311117289719b690f91f6a907c2c6f3e2

Let’s first get #1089 in.

real-or-random referenced this in commit 1ac7e31c5b on Mar 24, 2022

in README.md:73 in 7f09d0f311

70@@ -72,7 +71,6 @@ libsecp256k1 is built using autotools:
71 Usage examples
72 -----------
73   Usage examples can be found in the [examples](examples) directory. To compile them you need to configure with `--enable-examples`.

robot-dreams commented at 0:39 am on March 25, 2022:

Following these updated instructions only produces example_ecdsa. Should this also say something like:

The schnorr and ecdh modules aren’t enabled by default, so to compile the corresponding examples you will also need --enable-module-schnorr and --enable-module-ecdh.

real-or-random commented at 9:42 am on March 25, 2022:

Indeed. I think this can be done in a separate PR because seems to be a more general issue. At the moment, we don’t explain at all how to enable the modules. Maybe this should go to “build steps”.

robot-dreams commented at 0:44 am on March 25, 2022: contributor

Looks good, just one README comment

.cirrus.yml change looks OK: all CI checks passed, expected configurations were there (e.g. ASM:arm EXPERIMENTAL:yes), number of checks didn’t change (this PR and #1089 both had 66 checks each)

configure.ac change looks OK: searched for experimental and didn’t find anything else that needed changing

fanquake approved

fanquake commented at 7:27 am on March 25, 2022: member

ACK 7f09d0f311117289719b690f91f6a907c2c6f3e2 - When this is in, I think we’ll do a subtree update in Core, and prune some build cruft on our side.

real-or-random merged this on Mar 25, 2022

real-or-random closed this on Mar 25, 2022

robot-dreams cross-referenced this on Mar 25, 2022 from issue doc: Clarify configure flags for optional modules by robot-dreams

fanquake referenced this in commit 465d05253a on Mar 30, 2022

jonasnick cross-referenced this on Mar 30, 2022 from issue Upstream PRs 1064, 1049, 899, 1068, 1072, 1069, 1074, 1026, 1033, 748, 1079, 1088, 1090, 731, 1089, 995, 1094, 1093 by jonasnick

real-or-random referenced this in commit 6c0aecf72b on Apr 1, 2022

fanquake referenced this in commit afb7a6fe06 on Apr 6, 2022

jonasnick cross-referenced this on May 25, 2022 from issue Remove restriction that messages are exactly 32 bytes by jonasnick

gwillen referenced this in commit 35d6112a72 on May 25, 2022

hebasto cross-referenced this on Jul 21, 2022 from issue build: Add CMake-based build system by hebasto

patricklodder referenced this in commit 21badcf9d2 on Jul 25, 2022

patricklodder referenced this in commit 03002a9013 on Jul 28, 2022

janus referenced this in commit 3a0652a777 on Aug 4, 2022

str4d referenced this in commit 522190d5c3 on Apr 21, 2023

vmta referenced this in commit e1120c94a1 on Jun 4, 2023

vmta referenced this in commit 8f03457eed on Jul 1, 2023

build: stop treating schnorrsig, extrakeys modules as experimental #995