feat: ack receiver timestamps #1992

sterlingdeng · 2024-09-19T18:42:50Z

This PR is the implementation that addresses #1988

Manual E2E Testing/Validation instructions: sterlingdeng#2

Surfaces the TransportParameters to the application

Adds ReceiverTimestamps data structure to the space and expose methods.

This commit processes the inbound ACK frames that contain timestamps.

This commit handles sending frames that contain timestamps

Surfaces timing information to the CC interface.

djc · 2024-09-20T12:29:25Z

I don't think a feature makes sense for this, what's the motivation for that? Suggest squashing all of these commits because I don't think they make much sense on their own.

billylindeman · 2024-09-20T16:46:50Z

@djc I suggested using a feature bc it is a draft specification, perhaps we should name the feature after the specification draft-smith-quic-receive-ts https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html

djc · 2024-09-21T07:00:41Z

@djc I suggested using a feature bc it is a draft specification, perhaps we should name the feature after the specification draft-smith-quic-receive-ts https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html

I don't think that's a good reason to make it a feature. The current API surface will still become part of our public API, and it just adds a bunch of complexity. We've added draft functionality before, should be fine.

sterlingdeng · 2024-09-23T16:08:56Z

I can remove the feature flag in a follow up commit. Regarding squashing, my intention for the commits were to keep separate different parts of the PR for review and then squash at the end. If you want me to squash it now, I can do that too.

djc · 2024-09-24T07:00:08Z

I can remove the feature flag in a follow up commit. Regarding squashing, my intention for the commits were to keep separate different parts of the PR for review and then squash at the end. If you want me to squash it now, I can do that too.

While we are fans of clean commit histories with atomic commits, I think this falls short of that because the commits are not conceptually independent -- it doesn't make sense to add configuration that doesn't do anything, for example.

djc

Here's a bunch of mostly stylistic feedback.

quinn-proto/src/config.rs

djc · 2024-10-01T08:04:58Z

quinn-proto/src/congestion.rs

@@ -34,6 +34,20 @@ pub trait Controller: Send + Sync {
    ) {
    }

+    #[allow(unused_variables)]
+    /// Packet deliveries were confirmed with timestamps information.
+    fn on_ack_packet(


Are these actually changes you'd look to make to the on_ack() interface which you're adding a separate method for to keep it semver-compatible?

If so, should this default implementation call self.on_ack()?

Yea, I did it this way to prevent any breaking changes if there were users of the on_ack method. Yea, I think it makes sense for the default implementation to all self.on_ack, and then we can remove the congestion.on_ack call from on_packet_acked and just have it call congestion.on_ack_packet.

quinn-proto/src/connection/mod.rs

quinn-proto/src/frame.rs

djc · 2024-10-01T08:23:13Z

quinn-proto/src/frame.rs

+            self.next_pn -= 1;
+        }
+
+        let delta = self.data.get_var().unwrap();


Why is this safe?

This follows the same pattern done on AckIter, where unwrap is used.
I think it's considered safe because this is defined in the spec. If the value is expected to be a VARINT based on the spec but it isn't, we wouldn't be able to proceed in processing the packet.
Another option is to change the return type to be a tuple that includes an error.

I didn't include a SAFETY comment here because the others didn't have them either.

I think it's considered safe because this is defined in the spec.

What's in the spec doesn't matter. Quinn must not panic just because a peer is non-conformant. That would be a DoS vulnerability.

I imagine this is safe because scan_ack_timestamps_blocks validated the ranges already. If that was the intent, document it, even if we've failed to do so elsewhere.

quinn-proto/src/lib.rs

djc · 2024-10-01T08:24:07Z

quinn-proto/src/transport_parameters.rs

+            receive_timestamps_exponent: config
+                .ack_timestamp_config
+                .as_ref()
+                .map(|cfg| cfg.exponent),
+
+            max_recv_timestamps_per_ack: config
+                .ack_timestamp_config
+                .as_ref()
+                .map(|cfg| cfg.max_timestamps_per_ack),
+


I think these maybe shouldn't be separate Options, but a tuple wrapped in a single Option?

In this file, rather than using two individual parameters on the TransportParameters struct, I'm using the AckTimestampConfig and making max_timestamps_per_ack an Option. None signaling that the feature is disabled.
Exponent defaults to 0, which is what the spec states.

It could be worth proposing for the draft that the transport parameters related to this feature are grouped together similar to how PreferredAddress does it.

It could be worth proposing for the draft that the transport parameters related to this feature are grouped together similar to how PreferredAddress does it.

Yes, that would probably be good.

change field name and arg names

quinn-proto/src/frame.rs

sterlingdeng · 2024-10-01T19:35:58Z

quinn-proto/src/transport_parameters.rs

+            receive_timestamps_exponent: config
+                .ack_timestamp_config
+                .as_ref()
+                .map(|cfg| cfg.exponent),
+
+            max_recv_timestamps_per_ack: config
+                .ack_timestamp_config
+                .as_ref()
+                .map(|cfg| cfg.max_timestamps_per_ack),
+


In this file, rather than using two individual parameters on the TransportParameters struct, I'm using the AckTimestampConfig and making max_timestamps_per_ack an Option. None signaling that the feature is disabled.
Exponent defaults to 0, which is what the spec states.

It could be worth proposing for the draft that the transport parameters related to this feature are grouped together similar to how PreferredAddress does it.

sterlingdeng · 2024-10-01T19:43:03Z

quinn-proto/src/frame.rs

+            self.next_pn -= 1;
+        }
+
+        let delta = self.data.get_var().unwrap();


This follows the same pattern done on AckIter, where unwrap is used.
I think it's considered safe because this is defined in the spec. If the value is expected to be a VARINT based on the spec but it isn't, we wouldn't be able to proceed in processing the packet.
Another option is to change the return type to be a tuple that includes an error.

quinn-proto/src/frame.rs

sterlingdeng · 2024-10-01T20:40:20Z

quinn-proto/src/congestion.rs

@@ -34,6 +34,20 @@ pub trait Controller: Send + Sync {
    ) {
    }

+    #[allow(unused_variables)]
+    /// Packet deliveries were confirmed with timestamps information.
+    fn on_ack_packet(


Yea, I did it this way to prevent any breaking changes if there were users of the on_ack method. Yea, I think it makes sense for the default implementation to all self.on_ack, and then we can remove the congestion.on_ack call from on_packet_acked and just have it call congestion.on_ack_packet.

sterlingdeng · 2024-10-01T20:53:51Z

quinn-proto/src/frame.rs

+    fn new(largest: u64, basis_instant: Instant, exponent: u64, mut data: &'a [u8]) -> Self {
+        // We read and throw away the Timestamp Range Count value because
+        // it was already used to properly slice the data.
+        let _ = data.get_var().unwrap();


This unwrap is safe because the data byte slice was sliced using scan_ack_timestamp_blocks, which this get_var() was checked using ?.

sterlingdeng · 2024-10-01T20:55:03Z

quinn-proto/src/frame.rs

+            self.next_pn -= 1;
+        }
+
+        let delta = self.data.get_var().unwrap();


I didn't include a SAFETY comment here because the others didn't have them either.

sterlingdeng · 2024-10-01T20:55:23Z

quinn-proto/src/frame.rs

+    fn new(largest: u64, basis_instant: Instant, exponent: u64, mut data: &'a [u8]) -> Self {
+        // We read and throw away the Timestamp Range Count value because
+        // it was already used to properly slice the data.
+        let _ = data.get_var().unwrap();


I added a safety comment.

Ralith

As noted in the issue, please work with the IETF to reserve IDs. To avoid interop problems, public implementations must not use unreserved IDs

Ralith · 2024-10-12T20:45:09Z

quinn-proto/src/config.rs

+    pub fn max_ack_timestamps(&mut self, value: VarInt) -> &mut Self {
+        self.ack_timestamps_config.max_timestamps_per_ack = Some(value);


Suggested change

pub fn max_ack_timestamps(&mut self, value: VarInt) -> &mut Self {

self.ack_timestamps_config.max_timestamps_per_ack = Some(value);

pub fn max_ack_timestamps(&mut self, value: Option<VarInt>) -> &mut Self {

self.ack_timestamps_config.max_timestamps_per_ack = value;

What happens if someone specifies Some(0)? Document.

Is it useful to users for the quantity to be configurable, or should this just be a bool flag? We don't expose configuration for the maximum number of ACK ranges, for example.

Ralith · 2024-10-12T20:48:53Z

quinn-proto/src/config.rs

            .finish()
    }
 }

+/// Parameters for controlling the peer's acknowledgements with receiver timestamps.
+#[derive(Clone, Debug, PartialEq, Eq, Copy)]
+pub struct AckTimestampsConfig {


If this is public, then the main config struct should have a single config setter that accepts an Option<AckTimestampsConfig>. If you want setters for individual fields directly on the main struct, then it's confusing for this to be public.

Ralith · 2024-10-12T20:50:10Z

quinn-proto/src/congestion.rs

+        pn: u64,
+        now: Instant,
+        sent: Instant,
+        received: Option<Instant>,


received should use a type other than Instant to represent that it is not comparable to sent.

Ralith · 2024-10-12T20:51:16Z

quinn-proto/src/connection/mod.rs

@@ -227,6 +229,10 @@ pub struct Connection {
    /// no outgoing application data.
    app_limited: bool,

+    // Ack Receive Timestamps
+    // The timestamp config of the peer.
+    ack_timestamps_cfg: AckTimestampsConfig,


Ralith · 2024-10-12T20:53:00Z

quinn-proto/src/congestion.rs

@@ -34,6 +34,21 @@ pub trait Controller: Send + Sync {
    ) {
    }

+    #[allow(unused_variables)]
+    /// Packet deliveries were confirmed with timestamps information.
+    fn on_ack_packet(


Suggested change

fn on_ack_packet(

fn on_ack_timestamped(

Ralith · 2024-10-12T21:08:03Z

quinn-proto/src/connection/receiver_timestamps.rs

+    fn default() -> Self {
+        Self {
+            packet_number: 0,
+            timestamp: Instant::now(),


It's weird for default() to return different values over time. Maybe encode the delta here instead? Or, do we even need a Default impl?

Ralith · 2024-10-12T21:10:07Z

quinn-proto/src/connection/receiver_timestamps.rs

+        }
+        if let Some(v) = self.data.back() {
+            if packet_number <= v.packet_number {
+                warn!("out of order packets are not supported");


This will be very noisy on some real networks. It should be trace at most, or omitted entirely.

Ralith · 2024-10-12T21:10:51Z

quinn-proto/src/connection/receiver_timestamps.rs

+        });
+    }
+
+    pub(crate) fn subtract_below(&mut self, packet_number: u64) {


Ralith · 2024-10-12T21:14:43Z

quinn-proto/src/connection/receiver_timestamps.rs

+        }
+    }
+
+    pub(crate) fn iter(&self) -> impl DoubleEndedIterator<Item = PacketTimestamp> + '_ {


Document order.

Ralith · 2024-10-12T21:15:37Z

quinn-proto/src/connection/mod.rs

+        self.ack_timestamps_cfg = params.ack_timestamps_cfg;
+        if let Some(max) = params.ack_timestamps_cfg.max_timestamps_per_ack {
+            for space in self.spaces.iter_mut() {
+                space.pending_acks.set_receiver_timestamp(max.0 as usize);


Clamp this maximum to a reasonable value to guard against memory exhaustion attacks.

sterlingdeng added 11 commits September 19, 2024 10:52

Implement TransportParameters

bdd1115

Implement AckTimestampsConfig

cb6d5c4

Surfaces the TransportParameters to the application

Implement Ack Timestamps Frame

8dc4579

Implement ReceiverTimestamp data structure

d83b684

small cleanup

3366153

Process TransportParameters from peer if available

94cb098

Update spaces to include ReceiverTimestamps

76ddf51

Adds ReceiverTimestamps data structure to the space and expose methods.

Process ACK frames with timestamps

a5dae4a

This commit processes the inbound ACK frames that contain timestamps.

Send ACK frame with timestamps

ead292b

This commit handles sending frames that contain timestamps

Update congestion controller interface

c98b93f

Surfaces timing information to the CC interface.

Fix lint

3fd8f2e

Remove feature flag

3af9df4

sterlingdeng force-pushed the sdeng/receiver_timestamps branch from f7bd14c to 3af9df4 Compare September 24, 2024 21:04

sterlingdeng added 5 commits September 24, 2024 14:53

Set created_at time on Connection

6c56bc1

minor cleanup

9d9ee23

Fix lint

e47ade6

Fix unit tests

6ac89db

simplify into on_ack_received

96153e6

sterlingdeng changed the title ~~Receiver Timestamps~~ feat: ack receiver timestamps Sep 26, 2024

sterlingdeng added 2 commits September 26, 2024 11:57

add comment

ea3bb6a

revert some changes

caa86bd

sterlingdeng mentioned this pull request Sep 27, 2024

acknowledge timestamp frame manual testing sterlingdeng/quinn#2

Draft

sterlingdeng marked this pull request as ready for review September 27, 2024 23:10

djc reviewed Oct 1, 2024

View reviewed changes

fix easier PR comments

1e3c936

sterlingdeng added 2 commits October 1, 2024 11:27

refactor on nits

1cc4043

change field name and arg names

refactor transport parameter

67af041

sterlingdeng force-pushed the sdeng/receiver_timestamps branch from c9224f3 to 67af041 Compare October 1, 2024 19:29

sterlingdeng added 3 commits October 1, 2024 13:02

refactor nits

55d207d

consistent naming

e85e8f1

lint and test

64acada

sterlingdeng commented Oct 1, 2024

View reviewed changes

refactor some unidiomatic code

fa004c5

sterlingdeng force-pushed the sdeng/receiver_timestamps branch from 4a42bec to fa004c5 Compare October 2, 2024 20:39

sterlingdeng added 2 commits October 2, 2024 14:19

refactor some unwraps

53bcc35

fix lint

7bee845

Ralith requested changes Oct 12, 2024

View reviewed changes

sterlingdeng added 2 commits October 22, 2024 07:14

timestamp as duration

8bc98b1

best effort timestamp send

bb8df6b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ack receiver timestamps #1992

feat: ack receiver timestamps #1992

sterlingdeng commented Sep 19, 2024 •

edited

Loading

djc commented Sep 20, 2024

billylindeman commented Sep 20, 2024

djc commented Sep 21, 2024

sterlingdeng commented Sep 23, 2024

djc commented Sep 24, 2024

djc left a comment

djc Oct 1, 2024

sterlingdeng Oct 1, 2024

djc Oct 1, 2024

sterlingdeng Oct 1, 2024

sterlingdeng Oct 1, 2024

Ralith Oct 12, 2024

djc Oct 1, 2024

sterlingdeng Oct 1, 2024

djc Oct 11, 2024

sterlingdeng Oct 1, 2024

sterlingdeng Oct 1, 2024

sterlingdeng Oct 1, 2024

sterlingdeng Oct 1, 2024

sterlingdeng Oct 1, 2024

sterlingdeng Oct 1, 2024

Ralith left a comment

Ralith Oct 12, 2024

Ralith Oct 12, 2024

Ralith Oct 12, 2024

Ralith Oct 12, 2024

Ralith Oct 12, 2024

Ralith Oct 12, 2024

Ralith Oct 12, 2024

Ralith Oct 12, 2024

Ralith Oct 12, 2024

Ralith Oct 12, 2024

		pub fn max_ack_timestamps(&mut self, value: VarInt) -> &mut Self {
		self.ack_timestamps_config.max_timestamps_per_ack = Some(value);

feat: ack receiver timestamps #1992

Are you sure you want to change the base?

feat: ack receiver timestamps #1992

Conversation

sterlingdeng commented Sep 19, 2024 • edited Loading

djc commented Sep 20, 2024

billylindeman commented Sep 20, 2024

djc commented Sep 21, 2024

sterlingdeng commented Sep 23, 2024

djc commented Sep 24, 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

Ralith 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

sterlingdeng commented Sep 19, 2024 •

edited

Loading