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.

This change is deliberately stricter than the current BOLT 11 text, which
tells a reader to use the first p field; it is motivated by
lightning/bolts#1357.

Note: the same order dependence remains for the other single-occurrence fields
(s, d, m, n, h, x, c, f, 9), which all keep first-seen-wins
behavior. This PR is intentionally limited to the payment hash; the s
(payment address) field looks like the natural candidate for the same treatment
in a follow-up.

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 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!

[ziggie1984]

LGTM

Pending release notes

[ziggie1984]

Seconding @Lrifton92 on the stale comment above parse32Bytes (zpay32/decode.go:452-453), with a concrete suggestion — I think it's worth fixing in this PR rather than leaving to a follow-up:

// As BOLT-11 states, a reader must skip over the 32-byte fields if
// it does not have a length of 52, so avoid returning an error.

That tracks the pre-lightning/bolts#1243 text. The current reader requirements say the opposite:

MUST fail the payment if any field with fixed data_length (p, h, s, n) does not have the correct length (52, 52, 52, 53).

So the justification the comment gives for return nil, nil no longer exists in the spec. It matters here specifically because this PR's correctness argument leans on that return value: paymentHashSeen is set before parse32Bytes runs precisely so that a wrong-length first p still arms the duplicate check. Anyone reading the current comment will conclude the (nil, nil) return is spec-mandated, rather than a known divergence that #10326 is in the process of removing.

Suggested replacement:

	// A field with an unexpected length is reported as absent rather than
	// as an error, leaving it to the caller to decide whether a missing
	// field is fatal. Note that BOLT 11 is stricter, and requires a reader
	// to fail on a fixed-length field (p, h, s, n) with the wrong length.
	// For the payment hash the end result is the same: a lone wrong-length
	// field leaves PaymentHash nil and validateInvoice rejects the invoice,
	// and a wrong-length field paired with a valid one is rejected as a
	// duplicate.

Comment-only — behaviour for p is correct either way.

Separately, on the vectors: I ran lnd's decoder against the 28 vectors proposed in lightning/bolts#1357 (bolt11/invoice-test.json), before and after this branch. Base is 22/28, this branch is 25/28 — the three that flip are exactly the duplicate-p cases (Unknown fields and invalid fixed-length fields, Two distinct p fields…, The same p field twice…), and nothing else moves. The three still-failing vectors are pre-existing divergences unrelated to this PR: unknown required feature 100, missing s field, and non-canonical (high-S) signature with an n field are all accepted by Decode today. Probably worth an issue once those vectors land upstream.

[Roasbeef]

Thanks all. Pushed 0a7faa0 addressing the feedback:

• Release notes: moved the entry from BOLT Spec Updates to Bug Fixes in the v0.22.0 notes, with an interop note and a link to bolt11: reject duplicate payment hashes in invoices lightning/bolts#1357 as the motivation. Also added the same entry to the v0.20.5 and v0.21.4 notes since this will be backported (@yyforyongyu @saubyk).
• parse32Bytes comment (@ziggie1984 @Lrifton92): replaced with your suggested wording, noting the BOLT 11 divergence and why the end result for p is the same either way.
• Test vectors (@saubyk): kept a valid variant of the flipped vector that drops the second p but retains the wrong-length h and n fields, so the skip coverage survives. The duplicate vector stays in the decode table, now pinning ErrDuplicatePaymentHash via a new wantErr field instead of asserting any error.
• Property test (@saubyk): replaced TestDuplicatePaymentHashProperties with two table rows (identical, distinct) in TestParseTaggedFields, and dropped the rapid import.

Also added a note to the description calling out that the other single-occurrence fields still keep first-seen-wins behavior, with s as the natural follow-up candidate (@Lrifton92).

[ziggie1984]

can you remove this from 22 and fixup this commit maybe ?

[saubyk]

Only thing outstanding is removing the release note entry for 0.22, since this change is being targeted for 0.21. Other than that it's good to go.

[github-actions]

Created backport PR for v0.20.x-branch:

• [v0.20.x-branch] Backport #11190: zpay32: reject duplicate payment hash fields #11235 with remaining conflicts!

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-11190-to-v0.20.x-branch
git worktree add --checkout .worktree/backport-11190-to-v0.20.x-branch backport-11190-to-v0.20.x-branch
cd .worktree/backport-11190-to-v0.20.x-branch
git reset --hard HEAD^
git cherry-pick -x e2f2706367aa6e7a39f8ee14204b43087e27600f
git push --force-with-lease

[github-actions]

Created backport PR for v0.21.x-branch:

• [v0.21.x-branch] Backport #11190: zpay32: reject duplicate payment hash fields #11236 with remaining conflicts!

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-11190-to-v0.21.x-branch
git worktree add --checkout .worktree/backport-11190-to-v0.21.x-branch backport-11190-to-v0.21.x-branch
cd .worktree/backport-11190-to-v0.21.x-branch
git reset --hard HEAD^
git cherry-pick -x e2f2706367aa6e7a39f8ee14204b43087e27600f
git push --force-with-lease