From 28af6c723b011b98fb6022d0c0c19eced2512b93 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Wed, 1 Feb 2023 11:12:38 -0600 Subject: [PATCH 1/4] Don't rebroadcast closing TX upon restart While we don't rebroadcast the latest holder commitment transactions in `ChannelMonitor::update_monitor()` anymore if we had previously seen a closing tx on-chain, we still do so upon restarting / deserlization of `ChannelManager` With this change, we now also refrain from rebroadcasting upon restarting. --- lightning/src/chain/channelmonitor.rs | 21 +++++++++++++++------ lightning/src/ln/channelmanager.rs | 5 ++++- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 509ebed6fba..3c8cf97f372 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1236,6 +1236,11 @@ impl ChannelMonitor { self.inner.lock().unwrap().get_funding_txo().clone() } + /// Gets whether we've seen a confirmed transaction spending the funding output. + pub(crate) fn detected_funding_spend(&self) -> bool { + self.inner.lock().unwrap().detected_funding_spend() + } + /// Gets a list of txids, with their output scripts (in the order they appear in the /// transaction), which we must learn about spends of via block_connected(). pub fn get_outputs_to_watch(&self) -> Vec<(Txid, Vec<(u32, Script)>)> { @@ -2275,12 +2280,7 @@ impl ChannelMonitorImpl { // There's no need to broadcast our commitment transaction if we've seen one // confirmed (even with 1 confirmation) as it'll be rejected as // duplicate/conflicting. - let detected_funding_spend = self.funding_spend_confirmed.is_some() || - self.onchain_events_awaiting_threshold_conf.iter().find(|event| match event.event { - OnchainEvent::FundingSpendConfirmation { .. } => true, - _ => false, - }).is_some(); - if detected_funding_spend { + if self.detected_funding_spend() { continue; } self.broadcast_latest_holder_commitment_txn(broadcaster, logger); @@ -2338,6 +2338,15 @@ impl ChannelMonitorImpl { &self.funding_info } + pub fn detected_funding_spend(&self) -> bool { + self.funding_spend_confirmed.is_some() || + self.onchain_events_awaiting_threshold_conf + .iter().find(|event| match event.event { + OnchainEvent::FundingSpendConfirmation { .. } => true, + _ => false, + }).is_some() + } + pub fn get_outputs_to_watch(&self) -> &HashMap> { // If we've detected a counterparty commitment tx on chain, we must include it in the set // of outputs to watch for spends of, otherwise we're likely to lose user funds. Because diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index b2a23c7c1d4..fb060b63fae 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -7208,7 +7208,10 @@ where } for (funding_txo, monitor) in args.channel_monitors.iter_mut() { - if !funding_txo_set.contains(funding_txo) { + // There's no need to broadcast our commitment transaction if we've seen one + // confirmed (even with 1 confirmation) as it'll be rejected as + // duplicate/conflicting. + if !funding_txo_set.contains(funding_txo) && !monitor.detected_funding_spend() { log_info!(args.logger, "Broadcasting latest holder commitment transaction for closed channel {}", log_bytes!(funding_txo.to_channel_id())); monitor.broadcast_latest_holder_commitment_txn(&args.tx_broadcaster, &args.logger); } From b7462fd196d707c27cc1ae02a9d50066d26ccc4f Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Wed, 1 Feb 2023 11:23:03 -0600 Subject: [PATCH 2/4] f Indent --- lightning/src/chain/channelmonitor.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 3c8cf97f372..4fa543d16d3 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -2339,12 +2339,12 @@ impl ChannelMonitorImpl { } pub fn detected_funding_spend(&self) -> bool { - self.funding_spend_confirmed.is_some() || - self.onchain_events_awaiting_threshold_conf - .iter().find(|event| match event.event { - OnchainEvent::FundingSpendConfirmation { .. } => true, - _ => false, - }).is_some() + self.funding_spend_confirmed.is_some() || + self.onchain_events_awaiting_threshold_conf + .iter().find(|event| match event.event { + OnchainEvent::FundingSpendConfirmation { .. } => true, + _ => false, + }).is_some() } pub fn get_outputs_to_watch(&self) -> &HashMap> { From 65a1cfac85b6970aa93efc92cae26f6032205ea4 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Wed, 1 Feb 2023 11:40:38 -0600 Subject: [PATCH 3/4] f Avoid back-to-back locking --- lightning/src/chain/channelmonitor.rs | 19 ++++++++++++++----- lightning/src/ln/channelmanager.rs | 10 +++++----- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 4fa543d16d3..88c572d95ab 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1206,6 +1206,20 @@ impl ChannelMonitor { self.inner.lock().unwrap().broadcast_latest_holder_commitment_txn(broadcaster, logger); } + pub(crate) fn broadcast_latest_holder_commitment_txn_if_unspent( + &self, + broadcaster: &B, + logger: &L, + ) where + B::Target: BroadcasterInterface, + L::Target: Logger, + { + let mut locked_inner = self.inner.lock().unwrap(); + if !locked_inner.detected_funding_spend() { + locked_inner.broadcast_latest_holder_commitment_txn(broadcaster, logger); + } + } + /// Updates a ChannelMonitor on the basis of some new information provided by the Channel /// itself. /// @@ -1236,11 +1250,6 @@ impl ChannelMonitor { self.inner.lock().unwrap().get_funding_txo().clone() } - /// Gets whether we've seen a confirmed transaction spending the funding output. - pub(crate) fn detected_funding_spend(&self) -> bool { - self.inner.lock().unwrap().detected_funding_spend() - } - /// Gets a list of txids, with their output scripts (in the order they appear in the /// transaction), which we must learn about spends of via block_connected(). pub fn get_outputs_to_watch(&self) -> Vec<(Txid, Vec<(u32, Script)>)> { diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index fb060b63fae..afcd98b4c96 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -7208,12 +7208,12 @@ where } for (funding_txo, monitor) in args.channel_monitors.iter_mut() { - // There's no need to broadcast our commitment transaction if we've seen one - // confirmed (even with 1 confirmation) as it'll be rejected as - // duplicate/conflicting. - if !funding_txo_set.contains(funding_txo) && !monitor.detected_funding_spend() { + if !funding_txo_set.contains(funding_txo) { log_info!(args.logger, "Broadcasting latest holder commitment transaction for closed channel {}", log_bytes!(funding_txo.to_channel_id())); - monitor.broadcast_latest_holder_commitment_txn(&args.tx_broadcaster, &args.logger); + // There's no need to broadcast our commitment transaction if + // we've seen one confirmed (even with 1 confirmation) as it'll + // be rejected as duplicate/conflicting. + monitor.broadcast_latest_holder_commitment_txn_if_unspent(&args.tx_broadcaster, &args.logger); } } From c53c5a03ad95189e9161e78ddf23187caa1678a0 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Wed, 1 Feb 2023 14:14:59 -0600 Subject: [PATCH 4/4] f Log only on broadcast attempt --- lightning/src/chain/channelmonitor.rs | 2 ++ lightning/src/ln/channelmanager.rs | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 88c572d95ab..7260f6e7e52 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1216,6 +1216,8 @@ impl ChannelMonitor { { let mut locked_inner = self.inner.lock().unwrap(); if !locked_inner.detected_funding_spend() { + log_info!(logger, "Broadcasting latest holder commitment transaction for channel {}", + log_bytes!(locked_inner.get_funding_txo().0.to_channel_id())); locked_inner.broadcast_latest_holder_commitment_txn(broadcaster, logger); } } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index afcd98b4c96..5583736076b 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -7209,7 +7209,6 @@ where for (funding_txo, monitor) in args.channel_monitors.iter_mut() { if !funding_txo_set.contains(funding_txo) { - log_info!(args.logger, "Broadcasting latest holder commitment transaction for closed channel {}", log_bytes!(funding_txo.to_channel_id())); // There's no need to broadcast our commitment transaction if // we've seen one confirmed (even with 1 confirmation) as it'll // be rejected as duplicate/conflicting.