Bringing TCP implementation on pair with the Linux packetdrill test suite - #1155
Open
rhornig wants to merge 61 commits into
Open
Bringing TCP implementation on pair with the Linux packetdrill test suite#1155rhornig wants to merge 61 commits into
rhornig wants to merge 61 commits into
Conversation
…tChunk Both chunk types carry a data value that says what the counted bits or bytes actually contain. Peeking a sub-range built the new chunk from the length alone, so the slice silently reverted to the default fill instead of the original one, and serializing it produced different bytes than serializing the same region of the parent. Pass data through to the slice. No test moves: nothing in the suites splits a count chunk whose fill differs from the default. It matters for packets that carry a deliberate fill pattern and are later sliced -- retransmission queues do exactly that. Verified: fingerprints identical to the previous commit; full module suite 315 PASS, with only the 4 pre-existing ExternalProcess failures that also fail at master.
BEHAVIOR CHANGE -- affects the size of every PPP frame. RFC 1331's frame carried a trailing FCS field, which INET modelled as two bytes appended to every frame and then never checked. RFC 1661, which superseded it, places framing and the FCS below the layer this module represents. Dropping the trailer removes two bytes from every PPP frame. Nothing computed or verified it, so no check is lost; what changes is the frame size, and with it the timing of anything sent over a PPP link. Also adds endTxSchedulingPriority, which controls how a transmission-complete event is ordered against others at the same instant.
…_DIR LINKTYPE_PPP_WITH_DIR prefixes each record with a direction byte, and the recorder was not writing one -- so every captured PPP frame was read back one byte short, with the PPP header's first byte consumed as the direction and the rest of the frame misaligned. Declaring the traces as plain LINKTYPE_PPP matches what is actually written. Affects the contents of written .pcap files only; no simulation behavior depends on it. Verified: fingerprints identical to the previous commit, 50 module tests PASS.
Introduces the wire-level type surface these two features need, ahead of the logic that drives them: - option kinds 34 (TCP Fast Open, RFC 7413), 172/174 (AccECN) and 254 (the RFC 4727 experimental kind, which carries the pre-standardization Fast Open option identified by its 0xF989 sub-type) - the TcpOptionTcpFastOpen, TcpOptionTcpFastOpenExp and TcpOptionAccEcn classes, plus serializer support for reading and writing them - TcpHeader::aeBit, the AccECN Echo bit that repurposes RFC 3540's NS bit Nothing negotiates or emits any of this yet, so it is inert: the AE bit is written into the reserved nibble that previously always held zero, and it defaults to false, so every serialized header is byte-identical to before. Also refreshes the RFC citations in the option table to the current documents (793 -> 9293, 1323 -> 7323). Verified: all 516 fingerprint rows identical to the previous commit, 50 module tests PASS.
…elds Widens the application-facing TCP contract so socket users can express the options and per-write semantics the later features implement: - socket commands for timestamping, notsent_lowat, TCP_MAXSEG, TCP_NODELAY, TCP_CORK, socket ownership and writer-blocked state, plus Fast Open on open - per-write request tags: end-of-record, "more data coming" (MSG_MORE), zerocopy, and TX timestamping - a zerocopy completion callback on TcpSocket::ICallback - the congestion-control, RTT and chrono fields on TcpStatusInfo that a status request reports back This is contract surface only. The commands have no handlers yet and nothing sends the tags, so behavior is unchanged; the features that act on them arrive in later commits. Verified: all 516 fingerprint rows identical to the previous commit, 50 module tests PASS.
…untime Marks advertisedWindow, windowScalingFactor and timestampSupport @mutable so an application or test harness can model a setsockopt() that changes them before a connection is configured -- SO_RCVBUF in particular, which sets the offered window and, with windowScalingFactor back on automatic, the window scale that goes out in the SYN. NED-level only: it lifts the "not allowed at runtime" restriction on assignment and changes no default and no code path. Verified: all 516 fingerprint rows identical to the previous commit, 50 module tests PASS.
When the interface is disconnected while a frame is on the wire, the partially sent packet is truncated to the fraction that made it out, flagged as errored and delivered. That fraction was computed in bits, so it could land on a non-byte boundary -- and INET's byte-granular chunks cannot represent a sub-byte length, which makes the resulting packet unserializable: any attempt to serialize it (a pcap recorder, or a fingerprint ingredient that walks the bytes) dies converting bits to bytes. Truncate to a whole byte instead. Inert as things stand: with the current defaults no run produces a non-byte-aligned truncation. It matters once the connection carries TCP options and a shorter retransmission timeout, where a retransmit lands inside the disconnect window -- so this fix has to precede those changes. Verified: fingerprints identical to the previous commit (arptest included in the scope, since it exercises the reconnect path), 50 module tests PASS.
The base specification and several extensions have been superseded, so the references in the guides and the example configurations pointed at obsolete documents: RFC 793 -> RFC 9293 (TCP) RFC 1323 -> RFC 7323 (window scaling, timestamps) RFC 3517 -> RFC 6675 (SACK loss recovery) RFC 896 -> RFC 1122 (Nagle) Comments and prose only. Verified: fingerprints identical to the previous commit (the touched example configurations are all in the fingerprint scope).
The signal handles were split across two classes as protected statics, so which class owned a signal was an accident of which one first emitted it, and a new emitter had to either befriend the owner or re-declare its own. Move them to namespace scope in TcpSimsignals, so any TCP class can emit any signal by name. Every registered signal name is unchanged.
Two collaborators a TCP flavour can choose independently: how it grows the window when an acknowledgement takes new data off the wire, and what it does when it sees signs of loss. Separating them is what lets a flavour vary one without reimplementing the other. Declarations only; the implementations and the flavours that select them follow.
The standard growth law -- slow start, then the SMSS*SMSS/cwnd increment -- and the standard reaction to duplicate acknowledgements, taken out of the flavours that each carried their own copy.
NewReno's contribution over RFC 5681 is what it does with a partial acknowledgement: one that advances snd_una without covering the whole window outstanding when the loss was detected means another segment was lost, and recovery continues rather than ending.
The scoreboard-driven procedure lived in TcpConnectionSackUtil, so the connection owned a policy that belongs to the algorithm: which segments count as lost, what may be sent during recovery, and when the episode ends. It becomes a recovery strategy like the other two, and TcpConnectionSackUtil goes away. This is the strategy the branch's later loss-detection work builds on -- RACK, PRR and the undo paths all attach here rather than to the connection.
CUBIC replaces Reno's linear growth with a cubic function of the time since the last window reduction: the window climbs quickly back towards where loss occurred, flattens around it, then probes beyond. Growth no longer depends on the round-trip time, which is what makes it fair between paths of different lengths. Includes HyStart, which leaves slow start when an ack train spans the minimum round trip or the round's delay rises above it, rather than overshooting into loss. Only the growth law and the multiplicative decrease are CUBIC's own; recovery is the RFC 6675 strategy, which is what the split above makes possible.
TcpTahoe, TcpReno, TcpNewReno, DcTcp, TcpVegas and TcpWestwood each shrink to a choice of congestion control and recovery, plus whatever is genuinely their own. The base classes are renamed for what they are: TcpBaseAlg becomes TcpAlgorithmBase, and TcpTahoeRenoFamily becomes TcpClassicAlgorithmBase, since DcTcp and others derive from it too.
enqueueSentData()'s split branch inherited the lost/sacked flags from the region being split and then unconditionally cleared lost again on the very next line, so the trailing fragment of a partially retransmitted region always came back unmarked and with transmitCount=1. Consequences, all on the default lossDetectionMode="rack" path: setPipe() counted the fragment under both its rules (not-lost and retransmitted), one segment too high, which starved PRR right after the fast retransmit; F-RTO's transmitCount<=1 scan read the fragment as a first transmission and declared spurious timeouts that never happened; RACK's Karn guard was bypassed because the fragment claimed to have been sent only once; and getLost() under-reported. The fragment is a piece of the region it is split from -- lines below it truncate that region to prove it -- so it now also inherits transmitCount+1 and the original firstSentTime, keeping lastSentTime at the current time.
pipe can legitimately exceed the advertised window: RFC 6675's setPipe counts retransmitted-but-not-lost octets twice, and snd_wnd can shrink. The unsigned subtraction then wrapped to ~4G, so rule (2) always claimed that unsent data was sendable and returned before rules (3) and (4). stepC's window guard sent nothing, the rule-(3) last-resort retransmission was skipped, and recovery stalled to the RTO on every duplicate ACK. The arithmetic itself predates the recovery split (moved here by d38affbcf72); what is new is that stepC now enforces the window, turning the bogus rule-(2) hit into a dead end instead of an oversend. DcTcp still reaches the older unguarded sendDataDuringLossRecoveryPhase twin, so window enforcement differs between the two recovery send loops; that is left to the follow-up cleanup.
readHeaderOptions() reached the SACK handler through a check_and_cast, so a SACK block arriving on a connection whose algorithm has no recovery object (Vegas, Tahoe, Westwood, DumbTcp, or any flavour before established()) or a non-SACK one (Rfc5681/Rfc6582Recovery when SACK was never negotiated) aborted the whole simulation. The graceful "SACK received but sack_enabled is false" path that used to handle this had become unreachable behind the cast. Under emulation the abort is remotely triggerable by a peer that simply sends a SACK. Wire input is now treated like every other malformed option: log and mark the option invalid. The addSacks() sibling on the send path gets the same guard -- a SACK scheduled while still in SYN_RCVD has no recovery object to render it.
serializeOption() had no case for TCPOPTION_RFC3692_STYLE_EXPERIMENT_2, so the TcpOptionTcpFastOpenExp options the connection emits under fastopenExpOptionEnabled fell through to the default branch and threw "Unknown TCPOption kind=254". Every serializing configuration -- computed checksums, PcapRecorder, emulation -- died on the first Fast Open SYN. The new case mirrors deserializeOption()'s handling: the 0xF989 magic sub-type followed by the cookie bytes. The wire-format unit test only deserialized hand-crafted bytes, which is why the gap went unnoticed; it now round-trips through the serializer as well and pins the emitted bytes.
The deprecation shim threw unless initialWindow equalled "rfc2001", but the parameter's NED default is "rfc6928". The combination the shim exists to support -- an old ini that sets only increasedIWEnabled=true -- therefore always aborted, while the genuinely conflicting one (both knobs set) was the only accepted case. Comparing against the actual default restores the intended precedence rule and matches the ecnWillingness/tcpEcnMode shim that cites this block as its precedent. tcp_iw_2 pins it: increasedIWEnabled alone now warns and yields the RFC 3390 initial window.
processEce() was extracted from TcpReno::receivedDataAck by 6dfe0b153f7 but never called from anywhere, so classic ECN was inert for every non-DCTCP flavour: the receiver's ECE never reduced cwnd, CWR was never sent, and the peer's ecnEchoState stayed latched for the rest of the connection. It is called again from the classic ACK path, in the position the original occupied: only outside loss recovery, and, when it does react, in place of that ACK's slow-start/congestion-avoidance growth, which is why its return value now reports whether a reduction actually happened rather than merely whether an ECE was pending. DcTcp overrides receivedAckForUnackedData wholesale and keeps its own RFC 8257 response, so it does not double-react. tcp_ecn_reno_1 marks a client segment CE and pins the halving and the CWR.
…ic ones Duplicate-ACK detection used to live in the flavour-independent receive path; the recovery split moved it into TcpClassicAlgorithmBase and TcpCubic and left the base implementation empty. TcpVegas and TcpWestwood derive straight from TcpAlgorithmBase and override neither, so state->dupacks stayed at zero for their whole lifetime: TcpVegas::receivedDuplicateAck and TcpWestwood::receivedDuplicateAck became unreachable, taking Vegas' fine-grained timeout retransmit (which arms itself there via v_worried) with them, and every loss cost a full RTO. The counting moves back into TcpAlgorithmBase as countDuplicateAck(), with the "is this a duplicate" test factored out as isDuplicateAck() so the flavours that own a recovery object can keep deferring to it. TcpClassicAlgorithmBase and TcpCubic now share that one implementation; TcpTahoe keeps its own, which also drives its slow-start reset.
Rfc5681Recovery inflated cwnd by SMSS per duplicate ACK but never set state->lossRecovery, so the recovery-ending ACK was routed to ordinary congestion-avoidance growth and RFC 5681 step 6 never ran. Its receivedAckForUnackedData() -- where that deflation belongs -- was an unreachable "Not implemented" throw. Non-SACK Reno therefore came out of every loss episode at roughly ssthresh + k*SMSS, i.e. never actually reduced its window, permanently more aggressive than Reno. Entering fast retransmit now starts the recovery phase and the ending ACK sets cwnd back to ssthresh, mirroring the correctly ported Rfc6582Recovery (without its NewReno partial-ACK handling, which is 6582's job). Since the shared duplicate-ACK counter freezes for the duration of a recovery phase, step 4's per-duplicate-ACK inflation now keys off arrival inside the phase rather than off the counter, leaving in-recovery behaviour unchanged. tcp_fastrexmit_1 and the four stress tests pin traces recorded with the missing deflation; they are re-recorded. The transfers still complete with the same byte counts -- only the post-recovery pacing changes, and not uniformly in either direction (stresstest_1 and _2 finish later, _3 and msgq_1 earlier).
…umbing TcpCubic -- the default flavour -- derived straight from TcpAlgorithmBase while driving a recovery strategy, so it missed everything TcpClassicAlgorithmBase does around that strategy, despite frtoEnabled/tlpEnabled/lossUndoEnabled/ prrEnabled all defaulting to true: - tlpHighSeq was never cleared, so a Tail Loss Probe could fire at most once per connection and its congestion response never applied; - dataSent/segmentRetransmitted/segmentsAcked were not forwarded to the recovery, so PRR's prrOut undercounted (and PRR over-sent), F-RTO never saw an ACK for the pre-timeout data and so could never detect a spurious RTO, and D-SACK loss undo plus cumulative-ACK reordering detection were dead; - the retransmission timer never gave the recovery its onRexmitTimeout() hook, which is where the undo snapshot and the F-RTO episode are opened; - the RFC 3168 ECN response, wired back in one commit ago, was reachable for every classic flavour except this one. The duplicated getBytesInFlight, established/getRecovery and destructor collapse into the inherited ones (byte-identical before the merge). What is genuinely CUBIC keeps overriding: the growth law, the beta*cwnd reduction -- now expressed as the calculateSsthreshForRto/calculateCwndForRto hooks the shared timer path consults -- and the curve/HyStart reset after a timeout. Audited the remaining flavours on TcpAlgorithmBase and left them there: TcpTahoe predates fast recovery (a duplicate-ACK threshold takes it straight back to slow start), TcpVegas and TcpWestwood replace loss-based recovery with delay- and bandwidth-based retransmission of their own, and TcpNoCongestionControl has nothing to recover. None of them owns a recovery strategy for this base to drive.
87ca1377e0c, whose subject and every other hunk are about TcpSessionApp gaining per-write options, end-of-record and Fast Open, also turned this module's socketPeerClosed() from a no-op into an immediate close(). Nothing in that commit needs it, and it breaks the half-close case the module exists to support: a client that shuts down its write side while still reading now makes the server tear the connection down, so the server's next send() errors out. Its sibling TcpClientSocketIo, TcpServerListener and TcpServerHostApp all leave the socket alone on a peer close -- only TcpAppBase, which models a whole session rather than one composable direction, closes in response. Restores the pre-branch no-op.
…t hints at Rfc6675Recovery::prrInitCwndReduction() has had no callers since the recovery split; every loss-recovery entry path does the full PRR reset inline. Delete it. The neighbouring "// TODO why?" on Rfc6582Recovery's in-recovery sendData is answered rather than deleted: the classic per-duplicate-ACK cwnd inflation of RFC 6582 step 3.4 is deliberately replaced by deflating the flight-size side (an inferred SACK per duplicate ACK), so cwnd stays an honest window and sending the plain cwnd is right.
…pe sniffing Rfc5681Recovery chose its equation-(4) ssthresh with a dynamic_cast to TcpCubic, so any future flavour -- or a CUBIC subclass -- would silently have got the Reno formula instead of its own. The flavour-specific reduction is now reached through a virtual, the way Rfc6675Recovery::step4() already reaches it. calculateSsthreshForFastRecovery() could not be reused directly because it takes the reduction from cwnd, while this call site has already computed the flight size it wants reduced (plus the one MSS for the retransmission it is about to send), so the flight-size form joins it on the algorithm interface: TcpAlgorithmBase supplies RFC 5681's max(FlightSize/2, 2*SMSS) and TcpCubic's existing calculateSsthresh() becomes the override. Both branches keep computing exactly what they computed before.
getLost()/getSacked()/getRetrans() each walked the whole rexmit queue, and getBytesInFlight() calls all three -- on a path that already runs two or three times per ACK. Worse, setPipe() re-searched the queue from the front for every octet run, twice: once in checkSackBlock() and once inside isLost(), making the hottest routine in loss recovery quadratic in the number of scoreboard regions. The three totals are now walked once and cached until something touches the queue; every mutating method invalidates. Linux keeps the equivalent lost_out/sacked_out/retrans_out permanently up to date at each mutation point, which is the same idea with twenty places to get right instead of one. Debug builds keep walking even when the cache claims to be valid and assert the two agree, so a mutation that forgets to invalidate fails a test rather than a fingerprint. setPipe() carries its scoreboard iterator across the loop instead of re-searching, and in RACK mode reads the region's own lost flag, which is exactly what isLost() computes for a sequence number inside that region. Each iteration still covers one region-suffix and the two pipe increments keep their order, so pipe comes out bit-identical.
The cleanup this comment replaces was meant to fold DcTcp's duplicated ACK path back into the shared one. Investigating it showed the duplication is not equivalent code: composing would switch on machinery the fork silently skips -- the Tail Loss Probe outcome (so tlpHighSeq is never cleared and TLP fires at most once per connection, exactly the defect TcpCubic had until it moved onto TcpClassicAlgorithmBase), PRR, the scoreboard discard when recovery ends, and stepC's effective-MSS gate and window guard. Each is a behaviour change that owes its own commit and its own fingerprint review, so none of them belongs in a behaviour-preserving series. What lands instead is the finding itself, written where the next reader of this function will hit it -- including that this path is the last caller of Rfc6675Recovery::sendDataDuringLossRecoveryPhase, the older send loop that can transmit past the receiver's advertised window.
The main TCP renamed this signal and statistic to sndSeq; tcp_lwip kept emitting sndNxt, so the two stacks recorded the same quantity under two names and no analysis could plot them together. Recording-name change only -- nothing else in the tree referenced the old name. Note that tcp_lwip is excluded from the build (opp_makemake -X), so this is not covered by any build or test in the suite.
Several comments explained a behaviour by naming the validation script that pins it -- "gtests fastopen/client/fallback-exp-opt pins the kind-34 request", "prr-ss-30pkt pins the second '. 1001:2001' retransmit", and so on. That is authoring context, not intent: it only means something to someone with the corpus open, and it goes stale the moment a script is renamed. The Linux behaviour each comment exists to justify is kept; only the citation goes. Two mentions stay on purpose. Tcp.ned's module documentation names the differential packetdrill oracle because readers need to know how the Linux parity claims were established, and windowShrinkAllowed cites an explicit-read run as an illustration of the traffic pattern the parameter needs, not as the source of its behaviour.
DcTcp overrode receivedAckForUnackedData with a copy of the pre-split classic path: its own fast-recovery deflation, its own slow start and congestion avoidance, and its own RFC 6675 A/B/C loss-recovery block. Everything it skipped by doing so was listed in the comment this commit replaces -- the Tail Loss Probe outcome (so tlpHighSeq was never cleared and TLP fired at most once per connection), PRR, the scoreboard discard when recovery ends, and stepC's effective-MSS gate and window guard. What is genuinely DCTCP is the alpha estimator and the proportional reduction, and both fit the ECN hook the shared path already calls: processEce() means "apply this flavour's ECN congestion response, and tell me whether it replaced this ACK's window growth" -- exactly the role the fork's performSsCa flag played. It now takes the acked byte count, which DCTCP needs for its windowed average and RFC 3168 ignores. The override is the whole of DcTcp's sender-side congestion control; the fork is deleted, and TcpReno's recovery and congestion-control strategies take over the rest. This also retires the last caller of Rfc6675Recovery::sendDataDuringLossRecoveryPhase, the older send loop that could transmit past the receiver's advertised window; DCTCP now goes through stepC like everything else. Behaviour: the fingerprint suite's two dctcp rows and the 74-row TCP subset come out byte-identical, the differential oracle is unchanged at 303 MATCH, and the byte-exact AccECN CE accounting test still pins marked=988. That is narrower evidence than it looks -- neither harness drives DCTCP through loss recovery, which is where the newly reachable machinery would show. One difference is worth stating: the alpha estimator now pauses while loss recovery is in progress (previously it paused only while the duplicate-ACK count was at or above the threshold, which RACK-triggered recovery never reaches). No CE evidence is lost, because the byte and packet counters it reads are cumulative and their marks lag by design, so a recovery episode's marks are folded into the following round.
Measured, not assumed: the branch tip a21803ee was built in a separate workspace and the full suite run there, so every row could be attributed. 53 rows compute a different fingerprint with this workstream applied than without it, and all 53 carry TCP traffic -- the flavours whose duplicate-ACK counting was restored (inet-vegas, inet-westwood), the default flavour that gained the TLP/PRR/F-RTO plumbing, non-SACK Reno's recovery deflation, and the many examples that simply run a TCP application (bulktransfer, nclients, shutdownrestart, ipsec, arptest, flatnet, manetrouting, pcaprecorder, the styling showcase, and BGP, whose own sessions run over TCP). Only those rows are touched. The rest of the baseline is left alone on purpose. 296 rows already mismatched at a21803ee, across OSPF, QUIC, DiffServ, UDP, MIPv6 and the visualizer -- areas this workstream does not touch and whose drift it has not investigated; folding them in here would bury them. 243 still fail after this commit, all of them failing identically at the branch tip, and no row that passed at the tip fails now. Two caveats on what this commit can and cannot claim. Every one of the 53 rows was itself already stale, so its new value necessarily also absorbs whatever earlier drift that row carried -- for a row this workstream moves, the only correct value is the one measured now, and it cannot be split. And 64 rows error out rather than run (voipstream, tcp_lwip and the OSG visualizer are excluded from this build), so they are neither recorded nor verified here.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 3 potential issues.
4 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
Comment on lines
+71
to
+81
| conn->retransmitOneSegment(false); | ||
|
|
||
| //" | ||
| // Send a new segment if permitted by the new value of | ||
| // cwnd. This "partial window deflation" attempts to ensure that, | ||
| // when fast recovery eventually ends, approximately ssthresh amount | ||
| // of data will be outstanding in the network. Do not exit the fast | ||
| // recovery procedure (i.e., if any duplicate ACKs subsequently | ||
| // arrive, execute step 4 of Section 3.2 of [RFC5681]). | ||
| //" | ||
| conn->sendData(state->snd_cwnd); |
There was a problem hiding this comment.
🔴 Partial ACKs leave recovery inflated
During NewReno recovery, receivedAckForUnackedData omits partial-window deflation before sending again. Multiple losses can therefore produce oversized transmission bursts.
Prompt for agents
Restore RFC 6582 partial-window deflation in Rfc6582Recovery::receivedAckForUnackedData. For a partial ACK, reduce snd_cwnd by numBytesAcked, then add one effective SMSS when the ACK covers at least one SMSS, emit the resulting congestion-window changes, and only then retransmit/send under the corrected window. Preserve the first-partial-ACK timer behavior. Compare with the removed TcpNewReno implementation, which performed this arithmetic before sendData().
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+615
to
+620
| void Tcp::noteFastOpenCookieRequestUnanswered(const L3Address& remoteAddr, bool usedExpOption) | ||
| { | ||
| // Deliberately creates an entry when none exists: "request differently next | ||
| // time" is worth remembering even though no cookie was learned, which is | ||
| // precisely the unanswered-request case. | ||
| auto& entry = fastOpenCookieCache[remoteAddr]; |
There was a problem hiding this comment.
🔴 Fast Open cache grows unbounded
Each unanswered cookie request uses operator[] outside the bounded insertion path. Many non-supporting destinations can grow the cache without limit.
Prompt for agents
Make Tcp::noteFastOpenCookieRequestUnanswered create entries through the same capacity-enforcing logic as setFastOpenCookie, while preserving any existing entry and its peer MSS, option form, cookie, and escalation counter. Define and enforce sensible behavior for a zero cache capacity so no entry is inserted and no empty-container erase occurs.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+1595
to
+1601
| if (!eorSeqNums.empty()) { | ||
| auto it = eorSeqNums.upper_bound(state->snd_nxt); | ||
| if (it != eorSeqNums.end()) { | ||
| uint32_t distanceToBoundary = *it - state->snd_nxt; | ||
| if (bytes > distanceToBoundary) | ||
| bytes = distanceToBoundary; | ||
| } |
There was a problem hiding this comment.
🟡 Sequence wrap loses record boundaries
After sequence numbers wrap, upper_bound cannot find a low-valued future EOR boundary. A segment can cross that boundary and lose its PSH marker.
Prompt for agents
Replace natural integer ordering for eorSeqNums, pushSeqNums, forcedPushSeqNums, and zerocopySeqNums with wrap-aware boundary handling. In sendSegment, locate and prune boundaries according to TCP sequence arithmetic rather than std::set/std::map numeric order. Add a test that starts near UINT32_MAX, queues marked writes across wrap, and verifies segmentation, PSH, and zerocopy completion order.
Was this helpful? React with 👍 or 👎 to provide feedback.
levy
added a commit
that referenced
this pull request
Sep 2, 2026
FAIL with 2 blocking findings. 61 commits, 183 files, +15840/-3685 -- the largest change this rule set has audited by an order of magnitude, and the one where the summary earns its place: 837 lines against a 15840-line diff, and the architecture reads off it directly. F-1: the branch edits three files under common/packet/, the one sealed path in the tree, and states no permission. It points straight back at us -- the registry already records that this seal does not satisfy SR-AUDIT-FIRST, because it was recorded over two unsanctioned AV-ORG clusters. Decide the seal before blocking a three-line chunk repair behind it. F-2: no WHATSNEW entry, for a change that removes the sndNxt signal and statistic, renames the two base classes every out-of-tree TCP algorithm extends, removes 41 public functions, drops PppTrailer, and moves 53 fingerprints through modernized defaults. Each break is stated well in a commit message, and a commit message is read by whoever reads commit messages. F-4 is contested and the author is not at fault: the two baseline-only commits followed PR-SPLIT-BASELINE as it stood, and the rule inverted this morning. Their content is the best baseline provenance this audit has seen -- 53 rows measured in a separate workspace, each family named, and 296 pre-existing mismatches on master recorded and deliberately left. That last number is a project-level finding in its own right: if it is right, the fingerprint suite is not currently a gate for OSPF, QUIC, DiffServ, UDP, MIPv6 or the visualizer. The same rule change withdraws F-3 of pr-1125.md, which quoted the old wording.
levy
added a commit
that referenced
this pull request
Sep 8, 2026
Three audits in a row reported a subject two or three characters over the limit in the same numbered list as a missing release note and a sealed path edited without permission. A reader who learns the numbers mean nothing skims all of them. Two faults, and the first was mine. PR-MSG-SUBJECT says "below about 72" and check-commits.sh implemented that as a hard > 72 -- a gate stricter than the rule it enforces, which is exactly the drift the rule documents warn about. The rule now states both numbers and why: aim for 72, where git log --oneline still fits an 80-column terminal once the hash is counted, and fail above 80, where the subject stops fitting on its own. Between them nothing is gained by arguing. The length is a proxy for PR-SPLIT-ONE-CHANGE, and at 73 characters that proxy says nothing. The second fault is that reports had one severity. Findings now carry three -- Blocking, Finding, Note -- defined in audit/README.md. Only the first two are numbered; a note goes in an unnumbered Notes section, and the verdict counts findings rather than notes. The gate follows the same split: it prints VIOLATION for what fails it and note: for what is advisory, and only a VIOLATION sets the exit status. Applied to the three reports that carried the old shape. #1154 and #1122 each drop from three findings to two and a note; #1155 drops from five to four, and its ten over-long subjects become one note. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
levy
added a commit
that referenced
this pull request
Sep 11, 2026
The branch has not changed: same head 33e8b0d, same base, same 61 commits, same diffstat. Two things around it moved, and both produce findings the first audit could not have. Master advanced 99 commits and the branch no longer merges -- three conflicting paths, the two PPP serializer files and showcases.csv (F-6). The last of those also means the 53 re-recorded fingerprint rows must be re-measured before they are trustworthy. Four rules were added or inverted after 2026-09-01, so this is SR-RULE-CHANGE-STALES applied to a report rather than a seal. AR-ORG-CONTRACT-PURITY, written six days after the first audit, finds ITcpRecovery holding five no-op default bodies across three implementors (F-5) -- the same shape that made IIndicatorFigure a silent break in #1125, in a brand-new interface where no out-of-tree implementor exists yet to break. AR-EXT-MINIMAL-SURFACE and AR-EXT-VIRTUAL-IS-A-PROMISE ask their two questions for the first time here, and this is the largest instance either has seen: 28 new public functions with no caller outside their own class, 95 new virtuals overridden nowhere, against 174 new public functions. Spot-checked by hand -- Rfc6675Recovery::mayUndo, nextSeg and prrCwndReduction are public virtuals called only by their own .cc, and ITcpRecovery declares none of them. Recorded as questions, not findings. One gate delta is not the branch's fault and says so: IIeee80211Band appears on the head and not on master because master repaired it after this branch's base. A gate delta against a moved master needs that check every time. Nothing from the first audit was withdrawn. F-1 through F-4 keep their numbers. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
levy
added a commit
that referenced
this pull request
Sep 11, 2026
PR-MSG-WHY said "the body gives the reason". A commit with no body satisfies that vacuously, so nothing asked whether a body exists, no gate read one, and the T4 checklist had no item for commit messages at all. Two passes of the #1155 audit marked PR-MSG-WHY a PASS while 17 of its 61 commits had no body -- including 860-, 769- and 683-line commits that implement whole RFCs. PR-MSG-BODY names the precondition. A body is owed when the change is substantial, and whenever it repairs a defect, changes behavior or implements a standard, at any size. It is not owed when the subject is the whole story, nor when the change is its own explanation: a plan or documentation commit, a regenerated file, a WHATSNEW entry. Both numbers are measured rather than chosen. Across master's last 300 commits the no-body share is flat at 3-4% for every threshold from 50 lines up, so 50 is where the project already draws the line itself; and six of the nine commits above that line with no body are exactly the exempt kinds, which is where the exemption list comes from. The gate now reports 2 on master's last 300 -- both genuine misses -- 0 on each of the six other audited pull requests, and 12 on #1155. The rule also answers the question it invites, because diluting it would be the easy mistake: the WHAT belongs to the subject and the diff, the HOW belongs to the diff except for which mechanism and why that one, and the WHY is the body's only job. A body that restates the subject in longer words is worse than none. #1155 gains F-7 and says the two earlier passes were wrong. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
levy
added a commit
that referenced
this pull request
Sep 11, 2026
… 31 removals The change summary compared members owner by owner, so a renamed class showed as a long list of removals beside a long list of additions. TcpBaseAlg -> TcpAlgorithmBase alone accounted for 22 of them. #1155 falls from 80 removals to 49, and from 41 removed public functions to 25. F-2 is corrected and is still a finding: 25 removals, a renamed base class that stops every out-of-tree TCP algorithm compiling, and a missing release note are what they were. #1173 gains one "moved to another class" row. No other audited pull request changes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
levy
added a commit
that referenced
this pull request
Sep 11, 2026
The extractor read a parameter's default by walking every literal under the param node, which swept in the literals of its nested properties. msl's default read "s" -- its @Unit. mss's read empty. And tcpAlgorithmClass's read the @examples list, which hid the single most user-visible line of #1155: the default TCP flavour changing from TcpReno to TcpCubic. opp_nedtool gives the default as an attribute of the param. Use it. #1155's changed-parameter count rises from 1 to 9, and they are exactly the "modernized defaults" its own commit message claims: sackSupport, timestampSupport, delayedAcksEnabled and limitedTransmitEnabled all false to true, mss 536 to -1, advertisedWindow 14*mss to 65535. F-2 now names them instead of saying "the defaults change". Every summary is regenerated. Three headlines move: #1155 from 14 to 22 changed; #1124 from 6 added and 3 re-signed to 15 and 11, because protected virtuals became a reported kind after its summary was first placed and eight of them gained a State & argument; #1154 by one reclassified removal. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
levy
added a commit
that referenced
this pull request
Sep 11, 2026
…ound The branch has not moved: same head 33e8b0d, same merge base, same 61 commits, same diffstat as both earlier passes. What moved is the rule set. CR-* was written today, so this is the first pass that can say what shape the series has and not only whether each commit obeys. The branch predates CR-*, so no commit carries a Change: trailer and CR-TAG-* cannot be assessed. The classification is the reconstruction in classification-on-tcp-new.md, and the summary gains a "The commits" section built from it by commit_breakdown.py. CR-DEPTH-ONE confirms F-4 from the other side: commit 15 is the branch's only name+refactor. CR-OBL-INERT passes -- all 13 commits below the behavior level were checked by hand and none touches a baseline. F-8 is new and it is about the instrument. Regenerating the summary produced 210 added where the file said 547, on an unchanged branch with the tool at the same commit. The cause is that opp_nedtool and opp_msgtool were not on the PATH: the run wrote nothing to stderr and exited 0, and the missing parsers removed every NED and message fact, 337 of the 547. The generated preamble still claimed those facts came from those parsers. With the PATH corrected the body is byte-identical to the recorded file. A reader who trusted the regenerated numbers would have concluded the earlier audit overstated the change by a factor of two and a half. Doing the audit also found an error in the guide written yesterday. It said a test obligation not discharged in the same commit breaks TR-SHIP-WITH. That rule's unit is the pull request, not the commit, so the claim was wrong and would have produced a false finding here: the branch batches 74 test files into commit 35 and satisfies the rule. PR-SPLIT-BASELINE is the one whose unit is the commit. The guide now states both units, and the report carries the twenty-commit gap between behavior and test as a note rather than a finding, because the rule's own reason is what happened. Change: doc | behavior.change | - | commit-classification
levy
added a commit
that referenced
this pull request
Sep 11, 2026
The third audit pass left eight findings and no route through them. The order is not obvious and two of the choices are not the branch author's, so the plan states both before the first step. Two decisions gate the work. D-1 decides the common/packet/ seal, which rests on two clusters the project's own ledger calls Open (decide); until it is decided, a three-line chunk repair is blocked behind a seal that SR-AUDIT-FIRST forbids. D-2 decides how far the baseline attribution goes, and it dominates the cost of everything: the reconstruction says nine commits owe a fingerprint and the branch discharges all of them in two bulk commits. The plan recommends attributing all nine and says plainly that the audit's own cheaper wording -- squash each bulk commit into the one before it -- records a false cause for the second one, whose message claims a whole workstream. Three constraints fix the order. The rename split comes before the rebase, because git detects those three renames at 53, 56 and 64 per cent similarity and a replay over 99 commits of master can take them under the threshold, which would end the blame history of three central TCP files. The baselines come after the rebase, because showcases.csv is one of the three conflicting paths, so every value measured today is stale. And F-4, F-5 and F-7 share one history pass rather than three, because each extra pass risks the rename detection again. The twelve missing bodies are the step nobody else can do, so the plan gives the question each one must answer rather than asking for twelve bodies. The subjects are good, which is the trap: "undo a reduction that turned out to be unnecessary" reads like a reason and restates what the code does. Adding the Change: trailers is in the plan for a reason that is not tidiness. With them in place CR-OBL-INERT guards the baseline step: a commit claiming no behavior change that carries a fingerprint row fails the gate. Without them that step has no mechanical check at all. F-8 is carried separately because the repair is in opp_repl. Change: plan | behavior.add | - | pr-1155-findings
levy
added a commit
that referenced
this pull request
Sep 11, 2026
Nothing in the rule set asked a fix for evidence. TR-SHIP-WITH covers new behavior, and a fix is not new behavior, so a reviewer had to take the defect on trust and a later reader who suspected a regression in the same area had no way to tell whether it was the old defect returning. PR-MSG-REPRODUCE asks for the way to see the defect happen: the configuration, the scenario, and what goes wrong. A standalone regression test is the other acceptable form and it is justified only where the defect sits on a path many things cross, where a refactor could reintroduce it unnoticed, or where it came from a misreading of a standard the next reader could repeat. Most defects earn steps and nothing more. #1155 is why this needed writing down rather than assuming. Its fourteen fix commits all carry bodies, the bodies are among the best in the project -- f2b5fd1 traces an unsigned subtraction wrapping to ~4G through to a stalled recovery, and names what it leaves for a follow-up -- and not one of them names a configuration. They explain why the code was wrong from reading the code, which is a different thing from showing the defect. The rule selects the same commits as CR-DEPTH-FIX from the other side: one asks the author to declare the intent, this one asks for the evidence. Change: doc | behavior.add | - | commit-classification
levy
added a commit
that referenced
this pull request
Sep 11, 2026
TR-BASELINE-PROVENANCE asked for the reason the new values are right, which a series can satisfy with one sentence about fifty-three rows. That leaves room for an unintended change to ride through: a fingerprint says only that the trajectory differs and never which of the two is right, so a row nobody explained is a row nobody looked at. The rule now asks for each row, traced from the change to the behavior that row records. An unexplained row is an unintended change until somebody shows otherwise, and that is the finding. Rows that share one explanation are named together -- "the 31 rows under examples/inet/ all carry TCP traffic with the default algorithm, which this commit changes from TcpReno to TcpCubic" is one explanation for 31 rows and it is complete. A count is not an explanation. #1155 is the case: 53 rows move in two bulk commits and the branch cannot say which of its nineteen fix commits moves which row. Change: doc | behavior.change | - | commit-classification
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.