HyStart++: Modified Slow Start for TCP
RFC 9406

Thanks for this interesting and useful spec. I have a few comments, which I hope may be helpful.

## COMMENTS 

### Section 4.2

I found the almost-but-not-quite-C pseudocode ever so mildly problematic. Two specific points:

- I’m not confident that clamp() is such a universally understood idiom that you should use it without defining it.

- The pseudocode block below wouldn’t work right if it were actual C, because of the lack of braces in the outer and inner if statements. 

```
if (rttSampleCount >= N_RTT_SAMPLE AND
    currentRoundMinRTT != infinity AND
    lastRoundMinRTT != infinity)
  RttThresh = clamp(MIN_RTT_THRESH,
                    lastRoundMinRTT / 8,
                    MAX_RTT_THRESH)
  if (currentRoundMinRTT >= (lastRoundMinRTT + RttThresh))
    cssBaselineMinRtt = currentRoundMinRTT
    exit slow start and enter CSS
```

Admittedly it visibly isn’t quite C and the meaning is reasonably clear from the use of indentation. However since the pseudocode is the only specification of the algorithm (unlike some specifications that use pseudocode only as an adjunct to elucidate a prose specification), I think it would be worthwhile to try extra-hard to be unambiguous, which means either (ideally) referencing a formal grammar for the pseudocode dialect you're using, or at least adding a few more hints (a definition and some braces or other block delimiters, respectively, in the case of the things I flag above).

### Section 4.2

I’m curious, in the pseudocode block quoted in my previous point, where did the constant ‘8’ come from? It doesn’t seem to be from the HyStart paper (I didn’t read it carefully, just grepped it for ‘8’). It’s somewhat surprising to see this as a literal instead of a tunable, without discussion.

### Section 4.3

I'm not a transport guy, and “paced TCP implementation” doesn’t mean anything to me -- I would have benefited from a reference, this seems indicated also since the term isn’t just informative but normative (“A paced TCP implementation SHOULD…”). 

### Section 4.3

Please spell out BDP instead of using the initialism. (Or define it somewhere, but since you only use it once why not just spell it out.)

### Section 9

RFC 8174 should probably be a normative reference.

## NITS

s/send heavy/send-heavy/g

# GEN AD review of draft-ietf-tcpm-hystartplusplus-13

CC @larseggert

Thanks to Stewart Bryant for the General Area Review Team (Gen-ART) review
(https://mailarchive.ietf.org/arch/msg/gen-art/t0F_6gKPTPrCO6Lo9IvuVwUHY88).

## Comments

### Section 4.2, paragraph 0
```
  4.2.  Algorithm Details
```
I found this intermingling of prose and pseudocode hard to follow. It would be
better to have a full description of the algorithm in prose, plus a full
pseudocode implementation (that used loops, etc.)

### Section 4.2, paragraph 4
```
     lastRoundMinRTT = currentRoundMinRTT
     currentRoundMinRTT = infinity
     rttSampleCount = 0
```
currentRoundMinRTT is used in the first line but only initialized in the second
line - swap?

### Section 4.2, paragraph 21
```
     If CSS_ROUNDS rounds are complete, enter congestion avoidance.

     ssthresh = cwnd
```
Is this assignment part of entering congestion avoidance?

### Section 4.2, paragraph 21
```
     If loss or ECN-marking is observed anytime during standard slow start
     or CSS, enter congestion avoidance.

     ssthresh = cwnd
```
Is this assignment part of entering congestion avoidance?

### Inclusive language

Found terminology that should be reviewed for inclusivity; see
https://www.rfc-editor.org/part2/#inclusive_language for background and more
guidance:

 * Term `traditional`; alternatives might be `classic`, `classical`, `common`,
   `conventional`, `customary`, `fixed`, `habitual`, `historic`,
   `long-established`, `popular`, `prescribed`, `regular`, `rooted`,
   `time-honored`, `universal`, `widely used`, `widespread`

## Nits

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

### Typos

#### Section 1, paragraph 2
```
-    a novel Conservative Slow Start (CSS) phase is used to determine
-       -- ^
+    a new Conservative Slow Start (CSS) phase is used to determine
+        ^
```

### Grammar/style

#### Section 5, paragraph 2
```
dropping data packets or their acknowledgments. The ACK division attack outl
                               ^^^^^^^^^^^^^^^
```
Do not mix variants of the same word ("acknowledgment" and "acknowledgement")
within a single text.

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. Review generated by the [`ietf-reviewtool`][IRT].

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments
[IRT]: https://github.com/larseggert/ietf-reviewtool

Nothing to add beyond what Lars and John already said, but please do look at the comment of the Mohit's secdir review:

https://datatracker.ietf.org/doc/review-ietf-tcpm-hystartplusplus-12-secdir-lc-sethi-2023-01-22/

    The "Security Considerations" section only contains a pointer to RFC 5681. I
    think this is insufficient. I recommend copying the text from RFC 5681 that is
    applicable while retaining a reference to RFC 5681. For example, it is not
    clear to me if the RECOMMENDATION against ACK division attack stated in RFC
    5681 is also applicable for HyStart++.

Thanks for documenting this, from the deployment section it sounds like this is useful/beneficial.

Rob

Thank you to Mohit Sethi for the SECDIR review.

# Éric Vyncke, INT AD, comments for draft-ietf-tcpm-hystartplusplus-13
CC @evyncke

Thank you for the work put into this document. 

Please find below some non-blocking COMMENT points (but replies would be appreciated even if only for my own education), and some nits.

Special thanks to Michael Tüxen for the shepherd's detailed write-up including the WG consensus **and** the justification of the intended status (even if "experimental" would have been possibly another good choice). 

I hope that this review helps to improve the document,

Regards,

-éric

## COMMENTS

After writing my review, I have noticed that some of my comments were previously also made by John Scudder.

### Section 1

Mostly a nit, but it took me a while to parse `uses delay increase` as some terms are verb or word. Suggest "an increase of the delay is used by..." or use "RTT" rather than "delay" ?

### Section 4.2

I second John's point about the function clamp(), the verb was unknown to me (non-English speaker) before googling for the term.

### Section 4.3

s/A TCP implementation is REQUIRED/A TCP implementation of HyStart++ is REQUIRED/

Please add a reference for `A paced TCP implementation`

Please expand or add a reference for `BDP`

### Section 5

Please be specific, i.e., say the year rather than `As of the time of writing`

### Section 9.1

RFC 8174 should be normative.


## NITS

### min (

Let's be consistent about "min (" vs. "min(" ;-)

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. 

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments