bolt12: add Bech32 string codec and spec test vectors - #11001
Conversation
7c3ac94 to
14fb769
Compare
| return "", nil, fmt.Errorf("bolt12: %w", ErrEmptyString) | ||
| } | ||
|
|
||
| // The characters must be either all lowercase or all uppercase. |
There was a problem hiding this comment.
nit: Since && short-circuits, we could avoid allocating the uppercase copy unless it's actually needed by doing something like:
lower := strings.ToLower(cleaned)
if cleaned != lower && cleaned != strings.ToUpper(cleaned) {
return "", nil, fmt.Errorf("bolt12: %w", ErrMixedCase)
}
cleaned = lowerThis preserves the existing behavior while avoiding the extra ToUpper allocation when the input is already lowercase.
There was a problem hiding this comment.
Nice, strings.ToUpper now sits inside the condition.
| hrp := cleaned[:one] | ||
| if _, ok := validHRPs[hrp]; !ok { | ||
| return "", nil, fmt.Errorf( | ||
| "bolt12: %w %q (want lno/lnr/lni)", |
There was a problem hiding this comment.
Nit: The accepted HRPs are already defined in validHRPs, but the error message duplicates them as string literals. It might be worth deriving the list from validHRPs so there's a single source of truth if the accepted HRPs ever change.
There was a problem hiding this comment.
Have changed it such that we reuse the definition.
ViktorT-11
left a comment
There was a problem hiding this comment.
Really awesome progress on this 🔥🎉!
Just posting some initial findings when crosschecking with the spec & the other Lightning implementations 🚀.
| // ~210,000 characters. A cap of 300,000 accommodates all valid payloads | ||
| // while preventing hostile inputs from forcing excessive allocations | ||
| // during decoding. | ||
| maxBolt12StringLen = 300_000 |
There was a problem hiding this comment.
I'm not sure if we should include this cap, and if we do, I think we may need to raise the cap. BOLT12 doesn't specify any strict cap that should be enforced, and I double checked (I checked through LLM) in Eclair, Core Lightning & LDK, and I think none of those repos include such a cap.
Additionally, if these fields are set to the max:
offer_metadata: 65,535 bytes
offer_description: 65,535 valid UTF-8 bytes
offer_issuer: 65,535 valid UTF-8 bytes
The Encode function will accept that request and produce a string longer than 300k characters, while the Decode function will error with ErrStringTooLong.
So if we keep it, we should at least cap both Encode & Decode at the same length.
There was a problem hiding this comment.
Thank you, I wanted to be conservative here, but we can discuss if it makes sense to remove the limit. I have increased the limit and added some explanation to the constant's docstring.
| if i == 0 || !isBech32Char(s[i-1]) { | ||
| return "", fmt.Errorf( | ||
| "bolt12: %w: '+' must follow a bech32 "+ | ||
| "character", | ||
| ErrInvalidContinuation, | ||
| ) | ||
| } | ||
|
|
||
| // Skip '+' and any following whitespace. | ||
| j := i + 1 | ||
| for j < len(s) && isWhitespace(s[j]) { | ||
| j++ | ||
| } | ||
| if j >= len(s) || !isBech32Char(s[j]) { | ||
| return "", fmt.Errorf( | ||
| "bolt12: %w: '+' must precede a bech32 "+ | ||
| "character", ErrInvalidContinuation, | ||
| ) | ||
| } |
There was a problem hiding this comment.
This specifically ties the left & right neighbouring characters after the + char to be a "Bech32Char". I don't think that should be required for the lno1 prefix part of the of the string.
The test vectors do define that using the + char in the prefix is allowed:
https://github.com/lightning/bolts/blob/311119388a46dfa859da3d2eda0ca836cfc5f078/bolt12/format-string-test.json#L13-L15
That testcase inserts the + char at:
l+no1....
However if the + char was instead inserted at:
ln+o1....
Our implementation would now fail as the o char is not a "Bech32Char".
I crosschecked with Eclair, Core Lightning, and LDK, and they'd all allow the + char to be inserted anywhere in the prefix, so I therefore think that's the correct implementation.
If you think it makes sense, I can open a PR to add a testcase to the spec repo's test vectors, which inserts the + char at ln+o1...., just to get consensus on that this should be a valid case.
There was a problem hiding this comment.
Right, looking at the test vectors, it says '+ can join anywhere', so changed it to not taking the spec literally.
| { | ||
| name: "plus right neighbour not bech32", | ||
| input: "ln+o1pqps7sjq", | ||
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "plus left and right neighbour not bech32", | ||
| input: "lno+1pqps7sjq", | ||
| wantErr: true, | ||
| }, |
There was a problem hiding this comment.
following up on #11001 (comment):
I.e. I don't think these test cases should error?
There was a problem hiding this comment.
Agreed, have updated the tests.
vctt94
left a comment
There was a problem hiding this comment.
Awesome progress on this 🔥!
I reviewed the PR locally, all five commits passed the focused bolt12 tests independently, and the overall structure looks solid.
I left a few comments around keeping the Encode/Decode contract symmetric, along with a small release-note link issue.
While reviewing the new lni path, I also reproduced the unknown-even signature-range behavior previously discussed in #10941. I’m not duplicating the finding here, but the new string decoding path makes the existing asymmetry directly observable.
Thanks again for pushing this forward 🚀
14fb769 to
3d0cf2b
Compare
🟡 PR Severity: MEDIUM
🟡 Medium (1 file)
🟢 Low (9 files)
AnalysisThis PR adds a new To override, add a |
ba47762 to
70cd09c
Compare
|
Thanks all for your reviews 🙏 Rebased on master and fixed a go mod issue. |
| // string above maxBolt12StringLen, but the caller must set a smaller limit for | ||
| // its own medium. See the caller obligations in the package documentation. | ||
| func Decode(s string) (string, []byte, error) { | ||
| if len(s) > maxBolt12StringLen { |
There was a problem hiding this comment.
Nice-to-have / non-blocking: this limit is checked before stripContinuation, so continuation formatting counts toward the payload-derived string limit. At the boundary, Encode can produce an unwrapped string of exactly maxBolt12StringLen, while inserting a legal + (and optional whitespace) makes Decode return ErrStringTooLong even though the cleaned string and decoded payload are unchanged. This only affects unusually large strings near the 1 MiB limit, so I do not consider it blocking. It may still be worth applying the canonical/payload limit after normalization, keeping a separate raw transport limit if desired, and adding a wrapped-max-payload test.
| func toBech32Bytes(s string) ([]byte, error) { | ||
| result := make([]byte, len(s)) | ||
| for i := 0; i < len(s); i++ { | ||
| idx := strings.IndexByte(charset, s[i]) | ||
| if idx < 0 { | ||
| return nil, fmt.Errorf( | ||
| "bolt12: %w: invalid character 0x%02x at "+ | ||
| "position %d of the cleaned data "+ | ||
| "string %s", | ||
| ErrInvalidCharacter, s[i], i, s, |
There was a problem hiding this comment.
nit: Could we avoid including the full data string in this error since it can be about 1.68 MB and may expose invoice contents if logged?
The bolt12 package can already encode and decode the TLV layer but has no way to carry an offer as a human-transportable string, which is the form the spec specifies for QR codes, URLs and email signatures. BOLT 12's envelope is subtractive relative to BIP-173: there is no BCH checksum, because the BIP-340 signature over the Merkle root already secures the payload, and a '+' continuation marker may split the string across lines. btcutil/bech32's public API always wraps the checksum, so the alphabet layer is duplicated here rather than reused. Enforce a whitelist of BOLT 12 prefixes (lno, lnr, lni) on both Encode and Decode.
Vendor the BOLT 12 offers-test.json fixtures so the offer decoder and validator are checked against the specification's own strings rather than hand-authored ones, which cannot drift from the spec without someone noticing. Invalid vectors are tested to verify they are rejected at some layer, and an aggregate stage census pins the distribution across bech32 decode, TLV decode, and validation.
The invoice_request codec has round-trip coverage against locally- constructed messages only, so a canonical-encoding bug would go unnoticed until a real peer rejected a signature. Drive the decoder from the spec's signature-test invoice_request and assert that re-encoding is byte-identical to the wire bytes, because the signature commits to the Merkle root of that exact encoding.
Add changes for the bech32 work in bolt12.
70cd09c to
c73b0d0
Compare
ViktorT-11
left a comment
There was a problem hiding this comment.
Nice, thanks for the updates. LGTM 🔥!
Adding a few non-blocking comments below, where I think the comment regarding the offer length cap for decoding is important feedback.
| for j < len(s) && isWhitespace(s[j]) { | ||
| j++ | ||
| } | ||
| if j >= len(s) || !isContinuationNeighbour(s[j]) { | ||
| return "", fmt.Errorf( | ||
| "bolt12: %w: '+' must precede a "+ | ||
| "non-whitespace character", | ||
| ErrInvalidContinuation, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Not sure if: "bolt12: %w: '+' must precede a non-whitespace character" is the correct error message to use here, as you're actually skipping the whitespaces after the "+" char above.
|
|
||
| // maxBolt12RawStringLen is the largest raw BOLT 12 string the codec | ||
| // accepts, continuation markers and whitespace included. | ||
| maxBolt12RawStringLen = 2 * maxBolt12StringLen |
There was a problem hiding this comment.
I'm still not fully convinced that it's a great idea for only lnd to include this cap (at least for Decoding), and IMO we should increase this cap quite a bit if we do.
The risk I see with including it, is that there'll for some reason in the future be use cases where undefined unknown odd fields become used which pushes the length over our current limit. If that becomes the case, they'll become incompatible with old lnd implementations, despite them perhaps being fully payable by lnd.
One such use case would for example be a scheme that specifies:
- IF you can read and interpret the "undefined unknown odd fields", then pay via a today undefined payment method.
OR
- IF NOT, then proceed with the payment as per the BOLT12 specification today.
I.e. an idea similar to what "BIP 21/321" offers for on-chain & lightning payments, but using BOLT12 offers with lightning + a currently unknown payment method.
Ultimately, I don't see this issue as fully blocking, but I think it's worth discussing among the reviewers if this is something we actually want to include or not.
Personally, I'd be in favour of removing this cap or at least increase the cap quite a bit just to minimize the above ever becoming an issue.
There was a problem hiding this comment.
I agree with Viktor here. My main concern is that this turns an lnd resource limit into a restriction of the generic BOLT12 decoder.
Since unknown odd fields are explicitly meant to allow forward-compatible extensions, we could eventually reject otherwise valid/payable objects just because they exceed a limit chosen today.
I'd prefer Decode to enforce BOLT12 encoding rules and leave resource limits to the caller/transport.
Not blocking from my side either, but I'd lean toward removing the decoding cap.
There was a problem hiding this comment.
I'll research the tradeoffs here and will get back to it 🙏. Will address this in a follow-up.
| @@ -0,0 +1,7 @@ | |||
| # BOLT 12 Spec Test Vectors | |||
|
|
|||
| These test vectors are vendored from the upstream [lightning/bolts](https://github.com/lightning/bolts) specification repository. | |||
| t.Run(tc.Description, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| _, tlvBytes, bech32Err := Decode(tc.Bolt12) |
There was a problem hiding this comment.
I think potentially there would be value in expanding this test to call the bech32 Encode function with the contents of this, and then also call offer.Encode below, just to include test coverage of those functions with the contents of the test vectors.
Part of #10736.
Adds checksumless Bech32 string encoding and decoding (
Encode,Decode) for BOLT 12 objects (lno,lnr,lni), including+continuation line stripping, case normalization, and input length bounding before allocation.Additionally, this PR vendors upstream
lightning/boltstest vectors (offers-test.json,format-string-test.json) to verify Bech32 parsing, TLV decoding, and semantic validation with exact field-level hex and length assertions across all test cases.