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

Don't build secp256k1 by default. #2146

Closed
wants to merge 2 commits into from
Closed

Conversation

dvc94ch
Copy link
Contributor

@dvc94ch dvc94ch commented Jul 18, 2021

any features required to make a network behaviour work will be specified manually. for testing it is common to just add libp2p to the dev-dependencies to get a development-transport. This PR disables all the features that aren't required for building a development transport.

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Jul 18, 2021

Personally, I always use rust-libp2p with default-features = false. Given the additive nature of Cargo's features, I would suggest to go even one step further and have everything off by default. To make things like the development_transport easier to use, we could have a dedicated feature for that (testing for example) that gives you those things.

@dvc94ch dvc94ch force-pushed the no-secp256k1 branch 3 times, most recently from 8cc0080 to 825220a Compare July 19, 2021 10:40
@mxinden
Copy link
Member

mxinden commented Jul 20, 2021

I am fine with removing secp256k1 from the default feature set.

I would suggest to go even one step further and have everything off by default.

I see the default features as the equivalent to batteries included, allowing newcomers to get started without having to tinker with any cargo features. After all, getting started with rust-libp2p is already hard enough. I assume that advanced users will set default-features = false as a first optimization step either way. With that in mind, I am not in favor of "everything off by default".

@thomaseizinger
Copy link
Contributor

I assume that advanced users will set default-features = false as a first optimization step either way.

That works if your crate is the only one pulling in libp2p. The annoying thing is that once turned on, features in a dependency graph can never be turned off again. Thus, if you are pulling in a dependency that already depends on libp2p and that dependency did not turn off all default features, you are somewhat screwed.

Now, how often this case happens is debatable :)

But by having all features turned off by default, this is much less likely to happen. I really like how tokio approached this from version 1 onwards: selective features so you can pick what you need. If you don't want to bother, just enable full.

Regarding batteries included: The batteries are still there, you just need to plug them in :P

There is certainly potential for making things easier for newcomers. Cargo features and conditional compilation are a fact of life in the Rust world though, it is not like rust-libp2p is doing something completely different here.

At the risk of derailing the discussion completely: One thing that IMO we could do much better at is supporting different executors. For example, at the moment it possible to use tcp-tokio with async-std-dns. That doesn't make a whole lot of sense though. It would be nicer if the user would pick features such as: tokio, dns, tcp and tokio forwards to libp2p-tcp/tokio and libp2p-dns/tokio while tcp just enables libp2p-tcp without an executor. Generally, it has always been a bit of an annoyance to use rust-libp2p with the tokio runtime without pulling in all of the async-std dependencies. It is possible but the configuration always felt very fragile, especially because of the default choices.

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Jul 21, 2021

I am fine with removing secp256k1 from the default feature set.

I agree with @thomaseizinger, although I think this would already be an improvement as it pulls in three crates for a feature that is likely not used very often. So to turn your suggestion into a rule would be the following if I understand correctly:

  • protocols are enabled by default for easy discovery but other optional features aren't

@mxinden
Copy link
Member

mxinden commented Jul 22, 2021

I am torn.

On the one hand, also after reading through tokio-rs/tokio#1811, I like the idea of an empty default and an additional full which would enable all features. Users would use full if they just don't care or are just getting started and hand-pick their features otherwise. Missing a specific feature would simply surface as a compile time error which is easy to fix.

On the other hand there are special features, like secp256k1. Say I am a user that uses ed25519 for their own key, though still wanting to support remote peers using secp256k1. Accidentally not enabling the secp256k1 feature would not surface as a compile time error.

Do you know of more crates that follow the empty default and additional full pattern?

In the end, I think I am fine with either way, as long as:

  • It is well documented.
  • It is well communicated via corresponding CHANGELOG.md entries.
  • It is consistent across the whole rust-libp2p repository.

Thanks for all the input!

@thomaseizinger
Copy link
Contributor

Say I am a user that uses ed25519 for their own key, though still wanting to support remote peers using secp256k1.

I firmly believe that requirements like this have to come with a test, preferably e2e. At that point, we don't have a compile error yes, but we will fail in the next bigger feedback loop: testing.

In the end, I think I am fine with either way, as long as:

  • It is well documented.
  • It is well communicated via corresponding CHANGELOG.md entries.
  • It is consistent across the whole rust-libp2p repository.

That is exciting!

@dvc94ch Unless you are happy to do it, I am fine with taking on these tasks given that I brought up this idea. Let me know your preference!

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Jul 23, 2021

Sorry I haven't gotten around to it yet. But if you I'm happy to take you up on your offer. I'll see if I can add tls to libp2p-quic this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants