Skip to content

Fix Retransmission Timeout calculation according to RFC6298#34

Open
danielinux wants to merge 3 commits intowolfSSL:masterfrom
danielinux:rfc6298
Open

Fix Retransmission Timeout calculation according to RFC6298#34
danielinux wants to merge 3 commits intowolfSSL:masterfrom
danielinux:rfc6298

Conversation

@danielinux
Copy link
Member

Fixed RTO calculation according to RFC6298

- Fixed bug in smooth average RTT calculation
- Actually assign calculated RTO value (previously fixed to 1s)
- Enforce min/max RTO according to RFC
- Enforce Karn to only account for new ACK in RTO calculation

Depends on merging #33 first.

- Fixed bug in smooth average RTT calculation
- Actually assign calculated RTO value (previously fixed to 1s)
- Enforce min/max RTO according to RFC
- Enforce Karn to only account for new ACK in RTO calculation
@danielinux danielinux requested review from Copilot and gasbytes and removed request for Copilot February 19, 2026 16:56
Copilot AI review requested due to automatic review settings February 19, 2026 17:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements RFC 6298-compliant retransmission timeout (RTO) calculation for TCP, fixing several bugs in the existing implementation.

Changes:

  • Corrected the smooth RTT (SRTT) calculation formula to properly implement RFC 6298's exponential weighted moving average
  • Enabled actual assignment of calculated RTO values (previously hardcoded to 1 second) with min/max clamping
  • Implemented Karn's algorithm to exclude retransmitted segments from RTT measurements

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/wolfip.c Implements RFC 6298 RTO calculation, adds control RTO timer for connection management states, fixes SRTT formula, enforces RTO bounds, and applies Karn's algorithm
src/test/unit/unit.c Adds comprehensive test coverage for new RTO calculation logic and control state RTO behavior
README.md Documents RFC 6298 and RFC 5681 compliance for RTO computation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

else
err_ms = sample_ms - srtt_ms;
t->sock.tcp.rttvar = (3U * t->sock.tcp.rttvar + (err_ms << 2)) >> 2;
t->sock.tcp.srtt = (7U * t->sock.tcp.srtt + (sample_ms << 3)) >> 3;
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SRTT update formula does not match RFC 6298. According to RFC 6298 section 2.3, SRTT should be updated as SRTT = (7/8) * SRTT + (1/8) * R'. The current implementation shifts the sample by 3 (<< 3), but SRTT is already scaled by 8. This double-scales the new sample. The correct formula should be t->sock.tcp.srtt = (7U * t->sock.tcp.srtt + sample_ms) >> 3; to properly compute the weighted average.

Suggested change
t->sock.tcp.srtt = (7U * t->sock.tcp.srtt + (sample_ms << 3)) >> 3;
t->sock.tcp.srtt = (7U * t->sock.tcp.srtt + sample_ms) >> 3;

Copilot uses AI. Check for mistakes.
err_ms = srtt_ms - sample_ms;
else
err_ms = sample_ms - srtt_ms;
t->sock.tcp.rttvar = (3U * t->sock.tcp.rttvar + (err_ms << 2)) >> 2;
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RTTVAR update does not follow RFC 6298. According to RFC 6298 section 2.3, RTTVAR should be updated as RTTVAR = (3/4) * RTTVAR + (1/4) * |SRTT - R'|. Here RTTVAR is scaled by 4, and err_ms is the absolute difference. However, the formula shifts err_ms by 2 (<< 2), which scales it by 4 instead of just adding it. The correct formula should be t->sock.tcp.rttvar = (3U * t->sock.tcp.rttvar + err_ms) >> 2;.

Suggested change
t->sock.tcp.rttvar = (3U * t->sock.tcp.rttvar + (err_ms << 2)) >> 2;
t->sock.tcp.rttvar = (3U * t->sock.tcp.rttvar + err_ms) >> 2;

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments