feat(transport): Use a shorter ECN probe threshold initially (#1964)

* test(transport): handshake delay with ecn blackhole

This commit adds a test where a client connects to a server over a connection
dropping all ECN marked datagrams (ECN blackhole) in both directions, asserting
43 RTT to detect ECN blackhole, disable ECN and eventually establish connection.

* Bail out of ECN validation after three failed Initials

* Update neqo-transport/src/ecn.rs

Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Update neqo-transport/src/ecn.rs

Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Update neqo-transport/src/ecn.rs

Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Update neqo-transport/src/ecn.rs

Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Update neqo-transport/src/ecn.rs

Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Fixes

* clean-up tests

---------

Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Lars Eggert <lars@eggert.org>

neqo-transport/src/connection/tests/ecn.rs

@@ -12,11 +12,11 @@ use test_fixture::{
  fixture_init, now, DEFAULT_ADDR_V4,
 };
 
-use super::send_something_with_modifier;
+use super::{send_something_with_modifier, DEFAULT_RTT};
 use crate::{
  connection::tests::{
  connect_force_idle, connect_force_idle_with_modifier, default_client, default_server,
- migration::get_cid, new_client, new_server, send_something,
+ handshake_with_modifier, migration::get_cid, new_client, new_server, send_something,
  },
  ecn::ECN_TEST_COUNT,
  ConnectionId, ConnectionParameters, StreamType,
@@ -67,6 +67,30 @@ fn drop() -> fn(Datagram) -> Option<Datagram> {
  |_| None
 }
 
+fn drop_ecn_marked_datagrams() -> fn(Datagram) -> Option<Datagram> {
+ |d| (!d.tos().is_ecn_marked()).then_some(d)
+}
+
+#[test]
+fn handshake_delay_with_ecn_blackhole() {
+ let start = now();
+ let mut client = default_client();
+ let mut server = default_server();
+ let finish = handshake_with_modifier(
+ &mut client,
+ &mut server,
+ start,
+ DEFAULT_RTT,
+ drop_ecn_marked_datagrams(),
+ );
+
+ assert_eq!(
+ (finish - start).as_millis() / DEFAULT_RTT.as_millis(),
+ 15,
+ "expected 6 RTT for client to detect blackhole, 6 RTT for server to detect blackhole and 3 RTT for handshake to be confirmed.",
+ );
+}
+
 #[test]
 fn disables_on_loss() {
  let now = now();

neqo-transport/src/connection/tests/mod.rs

@@ -194,7 +194,6 @@ fn handshake_with_modifier(
  let mut did_ping = enum_map! {_ => false};
  while !is_done(a) {
  _ = maybe_authenticate(a);
- let had_input = input.is_some();
  // Insert a PING frame into the first application data packet an endpoint sends,
  // in order to force the peer to ACK it. For the server, this is depending on the
  // client's connection state, which is accessible during the tests.
@@ -213,12 +212,7 @@ fn handshake_with_modifier(
  a.test_frame_writer = None;
  did_ping[a.role()] = true;
  }
- assert!(had_input || output.is_some());
- if let Some(d) = output {
- input = modifier(d);
- } else {
- input = output;
- }
+ input = output.and_then(modifier);
  qtrace!("handshake: t += {:?}", rtt / 2);
  now += rtt / 2;
  mem::swap(&mut a, &mut b);

neqo-transport/src/ecn.rs

@@ -9,18 +9,30 @@ use std::ops::{AddAssign, Deref, DerefMut, Sub};
 use enum_map::EnumMap;
 use neqo_common::{qdebug, qinfo, qwarn, IpTosEcn};
 
-use crate::{packet::PacketNumber, recovery::SentPacket};
+use crate::{
+ packet::{PacketNumber, PacketType},
+ recovery::SentPacket,
+};
 
 /// The number of packets to use for testing a path for ECN capability.
 pub const ECN_TEST_COUNT: usize = 10;
 
+/// The number of packets to use for testing a path for ECN capability when exchanging
+/// Initials during the handshake. This is a lower number than [`ECN_TEST_COUNT`] to avoid
+/// unnecessarily delaying the handshake; we would otherwise double the PTO [`ECN_TEST_COUNT`]
+/// times.
+const ECN_TEST_COUNT_INITIAL_PHASE: usize = 3;
+
 /// The state information related to testing a path for ECN capability.
 /// See RFC9000, Appendix A.4.
 #[derive(Debug, PartialEq, Clone)]
 enum EcnValidationState {
  /// The path is currently being tested for ECN capability, with the number of probes sent so
  /// far on the path during the ECN validation.
- Testing(usize),
+ Testing {
+ probes_sent: usize,
+ initial_probes_lost: usize,
+ },
  /// The validation test has concluded but the path's ECN capability is not yet known.
  Unknown,
  /// The path is known to **not** be ECN capable.
@@ -31,7 +43,10 @@ enum EcnValidationState {
 
 impl Default for EcnValidationState {
  fn default() -> Self {
- Self::Testing(0)
+ Self::Testing {
+ probes_sent: 0,
+ initial_probes_lost: 0,
+ }
  }
 }
 
@@ -112,7 +127,7 @@ impl EcnInfo {
  /// We do not implement the part of the RFC that says to exit ECN validation if the time since
  /// the start of ECN validation exceeds 3 * PTO, since this seems to happen much too quickly.
  pub fn on_packet_sent(&mut self) {
- if let EcnValidationState::Testing(ref mut probes_sent) = &mut self.state {
+ if let EcnValidationState::Testing { probes_sent, .. } = &mut self.state {
  *probes_sent += 1;
  qdebug!("ECN probing: sent {} probes", probes_sent);
  if *probes_sent == ECN_TEST_COUNT {
@@ -138,6 +153,28 @@ impl EcnInfo {
  && (self.baseline - prev_baseline)[IpTosEcn::Ce] > 0
  }
 
+ pub fn on_packets_lost(&mut self, lost_packets: &[SentPacket]) {
+ if let EcnValidationState::Testing {
+ probes_sent,
+ initial_probes_lost: probes_lost,
+ } = &mut self.state
+ {
+ *probes_lost += lost_packets
+ .iter()
+ .filter(|p| p.packet_type() == PacketType::Initial && p.ecn_mark().is_ecn_marked())
+ .count();
+ // If we have lost all initial probes a bunch of times, we can conclude that the path
+ // is not ECN capable and likely drops all ECN marked packets.
+ if probes_sent == probes_lost && *probes_lost == ECN_TEST_COUNT_INITIAL_PHASE {
+ qdebug!(
+ "ECN validation failed, all {} initial marked packets were lost",
+ probes_lost
+ );
+ self.state = EcnValidationState::Failed;
+ }
+ }
+ }
+
  /// After the ECN validation test has ended, check if the path is ECN capable.
  pub fn validate_ack_ecn_and_update(
  &mut self,

neqo-transport/src/path.rs

@@ -1002,6 +1002,7 @@ impl Path {
  now: Instant,
  ) {
  debug_assert!(self.is_primary());
+ self.ecn_info.on_packets_lost(lost_packets);
  let cwnd_reduced = self.sender.on_packets_lost(
  self.rtt.first_sample_time(),
  prev_largest_acked_sent,