Conversation
Summary of ChangesHello @MPins, 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 significantly enhances the robustness and compliance of BOLT 11 invoice handling by integrating the latest specification updates. The primary focus is on enforcing strict validation for critical invoice fields, ensuring that payment hashes, payment secrets, and destination public keys meet their specified length requirements. This change prevents the processing of malformed invoices, improving overall network reliability and security. Highlights
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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 correctly updates the BOLT 11 invoice implementation to align with recent specification changes, enforcing mandatory field length validation for p, h, s, and n fields. Instead of skipping fields with incorrect lengths, the invoice decoding now fails, which improves spec compliance. The changes are well-implemented, with improved error handling in zpay32/decode.go and a new validation check for payment secrets in zpay32/invoice.go. All related tests have been diligently updated to reflect these new, stricter validation rules. The code is clean, adheres to the style guide, and the release notes are clear. Overall, this is a solid contribution.
| # Technical and Architectural Updates | ||
| ## BOLT Spec Updates | ||
|
|
||
| LND now [fail BOLT-11 payments](https://github.com/lightning/bolts/pull/1243) |
There was a problem hiding this comment.
nit: this release notes needs to be at the 0.22 release notes.
There was a problem hiding this comment.
Release notes at 0.22. Thank you @erickcestari !
🟡 PR Severity: MEDIUM
🟡 Medium (2 files)
🟢 Low (5 files)
AnalysisThe substantive changes in this PR are confined to the The remaining files are either test files (classified as LOW per *_test.go rules) or release notes documentation (LOW). The test file in No severity bump applied: only 3 non-test/non-generated files changed (threshold: 20) and 46 lines changed (threshold: 500). To override, add a |
|
@erickcestari after the rebase the test I will fix the test and include in the commit |
github-actions
Bot
added
severity-critical
PR Severity: CRITICAL
Critical (1 file)
Medium (2 files)
Low (1 file)
AnalysisThe PR implements BOLT11 invoice spec changes, with the primary non-trivial code changes in zpay32 (MEDIUM). However, the PR also touches The bump rules do not apply here (3 non-test/non-generated files, ~46 lines changed -- well below the 20-file and 500-line thresholds). To override, add a |
github-actions
Bot
added
severity-medium
PR Severity: MEDIUM Automated classification | 3 files | 46 lines changed Medium (2 files):
Low (6 files, excluded from severity):
Analysis: The highest-severity non-test files are in zpay32/*, which maps to MEDIUM severity. The PR modifies zpay32/decode.go and zpay32/invoice.go (invoice decoding and data structures for BOLT-11 invoices). No severity bump applies: only 3 non-excluded, non-auto-generated files changed (under the 20-file threshold), and 46 non-excluded lines changed (well under the 500-line threshold). The previous severity-critical label was incorrect. While the PR touches a test file under channeldb/migration_01_to_11/, that file is a *_test.go (LOW), not an actual migration. The substantive changes are confined to zpay32/. To override, add a severity-override-{critical,high,medium,low} label. |
erickcestari
left a comment
erickcestari
left a comment
There was a problem hiding this comment.
The new changes LGTM!
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
Very close! Think we just need another round to fix the findings and release notes.
github-actions
Bot
added
severity-critical
🟡 PR Severity: MEDIUM
🟡 Medium (2 files)
🟢 Low (6 files)
AnalysisThis PR ("BOLT11 invoice follows the new spec changes") updates BOLT11 invoice decoding/validation in The remaining 6 files are test-only changes ( No CRITICAL or HIGH packages (lnwallet, htlcswitch, contractcourt, sweep, peer, keychain, input, channeldb migration logic, funding, lnwire, routing, invoices, discovery, graph, etc.) are touched. Total non-test/non-generated lines changed (~60) and file count (3) are well below the bump thresholds (500 lines / 20 files), and no multiple distinct critical packages are involved, so no severity bump applies. The previous CRITICAL label appears to have been based on an earlier revision of this PR that likely touched different or additional files; the current file set only warrants MEDIUM. To override, add a |
|
@yyforyongyu: review reminder |
3 similar comments
|
@yyforyongyu: review reminder |
|
@yyforyongyu: review reminder |
|
@yyforyongyu: review reminder |
|
@yyforyongyu: review reminder |
1 similar comment
|
@yyforyongyu: review reminder |
|
@yyforyongyu: review reminder |
ae70e96 to
324963d
Compare
github-actions
Bot
added
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (1 file)
🟠 High (1 file)
🟡 Medium (2 files)
🟢 Low (5 files)
AnalysisThis revision adds a new non-test file, Non-test/non-generated file count (4: release notes, router_backend.go, decode.go, invoice.go) and line count (~86) remain below the 20-file/500-line bump thresholds, so no additional bump applies — the CRITICAL rating here comes directly from the migration-path and lnrpc-path rules, not from a size bump. Reviewers should confirm the To override, add a |
erickcestari
left a comment
erickcestari
left a comment
There was a problem hiding this comment.
Nice PR! The changes related to BOLT looks all correct to me. There's only the compatibility issue that is worth fixing.
Missing test for this:
This still lets malformed duplicate fixed-length fields through. For p/s/h/n, the duplicate guards above each parser (PaymentHash != nil, PaymentAddr.IsSome(), etc.) run before the length check, so a valid first field followed by an invalid-length duplicate is skipped and the invoice is accepted.
| Invoices created by lnd versions before v0.9.0 have no payment secret and | ||
| can no longer be decoded, so RPCs that return stored invoices, such as | ||
| `ListInvoices`, fail when these invoices are included. |
There was a problem hiding this comment.
I think we should still be able to call those RPC and be compatible. Only paying an invoice without s should be rejected. This also breaks the migration_11_invoices.go if the node as any invoice saved without payment_secret.
The zpay decoder should have an option that skips only the payment-secret check.
There was a problem hiding this comment.
Good point, thanks! I moved the payment secret check out of validateInvoice and into Decode, behind a new WithSkipPaymentSecretCheck() option. Decode stays strict by default, as the spec requires, and invoicesrpc.decodePayReq now uses the option, so ListInvoices, LookupInvoice and SubscribeInvoices still return invoices stored without a payment secret. A test in lnrpc/invoicesrpc/utils_test.go covers this.
Since the check is no longer in validateInvoice, NewInvoice and Encode also stop requiring it.
On the migration: I think migration_11_invoices.go itself is not affected, because it imports the frozen copy in channeldb/migration_01_to_11/zpay32, not the main zpay32 package. Only the test was affected, because it builds the payment request with zpay32.NewInvoice.
One thing that may be worth a look: downstream projects that call zpay32.Decode on stored invoices will also see the strict default. For example, loop's FixFaultyTimestamps in loopdb/sqlite.go runs every time
the database is opened. It decodes stored swap invoices from swaps with a publication deadline up to 2020 and returns an error if decoding fails. If some of those invoices have no payment secret, loopd would need the new option when it bumps its lnd dependency. I couldn't confirm whether such invoices exist in practice, so I'm just flagging it.
| options := []func(*zpay32.Invoice){ | ||
| zpay32.CLTVExpiry(uint64(testCltvDelta)), | ||
| zpay32.Description("test"), | ||
| zpay32.PaymentAddr([32]byte{}), |
There was a problem hiding this comment.
This test should pass without adding PaymentAddr
| zpay32.PaymentAddr([32]byte{}), |
There was a problem hiding this comment.
Done, the test now passes unchanged, since NewInvoice no longer requires a payment secret.
Decode now fails if a field with a fixed length (p, h, s, n) does not have the correct length (52, 52, 52, 53), as BOLT 11 requires since lightning/bolts#1243. Previously such fields were skipped. Each field is parsed before the duplicate check, so a wrong-length field fails with ErrInvalidFieldLength even if a valid field of the same type came first. A second valid payment hash is still rejected with ErrDuplicatePaymentHash, now detected through invoice.PaymentHash instead of a separate flag, since a parsed payment hash can no longer be nil. Decode now also requires a payment secret unless the invoice carries blinded paths. The new WithSkipPaymentSecretCheck option disables this check for callers that only display an invoice and never pay it.
Adapt the existing tests to the new behavior of Decode, parseTaggedFields, parse32Bytes and parseDestination. Test vectors without a payment secret gain an s field and are re-signed. Vectors that relied on wrong-length fields being skipped now expect ErrInvalidFieldLength. The routerrpc tests that decode invoices built with NewInvoice now set a payment address.
Add decode tests for an invoice without a payment secret and for p, h, s and n fields with an invalid length.
Invoices created by lnd versions before v0.9.0 have no payment secret. Since zpay32.Decode now rejects such invoices by default, decodePayReq uses WithSkipPaymentSecretCheck so that ListInvoices, LookupInvoice and SubscribeInvoices can still return them. The invoice is only converted for display here, never paid.
github-actions
Bot
added
severity-high
🟠 PR Severity: HIGH
🟠 High (2 files)
🟡 Medium (2 files)
🟢 Low (6 files)
AnalysisThe The current diff's highest-severity files are No severity bump applies: 5 non-test/non-generated files changed (threshold: 20) and ~116 non-test lines changed (threshold: 500), and no CRITICAL-tier packages are touched. Reviewers should confirm the To override, add a |
|
Thanks, @erickcestari! I addressed your comments: |
5 participants
Fixes #9718
Fixes #9842
This PR updates BOLT 11 invoice decoding to follow the current reader requirements of the spec. The length rule was clarified in lightning/bolts#1243.
Changes
Decodenow fails withErrInvalidFieldLengthif ap,h,sornfield does not have the correct length (52, 52, 52, 53). Previously such fields were silently skipped.validateInvoicenow rejects invoices without a payment secret (s), unless the invoice has blinded paths, whose final-hopPathIDserves the same purpose.Interaction with #11190
This PR is rebased on top of #11190 (reject duplicate payment hashes). Each field is now parsed before the duplicate check runs:
pfollowed by a second validpis still rejected withErrDuplicatePaymentHash.pis rejected withErrInvalidFieldLength, regardless of whether a validpcame before it.Since a parsed payment hash can no longer be nil,
invoice.PaymentHash != nilis enough to detect a duplicate, so the separatepaymentHashSeenflag was removed. The #11190 tests that relied on wrong-length fields being skipped now expectErrInvalidFieldLength.Compatibility
Stricter decoding. Invoices from other implementations that omit
sor contain wrong-length fixed fields are now rejected, as the spec requires.Old stored invoices. lnd has set a payment address on every new invoice since v0.9.0 (January 2020). Invoices created by earlier versions have no payment secret and can no longer be decoded. Because
invoicesrpc.CreateRPCInvoicedecodes the stored payment request, this affects the RPCs that return stored invoices:ListInvoicesfails for the whole call if any invoice in the requested range is one of these.LookupInvoicefails for these invoices.SubscribeInvoicesfails when it replays them from an oldadd_indexorsettle_index.The invoices themselves are not touched, and the SQL invoice migration does not decode payment requests, so node startup and payments are unaffected. This is noted in the release notes. If compatibility is wanted, a
DecodeOptionthat skips the payment-secret check could be added and used only indecodePayReq.Router check. The router-side check in
extractIntentFromSendRequest("payment request must contain either a payment address or blinded paths") is now also enforced by the decoder. It is kept as a defense in case the decoder is relaxed.Tests
sfield and are re-signed. Vectors that relied on skipping wrong-length fields now expectErrInvalidFieldLength.p,h,sandnfields.NewInvoiceinrouterrpcand the migration test now set a payment address.