Batch MPP claims into single ChannelMonitorUpdate#4552
Batch MPP claims into single ChannelMonitorUpdate#4552Abeeujah wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
|
👋 I see @jkczyz was un-assigned. |
| // 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 SummaryPrior Critical Issue: RESOLVEDThe Prior Issues Status
Inline Comments Posted (this review)
No new bugs or security issues found.The implementation correctly handles the main code paths: immediate flush, deferred flush (can't generate commitment), all-duplicates (startup replay), and closed-channel fallback. The |
Abeeujah
force-pushed
the
parallelize-mppclaims
branch
from
395b30d to
88d62c4
Compare
| // Register a completion action and RAA blocker for each | ||
| // successfully claimed (non-duplicate) HTLC. |
There was a problem hiding this comment.
Nit: This comment says "for each successfully claimed (non-duplicate) HTLC" but the code registers exactly one completion action and one RAA blocker for the entire batch (i.e., per combined ChannelMonitorUpdate). This is actually the correct behavior — one action per monitor update — but the comment is misleading and could confuse future readers.
| // Register a completion action and RAA blocker for each | |
| // successfully claimed (non-duplicate) HTLC. | |
| // Register a single completion action and RAA blocker for the | |
| // combined ChannelMonitorUpdate covering all claims in this batch. |
| let mut additional_update = self.build_commitment_no_status_check(logger); | ||
| // build_commitment_no_status_check may bump latest_monitor_id but we want them | ||
| // to be strictly increasing by one, so reset it here. | ||
| self.context.latest_monitor_update_id = combined_monitor_update.update_id; |
There was a problem hiding this comment.
Nit: This comment was copied from the single-HTLC version and only mentions build_commitment_no_status_check, but in the batch case the N individual get_update_fulfill_htlc calls also bumped latest_monitor_update_id. The reset here discards N+1 ID increments (N preimage bumps + 1 commitment bump), not just the commitment bump. Consider updating the comment for accuracy:
| let mut additional_update = self.build_commitment_no_status_check(logger); | |
| // build_commitment_no_status_check may bump latest_monitor_id but we want them | |
| // to be strictly increasing by one, so reset it here. | |
| self.context.latest_monitor_update_id = combined_monitor_update.update_id; | |
| // The N calls to get_update_fulfill_htlc and build_commitment_no_status_check | |
| // each bumped latest_monitor_update_id, but we merged everything into a single | |
| // ChannelMonitorUpdate with the first ID, so reset to that ID. | |
| self.context.latest_monitor_update_id = combined_monitor_update.update_id; |
| // Channel is closed, fall back to per-HTLC claiming against the closed channel monitor. | ||
| mem::drop(peer_state_lock); | ||
| mem::drop(per_peer_state); | ||
| for htlc in htlcs { | ||
| 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, 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 attribution_data = if let Some(phantom_secret) = htlc.prev_hop.phantom_shared_secret | ||
| { | ||
| Some(process_fulfill_attribution_data(None, &phantom_secret, 0)) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| 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, | ||
| ) | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
This closed-channel fallback calls claim_funds_from_hop per HTLC, which registers a separate PaymentClaimed action + ClaimedMPPPayment RAA blocker for each HTLC. Since all N HTLCs are on the same channel, PendingMPPClaim.channels_without_preimage has only ONE entry for this channel. When the first monitor update completes, its PaymentClaimed action's retain loop in handle_monitor_update_completion_actions processes ALL N blockers (they share the same PendingMPPClaimPointer via Arc::ptr_eq). The first blocker empties channels_without_preimage, then all N blockers push duplicate entries to freed_channels (since is_empty() is true for each).
This causes handle_monitor_update_release to be called N times redundantly for the same channel. This is a pre-existing behavior (the original per-HTLC path had the same issue), but since you're explicitly handling the batch case for open channels, you could apply the same "single action + single blocker" pattern here for consistency and correctness:
// Channel is closed. Register one combined action + blocker, then claim each HTLC individually.
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)),
}
});
// claim_funds_from_hop for each, but only the first carries the action/blockerNot blocking, but worth considering for a follow-up.
Abeeujah
force-pushed
the
parallelize-mppclaims
branch
from
88d62c4 to
5880633
Compare
|
|
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
Abeeujah
force-pushed
the
parallelize-mppclaims
branch
from
5880633 to
d801ec9
Compare
It's intentionally left unused right now, pending any design decision that would be made for the msat values that field holds, in the case of a single claim, there's a
This is why that field exist, the absense of the direction it should take is the reason it is intentionally currently left unused. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4552 +/- ##
==========================================
- Coverage 87.11% 87.02% -0.10%
==========================================
Files 161 161
Lines 109246 109527 +281
Branches 109246 109527 +281
==========================================
+ Hits 95173 95316 +143
- Misses 11592 11723 +131
- Partials 2481 2488 +7
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:
|
Makes sense, but could you drop a quick comment in the code explaining this? Anyone reading it later will just see |
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Hmm, would this not be simpler by using the holding cell instead? When we're failing, we call queue_fail_htlc which just shoves the failure into the holding cell then call check_free_peer_holding_cells to release them all at once. Rather than rebuilding that logic for claims, we should be able to do something similar. We'd have to handle the monitor updates with the preimages a bit differently, but not drastically so.
Abeeujah
force-pushed
the
parallelize-mppclaims
branch
from
d801ec9 to
5992824
Compare
| } else { | ||
| // Deferred flush: build a preimage-only ChannelMonitorUpdate so the | ||
| // preimage is durable now. The commitment_signed will be sent later | ||
| // when the holding cell is naturally flushed. | ||
| chan.build_preimage_only_monitor_update(payment_preimage, payment_info) | ||
| }; |
There was a problem hiding this comment.
Bug: Panic in production — missing monitor_updating_paused call in the deferred-flush path.
When maybe_free_holding_cell_htlcs returns None (channel can't generate a new commitment due to AWAITING_REMOTE_REVOKE, PEER_DISCONNECTED, LOCAL_STFU_SENT, or QUIESCENT), build_preimage_only_monitor_update creates the update but does NOT call chan.monitor_updating_paused(...), so MONITOR_UPDATE_IN_PROGRESS is never set on the channel.
The subsequent handle_new_monitor_update (line 10297) submits the update to the chain monitor. If persistence is synchronous (common in simple setups) and there are no other in-flight updates for this channel, all_updates_complete becomes true, triggering try_resume_channel_post_monitor_update → monitor_updating_restored, which hits a hard assert! at channel.rs:9839:
assert!(self.context.channel_state.is_monitor_update_in_progress()); // PANICSCompare with the single-HTLC path in get_update_fulfill_htlc_and_commit (line 7690) which always calls monitor_updating_paused(false, !update_blocked, false, ...) regardless of whether a commitment could be generated, ensuring MONITOR_UPDATE_IN_PROGRESS is set before the update is submitted.
Fix: After build_preimage_only_monitor_update, call:
chan.monitor_updating_paused(false, false, false, Vec::new(), Vec::new(), Vec::new(), &&logger);Reproduction scenario: Node receives an MPP payment with ≥2 parts on the same channel. Before calling claim_funds, the channel enters AWAITING_REMOTE_REVOKE (e.g., a prior commitment exchange just completed with the peer's commitment_signed but the peer hasn't sent revoke_and_ack yet). claim_funds takes the batch path, can_generate_new_commitment() returns false, deferred flush path is taken, and the preimage-only update is submitted. With synchronous persistence → panic.
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`
Abeeujah
force-pushed
the
parallelize-mppclaims
branch
from
5992824 to
f466457
Compare
| // 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.mpp_part.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."); |
There was a problem hiding this comment.
Nit: The counterparty_node_id expect message is duplicated from line 9604 (where it was already unwrapped for grouping). Since the grouping loop above already unwraps counterparty_node_id for every HTLC, by the time we reach the group.len() == 1 branch we know it's Some. The second expect at line 9621 is redundant — you could just reuse the counterparty_id from the group tuple.
Note: the same applies to claim_batch_funds_from_channel at line 10139 — the counterparty_node_id was already validated during grouping.
| let first_mpp_claim = this_mpp_claim.take(); | ||
| let first_raa_blocker = raa_blocker.take(); |
There was a problem hiding this comment.
The closed-channel fallback uses .take() so only the first HTLC in the batch gets the MonitorUpdateCompletionAction and RAA blocker (2nd+ get (None, None)). This is functionally correct — all HTLCs share the same channel, so one action + one blocker is sufficient — but it means each subsequent HTLC creates and persists a ChannelMonitorUpdate with no associated completion action.
Consider adding a brief comment explaining the .take() intent: "Only the first HTLC in the batch carries the completion action and RAA blocker; subsequent HTLCs only persist the preimage for safety."
| // Inject payment_info into the first PaymentPreimage step that | ||
| // matches our preimage (the holding cell may contain unrelated | ||
| // claims for other preimages from prior queueing). | ||
| for step in monitor_update.updates.iter_mut() { |
There was a problem hiding this comment.
This payment_info injection only fills the first matching PaymentPreimage step in the combined monitor update (then breaks). When there are N HTLCs in the batch, there are N PaymentPreimage steps for the same preimage, and only the first gets payment_info. This is correct because ChannelMonitor::provide_payment_preimage uses and_modify().or_insert_with() and will store payment_info from the first step, while subsequent steps with None are no-ops.
Worth a brief comment explaining this, since a reader might expect all N steps to carry payment_info:
| // Inject payment_info into the first PaymentPreimage step that | |
| // matches our preimage (the holding cell may contain unrelated | |
| // claims for other preimages from prior queueing). | |
| for step in monitor_update.updates.iter_mut() { | |
| // Inject payment_info into the first PaymentPreimage step that | |
| // matches our preimage. Only one step needs it — the ChannelMonitor | |
| // stores payment_info per payment_hash (not per HTLC), so subsequent | |
| // preimage steps with None are idempotent no-ops. | |
| for step in monitor_update.updates.iter_mut() { |
| &self | ||
| .context | ||
| .pending_inbound_htlcs | ||
| .iter() | ||
| .find(|h| h.htlc_id == htlc_id_arg) | ||
| .map(|h| h.payment_hash) | ||
| .unwrap_or(PaymentHash([0u8; 32])), |
There was a problem hiding this comment.
Nit: Redundant HTLC lookup — the htlc variable from the find at line 7730 already holds the matching HTLC, but this log statement does a second find to get the payment_hash. You can just use htlc.payment_hash from the earlier binding.
(The earlier htlc borrow does end at line 7763, but you could restructure to extract the payment_hash before the holding-cell loop, or restructure the scoping slightly.)
5 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