interop handshakeloss/corruption tests currently unsupported

[andrewkdinh]

This is a follow-up issue for #953 and #954 which are effectively testing the same thing of dropping/corrupting 30% of packets going each way. @nhorman has a branch here with some fixes for these tests to be supported in our runner. We also updated the interop test runner to allow more than 50 handshakes, as we decided that a peer should be able to do a new handshake if needed (currently being discussed with the maintainer). However, these 2 tests still failed.

After some investigation, @Sashan noticed that the timeout seemed to be calculated incorrectly (too high). Using the following change (reducing the kInitialRtt time described in RFC 9002 from the recommended value of 333ms to 1ms), I was able to pass these 2 tests at least 50% of the time. An issue with these tests are that they are non-deterministic.

diff --git a/ssl/quic/quic_statm.c b/ssl/quic/quic_statm.c
index f1e0f65914..18d450fe94 100644
--- a/ssl/quic/quic_statm.c
+++ b/ssl/quic/quic_statm.c
@@ -50,7 +50,8 @@ void ossl_statm_update_rtt(OSSL_STATM *statm,
 }
 
 /* RFC 9002 kInitialRtt value. RFC recommended value. */
-#define K_INITIAL_RTT               ossl_ms2time(333)
+#define K_INITIAL_RTT               ossl_ms2time(1)
 
 int ossl_statm_init(OSSL_STATM *statm)
 {

From RFC 9002:

When no previous RTT is available, the initial RTT SHOULD be set to 333 milliseconds. This results in handshakes starting with a PTO of 1 second, as recommended for TCP's initial RTO

To help us pass these 2 tests, we could consider always setting kInitialRTT to a lower value (need to be discussed what a reasonable value is) and/or decrease our time threshold loss detection (i.e. decreasing kTimeThreshold).

This was decided to not be required for QUIC server MVP. Instead, we should focus on this after 3.5, particularly by writing a test to drop exact packets and where we can control the time using FAKE_TIME.

[Sashan]

I worked independently and also coming to similar conclusions. I feel the calculation of various timeouts which are part of ACK handling need some attention. There is a section 6.3 Handling Retry Packets which suggests client may recompute roundtrip timer estimate (RTT estimate) upon reception of retry. I think this might be missing piece here.

My branch qclient-server adds few debug prints to ack manager and also simple test client and server in demos/quic/server-client. Both client and server produce detailed output with timestamps so their output can be better correlated with packet dumps. The output looks like this:

{ 32.405018259 } 0xf53ebfd4700 (ossl_quic_channel_subtick:1964) channel terminating
( Server ) { 46.168402415 } Waiting for connection
( Server ) { 49.10685637 } Accepted new connection
{ 35.226111247 } 0xf534617f000 (ch_determine_next_tick_deadline:2739) infinite overridden by packatiser
{ 35.189309120 } 0xf534617f000 (ch_determine_next_tick_deadline:2739) infinite overridden by packatiser
{ 35.108681351 } 0xf534617f000 (ch_determine_next_tick_deadline:2721) enc. level [3] overriding infinite
{ 35.70296762 } 0xf534617f000 (ch_determine_next_tick_deadline:2739) infinite overridden by packatiser
{ 36.30852082 } 0xf534617f000 (ch_determine_next_tick_deadline:2721) enc. level [3] overriding infinite
{ 36.71983363 } 0xf534617f000 (ch_determine_next_tick_deadline:2739) infinite overridden by packatiser
{ 36.73200963 } 0xf534617f000 (ch_determine_next_tick_deadline:2721) enc. level [3] overriding infinite
{ 36.73257200 } 0xf534617f000 (ch_determine_next_tick_deadline:2721) enc. level [3] overriding infinite
(Server) { 50.309799164 } Request is GET /file_1234.txt

Time starts counting at zero when application is started, it's in seconds. Lines without Client/Server annotation are from SSL library.

[andrewkdinh]

There is a section 6.3 Handling Retry Packets which suggests client may recompute roundtrip timer estimate (RTT estimate) upon reception of retry. I think this might be missing piece her

@Sashan This may help, but not all servers will send a Retry Packet. As far as I can tell, quic-go doesn't usually send a Retry Packet.

[andrewkdinh]

Other servers that pass these tests use various values for kInitialRTT but also usually have it configurable. I think having a lower kInitialRTT is part of the solution, but consistently passing these tests also likely involves some more involved changes.

• quic-go: 100ms
• kwik: 100ms
• ngtcp2: 333 ms
• picoquic: 250 ms

There is a section 6.3 Handling Retry Packets which suggests client may recompute roundtrip timer estimate (RTT estimate) upon reception of retry. I think this might be missing piece here.

@Sashan I've also implemented this. Please see the diff below, patched on top of Neil's branch.

diff --git a/include/internal/quic_ackm.h b/include/internal/quic_ackm.h
index c271dfca2e..56c43bdd9d 100644
--- a/include/internal/quic_ackm.h
+++ b/include/internal/quic_ackm.h
@@ -291,6 +291,15 @@ OSSL_TIME ossl_ackm_get_pto_duration(OSSL_ACKM *ackm);
 /* Returns the largest acked PN in the given PN space. */
 QUIC_PN ossl_ackm_get_largest_acked(OSSL_ACKM *ackm, int pkt_space);
 
+/*
+ * RFC 9002: The client MAY compute an RTT estimate to the server as the time
+ * period from when the first Initial packet was sent to when a Retry or a
+ * Version Negotiation packet is received. The client MAY use this value in
+ * place of its default for the initial RTT estimate.
+*/
+void ossl_ackm_update_rtt_initial(OSSL_ACKM *ackm, OSSL_STATM statm,
+                                  OSSL_TIME received_time);
+
 # endif
 
 #endif
diff --git a/ssl/quic/quic_ackm.c b/ssl/quic/quic_ackm.c
index ce6a0bdff9..1077bed4cd 100644
--- a/ssl/quic/quic_ackm.c
+++ b/ssl/quic/quic_ackm.c
@@ -749,6 +751,19 @@ stop:
     return acked_pkts;
 }
 
+void ossl_ackm_update_rtt_initial(OSSL_ACKM *ackm, OSSL_STATM statm,
+                                  OSSL_TIME received_time)
+{
+    struct tx_pkt_history_st *h = get_tx_history(ackm, QUIC_PN_SPACE_INITIAL);
+    OSSL_ACKM_TX_PKT *pkt = ossl_list_tx_history_head(&h->packets);
+    OSSL_TIME latest_rtt = ossl_time_subtract(received_time, pkt->time);
+
+    statm.min_rtt              = latest_rtt;
+    statm.smoothed_rtt         = latest_rtt;
+    statm.rtt_variance         = ossl_time_divide(latest_rtt, 2);
+    statm.have_first_sample    = 1;
+}
+
 /*
  * Create a singly-linked list of newly detected-lost packets in the given
  * packet number space. Returns the head of the list or NULL if no packets were
diff --git a/ssl/quic/quic_channel.c b/ssl/quic/quic_channel.c
index 3c576708eb..d1eb478d0d 100644
--- a/ssl/quic/quic_channel.c
+++ b/ssl/quic/quic_channel.c
@@ -2315,6 +2315,9 @@ static void ch_rx_handle_packet(QUIC_CHANNEL *ch, int channel_only)
                  */
                 ossl_quic_tx_packetiser_set_protocol_version(ch->txp, QUIC_VERSION_1);
 
+                ossl_ackm_update_rtt_initial(ch->ackm, ch->statm, ch->qrx_pkt->time);
+
                 /*
                  * And then request a restart of the QUIC connection 
                  */
@@ -2398,6 +2401,9 @@ static void ch_rx_handle_packet(QUIC_CHANNEL *ch, int channel_only)
                       &ch->qrx_pkt->hdr->src_conn_id, old_have_processed_any_pkt))
             ossl_quic_channel_raise_protocol_error(ch, OSSL_QUIC_ERR_INTERNAL_ERROR,
                                                    0, "handling retry packet");
+
+        ossl_ackm_update_rtt_initial(ch->ackm, ch->statm, ch->qrx_pkt->time);
+
         break;
 
     case QUIC_PKT_TYPE_0RTT: