Skip to content

fix(tcp): parse all five MP_CAPABLE lengths of RFC 8684 (#1042) - #1048

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1042-mptcp-capable-lengths
Oct 5, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1042-mptcp-capable-lengths

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort)
  • make test passes, and a test case covers the change
  • Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible

What is the purpose of your pull request?

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • chore — anything else

Description of your pull request and other information

Closes #1042. MP_CAPABLE now reads and makes all five RFC 8684 §3.1 lengths: 4, 12, 20, 22 and 24.

  • Schema: skey is present when length > 4 and rkey when length > 12. New fields are dl_len (UInt16, > 20) and checksum (2 octets, > 22), named to match DSS.
  • Reader: the guard now accepts (4, 12, 20, 22, 24).
  • Maker: takes dl_len and checksum, which **kwargs used to drop silently. It picks the length from how many of skey/rkey/dl_len/checksum are non-None. Passing a field without the one before it raises ProtocolError.
  • Data model change callers can notice: MPTCPCapable gains dl_len and checksum, and skey becomes Optional[int]. Direct constructors must pass both new fields, and to_dict() gains two keys.
  • Tests: a parse test and a make→read test for each length. 4/22/24 failed on main and 12/20 failed only on the missing dl_len. The [OptNo message-count tripwire goes from 32 to 33. No EXPECTED_FAILURES entry covered MP_CAPABLE.

make pylint/isort were not run in full. mypy reports nothing new in the touched regions.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 5, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on f1f0f2179: GOOD TO GO (ran on Sonnet; author Opus).

  • Schema matches RFC 8684 Figure 4, checked against the RFC text: skey if length > 4, rkey if > 12, then dl_len (16 bits) and checksum (16 bits, optional), giving lengths 22 and 24. Not checking flag A against the length is defensible: Length decides which fields are present.
  • Reader: 4, 12, 20, 22 and 24 parse, with each field None below its length. Lengths 3, 5, 11, 13, 19, 21, 23, 25 and 28 each raise ProtocolError: TCP: [OptNo 30] invalid format.
  • Maker: of all 16 combinations of set and unset fields, only the five contiguous prefixes build, and each round-trips byte-exact. The other 11 raise. Passing only skey still builds the 20-octet form (examples/generators/options.py:448).
  • Tests: the new tests fail on main (13 failed). The [OptNo count is 33, measured, so the 32→33 pin update is right. All MPTCP modules and test_option_roundtrip_unit pass.
  • Changelog: changelog_md.py --check is in step. The pin is 164 against a measured 164.

Labelled breaking. The data model MPTCPCapable.__init__ now requires dl_len and checksum positionally, so an existing constructor call raises TypeError. skey also becomes Optional[int], and to_dict() gains two keys. The changelog entry already says so.

Non-blocking: dl_len=70000 wraps silently, and the checksum length is not checked. Both are library-wide field behaviour, and DSS does the same.

@JarryShaw JarryShaw added breaking Breaks public-facing behaviour or API (apply alongside the type label) review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 5, 2026
@JarryShaw
JarryShaw force-pushed the fix/1042-mptcp-capable-lengths branch from f1f0f21 to a785d9b Compare October 5, 2026 21:20
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 5, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on a785d9bb7: GOOD TO GO (ran on Sonnet; rebase re-check).

git range-diff against the reviewed head f1f0f2179 shows exactly one changed hunk: the process.rst:104 pin, 164→165. The code and tests are byte-identical. On the new head, the count measures 165 and changelog_md.py --check is in step. test_tcp_mptcp_capable_length_unit gives 18 passed, test_conventions_doc_claims 37 passed, and test_changelog_md 60 passed. The earlier verdict and the breaking label stand.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 5, 2026
@JarryShaw
JarryShaw merged commit fcc4d57 into main Oct 5, 2026
73 checks passed
@JarryShaw
JarryShaw deleted the fix/1042-mptcp-capable-lengths branch October 5, 2026 23:52
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 5, 2026
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaks public-facing behaviour or API (apply alongside the type label) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

tcp: MP_CAPABLE parses only 12 and 20 of RFC 8684's five lengths

1 participant