Conversation
- 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
Only send ACKs with TS option if negotiated.
There was a problem hiding this comment.
Pull request overview
This PR updates wolfIP’s TCP behavior to only include the TCP Timestamp (TS) option in ACKs and data segments when TS has been negotiated, and expands TCP reliability logic around RTO/persist handling.
Changes:
- Gate TS option emission (ACK options, SYN/SYN-ACK TS inclusion, and data segment TS inclusion) on per-socket TS negotiation state (
ts_enabled/ts_offer). - Introduce RFC 6298-style RTO computation (SRTT/RTTVAR), Karn’s algorithm (skip RTT samples for retransmitted segments), and control-segment RTO handling for SYN/FIN states.
- Add TCP persist timer + zero-window probing and corresponding unit tests; update README to mention RTO computation support.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/wolfip.c | Implements TS negotiation gating; adds RTO/persist/control-RTO logic and new TCP socket fields/flags. |
| src/test/unit/unit.c | Adds/updates unit tests for control RTO, RFC6298 RTO updates, TS omission when not negotiated, and persist probing. |
| README.md | Documents retransmission timeout (RTO) computation support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| probe_byte = payload[0]; | ||
| if (tcp_seq_leq(seg_seq, t->sock.tcp.snd_una) && | ||
| tcp_seq_lt(t->sock.tcp.snd_una, tcp_seq_inc(seg_seq, seg_len))) { | ||
| probe_seq = t->sock.tcp.snd_una; | ||
| break; | ||
| } | ||
| probe_seq = seg_seq; |
There was a problem hiding this comment.
tcp_send_zero_wnd_probe() chooses probe_seq = snd_una when snd_una falls inside a queued segment, but still sends payload[0] as the probe byte. If snd_una is not equal to seg_seq, the probe payload no longer matches the probe sequence number, so the receiver will see an inconsistent byte/sequence pairing. Compute the payload offset as snd_una - seg_seq (when snd_una is within the segment) and use that byte for the probe.
| probe_byte = payload[0]; | |
| if (tcp_seq_leq(seg_seq, t->sock.tcp.snd_una) && | |
| tcp_seq_lt(t->sock.tcp.snd_una, tcp_seq_inc(seg_seq, seg_len))) { | |
| probe_seq = t->sock.tcp.snd_una; | |
| break; | |
| } | |
| probe_seq = seg_seq; | |
| if (tcp_seq_leq(seg_seq, t->sock.tcp.snd_una) && | |
| tcp_seq_lt(t->sock.tcp.snd_una, tcp_seq_inc(seg_seq, seg_len))) { | |
| uint32_t offset = t->sock.tcp.snd_una - seg_seq; | |
| if (offset < seg_len) { | |
| probe_byte = payload[offset]; | |
| } else { | |
| /* Fallback: should not happen if the sequence checks above are correct. */ | |
| probe_byte = payload[0]; | |
| } | |
| probe_seq = t->sock.tcp.snd_una; | |
| break; | |
| } | |
| probe_seq = seg_seq; | |
| probe_byte = payload[0]; |
|
|
||
| if (t->sock.tcp.state == TCP_LAST_ACK && tcp_seq_leq(fin_acked, ack)) { | ||
| tcp_ctrl_rto_stop(t); | ||
| t->sock.tcp.state = TCP_CLOSED; | ||
| close_socket(t); | ||
| return; |
There was a problem hiding this comment.
The new FIN-ack handling in tcp_ack() (e.g., TCP_LAST_ACK/TCP_FIN_WAIT_1 transitions + tcp_ctrl_rto_stop()) won’t run for TCP_LAST_ACK in the normal receive path: tcp_input() currently does not call tcp_ack() for TCP_LAST_ACK state, and instead closes the socket elsewhere. This means the control RTO timer may not be stopped when the peer ACKs our FIN, and the socket may be closed without validating the ACK actually covers the FIN. Consider updating tcp_input() to route ACK processing for TCP_LAST_ACK through tcp_ack() (and only close when fin_acked is acknowledged), so the control-RTO state is cleaned up correctly.
| if (ts->proto == WI_IPPROTO_TCP) | ||
| tcp_persist_stop(ts); |
There was a problem hiding this comment.
close_socket() now stops the persist timer but still leaves any active tmr_rto entry in the global timer heap. Because sockets are reused from a static array, a stale RTO timer can later fire on a different connection that reuses the same tsocket memory. Cancel ts->sock.tcp.tmr_rto (and set it to NO_TIMER) as part of close_socket() for TCP sockets (and similarly ensure control/data RTO timers are always canceled on close/reset paths).
| if (ts->proto == WI_IPPROTO_TCP) | |
| tcp_persist_stop(ts); | |
| if (ts->proto == WI_IPPROTO_TCP) { | |
| tcp_persist_stop(ts); | |
| if (ts->sock.tcp.tmr_rto != NO_TIMER) { | |
| wolfIP_timer_cancel(ts->sock.tcp.tmr_rto); | |
| ts->sock.tcp.tmr_rto = NO_TIMER; | |
| } | |
| } |
| | **Transport** | TCP | Connection management, reliable delivery | [RFC 793](https://datatracker.ietf.org/doc/html/rfc793), [RFC 9293](https://datatracker.ietf.org/doc/html/rfc9293) | | ||
| | **Transport** | TCP | Maximum Segment Size negotiation | [RFC 793](https://datatracker.ietf.org/doc/html/rfc793) | | ||
| | **Transport** | TCP | TCP Timestamps, RTT measurement, PAWS, Window Scaling | [RFC 7323](https://datatracker.ietf.org/doc/html/rfc7323) | | ||
| | **Transport** | TCP | Retransmission timeout (RTO) computation | [RFC 6298](https://datatracker.ietf.org/doc/html/rfc6298), [RFC 5681](https://datatracker.ietf.org/doc/html/rfc5681) | |
There was a problem hiding this comment.
PR description focuses on TCP timestamp negotiation (only sending TS on ACKs when negotiated), but this PR also introduces substantial additional behavior (RFC6298 RTO computation, persist timer/zero-window probes, control-segment RTO, Karn RTT sampling, congestion-control tweaks). Please either update the PR title/description to reflect the broader scope or split the additional transport/RTO/persist changes into separate PRs to keep review and regression risk manageable.
Only send ACKs with TS option if negotiated.
Based on previous PR: #35