diff --git a/quinn-proto/src/config.rs b/quinn-proto/src/config.rs index 15839b20d..e9f6602eb 100644 --- a/quinn-proto/src/config.rs +++ b/quinn-proto/src/config.rs @@ -52,7 +52,7 @@ pub struct TransportConfig { pub(crate) min_mtu: u16, pub(crate) mtu_discovery_config: Option, pub(crate) ack_frequency_config: Option, - pub(crate) ack_timestamp_config: Option, + pub(crate) ack_timestamp_config: AckTimestampsConfig, pub(crate) persistent_congestion_threshold: u32, pub(crate) keep_alive_interval: Option, @@ -228,7 +228,7 @@ impl TransportConfig { /// Defaults to `None`, which disables receiving acknowledgement timestamps from the sender. /// If `Some`, TransportParameters are sent to the peer to enable acknowledgement timestamps /// if supported. - pub fn ack_timestamp_config(&mut self, value: Option) -> &mut Self { + pub fn ack_timestamp_config(&mut self, value: AckTimestampsConfig) -> &mut Self { self.ack_timestamp_config = value; self } @@ -371,7 +371,7 @@ impl Default for TransportConfig { enable_segmentation_offload: true, - ack_timestamp_config: None, + ack_timestamp_config: AckTimestampsConfig::default(), } } } @@ -435,16 +435,17 @@ impl fmt::Debug for TransportConfig { } /// Parameters for controlling the peer's acknowledgements with receiver timestamps. -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq, Eq, Copy)] pub struct AckTimestampsConfig { - pub(crate) max_timestamps_per_ack: VarInt, + /// If max_timestamp_per_ack is None, this feature is disabled. + pub(crate) max_timestamps_per_ack: Option, pub(crate) exponent: VarInt, } impl AckTimestampsConfig { /// Sets the maximum number of timestamp entries per ACK frame. pub fn max_timestamps_per_ack(&mut self, value: VarInt) -> &mut Self { - self.max_timestamps_per_ack = value; + self.max_timestamps_per_ack = Some(value); self } @@ -459,7 +460,8 @@ impl AckTimestampsConfig { impl Default for AckTimestampsConfig { fn default() -> Self { Self { - max_timestamps_per_ack: 10u32.into(), + max_timestamps_per_ack: None, + // Default to 0 as per draft. exponent: 0u32.into(), } } diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index c7e5e71a9..2066a1bd1 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -231,7 +231,7 @@ pub struct Connection { // Ack Receive Timestamps // The timestamp config of the peer. - ack_timestamp_cfg: Option, + ack_timestamp_cfg: AckTimestampsConfig, streams: StreamsState, /// Surplus remote CIDs for future use on new paths @@ -345,7 +345,7 @@ impl Connection { &TransportParameters::default(), )), - ack_timestamp_cfg: None, + ack_timestamp_cfg: AckTimestampsConfig::default(), pto_count: 0, @@ -828,7 +828,7 @@ impl Connection { &mut self.spaces[space_id], buf, &mut self.stats, - None, + self.ack_timestamp_cfg, self.epoch, ); } @@ -1357,7 +1357,13 @@ impl Connection { return Err(TransportError::PROTOCOL_VIOLATION("unsent packet acked")); } - if ack.timestamps.is_some() != self.config.ack_timestamp_config.is_some() { + if ack.timestamps.is_some() + != self + .config + .ack_timestamp_config + .max_timestamps_per_ack + .is_some() + { return Err(TransportError::PROTOCOL_VIOLATION( "ack with timestamps expectation mismatched", )); @@ -1391,13 +1397,18 @@ impl Connection { } } - let mut timestamp_iter = self.config.ack_timestamp_config.as_ref().map(|cfg| { - let decoder = ack.timestamp_iter(self.epoch, cfg.exponent.0).unwrap(); - let mut v: tinyvec::TinyVec<[PacketTimestamp; 10]> = tinyvec::TinyVec::new(); - decoder.for_each(|elt| v.push(elt)); - v.reverse(); - v.into_iter().peekable() - }); + let mut timestamp_iter = + if let Some(_) = self.config.ack_timestamp_config.max_timestamps_per_ack { + let decoder = ack + .timestamp_iter(self.epoch, self.config.ack_timestamp_config.exponent.0) + .unwrap(); + let mut v: tinyvec::TinyVec<[PacketTimestamp; 10]> = tinyvec::TinyVec::new(); + decoder.for_each(|elt| v.push(elt)); + v.reverse(); + Some(v.into_iter().peekable()) + } else { + None + }; if newly_acked.is_empty() { return Ok(()); @@ -3290,7 +3301,7 @@ impl Connection { space: &mut PacketSpace, buf: &mut Vec, stats: &mut ConnectionStats, - peer_timestamp_config: Option, + ack_timestamps_config: AckTimestampsConfig, epoch: Instant, ) { debug_assert!(!space.pending_acks.ranges().is_empty()); @@ -3320,13 +3331,13 @@ impl Connection { delay as _, space.pending_acks.ranges(), ecn, - peer_timestamp_config.map(|cfg| { + ack_timestamps_config.max_timestamps_per_ack.map(|max| { ( // Safety: If peer_timestamp_config is set, receiver_timestamps must be set. space.pending_acks.receiver_timestamps_as_ref().unwrap(), epoch, - cfg.exponent.0, - cfg.max_timestamps_per_ack.0, + ack_timestamps_config.exponent.0, + max.0, ) }), buf, @@ -3385,25 +3396,12 @@ impl Connection { self.path.mtud.on_peer_max_udp_payload_size_received( u16::try_from(self.peer_params.max_udp_payload_size.into_inner()).unwrap_or(u16::MAX), ); - - { - self.ack_timestamp_cfg = if let (Some(max_timestamps_per_ack), Some(exponent)) = ( - params.max_recv_timestamps_per_ack, - params.receive_timestamps_exponent, - ) { - for space in self.spaces.iter_mut() { - space - .pending_acks - .set_receiver_timestamp(max_timestamps_per_ack.0 as usize); - } - Some(AckTimestampsConfig { - exponent, - max_timestamps_per_ack, - }) - } else { - None - }; - } + self.ack_timestamp_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); + } + }; } fn decrypt_packet( diff --git a/quinn-proto/src/transport_parameters.rs b/quinn-proto/src/transport_parameters.rs index a83f40414..f24fca4b3 100644 --- a/quinn-proto/src/transport_parameters.rs +++ b/quinn-proto/src/transport_parameters.rs @@ -18,7 +18,7 @@ use crate::{ cid_generator::ConnectionIdGenerator, cid_queue::CidQueue, coding::{BufExt, BufMutExt, UnexpectedEnd}, - config::{EndpointConfig, ServerConfig, TransportConfig}, + config::{AckTimestampsConfig, EndpointConfig, ServerConfig, TransportConfig}, shared::ConnectionId, ResetToken, Side, TransportError, VarInt, LOC_CID_COUNT, MAX_CID_SIZE, MAX_STREAM_COUNT, RESET_TOKEN_SIZE, TIMER_GRANULARITY, @@ -101,8 +101,7 @@ macro_rules! make_struct { pub(crate) preferred_address: Option, - pub(crate) receive_timestamps_exponent: Option, - pub(crate) max_recv_timestamps_per_ack: Option, + pub(crate) ack_timestamps_cfg: AckTimestampsConfig, } // We deliberately don't implement the `Default` trait, since that would be public, and @@ -124,8 +123,7 @@ macro_rules! make_struct { retry_src_cid: None, stateless_reset_token: None, preferred_address: None, - receive_timestamps_exponent: None, - max_recv_timestamps_per_ack: None, + ack_timestamps_cfg: AckTimestampsConfig::default(), } } } @@ -167,15 +165,7 @@ impl TransportParameters { VarInt::from_u64(u64::try_from(TIMER_GRANULARITY.as_micros()).unwrap()).unwrap(), ), - 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), + ack_timestamps_cfg: config.ack_timestamp_config, ..Self::default() } @@ -370,16 +360,15 @@ impl TransportParameters { // The below 2 fields are for the implementation of // https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html#name-extension-negotiation // Values of 0x00f0 and 0x00f1 arbitrarily chosen. - if let Some(x) = self.max_recv_timestamps_per_ack { + if let Some(max) = self.ack_timestamps_cfg.max_timestamps_per_ack { w.write_var(0x00f0); - w.write_var(x.size() as u64); - w.write(x); - } + w.write_var(max.size() as u64); + w.write(max); - if let Some(x) = self.receive_timestamps_exponent { + let exponent = self.ack_timestamps_cfg.exponent; w.write_var(0x00f1); - w.write_var(x.size() as u64); - w.write(x); + w.write_var(exponent.size() as u64); + w.write(exponent); } } @@ -447,16 +436,18 @@ impl TransportParameters { 0xff04de1b => params.min_ack_delay = Some(r.get().unwrap()), 0x00f0 => { - if len > 8 || params.max_recv_timestamps_per_ack.is_some() { + if len > 8 || params.ack_timestamps_cfg.max_timestamps_per_ack.is_some() { return Err(Error::Malformed); } - params.max_recv_timestamps_per_ack = Some(r.get().unwrap()); + params + .ack_timestamps_cfg + .max_timestamps_per_ack(r.get().unwrap()); } 0x00f1 => { - if len > 8 || params.receive_timestamps_exponent.is_some() { + if len > 8 { return Err(Error::Malformed); } - params.receive_timestamps_exponent = Some(r.get().unwrap()); + params.ack_timestamps_cfg.exponent(r.get().unwrap()); } _ => { @@ -511,10 +502,7 @@ impl TransportParameters { } // https://www.ietf.org/archive/id/draft-smith-quic-receive-ts-00.html#name-extension-negotiation - if params - .receive_timestamps_exponent - .map_or(false, |x| x.0 > 20) - { + if params.ack_timestamps_cfg.exponent.0 > 20 { return Err(Error::IllegalValue); } @@ -554,9 +542,10 @@ mod test { grease_quic_bit: true, min_ack_delay: Some(2_000u32.into()), - receive_timestamps_exponent: Some(3u32.into()), - max_recv_timestamps_per_ack: Some(5u32.into()), - + ack_timestamps_cfg: AckTimestampsConfig { + max_timestamps_per_ack: Some(10u32.into()), + exponent: 2u32.into(), + }, ..TransportParameters::default() }; params.write(&mut buf);