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

pull jonasnick wants to merge 2 commits into bitcoin-core:master from jonasnick:schnorrsig-stable changing 3 files +9 −27
  1. jonasnick commented at 11:54 am on October 20, 2021: contributor
    Fixes #992
  2. 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.
  3. real-or-random cross-referenced this on Oct 20, 2021 from issue Scope of the library by real-or-random
  4. sipa commented at 8:42 pm on October 20, 2021: contributor
    utACK c1b946cddfde16ab1e485303e4a96331100b6352
  5. 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:
  6. 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

  7. Rogelio165 approved
  8. 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.
  9. real-or-random cross-referenced this on Dec 21, 2021 from issue ci: Various improvements by real-or-random
  10. real-or-random cross-referenced this on Dec 21, 2021 from issue ci: Optimize build matrix by real-or-random
  11. 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…
  12. jonasnick force-pushed on Feb 24, 2022
  13. 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 :/

  14. jonasnick force-pushed on Mar 1, 2022
  15. real-or-random approved
  16. 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.

  17. 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…

  18. sipa commented at 3:54 pm on March 15, 2022: contributor
    I’m happy with either renaming or keeping things as they currently are.
  19. real-or-random cross-referenced this on Mar 16, 2022 from issue Schnorrsig API improvements by real-or-random
  20. 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.
  21. build: stop treating schnorrsig, extrakeys modules as experimental 80cf4eea5f
  22. jonasnick force-pushed on Mar 18, 2022
  23. README: mention that ARM assembly is experimental 7f09d0f311
  24. jonasnick commented at 1:34 pm on March 18, 2022: contributor
    Fixed the README issue.
  25. real-or-random approved
  26. real-or-random commented at 1:37 pm on March 18, 2022: contributor

    ACK 7f09d0f311117289719b690f91f6a907c2c6f3e2

    Let’s first get #1089 in.

  27. real-or-random referenced this in commit 1ac7e31c5b on Mar 24, 2022
  28. 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”.
  29. 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

  30. fanquake approved
  31. 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.
  32. real-or-random merged this on Mar 25, 2022
  33. real-or-random closed this on Mar 25, 2022
  34. robot-dreams cross-referenced this on Mar 25, 2022 from issue doc: Clarify configure flags for optional modules by robot-dreams
  35. fanquake referenced this in commit 465d05253a on Mar 30, 2022
  36. 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
  37. real-or-random referenced this in commit 6c0aecf72b on Apr 1, 2022
  38. fanquake referenced this in commit afb7a6fe06 on Apr 6, 2022
  39. jonasnick cross-referenced this on May 25, 2022 from issue Remove restriction that messages are exactly 32 bytes by jonasnick
  40. gwillen referenced this in commit 35d6112a72 on May 25, 2022
  41. hebasto cross-referenced this on Jul 21, 2022 from issue build: Add CMake-based build system by hebasto
  42. patricklodder referenced this in commit 21badcf9d2 on Jul 25, 2022
  43. patricklodder referenced this in commit 03002a9013 on Jul 28, 2022
  44. janus referenced this in commit 3a0652a777 on Aug 4, 2022
  45. str4d referenced this in commit 522190d5c3 on Apr 21, 2023
  46. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
  47. vmta referenced this in commit 8f03457eed on Jul 1, 2023


jonasnick real-or-random sipa t-bast siv2r Rogelio165 robot-dreams fanquake

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me