Skip to content
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

Go Back to Single Packet Number Space #1579

Closed
nibanks opened this issue Jul 17, 2018 · 14 comments
Closed

Go Back to Single Packet Number Space #1579

nibanks opened this issue Jul 17, 2018 · 14 comments
Labels
-tls -transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.

Comments

@nibanks
Copy link
Member

nibanks commented Jul 17, 2018

I think we should go back to a single packet number space, and allow for higher encryption levels/epochs to acknowledge packets sent/received in lower levels/epochs.

I have been working on the draft -13 updates for the last week or so, and having completely independent packet number spaces causes a lot of complexity. On the sender side, it requires separate loss detection logic per packet number space, though you still have one congestion control and recovery modules for the entire connection. Resolving the intersection of those two is a headache. On the receiver side, you need a separate tracker for all packets you have received in a given packet number space. At least in my implementation, that comes with a good bit more state than just a list/array of packet numbers. Then when it goes time to frame the acks for all these packet numbers, it's a mess to keep track of which types of packets need to be built, based on the data (both ACKs and CRYPTO) currently queued up.

The argument has been made, at least on the sender side, to use one contiguous set of packet numbers across all encryption levels/epochs. Yes, that does simply the loss detection logic somewhat, but doesn't help the receive and ACK side of things. Additionally, even if we were to mandate that behavior for everyone, the requirement that you ACK a packet in the same encryption level still causes a lot more complexity than the previous logic of always acknowledging in packets in the same (or greater) encryption level. It was a lot easier to implement the previous ACK model design, IMO.

Finally, for debugging purposes, it is really messy to see the same packet numbers, but at different encryption levels. It makes understanding exactly what's going on that much harder. It is a lot more natural to see packet numbers always increase for every packet send/received, independent of what encryption level is used.

Having all this extra complexity for just the handshake really seems like overkill. I understand and am all for the separate encryption levels, but I really think we should go back to a single packet number space and allow for ACK frames in higher encryption levels.

@nibanks
Copy link
Member Author

nibanks commented Jul 17, 2018

Additionally, I would like to go as far as saying Initial packets MUST be acknowledged in higher encryption levels (or implicitly). Then, the server would only have to handle Initial packets from the client that are for the initial CRYPTO payload. Otherwise, imposing the 1200 byte limit requires decrypting the packet.

@ekr
Copy link
Collaborator

ekr commented Jul 17, 2018 via email

@ianswett
Copy link
Contributor

If we do that, I believe we have to reopen an issue(#1018) Christian pointed out a while back, you can't tell what encryption level an acknowledgement is for, so injecting an unencrypted packet can create a hole that's very difficult to detect and you never end up recovering from.

Additionally, by acking at the highest level, it's fairly easy to send an ack the peer can't process, which is a rather nasty issue.

We'd also have to fix #1413 a different way, but that's not overly difficult, just a bit ugly.

Given the QUIC WG meeting is tomorrow morning, I'm not sure how much we can talk about this, but possibly we could get some feedback from those who've implemented it.

I've started mine, but haven't finished FYI.

@nibanks
Copy link
Member Author

nibanks commented Jul 17, 2018

@ekr I have heard you voice this experience before. Why do you feel acknowledging a packet in the same or higher level encryption level is harder?

The simplest way I see to implement it is the following: Along with the set of packet numbers to acknowledge, keep track of the highest encryption level you have received. Then, only send ACK frames in that encryption level or higher. On the receive side, to validate it, you have to keep a few bits to track which encryption level a packet was received. When that packet gets acknowledged, just validate the level.

@ianswett for #1018 does requiring ACK frames to be sent in Handshake encryption level or higher solve the problem?

@ekr
Copy link
Collaborator

ekr commented Jul 17, 2018 via email

@nibanks
Copy link
Member Author

nibanks commented Jul 17, 2018

  1. Reserve 2 bits per sent packet tracking structure for encryption level. I'd bet whatever structure you have already has 2 free bits.
  2. When processing an ACK frame, pass along the encryption level the frame was received in. Use that in your OnPacketAcked function to validate.

That logic was a LOT simpler than the logic I have to maintain for framing all my ACKs in the different packet levels right now.

@ianswett
Copy link
Contributor

Re #1018, removing ACKs from Initially encrypted solves that problem. But then if we need multi-packet ClientHellos or ServerHellos(ie: post-quantum), we're stuck with no fast recovery.

@nibanks
Copy link
Member Author

nibanks commented Jul 17, 2018

I'd argue quite a few things will likely have to change for multi-packet hellos and holding those off for a later QUIC version would be fine. Right now, they are single packet only. Additionally, implicit acknowledgements, based on handshake state could help out a lot with these issues as well.

@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -transport -tls labels Jul 18, 2018
@nibanks
Copy link
Member Author

nibanks commented Jul 18, 2018

For #1413 I'm still a little unsure how exactly the loss recovery works for 0-RTT during the handshake with separate packet numbers. It seems to get even more complicated (just goes back to the original state?) if just the sender uses a single packet number space. Bottom line, no matter what, I think we need additional text for #1413, even if we don't change packet number spaces (though I still really think we should).

@nibanks
Copy link
Member Author

nibanks commented Jul 18, 2018

Could someone also explain what the harm would be to allow ACKs for 0-RTT packets to be sent at the Handshake encryption level? Because if we allowed that, the first UDP datagram from the server, could potentially acknowledge everything it has received from the client, 0-RTT included.

[edit] I guess it could even if the ACK was at 1-RTT, if the server's Handshake CRYTPO was small enough. But being able to ACK at Handshake removes that requirement/obstacle.

@kazuho
Copy link
Member

kazuho commented Jul 18, 2018

If server rejects 0-RTT, you can never expect ACKs for 0-RTT packets.

Therefore, if you need a design that ensures you that the datagram delivery is fed back to CC, you should coalesce 0-RTT packets with Initial packets, and use the ACKs against Initial as a confirmation of delivery (of the datagram).

If you do not care that much, using ACKs carried in 1-RTT packets would work fairly well.

@nibanks
Copy link
Member Author

nibanks commented Jul 18, 2018

The TLS CRYPTO frames will tell you if 0-RTT was rejected, so you don't need to depend on ACKs in that scenario, correct?

I am more interested in the cases where 0-RTT is accepted, but perhaps the tail end of your 0-RTT packets didn't make it. In that case, ACKs in the Handshake encryption level would be most beneficial (I believe). Though I don't think you can do anything if the ACK just includes the first N packet numbers for your 0-RTT packets. It could mean there was loss, or it could mean those other packets just hadn't made it yet, by the time the server responded.

@nibanks
Copy link
Member Author

nibanks commented Jul 24, 2018

Does anyone have any additional comments or concerns about this or my PR (#1591)?

@martinthomson martinthomson added the needs-discussion An issue that needs more discussion before we can resolve it. label Sep 14, 2018
@mnot
Copy link
Member

mnot commented Sep 19, 2018

Discussed in NYC; agreement to close with no action.

@mnot mnot closed this as completed Sep 19, 2018
@mnot mnot added has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. and removed needs-discussion An issue that needs more discussion before we can resolve it. labels Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-tls -transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.
Projects
None yet
Development

No branches or pull requests

6 participants