-
Notifications
You must be signed in to change notification settings - Fork 976
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
feat(libp2p): add SwarmBuilder
#4120
Conversation
//CC @DougAnderson444 since this is related to https://github.com/DougAnderson444/libp2peasy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work Max!
Before you continue supporting more transports, I'd appreciate some exploration on how this can be turned into a SwarmBuilder
.
Specifically, we should make it possible to pass the identity in there but also generate a new one and then pass it to the Swarm
as well.
- Allow providing or generating identity - Use V1Lazy - Make NetworkBehaviour constructor take relay Behaviour in case with_relay was called - Rename to AuthenticatedMultiplexedTransport
Thank you for the input @thomaseizinger. What do you think of the most recent commit? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking very nice already. I'd say that is worth a blog-post once we ship it :)
Some good content for TWIR!
I suggest shipping a minimal Builder at first. Users that need advanced features can use the `with_other_transport` method or build a `Transport` without the builder.
- Make tls and noise configurable. - Prevent accidental behavior change through feature change. E.g. enabling "noise" feature should not make ones transport automatically offer noise. Instead one needs to explicitly call `with_noise` and `without_noise`.
In regards to the recent comments, happy to add shortcut methods to the various |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Support for QUIC draft 29 was removed with libp2p#4467. libp2p#4120 reintroduced it as a faulty merge. This commit removes it again.
interop-tests/src/arch.rs
Outdated
.with_websocket(tls::Config::new, mplex::MplexConfig::default) | ||
.with_websocket(noise::Config::new, mplex::MplexConfig::default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once more, great catch @thomaseizinger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interop tests made this one easy :)
Thank you @thomaseizinger for the last commits and the reviews in general. As always, very helpful. |
Version bump missed in libp2p#4120.
# Description Fixes #5996 https://github.com/libp2p/rust-libp2p/releases/tag/libp2p-v0.53.0 https://github.com/libp2p/rust-libp2p/blob/master/CHANGELOG.md ## Integration Nothing special is needed, just note that `yamux_window_size` is no longer applicable to libp2p (litep2p seems to still have it though). ## Review Notes There are a few simplifications and improvements done in libp2p 0.53 regarding swarm interface, I'll list a few key/applicable here. libp2p/rust-libp2p#4788 removed `write_length_prefixed` function, so I inlined its code instead. libp2p/rust-libp2p#4120 introduced new `libp2p::SwarmBuilder` instead of now deprecated `libp2p::swarm::SwarmBuilder`, the transition is straightforward and quite ergonomic (can be seen in tests). libp2p/rust-libp2p#4581 is the most annoying change I have seen that basically makes many enums `#[non_exhaustive]`. I mapped some, but those that couldn't be mapped I dealt with by printing log messages once they are hit (the best solution I could come up with, at least with stable Rust). libp2p/rust-libp2p#4306 makes connection close as soon as there are no handler using it, so I had to replace `KeepAlive::Until` with an explicit future that flips internal boolean after timeout, achieving the old behavior, though it should ideally be removed completely at some point. `yamux_window_size` is no longer used by libp2p thanks to libp2p/rust-libp2p#4970 and generally Yamux should have a higher performance now. I have resolved and cleaned up all deprecations related to libp2p except `BandwidthSinks`. Libp2p deprecated it (though it is still present in 0.54.1, which is why I didn't handle it just yet). Ideally Substrate would finally [switch to the official Prometheus client](paritytech/substrate#12699), in which case we'd get metrics for free. Otherwise a bit of code will need to be copy-pasted to maintain current behavior with `BandwidthSinks` gone, which I left a TODO about. The biggest change in 0.54.0 is libp2p/rust-libp2p#4568 that changed transport APIs and enabled unconditional potential port reuse, which can lead to very confusing errors if running two Substrate nodes on the same machine without changing listening port explicitly. Overall nothing scary here, but testing is always appreciated. # Checklist * [x] My PR includes a detailed description as outlined in the "Description" and its two subsections above. * [x] My PR follows the [labeling requirements]( https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process ) of this project (at minimum one label for `T` required) * External contributors: ask maintainers to put the right label on your PR. --- Polkadot Address: 1vSxzbyz2cJREAuVWjhXUT1ds8vBzoxn2w4asNpusQKwjJd --------- Co-authored-by: Dmitry Markin <[email protected]>
# Description Fixes #5996 https://github.com/libp2p/rust-libp2p/releases/tag/libp2p-v0.53.0 https://github.com/libp2p/rust-libp2p/blob/master/CHANGELOG.md ## Integration Nothing special is needed, just note that `yamux_window_size` is no longer applicable to libp2p (litep2p seems to still have it though). ## Review Notes There are a few simplifications and improvements done in libp2p 0.53 regarding swarm interface, I'll list a few key/applicable here. libp2p/rust-libp2p#4788 removed `write_length_prefixed` function, so I inlined its code instead. libp2p/rust-libp2p#4120 introduced new `libp2p::SwarmBuilder` instead of now deprecated `libp2p::swarm::SwarmBuilder`, the transition is straightforward and quite ergonomic (can be seen in tests). libp2p/rust-libp2p#4581 is the most annoying change I have seen that basically makes many enums `#[non_exhaustive]`. I mapped some, but those that couldn't be mapped I dealt with by printing log messages once they are hit (the best solution I could come up with, at least with stable Rust). libp2p/rust-libp2p#4306 makes connection close as soon as there are no handler using it, so I had to replace `KeepAlive::Until` with an explicit future that flips internal boolean after timeout, achieving the old behavior, though it should ideally be removed completely at some point. `yamux_window_size` is no longer used by libp2p thanks to libp2p/rust-libp2p#4970 and generally Yamux should have a higher performance now. I have resolved and cleaned up all deprecations related to libp2p except `BandwidthSinks`. Libp2p deprecated it (though it is still present in 0.54.1, which is why I didn't handle it just yet). Ideally Substrate would finally [switch to the official Prometheus client](paritytech/substrate#12699), in which case we'd get metrics for free. Otherwise a bit of code will need to be copy-pasted to maintain current behavior with `BandwidthSinks` gone, which I left a TODO about. The biggest change in 0.54.0 is libp2p/rust-libp2p#4568 that changed transport APIs and enabled unconditional potential port reuse, which can lead to very confusing errors if running two Substrate nodes on the same machine without changing listening port explicitly. Overall nothing scary here, but testing is always appreciated. # Checklist * [x] My PR includes a detailed description as outlined in the "Description" and its two subsections above. * [x] My PR follows the [labeling requirements]( https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process ) of this project (at minimum one label for `T` required) * External contributors: ask maintainers to put the right label on your PR. --- Polkadot Address: 1vSxzbyz2cJREAuVWjhXUT1ds8vBzoxn2w4asNpusQKwjJd --------- Co-authored-by: Dmitry Markin <[email protected]>
# Description Fixes #5996 https://github.com/libp2p/rust-libp2p/releases/tag/libp2p-v0.53.0 https://github.com/libp2p/rust-libp2p/blob/master/CHANGELOG.md ## Integration Nothing special is needed, just note that `yamux_window_size` is no longer applicable to libp2p (litep2p seems to still have it though). ## Review Notes There are a few simplifications and improvements done in libp2p 0.53 regarding swarm interface, I'll list a few key/applicable here. libp2p/rust-libp2p#4788 removed `write_length_prefixed` function, so I inlined its code instead. libp2p/rust-libp2p#4120 introduced new `libp2p::SwarmBuilder` instead of now deprecated `libp2p::swarm::SwarmBuilder`, the transition is straightforward and quite ergonomic (can be seen in tests). libp2p/rust-libp2p#4581 is the most annoying change I have seen that basically makes many enums `#[non_exhaustive]`. I mapped some, but those that couldn't be mapped I dealt with by printing log messages once they are hit (the best solution I could come up with, at least with stable Rust). libp2p/rust-libp2p#4306 makes connection close as soon as there are no handler using it, so I had to replace `KeepAlive::Until` with an explicit future that flips internal boolean after timeout, achieving the old behavior, though it should ideally be removed completely at some point. `yamux_window_size` is no longer used by libp2p thanks to libp2p/rust-libp2p#4970 and generally Yamux should have a higher performance now. I have resolved and cleaned up all deprecations related to libp2p except `BandwidthSinks`. Libp2p deprecated it (though it is still present in 0.54.1, which is why I didn't handle it just yet). Ideally Substrate would finally [switch to the official Prometheus client](paritytech/substrate#12699), in which case we'd get metrics for free. Otherwise a bit of code will need to be copy-pasted to maintain current behavior with `BandwidthSinks` gone, which I left a TODO about. The biggest change in 0.54.0 is libp2p/rust-libp2p#4568 that changed transport APIs and enabled unconditional potential port reuse, which can lead to very confusing errors if running two Substrate nodes on the same machine without changing listening port explicitly. Overall nothing scary here, but testing is always appreciated. # Checklist * [x] My PR includes a detailed description as outlined in the "Description" and its two subsections above. * [x] My PR follows the [labeling requirements]( https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process ) of this project (at minimum one label for `T` required) * External contributors: ask maintainers to put the right label on your PR. --- Polkadot Address: 1vSxzbyz2cJREAuVWjhXUT1ds8vBzoxn2w4asNpusQKwjJd --------- Co-authored-by: Dmitry Markin <[email protected]>
Description
Introduce the new
libp2p::SwarmBuilder
. Users should use the newlibp2p::SwarmBuilder
instead of the now deprecatedlibp2p::swarm::SwarmBuilder
. Seelibp2p::SwarmBuilder
docs on how to use the new builder.Fixes #3657.
Fixes #3563.
Fixes #3179.
Notes & open questions
Change checklist