lnwire: accept parameterless invalid onion payload

[vbrekher]

Change Description

BOLT 04 allows invalid_onion_payload to omit the TLV type and offset when the failure cannot be narrowed down to a specific field.

Treat an immediate EOF as the parameterless form while keeping partial encodings invalid.

Adds regression coverage for both the omitted-fields case and a type without its required offset.

Fixes #7664.

Steps to Test

go test ./lnwire

[github-actions]

🔴 PR Severity: CRITICAL

gh pr view | 3 files | 41 lines changed

🔴 Critical (1 file)

• lnwire/onion_error.go - modifies decoding logic for invalid_onion_payload failure messages in the Lightning wire protocol; incorrect parsing of onion failure TLVs can affect payment failure handling across the network.

🟢 Low (2 files)

• lnwire/onion_error_test.go - test-only change adding regression coverage.
• docs/release-notes/release-notes-0.22.0.md - release notes update.

Analysis

This PR fixes handling of the BOLT 04 invalid_onion_payload failure message so that an immediate EOF is treated as the valid parameterless form (per spec, the TLV type/offset fields are optional when the failure can't be narrowed to a specific field), while continuing to reject partial/malformed encodings. The core change lives in lnwire/onion_error.go, which falls under the wire-protocol category and directly affects how nodes parse failure messages relayed across HTLC payment paths — a change here that's subtly wrong could cause valid failure messages to be misparsed or invalid ones to be accepted, impacting payment routing/error propagation network-wide. The change is small in scope (5 lines added/1 removed in the non-test source file) and comes with new regression tests for both the omitted-fields and missing-offset cases, but the sensitivity of onion failure parsing warrants expert review.

---

_{To override, add a severity-override-{critical,high,medium,low} label.}

[Lrifton92]

Reviewed at 85cc205. No functional objection — the decode boundary is exactly where it should be, and I checked the ways it could have been wrong rather than the happy path.

err == io.EOF is exact here, not merely lucky. tlv.ReadVarInt reads the discriminant with io.ReadFull(r, buf[:1]) (tlv/varint.go:16) and returns that error unwrapped (:18). For a one-byte read io.ReadFull yields either a bare io.EOF (nothing read) or nil — never io.ErrUnexpectedEOF. A truncated multi-byte varint is converted to io.ErrUnexpectedEOF further down (:31, :47) and so still falls into case err != nil. So the parameterless form is reachable only when the varint is entirely absent, which is the BOLT 04 condition. The only argument for errors.Is would be robustness if ReadVarInt ever starts wrapping.

The type-without-offset case is genuinely covered. In TestInvalidOnionPayloadIncompleteFields the single testType byte is consumed as the varint, leaving ReadElements nothing for the uint16, so the error comes from the offset read rather than from a short varint. That is the case the change could plausibly have loosened, and it doesn't.

FuzzInvalidOnionPayload stays green, but structurally cannot cover the new path. onionFailureHarnessCustom asserts struct equality between msg and newMsg (lnwire/fuzz_test.go:608-623), not byte equality. The parameterless input decodes to {Type: 0, Offset: 0}, Encode writes varint 0 plus uint16 0, and that decodes back to {0, 0} — equal, so the harness passes.

That leaves one asymmetry worth naming: Encode (lnwire/onion_error.go) always writes both fields, so re-encoding a received parameterless failure produces three bytes where zero arrived. It is invisible to the harness for the reason above. Is there a path where a received failure is re-encoded onto the wire rather than the opaque blob being re-obfuscated? If not, this is purely cosmetic, but it is the one consequence the tests don't speak to.

The zero-value ambiguity is inert in lnd today. {0, 0} from an omitted field pair is indistinguishable from an explicit type 0 at offset 0, and the struct has no presence flag. I checked both consumers: routing/result_interpretation.go:423 penalises the reporting node on the concrete type alone, and lnrpc/routerrpc/router_backend.go:1738 maps to Failure_INVALID_ONION_PAYLOAD without surfacing the fields. Neither reads .Type or .Offset, and type 0 is not an assigned onion payload field in BOLT 04, so nothing observable changes. The one place it shows is Error(), which will render a "couldn't narrow it down" failure as InvalidOnionPayload(type=0, offset=0) in logs and in payment failure strings — arguably worth distinguishing, though I would not hold the fix for it.

LGTM.

[saubyk]

Appreciate the effort you put into the lnwire code here, @vbrekher.

As laid out in our guidelines for new contributors, PRs from first-time contributors are not prioritized for review. Rather than leave this open indefinitely, we are going to close it for now.

If you hit an actual failure that motivated this change, an issue with the reproduction details (lnd version, backend, logs, expected vs. observed behavior) would be much more useful to us than the patch on its own. We'll triage it from there.

For building a track record with the project, issue triage and reviewing open PRs are the best starting points. They demonstrate familiarity with the codebase far better than new code does, and they make future PRs from you much easier to prioritize.