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.

This is a re-review of the same PR. My prior review covered all the critical invariants and found no issues. After a second thorough pass examining the entire diff, I confirm:

Serialization compatibility: All TLV tags unchanged. write_claimable_htlc/Readable correctly route through mpp_part with identical tags. timer_ticks correctly reset to 0 on deserialization.
check_incoming_mpp_part / handle_claimable_htlc: The first_claimable_htlc guard correctly prevents failing HTLCs committed to new entries. All error paths in check_incoming_mpp_part return before pushing the HTLC.
fail_htlc! macro: htlc_source and value are correctly constructed before the macro definition and before claimable_htlc is moved. The continue 'next_forwardable_htlc ensures htlc_source is moved at most once.
MPP completion condition: Matches exactly between check_incoming_mpp_part (total_intended_recvd_value >= total_mpp_value) and check_mpp_timeout (same condition).
claim_funds_from_hop signature: Changed to &HTLCPreviousHopData — only reads fields, no ownership needed. All call sites pass references.
check_incoming_htlc_cltv parameterization: Both call sites pass MIN_CLTV_EXPIRY_DELTA, matching the previously hardcoded value.
ForwardTlvsInfo trait / ForwardNode<F> genericization: Correctly propagated with ForwardTlvs as the default where needed. Sealed trait prevents external implementation.
TrampolineForward field rename: incoming_shared_secret → trampoline_shared_secret at TLV tag 0 — name-only change, serialization compatible.
previous_hop_data() helper: Correctly returns slice of previous hop data for all HTLCSource variants.
check_mpp_timeout timer_ticks behavior: Minor difference from old code (ticks now incremented for completed MPPs before early-return), but functionally benign since the function returns false for complete MPPs regardless.
Test helper create_trampoline_forward_blinded_tail: Correctly computes fee_msat from BlindedPaymentPath::payinfo rather than hardcoding final_value_msat — this is more correct for single-hop blinded paths where the fee should be 0.

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.

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

Addressed 2x nits.

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

✅ Added second reviewer: @joostjager

TheBlueMatt

Only one real comment.

TheBlueMatt · 2026-04-10T11:41:26Z

lightning/src/ln/channelmanager.rs

-						sender_intended_value: outgoing_amt_msat,
-						timer_ticks: 0,
-						total_value_received: None,
+						htlc_value,


I will say, constructors in rust are way less readable then destructuring where you get the parameter name beside the value we're assigning :/

Yeah fair, I was also on the fence - will remove

TheBlueMatt · 2026-04-10T12:07:05Z

lightning/src/ln/channelmanager.rs

+
+	/// Returns a boolean indicating whether the HTLC has timed out on chain, accounting for a buffer
+	/// that gives us time to resolve it.
+	fn check_onchain_timeout(&self, height: u32, buffer: u32) -> bool {


nit: should buffer be hard-coded? its currently a constant across the code base and while we might want to make that configurable it probably should be constant between trampoline and not-trampoline.

TheBlueMatt · 2026-04-10T13:22:32Z

lightning/src/ln/channelmanager.rs

+		let ref mut claimable_payment = claimable_payments
+			.claimable_payments
+			.entry(payment_hash)
+			// Note that if we insert here we MUST NOT fail_htlc!()


We don't fail_htlc from here anymore.

TheBlueMatt · 2026-04-10T14:01:11Z

lightning/src/blinded_path/payment.rs

+/// Both [`ForwardTlvs`] (channel-based forwarding) and [`TrampolineForwardTlvs`] (trampoline
+/// node-based forwarding) implement this trait, allowing blinded path construction to be generic
+/// over the forwarding mechanism.
+pub trait ForwardTlvsInfo: Writeable + Clone {


hmm, do we really want people implementing their own ForwardTlvsInfo? Should we instead make it an internal thing and expose wrapper types?

Going with a sealed supertrait b/c it felt a bit cleaner and we have the pattern elsewhere.
Happy to do a wrapper if that's your pref!

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.

We only want to support trampoline and regular blinded paths, so we don't want to support implementation outside of this crate. We need the trait itself to be public as it's part of a public struct.

To create trampoline forwarding and single hop receiving tails.

carlaKC · 2026-04-10T18:49:29Z

Diff for last review round!

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

joostjager removed their request for review April 8, 2026 07:06

carlaKC requested a review from TheBlueMatt April 8, 2026 20:32

TheBlueMatt reviewed Apr 10, 2026

View reviewed changes

carlaKC added 3 commits April 10, 2026 14:11

ln/refactor: add previous_hop_data helper for HTLCSource

5875ab3

ln/refactor: rename shared secret and populate in HTLCPreviousHopData

cd2ff9b

carlaKC added 15 commits April 10, 2026 14:11

f remove constructor for ClaimableHTLC and MppPart

8c2c0ef

f Remove single use mpp_timer_tick

98d5e85

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

8f81a01

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

f hardcode HTLC timeout buffer

41d8c03

ln/refactor: remove claimable htlc from fail_htlc macro

aaa4b7f

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

c8cbe4f

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 remove comment

6089170

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

3f81b28

blinded_path/refactor: make construction generic over forwarding type

5b18620

To use helper functions for either trampoline or regular paths.

f seal ForwardTlvsInfo trait

0b82628

We only want to support trampoline and regular blinded paths, so we don't want to support implementation outside of this crate. We need the trait itself to be public as it's part of a public struct.

blinded_path: add constructor for trampoline blinded path

1b15a0e

ln/test: add multi-purpose trampoline test helper

380f744

To create trampoline forwarding and single hop receiving tails.

carlaKC force-pushed the 2299-mpp-prefactor branch from b2ac8f4 to 380f744 Compare April 10, 2026 18:45

carlaKC requested review from TheBlueMatt and valentinewallace April 10, 2026 18:49

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlaKC commented Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ldk-reviews-bot commented Mar 24, 2026 •

edited

Loading

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

edited

Loading