bolt12: finalize the codec package - #11191
bitromortac wants to merge 16 commits into
Conversation
github-actions
Bot
added
the
severity-high
🟠 PR Severity: HIGH
🟡 Medium (10 files)
🟢 Low (16 files)
AnalysisAll changed source files live under To override, add a |
| `ValidateInvoiceForPayment` to bundle the payer-side invoice checks into one | ||
| call. | ||
|
|
||
| * [BOLT 12 codec |
There was a problem hiding this comment.
This bullet links to 11146 but the finalization is this PR. That is also why the release notes check fails, it greps for a link to 11191.
There was a problem hiding this comment.
Removed the entry rather than renumbering it, and applied no-changelog. Nothing here is reachable by an operator yet. Also fixed the 11146 bullet, which promised invoice_request string entry points that this PR removes.
There was a problem hiding this comment.
A small correction to my earlier reply: this PR keeps the invoice request string entry points after all. EncodeInvoiceRequestString and DecodeInvoiceRequestString stay exported for requests that answer no offer, and the 11146 bullet names invoice requests again.
| // transmission, so a populated signature is required and verified against | ||
| // invreq_payer_id. Writer-side validation is delegated to | ||
| // (*InvoiceRequest).Encode. | ||
| func encodeInvoiceRequestString(ir *InvoiceRequest) (string, error) { |
There was a problem hiding this comment.
With this un-exported, nothing exported enforces the writer side signature MUST for the invoice_request wire form. Encode stays pre-sign permissive by design and the onion path emits raw TLV, so a wiring bug that forgets to sign only surfaces as a rejection at the remote peer. Same applies to an invoice emitted as raw TLV. Is the PayOffer milestone meant to own that, or should the codec keep one signed-encode entry point for the raw TLV forms, the way EncodeInvoiceString does for strings?
There was a problem hiding this comment.
You were right, and it was wider: the raw TLV path had no gate for either message. Added EncodeSigned on InvoiceRequest and Invoice, which requires the signature and verifies it, and signing now runs the write validator first.
Also fixed comments claiming a caller must encode before signing. Signing derives the Merkle root from AllRecords, so it never needs the encoded bytes.
| // in the offer range changes the id. That is what makes a store lookup by id | ||
| // the exact-match check the reader requirements ask for. | ||
| // | ||
| // The id is a local store key, not an interop value: BOLT 12 defines no offer |
There was a problem hiding this comment.
Was the Merkle root considered here? merkleRoot is already in the package and other implementations derive their offer id from it, so the same ids would agree with CLN and LDK. Once the id leaks into an RPC or a DB export the divergence is baked in. If SHA256 is deliberate that is fine, the doc already flags it, just checking it was weighed.
There was a problem hiding this comment.
Weighed, and the docstring you read was wrong. CLN does not use the Merkle root: common/bolt12.c hashes the offer TLV spans exactly as we do. Verified against its doc/schemas/offer.json — we match c5cde029... byte for byte, while the Merkle root gives 00d1e836.... Switching would break the only agreement that exists, since LDK tags the root and eclair uses it bare.
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Nice, this PR looks quite ready to be merged IMO, once the either mine or @GeorgeTsagk release note comment have been addressed.
In my opinion though, we could make this PR easier to review if we:
- Broke comment & test changes only commits into a separate PR. That'd make that PR easy to review and ready to be merged very quickly.
- Then also have the implementation logic changing commits such as for example 17a6bd7 or f7b9e9b into another separate PR. That'd make those commits quicker & easier to review.
Feel free to ignore the PR structure feedback though if you disagree.
| // The id is a local store key, not an interop value: BOLT 12 defines no offer | ||
| // identifier, and other implementations derive theirs from the offer's Merkle | ||
| // root, so the two disagree for the same offer. | ||
| func OfferID(m lnwire.PureTLVMessage) ([32]byte, error) { |
There was a problem hiding this comment.
Nit: Just noting that this is not used anywhere outside of tests, but probably to be expected at this stage.
There was a problem hiding this comment.
Correct here. It has four call sites on the integration branch, so it lands ahead of its consumers.
| // messages such as invoices, where unknown TLVs must be preserved to keep | ||
| // signatures valid). | ||
| // | ||
| // One writer rule stays unchecked: a suggested_value is not verified against |
There was a problem hiding this comment.
Maybe add a NOTE: prefix for this comment, i.e.:
// NOTE: One writer rule stays unchecked: ....
There was a problem hiding this comment.
Done. The commit introducing that line moved to #11237, so the prefix is added here instead.
| `ValidateInvoiceForPayment` to bundle the payer-side invoice checks into one | ||
| call. | ||
|
|
||
| * [BOLT 12 codec |
There was a problem hiding this comment.
nit: IMO, having a title called "BOLT 12 codec finalization" in the release notes is too implementation flow/PR specific, and for users which reads this it doesn't really make sense as they haven't been following the PR process.
IMO, if you're going to have release notes for this PR, break out the points that this specific PR fixed into their own bullet points. To be honest though, I'd also be supportive of just removing the release notes all together for this specific "cleanup" PR, as it doesn't add any meaningful functionality to v0.22.0 that wasn't already introduced by previous PRs.
There was a problem hiding this comment.
Removed the entry, as you suggested.
8436a80 to
23415ba
Compare
|
Rebased on github.com//pull/11237, the first half of this PR is in there. |
23415ba to
89354ee
Compare
| // against the type of the field erroneous_field names. Emitting a value the | ||
| // peer cannot decode is therefore possible, see the TODO in | ||
| // validateInvoiceErrorWrite. | ||
| func (ie *InvoiceError) encode() ([]byte, error) { |
There was a problem hiding this comment.
This un-export strands the outbound half of the message. The receiver flow emits an invoice_error as raw TLV when it rejects an invoice_request, and with encode internal there is no way to serialize one outside the package, while the fields stay exported so it can still be built. By this PR's own criterion that is something the onion caller needs. An exported encode here could also pick up the writer rule the NOTE above calls unchecked, the way EncodeSigned did for the signed messages. Or is the plan to add it together with its caller in the PayOffer PR?
There was a problem hiding this comment.
Good catch, thanks! InvoiceError keeps an exported Encode now, symmetric with DecodeInvoiceError, since an unsigned message has no signature check to skip. The suggested_value rule stays a TODO in validateInvoiceErrorWrite, because it needs the schema of the rejected message, so it fits better with the caller in the PayOffer PR.
The invoice reader skipped the must-understand check for types 240 to 1000, so an invoice carrying unknown even type 242 was accepted while the same type on an invoice_request was rejected. The spec exempts that range from the out-of-range rule only, and BOLT 1 still makes an unknown even type fatal. Core Lightning grants the range no exception either, so this was a divergence rather than a choice. Finding F8. lightningnetwork#10941 (comment)
The offer writer checked the allowed range but not the must-understand rule, while the offer reader checks both. The state is only reachable for an offer decoded and then mutated, since the typed field set cannot express an unknown type, but the asymmetry made a reader of the code work out why one of the two rules was missing. Finding F5. lightningnetwork#10789 (comment)
Decode capped the raw and the cleaned string length, which no other implementation does and which the spec does not ask for. The cap protected nothing: a decode allocates on the order of its input, each record is already bounded by tlv.MaxRecordSize and every subtype decoder bounds itself against the bytes present, and the input exists in the caller's memory before Decode runs. What it could do is reject a spec-valid message once unknown odd fields push a string past a limit chosen today. Encode keeps its payload bound, since there we choose what to emit, and the constant now says it is a writer policy. The caller bounds its own medium: the onion-message envelope for an invoice_request and an invoice, the RPC or CLI for a pasted or scanned offer string. Finding F41. lightningnetwork#11001 (comment)
The receiver identifies the offer an invoice request answers by hashing the request's offer fields and looking the result up in its store, so that lookup is the exact-match check the reader requirements ask for. The rule belongs in the codec, which owns the encoding and the range predicate, rather than in each caller. The hash covers the offer ranges of any pure-TLV message, so one function serves the offer, the invoice_request and the invoice. For an offer it equals the hash of the whole encoding, since every offer TLV already sits in those ranges. Finding F40. lightningnetwork#10941 (comment)
The invoice_request and invoice validators quote each spec bullet above the check it authorises and run in the spec's order, so they can be read beside the spec block. The two offer validators, which landed before that convention, had none: 29 and 37 stanza lines now, against zero before. The writer moves two checks to reach spec order, the chains rule ahead of the amount rules and the nil-key guard down to the issuer stanza it belongs to. Every check is otherwise the same one, and no test expectation changed, so no input flipped to a different first error. Both functions now also state the rules the codec cannot enforce and why, which is how the gaps stay visible. Finding F38.
DecodeInvoiceString folds the reader gates into the decode, so a caller that only wants to display an invoice it already validated when it stored it has no entry point. Such a caller had to reach for the raw bech32 decode and parse the bytes itself, which accepts any of the three prefixes. DecodeInvoiceStringUnvalidated pins the lni prefix and skips the gates. It is the one legitimate raw-decode caller, so the bech32 primitives can leave the API next. DecodeInvoiceString now delegates to it and adds the gates on top, so the prefix check and the TLV decode have one home rather than two copies. Finding F22.
Decode and Encode become decodeBech32 and encodeBech32. The bare names read like the message codec they sit next to, and no caller outside the package needs them: the string entry points fold bech32 into a validated call, and a caller that wants a raw decode has DecodeInvoiceStringUnvalidated. The rename is mechanical. Four comments in bech32.go named the old symbols, so they name the new ones now. The encoder docstring also stops claiming its payload bound mirrors a reader limit, because the reader no longer has one.
The verifiers go, because the readers call them and nothing verifies a signature on its own. The writer validators go, because Encode is the gate and already runs them. The reader validators that an exported entry point folds in go too: DecodeOfferString for the offer, and ValidateInvoiceForPayment for the invoice against its request. What stays exported is what a caller that reaches the codec over an onion message needs, because an invoice_request and an invoice arrive as raw TLV rather than as strings. Finding F22.
89354ee to
ddea711
Compare
GeorgeTsagk
left a comment
GeorgeTsagk
left a comment
There was a problem hiding this comment.
The share-one-features-record change (F21) got lost in the split. It was in this PR before, 11237's description claims it moved there, but neither 11237 as merged nor this branch carries it. So encode still goes through lnwire's default features record and strictFeaturesEncoder in subtypes.go is dead code. The original issue stands: the guard that keeps the Merkle leaf bytes independent of lnwire staying minimal only holds on the decode side. Cherry-picking 56b8999 from the pre-split branch restores it.
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Generally looking great, and I'm ready to ACK once my & @GeorgeTsagk latest feedback has been addressed 🔥.
| // a missing-field violation. Symmetric with validateInvoiceRead. | ||
| if inv.InvoiceAmount.ValOpt().UnwrapOr(0) == 0 { | ||
| return ErrZeroInvoiceAmount | ||
| } |
There was a problem hiding this comment.
Just pointing out that this check diverges from the spec, as the BOLT 12 spec only requires that the invoice_amount is present and represent the minimum accepted amount, it does not require it to be greater than zero. This is present for both read & writes.
This diverges from how this is handled in the other implementations as well, as they will accept offers with "zero" amounts.
There was a problem hiding this comment.
I don't see this as blocking though FYI.
There was a problem hiding this comment.
Agreed that ErrZeroInvoiceAmount goes beyond the spec. The check predates this PR, and this PR does not touch it, so I would keep it for now and change it in a follow-up. Dropping it also needs a rule for the payer, because BOLT 2 requires an HTLC amount_msat greater than 0, so there is nothing valid to send for a zero minimum.
| // reach a peer, so it is where the writer-side MUST is enforced. An invoice | ||
| // request travels as raw TLV inside an onion message, so that boundary is not | ||
| // the bech32 string form. | ||
| func (ir *InvoiceRequest) EncodeSigned() ([]byte, error) { |
There was a problem hiding this comment.
Since this PR now unexports the "encode" function and only exports this new EncodeSigned function for invoice requests, we're effectively disabling this use case: https://github.com/lightning/bolts/blob/1aadb719b4007c4cea0ba6e36b08c4fb53788dee/12-offer-encoding.md?plain=1#L376
Such invoice requests which are not a response to an offer, should explicitly not include a signature:
https://github.com/lightning/bolts/blob/1aadb719b4007c4cea0ba6e36b08c4fb53788dee/12-offer-encoding.md?plain=1#L489
So I think we should still export an Encode function which doesn't sign the Invoice request?
There was a problem hiding this comment.
Thanks, good catch that this flow was blocked! I think the signature is not the obstacle, though. The reader rejects any invoice request without a correct signature using invreq_payer_id, and Core Lightning and Eclair also require one, so the payer signs an offer-less request too. What was missing is the transport: that request is published as an lnr1 string, and this PR had removed the encoder. EncodeInvoiceRequestString is back on top of EncodeSigned, and DecodeInvoiceRequestString is exported. ValidateInvoiceForPayment now accepts a nil expectedNodeID for an offer-less request, because the payer usually cannot confirm invoice_node_id out of band. TestOfferPaymentFlow and TestOfferlessPaymentFlow walk both flows end to end.
UsableFallbackAddresses is un-exported rather than deleted. Nothing dispatches an on-chain fallback yet, so it has no caller. It stays because it encodes the reader's MUST-ignore rules for fallback addresses, and its tests exercise them. Finding F22.
An invoice_request and an invoice reach a peer as raw TLV inside an onion message, not as a bech32 string. Encode is permissive about the signature by design, because a caller must encode before it can derive the Merkle root it signs, so nothing enforced the writer-side MUST on the path the bytes actually take. A caller that forgot to sign would find out only when the remote peer rejected the message. EncodeSigned is that entry point for both messages. It requires the signature and verifies it against invreq_payer_id or invoice_node_id, and EncodeInvoiceString now delegates to it, so the raw and string forms carry one rule.
A signature over a message that breaks the writer requirements is worthless, because the peer rejects the message on read. Running the write validator before the Merkle root is derived keeps a key from signing bytes no correct reader accepts, and it moves the check off the caller, which previously had to encode once purely to trigger it.
EncodeSigned is the entry point for an invoice_request and an invoice that leave the node, and the bech32 wrappers cover the string forms, so no caller outside this package needs a serialisation that skips the signature check. Un-exporting encode makes the writer-side requirement a property of the API rather than a rule a caller has to remember. Offer follows for consistency, since EncodeOfferString covers the only form it travels in. InvoiceError keeps its exported Encode. It is unsigned, so there is no check to skip, and raw TLV is the only form a receiver can send it in.
A request that answers an offer reaches its peer as raw TLV inside an onion message. A request that answers no offer is published by its payer as an lnr1 string instead, such as a QR code, and the payee reads that string to answer with an invoice, so the string encoder stays. It now delegates to EncodeSigned, as EncodeInvoiceString does, so the signature rule lives in one place. The three tests that pinned the encoder's refusal to emit an invalid, unsigned or badly signed request move to EncodeSigned accordingly.
For an invoice that answers a request with no offer, BOLT 12 lets the payer reject it only if it cannot confirm invoice_node_id out of band. The payer published that request and never addressed the payee, so it usually has no key to compare against. ValidateInvoiceForPayment still required one and failed on nil, which left a payer no honest argument to pass. Its doc also named a node the payer sent to, which does not exist in this flow. A nil expectedNodeID is now accepted for such a request. A payer that did confirm a key passes it and gets the comparison, and a response to an offer still requires one. TestValidateInvoiceNodeID covers all three cases. Review: lightningnetwork#11191 (comment)
The strict features record only ever decodes. Encode goes through lnwire's features record, so strictFeaturesEncoder never ran and duplicated lnwire's encoding. The record now passes its encode side through to lnwire's record, leaving one definition of the writer and the minimality check where it matters, on decode. That makes minimal output on encode an lnwire property we rely on, so a test pins it: every features field of an offer, invoice request and invoice is written minimally, and the strict decoder accepts the result.
ddea711 to
3dda4c7
Compare
|
@GeorgeTsagk thanks for tracking this down! You are right that the change got lost, and that |
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Great job, LGTM 🔥🎉!!
4 participants
bolt12: finalize the codec
Closes the Cleanups item of the codec milestone in the BOLT 12 epic. Resolves the review feedback left open on the merged codec PRs, plus a scoping pass over the package.
Stacked on #11237, which carries the comment, test and refactor commits and is already approved.
Behaviour
SignInvoiceRequestandSignInvoicevalidate before deriving the Merkle root, so a key cannot sign bytes a correct reader would reject.follow spec stanzas in offer validatorsonly reorders existing checks. No input changes between accepted and rejected, but an offer breaking two rules at once now reports a different sentinel.Surface
115 exported symbols down to 100. Added
OfferID,DecodeInvoiceStringUnvalidated, andEncodeSignedonInvoiceRequestandInvoice.EncodeSignedcloses the gap @GeorgeTsagk found: every exported way out of the package now enforces the writer-side signature MUST, including the raw TLV form an onion message carries.Removed the invoice_request string encoder, since nothing emits that form. Its decoder stays for an
lnr1string another implementation hands us.Notes
The package docs land in one commit at the end, so they describe the final API rather than intermediate shapes.
Not in this PR