-
Notifications
You must be signed in to change notification settings - Fork 400
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lightning-invoice: explicitly enforce a 7089 B max length on BOLT11 invoice deser #3665
lightning-invoice: explicitly enforce a 7089 B max length on BOLT11 invoice deser #3665
Conversation
I've assigned @valentinewallace as a reviewer! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3665 +/- ##
==========================================
- Coverage 89.23% 89.20% -0.04%
==========================================
Files 155 155
Lines 119671 119874 +203
Branches 119671 119874 +203
==========================================
+ Hits 106787 106930 +143
- Misses 10268 10320 +52
- Partials 2616 2624 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good after feedback is addressed!
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
oh yeah here's some neat invoices I managed to generate: The longest valid invoice with all known tags (4458 B):
And the one generated in the test, at exactly 7089 B. It does need some non-standard tag fields to reach the limit though:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some formatting nits, otherwise LGTM.
The new `bech32-v0.11.0` version (prev: `v0.9.1`) now enforces a max length of 1023 bytes. Before there was no max. BOLT11 invoices can definitely exceed 1023 B with a long-ish description and 2 route hints, so this limit is likely too low. Having a limit is probably a good idea. What do other projects choose? Here's a brief survey: LDK (pre-0.1): (no limit) LDK (post-0.1): 1023 B LDK (post-PR): 7089 B LND[1]: 7089 B CLN[2]: (no limit) ACINQ[3][4]: (no limit) LND uses 7089 B, which was chosen to be "the max number of bytes that can fit in a QR code". LND's rationale is technically incorrect as QR codes actually have a max capacity of 7089 _numeric_ characters and only support up to 4296 all-uppercase alphanumeric characters. However, ecosystem-wide consistency is more important. A more conservative limit that would probably also suffice might be 2953 B, the QR code length limit for a lowercase bech32-encoded invoice. [1]: https://github.com/lightningnetwork/lnd/blob/6531d4505098eb14e6c24aedfd752fc15e85845d/zpay32/invoice.go#L87 [2]: https://github.com/ElementsProject/lightning/blob/0e7615b1b73eee161911763840d6260baf596755/common/bolt11.c#L683 [3]: https://github.com/ACINQ/lightning-kmp/blob/feda82c853660a792b911be518367a228ed6e0ee/modules/core/src/commonMain/kotlin/fr/acinq/lightning/payment/Bolt11Invoice.kt#L165 [4]: https://github.com/ACINQ/bitcoin-kmp/blob/master/src/commonMain/kotlin/fr/acinq/bitcoin/Bech32.kt#L122
d8f2aa5
to
3870823
Compare
Squashed and rebased on main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'm not exactly sure how to interpret the EDIT: The only mutation in |
d671596
into
lightningdevkit:main
Thanks @phlip9! Sorry for the delay, it's helpful for me if you re-request a review :) |
The long invoice tests work on LDK 0.0.125 but fail on LDK 0.1.0. Awaiting PR: <lightningdevkit/rust-lightning#3665>
Backported in #3697 |
v0.1.2 - Apr 02, 2025 - "Foolishly Edgy Cases" API Updates =========== * `lightning-invoice` is now re-exported as `lightning::bolt11_invoice` (lightningdevkit#3671). Performance Improvements ======================== * `rapid-gossip-sync` graph parsing is substantially faster, resolving a regression in 0.1 (lightningdevkit#3581). * `NetworkGraph` loading is now substantially faster and does fewer allocations, resulting in a 20% further improvement in `rapid-gossip-sync` loading when initializing from scratch (lightningdevkit#3581). * `ChannelMonitor`s for closed channels are no longer always re-persisted immediately after startup, reducing on-startup I/O burden (lightningdevkit#3619). Bug Fixes ========= * BOLT 11 invoices longer than 1023 bytes long (and up to 7089 bytes) now properly parse (lightningdevkit#3665). * In some cases, when using synchronous persistence with higher latency than the latency to communicate with peers, when receiving an MPP payment with multiple parts received over the same channel, a channel could hang and not make progress, eventually leading to a force-closure due to timed-out HTLCs. This has now been fixed (lightningdevkit#3680). * Some rare cases with multi-hop BOLT 11 route hints or multiple redundant blinded paths could have led to the router creating invalid `Route`s were fixed (lightningdevkit#3586). * Corrected the decay logic in `ProbabilisticScorer`'s historical buckets model. Note that by default historical buckets are only decayed if no new datapoints have been added for a channel for two weeks (lightningdevkit#3562). * `{Channel,Onion}MessageHandler::peer_disconnected` will now be called if a different message handler refused connection by returning an `Err` from its `peer_connected` method (lightningdevkit#3580). * If the counterparty broadcasts a revoked state with pending HTLCs, those will now be claimed with other outputs which we consider to not be vulnerable to pinning attacks if they are not yet claimable by our counterparty, potentially reducing our exposure to pinning attacks (lightningdevkit#3564).
The new
bech32-v0.11.0
version (prev:v0.9.1
) now enforces a max length of 1023 bytes. Before there was no max.BOLT11 invoices can definitely exceed 1023 B with a long-ish description and 2 route hints, so this limit is likely too low.
Having a limit is probably a good idea. What do other projects choose? Here's a brief survey:
LDK (pre-0.1): (no limit)
LDK (post-0.1): 1023 B
LDK (post-PR): 7089 B
LND 1: 7089 B
CLN 2: (no limit)
ACINQ 3 4: (no limit)
LND uses 7089 B, which was chosen to be "the max number of bytes that can fit in a QR code". LND's rationale is technically incorrect as QR codes actually have a max capacity of 7089 numeric characters and only support up to 4296 all-uppercase alphanumeric characters 5. However, ecosystem-wide consistency is probably more important.
A more conservative limit that would probably also suffice might be 2953 B. This would handle lowercase QR-encoded invoices while still giving us enough space for all "reasonable" invoices.