Prefactor for Trampoline MPP accumulation by carlaKC · Pull Request #4510 · lightningdevkit/rust-lightning

carlaKC · 2026-03-24T17:16:16Z

This PR contains a set of refactors pulled out of #4493:

Extract common timeout / MPP logic into separate functions
Allow construction of blinded trampoline paths
Misc constructors + validation

ldk-reviews-bot · 2026-03-24T17:16:20Z

👋 Thanks for assigning @valentinewallace as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

ldk-claude-review-bot · 2026-03-24T17:27:51Z

No issues found.

The PR is a well-executed mechanical refactoring. I verified:

Serialization compatibility: All TLV tags remain unchanged. The write_claimable_htlc/Readable impl correctly maps through mpp_part with identical tags (0, 1, 2, 3, 4, 5, 6, 8, 10). timer_ticks is correctly re-initialized to 0 on deserialization.
committed_to_claimable invariant: The first_claimable_htlc flag in handle_claimable_htlc correctly prevents failing an HTLC that was just committed to a new ClaimablePayment entry. All error paths in check_incoming_mpp_part return before pushing.
fail_htlc! macro refactoring: htlc_source and htlc_value are correctly constructed before the macro definition. All HTLCPreviousHopData fields are present and equivalent to the old inline construction.
check_mpp_timeout correctness: The MPP completion condition (total_intended_recvd_value >= total_mpp_value) matches between check_incoming_mpp_part and check_mpp_timeout, preserving the critical invariant documented in the code.
claim_funds_from_hop signature change: The function only reads/copies fields from prev_hop and does not need ownership. All call sites correctly pass references.
check_incoming_htlc_cltv parameterization: Both call sites pass MIN_CLTV_EXPIRY_DELTA (48), matching the previously hardcoded value.
Blinded path genericization: ForwardTlvsInfo trait, ForwardNode<F>, BlindedPaymentTlvsRef<'a, F> — all correctly propagate the generic parameter with ForwardTlvs as the default.
PendingHTLCRouting::TrampolineForward field rename: incoming_shared_secret → trampoline_shared_secret at TLV tag 0; only the field name changed, not the tag, so serialization is compatible.
New trampoline_shared_secret() arm: Correctly added for TrampolineForward in htlc_previous_hop_data(), where it was previously falling through to _ => None.

codecov · 2026-03-24T18:25:27Z

Codecov Report

❌ Patch coverage is 91.08527% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.19%. Comparing base (ab31f99) to head (f15271e).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	94.36%	12 Missing ⚠️
lightning/src/blinded_path/payment.rs	75.60%	9 Missing and 1 partial ⚠️
lightning/src/ln/onion_payment.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #4510    +/-   ##
========================================
  Coverage   86.19%   86.19%            
========================================
  Files         160      160            
  Lines      107537   107713   +176     
  Branches   107537   107713   +176     
========================================
+ Hits        92693    92848   +155     
- Misses      12220    12238    +18     
- Partials     2624     2627     +3

Flag	Coverage Δ
tests	`86.19% <91.08%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ldk-reviews-bot · 2026-03-28T00:00:34Z

🔔 1st Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-03-30T00:01:16Z

🔔 2nd Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

carlaKC · 2026-03-30T12:30:15Z

Some discussion in the dependent PR, taking @valentinewallace off as a reviewer for now to save some bot-spam.

carlaKC · 2026-04-02T13:54:48Z

Updated to include suggested refactor, and pulled some of the blinded path test utils up into this PR as well.

valentinewallace

Nothing blocking, thanks for that mpp part refactor! Probably can get a 2nd reviewer on here. I didn't review the code moves that carefully but claude said they're legit

valentinewallace · 2026-03-26T16:04:19Z

lightning/src/ln/channelmanager.rs


+impl ClaimableHTLC {
+	// Increments timer ticks and returns a boolean indicating whether HTLC is timed out.
+	fn mpp_timer_tick(&mut self) -> bool {


nit: I'd prefer to in-line this if it's only used in one place

valentinewallace · 2026-04-06T19:06:48Z

lightning/src/ln/channelmanager.rs

-			user_channel_id: val.prev_hop.user_channel_id.unwrap_or(0),
-			cltv_expiry: val.cltv_expiry,
-			value_msat: val.value,
+			counterparty_node_id: val.mpp_part.prev_hop.counterparty_node_id,


nit: it looks like mpp_part.prev_hop is accessed in a million places - might be nice to have a helper

Considered this but I found it a bit ick to have some fields accessed with a helper and others directly - direct access also didn't mess up the formatting quite enough for it to be worthwhile.

Defer to second reviewer to make a call? Happy to change !

valentinewallace · 2026-04-06T19:27:49Z

lightning/src/ln/channelmanager.rs

 							};
-							check_total_value!(purpose);
+
+							if let Err(_) = self.handle_claimable_htlc(


nit: Err(()) if possible in case it changes so we're forced to update it

ldk-reviews-bot · 2026-04-07T14:35:58Z

👋 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.

Pull out all fields that are common to incoming claimable and trampoline MPP HTLCs. This will be used in future commits to accumulate MPP HTLCs that are part of trampoline forwards - we can't claim these, but need to accumulate them in the same way as receives before forwarding onwards.

We'll use this shared logic when we need to timeout trampoline HTLCs. Note that there's a slight behavior change in this commit. Previously, we'd do a first pass to check out total received value and return early if we'd reached it without applying a MPP tick to any HTLC. Now, we'll apply the MPP tick as we accumulate our total value received. This does not make any difference, because we never MPP-timeout fully accumulated MPP payments so it doesn't matter if we've applied the tick when we've reached our full amount.

We'll re-use this to check trampoline MPP timeout in future commits.

In the commit that follows we're going to need to take ownership of our htlc before this macro is used, so we pull out the information we need in advance.

We're going to use the same logic for trampoline and for incoming MPP payments, so we pull this out into a separate function.

To allow re-use with trampoline payments which won't use the ClaimablePayment type, make handling generic for anything with MPP parts. Here we also move counterparty skimmed logic to claimable payments, as this doesn't apply for trampoline.

For trampoline payments, we don't want to enforce a minimum cltv delta between our incoming and outer onion outgoing CLTV because we'll calculate our delta from the inner trampoline onion's value. However, we still want to check that we get at least the CLTV that the sending node intended for us and we still want to validate our incoming value. Refactor to allow setting a zero delta, for use for trampoline payments.

To use helper functions for either trampoline or regular paths.

To create trampoline forwarding and single hop receiving tails.

carlaKC · 2026-04-07T17:34:12Z

Addressed 2x nits.

ldk-reviews-bot · 2026-04-07T17:34:21Z

✅ Added second reviewer: @joostjager

carlaKC force-pushed the 2299-mpp-prefactor branch from 40f75a4 to f15271e Compare March 24, 2026 17:19

ldk-reviews-bot requested a review from wpaulino March 24, 2026 17:26

valentinewallace requested review from valentinewallace and removed request for wpaulino March 25, 2026 15:57

carlaKC added this to Weekly Goals Mar 26, 2026

jkczyz assigned carlaKC Mar 26, 2026

carlaKC removed the request for review from valentinewallace March 30, 2026 12:30

carlaKC force-pushed the 2299-mpp-prefactor branch from f15271e to 6aae0dd Compare April 1, 2026 21:24

carlaKC force-pushed the 2299-mpp-prefactor branch from 6aae0dd to 0e82461 Compare April 2, 2026 14:51

carlaKC requested a review from valentinewallace April 6, 2026 14:21

valentinewallace previously approved these changes Apr 7, 2026

View reviewed changes

carlaKC added 11 commits April 7, 2026 13:20

ln/refactor: add previous_hop_data helper for HTLCSource

8e9ebcc

ln/refactor: rename shared secret and populate in HTLCPreviousHopData

4c7a978

f Remove single use mpp_timer_tick

15bd188

ln/refactor: move on chain timeout check into claimable htlc

ad7c200

We'll re-use this to check trampoline MPP timeout in future commits.

ln/refactor: remove claimable htlc from fail_htlc macro

6bd628f

In the commit that follows we're going to need to take ownership of our htlc before this macro is used, so we pull out the information we need in advance.

ln/refactor: move checks on incoming mpp accumulation into method

27ca19e

We're going to use the same logic for trampoline and for incoming MPP payments, so we pull this out into a separate function.

f Use Err(()) rather than Err(_)

766371b

carlaKC added 3 commits April 7, 2026 13:20

blinded_path/refactor: make construction generic over forwarding type

7653d6a

To use helper functions for either trampoline or regular paths.

blinded_path: add constructor for trampoline blinded path

4a693c8

ln/test: add multi-purpose trampoline test helper

b2ac8f4

To create trampoline forwarding and single hop receiving tails.

carlaKC dismissed valentinewallace’s stale review via b2ac8f4 April 7, 2026 17:33

carlaKC force-pushed the 2299-mpp-prefactor branch from 0e82461 to b2ac8f4 Compare April 7, 2026 17:33

ldk-reviews-bot requested a review from joostjager April 7, 2026 17:34

Conversation

carlaKC commented Mar 24, 2026

Uh oh!

ldk-reviews-bot commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-claude-review-bot commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 24, 2026

Codecov Report

Uh oh!

ldk-reviews-bot commented Mar 28, 2026

Uh oh!

ldk-reviews-bot commented Mar 30, 2026

Uh oh!

carlaKC commented Mar 30, 2026

Uh oh!

carlaKC commented Apr 2, 2026

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

valentinewallace Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

valentinewallace Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

carlaKC Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

valentinewallace Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Apr 7, 2026

Uh oh!

carlaKC commented Apr 7, 2026

Uh oh!

ldk-reviews-bot commented Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ldk-reviews-bot commented Mar 24, 2026 •

edited

Loading

ldk-claude-review-bot commented Mar 24, 2026 •

edited

Loading