bolt12: finalize the codec package - #11191
Open
bitromortac wants to merge 29 commits into
Open
bitromortac wants to merge 29 commits into
bitromortac wants to merge 29 commits into
Conversation
Bundle the payer-side invoice checks into one call so the sender flow cannot forget any of them: the reader gates, the expiry gate, the byte-for-byte mirror match against the originating request, and the binding of invoice_node_id to the node the payer expected to answer.
Decode/EncodeOfferString, Decode/EncodeInvoiceRequestString, and Decode/EncodeInvoiceString compose bech32, the per-message TLV codec, and the previously-landed validators into a single reader/writer API that consumers should use. Reader gates run against Bolt12Features, the package's known feature-bit set, defined in this commit. The string codecs that emit a wire-form message also gate Signature validity, since pre-sign Encode (used to compute the Merkle root) is permitted on (*X).Encode but not at the bech32 boundary.
The bolt12 codec parses bytes that arrive untrusted inside onion messages, so every decoder gets a property-based check that no input pattern drives a panic or breaks the encode/decode bijection. A further harness asserts merkleRoot is a pure function of its record set, since non-determinism there would silently break signature reproducibility. Wiring bolt12 into FUZZPKG makes 'make fuzz' exercise the package alongside the existing entries.
Add changes for the string-codec wrappers, the combined payment validator, and the fuzz harnesses in bolt12.
The comment standing where the writer-side feature check used to be argued its case in broken prose and used the word the reviewer found misleading. The file already has a form for a rule the codec cannot enforce, so use it and name the thing plainly: the bits are the caller's own and the reader checks them against the known bits it is given. Finding F11. lightningnetwork#10941 (comment)
Two comments on the payer-side checks called the key "the final blinded_node_id on the arrival path". Arrival is the receiver's view, which is how the spec states the writer rule, and these run on the payer, where the same key is the one it sent the request to. The phrase also collides with the separate reply-path arrival rule. The writer comment keeps its wording, which is correct there. Finding F15. lightningnetwork#10941 (comment)
The package doc claimed no LND dependencies while importing lnwire for the pure-TLV framing, the blinded-path types and the feature vector. Reusing those is deliberate, since it keeps one on-wire definition per structure, so say that instead of denying it. What the package really avoids is reaching into the daemon: the chain, the clock and the known feature bits all come from the caller. Finding F23.
The continuation error said a '+' must precede a non-whitespace character, while the code skips whitespace after the marker and then requires a neighbour. The rule it enforces is that a marker joins two characters, so say that. The README line ran to 129 columns. Findings F43 and F44. lightningnetwork#11001 (comment) lightningnetwork#11001 (comment)
The invoice path called a map of known feature bits a catalogue, a word that appears nowhere else in lnd. The other two readers in the same file already call it knownFeatures, and lnwire calls the same parameter featureNames, so the invoice path was the outlier. The type becomes InvoiceKnownFeatures, checkFeatures names its parameter like its callers do, and the word is gone from the docstrings and the test name. The struct itself stays: named fields are what stop a caller passing the blinded-path bits where the invoice bits belong. Findings F39 and F12. lightningnetwork#10941 (comment)
The census asserted how many invalid vectors fail at each of the three layers, counts that break on any re-vendoring of the spec vectors while proving nothing the table above it does not: that test already requires every invalid vector to be rejected and every valid one to pass.
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)
The strict features record was built only on the decode paths, so the guard that rejects a non-minimal feature vector ran on one side only. That guard exists to keep the Merkle leaf bytes stable, so it has to hold in both directions rather than depend on lnwire's encoder staying minimal. Encode now goes through the same record. Output is unchanged, the spec signature vectors still pass untouched, and the encoder is no longer dead code. Finding F21.
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 round-trip fixture paired erroneous_field 82, invreq_amount, with a suggested_value of 00 01 86 a0. The leading zero makes that a non-minimal tu64, so a peer decoding it as the field's own type fails and loses the correction the message exists to carry. The fixture now uses the canonical encoding. The writer cannot catch this yet, and the note claiming the schema is caller context overstated the obstacle: the package defines every field number and type for both messages, it just has no field-number to type table. That is now a TODO, and Encode's contract says the value is unchecked. Finding F18. lightningnetwork#10958 (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.
The round trip covered four of eleven fields and asserted byte identity only. Byte identity cannot see a field that is wired into the encode path but not into the decode path: such a field survives as an unknown TLV and re-encodes cleanly while its typed value disappears. Comparing the decoded struct against the fixture catches it, verified by dropping offer_quantity_max from the decode stream. The fixture also carries an unknown odd TLV in the offer range, so the offer keeps the byte-exact preservation its siblings already pin. Findings F31 and F2. lightningnetwork#10832 (comment) lightningnetwork#10789 (comment)
The round trip covered four of twenty-one fields and asserted byte identity only, which cannot see a field wired into the encode path but not into the decode path. Comparing the decoded struct against the fixture catches it, verified by dropping invreq_quantity from the decode stream. The request is also signed rather than carrying a placeholder, so the fixture is one a reader would accept. Finding F31. lightningnetwork#10832 (comment)
The round trip populated six of thirty fields despite its name, and asserted byte identity only, which cannot see a field wired into the encode path but not into the decode path. Comparing the decoded struct against the fixture catches it, verified by dropping invoice_relative_expiry from the decode stream. The invoice is signed and read-validated too, so the fixture is one a payer would accept. Finding F31. lightningnetwork#10832 (comment)
The request side had a test for the offer_amount times quantity overflow, the invoice side did not, and it was the only uncovered branch in the invoice amount check. Verified by neutering the guard, which makes the new case accept an invoice_amount of one against an authorized amount that wrapped to zero. Finding F9. lightningnetwork#10941 (comment)
Both decode tables matched error substrings where the decoder returns a sentinel, so a reworded message would keep passing a test that no longer proves anything. Four cases now use require.ErrorIs, and the rest stay on substrings because they assert ad-hoc messages with no sentinel behind them. The mixed idiom is the one TestDecodeChainsRecord already uses in this file. Finding F10. lightningnetwork#10941 (comment)
The vector table decoded each vector and compared every record against the expected hex, but never fed one back out. So the message-level Encode and the bech32 writer were only ever tested against fixtures we wrote ourselves. Each valid vector now has to reproduce its own string, which passes for all of them today and fails if the writer gains an extra character. Finding F45. lightningnetwork#11001 (comment)
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. Finding F22.
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.
Three symbols are un-exported rather than deleted: the two invoice_request string wrappers and UsableFallbackAddresses. Nothing transports that message as a string today, and nothing dispatches an on-chain fallback yet, so none of the three has a caller. They stay because the wrappers carry the lnr prefix the spec defines, the helper encodes the reader's MUST-ignore rules for fallback addresses, and their tests exercise both. Finding F22.
Covers the changes a consumer can observe: the invoice reader tightening, the decode length limits moving to the caller, the new OfferID operation and the narrowed export surface. The earlier 11146 entry drops invoice requests from its list of validated string entry points. The narrowing leaves those two wrappers internal, so the claim no longer holds for the shipped API.
6 tasks
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 |
GeorgeTsagk
reviewed
Sep 15, 2026
| `ValidateInvoiceForPayment` to bundle the payer-side invoice checks into one | ||
| call. | ||
|
|
||
| * [BOLT 12 codec |
Collaborator
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.
| // 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) { |
Collaborator
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?
| // 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 |
Collaborator
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.
ViktorT-11
reviewed
Sep 18, 2026
ViktorT-11
left a comment
ViktorT-11
left a comment
Collaborator
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) { |
Collaborator
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.
| // 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 |
Collaborator
There was a problem hiding this comment.
Maybe add a NOTE: prefix for this comment, i.e.:
// NOTE: One writer rule stays unchecked: ....
| `ValidateInvoiceForPayment` to bundle the payer-side invoice checks into one | ||
| call. | ||
|
|
||
| * [BOLT 12 codec |
Collaborator
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
bolt12: finalize the codec
Closes the Cleanups item of the codec milestone in the BOLT 12 epic. Based on #11146, so the diff is roughly half the size. It resolves the review feedback left open on the merged codec PRs, plus findings from a scoping pass over the package. No new protocol surface: the message types were already complete.
Behaviour
Decodeno longer caps input length.OfferIDderives an offer identity from an offer, an invoice request or an invoice.Surface: 15 symbols un-exported, 3 added, 119 to 107 exported. What stays is what a caller reaching the codec over an onion message needs.
Commits (Links to original comments in commit messages)
Not in this PR