Batch MPP claims into single ChannelMonitorUpdate#4552
Batch MPP claims into single ChannelMonitorUpdate#4552Abeeujah wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
Claiming multiple MPP parts on the same channel was partially sequential, requiring claimee to claim the first part and wait for the peer to respond again before other parts can be claimed. This UX results in claim latency, time spent waiting on channel monitor updates, requiring a full round-trip (RAA/CS) for HTLC fulfillment. This change optimizes the process by batching these claims into a single update and a single commitment_signed message. - Introduce UpdateFulfillsCommitFetch enum and the get_update_fulfill_htlcs_and_commit method to Channel. - Update ChannelManager to group claimable HTLCs by counterparty and channel ID before delegation. - Refactor chanmon_update_fail_tests.rs and payment_tests.rs to align with the new atomic batching semantics. Tests has been updated to reflect this new batching of MPP claims - `test_single_channel_multiple_mpp` - `auto_retry_partial_failure` - `test_keysend_dup_hash_partial_mpp`
|
I've assigned @jkczyz as a reviewer! |
| // Register a completion action and RAA blocker for each | ||
| // successfully claimed (non-duplicate) HTLC. | ||
| for (idx, value_opt) in htlc_value_msat.iter().enumerate() { | ||
| let this_mpp_claim = | ||
| pending_mpp_claim_ptr_opt.as_ref().map(|pending_mpp_claim| { | ||
| let claim_ptr = | ||
| PendingMPPClaimPointer(Arc::clone(pending_mpp_claim)); | ||
| (counterparty_node_id, chan_id, claim_ptr) | ||
| }); | ||
| let raa_blocker = | ||
| pending_mpp_claim_ptr_opt.as_ref().map(|pending_claim| { | ||
| RAAMonitorUpdateBlockingAction::ClaimedMPPPayment { | ||
| pending_claim: PendingMPPClaimPointer(Arc::clone( | ||
| pending_claim, | ||
| )), | ||
| } | ||
| }); | ||
| let definitely_duplicate = value_opt.is_none(); | ||
| debug_assert!( | ||
| !definitely_duplicate || idx > 0, | ||
| "First HTLC in batch should not be a duplicate" | ||
| ); | ||
| if !definitely_duplicate { | ||
| let action = MonitorUpdateCompletionAction::PaymentClaimed { | ||
| payment_hash, | ||
| pending_mpp_claim: this_mpp_claim, | ||
| }; | ||
| log_trace!(logger, "Tracking monitor update completion action for batch claim: {:?}", action); | ||
| peer_state | ||
| .monitor_update_blocked_actions | ||
| .entry(chan_id) | ||
| .or_insert(Vec::new()) | ||
| .push(action); | ||
| } | ||
| if let Some(raa_blocker) = raa_blocker { | ||
| if !definitely_duplicate { | ||
| peer_state | ||
| .actions_blocking_raa_monitor_updates | ||
| .entry(chan_id) | ||
| .or_insert_with(Vec::new) | ||
| .push(raa_blocker); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: This loop registers one PaymentClaimed completion action and one ClaimedMPPPayment RAA blocker per non-duplicate HTLC, but they all map to a single combined ChannelMonitorUpdate. When that monitor update completes, handle_monitor_update_completion_actions processes all N actions simultaneously:
-
The first
PaymentClaimedaction'sretainloop finds all N RAA blockers (they share the samePendingMPPClaimPointerviaArc::ptr_eq). Each blocker is processed in sequence—after the first one transitions the channel inchannels_without_preimage → channels_with_preimage, the remaining N-1 blockers each re-checkchannels_without_preimage.is_empty()and push duplicates intofreed_channels. -
Result: N entries in
freed_channels, causinghandle_monitor_update_releaseto be called N times for the same channel. This is redundant in the happy path but could prematurely unblock other channels' blocked monitor updates in edge cases (multi-channel MPP where only this channel has batched parts). -
The remaining N-1
PaymentClaimedactions find no blockers (already drained by the first action) and nopending_claiming_paymentsentry (already removed by the first action), so they're no-ops—but they still churn through the completion handler.
Fix: Register exactly ONE PaymentClaimed action and ONE RAA blocker for the entire batch, matching the single ChannelMonitorUpdate. Something like:
// One action + one blocker for the entire batch
let this_mpp_claim = pending_mpp_claim_ptr_opt.as_ref().map(|pending_mpp_claim| {
(counterparty_node_id, chan_id, PendingMPPClaimPointer(Arc::clone(pending_mpp_claim)))
});
let raa_blocker = pending_mpp_claim_ptr_opt.as_ref().map(|pending_claim| {
RAAMonitorUpdateBlockingAction::ClaimedMPPPayment {
pending_claim: PendingMPPClaimPointer(Arc::clone(pending_claim)),
}
});
let action = MonitorUpdateCompletionAction::PaymentClaimed {
payment_hash,
pending_mpp_claim: this_mpp_claim,
};
peer_state.monitor_update_blocked_actions.entry(chan_id).or_insert(Vec::new()).push(action);
if let Some(raa_blocker) = raa_blocker {
peer_state.actions_blocking_raa_monitor_updates.entry(chan_id).or_insert_with(Vec::new).push(raa_blocker);
}| UpdateFulfillsCommitFetch::AllDuplicateClaims {} => { | ||
| debug_assert!(false, "We shouldn't claim duplicatively from a payment"); | ||
| }, |
There was a problem hiding this comment.
Bug: The AllDuplicateClaims case only fires a debug_assert but doesn't handle startup replay. Compare with the single-HTLC path in claim_mpp_part (the UpdateFulfillCommitFetch::DuplicateClaim branch, ~lines 9768-9860), which does significant work during startup:
- Re-registers RAA blockers that may not yet be present if the monitor update hadn't completed before shutdown (lines 9784-9789).
- Queues or immediately processes the
PaymentClaimedcompletion action so thePaymentClaimedevent can be generated during replay (lines 9791-9855).
During startup replay, all claims in a batch can be duplicates if the ChannelMonitorUpdate with the preimages had already been applied before shutdown but the PaymentClaimed event wasn't generated yet. In that scenario, this code path silently drops the completion action, potentially causing:
- The
PaymentClaimedevent to never be emitted. - RAA blockers to not be re-registered, allowing premature RAA processing.
| if combined_monitor_update.update_id == 0 { | ||
| combined_monitor_update.update_id = monitor_update.update_id; |
There was a problem hiding this comment.
Nit: Using update_id == 0 as a sentinel to detect the first NewClaim works because valid monitor update IDs start at 1, but this is an implicit invariant. Consider using an Option<u64> or a dedicated first_update_id variable to make this more robust and self-documenting.
| for (_, _, group) in grouped_sources { | ||
| if group.len() == 1 { | ||
| // Single HTLC on this channel, use existing path. | ||
| let htlc = group.into_iter().next().unwrap(); | ||
| let this_mpp_claim = pending_mpp_claim_ptr_opt.as_ref().map(|pending_mpp_claim| { | ||
| let counterparty_id = htlc.prev_hop.counterparty_node_id.expect("Prior to upgrading to LDK 0.1, all pending HTLCs forwarded by LDK 0.0.123 or before must be resolved. It appears at least one claimable payment was not resolved. Please downgrade to LDK 0.0.125 and resolve the HTLC by claiming the payment prior to upgrading."); | ||
| let claim_ptr = PendingMPPClaimPointer(Arc::clone(pending_mpp_claim)); | ||
| (counterparty_id, htlc.prev_hop.channel_id, claim_ptr) | ||
| }); | ||
| let raa_blocker = pending_mpp_claim_ptr_opt.as_ref().map(|pending_claim| { | ||
| RAAMonitorUpdateBlockingAction::ClaimedMPPPayment { | ||
| pending_claim: PendingMPPClaimPointer(Arc::clone(pending_claim)), | ||
| } | ||
| }); | ||
| let raa_blocker = pending_mpp_claim_ptr_opt.as_ref().map(|pending_claim| { | ||
| RAAMonitorUpdateBlockingAction::ClaimedMPPPayment { | ||
| pending_claim: PendingMPPClaimPointer(Arc::clone(pending_claim)), | ||
| } | ||
| }); | ||
|
|
||
| // Create new attribution data as the final hop. Always report a zero hold time, because reporting a | ||
| // non-zero value will not make a difference in the penalty that may be applied by the sender. If there | ||
| // is a phantom hop, we need to double-process. | ||
| let attribution_data = | ||
| if let Some(phantom_secret) = htlc.prev_hop.phantom_shared_secret { | ||
| let attribution_data = | ||
| process_fulfill_attribution_data(None, &phantom_secret, 0); | ||
| Some(attribution_data) | ||
| } else { | ||
| None | ||
| }; | ||
| let attribution_data = | ||
| if let Some(phantom_secret) = htlc.prev_hop.phantom_shared_secret { | ||
| let attribution_data = | ||
| process_fulfill_attribution_data(None, &phantom_secret, 0); | ||
| Some(attribution_data) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let attribution_data = process_fulfill_attribution_data( | ||
| attribution_data, | ||
| &htlc.prev_hop.incoming_packet_shared_secret, | ||
| 0, | ||
| ); | ||
| let attribution_data = process_fulfill_attribution_data( | ||
| attribution_data, | ||
| &htlc.prev_hop.incoming_packet_shared_secret, | ||
| 0, | ||
| ); | ||
|
|
||
| self.claim_funds_from_hop( | ||
| htlc.prev_hop, | ||
| payment_preimage, | ||
| payment_info.clone(), | ||
| Some(attribution_data), | ||
| |_, definitely_duplicate| { | ||
| debug_assert!( | ||
| !definitely_duplicate, | ||
| "We shouldn't claim duplicatively from a payment" | ||
| ); | ||
| ( | ||
| Some(MonitorUpdateCompletionAction::PaymentClaimed { | ||
| payment_hash, | ||
| pending_mpp_claim: this_mpp_claim, | ||
| }), | ||
| raa_blocker, | ||
| ) | ||
| }, | ||
| ); | ||
| self.claim_funds_from_hop( | ||
| htlc.prev_hop, | ||
| payment_preimage, | ||
| payment_info.clone(), | ||
| Some(attribution_data), | ||
| |_, definitely_duplicate| { | ||
| debug_assert!( | ||
| !definitely_duplicate, | ||
| "We shouldn't claim duplicatively from a payment" | ||
| ); | ||
| ( | ||
| Some(MonitorUpdateCompletionAction::PaymentClaimed { | ||
| payment_hash, | ||
| pending_mpp_claim: this_mpp_claim, | ||
| }), | ||
| raa_blocker, | ||
| ) | ||
| }, | ||
| ); | ||
| } else { | ||
| // Multiple HTLCs on the same channel, batch into a single commitment. | ||
| self.claim_batch_funds_from_channel( | ||
| group, | ||
| payment_preimage, | ||
| payment_hash, | ||
| payment_info.clone(), | ||
| &pending_mpp_claim_ptr_opt, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The grouping logic uses group.len() == 1 to decide between the single-HTLC path and the batch path. For a single-source payment (sources.len() == 1), pending_mpp_claim_ptr_opt is None (set at line 9407). In that case the single HTLC takes the group.len() == 1 branch, which is correct.
However, for a multi-source MPP where each source happens to be on a different channel (no channel has >1 HTLC), every group has len==1, so ALL HTLCs go through the single-HTLC path. This means the batch path is only exercised when ≥2 parts of the same MPP arrive on the same channel—which is likely the intent, but worth documenting explicitly in a comment for future readers, since the grouping loop above suggests it handles all sources.
| } else { | ||
| let blocked_upd = self.context.blocked_monitor_updates.get(0); | ||
| let new_mon_id = blocked_upd | ||
| .map(|upd| upd.update.update_id) | ||
| .unwrap_or(combined_monitor_update.update_id); | ||
| combined_monitor_update.update_id = new_mon_id; | ||
| for held_update in self.context.blocked_monitor_updates.iter_mut() { | ||
| held_update.update.update_id += 1; | ||
| } | ||
|
|
||
| // Reset latest_monitor_update_id before building a new commitment so its ID is consecutive. | ||
| self.context.latest_monitor_update_id = self | ||
| .context | ||
| .blocked_monitor_updates | ||
| .last() | ||
| .map(|upd| upd.update.update_id) | ||
| .unwrap_or(combined_monitor_update.update_id); | ||
|
|
||
| if any_not_blocked { | ||
| debug_assert!(false, "If there is a pending blocked monitor we should have MonitorUpdateInProgress set"); | ||
| let update = self.build_commitment_no_status_check(logger); | ||
| self.context.blocked_monitor_updates.push(PendingChannelMonitorUpdate { update }); | ||
| } |
There was a problem hiding this comment.
The else branch handles two distinct cases conflated together:
!release_cs_monitor(blocked monitor updates exist) — needs to shift IDsrelease_cs_monitor && !any_not_blocked(no blocked updates, but all claims were put in holding cell) — no shifts needed
For case 2, blocked_monitor_updates is empty, so the for-loop and the last() call are no-ops, and latest_monitor_update_id is set to combined_monitor_update.update_id. This works correctly but is non-obvious. A comment or separate handling would make the two cases clearer.
Also: self.context.blocked_monitor_updates.get(0) should use .first() per idiomatic Rust.
| nodes[7].node.peer_disconnected(nodes[2].node.get_our_node_id()); | ||
| nodes[7].node.peer_disconnected(nodes[3].node.get_our_node_id()); | ||
| nodes[7].node.peer_disconnected(nodes[4].node.get_our_node_id()); | ||
| nodes[7].node.peer_disconnected(nodes[5].node.get_our_node_id()); |
There was a problem hiding this comment.
After delivering all 6 update_fulfill_htlc messages, the test asserts 6 PaymentForwarded events but doesn't validate their contents (e.g. which upstream channel each forward corresponds to, or fee amounts). The old test checked expect_payment_forwarded! with specific source nodes and fee values (Some(1000)). Consider adding at least a fee check to ensure the batch claim correctly reports forwarding fees.
Review SummaryThis PR introduces batching of MPP claims on the same channel into a single Bugs
Minor Issues
|
3 participants
Claiming multiple MPP parts on the same channel was partially sequential, requiring claimee to claim the first part and wait for the peer to respond again before other parts can be claimed. This UX results in claim latency, time spent waiting on channel monitor updates, requiring a full round-trip (RAA/CS) for HTLC fulfillment.
This change optimizes the process by batching these claims into a single update and a single commitment_signed message.
UpdateFulfillsCommitFetchenum and theget_update_fulfill_htlcs_and_commitmethod toChannel.ChannelManager to group claimable HTLCs by counterparty and channel ID before delegation.Tests has been updated to reflect this new batching of MPP claims
test_single_channel_multiple_mppauto_retry_partial_failuretest_keysend_dup_hash_partial_mppcloses #3986