[0.1] Backports for 0.1.2 #3697
Merged
+658
−76
Conversation
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
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
Now that `lightning` depends on `lightning-invoice`, we should re-export it like we do `bitcoin` and `types`.
|
👋 Thanks for assigning @G8XSU as a reviewer! |
TheBlueMatt
force-pushed
the
2025-04-0.1.2-backports-2
branch
from
f751bae to
735b4a3
Compare
jkczyz
approved these changes
Apr 1, 2025
|
👋 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. |
|
✅ Added second reviewer: @valentinewallace |
G8XSU
approved these changes
Apr 1, 2025
Merged
In a coming commit we'll need to hold references to `TestChannelManager` in threads, requiring that it be `Sync`. Fairly minor merge conflicts addressed in: * `lightning/src/util/test_utils.rs`
In a comming commit we'll add a test that relies heavily on lock fairness, which is not provided by the default Rust `Mutex`. Luckily, `parking_lot` provided an `unlock_fair`, which we use here, though it implies we have to manually implement lock poisoning. Trivial merge conflict resolved in `lightning/Cargo.toml`
When we claim an MPP payment, we need to track which channels have had the preimage durably added to their `ChannelMonitor` to ensure we don't remove the preimage from any `ChannelMonitor`s until all `ChannelMonitor`s have the preimage. Previously, we tracked each MPP part, down to the HTLC ID, as a part which we needed to get the preimage on disk for. However, this is not necessary - once a `ChannelMonitor` has a preimage, it applies it to all inbound HTLCs with the same payment hash. Further, this can cause a channel to wait on itself in cases of high-latency synchronous persistence - * If we have receive an MPP payment for which multiple parts came to us over the same channel, * and claim the MPP payment, creating a `ChannelMonitorUpdate` for the first part but enqueueing the remaining HTLC claim(s) in the channel's holding cell, * and we receive a `revoke_and_ack` for the same channel before the `ChannelManager::claim_payment` method completes (as each claim waits for the `ChannelMonitorUpdate` persistence), * we will cause the `ChannelMonitorUpdate` for that `revoke_and_ack` to go into the blocked set, waiting on the MPP parts to be fully claimed, * but when `claim_payment` goes to add the next `ChannelMonitorUpdate` for the MPP claim, it will be placed in the blocked set, since the blocked set is non-empty. Thus, we'll end up with a `ChannelMonitorUpdate` in the blocked set which is needed to unblock the channel since it is a part of the MPP set which blocked the channel. Trivial conflicts resolved in `lightning/src/util/test_utils.rs`
TheBlueMatt
force-pushed
the
2025-04-0.1.2-backports-2
branch
from
735b4a3 to
5006c6b
Compare
|
Grrr, I misread a conflict and skipped a commit from #3680 which we did actually need. Pushed it as well now. |
jkczyz
approved these changes
Apr 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Backports of #3671, #3665, and #3680