ln: make channel required in `MonitorUpdateCompletionAction`
What the developer wrote
Authored by Carla Kirk-Cohen
ln: make channel required in `MonitorUpdateCompletionAction`
`downstream_counterparty_and_funding_outpoint` was added to LDK in
0.0.116. We do not allow direct upgrades with pending forwards to 0.1
from 0.0.123 and below, so we can now assume that this field will
always be present.
This change also makes it impossible to create a
`EmitEventOptionAndFreeOtherChannel` action with nothing in it (no
event or channel), which could have been possible now that we've made
the event optional).
What changed, and why it matters
This commit tightens an internal data structure in the Lightning Dev Kit so that a channel identifier is always required when releasing a blocked channel after a monitor update. Previously the channel field was optional, which could have allowed creating an action that neither emitted an event nor freed a channel—an empty no-op. The change removes that possibility and updates serialization to treat the field as required, reflecting that older versions without it are no longer supported for direct upgrades.
Treat as a defensive cleanup/hardening commit. No immediate security response is indicated by the supplied materials. Reviewers may want to confirm that all downgrade paths from supported versions indeed populate the field, and that the change does not introduce panics on deserialization of state produced by supported upgrade paths.
Security signals we found
Internal state-machine hardening: removes an empty/no-op action variant that could arise from making the event optional while the channel remained optional.
Serialization change from upgradable_option to upgradable_required for a channel identifier field.
No direct security claim, exploit primitive, or vulnerability description in commit message or diff.
Evidence from the diff
In lightning/src/ln/channelmanager.rs, the EmitEventOptionAndFreeOtherChannel variant of MonitorUpdateCompletionAction changes downstream_counterparty_and_funding_outpoint from Option<EventUnblockedChannel> to EventUnblockedChannel. The corresponding TLV serialization is changed from upgradable_option to upgradable_required. Call sites are updated to construct the value directly and to unwrap it without Option handling. The commit message states this is safe because LDK 0.1 does not allow direct upgrades from 0.0.123 and below with pending forwards, so the field (added in 0.0.116) will always be present. It also prevents the now-possible construction of an action with neither an event nor a channel.
Changed components
lightning/src/ln/channelmanager.rsMonitorUpdateCompletionAction enumEmitEventOptionAndFreeOtherChannel variantChannel monitor update completion handlingInspect captured patch +20 / −27
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index ef3d59e..d60c21c 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -1394,7 +1394,7 @@ pub(crate) enum MonitorUpdateCompletionAction {
/// outbound edge.
EmitEventOptionAndFreeOtherChannel {
event: Option<events::Event>,
- downstream_counterparty_and_funding_outpoint: Option<EventUnblockedChannel>,
+ downstream_counterparty_and_funding_outpoint: EventUnblockedChannel,
},
/// Indicates we should immediately resume the operation of another channel, unless there is
/// some other reason why the channel is blocked. In practice this simply means immediately
@@ -1432,12 +1432,7 @@ impl_writeable_tlv_based_enum_upgradable!(MonitorUpdateCompletionAction,
// with multiple incoming HTLCS, so nodes cannot downgrade while trampoline payments
// are in the process of being resolved.
(0, event, upgradable_option),
- // LDK prior to 0.0.116 did not have this field as the monitor update application order was
- // required by clients. If we downgrade to something prior to 0.0.116 this may result in
- // monitor updates which aren't properly blocked or resumed, however that's fine - we don't
- // support async monitor updates even in LDK 0.0.116 and once we do we'll require no
- // downgrades to prior versions.
- (1, downstream_counterparty_and_funding_outpoint, upgradable_option),
+ (1, downstream_counterparty_and_funding_outpoint, upgradable_required),
},
);
@@ -9674,12 +9669,12 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
None,
Some(attribution_data),
|htlc_claim_value_msat, definitely_duplicate| {
- let chan_to_release = Some(EventUnblockedChannel {
+ let chan_to_release = EventUnblockedChannel {
counterparty_node_id: next_channel_counterparty_node_id,
funding_txo: next_channel_outpoint,
channel_id: next_channel_id,
blocking_action: completed_blocker,
- });
+ };
if definitely_duplicate && startup_replay {
// On startup we may get redundant claims which are related to
@@ -9732,15 +9727,15 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
}
(None, None)
} else if definitely_duplicate {
- if let Some(other_chan) = chan_to_release {
- (Some(MonitorUpdateCompletionAction::FreeOtherChannelImmediately {
- downstream_counterparty_node_id: other_chan.counterparty_node_id,
- downstream_channel_id: other_chan.channel_id,
- blocking_action: other_chan.blocking_action,
- }), None)
- } else {
- (None, None)
- }
+ (
+ Some(MonitorUpdateCompletionAction::FreeOtherChannelImmediately {
+ downstream_counterparty_node_id: chan_to_release
+ .counterparty_node_id,
+ downstream_channel_id: chan_to_release.channel_id,
+ blocking_action: chan_to_release.blocking_action,
+ }),
+ None,
+ )
} else {
let total_fee_earned_msat =
if let Some(forwarded_htlc_value) = forwarded_htlc_value_msat {
@@ -10006,13 +10001,11 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
if let Some(event) = event {
self.pending_events.lock().unwrap().push_back((event, None));
}
- if let Some(unblocked) = downstream_counterparty_and_funding_outpoint {
- self.handle_monitor_update_release(
- unblocked.counterparty_node_id,
- unblocked.channel_id,
- Some(unblocked.blocking_action),
- );
- }
+ self.handle_monitor_update_release(
+ downstream_counterparty_and_funding_outpoint.counterparty_node_id,
+ downstream_counterparty_and_funding_outpoint.channel_id,
+ Some(downstream_counterparty_and_funding_outpoint.blocking_action),
+ );
},
MonitorUpdateCompletionAction::FreeOtherChannelImmediately {
downstream_counterparty_node_id,
@@ -19513,12 +19506,12 @@ impl<
for action in actions.iter() {
if let MonitorUpdateCompletionAction::EmitEventOptionAndFreeOtherChannel {
downstream_counterparty_and_funding_outpoint:
- Some(EventUnblockedChannel {
+ EventUnblockedChannel {
counterparty_node_id: blocked_node_id,
funding_txo: _,
channel_id: blocked_channel_id,
blocking_action,
- }),
+ },
..
} = action
{
Why this scored 27/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.