Use HTLC CLTV instead of onion CLTV values for payment claim timer#4402
Use HTLC CLTV instead of onion CLTV values for payment claim timer#4402TheBlueMatt wants to merge 6 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @carlaKC as a reviewer! |
There was a problem hiding this comment.
Rebased trampoline work on this and all's good except one assert that's in conflict with the instructions in TrampolineHop's docs. test_update_add_htlc_bolt2_sender_cltv_expiry_too_high also unhappy.
Removing that, everything I have passes with these changes and it saves me a few commits fixing up onion building 🧹
lightning/src/ln/onion_utils.rs
Outdated
| } else { | ||
| cur_cltv | ||
| }; | ||
| let cltv = hop.cltv_expiry_delta().saturating_add(cur_cltv); |
There was a problem hiding this comment.
Can move this inside the if idx == 0 branch because we're only using it for final hops.
Perhaps also rename to outgoing_cltv_value?
There was a problem hiding this comment.
Went with declared_incoming_cltv since that felt much closer to what its actually being used for.
|
👋 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. |
TheBlueMatt
force-pushed
the
2026-02-trampoline-cltv-consistency
branch
from
5d10f6b to
610dc33
Compare
|
Fixed the broken assertion and one broken test and rebased, thanks! |
When we receive an HTLC as a part of a claim, we validate that the CLTV on the HTLC is >= the CLTV that the sender requested we receive, but then we use the CLTV value that the sender requested we receive as the deadline to claim the HTLC anyway. This isn't generally all that interesting (they're always the same unless the previous-hop node gave us "free CLTV"), but for trampoline payments where we're both a trampoline hop and the blinded intro point and the recipient, it means we end up allowing ourselves less claim time than we actually have. Instead, here, we just use the actual HTLC CLTV deadline.
The docs for `RouteHop::cltv_expiry_delta` claim that it includes any trampoline hops, but the way we actually implemented onion building it did not. Because the docs described a simpler and more backwards-compatible API, we update the onion-building logic to match rather than updating the docs.
Now that we've cleaned up trampoline CLTV building and added `Path::total_cltv_expiry_delta`, we can use both to do some basic validation of CLTV values on blinded tails in `Route::debug_assert_route_meets_params`
Now that we are consistently using the `RouteHop::cltv_expiry_delta` as the last hop's starting CLTV rather than summing trampoline hops, `starting_htlc_offset` is a bit confusing - its actually always the current block height. Thus, here we rename it.
TheBlueMatt
force-pushed
the
2026-02-trampoline-cltv-consistency
branch
from
610dc33 to
e39437d
Compare
|
Rebased, fixed rustfmt, and squashed. |
|
✅ Added second reviewer: @joostjager |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4402 +/- ##
==========================================
- Coverage 85.89% 85.88% -0.02%
==========================================
Files 159 159
Lines 104402 104448 +46
Branches 104402 104448 +46
==========================================
+ Hits 89681 89706 +25
- Misses 12216 12235 +19
- Partials 2505 2507 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
carlaKC
left a comment
carlaKC
left a comment
There was a problem hiding this comment.
Thanks for cleaning this up, saves me some commits down the line!
3 participants
Somewhat annoyingly built on #4373 because I needed to use some of the test infra that was built there and also now its hard to rebase :(
cc @carlaKC