ln/refactor: pass closure to create PaymentForwarded event
What the developer wrote
Authored by Carla Kirk-Cohen
ln/refactor: pass closure to create PaymentForwarded event
When we introduce trampoline forwards, we're going to want to provide
two external pieces of information to create events:
- When to emit an event: we only want to emit one trampoline event, even
when we have multiple incoming htlcs. We need to make multiple calls
to claim_funds_from_htlc_forward_hop to claim each individual htlc,
which are not aware of each other, so we rely on the caller's closure
to decide when to emit Some or None.
- Forwarding fees: we will not be able to calculate the total fee for
a trampoline forward when an individual outgoing htlcs is fulfilled,
because there may be other outgoing htlcs that are not accounted for
(we only get the htlc_claim_value_msat for the single htlc that was
just fulfilled). In future, we'll be able to provide the total fee
from the channelmanager's top level view.
What changed, and why it matters
This commit is a code cleanup (refactor) inside the Lightning Dev Kit's channel manager. It moves the creation of a 'PaymentForwarded' event out of an internal helper function and into the caller by passing a closure. The actual logic for normal forwards is preserved unchanged; the change is preparing the code for future 'trampoline' forwarding support. There is no indication this fixes a security bug or introduces a vulnerability.
No security action required. Treat as ordinary refactoring review; verify trampoline-forward follow-up commits for correct event emission and fee accounting.
Security signals we found
No security-relevant behavioral change in existing forwarding path
Fee calculation and debug assertion moved, not modified
Refactor explicitly described as preparation for trampoline forwards
No bounds checks, cryptographic operations, or permission changes introduced
Evidence from the diff
The patch refactors claim_funds_from_htlc_forward_hop in lightning/src/ln/channelmanager.rs to accept a closure make_payment_forwarded_event instead of constructing the Event::PaymentForwarded inline. It also adds a From<&HTLCPreviousHopData> impl for events::HTLCLocator. For the existing non-trampoline call site, the closure reproduces the prior event construction exactly, including the same fee calculation and debug assertion. The commit message frames this as groundwork for trampoline forwards, where event emission timing and total fee calculation must be controlled by the caller.
Changed components
lightning/src/ln/channelmanager.rsclaim_funds_from_htlc_forward_hopEvent::PaymentForwarded constructionInspect captured patch +57 / −45
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 9039373..d8661f4 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -879,6 +879,16 @@ mod fuzzy_channelmanager {
/// channel remains unconfirmed for too long.
pub cltv_expiry: Option<u32>,
}
+
+ impl From<&HTLCPreviousHopData> for events::HTLCLocator {
+ fn from(value: &HTLCPreviousHopData) -> Self {
+ events::HTLCLocator {
+ channel_id: value.channel_id,
+ user_channel_id: value.user_channel_id,
+ node_id: value.counterparty_node_id,
+ }
+ }
+ }
}
#[cfg(fuzzing)]
pub use self::fuzzy_channelmanager::*;
@@ -9324,18 +9334,16 @@ impl<
/// Claims funds for a forwarded HTLC where we are an intermediate hop.
///
/// Processes attribution data, calculates fees earned, and emits a [`Event::PaymentForwarded`]
- /// event upon successful claim.
+ /// event upon successful claim. `make_payment_forwarded_event` is responsible for creating a
+ /// single [`Event::PaymentForwarded`] event that represents the forward.
fn claim_funds_from_htlc_forward_hop(
- &self, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>,
- skimmed_fee_msat: Option<u64>, from_onchain: bool, startup_replay: bool,
- next_channel_counterparty_node_id: PublicKey, next_channel_outpoint: OutPoint,
- next_channel_id: ChannelId, next_user_channel_id: Option<u128>,
- hop_data: HTLCPreviousHopData, attribution_data: Option<AttributionData>,
- send_timestamp: Option<Duration>,
+ &self, payment_preimage: PaymentPreimage,
+ make_payment_forwarded_event: impl FnOnce(Option<u64>) -> Option<events::Event>,
+ startup_replay: bool, next_channel_counterparty_node_id: PublicKey,
+ next_channel_outpoint: OutPoint, next_channel_id: ChannelId, hop_data: HTLCPreviousHopData,
+ attribution_data: Option<AttributionData>, send_timestamp: Option<Duration>,
) {
- let prev_channel_id = hop_data.channel_id;
- let prev_user_channel_id = hop_data.user_channel_id;
- let prev_node_id = hop_data.counterparty_node_id;
+ let _prev_channel_id = hop_data.channel_id;
let completed_blocker = RAAMonitorUpdateBlockingAction::from_prev_hop_data(&hop_data);
// Obtain hold time, if available.
@@ -9411,7 +9419,7 @@ impl<
// immediately once we get going.
BackgroundEvent::MonitorUpdatesComplete {
channel_id, ..
- } => *channel_id == prev_channel_id,
+ } => *channel_id == _prev_channel_id,
}
});
assert!(channel_closed || matching_bg_event, "{:?}", *background_events);
@@ -9427,38 +9435,16 @@ impl<
None,
)
} else {
- let total_fee_earned_msat =
- if let Some(forwarded_htlc_value) = forwarded_htlc_value_msat {
- if let Some(claimed_htlc_value) = htlc_claim_value_msat {
- Some(claimed_htlc_value - forwarded_htlc_value)
- } else {
- None
- }
- } else {
- None
- };
- debug_assert!(
- skimmed_fee_msat <= total_fee_earned_msat,
- "skimmed_fee_msat must always be included in total_fee_earned_msat"
- );
+ let event = make_payment_forwarded_event(htlc_claim_value_msat);
+ if let Some(ref payment_forwarded) = event {
+ debug_assert!(matches!(
+ payment_forwarded,
+ &events::Event::PaymentForwarded { .. }
+ ));
+ }
(
Some(MonitorUpdateCompletionAction::EmitEventOptionAndFreeOtherChannel {
- event: Some(events::Event::PaymentForwarded {
- prev_htlcs: vec![events::HTLCLocator {
- channel_id: prev_channel_id,
- user_channel_id: prev_user_channel_id,
- node_id: prev_node_id,
- }],
- next_htlcs: vec![events::HTLCLocator {
- channel_id: next_channel_id,
- user_channel_id: next_user_channel_id,
- node_id: Some(next_channel_counterparty_node_id),
- }],
- total_fee_earned_msat,
- skimmed_fee_msat,
- claim_from_onchain_tx: from_onchain,
- outbound_amount_forwarded_msat: forwarded_htlc_value_msat,
- }),
+ event,
downstream_counterparty_and_funding_outpoint: chan_to_release,
}),
None,
@@ -9863,16 +9849,42 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
}
},
HTLCSource::PreviousHopData(hop_data) => {
+ let prev_htlcs = vec![events::HTLCLocator::from(&hop_data)];
self.claim_funds_from_htlc_forward_hop(
payment_preimage,
- forwarded_htlc_value_msat,
- skimmed_fee_msat,
- from_onchain,
+ |htlc_claim_value_msat: Option<u64>| -> Option<events::Event> {
+ let total_fee_earned_msat =
+ if let Some(forwarded_htlc_value) = forwarded_htlc_value_msat {
+ if let Some(claimed_htlc_value) = htlc_claim_value_msat {
+ Some(claimed_htlc_value - forwarded_htlc_value)
+ } else {
+ None
+ }
+ } else {
+ None
+ };
+ debug_assert!(
+ skimmed_fee_msat <= total_fee_earned_msat,
+ "skimmed_fee_msat must always be included in total_fee_earned_msat"
+ );
+
+ Some(events::Event::PaymentForwarded {
+ prev_htlcs,
+ next_htlcs: vec![events::HTLCLocator {
+ channel_id: next_channel_id,
+ user_channel_id: next_user_channel_id,
+ node_id: Some(next_channel_counterparty_node_id),
+ }],
+ total_fee_earned_msat,
+ skimmed_fee_msat,
+ claim_from_onchain_tx: from_onchain,
+ outbound_amount_forwarded_msat: forwarded_htlc_value_msat,
+ })
+ },
startup_replay,
next_channel_counterparty_node_id,
next_channel_outpoint,
next_channel_id,
- next_user_channel_id,
hop_data,
attribution_data,
send_timestamp,
Why this scored 16/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.