bolt12: add InvoiceRequest codec and structural validators#10832
bolt12: add InvoiceRequest codec and structural validators#10832bitromortac wants to merge 13 commits into
InvoiceRequest codec and structural validators#10832Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the foundational codec and validation logic for BOLT 12 InvoiceRequest messages. It establishes a new bolt12 package dedicated to the encoding, decoding, and structural validation of offers and invoice requests. Furthermore, it refactors the existing BlindedPath implementation to support the introduction node variants required by the specification, ensuring that both Pubkey and Sciddir forms are correctly handled throughout the codebase, including RPC surfaces. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the core BOLT 12 Offer and Invoice Request functionality, introducing the bolt12 package for TLV encoding, decoding, and extensive validation. It also refactors lnwire to include a shared BlindedPath implementation supporting both pubkey and sciddir introduction nodes, while updating the RPC and onion messaging layers to utilize these new types. Review feedback focused on improving documentation and comment maintainability, specifically recommending the removal of fragile line number references, fixing a broken documentation block in the validation logic, and ensuring the release notes accurately reflect the inclusion of the invoice_request codec.
github-actions
Bot
added
the
severity-critical
|
🔴 PR Severity: CRITICAL
🔴 Critical (8 files)
🟠 High (1 file)
🟡 Medium (12 files)
🟢 Low (1 file)
⚪ Excluded from counting (10 files)
Analysis This PR introduces BOLT 12 invoice request codec and structural validators, touching the The new To override, add a |
Add UnsignedRangeFunc and the SerialiseFieldsToSignFn / ExtraSignedFieldsFromTypeMapFn variants so callers with non-BOLT 7 v2 signed ranges (e.g. BOLT 12, which reserves only 240-1000) can plug in their own predicate. The existing SerialiseFieldsToSign and ExtraSignedFieldsFromTypeMap entry points keep their behaviour by delegating to the Fn variants with InUnsignedRange.
This gives us easier optional tlv field handling, which we will use for the following message definitions.
Introduce the canonical lnwire.BlindedPath / BlindedPaths codec with a sealed IntroductionNode sum-type covering both the BOLT 4 pubkey and sciddir variants. The codec gates every variable-length subfield against an io.LimitedReader. It fails closed on the encoder side so invalid input never hits the wire. This commit is a pure addition: no existing caller changes. Subsequent commits migrate OnionMessagePayload and the bolt12 message structs to consume the new codec.
Switch OnionMessagePayload.ReplyPath from *sphinx.BlindedPath to *lnwire.BlindedPath. The reply-path TLV is now produced and consumed by (*lnwire.BlindedPath).Record(), which honours the BOLT 4 sciddir_or_pubkey introduction-node form. The legacy decoder gated on a 67-byte minimum length and silently rejected reply paths whose introduction node used the 9-byte sciddir variant. The legacy replyPathRecord / replyPathSize / encodeReplyPath / decodeReplyPath / blindedHopSize / encodeBlindedHop / decodeBlindedHop helpers and the unused ErrNoHops sentinel are deleted. Consumers update mechanically: routing/route's OnionMessageBlindedPathToSphinxPath replyPath parameter, the onionmessage.OnionMessageUpdate field, the rpcserver onion-message subscription bridge, and the lnwire test utilities now use the lnwire type directly. The new TestOnionMessagePayloadRoundTrip "sciddir intro reply path" subtest pins the BOLT 4 spec fix.
Introduce the ChainsRecord subtype used by the offer_chains and invoice_chains TLV fields. Decoding caps the count at maxOfferChains to bound allocation.
The Offer struct models a long-lived, reusable BOLT 12 payment template. It defines TLV fields as optional records and exposes Encode/DecodeOffer for round-trip serialization. The struct implements lnwire.PureTLVMessage; AllRecords filters the decoded TypeMap through bolt12InUnsignedRange to derive any signed-range extras the encoder must re-emit, keeping offer_id and the Merkle root stable across encoders that understand a wider set of even/odd extensions.
ValidateOfferRead and ValidateOfferWrite enforce the codec-side portion of the BOLT 12 offer reader and writer requirements. Reader rules cover TLV range, even-feature-bit rejection, chain mismatch, dependency rules between offer_amount/description/currency, missing issuer identity, zero-hop blinded paths, and offer expiry. Writer rules mirror the same dependency and identity guards plus a defense-in-depth empty- offer_chains rejection. offer_currency is validated against the ISO 4217 registry via golang.org/x/text/currency (now a direct dependency); offer_issuer_id is verified to be an on-curve SEC1 compressed point on both read and write paths. Encode invokes Validate so invalid bytes never reach the wire.
Document that the introduction_node field in an OnionMessageUpdate's reply_path is passed through verbatim from the wire, potentially carrying either a 33-byte pubkey or a 9-byte sciddir form. Subscribers wishing to reply must resolve sciddir forms against their local channel graph. The SubscribeOnionMessages bridge is refactored to use a new marshallBlindedPath helper, ensuring a nil reply path remains nil in the RPC response rather than being emitted as an empty struct.
The InvoiceRequest is the BOLT 12 message that links a payer to an offer: it mirrors the offer's fields so the issuer can stay stateless, and adds the payer-specific fields and Schnorr signature that prove the request. It implements lnwire.PureTLVMessage so it round-trips through the shared TLV codec.
This ensures we copy all fields when replying to an offer.
ValidateInvoiceRequestRead and ValidateInvoiceRequestWrite enforce the structural BOLT 12 requirements an invoice request can be checked against on its own. The reader validates incoming requests. The writer catches out-of-range types in decoded-then-mutated requests before they leave the local boundary. Type 240 carries the signature and sits outside the allowed range by spec design. Both validators skip it during the range scan. Two reader MUSTs are deferred. Schnorr signature verification against the merkle root keyed by invreq_payer_id lands with the Invoice message, where the merkle and signing primitives are shared. Offer cross-validation requires an Offer reference the structural validator does not carry, and lands in the bolt12handler layer where both the request and the stored Offer are in scope.
Add a BOLT 12 release note for the invoice_request codec, completing the offer/invoice_request entries for the bolt12 package in 0.22.0.
bitromortac
force-pushed
the
2604-bolt12-1b
branch
from
38e5fc9 to
d3a1cc6
Compare
erickcestari
left a comment
erickcestari
left a comment
There was a problem hiding this comment.
Some tests are missing to achieve greater coverage, but it's looking good!
go test ./bolt12/ -coverprofile=/tmp/cover.out && \
go tool cover -func=/tmp/cover.out
ok github.com/lightningnetwork/lnd/bolt12 (cached) coverage: 92.1% of statements
github.com/lightningnetwork/lnd/bolt12/decode.go:13: decodeStream 75.0%
github.com/lightningnetwork/lnd/bolt12/invoice_request.go:100: AllRecords 100.0%
github.com/lightningnetwork/lnd/bolt12/invoice_request.go:109: allRecordProducers 100.0%
github.com/lightningnetwork/lnd/bolt12/invoice_request.go:139: Encode 66.7%
github.com/lightningnetwork/lnd/bolt12/invoice_request.go:155: DecodeInvoiceRequest 97.9%
github.com/lightningnetwork/lnd/bolt12/invoice_request.go:234: NewInvoiceRequestFromOffer 100.0%
github.com/lightningnetwork/lnd/bolt12/offer.go:72: AllRecords 100.0%
github.com/lightningnetwork/lnd/bolt12/offer.go:80: allRecordProducers 100.0%
github.com/lightningnetwork/lnd/bolt12/offer.go:99: Encode 66.7%
github.com/lightningnetwork/lnd/bolt12/offer.go:117: decodeOffer 96.4%
github.com/lightningnetwork/lnd/bolt12/pure_tlv.go:14: bolt12InUnsignedRange 100.0%
github.com/lightningnetwork/lnd/bolt12/pure_tlv.go:23: allRecordsFromTypeMap 100.0%
github.com/lightningnetwork/lnd/bolt12/pure_tlv.go:44: sortedTypes 100.0%
github.com/lightningnetwork/lnd/bolt12/subtypes.go:33: Record 50.0%
github.com/lightningnetwork/lnd/bolt12/subtypes.go:46: encodeChainsRecord 71.4%
github.com/lightningnetwork/lnd/bolt12/subtypes.go:62: decodeChainsRecord 84.6%
github.com/lightningnetwork/lnd/bolt12/tlv_types.go:14: Record 100.0%
github.com/lightningnetwork/lnd/bolt12/validate.go:151: isKnownInvreqTLVType 60.0%
github.com/lightningnetwork/lnd/bolt12/validate.go:170: ValidateInvoiceRequestWrite 86.0%
github.com/lightningnetwork/lnd/bolt12/validate.go:347: invreqAllowedRange 100.0%
github.com/lightningnetwork/lnd/bolt12/validate.go:359: checkInvreqAmountMeetsOffer 100.0%
github.com/lightningnetwork/lnd/bolt12/validate.go:403: getInvreqChain 100.0%
github.com/lightningnetwork/lnd/bolt12/validate.go:424: ValidateInvoiceRequestRead 75.8%
github.com/lightningnetwork/lnd/bolt12/validate.go:565: getInvoiceRequestOfferChains 100.0%
github.com/lightningnetwork/lnd/bolt12/validate.go:580: checkInvreqQuantity 100.0%
github.com/lightningnetwork/lnd/bolt12/validate.go:629: checkBip353Name 100.0%
github.com/lightningnetwork/lnd/bolt12/validate.go:682: checkBip353Alphabet 100.0%
github.com/lightningnetwork/lnd/bolt12/validate.go:700: offerAllowedRange 100.0%
github.com/lightningnetwork/lnd/bolt12/validate.go:707: isKnownOfferTLVType 66.7%
github.com/lightningnetwork/lnd/bolt12/validate.go:722: ValidateOfferRead 97.7%
github.com/lightningnetwork/lnd/bolt12/validate.go:833: getOfferChains 100.0%
github.com/lightningnetwork/lnd/bolt12/validate.go:848: ValidateOfferWrite 93.3%
github.com/lightningnetwork/lnd/bolt12/validate.go:933: checkISO4217 75.0%
github.com/lightningnetwork/lnd/bolt12/validate.go:944: checkFeatures 100.0%
github.com/lightningnetwork/lnd/bolt12/validate.go:970: checkBlindedPaths 85.7%
github.com/lightningnetwork/lnd/bolt12/validate.go:996: checkPubKeyNotNil 100.0%
github.com/lightningnetwork/lnd/bolt12/validate.go:1009: checkAmountPositive 100.0%
github.com/lightningnetwork/lnd/bolt12/validate.go:1022: checkUTF8 100.0%
total: (statements) 92.1%
| // reply_path is the blinded path that should be used when replying to a | ||
| // received message. | ||
| /* | ||
| reply_path is the blinded path that should be used when replying to a |
There was a problem hiding this comment.
Since we'll change the reply_path to might be a public key or a sciddir, we should also update the reply_path description in .proto.
| if !ir.InvreqPayerID.IsSome() { | ||
| return ErrMissingPayerID | ||
| } |
There was a problem hiding this comment.
We also should check if the pubkey inside the TLV option at InvreqPayerID and OfferIssuerID is nil which would crash the encoder. Similar to what we have done to OfferIssuerID in the offer encoding.
Maybe a helper function to do it?
| if t == signatureTLVType { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Shouldn't we skip the whole unsigned TLV range?
| if t == signatureTLVType { | |
| continue | |
| } | |
| if bolt12InUnsignedRange(t) { | |
| continue | |
| } |
There was a problem hiding this comment.
I don't think it is a good idea to just accept all fields we don't know yet only because the Spec excludes them from signing. For example we cannot skip even fields we do not understand, moreover also allowing unauthenticated odd fields is in my opinion a risk, because there can be bad data in it.
There was a problem hiding this comment.
Good point! I agree with ziggie, that unconditionally skipping the range would be too broad.
Signing and validation should be treated separately. BOLT 12 excludes the entire 240..1000 signature range from the Merkle root. During validation, however, the normal BOLT TLV rule still applies within that range: accept known signature fields, reject unknown even types, and ignore unknown odd types as optional extensions.
| if t == signatureTLVType { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Shouldn't we skip the whole unsigned TLV range?
| if t == signatureTLVType { | |
| continue | |
| } | |
| if bolt12InUnsignedRange(t) { | |
| continue | |
| } |
vctt94
left a comment
vctt94
left a comment
There was a problem hiding this comment.
Thanks for the update!
I agree with Erick’s requested changes around:
-
checking present-but-nil pubkeys inside
InvreqPayerID/OfferIssuerID, sinceIsSome()alone can still allow an encoder panic. I also reproduced theInvreqPayerIDcase locally with a smallValidateInvoiceRequestWritetable row usingInvreqPayerID = Some(nil), which currently demonstrates the issue: ad5cb9d -
skipping the whole BOLT 12 unsigned/signature TLV range instead of only type
240during the invoice_request range scans.
A couple of extra notes from my pass:
-
The
SubscribeOnionMessagesreply_pathupdate looks good to me. It addresses the prior #10789 discussion by documenting thatintroduction_nodeis passed through verbatim and may be either a 33-byte pubkey or a 9-byte sciddir, and by keeping a nil reply path nil in the RPC response. -
The #10789 onion-message issues around final-hop namespace multiplicity and unknown even final-hop TLVs still look intentionally deferred, matching the PR description, so I did not treat them as blockers for this PR.
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
Great work again !!!
I don't have much here just some clarifying questions.
Thank you for working on this.
| OfferIssuerID tlv.OptionalRecordT[tlv.TlvType22, *btcec.PublicKey] | ||
|
|
||
| // InvreqMetadata is a blob of metadata provided by the payer. | ||
| InvreqMetadata tlv.OptionalRecordT[tlv.TlvType0, tlv.Blob] |
There was a problem hiding this comment.
Nit: maybe add a comment why this MetaData has the Type 0 value ? (Because it kinda identifies the InvoiceRequest)
|
|
||
| // TestInvoiceRequestRoundTrip pins encode→decode→re-encode for an | ||
| // InvoiceRequest with a representative subset of optional fields. | ||
| func TestInvoiceRequestRoundTrip(t *testing.T) { |
There was a problem hiding this comment.
Could we add explicit all-fields codec coverage for Offer and InvoiceRequest?
These types are effectively BOLT 12 TLV schemas, and each field has to stay wired through several places: the struct definition, allRecordProducers, decode record setup,
SetOptFromMap, and for mirrored offer fields, NewInvoiceRequestFromOffer.
A subset round-trip test is useful, but it won’t necessarily catch a field that was added to the struct and then forgotten in allRecordProducers or decode wiring. Since these
canonical TLV bytes feed BOLT 12 IDs/signing semantics, silently dropping a field on encode/decode would be hard to spot and potentially consensus/interoperability-sensitive.
| // payer metadata; the caller should subsequently sign the request. | ||
| // | ||
| // Per "MUST copy all fields from the offer (including unknown fields)", the | ||
| // offer's unknown TLVs are carried via the decodedTLVs sidecar so they are |
There was a problem hiding this comment.
nit: unknown uneven TLVs or ? Because odd unknown are not allowed ?
| encoded, err := offer.Encode() | ||
| require.NoError(t, err) | ||
|
|
||
| const unknownType = 33 |
There was a problem hiding this comment.
should we add a test which check if we add the 34 tlv field, then we should fail correct ?
| return true | ||
| } | ||
| switch typ { | ||
| case 0, 80, 82, 84, 86, 88, 89, 90, 91, 240: |
There was a problem hiding this comment.
Nit: I would prefer:
case invreqMetadataType,
invreqChainType,
invreqAmountType,
invreqFeaturesType,
invreqQuantityType,
invreqPayerIDType,
invreqPayerNoteType,
invreqPathsType,
invreqBip353NameType,
signatureTLVType:
return true
| if t == signatureTLVType { | ||
| continue | ||
| } |
There was a problem hiding this comment.
I don't think it is a good idea to just accept all fields we don't know yet only because the Spec excludes them from signing. For example we cannot skip even fields we do not understand, moreover also allowing unauthenticated odd fields is in my opinion a risk, because there can be bad data in it.
| if t == signatureTLVType { | ||
| continue | ||
| } |
| // that check is deferred until the merkle/signing primitives land with the | ||
| // Invoice message (see the TODO at the end of this function). Until then, a | ||
| // caller wiring this into a handler MUST verify the signature itself. | ||
| func ValidateInvoiceRequestRead(ir *InvoiceRequest, |
There was a problem hiding this comment.
should we return an error if we have not signature at all, I know the signature validation will follow but if there is no signature present we should fail ?
| // signature is intentionally omitted from this list: the spec's | ||
| // unsigned offerless variant conflicts with the unconditional | ||
| // reader signature check, and other implementations require a | ||
| // signature on every invoice_request. We always sign, so a | ||
| // present signature here is expected. |
There was a problem hiding this comment.
is this worth reiterating on the spec level. So the read side requires a signature, whereas the InvoiceRequest for a non-offer does not ?
Abdulkbk
left a comment
Abdulkbk
left a comment
There was a problem hiding this comment.
Solid PR, had a few pieces of feedback.
| // Pre-sign Encode (used to compute the Merkle root) is permitted to run | ||
| // without a signature; the bech32 string-codec layer is where the | ||
| // signature becomes mandatory. | ||
| ErrMissingSignature = errors.New("missing signature") |
There was a problem hiding this comment.
seems this isn't used atm probably should introduce it when we add sig verification?
| if ir.OfferMetadata.IsSome() || ir.OfferChains.IsSome() || | ||
| ir.OfferAmount.IsSome() || ir.OfferCurrency.IsSome() || | ||
| ir.OfferFeatures.IsSome() || | ||
| ir.OfferQuantityMax.IsSome() || |
There was a problem hiding this comment.
Should we also check for hanging InvreqQuantity since it's dependent on OfferQuantityMax
| // checkBip353Name validates the wire layout and alphabet of | ||
| // invreq_bip_353_name. Both name and domain MUST contain only DNS-safe | ||
| // characters per the BOLT 12 reader and writer requirements. | ||
| func checkBip353Name(opt tlv.OptionalRecordT[tlv.TlvType91, tlv.Blob]) error { |
There was a problem hiding this comment.
[]byte{0x00, 0x00} passes (returning nil), though it makes no sense to have an empty name and domain; the spec didn't explicitly prohibit this.
|
|
||
| return fmt.Errorf( | ||
| "%w: offer fields present on non-offer "+ | ||
| "response", ErrOutOfRangeType, |
There was a problem hiding this comment.
Should we create a new sentinel like ErrOfferFieldOnSpontaneous because ErrOutOfRangeType is documented as "is returned when a TLV type falls outside the allowed offer ranges"? Same in the ValidateInvoiceRequestRead spontaneous branch.
| func ValidateInvoiceRequestWrite(ir *InvoiceRequest) error { | ||
| // - if it is responding to an offer: | ||
| isResponse := ir.OfferIssuerID.IsSome() || ir.OfferPaths.IsSome() | ||
| //nolint:nestif |
There was a problem hiding this comment.
should we extract validateInvreqResponseWrite and validateInvreqSpontaneousWrite out of ValidateInvoiceRequestWrite? It will enable us to remove the lint exemption and let each branch be tested in isolation
6 participants
Based on #10789 (last five commits), part of #10736.
Adds the BOLT 12
InvoiceRequestmessage struct withPureTLVMessage-basedEncode/Decodeand the structuralValidateInvoiceRequestRead/Writevalidators. Signature verification and offer cross-validation are deferred to theInvoiceandbolt12handlerlayers respectively.I added a commit to address the discussion around the
SubscribeOnionMessagesrpc. There were other requests concerning thelnwireonion message implementation, which I haven't addressed yet. I'll open an extra PR for that.