fix(hip): PUZZLE and SOLUTION builders must stop inventing wire-format values (#653, #654, #655)

[JarryShaw]

Fixes #653
Fixes #654
Fixes #655

One PR, and why

All three defects live in _make_param_puzzle and _make_param_solution, and they are one root cause in three guises: both builders derived wire-format values from the payload value instead of taking them from the data model. #653 derived the field width from the value, #655 derived it without consulting version, and #654 derived SOLUTION's Reserved octet from a lifetime that SOLUTION does not have — with the derivation machinery crashing on a legal input.

One half is genuinely separable and shipped here anyway, which is worth stating plainly rather than leaving in the issue: #654's exception-hygiene repair (bare ValueError to ProtocolError) serves PUZZLE too, independent of SOLUTION's rename, and #654 itself calls the two repairs separable with one riskier than the other. It could have gone first, smaller and non-breaking. It rides along because it is four lines inside the same two functions the rest of this change rewrites, and splitting it would buy a rebase rather than a smaller review.

The rest is mechanically inseparable. #653 and #655 rewrite the same len= expression in both builders, and a width resolution cannot be written twice. #654's SOLUTION half deletes the lifetime local that sits between them. And all three add or rename fields on the same two dataclasses in pcapkit/protocols/data/internet/hip.py. Split into separate PRs, every hunk would conflict and the second would be a rebase of the first, for one coherent public-API change shipped twice.

RFC text, fetched not paraphrased

https://www.rfc-editor.org/rfc/rfc7401.txt, sha256 09366b9f83dc80593172304ffcf05f57afd186aacb468ac6170a8fe26944c4be, and https://www.rfc-editor.org/rfc/rfc5201.txt, sha256 8b42d181a8e239713eb8d608c11e8e75829561d994adbd3caa96c6db6e69cef6 — both matching the hashes the issues record. SOLUTION is §5.2.5 and PUZZLE is §5.2.4 in both RFCs, and RHASH_len is defined in RFC 7401 §2.3.

RFC 7401 §5.2.5, verbatim:

    |  #K, 1 byte   |   Reserved    |        Opaque, 2 bytes        |
    ...
    Length              4 + RHASH_len / 4
    Reserved            zero when sent, ignored when received
    Random #I           random number of size RHASH_len bits
    Puzzle solution #J  random number of size RHASH_len bits

against RFC 7401 §5.2.4, where a lifetime genuinely lives:

    |  #K, 1 byte   |    Lifetime   |        Opaque, 2 bytes        |
    ...
    Length         4 + RHASH_len / 8
    Lifetime       puzzle lifetime 2^(value - 32) seconds

RFC 5201 states the same two field names, and fixes both widths as constants — §5.2.4: Random #I, 8 bytes, Length 12, "Random #I is represented as a 64-bit integer"; §5.2.5: Random #I, 8 bytes, Puzzle solution #J, 8 bytes, Length 20. RFC 7401 §2.3 defines RHASH_len as "the natural output length of RHASH in bits", which is why the width has to come from somewhere other than the version under HIPv2.

#653 — the silent difference, in bytes

Hand-written wire octets (not produced by the builder under test — the only way to present a value narrower than its field), parsed then rebuilt. Measured on 0c7f2b7c9:

  wire in  (28 octets): 01 41 00 14 01 20 6f 70 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 01 00 00 00 00
  parsed   : random=1 solution=1
  rebuilt  : wire Length field 20 -> 6
  wire out (12 octets): 01 41 00 06 01 20 6f 70 01 01 00 00
  identical across the round trip? False

and PUZZLE:

  wire in  (20 octets): 01 01 00 0c 01 20 6f 70 00 00 00 00 00 00 00 01 00 00 00 00
  rebuilt  : wire Length field 12 -> 5
  wire out (12 octets): 01 01 00 05 01 20 6f 70 01 00 00 00
  identical across the round trip? False

After, both are byte-identical: 20 -> 20 and 12 -> 12, identical across the round trip? True. Nothing raised before — the integers survive and the cycle closes, so a conformant peer reads RHASH_len = 8 bits where the sender said 64.

#654 — reachable from conformant input

The octet at SOLUTION offset 5, the Reserved position, before:

  lifetime=0      -> ValueError: expected a positive input   isinstance(exc, BaseError)=False   at pcapkit/protocols/internet/hip.py:3198
  lifetime=1      -> schema.lifetime=0x20  wire octet[5]=0x20  len=20
  lifetime=2      -> schema.lifetime=0x21  wire octet[5]=0x21  len=20
  lifetime=32     -> schema.lifetime=0x25  wire octet[5]=0x25  len=20
  lifetime=3600   -> schema.lifetime=0x2b  wire octet[5]=0x2b  len=20

and the conformant path — a SOLUTION whose Reserved is the mandated 0x00:

  wire in  : 01 41 00 14 01 00 6f 70 80 00 00 00 00 00 00 00 80 00 00 00 00 00 00 00 00 00 00 00
  parsed   : lifetime=datetime.timedelta(0)
  re-serialise raised builtins.ValueError: expected a positive input, got 0.0
  isinstance(exc, pcapkit.utilities.exceptions.BaseError) = False

After: reserved is 0x00 for every from-scratch build, the conformant input round-trips byte-identically, and a received non-zero Reserved (0x2b) is reproduced verbatim rather than re-derived.

The PUZZLE Lifetime sites, where a lifetime is real, now raise in-library:

  re-serialise raised pcapkit.utilities.exceptions.ProtocolError: HIPv2: [ParamNo 257] invalid lifetime: 0.0 is not a positive number of seconds
  isinstance(exc, BaseError) = True

Which exception, and why

ProtocolError. It is already what both readers raise for a malformed PUZZLE or SOLUTION, and it is declared ProtocolError(BaseError, ValueError) — the precedent FieldValueError(BaseError, ValueError) sets. Deriving from the builtin it replaces is what makes this non-breaking: a caller already written around today's bare ValueError keeps working. EnumError is (BaseError, TypeError), so choosing it by name would silently stop those callers catching anything. Both properties are asserted in the test.

The same guard covers the upper end, because UInt8Field wraps rather than raising — measured, 300 packs as 0x2c and -1 as 0xff — so an out-of-range lifetime would otherwise be written as some other valid-looking duration.

#655 — the version=1 / version=2 table

Before, on 0c7f2b7c9 — the v1 and v2 columns are identical at every width:

bits	SOL len v1	SOL len v2	PUZ len v1	PUZ len v2
1	6	6	5	5
8	6	6	5	5
9	8	8	6	6
15	8	8	6	6
16	8	8	6	6
17	10	10	7	7
32	12	12	8	8
56	18	18	11	11
57	20	20	12	12
64	20	20	12	12
65	22	22	13	13
128	36	36	20	20

After — v1 is the RFC's constant, v2 unchanged:

bits	SOL len v1	SOL len v2	PUZ len v1	PUZ len v2
1	20	6	12	5
8	20	6	12	5
9	20	8	12	6
15	20	8	12	6
16	20	8	12	6
17	20	10	12	7
32	20	12	12	8
56	20	18	12	11
57	20	20	12	12
64	20	20	12	12
65	ProtocolError	22	ProtocolError	13
128	ProtocolError	36	ProtocolError	20

At 65 and 128 bits the value cannot fit HIPv1's 64-bit field at all, so it is refused at build time with a message saying why, instead of building a Length the reader then rejects.

Why these widths discriminate

Multiples of 8 cannot tell the candidate formulas apart — at 8, 16, 32, 56, 64 and 128 bits the correct 2 * ceil(b / 8) agrees with #608's ceil(b / 4) and with the floor variant, which is exactly why every byte-aligned fixture passed through #608 unharmed. So 1, 9, 15, 17, 57 and 65 carry the weight (at 1 bit: correct 6, quarter 5, floor 4 — all three differ), and the byte-aligned rows are kept as controls. 57 and 65 bracket HIPv1's field: 57 is the narrowest value whose derived width reached the required 20, 65 the narrowest that overshoots.

Evidence discipline

• Import provenance. __editable__* stripped from sys.meta_path (it sits at index 4, after PathFinder), worktree at sys.path[0], PYTHONSAFEPATH=1, and pcapkit.__file__ asserted before any other import. Measured: /local/home/jarryx/GitHub/PyPCAPKit/.claude/worktrees/agent-a5bae1f1fdefc68a0/pcapkit/__init__.py, CPython 3.14.7. The "before" figures come from an immutable git archive 0c7f2b7c9 snapshot.
• Fail-then-pass. All five new tests fail on 0c7f2b7c9 (pytest exit code 1, read from a file) and pass here (exit code 0), each for the defect's own reason: AssertionError: 12 != 5, 6 != 12, 7 != 12, 8 != 12, 11 != 12 and ProtocolError not raised for Neither the HIP PUZZLE nor SOLUTION builder consults version when sizing, so HIPv1 rejects what it builds #655; ValueError: expected a positive input and 'reserved' not found in {... 'lifetime': 'timedelta' ...} for SOLUTION's Reserved octet is written as a PUZZLE Lifetime, and lifetime=0 escapes a bare ValueError from math.log2 #654; AttributeError: 'SolutionParameter' object has no attribute 'rhash_len' for Re-serialising a parsed HIP PUZZLE or SOLUTION loses leading zero octets: a len=20 parameter rebuilds as len=6 #653. In the Neither the HIP PUZZLE nor SOLUTION builder consults version when sizing, so HIPv1 rejects what it builds #655 test PUZZLE is asserted before SOLUTION deliberately, because PUZZLE's keyword arguments are unchanged by this PR and so its assertion is reached on the old tree; SOLUTION's cannot be, since there the old builder crashes on its own lifetime default first.
  One precision point, raised by the cross-review: test_hip_puzzle_and_solution_size_from_version_under_hipv1 reports PASSED on pytest's own per-test line on the old tree even though all twelve of its subtests report SUBFAILED with correct tracebacks — a quirk of pytest 9's built-in subtests plugin, not of the test. So "all five fail" holds at the process exit code and for the substance of every assertion, but anyone re-running this should grep SUBFAILED as well as FAILED.
• Coverage. 100% statement and branch on all three changed modules, before and after, with statements 1385 → 1411 and branches 316 → 334. Since every changed line already executed, the subtest count is the figure that moved: 30 tests / 434 subtests → 35 / 455.
• Scoped unit tier green: 291 passed, 1191 subtests, exit code 0. Three *_runtime.py modules (test_ip_runtime.py, test_ipv6_extension_runtime.py, test_ipv6_reassembly_runtime.py) are excluded: they are fixture-dependent tier and their 5 failures reproduce identically on the pristine 0c7f2b7c9 snapshot, so they pre-date this change and are the missing generated captures.
• EXPECTED_FAILURES unchanged — imported rather than grepped, since ** unpacking defeats a grep. Still 45 entries, same four HIP ones (ENCRYPTED, HIP_TRANSFORM, HOST_ID, R1_Counter). Nothing moved, and nothing was deleted.
• No fixture regeneration needed: the round-trip suite is unit tier and builds its own octets, and no HIP capture is committed.

Coordination with #651

#651 is being fixed in the same two files. This PR stays entirely out of the padding expressions: length=4 + schema.len + (8 - schema.len % 8) % 8 in both readers and PaddingField(length=lambda pkt: (8 - (pkt['len'] % 8)) % 8) appear only as unchanged context, and HIP.make's total_length // 8 + 4 (hip.py:615) has zero hits in the diff. The only % 8 in an added line is rhash_len % 8, a whole-octets check on a bit count in the new width helper.

Deriving the width from Data_*Parameter.length instead of adding rhash_len was considered and rejected precisely because it would require inverting that padding expression — coupling this change to #651's.

Measured against #664, not assumed

git merge-tree of this branch and #664's head reports exactly one conflict, in tests/protocols/internet/test_hip_unit.py — both pcapkit/protocols/internet/hip.py and pcapkit/protocols/schema/internet/hip.py auto-merge with zero conflict markers. So the site partitioning held.

On correctness the two are order-independent, and this was checked rather than inferred. #664's entire change to pcapkit/protocols/internet/hip.py is replacing length=4 + schema.len + (8 - schema.len % 8) % 8 with a call to a new parameter_total_len(schema.len) helper, in every reader; it never touches schema.len itself nor any builder's len=. This PR changes what schema.len is for these two parameters, and the Reserved octet. schema.len is the input to #664's helper, so the two compose: neither reads the other's quantity.

Verified empirically by extracting the merged tree and running both sides' suites against the merged library:

• all five of this PR's new tests pass against the merged library (exit code 0, 21 subtests);
• fix(hip): pad HIP parameters to the record length RFC 7401 gives, not the contents (#651) #664's HIP suite against the merged library is 26 passed / 1 failed, the single failure being its own copy of test_hip_parameter_constructors_cover_data_model_and_default_paths, which constructs SolutionParameter(lifetime=...) — a call site this PR's rename requires updating, not a logic conflict.

Getting there found a real interaction worth recording: the byte-exact assertions in this PR's #653 and #654 tests originally compared against a hardcoded literal that included four trailing padding octets, so on the merged library they failed with b'…\x01' != b'…\x01\x00\x00\x00\x00' — the merged library correctly emits no padding for a 24-octet record. That was a property of the test fixtures, not of the fix: every schema.len, rhash_len, reserved and ProtocolError assertion passed on the merged library throughout, and the #655 version table test passed unchanged. Those assertions are now padding-agnostic, so this PR no longer encodes a padding rule and merges in either order without touching its expected bytes.

Handover, whichever merges second: resolve the one test-file conflict (both sides append methods to the same class). If #665 merges first, #664 additionally needs its test_hip_parameter_constructors_cover_data_model_and_default_paths updated for the lifetime → reserved rename and the new required rhash_len. If #664 merges first, this PR needs nothing beyond the conflict resolution.

Breaking changes, deliberate

• Data_PuzzleParameter and Data_SolutionParameter gain a required rhash_len; Data_SolutionParameter.lifetime becomes reserved: int. Both appended or renamed in place, with rhash_len last so positional construction is least disturbed.
• SolutionParameter (schema) renames lifetime to reserved.
• _make_param_solution no longer accepts lifetime=; it is swallowed by **kwargs, as the library does for every unknown keyword, and the RFC-mandated zero is written instead.
• reserved= is None-sentinelled rather than defaulted to 0, so an explicit value overrides the one a parsed parameter carries. Added after the cross-review pointed out that Data_SolutionParameter is immutable, so without it a caller holding a parsed parameter had no way to write the conformant zero over a peer's non-conformant Reserved. rhash_len= works the same way; the plain data fields still let param win, as they did before.

Found and deliberately not fixed

• PUZZLE's Lifetime read is lossy below 0x0c. 2^(t - 32) seconds for t <= 11 is under timedelta's microsecond resolution, so every such octet parses to timedelta(0) and the original value is unrecoverable. That is why a PUZZLE with Lifetime = 0x00 now raises on re-serialisation rather than round-tripping: clamping would silently emit a different lifetime, which is the Re-serialising a parsed HIP PUZZLE or SOLUTION loses leading zero octets: a len=20 parameter rebuilds as len=6 #653 failure mode. Fixing it properly means carrying the raw octet, a third data-model change none of these three issues asks for. Worth its own issue.
• UInt8Field wraps silently on out-of-range input (300 → 0x2c, -1 → 0xff). Guarded here for Lifetime only. reserved is left unguarded, because this is a property of every UInt8Field in the library rather than a HIP one.
• examples/generators/options.py:975-977 is now stale. SOLUTION's {'lifetime': 1} override is a no-op (the keyword is gone), and the comment "lifetime=0 reaches math.log2(0)" now describes a ProtocolError. Left untouched on purpose: HIP parameter padding aligns the contents, not the record, so every parameter pcapkit emits is 4 (mod 8) octets #651's worker owns the HIP_COPIES commentary in that file, and two workers editing it is the clobbering case. Flagged rather than raced.
• rhash_len == 0 is accepted, i.e. a zero-width payload field, which no real hash produces. It is what the derived path yields for the default random=0, what a Length = 4 parameter parses back to, and what this builder produced before the change — so rejecting it would turn a degenerate-but-self-consistent case into a new failure for callers passing no value at all. Documented in _make_puzzle_field_width's docstring rather than changed.
• version is not validated: anything other than 1 is treated as HIPv2. That mirrors both readers, whose guards are written version == 1 rather than exhaustively, so reader and builder agree. Validating it belongs with HIP.make's public signature.
• NumberField raises a bare ValueError: negative shift count when unpacking a PUZZLE or SOLUTION whose Length is below 4. Measured identically on 0c7f2b7c9, so it pre-dates this change, and it lives in the field machinery rather than in HIP. It does mean a negative rhash_len is unreachable through the parse path.
• No changelog entry — consolidated in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657, which currently needs a rebase to drop 2c4212a02. Not rebased here.

[JarryShaw]

GOOD TO GO

Cross-review by an independent agent on a different model (Sonnet) from the one that wrote the change (Opus), briefed to falsify rather than to bless, running read-only. It fetched both RFCs and built its own pristine 0c7f2b7c9 snapshot rather than reusing mine, and re-measured at inputs disjoint from the ones in the PR body — 2, 24, 33, 48, 63, 66 and 100 bits for the version table, and RHASH_len of 8, 16, 40 and 128 bits for the round trip.

Verdicts

Claim	Verdict
A — RFC section numbers, field names, formulas (7401 §5.2.4/§5.2.5/§2.3, 5201 §5.2.4/§5.2.5)	CONFIRMED, both sha256 matched, no error found anywhere in the PR body, issues or code comments
B — #653 round trip byte-exact	CONFIRMED at four widths the PR never used
C — #654 Reserved octet before/after	CONFIRMED; design choice stress-tested, see below
D — ProtocolError is the right exception, breaks no caller	CONFIRMED; surveyed all eight (BaseError, ValueError) siblings and found none better, and confirmed no caller catches the bare ValueError on this path
E — #655 version table; raising at 65+ bits is not a regression	CONFIRMED; it verified that the old "success" was already rejected by pcapkit's own reader every time, so eager raising is a strict improvement
F — the new width guards	mostly correct, two narrow gaps found — see below
G — #651 padding separation	CONFIRMED by direct diff inspection; zero total_length hits, both padding expressions present only as context
H — coverage and EXPECTED_FAILURES	CONFIRMED, exact numeric match on an independent measurement
I — fail-then-pass	CONFIRMED, with one reporting nuance now recorded in the PR body
J — the one-PR decision	reasonable, with one nuance now stated explicitly in the PR body
K — other	no undisclosed API break; rhash_len being required is the right call

What it disputed, and what I changed

One finding was acted on. It independently spotted that reserved= was silently ignored whenever param= was given — and went further than I had, establishing that Data_SolutionParameter is immutable (parsed.reserved = 0 raises UnsupportedCall), so there was no way at all for a caller holding a parsed parameter to write the conformant zero over a peer's non-conformant Reserved. That turned a cosmetic wart into a real gap. reserved= is now None-sentinelled exactly as rhash_len= already was, an explicit value overrides param, and there is a test for it including the immutability that makes it necessary. Statements 1411 → 1414, branches 334 → 338, coverage still 100%.

Two findings were deliberately not acted on, and are now documented in the code rather than left implicit:

• rhash_len == 0 is accepted — a zero-width payload field, which no real hash produces. Rejecting it would be wrong here: it is what the derived path yields for the default random=0, what a Length = 4 parameter parses back to, and what this builder produced before the change. Rejecting it would turn a degenerate-but-self-consistent case into a new failure for callers that pass no value at all, and would take the round-trip generator's own PUZZLE/SOLUTION cases with it.
• version is not validated — anything other than 1 is treated as HIPv2. That mirrors both readers, whose guards are written version == 1 rather than exhaustively, so reader and builder agree. Validating it belongs with HIP.make's public signature, not in a width helper.

Both now have a "two things this deliberately does not reject, both of which look like oversights and are not" paragraph in _make_puzzle_field_width's docstring.

On the judgement call I asked it to stress-test — whether re-serialising should carry a received non-zero Reserved or always write zero — it argued both sides and landed on carrying it being defensible for a dissection library, on the grounds that make-after-unpack is expected to reproduce what was captured (which is exactly what #653 is about), that "ignored when received" forbids interpreting the value rather than preserving it, and that the from-scratch path already defaults to the conformant 0x00. Its objection was the missing override, which is now fixed.

Two methodology notes worth recording:

• It initially contradicted the UInt8Field wrap-around claim, getting struct.error by calling the bound field's .pack() directly. It then identified its own method as flawed — an isolated field call bypasses the schema's packing orchestration and mutates shared field state — and re-verified through the real path (bytes(Schema_PuzzleParameter(lifetime=300, ...)) → 0x2c, -1 → 0xff, 256 → 0x00), confirming the claim. Recorded because a reviewer who stopped at the first measurement would have filed a false refutation.
• It found that test_hip_puzzle_and_solution_size_from_version_under_hipv1 reports PASSED on pytest's per-test line on the old tree even though all twelve of its subtests report SUBFAILED with correct tracebacks — a quirk of pytest 9's built-in subtests plugin. So "all five new tests fail on 0c7f2b7c9" holds at the process exit code and for the substance of every assertion, but anyone re-running it should grep SUBFAILED as well as FAILED. The PR body now says so.

One extra observation it contributed that is not in any of the three issues: at RHASH_len = 16 with random=1, solution=0xFFFF the old code round-trips correctly by coincidence, because max(bit_length(1), bit_length(0xFFFF)) == 16 happens to equal the true field width. So the old formula only fails when both fields are narrower than the truth — a sharper statement of #653's blast radius than the issue's own both-fields-equal-1 example gives.

Not verified by the cross-review

Nothing was left unverifiable. CI on this PR is still queued behind other open pull requests in this repo and has not reported yet; the evidence above is local, and the lint flag sets were run locally from the Makefile's own variables (pylint 7.14 → 7.19, i.e. slightly better than main; mypy clean; isort clean).