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

refactor(network): use new libp2p SwarmBuilder #4695

Closed
wants to merge 4 commits into from

Conversation

mxinden
Copy link

@mxinden mxinden commented Sep 3, 2023

Proposed Changes

libp2p/rust-libp2p#4120 introduces a new SwarmBuilder simplifying the composition of transports and behaviours into a Swarm.

This commit showcases the usage of the new SwarmBuilder in lighthouse.

Additional Info

Note that libp2p/rust-libp2p#4120 is not yet merged. Thus this pull request is just here to proof that libp2p/rust-libp2p#4120 works for lighthouse.

//CC @thomaseizinger

libp2p/rust-libp2p#4120 introduces a new `SwarmBuilder` simplifying the
composition of transports and behaviours into a `Swarm`.

This commit showcases the usage of the new `SwarmBuilder` in lighthouse.
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

let (swarm_builder, bandwidth) =
libp2p::SwarmBuilder::with_existing_identity(local_keypair.clone())
.with_tokio()
// Using `with_other_transport` in order to support mplex. In case mplex would not be needed, the below would be as simple as:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How hard would it be to support a custom encryption protocol in the builder?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean a custom multiplexer? Possible. But I don't expect us to write a new multiplexer for TCP any time soon. Thus I would rather like to add such option once it is needed.

In this particular case, Ethereum is planning to move away from mplex. See ethereum/consensus-specs#3490

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean a custom multiplexer?

Yes.

But I don't expect us to write a new multiplexer for TCP any time soon. Thus I would rather like to add such option once it is needed.

But it would support these usecases and allow us to eventually deprecate the other upgrade path. I really do not want us to introduce more ways of doing things without deprecating old ones.

@AgeManning
Copy link
Member

Awesome thanks!

I guess we wait for the new builder to land right?

@AgeManning AgeManning changed the base branch from stable to unstable September 6, 2023 07:13
@thomaseizinger
Copy link

I guess we wait for the new builder to land right?

Yes, this PR is just to validate that we are on the right pathway with the implementation :)

@mxinden
Copy link
Author

mxinden commented Oct 11, 2023

With libp2p/rust-libp2p#4120 merged, this pull request is now updated. Note that the new SwarmBuilder i.e. latest libp2p master is not yet released. Thus keeping this pull request as draft.

Comment on lines +376 to +386
let mut mplex_config = libp2p_mplex::MplexConfig::new();
mplex_config.set_max_buffer_size(256);
mplex_config
.set_max_buffer_behaviour(libp2p_mplex::MaxBufferBehaviour::Block);
mplex_config
},
|| {
let mut yamux_config = libp2p::yamux::Config::default();
yamux_config
.set_window_update_mode(libp2p::yamux::WindowUpdateMode::on_read());
yamux_config

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mxinden Can you create issues for making these chainable?

Comment on lines +375 to +381
|| {
let mut mplex_config = libp2p_mplex::MplexConfig::new();
mplex_config.set_max_buffer_size(256);
mplex_config
.set_max_buffer_behaviour(libp2p_mplex::MaxBufferBehaviour::Block);
mplex_config
},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could simplify this a lot by making a free-function that returns you the ready to use config, then you could just reference it.

Comment on lines +395 to +396
// TODO: Websocket should be optional.
.with_websocket(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought websocket was removed with the introduction of QUIC? I thought I saw something like that in @AgeManning's QUIC PR.

bors bot pushed a commit that referenced this pull request Oct 19, 2023
## Issue Addressed

updates libp2p to the latest version and uses the new `SwarmBuilder`. Superseeds #4695
CC @mxinden I don't think we can use both `bandwidth_loggers` with the new syntax right?
@jxs
Copy link
Member

jxs commented Oct 19, 2023

Superseeded by #4864 thanks @mxinden!

@jxs jxs closed this Oct 19, 2023
jxs added a commit to jxs/lighthouse that referenced this pull request Nov 4, 2023
updates libp2p to the latest version and uses the new `SwarmBuilder`. Superseeds sigp#4695
CC @mxinden I don't think we can use both `bandwidth_loggers` with the new syntax right?
@jxs jxs mentioned this pull request Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants