Convert of `handle_new_monitor_update_internal` to a function
What changed, and why it matters
This commit is a straightforward internal code cleanup in the Lightning Dev Kit's Rust implementation. It converts a chunk of logic that was previously duplicated through a macro into a regular Rust function. The behavior is intended to remain identical; the change only reduces code size and improves maintainability. There is no indication of a security fix or vulnerability being addressed.
No security action required. Treat as a normal maintainability refactor.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The patch refactors handle_new_monitor_update_internal from a macro into a generic function in lightning/src/ln/channelmanager.rs. The function takes explicit parameters (AChannelManager, logger, in-flight updates map, channel ID, funding outpoint, counterparty node ID, and the new monitor update) and returns a tuple (bool, bool) indicating whether the update completed and whether all in-flight updates are now complete. The surrounding macros (handle_post_close_monitor_update, handle_new_monitor_update_locked_actions_handled_by_caller, handle_new_monitor_update) are updated to call this function and to perform the previously inline all_completed actions outside the call. The logic for tracking in-flight monitor updates, replaying startup events, and invoking completion actions is preserved.
Changed components
lightning/src/ln/channelmanager.rsInspect captured patch +87 / −98
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 67d3c3c..f4fff82 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -3696,57 +3696,54 @@ macro_rules! handle_initial_monitor {
};
}
-macro_rules! handle_new_monitor_update_internal {
- (
- $self: ident, $funding_txo: expr, $update: expr, $peer_state: expr, $logger: expr,
- $chan_id: expr, $counterparty_node_id: expr, $all_completed: expr
- ) => {{
- let in_flight_updates = &mut $peer_state
- .in_flight_monitor_updates
- .entry($chan_id)
- .or_insert_with(|| ($funding_txo, Vec::new()))
- .1;
- // During startup, we push monitor updates as background events through to here in
- // order to replay updates that were in-flight when we shut down. Thus, we have to
- // filter for uniqueness here.
- let update_idx =
- in_flight_updates.iter().position(|upd| upd == &$update).unwrap_or_else(|| {
- in_flight_updates.push($update);
- in_flight_updates.len() - 1
- });
- if $self.background_events_processed_since_startup.load(Ordering::Acquire) {
- let update_res =
- $self.chain_monitor.update_channel($chan_id, &in_flight_updates[update_idx]);
- let update_completed = handle_monitor_update_res($self, update_res, $chan_id, $logger);
- if update_completed {
- let _ = in_flight_updates.remove(update_idx);
- if in_flight_updates.is_empty() {
- $all_completed;
- }
- }
- update_completed
- } else {
- // We blindly assume that the ChannelMonitorUpdate will be regenerated on startup if we
- // fail to persist it. This is a fairly safe assumption, however, since anything we do
- // during the startup sequence should be replayed exactly if we immediately crash.
- let event = BackgroundEvent::MonitorUpdateRegeneratedOnStartup {
- counterparty_node_id: $counterparty_node_id,
- funding_txo: $funding_txo,
- channel_id: $chan_id,
- update: in_flight_updates[update_idx].clone(),
- };
- // We want to track the in-flight update both in `in_flight_monitor_updates` and in
- // `pending_background_events` to avoid a race condition during
- // `pending_background_events` processing where we complete one
- // `ChannelMonitorUpdate` (but there are more pending as background events) but we
- // conclude that all pending `ChannelMonitorUpdate`s have completed and its safe to
- // run post-completion actions.
- // We could work around that with some effort, but its simpler to just track updates
- // twice.
- $self.pending_background_events.lock().unwrap().push(event);
- false
- }
- }};
+fn handle_new_monitor_update_internal<CM: AChannelManager, LG: Logger>(
+ cm: &CM,
+ in_flight_monitor_updates: &mut BTreeMap<ChannelId, (OutPoint, Vec<ChannelMonitorUpdate>)>,
+ channel_id: ChannelId, funding_txo: OutPoint, counterparty_node_id: PublicKey,
+ new_update: ChannelMonitorUpdate, logger: LG,
+) -> (bool, bool) {
+ let in_flight_updates = &mut in_flight_monitor_updates
+ .entry(channel_id)
+ .or_insert_with(|| (funding_txo, Vec::new()))
+ .1;
+ // During startup, we push monitor updates as background events through to here in
+ // order to replay updates that were in-flight when we shut down. Thus, we have to
+ // filter for uniqueness here.
+ let update_idx =
+ in_flight_updates.iter().position(|upd| upd == &new_update).unwrap_or_else(|| {
+ in_flight_updates.push(new_update);
+ in_flight_updates.len() - 1
+ });
+
+ if cm.get_cm().background_events_processed_since_startup.load(Ordering::Acquire) {
+ let update_res =
+ cm.get_cm().chain_monitor.update_channel(channel_id, &in_flight_updates[update_idx]);
+ let update_completed = handle_monitor_update_res(cm, update_res, channel_id, logger);
+ if update_completed {
+ let _ = in_flight_updates.remove(update_idx);
+ }
+ (update_completed, update_completed && in_flight_updates.is_empty())
+ } else {
+ // We blindly assume that the ChannelMonitorUpdate will be regenerated on startup if we
+ // fail to persist it. This is a fairly safe assumption, however, since anything we do
+ // during the startup sequence should be replayed exactly if we immediately crash.
+ let event = BackgroundEvent::MonitorUpdateRegeneratedOnStartup {
+ counterparty_node_id,
+ funding_txo,
+ channel_id,
+ update: in_flight_updates[update_idx].clone(),
+ };
+ // We want to track the in-flight update both in `in_flight_monitor_updates` and in
+ // `pending_background_events` to avoid a race condition during
+ // `pending_background_events` processing where we complete one
+ // `ChannelMonitorUpdate` (but there are more pending as background events) but we
+ // conclude that all pending `ChannelMonitorUpdate`s have completed and its safe to
+ // run post-completion actions.
+ // We could work around that with some effort, but its simpler to just track updates
+ // twice.
+ cm.get_cm().pending_background_events.lock().unwrap().push(event);
+ (false, false)
+ }
}
macro_rules! handle_post_close_monitor_update {
@@ -3754,28 +3751,27 @@ macro_rules! handle_post_close_monitor_update {
$self: ident, $funding_txo: expr, $update: expr, $peer_state_lock: expr, $peer_state: expr,
$per_peer_state_lock: expr, $counterparty_node_id: expr, $channel_id: expr
) => {{
- let logger =
- WithContext::from(&$self.logger, Some($counterparty_node_id), Some($channel_id), None);
- handle_new_monitor_update_internal!(
+ let (update_completed, all_updates_complete) = handle_new_monitor_update_internal(
$self,
- $funding_txo,
- $update,
- $peer_state,
- logger,
+ &mut $peer_state.in_flight_monitor_updates,
$channel_id,
+ $funding_txo,
$counterparty_node_id,
- {
- let update_actions = $peer_state
- .monitor_update_blocked_actions
- .remove(&$channel_id)
- .unwrap_or(Vec::new());
+ $update,
+ WithContext::from(&$self.logger, Some($counterparty_node_id), Some($channel_id), None),
+ );
+ if all_updates_complete {
+ let update_actions = $peer_state
+ .monitor_update_blocked_actions
+ .remove(&$channel_id)
+ .unwrap_or(Vec::new());
- mem::drop($peer_state_lock);
- mem::drop($per_peer_state_lock);
+ mem::drop($peer_state_lock);
+ mem::drop($per_peer_state_lock);
- $self.handle_monitor_update_completion_actions(update_actions);
- }
- )
+ $self.handle_monitor_update_completion_actions(update_actions);
+ }
+ update_completed
}};
}
@@ -3792,19 +3788,16 @@ macro_rules! handle_new_monitor_update_locked_actions_handled_by_caller {
(
$self: ident, $funding_txo: expr, $update: expr, $peer_state: expr, $chan_context: expr
) => {{
- let logger = WithChannelContext::from(&$self.logger, &$chan_context, None);
- let chan_id = $chan_context.channel_id();
- let counterparty_node_id = $chan_context.get_counterparty_node_id();
- handle_new_monitor_update_internal!(
+ let (update_completed, _all_updates_complete) = handle_new_monitor_update_internal(
$self,
+ &mut $peer_state.in_flight_monitor_updates,
+ $chan_context.channel_id(),
$funding_txo,
+ $chan_context.get_counterparty_node_id(),
$update,
- $peer_state,
- logger,
- chan_id,
- counterparty_node_id,
- {}
- )
+ WithChannelContext::from(&$self.logger, &$chan_context, None),
+ );
+ update_completed
}};
}
@@ -3813,29 +3806,25 @@ macro_rules! handle_new_monitor_update {
$self: ident, $funding_txo: expr, $update: expr, $peer_state_lock: expr, $peer_state: expr,
$per_peer_state_lock: expr, $chan: expr
) => {{
- let logger = WithChannelContext::from(&$self.logger, &$chan.context, None);
- let chan_id = $chan.context.channel_id();
- let counterparty_node_id = $chan.context.get_counterparty_node_id();
- handle_new_monitor_update_internal!(
+ let (update_completed, all_updates_complete) = handle_new_monitor_update_internal(
$self,
+ &mut $peer_state.in_flight_monitor_updates,
+ $chan.context.channel_id(),
$funding_txo,
+ $chan.context.get_counterparty_node_id(),
$update,
- $peer_state,
- logger,
- chan_id,
- counterparty_node_id,
- {
- if $chan.blocked_monitor_updates_pending() == 0 {
- handle_monitor_update_completion!(
- $self,
- $peer_state_lock,
- $peer_state,
- $per_peer_state_lock,
- $chan
- );
- }
- }
- )
+ WithChannelContext::from(&$self.logger, &$chan.context, None),
+ );
+ if all_updates_complete && $chan.blocked_monitor_updates_pending() == 0 {
+ handle_monitor_update_completion!(
+ $self,
+ $peer_state_lock,
+ $peer_state,
+ $per_peer_state_lock,
+ $chan
+ );
+ }
+ update_completed
}};
}
Why this scored 15/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.