diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 29bc85ff73..577a93277c 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -48,7 +48,7 @@ use crate::{ quic_datagrams::{DatagramTracking, QuicDatagrams}, recovery::{LossRecovery, RecoveryToken, SendProfile, SentPacket}, recv_stream::RecvStreamStats, - rtt::{RttEstimate, GRANULARITY}, + rtt::{RttEstimate, GRANULARITY, INITIAL_RTT}, send_stream::SendStream, stats::{Stats, StatsCell}, stream_id::StreamType, @@ -594,9 +594,25 @@ impl Connection { fn make_resumption_token(&mut self) -> ResumptionToken { debug_assert_eq!(self.role, Role::Client); debug_assert!(self.crypto.has_resumption_token()); + // Values less than GRANULARITY are ignored when using the token, so use 0 where needed. let rtt = self.paths.primary().map_or_else( - || RttEstimate::default().estimate(), - |p| p.borrow().rtt().estimate(), + // If we don't have a path, we don't have an RTT. + || Duration::from_millis(0), + |p| { + let rtt = p.borrow().rtt().estimate(); + if p.borrow().rtt().is_guesstimate() { + // When we have no actual RTT sample, do not encode a guestimated RTT larger + // than the default initial RTT. (The guess can be very large under lossy + // conditions.) + if rtt < INITIAL_RTT { + rtt + } else { + Duration::from_millis(0) + } + } else { + rtt + } + }, ); self.crypto diff --git a/neqo-transport/src/connection/tests/resumption.rs b/neqo-transport/src/connection/tests/resumption.rs index 7410e76ef8..01e977e506 100644 --- a/neqo-transport/src/connection/tests/resumption.rs +++ b/neqo-transport/src/connection/tests/resumption.rs @@ -6,7 +6,16 @@ use std::{cell::RefCell, mem, rc::Rc, time::Duration}; -use test_fixture::{assertions, now}; +use neqo_common::{Datagram, Decoder, Role}; +use neqo_crypto::AuthenticationStatus; +use test_fixture::{ + assertions, + header_protection::{ + apply_header_protection, decode_initial_header, initial_aead_and_hp, + remove_header_protection, + }, + now, split_datagram, +}; use super::{ connect, connect_with_rtt, default_client, default_server, exchange_ticket, get_tokens, @@ -14,7 +23,9 @@ use super::{ }; use crate::{ addr_valid::{AddressValidation, ValidateAddress}, - ConnectionParameters, Error, Version, + frame::FRAME_TYPE_PADDING, + rtt::INITIAL_RTT, + ConnectionParameters, Error, State, Version, MIN_INITIAL_PACKET_SIZE, }; #[test] @@ -75,6 +86,115 @@ fn remember_smoothed_rtt() { ); } +fn ticket_rtt(rtt: Duration) -> Duration { + // A simple ACK_ECN frame for a single packet with packet number 0 with a single ECT(0) mark. + const ACK_FRAME_1: &[u8] = &[0x03, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00]; + + let mut client = new_client( + ConnectionParameters::default().versions(Version::Version1, vec![Version::Version1]), + ); + let mut server = default_server(); + let mut now = now(); + + let client_initial = client.process_output(now); + let (_, client_dcid, _, _) = + decode_initial_header(client_initial.as_dgram_ref().unwrap(), Role::Client).unwrap(); + let client_dcid = client_dcid.to_owned(); + + now += rtt / 2; + let server_packet = server.process(client_initial.as_dgram_ref(), now).dgram(); + let (server_initial, server_hs) = split_datagram(server_packet.as_ref().unwrap()); + let (protected_header, _, _, payload) = + decode_initial_header(&server_initial, Role::Server).unwrap(); + + // Now decrypt the packet. + let (aead, hp) = initial_aead_and_hp(&client_dcid, Role::Server); + let (header, pn) = remove_header_protection(&hp, protected_header, payload); + assert_eq!(pn, 0); + let pn_len = header.len() - protected_header.len(); + let mut buf = vec![0; payload.len()]; + let mut plaintext = aead + .decrypt(pn, &header, &payload[pn_len..], &mut buf) + .unwrap() + .to_owned(); + + // Now we need to find the frames. Make some really strong assumptions. + let mut dec = Decoder::new(&plaintext[..]); + assert_eq!(dec.decode(ACK_FRAME_1.len()), Some(ACK_FRAME_1)); + assert_eq!(dec.decode_varint(), Some(0x06)); // CRYPTO + assert_eq!(dec.decode_varint(), Some(0x00)); // offset + dec.skip_vvec(); // Skip over the payload. + + // Replace the ACK frame with PADDING. + plaintext[..ACK_FRAME_1.len()].fill(FRAME_TYPE_PADDING.try_into().unwrap()); + + // And rebuild a packet. + let mut packet = header.clone(); + packet.resize(MIN_INITIAL_PACKET_SIZE, 0); + aead.encrypt(pn, &header, &plaintext, &mut packet[header.len()..]) + .unwrap(); + apply_header_protection(&hp, &mut packet, protected_header.len()..header.len()); + let si = Datagram::new( + server_initial.source(), + server_initial.destination(), + server_initial.tos(), + packet, + ); + + // Now a connection can be made successfully. + now += rtt / 2; + client.process_input(&si, now); + client.process_input(&server_hs.unwrap(), now); + client.authenticated(AuthenticationStatus::Ok, now); + let finished = client.process_output(now); + + assert_eq!(*client.state(), State::Connected); + + now += rtt / 2; + _ = server.process(finished.as_dgram_ref(), now); + assert_eq!(*server.state(), State::Confirmed); + + // Don't deliver the server's handshake finished, it has ACKs. + // Now get a ticket. + let validation = AddressValidation::new(now, ValidateAddress::NoToken).unwrap(); + let validation = Rc::new(RefCell::new(validation)); + server.set_validation(&validation); + send_something(&mut server, now); + server.send_ticket(now, &[]).expect("can send ticket"); + let ticket = server.process_output(now).dgram(); + assert!(ticket.is_some()); + now += rtt / 2; + client.process_input(&ticket.unwrap(), now); + let token = get_tokens(&mut client).pop().unwrap(); + + // And connect again. + let mut client = default_client(); + client.enable_resumption(now, token).unwrap(); + let ticket_rtt = client.paths.rtt(); + let mut server = resumed_server(&client); + + connect_with_rtt(&mut client, &mut server, now, Duration::from_millis(50)); + assert_eq!( + client.paths.rtt(), + Duration::from_millis(50), + "previous RTT should be completely erased" + ); + ticket_rtt +} + +#[test] +fn ticket_rtt_less_than_default() { + assert_eq!( + ticket_rtt(Duration::from_millis(10)), + Duration::from_millis(10) + ); +} + +#[test] +fn ticket_rtt_larger_than_default() { + assert_eq!(ticket_rtt(Duration::from_millis(500)), INITIAL_RTT); +} + /// Check that a resumed connection uses a token on Initial packets. #[test] fn address_validation_token_resume() { diff --git a/neqo-transport/src/path.rs b/neqo-transport/src/path.rs index 35d29f0253..49c289f60b 100644 --- a/neqo-transport/src/path.rs +++ b/neqo-transport/src/path.rs @@ -27,7 +27,7 @@ use crate::{ packet::PacketBuilder, pmtud::Pmtud, recovery::{RecoveryToken, SentPacket}, - rtt::RttEstimate, + rtt::{RttEstimate, RttSource}, sender::PacketSender, stats::FrameStats, tracking::PacketNumberSpace, @@ -984,7 +984,7 @@ impl Path { &self.qlog, now - sent.time_sent(), Duration::new(0, 0), - false, + RttSource::Guesstimate, now, ); } diff --git a/neqo-transport/src/recovery/mod.rs b/neqo-transport/src/recovery/mod.rs index 5820dc07a0..f2c3e8e298 100644 --- a/neqo-transport/src/recovery/mod.rs +++ b/neqo-transport/src/recovery/mod.rs @@ -27,7 +27,7 @@ use crate::{ packet::PacketNumber, path::{Path, PathRef}, qlog::{self, QlogMetric}, - rtt::RttEstimate, + rtt::{RttEstimate, RttSource}, stats::{Stats, StatsCell}, tracking::{PacketNumberSpace, PacketNumberSpaceSet}, }; @@ -558,9 +558,13 @@ impl LossRecovery { now: Instant, ack_delay: Duration, ) { - let confirmed = self.confirmed_time.map_or(false, |t| t < send_time); + let source = if self.confirmed_time.map_or(false, |t| t < send_time) { + RttSource::AckConfirmed + } else { + RttSource::Ack + }; if let Some(sample) = now.checked_duration_since(send_time) { - rtt.update(&self.qlog, sample, ack_delay, confirmed, now); + rtt.update(&self.qlog, sample, ack_delay, source, now); } } diff --git a/neqo-transport/src/rtt.rs b/neqo-transport/src/rtt.rs index 7bfbbe4aaa..027b574aad 100644 --- a/neqo-transport/src/rtt.rs +++ b/neqo-transport/src/rtt.rs @@ -28,6 +28,17 @@ pub const GRANULARITY: Duration = Duration::from_millis(1); // Defined in -recovery 6.2 as 333ms but using lower value. pub const INITIAL_RTT: Duration = Duration::from_millis(100); +/// The source of the RTT measurement. +#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy)] +pub enum RttSource { + /// RTT guess from a retry or dropping a packet number space. + Guesstimate, + /// Ack on an unconfirmed connection. + Ack, + /// Ack on a confirmed connection. + AckConfirmed, +} + #[derive(Debug)] #[allow(clippy::module_name_repetitions)] pub struct RttEstimate { @@ -37,6 +48,7 @@ pub struct RttEstimate { rttvar: Duration, min_rtt: Duration, ack_delay: PeerAckDelay, + best_source: RttSource, } impl RttEstimate { @@ -58,6 +70,7 @@ impl RttEstimate { rttvar: Duration::from_millis(0), min_rtt: rtt, ack_delay: PeerAckDelay::Fixed(Duration::from_millis(25)), + best_source: RttSource::Ack, } } @@ -83,17 +96,24 @@ impl RttEstimate { self.ack_delay.update(cwnd, mtu, self.smoothed_rtt); } + pub fn is_guesstimate(&self) -> bool { + self.best_source == RttSource::Guesstimate + } + pub fn update( &mut self, qlog: &NeqoQlog, mut rtt_sample: Duration, ack_delay: Duration, - confirmed: bool, + source: RttSource, now: Instant, ) { + debug_assert!(source >= self.best_source); + self.best_source = max(self.best_source, source); + // Limit ack delay by max_ack_delay if confirmed. let mad = self.ack_delay.max(); - let ack_delay = if confirmed && ack_delay > mad { + let ack_delay = if self.best_source == RttSource::AckConfirmed && ack_delay > mad { mad } else { ack_delay @@ -205,6 +225,7 @@ impl Default for RttEstimate { rttvar: INITIAL_RTT / 2, min_rtt: INITIAL_RTT, ack_delay: PeerAckDelay::default(), + best_source: RttSource::Guesstimate, } } }