← Watch feed
Pass constructed `PendingAddHTLCInfo` to chanman `forward_htlcs`
Public commit record
85/100 · Strong
What the developer wrote
Authored by Matt Corallo
Pass constructed `PendingAddHTLCInfo` to chanman `forward_htlcs`
We jump through some hoops in order to pass a small list of objects
to `forward_htlcs` on a per-channel basis rather than per-HTLC.
Then, `forward_htlcs` builds a `PendingAddHTLCInfo` for each HTLC
for insertion. Worse, in some `forward_htlcs` callsites we're
actually starting with a `PendingAddHTLCInfo`, converting it to a
tuple, then back inside `forward_htlcs`.
Instead, here we just pass a list of built `PendingAddHTLCInfo`s to
`forward_htlcs`, cleaning up a good bit of code and even avoiding
an allocation of the HTLCs vec in many cases.
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Explains rationale or failure mode
What changed, and why it matters
This commit is a straightforward internal code cleanup in the Lightning Dev Kit's channel manager. It changes how pending HTLC (Hashed Time-Locked Contract) forwarding information is passed between functions: instead of converting data into tuples and then back into a structured object, the structured object is passed directly. The commit message and diff show no intent to fix a security issue, and the behavior appears functionally equivalent.
Recommended action
No security action required. Treat as a normal refactoring/code-quality change.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The patch refactors forward_htlcs and related call sites to accept an iterator of PendingAddHTLCInfo structs directly, rather than a per-source tuple (u64, PublicKey, OutPoint, ChannelId, u128, Vec<(PendingHTLCInfo, u64)>) that forward_htlcs previously destructured and rebuilt into PendingAddHTLCInfo. This removes the PerSourcePendingForward type alias, eliminates intermediate tuple allocations, and updates post_monitor_update_unlock and channel resumption paths to use Vec<PendingAddHTLCInfo> instead of Option<PerSourcePendingForward>. The logic of grouping HTLCs by short channel ID and inserting them into self.forward_htlcs is preserved.
Changed components
lightning/src/ln/channelmanager.rsInspect captured patch +57 / −96
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index eae26cc..9d32d4f 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -449,7 +449,7 @@ pub(super) enum PendingHTLCStatus {
pub(super) struct PendingAddHTLCInfo {
pub(super) forward_info: PendingHTLCInfo,
- // These fields are produced in `forward_htlcs()` and consumed in
+ // These fields are set before calling `forward_htlcs()` and consumed in
// `process_pending_htlc_forwards()` for constructing the
// `HTLCSource::PreviousHopData` for failed and forwarded
// HTLCs.
@@ -766,10 +766,6 @@ impl_writeable_tlv_based_enum!(SentHTLCId,
},
);
-// (src_outbound_scid_alias, src_counterparty_node_id, src_funding_outpoint, src_chan_id, src_user_chan_id)
-type PerSourcePendingForward =
- (u64, PublicKey, OutPoint, ChannelId, u128, Vec<(PendingHTLCInfo, u64)>);
-
type FailedHTLCForward = (HTLCSource, PaymentHash, HTLCFailReason, HTLCHandlingFailureType);
mod fuzzy_channelmanager {
@@ -1421,7 +1417,7 @@ enum PostMonitorUpdateChanResume {
user_channel_id: u128,
unbroadcasted_batch_funding_txid: Option<Txid>,
update_actions: Vec<MonitorUpdateCompletionAction>,
- htlc_forwards: Option<PerSourcePendingForward>,
+ htlc_forwards: Vec<PendingAddHTLCInfo>,
decode_update_add_htlcs: Option<(u64, Vec<msgs::UpdateAddHTLC>)>,
finalized_claimed_htlcs: Vec<(HTLCSource, Option<AttributionData>)>,
failed_htlcs: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>,
@@ -6798,15 +6794,16 @@ impl<
..payment.forward_info
};
- let mut per_source_pending_forward = [(
- payment.prev_outbound_scid_alias,
- payment.prev_counterparty_node_id,
- payment.prev_funding_outpoint,
- payment.prev_channel_id,
- payment.prev_user_channel_id,
- vec![(pending_htlc_info, payment.prev_htlc_id)],
- )];
- self.forward_htlcs(&mut per_source_pending_forward);
+ let forward = [PendingAddHTLCInfo {
+ prev_outbound_scid_alias: payment.prev_outbound_scid_alias,
+ prev_htlc_id: payment.prev_htlc_id,
+ prev_counterparty_node_id: payment.prev_counterparty_node_id,
+ prev_channel_id: payment.prev_channel_id,
+ prev_funding_outpoint: payment.prev_funding_outpoint,
+ prev_user_channel_id: payment.prev_user_channel_id,
+ forward_info: pending_htlc_info,
+ }];
+ self.forward_htlcs(forward);
Ok(())
}
@@ -7037,7 +7034,7 @@ impl<
next_packet_details_opt.map(|d| d.next_packet_pubkey),
) {
Ok(info) => {
- let to_pending_add = |info| PendingAddHTLCInfo {
+ let pending_add = PendingAddHTLCInfo {
prev_outbound_scid_alias: incoming_scid_alias,
prev_counterparty_node_id: incoming_counterparty_node_id,
prev_funding_outpoint: incoming_funding_txo,
@@ -7059,7 +7056,7 @@ impl<
Some(incoming_channel_id),
Some(update_add_htlc.payment_hash),
);
- if info.routing.should_hold_htlc() {
+ if pending_add.forward_info.routing.should_hold_htlc() {
let mut held_htlcs = self.pending_intercepted_htlcs.lock().unwrap();
let intercept_id = intercept_id();
match held_htlcs.entry(intercept_id) {
@@ -7068,7 +7065,6 @@ impl<
logger,
"Intercepted held HTLC with id {intercept_id}, holding until the recipient is online"
);
- let pending_add = to_pending_add(info);
entry.insert(pending_add);
},
hash_map::Entry::Occupied(_) => {
@@ -7085,7 +7081,6 @@ impl<
self.pending_intercepted_htlcs.lock().unwrap();
match pending_intercepts.entry(intercept_id) {
hash_map::Entry::Vacant(entry) => {
- let pending_add = to_pending_add(info);
if let Ok(intercept_ev) =
create_htlc_intercepted_event(intercept_id, &pending_add)
{
@@ -7125,7 +7120,7 @@ impl<
},
}
} else {
- htlc_forwards.push((info, update_add_htlc.htlc_id))
+ htlc_forwards.push(pending_add);
}
},
Err(inbound_err) => {
@@ -7145,15 +7140,7 @@ impl<
// Process all of the forwards and failures for the channel in which the HTLCs were
// proposed to as a batch.
- let pending_forwards = (
- incoming_scid_alias,
- incoming_counterparty_node_id,
- incoming_funding_txo,
- incoming_channel_id,
- incoming_user_channel_id,
- htlc_forwards,
- );
- self.forward_htlcs(&mut [pending_forwards]);
+ self.forward_htlcs(htlc_forwards);
for (htlc_fail, failure_type, failure_reason) in htlc_fails.drain(..) {
let failure = match htlc_fail {
HTLCFailureMsg::Relay(fail_htlc) => HTLCForwardInfo::FailHTLC {
@@ -7247,7 +7234,7 @@ impl<
let mut new_events = VecDeque::new();
let mut failed_forwards = Vec::new();
- let mut phantom_receives: Vec<PerSourcePendingForward> = Vec::new();
+ let mut phantom_receives: Vec<PendingAddHTLCInfo> = Vec::new();
let mut forward_htlcs = new_hash_map();
mem::swap(&mut forward_htlcs, &mut self.forward_htlcs.lock().unwrap());
@@ -7294,7 +7281,7 @@ impl<
None,
);
}
- self.forward_htlcs(&mut phantom_receives);
+ self.forward_htlcs(phantom_receives);
if self.check_free_holding_cells() {
should_persist = NotifyOption::DoPersist;
@@ -7314,7 +7301,7 @@ impl<
fn forwarding_channel_not_found(
&self, forward_infos: impl Iterator<Item = HTLCForwardInfo>, short_chan_id: u64,
forwarding_counterparty: Option<PublicKey>, failed_forwards: &mut Vec<FailedHTLCForward>,
- phantom_receives: &mut Vec<PerSourcePendingForward>,
+ phantom_receives: &mut Vec<PendingAddHTLCInfo>,
) {
for forward_info in forward_infos {
match forward_info {
@@ -7436,14 +7423,15 @@ impl<
current_height,
);
match create_res {
- Ok(info) => phantom_receives.push((
+ Ok(info) => phantom_receives.push(PendingAddHTLCInfo {
+ forward_info: info,
prev_outbound_scid_alias,
+ prev_htlc_id,
prev_counterparty_node_id,
- prev_funding_outpoint,
prev_channel_id,
+ prev_funding_outpoint,
prev_user_channel_id,
- vec![(info, prev_htlc_id)],
- )),
+ }),
Err(InboundHTLCErr { reason, err_data, msg }) => {
failure_handler(
msg,
@@ -7495,7 +7483,7 @@ impl<
fn process_forward_htlcs(
&self, short_chan_id: u64, pending_forwards: &mut Vec<HTLCForwardInfo>,
failed_forwards: &mut Vec<FailedHTLCForward>,
- phantom_receives: &mut Vec<PerSourcePendingForward>,
+ phantom_receives: &mut Vec<PendingAddHTLCInfo>,
) {
let mut forwarding_counterparty = None;
@@ -9572,8 +9560,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
fn post_monitor_update_unlock(
&self, channel_id: ChannelId, counterparty_node_id: PublicKey, funding_txo: OutPoint,
user_channel_id: u128, unbroadcasted_batch_funding_txid: Option<Txid>,
- update_actions: Vec<MonitorUpdateCompletionAction>,
- htlc_forwards: Option<PerSourcePendingForward>,
+ update_actions: Vec<MonitorUpdateCompletionAction>, htlc_forwards: Vec<PendingAddHTLCInfo>,
decode_update_add_htlcs: Option<(u64, Vec<msgs::UpdateAddHTLC>)>,
finalized_claimed_htlcs: Vec<(HTLCSource, Option<AttributionData>)>,
failed_htlcs: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>,
@@ -9634,9 +9621,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
self.handle_monitor_update_completion_actions(update_actions);
- if let Some(forwards) = htlc_forwards {
- self.forward_htlcs(&mut [forwards][..]);
- }
+ self.forward_htlcs(htlc_forwards);
if let Some(decode) = decode_update_add_htlcs {
self.push_decode_update_add_htlcs(decode);
}
@@ -10263,7 +10248,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
channel_ready: Option<msgs::ChannelReady>, announcement_sigs: Option<msgs::AnnouncementSignatures>,
tx_signatures: Option<msgs::TxSignatures>, tx_abort: Option<msgs::TxAbort>,
channel_ready_order: ChannelReadyOrder,
- ) -> (Option<(u64, PublicKey, OutPoint, ChannelId, u128, Vec<(PendingHTLCInfo, u64)>)>, Option<(u64, Vec<msgs::UpdateAddHTLC>)>) {
+ ) -> (Vec<PendingAddHTLCInfo>, Option<(u64, Vec<msgs::UpdateAddHTLC>)>) {
let logger = WithChannelContext::from(&self.logger, &channel.context, None);
log_trace!(logger, "Handling channel resumption with {} RAA, {} commitment update, {} pending forwards, {} pending update_add_htlcs, {}broadcasting funding, {} channel ready, {} announcement, {} tx_signatures, {} tx_abort",
if raa.is_some() { "an" } else { "no" },
@@ -10279,13 +10264,19 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
let counterparty_node_id = channel.context.get_counterparty_node_id();
let outbound_scid_alias = channel.context.outbound_scid_alias();
- let mut htlc_forwards = None;
+ let mut htlc_forwards = Vec::new();
if !pending_forwards.is_empty() {
- htlc_forwards = Some((
- outbound_scid_alias, channel.context.get_counterparty_node_id(),
- channel.funding.get_funding_txo().unwrap(), channel.context.channel_id(),
- channel.context.get_user_id(), pending_forwards
- ));
+ htlc_forwards = pending_forwards.into_iter().map(|(forward_info, prev_htlc_id)| {
+ PendingAddHTLCInfo {
+ forward_info,
+ prev_outbound_scid_alias: outbound_scid_alias,
+ prev_htlc_id,
+ prev_counterparty_node_id: counterparty_node_id,
+ prev_channel_id: channel.context.channel_id(),
+ prev_funding_outpoint: channel.funding.get_funding_txo().unwrap(),
+ prev_user_channel_id: channel.context.get_user_id(),
+ }
+ }).collect();
}
let mut decode_update_add_htlcs = None;
if !pending_update_adds.is_empty() {
@@ -12130,44 +12121,22 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
}
#[inline]
- fn forward_htlcs(&self, per_source_pending_forwards: &mut [PerSourcePendingForward]) {
- for &mut (
- prev_outbound_scid_alias,
- prev_counterparty_node_id,
- prev_funding_outpoint,
- prev_channel_id,
- prev_user_channel_id,
- ref mut pending_forwards,
- ) in per_source_pending_forwards
- {
- if !pending_forwards.is_empty() {
- for (forward_info, prev_htlc_id) in pending_forwards.drain(..) {
- let scid = match forward_info.routing {
- PendingHTLCRouting::Forward { short_channel_id, .. } => short_channel_id,
- PendingHTLCRouting::TrampolineForward { .. }
- | PendingHTLCRouting::Receive { .. }
- | PendingHTLCRouting::ReceiveKeysend { .. } => 0,
- };
-
- let pending_add = PendingAddHTLCInfo {
- prev_outbound_scid_alias,
- prev_counterparty_node_id,
- prev_funding_outpoint,
- prev_channel_id,
- prev_htlc_id,
- prev_user_channel_id,
- forward_info,
- };
+ fn forward_htlcs<I: IntoIterator<Item = PendingAddHTLCInfo>>(&self, pending_forwards: I) {
+ for htlc in pending_forwards.into_iter() {
+ let scid = match htlc.forward_info.routing {
+ PendingHTLCRouting::Forward { short_channel_id, .. } => short_channel_id,
+ PendingHTLCRouting::TrampolineForward { .. }
+ | PendingHTLCRouting::Receive { .. }
+ | PendingHTLCRouting::ReceiveKeysend { .. } => 0,
+ };
- match self.forward_htlcs.lock().unwrap().entry(scid) {
- hash_map::Entry::Occupied(mut entry) => {
- entry.get_mut().push(HTLCForwardInfo::AddHTLC(pending_add));
- },
- hash_map::Entry::Vacant(entry) => {
- entry.insert(vec![HTLCForwardInfo::AddHTLC(pending_add)]);
- },
- }
- }
+ match self.forward_htlcs.lock().unwrap().entry(scid) {
+ hash_map::Entry::Occupied(mut entry) => {
+ entry.get_mut().push(HTLCForwardInfo::AddHTLC(htlc));
+ },
+ hash_map::Entry::Vacant(entry) => {
+ entry.insert(vec![HTLCForwardInfo::AddHTLC(htlc)]);
+ },
}
}
}
@@ -12502,7 +12471,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
Vec::new(), Vec::new(), None, responses.channel_ready, responses.announcement_sigs,
responses.tx_signatures, responses.tx_abort, responses.channel_ready_order,
);
- debug_assert!(htlc_forwards.is_none());
+ debug_assert!(htlc_forwards.is_empty());
debug_assert!(decode_update_add_htlcs.is_none());
if let Some(upd) = channel_update {
peer_state.pending_msg_events.push(upd);
@@ -16563,15 +16532,7 @@ impl<
},
}
} else {
- let mut per_source_pending_forward = [(
- htlc.prev_outbound_scid_alias,
- htlc.prev_counterparty_node_id,
- htlc.prev_funding_outpoint,
- htlc.prev_channel_id,
- htlc.prev_user_channel_id,
- vec![(htlc.forward_info, htlc.prev_htlc_id)],
- )];
- self.forward_htlcs(&mut per_source_pending_forward);
+ self.forward_htlcs([htlc]);
}
},
_ => return,
Risk score
Our methodology →Why this scored 15/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.