lightning: refactor htlc switch #10230
Merged
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
f321x
force-pushed
the
refactor_htlc_handling
branch
7 times, most recently
from
1a642d4 to
4958484
Compare
f321x
force-pushed
the
refactor_htlc_handling
branch
from
4958484 to
82f6188
Compare
SomberNight
reviewed
Sep 24, 2025
Member
SomberNight
left a comment
SomberNight
left a comment
There was a problem hiding this comment.
Looks promising!
I've spent hours looking at this already but have no more time now. It is huge. Adding initial remarks.
Looked up to 3d0f172 for now.
Please try to avoid force-pushing this branch if possible.
ecdsa
reviewed
Sep 25, 2025
ecdsa
reviewed
Sep 25, 2025
ecdsa
reviewed
Sep 25, 2025
SomberNight
reviewed
Sep 25, 2025
ecdsa
reviewed
Sep 25, 2025
f321x
force-pushed
the
refactor_htlc_handling
branch
9 times, most recently
from
d108ea8 to
76bcd2e
Compare
Member
Author
|
I rebased the branch from HEAD d108ea8 to new HEAD 2fe9d1a Now the commits should be functionally independent and pass the unittests. The fixes of the followup commit have been integrated in the previous commits. |
f321x
force-pushed
the
refactor_htlc_handling
branch
from
76bcd2e to
2fe9d1a
Compare
Member
|
Thanks for rebasing. it is definitely easier to read now. |
f321x
force-pushed
the
refactor_htlc_handling
branch
from
472b77c to
987b300
Compare
Member
Author
Member
|
The first commit looks good to me. The commit with |
Member
|
I managed to extract a functionally independent part from the first commit. see this branch |
Member
|
987b300 looks good. |
refactor `htlc_switch` to new architecture to make it more robust against partial settlement of htlc sets and increase maintainability. Htlcs are now processed in two steps, first the htlcs are collected into sets from the channels, and potentially failed on their own already. Then a second loop iterates over the htlc sets and finalizes only on whole sets. # Conflicts: # electrum/lnpeer.py
Add `test_reject_invalid_min_final_cltv_delta` which is supposed to test that the peer rejects incoming htlcs with final cltv delta differing from what has been requested in the lightning invoice.
Test that lnpeer is rejecting incoming htlcs for invoices that are already expired.
Test that lnpeer rejects incoming htlcs for payments that have already been paid so invoices cannot be paid twice.
…_htlc Adds unittest to verify that lnpeer doesn't settle any htlcs of incomplete mpp.
1. Alice sends two HTLCs to Bob, not reaching total_msat, and eventually they MPP_TIMEOUT 2. Bob fails both HTLCs 3. Alice then retries and sends HTLCs again to Bob, for the same RHASH, this time reaching total_msat, and the payment succeeds Test that the sets are properly cleaned up after MPP_TIMEOUT and the sender gets a second chance to pay the same invoice.
Add sanity check that bob is not forwarding more sats to carol if than he receives from alice. (he only forwards once and doesn't try to forward multiple times). This should get caught by asserts in lnworker/lnpeer, nevertheless it seems to make sense to just add this test to prevent regressions of this kind.
Adds test_forwarder_fails_for_inconsistent_trampoline_onions which checks that a forwarder compares the trampoline onions of a mpp set and fails the set if the onions are not similar. In the test alice sends a mpp through bob with 2 htlcs, in one trampoline onion amt_to_forward is off by 1 msat so bob fails the htlc set instead of initiating the trampoline forwarding.
Add test `test_hold_invoice_set_doesnt_get_expired` to test_lnpeer to ensure a mpp set on which a hold invoice callback doesn't get expired automatically if the cltv_abs falls below MIN_FINAL_CLTV_DELTA_ACCEPTED as these sets should only get failed if the htlcs are safe to fail by the target of the hold invoice callback (e.g. swap got refunded successfully).
Benchmark how long a call to _run_htlc_switch_iteration takes with 10 trampoline mpp sets of 1 htlc each.
It seems useful to report exceptions happening in the htlc_switch to the crash reporter as it shouldn't raise exceptions in theory and this could help catch subtle bugs.
Adds test_payment_bundle_with_hold_invoice to simulate the use of a payment bundle in which one invoice of the bundle needs to trigger a hold invoice callback (similar to submarine swaps). Also modifies the test helper _test_simple_payment() to compare the results of all payment attempts instead of just returning after the first (of multiple) payments raises its result causing the test to miss if all payments were successful or not.
Splits `LNWallet.dont_settle_htlcs` into `LNWallet.dont_settle_htlcs` and `LNWallet.dont_expire_htlcs`. Registering a payment hash in dont_settle_htlcs will prevent it from getting fulfilled if we have the preimage stored. The preimage will not be released before the the payment hash gets removed from dont_settle_htlcs. Htlcs can still get expired as usual or failed if no preimage is known. This is only used by Just-in-time channel openings. Registering a payment hash in dont_expire_htlcs allows to overwrite the minimum final cltv delta value after which htlcs would usually get expired. This allows to delay expiry of htlcs or, if the value in the dont_settle_htlcs dict is None, completely prevent expiry and let the htlc get expired onchain. Splitting this up in two different dicts makes it more explicit and easier to reason about what they are actually doing. Please enter the commit message for your changes. Lines starting
Adds test for the dont_settle_htlcs functionality of lnworker used by Just-In-Time channels.
Adds unittest to test the dont_expire_htlcs logic
Use the `OnionFailureCode.INVALID_ONION_VERSION` (BADONION | PERM | 4) code when sending back `update_fail_malformed_htlc` as just sending a plain `BADONION` is not explicitly mentioned as correct in the spec.
Adds a simple forwarding test where the receiver fails a malformed onion with `update_fail_malformed_htlc`.
f321x
force-pushed
the
refactor_htlc_handling
branch
from
987b300 to
59586d6
Compare
Member
Author
Member
|
59586d6 looks good to me |
Member
|
Thanks a lot for working on this! It really is a substantial and quite deep change, for the better. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Refactors htlc_switch to make handling of mpp sets more robust. The changes are described per commit in the commit message.