Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scope of the library #997

Closed
real-or-random opened this issue Oct 20, 2021 · 18 comments
Closed

Scope of the library #997

real-or-random opened this issue Oct 20, 2021 · 18 comments
Labels
meta/development processes, conventions, developer documentation, etc.
Milestone

Comments

@real-or-random
Copy link
Contributor

I think we should have a documented policy on the scope of the library. The discussion comes up from time to time [0] and has never really been resolved, I think. We don't strictly need answers to these questions but I feel a written policy would avoid some discussions and bikeshedding in the future. And of course, a policy won't be written in stone, we can still change it if have a reason to do so.

One way to approach this is "features that are used by Bitcoin Core now or a have a good chance to be used by Bitcoin Core in the future". ECDH was somewhat of an exception here but maybe it will be used in the future for BIP324 (P2P encryption).

Another approach is to add features that are useful to the wider ecosystem. Of course, this does not mean that we need to accept every feature that meets this definition --- including features means that we commit on maintaining them.

I think with ECDH and Java bindings, the thinking here was more towards the latter approach. Personally I lean more towards the former idea because it avoids the need to take decisions about what to include, and it saves resources. (For example, Java bindings have been removed for a reason.)

A somewhat related discussion is the one about experimental features (#817). For example, if we view musig2 as something that can be experimented around with in a fork such as secp256k1-zkp but then plan on porting it to here [0], then what's the difference to something which can go in as an experimental module? Is musig2 "super-experimental", is it just not clear yet if it has a good chance of ending up in Bitcoin Core or is there just really no difference in the end?

Right now, I think there's a good chance that #995 will be merged, so if we wanted to deprecate experimental modules (as has been proposed in #817 but couldn't find consensus), now would be a good time.

[0] bitcoin/bitcoin#23326 (comment)

@sipa
Copy link
Contributor

sipa commented Oct 20, 2021

I lean more towards saying that the scope of the library is more than Bitcoin Core, though that does make the drawing the line somewhat fuzzier.

I think there is a place for experimental features, but they should actually be treated that way. E.g. there should be an actual expected migration to maturity, and we should aim for such features to actually not be in common use until they are mature. For example, bitcoin/bitcoin#23326 makes a good case for having a feature that's expected to be eventually widely used first in an experimental form so it can be, well, experimented with.

@jonasnick
Copy link
Contributor

I think "features that are used by Bitcoin Core now or a have a good chance to be used by Bitcoin Core in the future" is a good rule. Experimental modules should be deprecated.

We added MuSig to -zkp because there was no demand to add it into Core before bitcoin/bitcoin#23326. In light of that, it makes sense to add it to libsecp proper once it has been stabilized. Alternatively we could have a branch here with the MuSig2 implementation. The downside would be that our process of keeping libsecp and libsecp-zkp in sync would get quite a bit more difficult because -zkp only looks at libsecp master.

@sipa
Copy link
Contributor

sipa commented Oct 20, 2021

Hmm, my view would be that libsecp256k1-zkp exists to serve the Elements ecosystem, and libsecp256k1 exists to serve the Bitcoin ecosystem. That shouldn't just mean Bitcoin Core - it's a separate library for a reason - but we should probably document to what extent Bitcoin Core has a special place (if any), e.g. its use of experimental features (if any).

I don't think it would be a desirable outcome that people have to use libsecp256k1-zkp to get access to features that are actually widely used in Bitcoin (even if only outside Bitcoin Core). If they meet that bar, they're probably worth spending the review/productionizing cycles on to get them in libsecp256k1 proper.

The immediate bar for being a non-experimental module in libsecp256k1 should be "what we're willing to maintain as a stable API". But what we should be willing to maintain as a stable API should be prioritized based on features that are needed by the ecosystem.

An open question for me is what the bar should be for being added as an experimental module (again, if any).

@jonasnick
Copy link
Contributor

The appeal of serving Core only is that no one needs to decide what counts as important to the Bitcoin ecosystem and ensures that this continues to be a small and well-maintained library. That is also in the best interest of Core.

I don't see anything fundamentally wrong with people using libsecp-zkp if they want additional features, since -zkp is just libsecp with additional modules.

But perhaps that doesn't matter too much because it's hard to find an example for something that is widely used in Bitcoin but not in Core. At least for the MuSig example (ignoring bitcoin/bitcoin#2332) it would be difficult to imagine that it's widely used in the Bitcoin ecosystem but not in Core (perhaps there could be the case where it's used only in the Lightning ecosystem).

@real-or-random
Copy link
Contributor Author

Hmm, my view would be that libsecp256k1-zkp exists to serve the Elements ecosystem, and libsecp256k1 exists to serve the Bitcoin ecosystem.

That's a good point, and maybe that's the right view to see it. It's just not what's done in practice. -zkp started for the Elements ecosystem but now it's used as a place to get your Bitcoin schemes merged because (i) it's easier to get things merged there (is it?), and (ii) people may assume that our policy is to accept Core-only features (@jonasnick used exactly this reasoning above for musig! [1]) This again shows we should have a documented policy.

The two examples are ECDSA adaptor sigs and ECDSA sign-to-contract including anti-exfil signing with HW wallets. At least the latter thing is certainly relevant to Core as well.

Of course you could have those in another third fork secp256k1-beyondcore (with a better name obviously) but this would it even harder for users who want features from all three repos then, e.g., hardware wallets. It may be possible to solve this with a proper module system but this introduces a bunch of other problems. (Nevertheless I think we should support every change here that makes it easier for others to implement more modules in their repos.)

[1] The story with MuSig may be a little bit more involved. I assume with MuSig2 there's more demand in Core than there was with MuSig1, and given that MuSig1 is -zkp already, it made sense to PR MuSig2 there.

@sarvalabs-sai

This comment was marked as off-topic.

@real-or-random

This comment was marked as off-topic.

@sarvalabs-sai

This comment was marked as off-topic.

@sipa

This comment was marked as off-topic.

@sarvalabs-sai

This comment was marked as off-topic.

@sipa

This comment was marked as off-topic.

@sarvalabs-sai

This comment was marked as off-topic.

@sarvalabs-sai

This comment was marked as off-topic.

@michaelfolkson

This comment was marked as off-topic.

@sarvalabs-sai

This comment was marked as off-topic.

@michaelfolkson
Copy link

I agree with all the above. Intuitively restricting libsecp256k1 to just what is needed in Bitcoin Core seems unnecessarily restrictive but without that restriction the line gets increasingly fuzzy.

e.g.

Hmm, my view would be that libsecp256k1-zkp exists to serve the Elements ecosystem, and libsecp256k1 exists to serve the Bitcoin ecosystem. That shouldn't just mean Bitcoin Core - it's a separate library for a reason - but we should probably document to what extent Bitcoin Core has a special place (if any), e.g. its use of experimental features (if any).

I always considered Liquid a subset of the Bitcoin ecosystem like I consider Lightning a subset of the Bitcoin ecosystem. Applied to the libsecp256k1/libsecp256k1-zkp scope discussion that would mean everything in libsecp256k1-zkp that is active on Liquid would eventually make its way into libsecp256k1 and then libsecp256k1-zkp effectively becomes just a staging ground for libsecp256k1. That seems unnecessarily loose to me.

But perhaps that doesn't matter too much because it's hard to find an example for something that is widely used in Bitcoin but not in Core. At least for the MuSig example (ignoring bitcoin/bitcoin#2332) it would be difficult to imagine that it's widely used in the Bitcoin ecosystem but not in Core (perhaps there could be the case where it's used only in the Lightning ecosystem).

The alternative to defining a clear scope is to just take it case by case. MuSig2 in Lightning provides some interesting case studies.

  1. MuSig2 could be used in Lightning for "recursive" or nested MuSig2 which wouldn't be a Lightning protocol issue (other than supporting Schnorr signatures). Some Lightning implementations might choose to support it while others may not.

  2. Changing the 2-of-2 multisig that is funded by both counterparties when opening the channel to a single pubkey with MuSig2 would be a Lightning protocol issue.

My view would be that supporting the Lightning protocol in 2) would make MuSig2 a no brainer for libsecp256k1. I'm unsure on whether supporting individual Lightning implementations in 1) and not necessarily the Lightning protocol would be a strong enough argument for libsecp256k1.

@jonasnick
Copy link
Contributor

As @sipa @real-or-random and I discussed in person, we could document the scope in the README.md or a (to be added) CONTRIBUTING.md. Similar to the scope, we should mention the minimum requirements for new schemes considered for the library. They include a clear specification and high confidence in the scheme's security. We should also document that developers should reach out to the library devs (IRC, ...) before working on a new scheme to avoid disappointment when it turns out to not fulfill the requirements.

@jonasnick jonasnick added this to the 0.4.1 or 0.5.0 milestone Sep 25, 2023
@real-or-random real-or-random added the meta/development processes, conventions, developer documentation, etc. label Nov 13, 2023
real-or-random added a commit that referenced this issue Dec 7, 2023
0e5ea62 CONTRIBUTING: add some coding and style conventions (Jonas Nick)
1a432cb README: update first sentence (Jonas Nick)
0922a04 docs: move coverage report instructions to CONTRIBUTING (Jonas Nick)
76880e4 Add CONTRIBUTING.md including scope and guidelines for new code (Jonas Nick)

Pull request description:

  Following offline discussions, this PR documents the scope of the library and the requirements for adding new modules. I think this fixes most of #997. It also updates the README very slightly.

  In addition, I added some coding conventions that I remembered explaining to new contributors in the past year. Even though it's far from exhaustive, I think this is an easy improvement to the CONTRIBUTING.md. Feel free to suggest more conventions.

ACKs for top commit:
  sipa:
    ACK 0e5ea62
  real-or-random:
    ACK 0e5ea62

Tree-SHA512: ffdbab22982fd632de92e81bd135f141ac86e24cc0dcfc0e1ae12b0d2a2e4f91377ab2c0cc440cb919889eaed8bfc1447b880fa1430fd771b956f2af0fe3766e
@real-or-random
Copy link
Contributor Author

we should have a documented policy on the scope of the library

This has been resolved by #1431. Discussions on how to interpret the policy can be discussed on a case-by-case basis. Further discussions about concretizing the scope further or changing the scope could happen in separate issues and PRs, or here in this issue, in which case we can reopen it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta/development processes, conventions, developer documentation, etc.
Projects
None yet
Development

No branches or pull requests

6 participants
@sipa @real-or-random @jonasnick @michaelfolkson @sarvalabs-sai and others