REVIEW-80d0966.md (1852B)
1 # Review: 80d0966 2 3 ## Findings (ordered by severity) 4 5 - High: `encodeEnvelope` can append an extension TLV, but `decodeEnvelope` never 6 parses or surfaces extensions, so extension data is lost and invalid 7 extensions can’t be detected; this breaks roundtrips for any envelope using 8 `mext` and contradicts the API comment about invalid extensions. 9 (`lib/Lightning/Protocol/BOLT1.hs:512-517`, 10 `lib/Lightning/Protocol/BOLT1.hs:638-655`) 11 12 - High: `decodeTlvStream` is hard-wired to treat only TLV types 1 and 3 as 13 “known,” so any even TLV in other contexts (including future extension TLVs) 14 will be rejected even when it should be accepted. This makes the exported TLV 15 decoder unusable for anything besides `init_tlvs`. 16 (`lib/Lightning/Protocol/BOLT1.hs:264-275`) 17 18 - Medium: Length fields are encoded with `fromIntegral (BS.length ...)` and no 19 bounds checks, so payloads longer than 65535 bytes will wrap and produce 20 invalid encodings instead of failing fast. This affects every `u16` length 21 field encoder. 22 (`lib/Lightning/Protocol/BOLT1.hs:435-487`) 23 24 - Medium: `decodeMessage` treats `MsgUnknown` odd types as 25 `DecodeInsufficientBytes`, which is misleading and can cause clients calling 26 `decodeMessage` directly to close/abort when the spec says unknown odd should 27 be ignored. 28 (`lib/Lightning/Protocol/BOLT1.hs:624-636`) 29 30 - Low: `unhex` uses `error` on invalid input, which is a partial failure path in 31 tests; consider total helpers in tests for consistency with safety guidance. 32 (`test/Main.hs:328-331`) 33 34 ## Open questions / assumptions 35 36 - Should `decodeEnvelope` return extension TLVs (e.g., via `Envelope`), or should 37 there be a separate decode API for validating and extracting extensions? 38 - Should `decodeTlvStream` accept a known-type predicate or map so it can be 39 reused across message-specific TLVs? 40