bolt12+lnwire: add codec foundation with Offer message#10789
bolt12+lnwire: add codec foundation with Offer message#10789bitromortac wants to merge 8 commits into
Conversation
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (2 files)
🟡 Medium (7 files)
AnalysisThis PR introduces a new The new Additionally, with ~1,335 non-test lines changed (exceeding the 500-line bump threshold), a severity bump is triggered — though the PR is already at the maximum level of CRITICAL. To override, add a |
🔴 PR Severity: CRITICAL
🔴 Critical (2 files)
🟡 Medium (7 files)
AnalysisThis PR introduces a new The new Additionally, with ~1,335 non-test lines changed (exceeding the 500-line bump threshold), a severity bump is triggered — though the PR is already at the maximum level of CRITICAL. To override, add a |
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 establishes the foundational codec infrastructure for BOLT 12 support in LND. It introduces a dedicated bolt12 package to manage the serialization and validation of offer messages, while extending lnwire with necessary utilities to support the required TLV-based message structures. 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 codec, including encoding, decoding, and validation logic. It introduces the Offer message type, support for blinded paths, and chain-specific validation. Additionally, it extends the lnwire package with generic TLV helpers and predicate-driven serialization to accommodate BOLT 12's specific signed-range requirements. Feedback includes a critical fix for a dangling pointer in the AddOpt helper and a recommendation to use unsigned comparisons for expiry timestamps to avoid potential overflow issues.
bitromortac
force-pushed
the
2604-bolt12-1a
branch
from
98bd70d to
0886f55
Compare
bitromortac
force-pushed
the
2604-bolt12-1a
branch
from
0886f55 to
6999551
Compare
Abdulkbk
left a comment
Abdulkbk
left a comment
There was a problem hiding this comment.
Had a first pass and left one question. Would take a deeper look.
erickcestari
left a comment
erickcestari
left a comment
There was a problem hiding this comment.
Still going through the diff. A few things worth tightening already flagged inline. Requesting changes for now so we can iterate on those while I finish the rest. Thanks!
| func (p PubkeyIntro) validate() error { | ||
| switch p.Pubkey[0] { | ||
| case 0x02, 0x03: | ||
| return nil | ||
| } | ||
|
|
||
| return fmt.Errorf("%w: 0x%02x", ErrInvalidIntroNode, p.Pubkey[0]) | ||
| } |
There was a problem hiding this comment.
We could use the btcec.ParsePubKey to validate if the point is really in the curve.
There was a problem hiding this comment.
Yes, good point. I'm using *btcec.PublicKey for pubkey-based fields now. That way we get the validation when during decode. Currently this checks only for non-nilness, will probably also add the on-curve check here in the next round.
There was a problem hiding this comment.
Have added the checks that keys are on curve.
| EncryptedData: hop.CipherText, | ||
| } | ||
| bp.BlindedHops = append(bp.BlindedHops, rpcHop) | ||
| bp.IntroductionNode = oMsg.ReplyPath.IntroductionNode.Bytes() |
There was a problem hiding this comment.
bp.IntroductionNode used to be a fixed 33-byte pubkey; now it's 33 bytes for PubkeyIntro or 9 bytes for SciddirIntro with no discriminator, so any client doing btcec.ParsePubKey(bp.IntroductionNode) (as the proto comment promises) breaks the first time a sciddir reply path arrives.
There was a problem hiding this comment.
That's a good point. I'd need to think more about how that's used on the client side. I'd tend towards updating the proto description and later add a one_of for the two introduction node types?
There was a problem hiding this comment.
To not increase the scope of this PR I added a TODO(bolt12) (will use this to keep track of lightweight things we need to fix) to SubscribeOnionMessages. We need to decide if we want to convert to a pubkey (which adds a db roundtrip) or if we just update the proto description that the field can contain both data types (together with adding a convenience one_of).
There was a problem hiding this comment.
Leaning more toward oneoff because, aside from the lookup cost, we'd also need a fallback when we fail to resolve the scid. Updating the proto description and maybe a follow-up adding the one_off is the cleanest path imo.
There was a problem hiding this comment.
Yes, I agree with that. I'll follow up with an RPC update PR.
There was a problem hiding this comment.
I have experimented with this a bit more and if we add a one_of to lightning.proto:BlindedPath we would leak protocol internals to the RPCs. With this we would have to make other endpoints aware of the scid intro node, which doesn't make too much sense as it's just a pointer to the node id. For the RPCs where it's cheap (like DecodeOffer) we can do the graph lookup and skip over paths that don't resolve (with logging). Instead I'd update the SubscribeOnionMessages proto description that we may pass out scid-based intro nodes verbatim. If needed we can later add a bool resolveScidIntros or similar to that RPC. That way the behavior is constrained to a single RPC, which is anyhow more of a debugging kind.
bitromortac
force-pushed
the
2604-bolt12-1a
branch
from
6999551 to
753cfde
Compare
| // valid offer must run ValidateOfferRead. Unknown TLVs are preserved on the | ||
| // returned offer so a later Encode can re-emit signed-range extras and keep | ||
| // offer_id stable. | ||
| func DecodeOffer(data []byte) (*Offer, error) { |
There was a problem hiding this comment.
Q: Should we set a maximum ceiling size for offers to avoid decoding huge offers?
There was a problem hiding this comment.
I added a check to DecodeOfferString (included in future PR), to have this function be decode only without validation.
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.
bitromortac
force-pushed
the
2604-bolt12-1a
branch
from
753cfde to
595c5a9
Compare
Abdulkbk
left a comment
Abdulkbk
left a comment
There was a problem hiding this comment.
Another pass, mostly nits and a few suggestions.
| EncryptedData: hop.CipherText, | ||
| } | ||
| bp.BlindedHops = append(bp.BlindedHops, rpcHop) | ||
| bp.IntroductionNode = oMsg.ReplyPath.IntroductionNode.Bytes() |
There was a problem hiding this comment.
Leaning more toward oneoff because, aside from the lookup cost, we'd also need a fallback when we fail to resolve the scid. Updating the proto description and maybe a follow-up adding the one_off is the cleanest path imo.
erickcestari
left a comment
erickcestari
left a comment
There was a problem hiding this comment.
Nice work!
LGTM! 🫴
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.
bitromortac
force-pushed
the
2604-bolt12-1a
branch
from
595c5a9 to
6c70938
Compare
github-actions
Bot
added
severity-critical
bitromortac
force-pushed
the
2604-bolt12-1a
branch
from
6c70938 to
bd16d49
Compare
4 participants
Part of the first milestone in #10736 (total WIP of the first milestone can be found here).
This PR adds a new
bolt12/package skeleton, its first message struct (Offer), and thelnwirework needed to host it. Includes TLV subtypes as well as reader/write message validation.