Skip to content

Reconstruct ChannelManager forwarded HTLCs maps from Channels #4227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
valentinewallace merged 9 commits into from
Dec 11, 2025
Merged

Reconstruct ChannelManager forwarded HTLCs maps from Channels #4227

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4561bc5
Git-ignore lightning-tests/target
valentinewallace Dec 2, 2025
4f055ac
Store inbound committed update_adds in Channel
valentinewallace Nov 13, 2025
c27093d
Extract util for HTLCIntercepted event creation
valentinewallace Dec 2, 2025
26992e1
Extract method to dedup pre-decode update_add
valentinewallace Nov 17, 2025
005da38
Rename manager HTLC forward maps to _legacy
valentinewallace Nov 20, 2025
7c4d021
Tweak pending_htlc_intercepts ser on manager read
valentinewallace Nov 20, 2025
64de989
Gather to-decode HTLC fwds from channels on manager read
valentinewallace Nov 20, 2025
cb398f6
Rebuild manager forwarded htlcs maps from Channels
valentinewallace Nov 20, 2025
a24dcff
Test 0.2 -> 0.3 reload with with forward htlcs present
valentinewallace Dec 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 61 additions & 31 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,14 @@ enum InboundHTLCState {
/// channel (before it can then get forwarded and/or removed).
/// Implies AwaitingRemoteRevoke.
AwaitingAnnouncedRemoteRevoke(InboundHTLCResolution),
Committed,
/// An HTLC irrevocably committed in the latest commitment transaction, ready to be forwarded or
/// removed.
Committed {
Comment thread
valentinewallace marked this conversation as resolved.
/// Used to rebuild `ChannelManager` HTLC state on restart. Previously the manager would track
/// and persist all HTLC forwards and receives itself, but newer LDK versions avoid relying on
/// its persistence and instead reconstruct state based on `Channel` and `ChannelMonitor` data.
update_add_htlc_opt: Option<msgs::UpdateAddHTLC>,
Comment thread
valentinewallace marked this conversation as resolved.

@TheBlueMatt TheBlueMatt Dec 11, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bleh, writing the full onion packet for every pending HTLC (not just ones waiting to be forwarded) on every write (today in ChannelManager but also eventually in ChannelMonitorUpdates) is pretty gross (especially if we add an extra onion in the HTLCs for extra data, which we kinda need to do). Maybe the solution is to remove this field once the HTLC has been (irrevocably, by persisting a monitor update) forwarded, though it might be a bit annoying code-wise.

@valentinewallace valentinewallace Dec 16, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this will cause real performance issues? 1000 outstanding inbound HTLCs = 1.3 extra MB per manager persist. It seems potentially like a premature optimization, or at least like the LN might have to really take off in between the time we merge this change and when we release support for not having to persist the manager.

That said, it might be easy to remove these irrevocable forwards on timer_tick_occurred? Or possibly return them from the call to channel.monitor_updating_restored(..) and remove them then.

@TheBlueMatt TheBlueMatt Dec 16, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An extra MiB (on an object that's usually smaller than, or close to for a large routing node, a single MiB or so - its currently 255K on my node) is a lot of extra overhead for an object we write all the time. But I'm also interested in its size in ChannelMonitorUpdates - storing all pending HTLCs takes ChannelMonitorUpdates from small diffs to huge objects, adding a ton of write time...

That said, it might be easy to remove these irrevocable forwards on timer_tick_occurred?

That seems like a lot of overhead to iterate every HTLC and see if its been forwarded on each timer tick.

Or possibly return them from the call to channel.monitor_updating_restored(..) and remove them then.

This seems simpler.

@valentinewallace valentinewallace Jan 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #4303

},
/// Removed by us and a new commitment_signed was sent (if we were AwaitingRemoteRevoke when we
/// created it we would have put it in the holding cell instead). When they next revoke_and_ack
/// we'll drop it.
Expand All @@ -235,7 +242,7 @@ impl From<&InboundHTLCState> for Option<InboundHTLCStateDetails> {
InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => {
Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToAdd)
},
InboundHTLCState::Committed => Some(InboundHTLCStateDetails::Committed),
InboundHTLCState::Committed { .. } => Some(InboundHTLCStateDetails::Committed),
InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailRelay(_)) => {
Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail)
},
Expand All @@ -256,7 +263,7 @@ impl fmt::Display for InboundHTLCState {
InboundHTLCState::RemoteAnnounced(_) => write!(f, "RemoteAnnounced"),
InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => write!(f, "AwaitingRemoteRevokeToAnnounce"),
InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => write!(f, "AwaitingAnnouncedRemoteRevoke"),
InboundHTLCState::Committed => write!(f, "Committed"),
InboundHTLCState::Committed { .. } => write!(f, "Committed"),
InboundHTLCState::LocalRemoved(_) => write!(f, "LocalRemoved"),
}
}
Expand All @@ -268,7 +275,7 @@ impl InboundHTLCState {
InboundHTLCState::RemoteAnnounced(_) => !generated_by_local,
InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => !generated_by_local,
InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => true,
InboundHTLCState::Committed => true,
InboundHTLCState::Committed { .. } => true,
InboundHTLCState::LocalRemoved(_) => !generated_by_local,
}
}
Expand Down Expand Up @@ -296,7 +303,7 @@ impl InboundHTLCState {
},
InboundHTLCResolution::Resolved { .. } => false,
},
InboundHTLCState::Committed | InboundHTLCState::LocalRemoved(_) => false,
InboundHTLCState::Committed { .. } | InboundHTLCState::LocalRemoved(_) => false,
}
}
}
Expand Down Expand Up @@ -4102,7 +4109,7 @@ where

if self.pending_inbound_htlcs.iter()
.any(|htlc| match htlc.state {
InboundHTLCState::Committed => false,
InboundHTLCState::Committed { .. } => false,
// An HTLC removal from the local node is pending on the remote commitment.
InboundHTLCState::LocalRemoved(_) => true,
// An HTLC add from the remote node is pending on the local commitment.
Expand Down Expand Up @@ -4531,7 +4538,7 @@ where
(InboundHTLCState::RemoteAnnounced(..), _) => true,
(InboundHTLCState::AwaitingRemoteRevokeToAnnounce(..), _) => true,
(InboundHTLCState::AwaitingAnnouncedRemoteRevoke(..), _) => true,
(InboundHTLCState::Committed, _) => true,
(InboundHTLCState::Committed { .. }, _) => true,
(InboundHTLCState::LocalRemoved(..), true) => true,
(InboundHTLCState::LocalRemoved(..), false) => false,
})
Expand Down Expand Up @@ -7320,7 +7327,7 @@ where
payment_preimage_arg
);
match htlc.state {
InboundHTLCState::Committed => {},
InboundHTLCState::Committed { .. } => {},
InboundHTLCState::LocalRemoved(ref reason) => {
if let &InboundHTLCRemovalReason::Fulfill { .. } = reason {
} else {
Expand Down Expand Up @@ -7413,7 +7420,7 @@ where

{
let htlc = &mut self.context.pending_inbound_htlcs[pending_idx];
if let InboundHTLCState::Committed = htlc.state {
if let InboundHTLCState::Committed { .. } = htlc.state {
} else {
debug_assert!(
false,
Expand Down Expand Up @@ -7548,7 +7555,7 @@ where
for (idx, htlc) in self.context.pending_inbound_htlcs.iter().enumerate() {
if htlc.htlc_id == htlc_id_arg {
match htlc.state {
InboundHTLCState::Committed => {},
InboundHTLCState::Committed { .. } => {},
InboundHTLCState::LocalRemoved(_) => {
return Err(ChannelError::Ignore(format!("HTLC {} was already resolved", htlc.htlc_id)));
},
Expand Down Expand Up @@ -8716,7 +8723,7 @@ where
false
};
if swap {
let mut state = InboundHTLCState::Committed;
let mut state = InboundHTLCState::Committed { update_add_htlc_opt: None };
mem::swap(&mut state, &mut htlc.state);

if let InboundHTLCState::AwaitingRemoteRevokeToAnnounce(resolution) = state {
Expand Down Expand Up @@ -8755,14 +8762,21 @@ where
PendingHTLCStatus::Forward(forward_info) => {
log_trace!(logger, " ...promoting inbound AwaitingAnnouncedRemoteRevoke {} to Committed, attempting to forward", &htlc.payment_hash);
to_forward_infos.push((forward_info, htlc.htlc_id));
htlc.state = InboundHTLCState::Committed;
htlc.state = InboundHTLCState::Committed {
Comment thread
joostjager marked this conversation as resolved.
// HTLCs will only be in state `InboundHTLCResolution::Resolved` if they were

@TheBlueMatt TheBlueMatt Dec 11, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should this be an assertion that gets validated more explicitly on deserialization? This comment seems to imply this code is not reachable and will somehow misbehave, the second isn't clear to me but we should validate the first either way.

@valentinewallace valentinewallace Dec 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant in this comment is that this code is reachable but it shouldn't matter that we set update_add_htlc_opt to None here because we already require this HTLC to not be here on restart, according to the changelog (and restart is the only time when update_add_htlc_opt is used).

We can start returning explicitly DecodeError::InvalidValue if this variant is present on read -- to be clear, is this what you're advocating for?

I unfortunately am not sure the exact misbehavior that can occur, according to the changelog it comes from #3355 but it isn't explained in much detail that I can see

@TheBlueMatt TheBlueMatt Dec 16, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already require this HTLC to not be here on restart, according to the changelog (and restart is the only time when update_add_htlc_opt is used).
We can start returning explicitly DecodeError::InvalidValue if this variant is present on read -- to be clear, is this what you're advocating for?

Right, if there's some implicit assumption based on the changelog and upgrade behavior, we really need to be validating that before allowing an object to be deserialized.

@valentinewallace valentinewallace Jan 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #4303

// received on an old pre-0.0.123 version of LDK. In this case, the HTLC is
// required to be resolved prior to upgrading to 0.1+ per CHANGELOG.md.
update_add_htlc_opt: None,
};
},
}
},
InboundHTLCResolution::Pending { update_add_htlc } => {
log_trace!(logger, " ...promoting inbound AwaitingAnnouncedRemoteRevoke {} to Committed", &htlc.payment_hash);
pending_update_adds.push(update_add_htlc);
htlc.state = InboundHTLCState::Committed;
pending_update_adds.push(update_add_htlc.clone());
htlc.state = InboundHTLCState::Committed {
update_add_htlc_opt: Some(update_add_htlc),
Comment thread
joostjager marked this conversation as resolved.
};
},
}
}
Expand Down Expand Up @@ -9297,7 +9311,7 @@ where
// in response to it yet, so don't touch it.
true
},
InboundHTLCState::Committed => true,
InboundHTLCState::Committed { .. } => true,
InboundHTLCState::LocalRemoved(_) => {
// We (hopefully) sent a commitment_signed updating this HTLC (which we can
// re-transmit if needed) and they may have even sent a revoke_and_ack back
Expand Down Expand Up @@ -14518,6 +14532,7 @@ where
}
}
let mut removed_htlc_attribution_data: Vec<&Option<AttributionData>> = Vec::new();
let mut inbound_committed_update_adds: Vec<Option<msgs::UpdateAddHTLC>> = Vec::new();
(self.context.pending_inbound_htlcs.len() as u64 - dropped_inbound_htlcs).write(writer)?;
for htlc in self.context.pending_inbound_htlcs.iter() {
if let &InboundHTLCState::RemoteAnnounced(_) = &htlc.state {
Expand All @@ -14537,8 +14552,9 @@ where
2u8.write(writer)?;
htlc_resolution.write(writer)?;
},
&InboundHTLCState::Committed => {
&InboundHTLCState::Committed { ref update_add_htlc_opt } => {
3u8.write(writer)?;
inbound_committed_update_adds.push(update_add_htlc_opt.clone());
Comment thread
valentinewallace marked this conversation as resolved.
},
&InboundHTLCState::LocalRemoved(ref removal_reason) => {
4u8.write(writer)?;
Expand Down Expand Up @@ -14914,6 +14930,7 @@ where
(69, holding_cell_held_htlc_flags, optional_vec), // Added in 0.2
(71, holder_commitment_point_previous_revoked, option), // Added in 0.3
(73, holder_commitment_point_last_revoked, option), // Added in 0.3
(75, inbound_committed_update_adds, optional_vec),
});

Ok(())
Expand Down Expand Up @@ -14997,7 +15014,7 @@ where
};
InboundHTLCState::AwaitingAnnouncedRemoteRevoke(resolution)
},
3 => InboundHTLCState::Committed,
3 => InboundHTLCState::Committed { update_add_htlc_opt: None },
4 => {
let reason = match <u8 as Readable>::read(reader)? {
0 => InboundHTLCRemovalReason::FailRelay(msgs::OnionErrorPacket {
Expand Down Expand Up @@ -15301,6 +15318,7 @@ where

let mut pending_outbound_held_htlc_flags_opt: Option<Vec<Option<()>>> = None;
let mut holding_cell_held_htlc_flags_opt: Option<Vec<Option<()>>> = None;
let mut inbound_committed_update_adds_opt: Option<Vec<Option<msgs::UpdateAddHTLC>>> = None;

read_tlv_fields!(reader, {
(0, announcement_sigs, option),
Expand Down Expand Up @@ -15350,6 +15368,7 @@ where
(69, holding_cell_held_htlc_flags_opt, optional_vec), // Added in 0.2
(71, holder_commitment_point_previous_revoked_opt, option), // Added in 0.3
(73, holder_commitment_point_last_revoked_opt, option), // Added in 0.3
(75, inbound_committed_update_adds_opt, optional_vec),
});

let holder_signer = signer_provider.derive_channel_signer(channel_keys_id);
Expand Down Expand Up @@ -15473,6 +15492,17 @@ where
return Err(DecodeError::InvalidValue);
}
}
if let Some(update_adds) = inbound_committed_update_adds_opt {
let mut iter = update_adds.into_iter();
for htlc in pending_inbound_htlcs.iter_mut() {
if let InboundHTLCState::Committed { ref mut update_add_htlc_opt } = htlc.state {
*update_add_htlc_opt = iter.next().ok_or(DecodeError::InvalidValue)?;
}
}
if iter.next().is_some() {
return Err(DecodeError::InvalidValue);
}
}

if let Some(attribution_data_list) = removed_htlc_attribution_data {
let mut removed_htlcs = pending_inbound_htlcs.iter_mut().filter_map(|status| {
Expand Down Expand Up @@ -16057,7 +16087,7 @@ mod tests {
amount_msat: htlc_amount_msat,
payment_hash: PaymentHash(Sha256::hash(&[42; 32]).to_byte_array()),
cltv_expiry: 300000000,
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
Comment thread
valentinewallace marked this conversation as resolved.
});

node_a_chan.context.pending_outbound_htlcs.push(OutboundHTLCOutput {
Expand Down Expand Up @@ -16903,7 +16933,7 @@ mod tests {
amount_msat: 1000000,
cltv_expiry: 500,
payment_hash: PaymentHash::from(payment_preimage_0),
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
});

let payment_preimage_1 =
Expand All @@ -16913,7 +16943,7 @@ mod tests {
amount_msat: 2000000,
cltv_expiry: 501,
payment_hash: PaymentHash::from(payment_preimage_1),
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
});

let payment_preimage_2 =
Expand Down Expand Up @@ -16953,7 +16983,7 @@ mod tests {
amount_msat: 4000000,
cltv_expiry: 504,
payment_hash: PaymentHash::from(payment_preimage_4),
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
});

// commitment tx with all five HTLCs untrimmed (minimum feerate)
Expand Down Expand Up @@ -17342,7 +17372,7 @@ mod tests {
amount_msat: 2000000,
cltv_expiry: 501,
payment_hash: PaymentHash::from(payment_preimage_1),
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
});

chan.context.pending_outbound_htlcs.clear();
Expand Down Expand Up @@ -17593,7 +17623,7 @@ mod tests {
amount_msat: 5000000,
cltv_expiry: 920150,
payment_hash: PaymentHash::from(htlc_in_preimage),
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
}));

chan.context.pending_outbound_htlcs.extend(
Expand Down Expand Up @@ -17656,7 +17686,7 @@ mod tests {
amount_msat,
cltv_expiry: 920150,
payment_hash: PaymentHash::from(htlc_in_preimage),
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
},
));

Expand Down Expand Up @@ -17722,7 +17752,7 @@ mod tests {
amount_msat: 100000,
cltv_expiry: 920125,
payment_hash: htlc_0_in_hash,
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
});

let htlc_1_in_preimage =
Expand All @@ -17740,7 +17770,7 @@ mod tests {
amount_msat: 49900000,
cltv_expiry: 920125,
payment_hash: htlc_1_in_hash,
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
});

chan.context.pending_outbound_htlcs.extend(
Expand Down Expand Up @@ -17792,7 +17822,7 @@ mod tests {
amount_msat: 30000,
payment_hash,
cltv_expiry: 920125,
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
},
));

Expand Down Expand Up @@ -17833,7 +17863,7 @@ mod tests {
amount_msat: 29525,
payment_hash,
cltv_expiry: 920125,
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
},
));

Expand Down Expand Up @@ -17870,7 +17900,7 @@ mod tests {
amount_msat: 29525,
payment_hash,
cltv_expiry: 920125,
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
},
));

Expand Down Expand Up @@ -17907,7 +17937,7 @@ mod tests {
amount_msat: 29753,
payment_hash,
cltv_expiry: 920125,
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
},
));

Expand Down Expand Up @@ -17959,7 +17989,7 @@ mod tests {
amount_msat,
cltv_expiry,
payment_hash,
state: InboundHTLCState::Committed,
state: InboundHTLCState::Committed { update_add_htlc_opt: None },
}),
);

Expand Down