-
Notifications
You must be signed in to change notification settings - Fork 946
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
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
V1Lazy
for a listener in the negotiation, whereby the listener delays flushing of the multistream version header, is hereby removed. The only remaining effect ofV1Lazy
as compared toV1
is 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 permitsV1Lazy
dialers to be interoperable withV1
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 after sufficient experience we may eventually be bold enough to make this lazy dialer flush a part of the defaultV1
implementation, removing the dedicatedV1Lazy
version identifier. As an experiment I temporarily changed the default upgrade strategy toV1Lazy
for all substreams and at least all the tests and theipfs-kad
example worked fine.