bolt12: add Invoice codec and structural validators#10941
Conversation
github-actions
Bot
added
the
severity-critical
PR Severity: CRITICAL
Critical (1 file)
Medium (7 files)
Low (1 file)
Excluded from counting (5 files)
AnalysisThe critical classification is driven by changes to The bulk of this PR (~2,300 non-test lines) is new BOLT 12 invoice parsing/validation logic in the The To override, add a |
bitromortac
force-pushed
the
2604-bolt12-1c
branch
2 times, most recently
from
03cc142 to
2ab1ae2
Compare
github-actions
Bot
added
severity-high
🟠 PR Severity: HIGH
🟡 Medium (4 files)
🟢 Low (1 file)
AnalysisThe changed files are all in the Severity bump applied: 1,278 non-test lines changed exceeds the 500-line threshold, bumping the classification from MEDIUM → HIGH. Test files excluded from counting: To override, add a |
|
/gateway review |
|
✅ Review posted: #10941 (review) 6 finding(s); 6 inline, 0 in body. 🔁 Need a re-review after pushing changes? Reply with |
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds the BOLT 12 Invoice message, its blinded_payinfo/fallback_address subtype codecs, a truncated-uint32 TLV type, and the structural reader/writer/cross-message validators, and threads a runtime feature-bit catalogue into the offer and invoice_request readers. The codec work is careful and unusually well-tested: round-trip bijection, truncation rejection, unknown-field mirroring, and every reader/writer rejection path are all pinned by table tests, and the deferred concerns (signing, expiry clock, on-chain fallback rules) are documented inline.
The concerns worth resolving before merge are not in the new codec math but at its edges. First, the feature-catalogue refactor is a breaking signature change to three already-shipped validators, and this diff contains no non-test caller updates even though rpcserver.go was dropped from the PR since the last severity classification — confirm every caller compiles. Second, the same refactor silently removes the write-side unknown-even-feature-bit rejection that landed three weeks ago in #10832. Third, ValidateInvoiceRead authenticates nothing cryptographically yet; that is documented and intentional, but the identity binding it does perform has a gap when offer_issuer_id is absent, so callers must not mistake a passing invoice for an authentic one.
Findings: 🔴 0 Blocker · 🟠 3 Major · 🟡 3 Minor · 🔵 0 Nit
| return err | ||
| } | ||
|
|
||
| // check UTF-8 constraints and BIP 353 |
There was a problem hiding this comment.
I removed feature bit validation from writer validation as otherwise we'd need to pass in known features. I think it should be ok to rely on ourselves to set feature bits correctly.
| // - MUST reject the invoice if this leaves no usable paths. | ||
| var usablePaths int | ||
| for i := range bp.Infos { | ||
| fv := bp.Infos[i].Features |
There was a problem hiding this comment.
Have added the docstring and signature validation will come later, in addition to checking against the blinded path.
| // BlindedPayInfos holds a list of BlindedPayInfo entries for the | ||
| // invoice_blindedpay field. | ||
| type BlindedPayInfos struct { | ||
| Infos []BlindedPayInfo |
There was a problem hiding this comment.
This is a known issue as documented. I favored using the internal data structure for tlv mechanics over keeping the byte-wise representation. Added an error path to detect this explicitly.
| return err | ||
| } | ||
|
|
||
| // - MUST reject the invoice if signature is not a valid signature using |
There was a problem hiding this comment.
Yes, documented explicitly. Imo validation should only read and not modify in this case, so this has to be checked by the pathfinding system later.
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
Add the truncated uint32 (tu32) TLV type used by invoice_relative_expiry and the dynamic invoice subtypes BlindedPayInfo and FallbackAddress, along with their encode/decode helpers and round-trip tests. These primitives are the building blocks for the BOLT 12 Invoice message struct that follows. Isolating them keeps that codec commit focused on the message shape rather than its component records.
…alidators Inject known feature-bit catalogues into the read-side validators to enable correct must-understand capability checks, and remove write-side feature enforcement entirely. Whether a feature bit is "unknown" is a runtime property of the reading node, not of the wire format or pure codec.
Add the BOLT 12 Invoice message: a struct mirroring the invoice_request fields (types 0-91) plus the invoice-specific fields (types 160-176) and the signature (type 240), together with its pure-TLV Encode/DecodeInvoice codec and the UsableFallbackAddresses accessor that applies the spec's MUST-ignore filter. Additionally, add the NewInvoiceFromRequest constructor to build an Invoice from a corresponding request. This copies all non-signature fields from the request (including unknown signed-range TLVs via the decodedTLVs sidecar) and mirrors invreq_amount into invoice_amount.
Implement the structural validators for the BOLT 12 invoice, adding ValidateInvoiceWrite, ValidateInvoiceRead, ValidateInvoiceExpiry, and ValidateInvoiceAgainstRequest. The validators implement the spec writer and reader requirements in the order the spec lists them. The reader confirms the signature TLV is present but defers actual Schnorr verification until the merkle and signing primitives land, mirroring the ValidateInvoiceRequestRead precedent.
Add release notes for the BOLT 12 invoice codec.
bitromortac
force-pushed
the
2604-bolt12-1c
branch
from
2ab1ae2 to
0f49d8e
Compare
| return err | ||
| } | ||
|
|
||
| // check UTF-8 constraints and BIP 353 |
There was a problem hiding this comment.
I removed feature bit validation from writer validation as otherwise we'd need to pass in known features. I think it should be ok to rely on ourselves to set feature bits correctly.
| // - MUST reject the invoice if this leaves no usable paths. | ||
| var usablePaths int | ||
| for i := range bp.Infos { | ||
| fv := bp.Infos[i].Features |
There was a problem hiding this comment.
Have added the docstring and signature validation will come later, in addition to checking against the blinded path.
| // BlindedPayInfos holds a list of BlindedPayInfo entries for the | ||
| // invoice_blindedpay field. | ||
| type BlindedPayInfos struct { | ||
| Infos []BlindedPayInfo |
There was a problem hiding this comment.
This is a known issue as documented. I favored using the internal data structure for tlv mechanics over keeping the byte-wise representation. Added an error path to detect this explicitly.
| return err | ||
| } | ||
|
|
||
| // - MUST reject the invoice if signature is not a valid signature using |
There was a problem hiding this comment.
Yes, documented explicitly. Imo validation should only read and not modify in this case, so this has to be checked by the pathfinding system later.
| // WARNING: RawFeatureVector re-encodes to minimal length, so setting | ||
| // non-minimal feature bytes (trailing zeros) yields different wire | ||
| // bytes than were read and invalidates the invoice signature. |
There was a problem hiding this comment.
I believe it would be worth a change in the spec to only allow minimal encoded features in all lightning types.
This would be similar to what is being done with channel_announcement and node_announcement:
lightning/bolts#1341 (review)
erickcestari
left a comment
erickcestari
left a comment
There was a problem hiding this comment.
LGTM! Nice work!🏅
There are only some nits that are non-blocking from my side.
| RPC response rather than being emitted as an empty struct. | ||
|
|
||
| * [BOLT 12 invoice | ||
| codec](https://github.com/lightningnetwork/lnd/pull/10999): add the |
There was a problem hiding this comment.
nit:
| codec](https://github.com/lightningnetwork/lnd/pull/10999): add the | |
| codec](https://github.com/lightningnetwork/lnd/pull/10941): add the |
|
|
||
| // InvoiceNodeID is the public key of the recipient node, used to verify | ||
| // the signature. | ||
| InvoiceNodeID tlv.OptionalRecordT[tlv.TlvType176, [33]byte] |
There was a problem hiding this comment.
nit:
| InvoiceNodeID tlv.OptionalRecordT[tlv.TlvType176, [33]byte] | |
| InvoiceNodeID tlv.OptionalRecordT[tlv.TlvType176, *btcec.PublicKey] |
| // - For each invoice_blindedpay.payinfo: | ||
| // - MUST NOT use the corresponding invoice_paths.path if | ||
| // payinfo.features has any unknown even bits set. | ||
| // - MUST reject the invoice if this leaves no usable paths. | ||
| // NOTE: This loop only counts usable paths to ensure at least one | ||
| // exists. Callers MUST re-apply the same knownBlindedFeatures filter | ||
| // when selecting a path downstream to avoid using an unusable path, | ||
| // as the unfiltered list is returned. | ||
| var usablePaths int | ||
| for i := range bp.Infos { | ||
| fv := bp.Infos[i].Features | ||
| wrapped := lnwire.NewFeatureVector(&fv, knownBlindedFeatures) | ||
| if len(wrapped.UnknownRequiredFeatures()) == 0 { | ||
| usablePaths++ | ||
| } | ||
| } | ||
|
|
||
| if usablePaths == 0 { | ||
| return ErrNoUsablePaths | ||
| } |
There was a problem hiding this comment.
nit: We could have a similar method of UsableFallbackAddresses, but for the BlindedPaths fields.
// UsablePath pairs a blinded path with its payment parameters, as returned by
// UsablePaths after the BOLT 12 reader's feature filter has been applied.
type UsablePath struct {
// Path is the blinded path to the recipient.
Path lnwire.BlindedPath
// PayInfo is the blinded_payinfo for Path.
PayInfo BlindedPayInfo
}
// UsablePaths returns the invoice_paths entries a payer may use, each paired
// with its blinded_payinfo, after applying the BOLT 12 reader rule that a path
// MUST NOT be used when its payinfo.features has unknown required (even) bits
// set. knownBlindedFeatures names the feature bits the reader understands.
//
// The result is empty when invoice_paths or invoice_blindedpay is absent, or
// when the two lists differ in length; ValidateInvoiceRead rejects those cases
// separately, so a caller that validates first can treat an empty result as
// "no usable paths".
func (inv *Invoice) UsablePaths(
knownBlindedFeatures map[lnwire.FeatureBit]string) []UsablePath {
paths := inv.InvoicePaths.ValOpt().UnwrapOr(lnwire.BlindedPaths{})
bp := inv.InvoiceBlindedPay.ValOpt().UnwrapOr(BlindedPayInfos{})
// Entries pair by index; a length mismatch is rejected upstream by
// ValidateInvoiceRead, so guard here to stay in bounds.
if len(paths.Paths) != len(bp.Infos) {
return nil
}
var usable []UsablePath
for i := range bp.Infos {
// MUST NOT use the path if payinfo.features has any unknown even
// bits set.
fv := bp.Infos[i].Features
wrapped := lnwire.NewFeatureVector(&fv, knownBlindedFeatures)
if len(wrapped.UnknownRequiredFeatures()) > 0 {
continue
}
usable = append(usable, UsablePath{
Path: paths.Paths[i],
PayInfo: bp.Infos[i],
})
}
return usable
}Then:
| // - For each invoice_blindedpay.payinfo: | |
| // - MUST NOT use the corresponding invoice_paths.path if | |
| // payinfo.features has any unknown even bits set. | |
| // - MUST reject the invoice if this leaves no usable paths. | |
| // NOTE: This loop only counts usable paths to ensure at least one | |
| // exists. Callers MUST re-apply the same knownBlindedFeatures filter | |
| // when selecting a path downstream to avoid using an unusable path, | |
| // as the unfiltered list is returned. | |
| var usablePaths int | |
| for i := range bp.Infos { | |
| fv := bp.Infos[i].Features | |
| wrapped := lnwire.NewFeatureVector(&fv, knownBlindedFeatures) | |
| if len(wrapped.UnknownRequiredFeatures()) == 0 { | |
| usablePaths++ | |
| } | |
| } | |
| if usablePaths == 0 { | |
| return ErrNoUsablePaths | |
| } | |
| // - For each invoice_blindedpay.payinfo: | |
| // - MUST NOT use the corresponding invoice_paths.path if | |
| // payinfo.features has any unknown even bits set. | |
| // - MUST reject the invoice if this leaves no usable paths. | |
| // UsablePaths applies that filter; a caller selecting a path downstream | |
| // should use it rather than the unfiltered invoice_paths. | |
| if len(inv.UsablePaths(knownBlindedFeatures)) == 0 { | |
| return ErrNoUsablePaths | |
| } |
|
/gateway dismiss F1 per #10941 (comment) |
|
🚫 Dismissed F1 (major) by @saubyk — per #10941 (comment) Open findings on this PR: 🟠 F2 (major) · 🟠 F3 (major) · 🟡 F4 (minor) · 🟡 F5 (minor) · 🟡 F6 (minor) |
|
/gateway dismiss F6 per #10941 (comment) |
|
🚫 Dismissed F6 (minor) by @saubyk — per #10941 (comment) Open findings on this PR: 🟠 F2 (major) · 🟠 F3 (major) · 🟡 F4 (minor) · 🟡 F5 (minor) |
|
/gateway dismiss F3 per #10941 (comment) |
|
🚫 Dismissed F3 (major) by @saubyk — per #10941 (comment) Open findings on this PR: 🟠 F2 (major) · 🟡 F4 (minor) · 🟡 F5 (minor) |
|
/gateway dismiss F4 per #10941 (comment) |
|
🚫 Dismissed F4 (minor) by @saubyk — per #10941 (comment) Open findings on this PR: 🟠 F2 (major) · 🟡 F5 (minor) |
|
/gateway re-review |
|
✅ Re-review posted: #10941 (review) Prior findings: 1 addressed, 1 still unresolved, 0 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
There was a problem hiding this comment.
The maintainer dismissed F1, F3, F4, and F6, and this revision resolved F5 by adding the entry-count caps to both list decoders. F2 remains open: the breaking arity change to ValidateOfferRead/ValidateInvoiceRequestRead/checkFeatures still carries no in-diff caller update, and independent change-impact analysis reconfirms the build-break risk cannot be cleared from the reach index alone. Three new, low-severity concerns surfaced in the invoice validators — an expiry boundary off-by-one, an unbounded invoice_amount for fixed-amount offers, and a feature-map parameter-transposition hazard.
Findings: 🔴 0 Blocker · 🟠 1 Major · 🟡 3 Minor · 🔵 0 Nit
Status of prior findings
- F5 addressed: Fixed in
bolt12/subtypes.go—decodeBlindedPayInfosanddecodeFallbackAddrsnow cap entries atmaxBlindedPayInfos/maxFallbackAddrs(32) and returnErrTooManyBlindedPayInfos/ErrTooManyFallbackAddrs, matching the siblingdecodeChainsRecordbound, with "exceeds cap" test coverage added for both.
|
|
||
| return nil | ||
| }); err != nil { | ||
| return err |
There was a problem hiding this comment.
🟡 Minor F7: Invoice expiry rejects one second early vs BOLT 12
ValidateInvoiceExpiry treats the invoice as expired when carry == 0 && uint64(now.Unix()) >= expiry, where expiry = invoice_created_at + invoice_relative_expiry. BOLT 12's reader rule rejects only when the current time is greater than created_at + seconds_from_creation, so >= expires the invoice at the exact boundary second — one second early — rejecting a payment the spec still considers valid. This is also internally inconsistent with ValidateOfferRead, which uses strict > (uint64(now.Unix()) > expiry) for offer_absolute_expiry. Change the comparator to >.
| // Spec MUST: if invreq_amount (type 82) is present, invoice_amount | ||
| // (type 170) must equal it. The byte-mirror loop cannot relate fields | ||
| // with differing type numbers. | ||
| return fn.MapOptionZ( |
There was a problem hiding this comment.
🟡 Minor F8: invoice_amount unbounded to offer_amount when invreq_amount absent
ValidateInvoiceAgainstRequest's only amount cross-check is req.InvreqAmount present → inv.InvoiceAmount must equal it. When the payer relied on a fixed offer_amount and set no invreq_amount, invoice_amount is left unconstrained: the byte-mirror excludes it (type 170 is in the invoice-specific 160-176 range, outside invreqAllowedRange), and the WhenSome cross-check never fires. A payee can then return an invoice charging more than the offer advertised and this validator accepts it. BOLT 12 makes this a SHOULD ("confirm invoice_amount.msat is within the authorized range"), and the currency case genuinely needs a caller-supplied exchange rate — but the native fixed-amount case is offer_amount × invreq_quantity, computable here and already done for the request side in checkInvreqAmountMeetsOffer. Either add that comparison or state in this function's contract that the offer-amount bound is delegated.
| } | ||
|
|
||
| // - if invoice_features contains unknown odd bits that are non-zero: | ||
| // - MUST ignore the bit. |
There was a problem hiding this comment.
🟡 Minor F9: Adjacent identical-type feature maps invite a silent swap
ValidateInvoiceRead(inv, activeChain, knownFeatures, knownBlindedFeatures map[lnwire.FeatureBit]string) places two parameters of identical type adjacent in the signature. ValidateInvoiceRead(inv, chain, knownBlindedFeatures, knownFeatures) — transposed — compiles cleanly and validates invoice_features against the blinded-path catalogue and vice versa, either spuriously rejecting valid invoices or accepting required bits that should have been rejected. A single options struct with named fields makes the two sets impossible to swap positionally.
| // caller wiring this into a handler MUST verify the signature itself. | ||
| func ValidateInvoiceRequestRead(ir *InvoiceRequest, | ||
| activeChain [32]byte) error { | ||
| activeChain [32]byte, |
There was a problem hiding this comment.
🟠 Major F2 · unresolved
Still unresolved on the current head. ValidateOfferRead, ValidateInvoiceRequestRead, and checkFeatures retain the added required knownFeatures map[lnwire.FeatureBit]string parameter — a breaking signature change to code that landed weeks ago (#10789/#10832, per blame). The current diff contains no non-test caller update, and the callers[] "low" reach bucket does not clear this: that index is built from the default branch and cannot see this PR series' own callers. Two signals still point at real blast radius: rpcserver.go (+30/-19) rode an earlier push of this PR and was then dropped from the file set, and no out-of-package call site appears in the diff. The author states callers were updated, but that is not evidenced here. Before merge, confirm every out-of-package caller compiles against the new arity — land the caller updates in this PR or cite the merged PR that carries them.
3 participants
Based on #10832, part of #10736.
Adds the BOLT 12
Invoicemessage struct withPureTLVMessage-basedEncode/Decode, alongside the structuralValidateInvoiceRead/Writeand cross-messageValidateInvoiceAgainstRequestvalidators.Additionally, this PR injects known feature-bit catalogues into the
OfferandInvoiceRequestread-side validators to enable must-understand capability checks at runtime, because features are defined outside of bolt12's boundaries.