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

chore: support yamux to wherever mplex is supported #1579

Merged
merged 18 commits into from
May 5, 2023

Conversation

maschad
Copy link
Member

@maschad maschad commented Feb 4, 2023

Closes #1578

@maschad
Copy link
Member Author

maschad commented Feb 4, 2023

It may be wise to merge this after #1533 is merged due to potential conflicts

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

Stream muxers are negotiated in the order in which they are configured - if mplex is first and both peers support mplex, only mplex will be used.

Yamux appears to be configured behind mplex everywhere here so it's not actually going to be used in the tests.

@maschad
Copy link
Member Author

maschad commented Feb 14, 2023

Stream muxers are negotiated in the order in which they are configured - if mplex is first and both peers support mplex, only mplex will be used.

Is there any particular reason for enforcing this? I know that Yamux should be used over mplex so should we default to Yamux and allow Mplex to be configured in the case where a consumer wants to provide backward-compatibility with legacy nodes?

@p-shahi
Copy link
Member

p-shahi commented Feb 14, 2023

Stream muxers are negotiated in the order in which they are configured - if mplex is first and both peers support mplex, only mplex will be used.

Is there any particular reason for enforcing this? I know that Yamux should be used over mplex so should we default to Yamux and allow Mplex to be configured in the case where a consumer wants to provide backward-compatibility with legacy nodes?

I think @achingbrain is saying that it should be reversed: i.e. streamMuxers: [mplex(),yamux()], should be streamMuxers: [yamux(),mplex()],

@maschad maschad mentioned this pull request Mar 7, 2023
@maschad maschad requested a review from achingbrain March 7, 2023 01:00
@p-shahi p-shahi mentioned this pull request Mar 14, 2023
@BigLep
Copy link
Contributor

BigLep commented Mar 31, 2023

@maschad : what are the next steps here to complete this? Does ChainSafe/js-libp2p-yamux#26 need to get landed? I see it as marked as draft. What do we need to to for it to get reviewed?

This work isn't urgent is important. It seems like every month in IPFS-land we hit bumps around still needing to fallback to mplex because yamux hasn't been rolled out more broadly or at least isn't on by default in new versions.

@maschad
Copy link
Member Author

maschad commented Mar 31, 2023

Yes @BigLep that's correct. I have some remaining work on ChainSafe/js-libp2p-yamux#26 to ensure it's throwing the correct error when the stream resets, before it can be reviewed. I will prioritize that this week.

@BigLep
Copy link
Contributor

BigLep commented Mar 31, 2023

Thanks @maschad . I'll defer to @p-shahi on overall priorities as he has better lay of the land and certainly universal connectivity is the key thing to get ahead on. I just want to make sure this doesn't languish. Thanks a lot!

@maschad maschad removed the request for review from mpetrunic April 18, 2023 01:22
@maschad maschad force-pushed the chore/support-yamux-where-mplex branch from efdf585 to 79e82d5 Compare April 26, 2023 23:44
@maschad maschad self-assigned this Apr 27, 2023
@maschad
Copy link
Member Author

maschad commented May 1, 2023

Once ChainSafe/js-libp2p-yamux#26 is merged we can upgrade to 4.0.1 and then this can be merged This is ready for re-review.

@p-shahi
Copy link
Member

p-shahi commented May 5, 2023

Should this be included in #1707 @achingbrain

@achingbrain achingbrain merged commit 59f34a2 into libp2p:master May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support Yamux where Mplex is supported
4 participants