Experimental features should be deprecated #817

1. 
   rustyrussell commented at 9:57 pm on September 15, 2020: contributor

   If a feature is genuinely experimental, keep it in a separate branch.

   Otherwise, it will be compiled in, as there may be a user. This becomes inevitable in larger systems: in particular, c-lightning uses both libsecp and libwally. The latter includes libsecp, and you can’t have two libraries with the same symbols, so c-lightning needs to use the libwally one. Packaging systems fix this, but they have the same problem: they must configure in everything.

   You can have boutique builds that omit features for embedded use, sure. But the default will be kitchen sink.

   I know it’s hard: labelling something experimental is a good way of avoiding commitment. But it is a mere fig leaf which doesn’t work, and mature libraries don’t get that luxury.

   If you’re not prepared to commit to an API (ideally an ABI too), you simply cannot include something. In that case, it needs to be a separate library (and if it gets folded in later, all the names have to change!).

2. 
   real-or-random commented at 10:42 pm on September 15, 2020: contributor

   If a feature is genuinely experimental, keep it in a separate branch.

   I think this would make it easier to develop large features, e.g.. Schnorr signatures. Currently our model (and that of Bitcoin Core) is to have large atomic changes in a single PR developed by a single party who addresses review comments over and over. And reviewers keep reviewing the same stuff because people don’t want to commit and the PR takes forever to be merged. I believe it’s easier to develop large changes in a separate feature branch (or fork), but then create small PRs against this feature branch to avoid that PRs become huge, and later create a single large PR against master. Some things are simply too large to be handled in a single PR.

   This is what Bitcoin Core is effectively doing with secp256k1.

   (Sorry, this is somewhat off-topic.)

3. 
   sipa commented at 11:28 pm on September 15, 2020: contributor

   I think there are a few somewhat related issues here.

   “experimental” used to not mean very much

   This library wasn’t well maintained for a long time, and that led to a few things remaining marked experimental for far longer than they should have been. This has improved lately, and ECDH (which was effectively stable for years) is about to be marked non-experimental (see #809). I think the ARM assembly will in the near future become non-experimental as well, but I’d like to see CI support for it.

   Beyond those two, the only remaining experimental feature is the recently-merged BIP340 support. It is experimental for two reasons:

   • There are a few discussions still ongoing that may impact the API (variable-length message support, batch validation) without changes to the (currently supported) part of the scheme. It’s not impossible to just add new functions for these once they’re worked out, but given the next point, I think this may not be needed.
   • It’s very hard to be sure what exact semantics of BIP340 will actually activate on Bitcoin (or if it will at all), and if that ends up being different from what is implemented now, eventually this library will want to implement the final scheme. I think changes are very unlikely at this point, but it’s also not exactly under our control.

   This is all to say: I think our recent notion of what “experimental” is narrower now than what it has been for a long time, and for what is left, there are specific criteria for letting them graduate - not an open-ended noncommitment.

   If you can’t commit to an API, you can’t include it.

   I disagree here. There are certainly packaging/compatibility complexities (see next point), but there shouldn’t be a reason why software that uses a pinned version of their dependencies (through subtrees, submodules, commit hashes in dependency systems, …) cannot use an experimental API. It works, it’s tested and reviewed, and it’s included in the library so that things can be developed against it - they just have to be prepared to, for now, adapt to API changes when updating.

   As for leaving things in a branch… perhaps that is an approach, but may make it harder to start developing software that builds on it.

   I think experimental feature just means you can only use it in a controlled environment.

   Linking issues when dependencies need overlapping features

   This is a hard problem, though it’s not restricted to experimental-ness - any kind of optionality in features can cause this problem. It’s even worse when there are downstream projects that add extra functionality (e.g. secp256k1-zkp; what if you need the rangeproofs from there, but also BIP340 support from the current upstream master, while -zkp isn’t rebased on top of that?).

   I feel that perhaps at some point it will be needed to support a compile-time option to add some prefix to all exported symbol names.

   (Non-experimental) features should be default-on when possible

   Ignoring experimental features - whether you agree with them or not - perhaps this is the main point you’re trying to convey? When there are stable features in a library that are optional, they will be enabled in generic builds, and even when pinned within projects due to the need to provide for multiple indirect dependencies (e.g. libwally as you point out).

   So that’s an argument to make them default-on, or even non-optional, to minimize the potential configuration combinations that end users may end up with.

   Perhaps ECDH is a good candidate to make default to on now because of that. I’m less certain about recovery, as it’s something that (I personally think) should be discouraged from being relied on in new systems (potential patent concerns, and aggregation schemes being more powerful in general).

4. 
   rustyrussell commented at 5:31 am on September 16, 2020: contributor

   So that’s an argument to make them default-on, or even non-optional, to minimize the potential configuration combinations that end users may end up with.

   Yes. All optional features will get turned on for the vast majority of users. If that’s not safe, we need to seriously consider whether the feature should be included at all. If we simply don’t like the feature, then we should state those reasons clearly in documentation to discourage new users.

   And of course we can have a multitude of cut-down flags to eliminate parts of the library for specialized envs.

   I think experimental feature just means you can only use it in a controlled environment.

   If experimental means “API unstable”, then I’m afraid it hasn’t met the bar for inclusion in a mature library. Which is totally fine: make it a separate library. When the result has matured sufficiently, include it in libsecp256k1.

   Otherwise you’re not a mature library: this is part of the commitment which is reasonably expected from libraries, and it’s not arbitrary. Breaking an API means pain and suffering for downstream projects; please don’t call a library version 1.0 and then try to change the API.

5. 
   sipa commented at 6:18 am on September 16, 2020: contributor

   Yes. All optional features will get turned on for the vast majority of users. If that’s not safe, we need to seriously consider whether the feature should be included at all. If we simply don’t like the feature, then we should state those reasons clearly in documentation to discourage new users.

   That seems very reasonable.

   If experimental means “API unstable”, then I’m afraid it hasn’t met the bar for inclusion in a mature library. Which is totally fine: make it a separate library. When the result has matured sufficiently, include it in libsecp256k1.

   What do you mean by separate library?

   • Something completely independent that only implements BIP340? I think that would be enormous waste of effort in duplication of internals, review, gaining confidence, setting up testing infrastructure, …
   • A separately maintained branch of secp256k1 that adds BIP340? That would be somewhat less duplication, but make things unnecessarily difficult for developing things on top, and when combined with other “feature branches” like -zkp in one application, result in the exact same linker issues you were complaining about in the first place.

   It’s far from ideal, but I think having a feature that’s just temporarily in an experimental state is a good middle road. It gets the code changes in mainline, lets testing happen on interactions between the feature and other things being developed, signifies the code is reviewed and tested, lets downstream extensions rebase on it, make it easy for projects that pin the version to start developing on top of without conflicts, … but also means some changes can still happen, and users will be aware of this.

   please don’t call a library version 1.0 and then try to change the API.

   libsecp256k1 hasn’t released a version 1.0.

6. 
   rustyrussell commented at 7:30 am on September 16, 2020: contributor

   We can’t have both “we want people to start using this feature” and “sorry, there’s no API stability”; they are opposed in practice.

   I think having a feature that’s just temporarily in an experimental state is a good middle road.

   Well, in the BIP340 case there’s a clear path (since it will presumably become non-experimental once BIP340 is merged into bitcoin-core?). I still prefer it as a separate library, which would not have been as bad if it had started that way (note: we can share non-ABI-stable internals between two dependent libraries, which lets us do most things sanely). Not worth it now, but I don’t know what other changes we’ll see in future.

   I’m arguing for weaning off the existing ones, and never adding another. For example, it seems (from rough reading) recovery could be moved out to a separate library if there are patent concerns which means it should never be a “core” thing.

   libsecp256k1 hasn’t released a version 1.0.

   Yes. But I hope to see it one day!

7. 
   real-or-random commented at 8:49 am on September 16, 2020: contributor

   It’s far from ideal, but I think having a feature that’s just temporarily in an experimental state is a good middle road. It gets the code changes in mainline, lets testing happen on interactions between the feature and other things being developed, signifies the code is reviewed and tested, lets downstream extensions rebase on it, make it easy for projects that pin the version to start developing on top of without conflicts, … but also means some changes can still happen, and users will be aware of this.

   please don’t call a library version 1.0 and then try to change the API.

   libsecp256k1 hasn’t released a version 1.0. @sipa Having experimental modules is hard, I’m concerned that it becomes even harder after we release a 1.0. We have two kinds of users: 1) projects that pin a commit (primarily Bitcoin Core but also others) and 2) projects that want a general library.

   Now what will happen for the latter? Distributions tend to enable all features because it makes sense to do so, (related: #675) and I’m not sure if we can convince them from not enabling experimental features. Somebody will need the experimental feature, so distros will enable it.

   Assume we have something like semver. What kind of release is an API break in an experimental module? Major release? Then there is no difference to normal modules. Minor release? Then users of experimental modules will need to pin version numbers, which creates all kinds of issues in dependency managers etc.

   The only way I see is to exclude experimental features from the versioning, discourage distros from enabling experimental features, and recommend that users who need experimental modules should vendor the library/pin a commit. (And put all the blame on them if they don’t stick to the policy.)

   It’s far from ideal, but I think having a feature that’s just temporarily in an experimental state is a good middle road. It gets the code changes in mainline, lets testing happen on interactions between the feature and other things being developed, signifies the code is reviewed and tested, lets downstream extensions rebase on it, make it easy for projects that pin the version to start developing on top of without conflicts, … but also means some changes can still happen, and users will be aware of this.

   Would a single experimental branch be more difficult to work with? You can test interactions, downstream extensions can rebase on it, and projects can pin commits of the experimental branch. You sadi that a separate branch will make things unnecessarily difficult for developing things on top. Can you explain how? @rustyrussell

   For example, it seems (from rough reading) recovery could be moved out to a separate library if there are patent concerns which means it should never be a “core” thing.

   Recovery is not an experimental module. And it’s actually used within Bitcoin Core, so we need this. (But I get what you’re saying.)

8. 
   gmaxwell commented at 3:56 pm on September 16, 2020: contributor

   Experimental features just don’t have any promises. Breaking the interface for one isn’t any kind of API break because no API commitment was made at all. It may not be possible to design a really good and safe API for something without actually also attempting to implement the something in an application and getting feedback from how it integrates. While that is going on there needs to be a mechanism to coordinate with the new application(s).

   They could, for sure, be in a separately forked copy of the library– but this software exists as a freestanding library rather than a directory in bitcoin-core in part because it was believed it would be easier maintained this way.

   I think this choice has, for whatever reason, panned out somewhat poorly, resulting in a outright dangerous lack of attention and– at times– a near total inability to move things forward (although lately it’s been better). It has also not resulted in a particularly large amount of of adoption by other users (with a few exceptions, e.g. libwally/c-lightning)– instead obviously provably less secure alternatives are ubiquitous in hardware wallets, as an example. If the library was further split into more separately maintained things I don’t believe an outcome other than abandonment is likely.

   Similarly, there have been features such as ECDH where holding them to the safety and integrity standard used in the library would have simply precluded their development as before they existed had zero usage to justify the hundreds of man hours of review and testing needed to mature them. Without the ability to have unsupported/test components the response to attempt to develop such things would be “don’t bother”. Again, it could be developed somewhere else, but I suspect that would just end up forgotten and abandoned (or else, all development would move to that, leaving this side abandoned and just renaming the library you’re taking issue with).

   I don’t follow this separate library discussion. This library is a single compilation unit, it would be a single file library except its nicer to maintain split into modules. The overall design depends pretty heavily on inlining, and none of the internal symbols are exported at all. Exposing the internals directly would be contrary to the design principle of only providing safe, supported, “complete cryptosystem” interfaces as much as possible.

   Having a configurable prefix– as is pretty common other crypto libraries that are mostly copied rather than linked– would probably be pretty useful. The symbol naming is already pretty well set up for it because everything is already prefixed “secp256k1_”, so that could just be the default prefix. Some thought would need to be given on how to handle the typedefs.

   As far as “supported but you probably shouldn’t use it in new things”, perhaps: Rename the relevant functions something with deprecated in them, keep the original name as a wrapper but with __attribute((deprecated)) set on it. Existing callers will change the name of their call to silence the warnings, thus getting notified.

   And it’s actually used within Bitcoin Core, so we need this.

   It’s used only by “signmessage” – nowhere by the bitcoin protocol. And signmessage is increasingly useless because it doesn’t work with the address types mostly being used by users today, and without using the same addresses there is almost no justification for using signmessage over, say, PGP or signify… I think it’s likely that bitcoin would (eventually) drop signmessage if any of the several efforts to make an alternative that worked more generally were finished.

9. real-or-random cross-referenced this on Sep 17, 2020 from issue Add static assertion that uint32_t is unsigned int or wider by real-or-random
10. 
    real-or-random commented at 3:25 pm on September 17, 2020: contributor

    (oops I mentioned the wrong issue in my PR, please ignore the mention above this comment.)
11. 
    luke-jr commented at 2:54 pm on September 18, 2020: member

    Maybe libsecp256k1 can/should install a libsecp256k1.so and libsecp256k1_experimental.so, and pkg-config files for each module to autoselect which library to include?
12. 
    real-or-random commented at 3:31 pm on September 18, 2020: contributor

    @luke-jr That’s an interesting idea but I wonder if this really helps. I don’t think we would want to ship a pkg-config for experimental features. (What’s the point in checking for some version of an unstable API?)
13. 
    rustyrussell commented at 4:59 am on September 20, 2020: contributor

    @gmaxwell Actually, libsecp is becoming widely used; every lightning implementation uses it, for example. And the schnorr stuff is likely to complete this dominance, since it’s just Damn Nice for any new uses.

    Similarly, there have been features such as ECDH where holding them to the safety and integrity standard used in the library would have simply precluded their development as before they existed had zero usage to justify the hundreds of man hours of review and testing needed to mature them.

    That… doesn’t help me. I’m using it. I do not treat it any differently from the rest of libsecp256k1. If it had, at least, been a separate library I might have (for example, auditing the code). You’re certainly implying that I should have!

    This library is a single compilation unit, it would be a single file library except its nicer to maintain split into modules. The overall design depends pretty heavily on inlining, and none of the internal symbols are exported at all. Exposing the internals directly would be contrary to the design principle of only providing safe, supported, “complete cryptosystem” interfaces as much as possible.

    But another library could share most of this inlining, and I disagree that exposing (say) secp256k1_internal* symbols would break the design, if it came to that.

    WRT deprecating things, it generally implies a pending removal (and timeline here should probably be >= 24 months). I wouldn’t start a deprecation unless there’s a clear, documented path to removal. Sometimes it’s better to simply label “/* THIS IS A NASTY FUNCTION, HERE FOR LEGACY USE CASES. Please use xxx instead and make all of our lives better */”.

14. 
    luke-jr commented at 12:49 pm on September 20, 2020: member

    (What’s the point in checking for some version of an unstable API?)

    To make sure the user has that correct version and get the CFLAGS/etc correct for it… Same as with a stable API.

15. 
    laanwj commented at 11:30 am on October 17, 2020: member

    I think the ARM assembly will in the near future become non-experimental as well, but I’d like to see CI support for it.

    Speaking of which, we need to do some benchmarking of it. At the time of writing GCC was generating quite horrible code for ARM with a plenty of stack spilling. Back then it was an impressive improvement. But it’s very possible that in the meantime, GCC has grown “good enough” to get close to, or even beat the manually implemented code. (and now there is #815 too that changes affected functions)

    I do still have ARM32 hardware in use (i.MX6) so might give it at try.

16. 
    gmaxwell commented at 4:15 am on October 18, 2020: contributor

    I was recently benchmarking gcc 9.2 on i.MX6 and the code you wrote was still a massssiiiivvvveee speedup. :) (if you dig through the safegcd thread you can see some numbers w/o asm on 32-bit arm)
17. ysangkok cross-referenced this on Nov 1, 2020 from issue Provide pkg-config packages for optional modules by ysangkok
18. real-or-random cross-referenced this on Feb 1, 2021 from issue Add doc/release-process.md by jonasnick
19. jonasnick cross-referenced this on Jul 7, 2021 from issue Add release-process.md by jonasnick
20. real-or-random cross-referenced this on Oct 19, 2021 from issue Stabilization of schnorrsig by real-or-random
21. real-or-random cross-referenced this on Oct 19, 2021 from issue Enable non-experimental modules by default by real-or-random
22. real-or-random cross-referenced this on Oct 20, 2021 from issue Scope of the library by real-or-random
23. real-or-random referenced this in commit 423b6d19d3 on Dec 25, 2021
24. real-or-random referenced this in commit 2286f80902 on Nov 22, 2022

Contributors
rustyrussell real-or-random sipa gmaxwell luke-jr laanwj