search

multiformats / go-multistream

an implementation of the multistream protocol in go
MIT License
40 stars 27 forks source link

remove unused ls command #76

Closed marten-seemann closed 2 years ago

marten-seemann commented 2 years ago

As far as I can tell, ls is the main reason we're using a 64 kB buffer when reading tokens in https://github.com/multiformats/go-multistream/blob/711a0aaf37fdd541271c6c829083c59247219108/multistream.go#L440-L444.

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.

mxinden commented 1 year ago

If I am not mistaken this pull request broke compatibility with rust-libp2p.

According to the multistream-select specification implementations should support the ls command. Or at least it does not specify the ls command as optional. Please correct me in case I am missing something.

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 ls and then proceeds with the protocol that the remote speaks. See https://github.com/libp2p/rust-libp2p/issues/2925 for details (//CC @dignifiedquire).

I see two options:

  1. Remove the above optimization in rust-libp2p and make ls support explicitly optional in the mulistream-select specification.
  2. Reintroduce ls support in go-libp2p.

I am leaning towards (1). Thoughts @marten-seemann @Stebalien @dignifiedquire?

dignifiedquire commented 1 year ago

My vote would go for (1) as well.

marten-seemann commented 1 year ago

According to the multistream-select specification implementations should support the ls command.

Should or SHOULD? ;) To me it’s unclear if this is a required or an optional feature (which is why we SHOULD use more RFC 2119 terminology in our specs).

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 ls? So (1) would be my preference as well.