Fix PaymentForwarded fields on restart claim
What changed, and why it matters
This commit fixes a bug in the Lightning Dev Kit where, after a node restarts, it was using the wrong payment channel's details when reconstructing forwarded payments. Specifically, it used the upstream (incoming) channel's information where it should have used the downstream (outgoing) channel's. This could cause incorrect data in PaymentForwarded events and potentially wrong decisions about whether the downstream channel is closed, but it does not appear to allow theft of funds or direct remote exploitation.
Review whether any deployed nodes logged or acted on incorrect PaymentForwarded fields or downstream-closed flags after restart; upgrade to include this fix. No immediate emergency response appears required because the bug is in accounting/event metadata rather than fund-custody logic.
Security signals we found
Incorrect channel metadata used in post-reload claim replay
PaymentForwarded event fields could be wrong after restart
Downstream closure detection used upstream counterparty lookup
user_channel_id was incorrectly None for reconstructed forwards
Evidence from the diff
The patch changes Channel::inbound_forwarded_htlcs to return the full OutboundHop struct (including node_id, channel_id, funding_txo, user_channel_id, and amt_msat) instead of only the outbound amount. ChannelManager then uses these downstream fields when replaying pending claims after reload. Previously it used the upstream channel monitor’s counterparty_node_id, funding_txo, and channel_id, and set user_channel_id to None. The test update removes an assertion that next_user_channel_id was None and now expects the proper forwarded event via expect_payment_forwarded!.
Changed components
lightning/src/ln/channel.rslightning/src/ln/channelmanager.rsPaymentForwarded event generationChannel reload / deserialization pathInspect captured patch +16 / −23
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 37a0661..4a0d117 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -7942,7 +7942,7 @@ where
/// when reconstructing the set of pending HTLCs when deserializing the `ChannelManager`.
pub(super) fn inbound_forwarded_htlcs(
&self,
- ) -> impl Iterator<Item = (PaymentHash, HTLCPreviousHopData, u64)> + '_ {
+ ) -> impl Iterator<Item = (PaymentHash, HTLCPreviousHopData, OutboundHop)> + '_ {
// We don't want to return an HTLC as needing processing if it already has a resolution that's
// pending in the holding cell.
let htlc_resolution_in_holding_cell = |id: u64| -> bool {
@@ -7970,7 +7970,7 @@ where
phantom_shared_secret,
trampoline_shared_secret,
blinded_failure,
- outbound_hop: OutboundHop { amt_msat, .. },
+ outbound_hop,
},
} => {
if htlc_resolution_in_holding_cell(htlc.htlc_id) {
@@ -7991,7 +7991,7 @@ where
counterparty_node_id: Some(counterparty_node_id),
cltv_expiry: Some(htlc.cltv_expiry),
};
- Some((htlc.payment_hash, prev_hop_data, *amt_msat))
+ Some((htlc.payment_hash, prev_hop_data, *outbound_hop))
},
_ => None,
})
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index b7b3969..897f10c 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -18610,11 +18610,11 @@ impl<
// that it is handled.
let mut already_forwarded_htlcs: HashMap<
(ChannelId, PaymentHash),
- Vec<(HTLCPreviousHopData, u64)>,
+ Vec<(HTLCPreviousHopData, OutboundHop)>,
> = new_hash_map();
let prune_forwarded_htlc = |already_forwarded_htlcs: &mut HashMap<
(ChannelId, PaymentHash),
- Vec<(HTLCPreviousHopData, u64)>,
+ Vec<(HTLCPreviousHopData, OutboundHop)>,
>,
prev_hop: &HTLCPreviousHopData,
payment_hash: &PaymentHash| {
@@ -18663,13 +18663,13 @@ impl<
.or_insert_with(Vec::new)
.push(update_add_htlc);
}
- for (payment_hash, prev_hop, outbound_amt_msat) in
+ for (payment_hash, prev_hop, next_hop) in
funded_chan.inbound_forwarded_htlcs()
{
already_forwarded_htlcs
.entry((prev_hop.channel_id, payment_hash))
.or_insert_with(Vec::new)
- .push((prev_hop, outbound_amt_msat));
+ .push((prev_hop, next_hop));
}
}
}
@@ -19378,34 +19378,33 @@ impl<
if let Some(forwarded_htlcs) =
already_forwarded_htlcs.remove(&(*channel_id, payment_hash))
{
- for (prev_hop, outbound_amt_msat) in forwarded_htlcs {
+ for (prev_hop, next_hop) in forwarded_htlcs {
let new_pending_claim =
!pending_claims_to_replay.iter().any(|(src, _, _, _, _, _, _, _)| {
matches!(src, HTLCSource::PreviousHopData(hop) if hop.htlc_id == prev_hop.htlc_id && hop.channel_id == prev_hop.channel_id)
});
if new_pending_claim {
- let counterparty_node_id = monitor.get_counterparty_node_id();
let is_downstream_closed = channel_manager
.per_peer_state
.read()
.unwrap()
- .get(&counterparty_node_id)
+ .get(&next_hop.node_id)
.map_or(true, |peer_state_mtx| {
!peer_state_mtx
.lock()
.unwrap()
.channel_by_id
- .contains_key(channel_id)
+ .contains_key(&next_hop.channel_id)
});
pending_claims_to_replay.push((
HTLCSource::PreviousHopData(prev_hop),
payment_preimage,
- outbound_amt_msat,
+ next_hop.amt_msat,
is_downstream_closed,
- counterparty_node_id,
- monitor.get_funding_txo(),
- *channel_id,
- None,
+ next_hop.node_id,
+ next_hop.funding_txo,
+ next_hop.channel_id,
+ Some(next_hop.user_channel_id),
));
}
}
diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs
index c7e7175..42986bc 100644
--- a/lightning/src/ln/reload_tests.rs
+++ b/lightning/src/ln/reload_tests.rs
@@ -1958,14 +1958,8 @@ fn test_reload_node_with_preimage_in_monitor_claims_htlc() {
);
// When the claim is reconstructed during reload, a PaymentForwarded event is generated.
- // This event has next_user_channel_id as None since the outbound HTLC was already removed.
// Fetching events triggers the pending monitor update (adding preimage) to be applied.
- let events = nodes[1].node.get_and_clear_pending_events();
- assert_eq!(events.len(), 1);
- match &events[0] {
- Event::PaymentForwarded { total_fee_earned_msat: Some(1000), .. } => {},
- _ => panic!("Expected PaymentForwarded event"),
- }
+ expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], Some(1000), false, false);
check_added_monitors(&nodes[1], 1);
// Reconnect nodes[1] to nodes[0]. The claim should be in nodes[1]'s holding cell.
Why this scored 43/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.