fix(hip): correct R1_COUNTER's counter width and LOCATOR_SET's Length unit (#672) (#679)

[JarryShaw]

Fixes #672. Fixes #679.

Two wire-format defects in the HIP schema and protocol modules, fixed together. Both change the
octets pcapkit emits, so both are breaking as well as fix.

Why one PR and not two

They are independent defects and I considered splitting them. Three things decided it:

1. They share three files, and in two of them the two fixes sit within 120 lines of each other —
   pcapkit/protocols/schema/internet/hip.py, pcapkit/protocols/internet/hip.py and
   examples/generators/options.py. Sequencing two PRs over the same hunks costs a rebase and invites
   exactly the clobber that collapsing Re-serialising a parsed HIP PUZZLE or SOLUTION loses leading zero octets: a len=20 parameter rebuilds as len=6 #653/SOLUTION's Reserved octet is written as a PUZZLE Lifetime, and lifetime=0 escapes a bare ValueError from math.log2 #654/Neither the HIP PUZZLE nor SOLUTION builder consults version when sizing, so HIPv1 rejects what it builds #655 into one PR avoided.
2. The headline evidence requires both. The RFC-only walk over options-internet.pcap finds two
   violations, one per defect — the R1_COUNTER phantom record and the empty LOCATOR_SET. Fixing
   either alone leaves the walk reporting a non-zero count, so neither PR could have stated a clean
   result on its own.
3. HIP_COPIES' note is only truthful once both have landed. Its one-copy tally goes 44 → 46, and
   it takes both fixes to get there: R1_COUNTER moves CONSTRUCT → OK and LOCATOR_SET moves
   MISMATCH → OK. Written twice, in two PRs, one of them would have been wrong when merged.

#672 — R1_COUNTER packed 12 octets where the RFC total is 16

R1CounterParameter.counter was a UInt32Field. RFC 7401 §5.2.3 states the width twice — "R1
generation counter, 8 bytes" in the diagram, and "contains a 64-bit unsigned integer in network byte
order" in the prose below it — and RFC 5201 §5.2.3 gives the same Reserved, 4 bytes + 8-octet layout,
so no version excused it. The parameter declared the correct len=12 and packed eight of those
twelve, for a record of 12 octets against the 16 that Total Length = 11 + Length - (Length + 3) % 8
requires. Both codes that reach the class are affected.

Measured at counter=1, before and after:

code	before	after
128 R1_Counter v1	00 80 00 0c 00000000 00000001 — 12 octets, 4 (mod 8)	00 80 00 0c 00000000 0000000000000001 — 16, aligned
129 R1_COUNTER v2	00 81 00 0c 00000000 00000001 — 12 octets, 4 (mod 8)	00 81 00 0c 00000000 0000000000000001 — 16, aligned

No padding either way: Length = 12 plus the four header octets is already a multiple of eight, so the
four missing octets were the counter's, not the padding's. A lone R1_COUNTER in a packet went from
ProtocolError: HIPv2: invalid format to parsing back with the counter intact — the two-copy pairing in
examples/generators/options.py is what had concealed that, two four-octet shortfalls summing to eight.
A counter above 2**32 now survives the round trip at all, which is the assertion no amount of padding
can satisfy.

_hip_overrides() now gives both codes a non-zero counter, which is the other half of #672 and the
more useful half. See "the fixture walk" below.

#679 — LOCATOR_SET was conformant only at one shape, by two wrongs

Two defects, neither fixable alone, which is why #651 and #664 excluded this one site deliberately
rather than accidentally:

1. padding read the nested Locator.len. ListField packs each nested schema into the enclosing
   packet context and Schema.pack opens with packet.update(self.__dict__), so by the time padding
   was evaluated — after the list, fields being packed in declaration order — pkt['len'] was the last
   locator's. That is 4 for any IPv6 locator whatever the count, so the old expression appended exactly
   four octets every time.
2. len was sum(Locator.len). RFC 8046 §4 gives Locator Length "in 4-octet units" and it covers
   only the Locator field; RFC 7401 §5.2.1's Length is "length of the Contents, in bytes". So 4n
   was declared for 24n octets of contents.

4 + 24n + 4 = 24n + 8, and since 24n is a multiple of eight the RFC total for a byte-count Length
of 24n is the same 24n + 8. Hence conformance at n = 1, 2, 5 — and nowhere else.

Shapes, not counts

Sampling counts within the homogeneous plain-IPv6 shape is how the cancellation stayed hidden: n = 1, 2
and 5 pass on main and would pass a half-fix too. Asserted against 11 + Length - (Length + 3) % 8,
written out locally in the test rather than imported from parameter_total_len:

shape	contents	len before	packed before	len after	packed after	RFC total
empty	0	0	4	0	8	8
plain IPv6 n=1	24	4	32	24	32	32
plain IPv6 n=2	48	8	56	48	56	56
plain IPv6 n=5	120	20	128	120	128	128
SPI n=1	28	5	35	28	32	32
SPI n=2	56	10	63	56	64	64
mixed plain→SPI	52	9	59	52	56	56
mixed SPI→plain	52	9	60	52	56	56

Four of the eight were not multiples of eight before. The three plain shapes are byte-for-byte unchanged
in length, which is the baseline a correct fix must not move — only the two Length octets differ,
0004 → 0018 at n = 1.

This also independently reproduces the SPI and mixed figures (35 / 63 / 59 / 60) that #679's owner
recorded as #664's worker's measurement rather than their own, having hit FieldValueError: invalid locator type or length. The reason is a rough edge left unfixed here: _make_param_locator_set sets
length = 5 for an spi but leaves type at 0, and locator_value_selector accepts only
type == 0, len == 4 or type == 1, len == 5 — so spi= needs an explicit type=1. Noted at the
test's SPI constant.

The reader consumed the same quantity — this is why #679 was a different class of change

locators is ListField(length=lambda pkt: pkt['len']), and Schema.unpack reads exactly
field.length octets off the stream and hands only those to the list. With Length at 4n, a set of n
plain locators was offered 4n octets of a 24n-octet contents. The expression was right and the
quantity was wrong, so fixing the unit repairs parsing rather than breaking it:

shape	before	after
plain n=2	1 locator parsed, SchemaWarning: packet length < 0, repack differs	2 parsed, repack identical
plain n=5	1 locator parsed, repack differs	5 parsed, repack identical
empty / SPI / mixed, whole packet	raised, or two parameters out of one record	1 parameter, correct length

The silent case is the sharpest: a one-copy plain LOCATOR_SET packet succeeded on main while
parsing as two parameters, the second fabricated out of the 20n octets never consumed, with a reported
length of 12 for a 32-octet record. _read_param_locator_set now reports
parameter_total_len(schema.len) — the last of the 49 record lengths still on the pre-#651 expression.

How the shadowing is fixed

locator_set_len_callback, installed as the locators ListField's callback, snapshots the
parameter's Length under LOCATOR_SET_LEN when the field is resolved — before any nested Locator
has packed into the context. locator_set_padding_len reads that snapshot and defers to
parameter_padding_len for the arithmetic, so the formula still lives in one place. The callback fires
on both the pack and the unpack path, which is what makes one expression correct on both rather than
accidentally correct on the one where nothing shadows anything (Schema.unpack hands each field
packet.copy(), so the shadowing is pack-only).

The fixture walk is trustworthy again — and it did not say what #672 claimed

#672 cites an RFC-only walk over options-internet.pcap reporting 0 violations over 92 records,
becoming 1 when a counter is patched to aabbccdd. A walker written from scratch for this PR — pure
struct, no pcapkit import, stride taken solely from 11 + Length - (Length + 3) % 8 — does not
reproduce those numbers, and the disagreement is worth more than the agreement:

fixture	records	violations
f0999858e, counter = 0 (as generated)	91	1 — frame 102: type 193 padding not zeroed: 00c10000
f0999858e, counter = 0xaabbccdd	91	2 — the above, plus frame 101: type 0 padding not zeroed: aabbccdd
this PR, counter = 0xaabbccdd	92	0

So the "0 violations" baseline was never right: the empty LOCATOR_SET at frame 102 —
00 c1 00 00 00 c1 00 00, two four-octet records where 11 + 0 - (0 + 3) % 8 = 8 is required — is
independently detectable by a literal RFC walk and was there all along. Patching the counter takes it
from 1 to 2, not from 0 to 1. The record count is 91 rather than 92 because the LOCATOR_SET frame's
two copies collapse into one mis-parsed record, offsetting the phantom that fills a slot in the
R1_COUNTER frame; after the fix both frames yield two real records each and the count is 92.

Cross-checked two independent ways that agree exactly — patching the generated bytes, and regenerating
from a tree whose _hip_overrides() supplies the non-zero counter.

The 0 is now a real 0, and specifically one that cannot be concealing a mis-stride, because
R1_COUNTER carries aabbccdd rather than zeros. That is what the override in _hip_overrides() is
for, and why it is worth keeping now that the width is right: a zero-valued field cannot discriminate a
width defect from a correct one, which is the same trap that let the SOLUTION width (#608) survive as
long as it did.

EXPECTED_FAILURES did not move

Verified by importing the table rather than grepping it, on both trees: 44 entries each,
byte-identical, three of them HIP. hip-parameter/R1_Counter stays, and deliberately so — its cause
is the schema registry key, not the width. At one copy on main the width defect fires first and hides
it; at two copies the pair cancels the width and this is what fails, which is the setting the table runs
at and why its recorded gap reads PARSE / "no attribute 'counter'". After this PR the registry
defect is what remains at both settings. Filed as #690, with the measurement, and not fixed here.

Follow-ups filed, not folded in

• HIP_COPIES can drop to 1 now that #672 and #679 have landed: measured 46 OK of 49 at both settings #689 — HIP_COPIES can drop to 1. Measured 44 → 46 OK of 49 at one copy, and one copy and two
  copies now give the identical set: HOST_ID, HIP_TRANSFORM and R1_Counter, none of which a second
  copy helps. Left at two here because changing it halves every hip-parameter frame in the fixture the
  walk above reads, which would make these before-and-after figures incomparable.
• HIPv1 R1_Counter (128) parses as UnassignedParameter: R1CounterParameter registers code=129 only #690 — R1CounterParameter registers code=129 only, so HIPv1 code 128 parses as an
  UnassignedParameter.
• PUZZLE's Lifetime read is lossy below 0x0c, since 2^(t−32) s falls under timedelta's
  microsecond resolution and the octet is unrecoverable. Unfiled, noted in fix(hip): pad HIP parameters to the record length RFC 7401 gives, not the contents (#651) #664, and untouched here —
  nothing in this PR goes near it.

Verification

.venv CPython 3.14.7. Every measurement took pcapkit.__file__ from the tree under test and asserted
it, having stripped the editable finder with getattr(f, '__module__', '') — filtering on
type(f).__name__ or type(f).__module__ strips nothing, indices 1-4 of sys.meta_path being classes.
Measured /local/.../worktrees/agent-a65b30a424080d398/pcapkit/__init__.py for "after" and a
git archive f0999858e extraction for "before".

• Fails without the fix: all 10 new test methods -- 4 in
  test_hip_r1_counter_width_unit.py, 6 in test_hip_locator_set_length_unit.py -- fail on a pristine
  f0999858e tree, plus the updated test_fields_ipaddress.py pin, for 11 distinct failing methods.
  None of them passes on main. Exit code read from a file rather than a pipeline: rc=1, 41 failed /
  29 passed / 58 subtests passed. With the fix: rc=0, 10 passed, 37 subtests passed.
• Scoped runs, all rc=0 read from a file: the two new modules; test_hip_unit.py +
  test_fields_ipaddress.py + test_fields_collections.py (65 passed, 226 subtests);
  test_option_roundtrip_unit.py (6 passed, 360 subtests); tests/project/ (119 passed, 471 subtests).
• Coverage stays 100% on all three HIP modules — protocols/internet/hip.py 925 statements / 314
  branches, schema/internet/hip.py 384 → 389 statements / 28 branches, data/internet/hip.py 118 — 0
  missed and 0 partial before and after. The changed lines in protocols/internet/hip.py already
  executed, so the gain is in assertions rather than reach: 586 → 623 subtests over the same scope,
  +37 from the two new modules. coverage run -m pytest, not pytest-cov.
• pylint at the Makefile's flag set, mypy at the Makefile's flag set, and isort are each at
  exact parity with f0999858e over the changed library files: the pylint message multiset is
  identical after adding the unused-argument disable the module's other callbacks carry, mypy reports
  "Success: no issues found in 2 source files" on both, and isort's two pre-existing options.py hunks
  are identical before and after (neither is in this diff — _mh_option_overrides' import ipaddress
  and scapy.all's import order).
• tests/protocols/internet/test_ip_runtime.py and test_ipv6_extension_runtime.py fail 5 tests —
  identically on pristine main, being runtime-tier cases that need make samples, which was not
  run in either tree.

The #664 exclusion guard is updated rather than deleted. It still passed unchanged, because
locator_set_padding_len is not parameter_padding_len — but its prose and failure messages had become
false. It now classifies three buckets, requires the bespoke one to be empty, and pins both that
LOCATOR_SET is the single parameter on the snapshot callback and that the callback is installed on the
locators field, so both ways of regressing the shadowing fix are caught.

No changelog file on this branch. Bullet text is handed to the maintainer for #657.

I have not claimed anything about CI.

[JarryShaw]

GOOD TO GO

Cross-review verdict, posted by the authoring agent. Reviewing model: Sonnet (the author ran on
Opus, so the reviewing model differs from the authoring one, which is the point of the exercise). No
model substitution was needed. The reviewer ran read-only in its own worktree, briefed to falsify
rather than to bless, and produced every number itself — from a separate harness, its own throwaway
scripts, a from-scratch pure-struct RFC walker, and two independent tree extractions
(git archive f0999858e). It asserted pcapkit.__file__ for every tree it measured.

Confirmed independently, not by agreement with the PR body

• RFC text. Fetched RFC 7401 (sha256 09366b9f83…, matching the digest R1_COUNTER packs 12 octets where RFC 7401 §5.2.3 requires 16: counter is 4 octets, not the stated 8 #672 cites) and RFC 5201
  directly. §5.2.3 of both states "R1 generation counter, 8 bytes" and "a 64-bit unsigned integer",
  verbatim.
• R1_COUNTER octets, re-derived: 0080000c0000000000000001 / 0081000c0000000000000001 (12,
  4 mod 8) → 0080000c000000000000000000000001 / 0081000c000000000000000000000001 (16, aligned).
• All eight LOCATOR_SET shapes, every cell reproduced, each after value equal to a locally
  derived 11 + Length - (Length + 3) % 8.
• The baseline, byte-for-byte: n = 1/2/5 pack 32/56/128 before and after, and diffing the raw hex
  shows only the two Length octets change — 0004→0018, 0008→0030, 0014→0078, nothing else.
• The reader, which the author flagged as the claim to attack hardest. Reproduced in full,
  including the silent case: a one-copy plain LOCATOR_SET on main parses as two parameters, the
  LOCATOR_SET reporting length = 12 for a 32-octet record and the fabricated second one surfacing
  as Unassigned_8193 with a garbage length of 3520. After: one parameter, length = 32.
• The fixture walk. Its own walker, generating the capture from each tree, returns 91 records / 1
  violation before, 91 / 2 with a non-zero counter, and 92 / 0 after — all three rows and both
  violation strings matching letter for letter. It states plainly that R1_COUNTER packs 12 octets where RFC 7401 §5.2.3 requires 16: counter is 4 octets, not the stated 8 #672's original "0 over 92
  becoming 1" is not reproducible and is contradicted by an independent walk.
• EXPECTED_FAILURES, imported rather than grepped: 44 entries on both trees, byte-identical over
  the positionally-unpacked Gap tuples. It went further than asked and re-ran code 128 on the fixed
  tree, confirming AttributeError: 'UnassignedParameter' object has no attribute 'counter' still
  reproduces — so the recorded cause is the registry key (HIPv1 R1_Counter (128) parses as UnassignedParameter: R1CounterParameter registers code=129 only #690), demonstrated rather than assumed.
• Coverage, re-measured with coverage run -m pytest: 384 → 389 statements in
  schema/internet/hip.py, 925/314 and 118 unchanged elsewhere, 100% with 0 missed and 0 partial on
  all three modules before and after; 586 → 623 subtests.

What it added that the review was not asked for

• locator_value_selector admits only type==0,len==4 and type==1,len==5, raising
  FieldValueError before packing for anything else — so sum(8 + Locator.len * 4) has no
  unreachable third shape to be wrong about, and it is dimensionally right per RFC 8046 §4 rather than
  right by coincidence at the two admitted cases.
• _read_param_r1_counter's schema.len != 12 guard was never wrong and needed no change: Length is
  the declared contents field (4 reserved + 8 counter), independent of what the defective writer
  actually packed.
• The param=-driven rebuild path shares the same len= expression, so it is not separately
  arithmetic'd.
• locator_set_padding_len swept against parameter_padding_len over length = 0..39 — five full
  residue cycles, identical at every point, with the negative-Length FieldValueError guard intact.
• No reachable KeyError on LOCATOR_SET_LEN: field order is fixed by the class declaration and both
  the pack and unpack loops process fields strictly in it, so padding cannot resolve before
  locators. A direct call with an empty dict does raise, which is an internal and deliberately loud
  failure mode rather than an API path.

What it disputed

One finding, and it was correct. The PR body's opening verification bullet said "9 new test
methods" where the two files declare 10 — an internal inconsistency with the same body's own
"10 passed". Corrected before the review landed; the body now reads 10, with the 4/6 split named.

It also hit the documented pytest-9 trap while establishing that the tests fail on main: 7 of the
10 methods print PASSED at the per-test line while every one of their subtests reads SUBFAILED.
Substantively all 10 have a failing assertion on main and none passes cleanly — but it is worth
recording that a reader checking only the per-test lines would have concluded the opposite.

Stated as unverified, rather than folded away

• pylint / mypy / isort exact parity with f0999858e — UNVERIFIED by the reviewer, which spent its
  budget on the RFC, reader and fixture claims instead. Those three were measured by the author
  (message multiset identical, Success: no issues found in 2 source files on both trees, and isort's
  two pre-existing options.py hunks unchanged); they are not independently confirmed.
• The aggregate 41 failed / 29 passed / 58 subtests passed from running three files together — the
  reviewer reproduced the two-file subset (40 failed / 7 passed) and the pin test individually, and did
  not run the third file whole. The arithmetic reconciles (40 + 1 pin = 41; 7 + 22 from
  test_fields_ipaddress.py = 29), but the aggregate itself is the author's measurement.

Nothing found overturns the change. This PR remains unmerged and awaits the maintainer — no CI
claim is made here.