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

The future of development_transport #3657

Closed
thomaseizinger opened this issue Mar 22, 2023 · 9 comments · Fixed by #4120
Closed

The future of development_transport #3657

thomaseizinger opened this issue Mar 22, 2023 · 9 comments · Fixed by #4120
Labels
help wanted priority:important The changes needed are critical for libp2p, or are blocking another project

Comments

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Mar 22, 2023

Motivation

In the early days of rust-libp2p, a development_transport was added to make it easier on getting started with development. There are multiple issues with it:

  1. Naming: It suggests that it is only while developing which isn't necessarily the case.
  2. Flavors: The async-std flavor is called development_transport and the tokio flavor is called tokio_development_transport. Those are not aligned.
  3. Discoverability: It is hard to discover these functions because they are free functions at the module root. IDE auto-complete doesn't suggest them if you are trying to create a Swarm.
  4. Scope: It is unclear, which features the transport should contain. All available transports / muxers etc or just a set of "recommended" ones.

In my opinion, it is useful to have a "default" transport that contains a recommended set of transports and muxers. In my eyes, those are:

  • TCP
  • DNS
  • yamux
  • noise
  • TLS

Websocket as well as relay are purposely left out as I'd consider those too niche. WebRTC and QUIC are still in alpha state. Mplex is considered deprecated and we shouldn't encourage it.

Suggestion

There are multiple ways of fixing this problem. In #3635 (comment), I sketched a feature that incorporates a default_transport into the SwarmBuilder. This would be very easy to discover but it would also mean that we would require optional dependencies from libp2p-swarm to the various transport and muxer crates.

Alternatively, we might be able to attach functions via an extension trait that lives in libp2p but I am not sure if we can create a good, type-system driven builder then.

Are you planning to do it yourself in a pull request?

No.

@thomaseizinger thomaseizinger added priority:important The changes needed are critical for libp2p, or are blocking another project help wanted labels Mar 22, 2023
@drHuangMHT
Copy link
Contributor

drHuangMHT commented Mar 30, 2023

I'm in favor of the default_transport naming. But TLS requires certificates to be properly configured, which is kind of tricky.

@thomaseizinger
Copy link
Contributor Author

In my opinion, it is useful to have a "default" transport that contains a recommended set of transports and muxers. In my eyes, those are:

* TCP

* DNS

* yamux

* noise

* TLS

Websocket as well as relay are purposely left out as I'd consider those too niche. WebRTC and QUIC are still in alpha state. Mplex is considered deprecated and we shouldn't encourage it.

A different option might be to activate all transports that are compiled into the application, i.e. activated through the cargo features. This might need some cfg trickery to get right but would result in a quite nice user experience.

@thomaseizinger
Copy link
Contributor Author

A different option might be to activate all transports that are compiled into the application, i.e. activated through the cargo features. This might need some cfg trickery to get right but would result in a quite nice user experience.

I am not sure that is a good idea. Functions shouldn't change their behaviour when features are activated or deactivated as features are unified across the dependency graph. It would be very surprising to users if suddenly they are exposing a transport because they are pulling in a different library that happens to activate another feature.

@thomaseizinger
Copy link
Contributor Author

Started a PoC here: #3792

@thomaseizinger
Copy link
Contributor Author

Documenting an out-of-band discussion:

The issue with the development_transport and anything that follows its rough design are:

  • Users cannot configure individual transports. It is an all or nothing approach. This is bad because it means that really simple usecases are simple but any slight change in configuration forces users to use the complex composition of transports that we have today.
  • Free functions are hard to discover. Users come in with the goal of building a Swarm and are "stopped" by having to provide a transport with little (type-system) guidance on how to proceed.

In addition, the problem with composing the transports manually is that you have to know the nitty-gritty details about the order of composing noise/tls, multiplexer etc.

We can fix both issues by providing something like the following (type-safe) builder pattern on SwarmBuilder:

SwarmBuilder::new(behaviour, identity)
   .with_tokio_executor()
   .with_default_tcp() # or .with_tcp(|config| config.no_delay(true))
   .with_noise()
   .with_tls()
   .with_yamux()
   .build()

A few things that are interesting about this API:

  • Simple as well as complex usecases use the same API, e.g. configuring TCP nodelay does not require a complete change in composing transports
  • Complexity of how to compose transports is up to us
  • Footguns, such as DNS needing to be the most outer transports can be solved by us
  • Enforce that the local peer id is the same as the keypair used in the transports
  • We can automatically choose the correct transport based on the executor with a type-safe builder pattern

What we need to solve are:

  • Should we make the builder type-safe by enforcing that choosing TCP requires choosing a multiplexer and encryption protocol?
  • How do we allow users to compose a custom transport (like libp2p-tor)?

@zenitogr
Copy link

what is the status of this issue? I am trying to run the file sharing example and I am getting that there is no development_transport in libp2p!

@thomaseizinger
Copy link
Contributor Author

what is the status of this issue? I am trying to run the file sharing example and I am getting that there is no development_transport in libp2p!

The status is that we need to figure out a flexible yet easy to use design that serves several usecases.

The above discussion is my latest thinking with the only addition being that the new SwarmBuilder will have to live in libp2p instead of libp2p-swarm for dependency reasons.

mergify bot pushed a commit that referenced this issue Apr 21, 2023
Due to cargo's feature unification, a full build of our workspace doesn't actually check whether the examples compile as standalone projects.

We add a new CI check that iterates through all crates in the `examples/` directory and runs a plain `cargo check` on them. Any failure is bubbled up via `set -e`, thus failing CI in case one of the `cargo check` commands fails.

To fix the current failures, we construct a simple TCP transport everywhere where we were previously using `development_transport`. That is because `development_transport` requires `mplex` which is now deprecated.

Related #3657.
Related #3809.

Pull-Request: #3811.
@mxinden
Copy link
Member

mxinden commented Jun 25, 2023

I tried to tackle this over the weekend, that is an interactive TransportBuilder, though I hit a wall in every direction I explored. The root of the problem is the cardinality explosion, i.e. the number of different scenarios one needs to handle. E.g. when providing options for tcp, encryption (noise, tls, noise & tls), dns, websocket, relay, one already has 48 different scenarios. Each of these 48 scenarios results in a unique type, which only on the final step is put into a uniform box, i.e. a transport::Boxed. Thus e.g. match arms and if statements need to deeply nest, box on the final step and then return. On top of all of that come Rust feature sets.

  • Using transport::Boxed on each sub-transport makes some of this easier, though arguably ugly and maybe less performant.

  • Leveraging the trait system quickly gets out of hand, just in the number of trait parameters on each builder step.

  • Using declarative macros thus far resulted in a mess for me. There are some transports that need to be ORed (tcp and websocket), there are some transports that need to be wrapped (tcp by dns), there are some transports that need to be first (websocket), there are some transports that need to be upgraded (tcp, relay, websocket), while others (in the future quic and webrtc) do not.

I have to give this more thought. Maybe someone has a neat trick. Hope this is not too much discouragement.

@thomaseizinger
Copy link
Contributor Author

Maybe the following works:

  1. Make cfg'd functions based on enabled features that return either a DummyTransport / DummyUpgrade or the actual transport / upgrade. This is for noise, TLS etc
  2. Unconditionally compose all these functions, pretending as if we'd have all features activated.

This may help with the cardinality because it results in a single SelectUpgrade between TLS and noise for example.

Only issue could be the configs of the various transports / upgrades. Those types are not available when the features are off. Perhaps we need to change that?

@mergify mergify bot closed this as completed in #4120 Oct 10, 2023
mergify bot pushed a commit that referenced this issue Oct 10, 2023
Introduce the new `libp2p::SwarmBuilder`. Users should use the new `libp2p::SwarmBuilder` instead of the now deprecated `libp2p::swarm::SwarmBuilder`. See `libp2p::SwarmBuilder` docs on how to use the new builder.

Fixes #3657.
Fixes #3563.
Fixes #3179.

Pull-Request: #4120.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted priority:important The changes needed are critical for libp2p, or are blocking another project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants