-
Notifications
You must be signed in to change notification settings - Fork 279
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
discussion: should we remove Noise Pipes from the noise-libp2p spec? #249
Comments
I'll rephrase and provide some perspective to check if I've understood correctly.
Conclusion: IK makes sense if we're sending arbitrary application data we want the other party to be able to process IMMEDIATELY, and even respond to in their handshake response. Since that's not the case here -- as the client needs to intersect its capabilities with the responder's in all cases -- we can simplify the handshake if we assume the pipelining is always available and supported, such that the client can always append its muxer selection + first stream open + first app message to the third and and final message of XX. |
I'm in favour of removing it, at least for the time being. I've stated similar reasons for why |
@yusefnapora on reading the spec, it's not clear to me which part is optional: IK or Noise Pipes? Regardless, the spec doesn't propose a good strategy to deal with this optionality.
|
@raulk the way the spec is currently written, if you don't support Noise Pipes, you also don't support The idea is that the non-Pipes peer always does Anyway, the worst-case scenario in the current spec is always a 1.5-RTT handshake. We never terminate the connection or start over from scratch; the initiator's first message is always used. If we do remove Noise Pipes, then we would get rid of |
In terms of RTTs, even if pipelining (sending the muxer selection, etc., along with the final handshake message) isn't supported, the initiator should be able to send another message without waiting yet another round trip, right? |
@Stebalien yep 👍 I posted my analysis of the practical issues that motivated us to reconsider Noise Pipes support, to begin with. Now, as for the decision, I am in favour of simplifying the first version of this handshake by making it support only XX. Reasons:
|
@raulk sound good to me - I'll make a PR to go-libp2p-noise to remove Pipes and update the spec. |
I am closing here since #260 removed noise pipes and the IK handshake from the specification. |
Context: #246 is currently blocking a correct implementation of Noise Pipes.
The issue is resolvable, but @Stebalien and I had a sync chat just now and are wondering if Noise Pipes is worth the cost in terms of complexity.
In our design for Noise Pipes, the
IK
handshake is 1-RTT, not 0-RTT. While we can send early data in the handshake payload, we can't really trust that data until the handshake completes. Since we're not planning to send arbitrary application data in the early payload, we have to wait for the handshake to complete before handing the conn off to the app layer.From a client's perspective, the 0.5 RTT difference isn't meaningful. If a client does an
IK
handshake, they need to receive one handshake message from the server before they send their first transport message.With
XX
they still only need to receive one message from the server, because the client can send their finalXX
handshake message immediately followed by transport messages containing their actual data.Since in the majority of cases, it's the client who wants to send the first data after connection establishment, most connections won't benefit from using
IK
in practice.Given that the benefits aren't that amazing, it's worth considering if we should shelve Noise Pipes, and just use
XX
as our only handshake. Doing so would remove a lot of complexity from the spec and implementations, and we could always revisit in a new version if we decide that the extra 0.5 RTT is worth it after all.noise interest group: @raulk, @tomaka, @romanb, @shahankhatch, @Mikerah, @djrtwo, @dryajov, @mpetrunic, @AgeManning, @morrigan, @araskachoi, @mhchia
The text was updated successfully, but these errors were encountered: