Revise and add additional 0-rtt doc comments #1826
Conversation
| @@ -778,6 +778,14 @@ pub struct ServerConfig { | |||
|
|
|||
| impl ServerConfig { | |||
| /// Create a default config with a particular handshake token key | |||
| /// | |||
| /// 0-rtt | |||
There was a problem hiding this comment.
I think we usually use ##-like headers in docstrings.
| /// 0-rtt | ||
| /// --- | ||
| /// | ||
| /// In general, where quinn automatically constructs a `ServerConfig`, it uses as the |
There was a problem hiding this comment.
A bunch of nits:
- Capitalize "Quinn"
- It feels like "In general" belongs on the
ServerConfigtype, not justServerConfig::new()-- this note could then be shorter and reference the type's docstring to describe the implications of the value passed here ascrypto - Also we should be mindful of the abstraction, so ideally in this general API we should not refer specifically to
rustls::ServerConfigor specific fields that it has, but only refer generally to configuration settings that support early data/0-RTT -- we can be more specific in rustls-specific API documentation - I also don't like "automatically" -- maybe more specific like saying "when Quinn API is used to construct a default
ServerConfig" - I don't think
max_early_data_sizehas to be specificallyu32::MAX(in the sense that something smaller should work, too?) -- good to be precise here; I think what you're trying to say is that the configuration has to support early data
There was a problem hiding this comment.
I don't think max_early_data_size has to be specifically u32::MAX
It does. RFC 9001:
Servers MUST NOT send the early_data extension with a max_early_data_size field set to any value other than 0xffffffff. A client MUST treat receipt of a NewSessionTicket that contains an early_data extension with any other value as a connection error of type PROTOCOL_VIOLATION.
| /// 0-rtt | ||
| /// --- | ||
| /// | ||
| /// In general, where quinn automatically constructs a `ClientConfig`, it uses as the |
There was a problem hiding this comment.
Similar nits as for the server.
| /// | ||
| /// When the `ZeroRttAccepted` future completes, the connection has been fully established. | ||
| /// Outgoing |
| /// For outgoing connections, the initial attempt to convert to 0-RTT will proceed if the | ||
| /// [`crypto::ClientConfig`][crate::crypto::ClientConfig] attempts to resume a previous TLS | ||
| /// session. However, **the remote endpoint may not actually _accept_ the 0-RTT data**--yet | ||
| /// still accept the connection attempt in general. This may occur, for example, if the remote |
There was a problem hiding this comment.
"has restarted" feels kinda specific to the little example script that you may have tested with. Would be nice to be more specific to say "lost resumption state".
There was a problem hiding this comment.
While I agree we should be technically correct, I also think it's pedagogically important here to ground it in a concrete and common-to-encounter example (server restart). If I had slightly less background knowledge here I might not know what a server "losing resumption state" means or whether it was something common or esoteric. Also, if my understanding is correct, the condition described in this sentence could occur in cases other than lost resumption state--even, hypothetically, a server for some reason being programmed to reject early data based on a random coin toss.
I'm trying to think of ways to rephrase this to make it feel more general. Perhaps: "One common reason this may occur is if the remote endpoint has restarted and thus lost its resumption state since the local endpoint last connected to it." I feel like that's a lot more clunky, grammatically convoluted, and lengthy, though. What do you think?
There was a problem hiding this comment.
Maybe something like:
"However, the server may at its discretion decide to reject 0-RTT data while otherwise accepting the connection. The ability to accept 0-RTT data depends on resumption state being stored in the server, which servers will limit due to any number of reasons. Some servers only store resumption state in memory, such that a restart of the server will reset all resumption state."
| /// By default, quinn uses [`rustls::ClientConfig`] as the client crypto implementation. It | ||
| /// allows the initial attempt to convert to 0-RTT to proceed if its `enable_early_data` field | ||
| /// is set to true and its `resumption` field recognizes the server name (which defaults to an | ||
| /// in-memory cache of 256 server names). **If you are manually providing the client crypto | ||
| /// config, you must set `enable_early_data` to true to get 0-RTT working.** |
There was a problem hiding this comment.
I don't think this should be documented here.
| /// | ||
| /// For incoming connections, a 0.5-RTT connection will always be successfully constructed. | ||
| /// By default, quinn uses [`rustls::ServerConfig`] as the server crypto implementation. Its |
| /// the client, it cannot resolve for at least two round trips after the creation of the | ||
| /// connection. |
There was a problem hiding this comment.
Can you briefly describe why "it cannot resolve for at least two round trips"?
There was a problem hiding this comment.
After further inspection of the RFCs, I realized I am confused about this bit. I will pick this back up when I understand better.
|
Thanks for the feedback! Happy to workshop this into something we both think is good. |
I had a hard time getting 0-RTT actually working in practice. It escalated to me forking rustls so I could put in a bunch of print statements. Although I did find the 0-rtt tests, there were some subtle enough pitfalls that I didn't notice the relevant ways my attempts were diverging.
In hopes of improving understandability and discoverability of this, this PR adds notes about these pitfalls to some of the doc comments, and also largely rewrites the doc comment for
Connecting::into_0rttto make its behavior more clear, especially across various edge cases.This PR does not change any code or APIs.