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.

Sign up for GitHub

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

Jump to bottom

feat(multistream-select): Use Version::V1Lazy by default #3563

Closed
mxinden opened this issue Mar 7, 2023 · 5 comments · Fixed by #4120
Closed

feat(multistream-select): Use Version::V1Lazy by default #3563

mxinden opened this issue Mar 7, 2023 · 5 comments · Fixed by #4120
Labels
getting-started Issues that can be tackled if you don't know the internals of libp2p very well help wanted

Comments

@mxinden
Copy link
Member

mxinden commented Mar 7, 2023 •
edited by thomaseizinger
Loading

Description

Current default is Version::V1. There is no practical downside to using Version::V1Lazy. We should use it as a default.

See #3749 for a previous attempt with some useful information on how to proceed.

Motivation

0 RTT protocol negotiation when only one protocol is available.

Current Implementation

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

Maybe
@mxinden mxinden added difficulty:easy help wanted getting-started Issues that can be tackled if you don't know the internals of libp2p very well labels Mar 7, 2023
@mxinden mxinden mentioned this issue Mar 7, 2023
Merged
4 tasks
@tcoratger tcoratger mentioned this issue Apr 9, 2023
Closed
2 tasks
@mxinden
Copy link
Member Author

mxinden commented Apr 12, 2023

Documenting an out-of-band discussion with @thomaseizinger. We might as well be able to drop Version::V1 and thus all of Version and always do V1Lazy. Worth exploring.

@MarcoPolo
Copy link
Contributor

Am I correct in my understanding that without this, streams pay a 1 RT penalty right now in rust-libp2p, and thus not cheap? cc @mxinden

@thomaseizinger
Copy link
Contributor

Am I correct in my understanding that without this, streams pay a 1 RT penalty right now in rust-libp2p, and thus not cheap? cc @mxinden

At the moment, users need to configure that they want the 0RTT behaviour. Removing this configuration option and simply always doing it is what this issue is about.

@mxinden mxinden mentioned this issue Jun 20, 2023
Merged
4 tasks
@thomaseizinger thomaseizinger mentioned this issue Sep 12, 2023
Merged
4 tasks
@thomaseizinger
Copy link
Contributor

I've tagged #4120 as closing this one because the config option will no longer be available to users through this builder.

@thomaseizinger
Copy link
Contributor

@mxinden Given that #4120 will make this the default for a lot of users, do you mind attempting to add the test that we discussed in #3749 just to be sure that we don't break anyone unnecessarily?

@mergify mergify bot closed this as completed in #4120 Oct 10, 2023
mergify bot pushed a commit that referenced this issue Oct 10, 2023
@mxinden
feat(libp2p): add SwarmBuilder
d605255 
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.
mxinden added a commit to mxinden/rust-libp2p that referenced this issue Oct 26, 2023
@mxinden
feat(multistream-select): remove Version
a4e8d80 
Removes multistream-select version selection. Instead the mechanism of `Version::V1Lazy` is always
used.

See also libp2p#3563 and
libp2p#3749.
@mxinden mxinden mentioned this issue Oct 26, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
getting-started Issues that can be tackled if you don't know the internals of libp2p very well help wanted
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(libp2p): add SwarmBuilder mxinden/rust-libp2p
3 participants
@MarcoPolo @thomaseizinger @mxinden