From 9878edeebaae6a33f08c609896d78c1d471a7577 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 4 May 2023 15:16:17 -0700 Subject: [PATCH 1/2] Prevent ChannelForceClosed monitor update error after detecting spend If we detected a spend for a channel onchain prior to handling its `ChannelForceClosed` monitor update, we'd log a concerning error message and return an error unnecessarily. The channel has already been closed, so handling the `ChannelForceClosed` monitor update at this point should be a no-op. --- lightning/src/chain/channelmonitor.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 7d1a325c7..bf3008774 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -2457,7 +2457,9 @@ impl ChannelMonitorImpl { self.latest_update_id = updates.update_id; - if ret.is_ok() && self.funding_spend_seen { + // Refuse updates after we've detected a spend onchain, but only if we haven't processed a + // force closed monitor update yet. + if ret.is_ok() && self.funding_spend_seen && self.latest_update_id != CLOSED_CHANNEL_UPDATE_ID { log_error!(logger, "Refusing Channel Monitor Update as counterparty attempted to update commitment after funding was spent"); Err(()) } else { ret } From 1aeb8216e1c6b357762a9be5f639fdc2c076a647 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 4 May 2023 15:37:25 -0700 Subject: [PATCH 2/2] Improve logging around redundant force close monitor updates --- lightning/src/chain/channelmonitor.rs | 13 +++++++++++-- lightning/src/ln/channelmanager.rs | 2 ++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index bf3008774..e3be6bff2 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -2339,8 +2339,16 @@ impl ChannelMonitorImpl { F::Target: FeeEstimator, L::Target: Logger, { - log_info!(logger, "Applying update to monitor {}, bringing update_id from {} to {} with {} changes.", - log_funding_info!(self), self.latest_update_id, updates.update_id, updates.updates.len()); + if self.latest_update_id == CLOSED_CHANNEL_UPDATE_ID && updates.update_id == CLOSED_CHANNEL_UPDATE_ID { + log_info!(logger, "Applying post-force-closed update to monitor {} with {} change(s).", + log_funding_info!(self), updates.updates.len()); + } else if updates.update_id == CLOSED_CHANNEL_UPDATE_ID { + log_info!(logger, "Applying force close update to monitor {} with {} change(s).", + log_funding_info!(self), updates.updates.len()); + } else { + log_info!(logger, "Applying update to monitor {}, bringing update_id from {} to {} with {} change(s).", + log_funding_info!(self), self.latest_update_id, updates.update_id, updates.updates.len()); + } // ChannelMonitor updates may be applied after force close if we receive a preimage for a // broadcasted commitment transaction HTLC output that we'd like to claim on-chain. If this // is the case, we no longer have guaranteed access to the monitor's update ID, so we use a @@ -2407,6 +2415,7 @@ impl ChannelMonitorImpl { _ => false, }).is_some(); if detected_funding_spend { + log_trace!(logger, "Avoiding commitment broadcast, already detected confirmed spend onchain"); continue; } self.broadcast_latest_holder_commitment_txn(broadcaster, logger); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 3bf215d0a..b18e7a8f2 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -7756,6 +7756,8 @@ where for (funding_txo, _) in args.channel_monitors.iter() { if !funding_txo_set.contains(funding_txo) { + log_info!(args.logger, "Queueing monitor update to ensure missing channel {} is force closed", + log_bytes!(funding_txo.to_channel_id())); let monitor_update = ChannelMonitorUpdate { update_id: CLOSED_CHANNEL_UPDATE_ID, updates: vec![ChannelMonitorUpdateStep::ChannelForceClosed { should_broadcast: true }],