ln/refactor: move mpp timeout into helper function
What the developer wrote
Authored by Carla Kirk-Cohen
ln/refactor: move mpp timeout into helper function
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.
What changed, and why it matters
This is a code cleanup (refactor) that moves the multi-part payment (MPP) timeout logic into a reusable helper function. The commit message notes a tiny behavior change in when timeout ticks are counted, but explicitly states this makes no practical difference because fully received payments are never timed out anyway. There is no indication of a security fix or vulnerability.
No security action needed. Treat as ordinary refactoring. Reviewers may want to verify the author's claim that the tick-order change is harmless, but the commit message itself explains the equivalence.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The change extracts MPP timeout handling from a closure in channelmanager.rs into a new check_mpp_timeout helper. Previously the code first summed received values and returned early if the MPP total was met, without incrementing any timer ticks. Now it increments timer ticks while summing and only returns false if the total is met. The commit author asserts this is behaviorally equivalent because fully accumulated MPP payments are never timed out. The patch also updates the timeout failure path to use HTLCSource::PreviousHopData and HTLCHandlingFailureType::Receive directly, preparing for future trampoline HTLC timeout handling.
Changed components
lightning/src/ln/channelmanager.rsInspect captured patch +46 / −24
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index dd91f3c..26eb42d 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -1250,6 +1250,31 @@ impl ClaimablePayment {
}
}
+/// Increments MPP timeout tick for all HTLCs and returns a boolean indicating whether the HTLC
+/// set has hit its MPP timeout. Will return false if the set has reached the sender's intended
+/// total, as the MPP has completed in this case.
+fn check_mpp_timeout<'a>(
+ htlcs: impl Iterator<Item = &'a mut MppPart>, onion_fields: &RecipientOnionFields,
+) -> bool {
+ // This condition determining whether the MPP is complete here must match exactly the condition
+ // used in `process_pending_htlc_forwards`.
+ let total_mpp_value = onion_fields.total_mpp_amount_msat;
+ let mut total_intended_recvd_value = 0;
+ let mut timed_out = false;
+ for htlc in htlcs {
+ total_intended_recvd_value += htlc.sender_intended_value;
+ htlc.timer_ticks += 1;
+ if htlc.timer_ticks >= MPP_TIMEOUT_TICKS {
+ timed_out = true;
+ }
+ }
+ if total_intended_recvd_value >= total_mpp_value {
+ return false;
+ }
+
+ timed_out
+}
+
/// Represent the channel funding transaction type.
enum FundingType {
/// This variant is useful when we want LDK to validate the funding transaction and
@@ -8909,39 +8934,36 @@ impl<
self.claimable_payments.lock().unwrap().claimable_payments.retain(
|payment_hash, payment| {
if payment.htlcs.is_empty() {
- // This should be unreachable
debug_assert!(false);
return false;
}
- // Check if we've received all the parts we need for an MPP.
- // This condition determining whether the MPP is complete here must match
- // exactly the condition used in `process_pending_htlc_forwards`.
- let total_intended_recvd_value =
- payment.htlcs.iter().map(|h| h.mpp_part.sender_intended_value).sum();
- let total_mpp_value = payment.onion_fields.total_mpp_amount_msat;
- if total_mpp_value <= total_intended_recvd_value {
- return true;
- } else if payment.htlcs.iter_mut().any(|htlc| {
- htlc.mpp_part.timer_ticks += 1;
- return htlc.mpp_part.timer_ticks >= MPP_TIMEOUT_TICKS;
- }) {
- let htlcs = payment
- .htlcs
- .drain(..)
- .map(|htlc: ClaimableHTLC| (htlc.mpp_part.prev_hop, *payment_hash));
- timed_out_mpp_htlcs.extend(htlcs);
- return false;
+ let mpp_timeout = check_mpp_timeout(
+ payment.htlcs.iter_mut().map(|htlc| &mut htlc.mpp_part),
+ &payment.onion_fields,
+ );
+ if mpp_timeout {
+ timed_out_mpp_htlcs.extend(payment.htlcs.drain(..).map(|h| {
+ (
+ HTLCSource::PreviousHopData(h.mpp_part.prev_hop),
+ *payment_hash,
+ HTLCHandlingFailureType::Receive { payment_hash: *payment_hash },
+ )
+ }));
}
- true
+ return !mpp_timeout;
},
);
- for htlc_source in timed_out_mpp_htlcs.drain(..) {
- let source = HTLCSource::PreviousHopData(htlc_source.0.clone());
+ for (htlc_source, payment_hash, failure_type) in timed_out_mpp_htlcs.drain(..) {
let failure_reason = LocalHTLCFailureReason::MPPTimeout;
let reason = HTLCFailReason::from_failure_code(failure_reason);
- let receiver = HTLCHandlingFailureType::Receive { payment_hash: htlc_source.1 };
- self.fail_htlc_backwards_internal(&source, &htlc_source.1, &reason, receiver, None);
+ self.fail_htlc_backwards_internal(
+ &htlc_source,
+ &payment_hash,
+ &reason,
+ failure_type,
+ None,
+ );
}
for (err, counterparty_node_id) in handle_errors {
Why this scored 12/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.