[multistream] Make the lazy variant more interoperable. #1855
Conversation
The remaining optimisation for `V1Lazy` for a listener in the negotiation, whereby the listener delays flushing of the multistream version header, is hereby removed. The remaining effect of `V1Lazy` is only on the side of the dialer, which delays flushing of its singular protocol proposal in order to send it together with the first application data (or an attempt is made to read from the negotiated stream, which similarly triggers a flush of the protocol proposal). This permits `V1Lazy` dialers to be interoperable with `V1` listeners. The remaining theoretical pitfall whereby application data gets misinterpreted as another protocol proposal by a listener remains, however unlikely. `V1` remains the default, but we may eventually risk just making this lazy dialer flush a part of the default `V1` implementation, removing the dedicated `V1Lazy` version identifier.
| /// > listener should either be known to support all of the dialer's optimistically chosen | ||
| /// > protocols or there is must be no intermediate protocol without a payload and none of | ||
| /// > the protocol payloads must have the potential for being mistaken for a multistream-select | ||
| /// > protocol message. This avoids rare edge-cases whereby the listener may not recognize |
There was a problem hiding this comment.
Let me know if I understand correctly:
If I create a protocol named /foo and the first message sent after opening a /foo substream is /bar\nhello world, then, if the listener doesn't support protocol /foo, it will misinterpret the payload as the dialer opening the protocol /bar and sending hello world.
There was a problem hiding this comment.
Yes, see multiformats/go-multistream#20 for more details (that link is also in the code).
| if *v == version { | ||
| *this.state = State::Expecting { io, protocol, version }; | ||
| match (&msg, version) { | ||
| (Message::Header(Version::V1), Some(Version::V1)) => { |
There was a problem hiding this comment.
The fact that we assume that version can never be V1Lazy looks a bit like a hack to me.
In an ideal world, we would differentiate the Version enum (containing V1 and V1Lazy) from the VersionHandshake (containing only V1), no?
There was a problem hiding this comment.
The fact that we assume that version can never be V1Lazy looks a bit like a hack to me.
By definition of these versions, they're now indistinguishable on the wire and it is always expected to receive V1. So receiving V1Lazy is considered an error (in the decoder really).
In an ideal world, we would differentiate the Version enum (containing V1 and V1Lazy) from the VersionHandshake (containing only V1), no?
Since V1Lazy is now really the same negotiation protocol as V1, only differing subtly in the behaviour of the dialer, it may be cleaner to separate this "lazy flush yes/no" setting from the Version, making it a separate configuration option for the dialer only, since V1Lazy used for a listener now has no effect at all. However, for transport and substream upgrades we currently only have this choice of Version without a concept of separate parameters for the roles of dialer and/or listener. Maybe Version::V1 { dialer_lazy_flush: bool } would already be an improvement, though be another API change. It would always be decoded from the wire with dialer_lazy_flush: false since that option is local. Should I give that a try? So due to the fact that this would require larger API changes, together with my hope that V1Lazy will eventually disappear altogether, merging its behaviour into V1, led me to the current approach, but I'm open to alternatives.
There was a problem hiding this comment.
It would always be decoded from the wire with dialer_lazy_flush: false since that option is local.
What I had in mind is that you wouldn't decode a Version, but a new enum called for example VersionHandshake that would only contain V1 without any parameter.
I don't know if that makes sense, and I'm also not strongly opinionated. I'm fine with these changes.
There was a problem hiding this comment.
What I had in mind is that you wouldn't decode a Version, but a new enum called for example VersionHandshake that would only contain V1 without any parameter.
I see, separating these two kinds of versions. I will give that a try.
There was a problem hiding this comment.
My proposal is 70f041b.
Every multistream-select version maps to a specific header line, but there may be different variants of the same multistream-select version using the same header line, i.e. the same wire protocol.
Closes #1853. The remaining optimisation for
V1Lazyfor a listener in the negotiation, whereby the listener delays flushing of the multistream version header, is hereby removed. The only remaining effect ofV1Lazyas compared toV1is on the side of the dialer, which delays flushing of its singular protocol proposal (i.e. if it has no alternatives) in order to send it together with the first application data, e.g. a request, or an attempt is made to read from the negotiated stream, which similarly triggers a flush of the pending protocol proposal, as before. This permitsV1Lazydialers to be interoperable withV1listeners. The remaining theoretical pitfall whereby application data gets misinterpreted as another protocol proposal by a listener remains, however unlikely.V1remains the default, but after sufficient experience we may eventually be bold enough to make this lazy dialer flush a part of the defaultV1implementation, removing the dedicatedV1Lazyversion identifier. As an experiment I temporarily changed the default upgrade strategy toV1Lazyfor all substreams and at least all the tests and theipfs-kadexample worked fine.