From d450e0fb2c73d4310bf63aaac6fbf5217cc9bf27 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 4 Jul 2023 15:13:32 +0000 Subject: [PATCH 1/3] Handle monitor completion actions for closed channels If a channel has been closed, there may still be some `ChannelMonitorUpdate`(s) which are pending completion. These in-flight updates may also be blocking another channel from letting an update fly, e.g. for forwarded payments where the payment preimage will be removed from the downstream channel after the upstream channel has closed. Luckily all the infrastructure to handle this case is already in place - we just need to process the `monitor_update_blocked_actions` for closed channels. --- lightning/src/ln/channelmanager.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 61895544e8f..d4d1a926825 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -5001,24 +5001,29 @@ where if peer_state_mutex_opt.is_none() { return } peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap(); let peer_state = &mut *peer_state_lock; - let mut channel = { - match peer_state.channel_by_id.entry(funding_txo.to_channel_id()){ - hash_map::Entry::Occupied(chan) => chan, - hash_map::Entry::Vacant(_) => return, - } - }; + let channel = + if let Some(chan) = peer_state.channel_by_id.get_mut(&funding_txo.to_channel_id()) { + chan + } else { + let update_actions = peer_state.monitor_update_blocked_actions + .remove(&funding_txo.to_channel_id()).unwrap_or(Vec::new()); + mem::drop(peer_state_lock); + mem::drop(per_peer_state); + self.handle_monitor_update_completion_actions(update_actions); + return; + }; let remaining_in_flight = if let Some(pending) = peer_state.in_flight_monitor_updates.get_mut(funding_txo) { pending.retain(|upd| upd.update_id > highest_applied_update_id); pending.len() } else { 0 }; log_trace!(self.logger, "ChannelMonitor updated to {}. Current highest is {}. {} pending in-flight updates.", - highest_applied_update_id, channel.get().context.get_latest_monitor_update_id(), + highest_applied_update_id, channel.context.get_latest_monitor_update_id(), remaining_in_flight); - if !channel.get().is_awaiting_monitor_update() || channel.get().context.get_latest_monitor_update_id() != highest_applied_update_id { + if !channel.is_awaiting_monitor_update() || channel.context.get_latest_monitor_update_id() != highest_applied_update_id { return; } - handle_monitor_update_completion!(self, peer_state_lock, peer_state, per_peer_state, channel.get_mut()); + handle_monitor_update_completion!(self, peer_state_lock, peer_state, per_peer_state, channel); } /// Accepts a request to open a channel after a [`Event::OpenChannelRequest`]. From f9521a4bdaa4cfdaf08f6797d1a84e96475c9f49 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 5 Jul 2023 02:15:42 +0000 Subject: [PATCH 2/3] Run monitor update completion actions for pre-startup completion If a `ChannelMonitorUpdate` completes being persisted, but the `ChannelManager` isn't informed thereof (or isn't persisted) before shutdown, on startup we may still have it listed as in-flight. When we compare the available `ChannelMonitor` with the in-flight set, we'll notice it completed and remove it, however this may leave some post-update actions dangling which need to complete. Here we handle this with a new `BackgroundEvent` indicating we need to handle any post-update action(s) for a given channel. --- lightning/src/ln/chanmon_update_fail_tests.rs | 15 ++++++--- lightning/src/ln/channelmanager.rs | 33 +++++++++++++++++++ 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index 707c27908ed..8d4c40b64d0 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -2346,6 +2346,7 @@ fn do_channel_holding_cell_serialize(disconnect: bool, reload_a: bool) { RecipientOnionFields::secret_only(payment_secret_2), PaymentId(payment_hash_2.0)).unwrap(); check_added_monitors!(nodes[0], 0); + let chan_0_monitor_serialized = get_monitor!(nodes[0], chan_id).encode(); chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); nodes[0].node.claim_funds(payment_preimage_0); @@ -2365,8 +2366,9 @@ fn do_channel_holding_cell_serialize(disconnect: bool, reload_a: bool) { // disconnect the peers. Note that the fuzzer originally found this issue because // deserializing a ChannelManager in this state causes an assertion failure. if reload_a { - let chan_0_monitor_serialized = get_monitor!(nodes[0], chan_id).encode(); reload_node!(nodes[0], &nodes[0].node.encode(), &[&chan_0_monitor_serialized], persister, new_chain_monitor, nodes_0_deserialized); + persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); } else { nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id()); } @@ -2406,9 +2408,14 @@ fn do_channel_holding_cell_serialize(disconnect: bool, reload_a: bool) { assert_eq!(pending_cs.commitment_signed, cs); } else { panic!(); } - // There should be no monitor updates as we are still pending awaiting a failed one. - check_added_monitors!(nodes[0], 0); - check_added_monitors!(nodes[1], 0); + if reload_a { + // The two pending monitor updates were replayed (but are still pending). + check_added_monitors(&nodes[0], 2); + } else { + // There should be no monitor updates as we are still pending awaiting a failed one. + check_added_monitors(&nodes[0], 0); + } + check_added_monitors(&nodes[1], 0); } // If we finish updating the monitor, we should free the holding cell right away (this did diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index d4d1a926825..b4eed11fc39 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -530,6 +530,13 @@ enum BackgroundEvent { funding_txo: OutPoint, update: ChannelMonitorUpdate }, + /// Some [`ChannelMonitorUpdate`] (s) completed before we were serialized but we still have + /// them marked pending, thus we need to run any [`MonitorUpdateCompletionAction`] (s) pending + /// on a channel. + MonitorUpdatesComplete { + counterparty_node_id: PublicKey, + channel_id: [u8; 32], + }, } #[derive(Debug)] @@ -4191,6 +4198,22 @@ where } let _ = handle_error!(self, res, counterparty_node_id); }, + BackgroundEvent::MonitorUpdatesComplete { counterparty_node_id, channel_id } => { + let per_peer_state = self.per_peer_state.read().unwrap(); + if let Some(peer_state_mutex) = per_peer_state.get(&counterparty_node_id) { + let mut peer_state_lock = peer_state_mutex.lock().unwrap(); + let peer_state = &mut *peer_state_lock; + if let Some(chan) = peer_state.channel_by_id.get_mut(&channel_id) { + handle_monitor_update_completion!(self, peer_state_lock, peer_state, per_peer_state, chan); + } else { + 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); + self.handle_monitor_update_completion_actions(update_actions); + } + } + }, } } NotifyOption::DoPersist @@ -8518,6 +8541,16 @@ where update: update.clone(), }); } + if $chan_in_flight_upds.is_empty() { + // We had some updates to apply, but it turns out they had completed before we + // were serialized, we just weren't notified of that. Thus, we may have to run + // the completion actions for any monitor updates, but otherwise are done. + pending_background_events.push( + BackgroundEvent::MonitorUpdatesComplete { + counterparty_node_id: $counterparty_node_id, + channel_id: $funding_txo.to_channel_id(), + }); + } if $peer_state.in_flight_monitor_updates.insert($funding_txo, $chan_in_flight_upds).is_some() { log_error!(args.logger, "Duplicate in-flight monitor update set for the same channel!"); return Err(DecodeError::InvalidValue); From 550cf91439a2aa2befe89704e13c781800e8612e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 5 Jul 2023 17:06:19 +0000 Subject: [PATCH 3/3] Add comment describing when a completion action can be discarded In an older PR a reviewer had asked why the discarding of a channel being blocked on another monitor update is okay if the blocked channel has since closed. At the time, this was not actually okay - the monitor updates in the channel weren't moved to the `ChannelManager` on close so the whole pipeline was busted, but with the changes in 4041f0899f86eaf6a0a4576a91918fa54026ac46 the handling of channel closes with pending monitor updates is now correct, and so is the existing code block. --- lightning/src/ln/channelmanager.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index b4eed11fc39..7ec75e20fa3 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -8943,6 +8943,12 @@ where blocked_peer_state.lock().unwrap().actions_blocking_raa_monitor_updates .entry(blocked_channel_outpoint.to_channel_id()) .or_insert_with(Vec::new).push(blocking_action.clone()); + } else { + // If the channel we were blocking has closed, we don't need to + // worry about it - the blocked monitor update should never have + // been released from the `Channel` object so it can't have + // completed, and if the channel closed there's no reason to bother + // anymore. } } }