Revise and add additional 0-rtt doc comments #1826

gretchenfrage · 2024-04-16T04:47:49Z

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_0rtt to make its behavior more clear, especially across various edge cases.

This PR does not change any code or APIs.

djc

Thanks for fleshing out these docs, great idea!

I have lots of little feedback about how we should document this stuff, but would be happy to get something like this merged.

djc · 2024-04-16T07:52:36Z

quinn-proto/src/config.rs

@@ -778,6 +778,14 @@ pub struct ServerConfig {

 impl ServerConfig {
 /// Create a default config with a particular handshake token key
+ ///
+ /// 0-rtt


I think we usually use ##-like headers in docstrings.

Changed in all locations.

djc · 2024-04-16T07:57:09Z

quinn-proto/src/config.rs

+ /// 0-rtt
+ /// ---
+ ///
+ /// In general, where quinn automatically constructs a `ServerConfig`, it uses as the


A bunch of nits:

Capitalize "Quinn"

It feels like "In general" belongs on the ServerConfig type, not just ServerConfig::new() -- this note could then be shorter and reference the type's docstring to describe the implications of the value passed here as crypto

Also we should be mindful of the abstraction, so ideally in this general API we should not refer specifically to rustls::ServerConfig or 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_size has to be specifically u32::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

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.

All should be addressed.

djc · 2024-04-16T07:57:40Z

quinn-proto/src/config.rs

+ /// 0-rtt
+ /// ---
+ ///
+ /// In general, where quinn automatically constructs a `ClientConfig`, it uses as the


Similar nits as for the server.

Should be addressed.

djc · 2024-04-16T07:58:11Z

quinn/src/connection.rs

 ///
- /// When the `ZeroRttAccepted` future completes, the connection has been fully established.
+ /// Outgoing


Nit: header style.

djc · 2024-04-16T07:59:34Z

quinn/src/connection.rs

+ /// 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


"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".

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?

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."

I changed it to something based on that.

djc · 2024-04-16T08:00:01Z

quinn/src/connection.rs

+ /// 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.**


I don't think this should be documented here.

djc · 2024-04-16T08:00:23Z

quinn/src/connection.rs

 ///
- /// 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


rustls-specific.

djc · 2024-04-16T08:01:38Z

quinn/src/send_stream.rs

+ /// the client, it cannot resolve for at least two round trips after the creation of the
+ /// connection.


Can you briefly describe why "it cannot resolve for at least two round trips"?

After further inspection of the RFCs, I realized I am confused about this bit. I will pick this back up when I understand better.

I spent a while investigating this, and I was wrong about it. I was able to get it to resolve faster by creating a co-routine in the client that opens a new stream and writes a byte to it every millisecond, to trigger the server's ack threshold sooner. I think the moral is that stopped shouldn't be used for latency-sensitive things since acks can be delayed, so I added a note to that effect.

Right, relying on transport-layer ACKs to drive application-layer behavior is almost always a bad idea.

gretchenfrage · 2024-04-16T23:14:47Z

Thanks for the feedback! Happy to workshop this into something we both think is good.

gretchenfrage · 2024-06-14T23:01:31Z

Thanks to some other changes that have been made to the code base since this was last reviewed, some of the "where should this be documented" questions sort of solved themselves. I've shuffled things around, changed some things, and tried to make sure the current revision reflects the feedback given.

Current change set:

Rustls-specific documentation on crypto::rustls::ServerConfig and crypto::rustls::ClientConfig
Non-Rustls-specific documentation Connection::into_0rtt
A warning regarding latency on SendStream::stopped

Thanks!

gretchenfrage marked this pull request as ready for review April 16, 2024 05:01

djc reviewed Apr 16, 2024

View reviewed changes

gretchenfrage added 2 commits June 14, 2024 17:55

docs: Revise and add additionall 0-rtt doc comments

e168e65

docs(quinn): Revise SendStream.stopped docs comment

d31f6c8

gretchenfrage force-pushed the 0rtt-docs-changes branch from 71945e0 to d31f6c8 Compare June 14, 2024 22:55

gretchenfrage requested a review from djc June 14, 2024 23:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revise and add additional 0-rtt doc comments #1826

Revise and add additional 0-rtt doc comments #1826

gretchenfrage commented Apr 16, 2024

djc left a comment

djc Apr 16, 2024

gretchenfrage Jun 14, 2024

djc Apr 16, 2024

Ralith Apr 16, 2024

gretchenfrage Jun 14, 2024

djc Apr 16, 2024

gretchenfrage Jun 14, 2024

djc Apr 16, 2024

gretchenfrage Jun 14, 2024

djc Apr 16, 2024

gretchenfrage Apr 16, 2024

djc Apr 17, 2024

gretchenfrage Jun 14, 2024

djc Apr 16, 2024

gretchenfrage Jun 14, 2024

djc Apr 16, 2024

gretchenfrage Jun 14, 2024

djc Apr 16, 2024

gretchenfrage Apr 21, 2024

gretchenfrage Jun 14, 2024

Ralith Jun 15, 2024

gretchenfrage commented Apr 16, 2024

gretchenfrage commented Jun 14, 2024

		/// the client, it cannot resolve for at least two round trips after the creation of the
		/// connection.

Revise and add additional 0-rtt doc comments #1826

Are you sure you want to change the base?

Revise and add additional 0-rtt doc comments #1826

Conversation

gretchenfrage commented Apr 16, 2024

djc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gretchenfrage commented Apr 16, 2024

gretchenfrage commented Jun 14, 2024