Re: [quicwg/base-drafts] Include ack_delay when deciding with PN space to arm PTO for (#3666)

martinduke <notifications@github.com> Mon, 18 May 2020 17:01 UTC

Return-Path: <noreply@github.com>
X-Original-To: quic-issues@ietfa.amsl.com
Delivered-To: quic-issues@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 19FF63A08F0 for <quic-issues@ietfa.amsl.com>; Mon, 18 May 2020 10:01:58 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.199
X-Spam-Level:
X-Spam-Status: No, score=-1.199 tagged_above=-999 required=5 tests=[DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_IMAGE_ONLY_32=0.001, HTML_MESSAGE=0.001, MAILING_LIST_MULTI=-1, SPF_HELO_NONE=0.001, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=github.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 3wO1AnJHg76z for <quic-issues@ietfa.amsl.com>; Mon, 18 May 2020 10:01:55 -0700 (PDT)
Received: from out-28.smtp.github.com (out-28.smtp.github.com [192.30.252.211]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id B8EE23A08ED for <quic-issues@ietf.org>; Mon, 18 May 2020 10:01:55 -0700 (PDT)
Received: from github-lowworker-edec459.ac4-iad.github.net (github-lowworker-edec459.ac4-iad.github.net [10.52.18.32]) by smtp.github.com (Postfix) with ESMTP id B9EC28C10CE for <quic-issues@ietf.org>; Mon, 18 May 2020 10:01:54 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=github.com; s=pf2014; t=1589821314; bh=QZDQ8rDAnLMnYaVzZr9bZNBLEb7c28xfr9+RluuJMK0=; h=Date:From:Reply-To:To:Cc:In-Reply-To:References:Subject:List-ID: List-Archive:List-Post:List-Unsubscribe:From; b=M4WLqOJY3qCzidHOMfvgNNi3Gc0xPiy9pD2Xekm77yfIRlZk+Fl7YVWiVZn5x/j5X r54qoC+QuqEN2xaSCNx2GTt5GtZ8t+BQzPU4PpUNqbLtyA6E1a8H3PIVONvIXViRZg m7YYwdklZRKe3LOWpRlyrMwEtM122cDFaGRgBulQ=
Date: Mon, 18 May 2020 10:01:54 -0700
From: martinduke <notifications@github.com>
Reply-To: quicwg/base-drafts <reply+AFTOJK6P7THYSJ3BBDD5XSF4Z2QIFEVBNHHCJ5SAW4@reply.github.com>
To: quicwg/base-drafts <base-drafts@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Message-ID: <quicwg/base-drafts/pull/3666/review/413750535@github.com>
In-Reply-To: <quicwg/base-drafts/pull/3666@github.com>
References: <quicwg/base-drafts/pull/3666@github.com>
Subject: Re: [quicwg/base-drafts] Include ack_delay when deciding with PN space to arm PTO for (#3666)
Mime-Version: 1.0
Content-Type: multipart/alternative; boundary="--==_mimepart_5ec2bf82a9acc_b4b3ff6c18cd9601112a7"; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: list
X-GitHub-Sender: martinduke
X-GitHub-Recipient: quic-issues
X-GitHub-Reason: subscribed
X-Auto-Response-Suppress: All
X-GitHub-Recipient-Address: quic-issues@ietf.org
Archived-At: <https://mailarchive.ietf.org/arch/msg/quic-issues/7XR3y1hrLqVh1xVO7hJ1tFunNwo>
X-BeenThere: quic-issues@ietf.org
X-Mailman-Version: 2.1.29
List-Id: Notification list for GitHub issues related to the QUIC WG <quic-issues.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/quic-issues>, <mailto:quic-issues-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/quic-issues/>
List-Post: <mailto:quic-issues@ietf.org>
List-Help: <mailto:quic-issues-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/quic-issues>, <mailto:quic-issues-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 18 May 2020 17:01:58 -0000

@martinduke requested changes on this pull request.



> -  space = Initial
-  for pn_space in [ Handshake, ApplicationData ]:
-    if (times[pn_space] != 0 &&
-        (time == 0 || times[pn_space] < time) &&
-        # Skip ApplicationData until handshake completion.
-        (pn_space != ApplicationData ||
-          IsHandshakeComplete()):
-      time = times[pn_space];
-      space = pn_space
+GetEarliestTimeAndSpace(times, ack_delay):
+  time, space = times[Initial], Initial
+  if (times[Handshake] != 0 &&
+      (time == 0 || times[Handshake] < time)):
+    time, space = times[Handshake], Handshake
+  # Skip ApplicationData until handshake completion.
+  if (pn_space == ApplicationData &&

I think we need to delete this line.

> @@ -1270,7 +1273,7 @@ PeerCompletedAddressValidation():
        has received HANDSHAKE_DONE
 
 SetLossDetectionTimer():
-  earliest_loss_time, _ = GetEarliestTimeAndSpace(loss_time)
+  earliest_loss_time, _ = GetEarliestTimeAndSpace(loss_time, 0)
   if (earliest_loss_time != 0):
     // Time threshold loss detection.

I confess to not really understanding this bit of code at all. If any packet has been sent in any PN space, we don't actually  go through the motions of setting the timer? Before we check for anti-amplification limit, etc?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3666#pullrequestreview-413750535