-
Notifications
You must be signed in to change notification settings - Fork 27
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
remove unused ls command #76
Conversation
0e4726f
to
ec4e518
Compare
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.
No objections here.
If I am not mistaken this pull request broke compatibility with rust-libp2p. According to the multistream-select specification implementations should support the rust-libp2p makes use of an optimization documented in the Listing section of the multistream-select specification. More concretely, when having more than 3 protocols available to negotiate, rust-libp2p first sends an I see two options:
I am leaning towards (1). Thoughts @marten-seemann @Stebalien @dignifiedquire? |
My vote would go for (1) as well. |
Should or SHOULD? ;) Having the protocol list in multistream seems like a weird layering violation. We have Identify to send our list of protocols already, so why waste a roundtrip for the |
go-multistream removed `ls` support in multiformats/go-multistream#76. This breaks compatibility with the multistream implementation in Rust see libp2p/rust-libp2p#2925. This commit updates the specification after the fact, explicitly making `ls` support optional.
As far as I can tell,
ls
is the main reason we're using a 64 kB buffer when reading tokens ingo-multistream/multistream.go
Lines 440 to 444 in 711a0aa
Given that this protocols is used by neither libp2p, IPFS or lotus, and we're in the process of replacing multistream with Protocol Select anyway, it seems safe to remove, especially given that this doesn't seem to be part of the spec.