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

disable mplex stream muxer #7689

Merged
merged 3 commits into from
Nov 29, 2021
Merged

disable mplex stream muxer #7689

merged 3 commits into from
Nov 29, 2021

Conversation

marten-seemann
Copy link
Contributor

@marten-seemann marten-seemann commented Nov 25, 2021

Compatibility considerations: The reason we have mplex in IPFS is that js-libp2p doesn't support yamux, so mplex is the fallback stream muxer there. We don't care about js-libp2p in lotus. As far as I know (someone with more ecosystem insight please double check!), all Filecoin implementations support yamux (at least Venus does), so there's no need for a fallback.

@marten-seemann marten-seemann requested a review from a team as a code owner November 25, 2021 17:39
@codecov
Copy link

codecov bot commented Nov 25, 2021

Codecov Report

Merging #7689 (134aee4) into master (ab55a6f) will decrease coverage by 0.08%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7689      +/-   ##
==========================================
- Coverage   39.56%   39.48%   -0.09%     
==========================================
  Files         637      637              
  Lines       67924    67909      -15     
==========================================
- Hits        26877    26811      -66     
- Misses      36445    36482      +37     
- Partials     4602     4616      +14     
Impacted Files Coverage Δ
node/builder.go 75.75% <ø> (ø)
node/modules/lp2p/smux.go 9.09% <25.00%> (+5.24%) ⬆️
node/modules/dtypes/mpool.go 87.50% <0.00%> (-12.50%) ⬇️
chain/stmgr/call.go 67.87% <0.00%> (-7.28%) ⬇️
chain/events/observer.go 71.64% <0.00%> (-6.72%) ⬇️
chain/sub/incoming.go 54.43% <0.00%> (-3.38%) ⬇️
extern/storage-sealing/fsm.go 55.77% <0.00%> (-2.79%) ⬇️
storage/wdpost_sched.go 75.49% <0.00%> (-1.97%) ⬇️
node/impl/full/mpool.go 25.21% <0.00%> (-1.74%) ⬇️
chain/gen/gen.go 68.61% <0.00%> (-1.24%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab55a6f...134aee4. Read the comment docs.

@jennijuju jennijuju requested review from whyrusleeping and removed request for a team November 25, 2021 17:51
@jennijuju jennijuju added this to the v1.13.2 milestone Nov 25, 2021
@vyzo
Copy link
Contributor

vyzo commented Nov 25, 2021

needs go mod tidy

go.sum Outdated Show resolved Hide resolved
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM. I like mplex as a simpler fallback, but that simplicity does have a cost which doesn't seem warranted here.

I expect we'll introduce separate "gateway" nodes eventually that might want to enable that transport, but we can cross that bridge later.

@Stebalien Stebalien merged commit 7971470 into master Nov 29, 2021
@Stebalien Stebalien deleted the disable-mplex branch November 29, 2021 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants