bolt12: add InvoiceRequest codec and structural validators#10832
Conversation
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 |
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.
There was a problem hiding this comment.
I thought about that too, but didn't do it intentionally, as we usually set the introduction_node to a pubkey and it would add more confusion than utility imo. I think keeping the doc change on the OnionMessageUpdate` is enough to document the special behavior
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.
| // 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 ?
There was a problem hiding this comment.
right, I checked other implementations and I think eclair and cln require sigs as well, so probably the spec should be updated (have taken note of that)
There was a problem hiding this comment.
please create an issue on the BOLT repo so that it is not forgotten
Abdulkbk
left a comment
Abdulkbk
left a comment
There was a problem hiding this comment.
Solid PR, had a few pieces of feedback.
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.
bitromortac
force-pushed
the
2604-bolt12-1b
branch
from
d3a1cc6 to
20125ec
Compare
github-actions
Bot
added
severity-critical
|
Thanks all for your reviews! Rebased this on master (previous commit was d3a1cc6). Edit: will fix the linter problems with the next round. |
bitromortac
force-pushed
the
2604-bolt12-1b
branch
from
20125ec to
6e450ba
Compare
erickcestari
left a comment
erickcestari
left a comment
There was a problem hiding this comment.
LGTM! 📨
Nice work !!!
vctt94
left a comment
vctt94
left a comment
There was a problem hiding this comment.
LGTM!
Great job on this PR!
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.
|
/gateway review |
|
✅ Review posted: #10832 (review) 3 finding(s); 3 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_request message struct, PureTLVMessage-based Encode/DecodeInvoiceRequest, and the structural ValidateInvoiceRequestRead/Write validators, deliberately deferring Schnorr-signature verification and offer cross-validation to later layers. The scoping is clear and the spec-to-code mapping is faithful — the read/write asymmetry on spontaneous-request fields, the unsigned-range handling, and the invreq_chain default-to-mainnet rule all match the BOLT 12 text and are well documented in comments. Test coverage of the new validators is thorough (sentinel table tests, amount-overflow, chain, BIP-353 layout).
The remaining concerns are minor API/behavior sharp edges rather than correctness defects: a constructor that can silently build an un-encodable request, a read-path validator that doesn't mirror the write-path nil-pubkey guard, and an observable RPC output change in SubscribeOnionMessages worth a release note.
Findings: 🔴 0 Blocker · 🟠 0 Major · 🟡 3 Minor · 🔵 0 Nit
|
|
||
| if len(metadata) > 0 { | ||
| ir.InvreqMetadata = tlv.SomeRecordT( | ||
| tlv.RecordT[tlv.TlvType0, tlv.Blob]{ |
There was a problem hiding this comment.
🟡 Minor F1: Constructor can build a request that fails its own Encode()
NewInvoiceRequestFromOffer sets InvreqMetadata only when len(metadata) > 0 and InvreqPayerID only when payerID != nil, but ValidateInvoiceRequestWrite (invoked by Encode) requires both for an offer-response (ErrMissingMetadata, ErrMissingPayerID). Passing a nil payerID or empty metadata therefore returns a struct that constructs cleanly but can never encode, with the failure surfacing far from the construction site. The docstring documents neither precondition. Consider returning (*InvoiceRequest, error) and rejecting empty/nil inputs at the boundary, or documenting that both are mandatory — so "forgot to supply metadata/payer key" fails at the call site, not at a later Encode().
There was a problem hiding this comment.
let's do that, I think it makes sense to catch this in the contructor.
There was a problem hiding this comment.
Agree, thanks also for owning the review bot's comments 🙏
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
| // 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.
please create an issue on the BOLT repo so that it is not forgotten
|
|
||
| if len(metadata) > 0 { | ||
| ir.InvreqMetadata = tlv.SomeRecordT( | ||
| tlv.RecordT[tlv.TlvType0, tlv.Blob]{ |
There was a problem hiding this comment.
let's do that, I think it makes sense to catch this in the contructor.
|
/gateway dismiss F2 per #10832 (comment) |
|
/gateway dismiss F3 per #10832 (comment) |
|
🚫 Dismissed F2 (minor) by @saubyk — per #10832 (comment) Open findings on this PR: 🟡 F1 (minor) · 🟡 F3 (minor) |
|
🚫 Dismissed F3 (minor) by @saubyk — per #10832 (comment) Open findings on this PR: 🟡 F1 (minor) |
This ensures we copy all fields when replying to an offer.
The offer writer rejected a present-but-nil offer_issuer_id with an ad-hoc error. Introduce a typed ErrNilPublicKey sentinel and use it here so the rejection is recoverable by callers and reusable by the invoice_request writer, which guards the same hazard for its own pubkey fields.
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
6e450ba to
e9a2cb4
Compare
|
Thanks for all the reviews! I've added also some checks for the nil pubkeys and addressed feedback.
The intention was just to save the additional CI run |
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.