zpay32: reject duplicate payment hash fields

[Roasbeef]

Change Description

In this PR, we reject BOLT 11 invoices that contain more than one payment
hash (p) field. zpay32.Decode previously kept the first supported payment
hash and ignored later fields, so the interpreted payment hash depended on
field order.

The decoder now tracks whether a p field has appeared separately from whether
its contents parsed successfully. Any later p field returns
ErrDuplicatePaymentHash. This covers identical hashes, distinct hashes, and a
malformed first field followed by a valid one.

Steps to Test

go test ./zpay32
make lint-native

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change is not insubstantial. Typo fixes are not accepted to fight bot spam.
• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.
• Any new lncli commands have appropriate tags in the comments for the rpc in the proto file.
• There is a change description in the release notes, or [skip ci] in the commit message for small changes.

[github-actions]

🟡 PR Severity: MEDIUM

gh pr view | 5 files | 134 lines changed

🟡 Medium (2 files)

• zpay32/decode.go - BOLT-11 invoice decoding logic
• zpay32/invoice.go - BOLT-11 invoice data structures/encoding

🟢 Low (3 files)

• docs/release-notes/release-notes-0.22.0.md - release notes
• zpay32/invoice_internal_test.go - test-only change
• zpay32/invoice_test.go - test-only change

Analysis

The substantive change is confined to zpay32/*, which handles BOLT-11 invoice encoding/decoding and falls under the MEDIUM tier. No CRITICAL or HIGH packages are touched, and the change is small (3 non-test files, ~24 non-test lines), well under the thresholds for a severity bump. Test and release-note updates accompany the code change but don't independently raise severity.

---

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

[Lrifton92]

Reviewed at cd4ec81.

The change does what the description says, and I checked the parts that could go wrong rather than the parts that were claimed.

The duplicate check fires on presence, not on parse success. paymentHashSeen is set at zpay32/decode.go:304 before parse32Bytes runs at :306, so a first p field with an unsupported length (where parse32Bytes returns nil, nil, :454-456) still arms the check and the second p returns ErrDuplicatePaymentHash. That is the case the old code got wrong: the first field was skipped, the second was accepted, and the interpreted hash depended on order. The "malformed then valid" table case and the property test cover exactly that shape.

The error is reachable before the signature is touched. Decode calls parseData at zpay32/decode.go:152, and signature verification / recovery only happens at :189 and :195. So the two hand-built invoices in TestDecodeDuplicatePaymentHashes do not need a valid signature for require.ErrorIs to hold, which is why the test can be that short.

The removed test vector is not a BOLT 11 vector. I grepped the current 11-payment-encoding.md for the lnbc241pveeq09pp5...pp3... string; it is not there, so flipping it from valid to invalid does not contradict a spec example.

Two things, neither blocking.

The comment above parse32Bytes now states the opposite of the spec and of this PR. zpay32/decode.go:452-453 still says "As BOLT-11 states, a reader must skip over the 32-byte fields if it does not have a length of 52". The current reader requirements say a reader MUST fail the payment if any of p, h, s, n does not have the correct length. In practice lnd already fails a lone wrong-length p through validateInvoice (zpay32/invoice.go:386), and this PR fails the wrong-length-then-valid pair, so behaviour is right for p; only the comment is stale. Since this function is what the new check leans on, it seems worth fixing here.

The same order dependence is still there for the other single-occurrence fields. s (zpay32/decode.go:309-321), d, m, n, h, x, c, f and 9 all keep first-seen-wins, and s has the identical "malformed then valid" shape: a first s with the wrong length leaves PaymentAddr unset and the second one is taken. The spec requires exactly one p and exactly one s, and only allows multiples (with preference order) for fields like f, r and b. If the intent is to keep this PR to the payment hash only, a one-line note in the description would save the next reader from asking; otherwise s looks like the natural candidate for the same treatment in a follow-up.

LGTM.

[MPins]

Overlap with #10326. I have that one open for #9842 / #9718, implementing the reader changes from lightning/bolts#1243 (merged 2025-06-03), which replaced "MUST skip over … p, h or n fields that do NOT have data_lengths of 52, 52 or 53" with "MUST fail the payment if any field with fixed data_length (p, h, s, n) does not have the correct length". It changes parse32Bytes and parseDestination to return ErrInvalidFieldLength, reworks the same case fieldTypeP block this PR touches, and rewrites the same lnbc241pveeq09… vector that's flipped to valid: false here. Whichever lands first, the other needs a rebase — happy to rebase mine on top of this if you'd rather this one go in first.

The two checks compose differently depending on their order. This PR checks for the duplicate before parsing; #10326 parses first and then checks. I combined both locally and decoded the BOLT 11 "fields which must be ignored" example:

• duplicate check first (this PR): invoice contains multiple payment hashes
• parse first (#10326): payment hash: invalid field length

With the duplicate check first, once the length validation is in, a wrong-length p following a valid one is reported as a duplicate rather than as the length error and ErrInvalidFieldLength becomes unreachable on that path. Parsing first keeps each error pointing at the first problem found. It also makes paymentHashSeen unnecessary, since parse32Bytes no longer returns (nil, nil) — invoice.PaymentHash != nil carries the same information. The "malformed first field followed by a valid one" case from the description still fails either way after #1243, so nothing is lost.

[saubyk]

Simple but critical change. Just a few observations for consideration.

[saubyk]

The current BOLT 11 text still says a payer "SHOULD use the first p field as the payment hash", and the writer rules allow a preferred field followed by less-preferred ones. Rejecting a second p is deliberately ahead of the spec until lightning/bolts#1357 merges, so filing this under "BOLT Spec Updates" reads as if it tracks a merged spec change.

Suggest moving it to Bug Fixes (or Breaking Changes) with a one-line interop note that invoices carrying more than one p field are now refused, and linking lightning/bolts#1357 as the motivation.

[saubyk]

Two things about flipping this vector:

1. The old case was also the only coverage that wrong-length h and n fields are skipped (hp3, np3 in the string). With it flipped to invalid, a regression in that skipping would go unnoticed. Worth keeping a variant without the second p so that coverage survives.

2. valid: false here only asserts require.Error, so any decode failure passes, including a typo introduced while editing this long string. Since the point of the case is the duplicate rejection, it should pin ErrDuplicatePaymentHash (e.g. an optional wantErr on the table entry, or move it into TestDecodeDuplicatePaymentHashes).

[saubyk]

The drawn bytes cannot influence the outcome: parseTaggedFields returns ErrDuplicatePaymentHash on seeing the second p type before it ever reads the hash contents, so rapid.Check repeats the same two fixed assertions 100 times. The identical/distinct pair is also already covered end-to-end in TestDecodeDuplicatePaymentHashes.

Suggest replacing this with two table rows in TestParseTaggedFields (identical, distinct) and dropping the rapid import from this file.

[yyforyongyu]

Pending minior release notes, otherwise LGTM!

[saubyk]

Also release notes to target 0.20.5 and 0.21.4 instead of 0.22.0