bolt12: split off review cleanups and test changes from #11191 - #11237
Merged
saubyk merged 13 commits intoSep 24, 2026
Merged
Conversation
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.
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 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 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)
bitromortac
force-pushed
the
20260923-9259-dispatch
branch
from
September 24, 2026 08:11
cf86678 to
59739d6
Compare
🟡 PR Severity: MEDIUM
🟡 Medium (5 files)
🟢 Low (7 files)
AnalysisAll changes are confined to the To override, add a |
6 tasks
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.
This PR takes the commits from #11191 that do not change behavior and puts them in a separate PR. The goal is to make #11191 smaller and to let these changes merge on their own.
Most commits answer review threads on the earlier BOLT 12 PRs. They fix comments and names, and they remove tests that break each time the spec vectors are vendored again.
The round-trip tests for offer, invoice_request and invoice now fill every field. They also compare the decoded struct with the fixture. A byte-identity check alone cannot detect a field that the encoder writes but the decoder does not read. That field comes back as an unknown TLV and encodes again to the same bytes.
The encoder now uses the same strict features record as the decoder. This keeps the Merkle leaf bytes stable because the encoder no longer relies on lnwire to stay minimal. The encoded output does not change, and the spec signature vectors pass without changes.
No release note, because nothing here changes behavior.