Force-close if finish closing_signed negotiation takes a full minute

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -246,8 +246,6 @@ enum ChannelState {
 	RemoteShutdownSent = 1 << 10,
 	/// Flag which is set on ChannelFunded or FundingSent after sending a shutdown message. At this
 	/// point, we may not add any new HTLCs to the channel.
-	/// TODO: Investigate some kind of timeout mechanism by which point the remote end must provide
-	/// us their shutdown.
 	LocalShutdownSent = 1 << 11,
 	/// We've successfully negotiated a closing_signed dance. At this point ChannelManager is about
 	/// to drop us, but we store this anyway.
@@ -486,6 +484,13 @@ pub(super) struct Channel<Signer: Sign> {
 	commitment_secrets: CounterpartyCommitmentSecrets,
 
 	channel_update_status: ChannelUpdateStatus,
+	/// Once we reach `closing_negotiation_ready`, we set this, indicating if closing_signed does
+	/// not complete within a single timer tick (one minute), we should force-close the channel.
+	/// This prevents us from keeping unusable channels around forever if our counterparty wishes
+	/// to DoS us.
+	/// Note that this field is reset to false on deserialization to give us a chance to connect to
+	/// our peer and start the closing_signed negotiation fresh.
+	closing_signed_in_flight: bool,
 
 	/// Our counterparty's channel_announcement signatures provided in announcement_signatures.
 	/// This can be used to rebroadcast the channel_announcement message later.
@@ -723,6 +728,7 @@ impl<Signer: Sign> Channel<Signer> {
 			commitment_secrets: CounterpartyCommitmentSecrets::new(),
 
 			channel_update_status: ChannelUpdateStatus::Enabled,
+			closing_signed_in_flight: false,
 
 			announcement_sigs: None,
 
@@ -984,6 +990,7 @@ impl<Signer: Sign> Channel<Signer> {
 			commitment_secrets: CounterpartyCommitmentSecrets::new(),
 
 			channel_update_status: ChannelUpdateStatus::Enabled,
+			closing_signed_in_flight: false,
 
 			announcement_sigs: None,
 
@@ -3321,16 +3328,38 @@ impl<Signer: Sign> Channel<Signer> {
 		self.closing_fee_limits = Some((proposed_total_fee_satoshis, proposed_max_total_fee_satoshis));
 	}
 
+	/// Returns true if we're ready to commence the closing_signed negotiation phase. This is true
+	/// after both sides have exchanged a `shutdown` message and all HTLCs have been drained. At
+	/// this point if we're the funder we should send the initial closing_signed, and in any case
+	/// shutdown should complete within a reasonable timeframe.
+	fn closing_negotiation_ready(&self) -> bool {
+		self.pending_inbound_htlcs.is_empty() && self.pending_outbound_htlcs.is_empty() &&
+			self.channel_state &
+				(BOTH_SIDES_SHUTDOWN_MASK | ChannelState::AwaitingRemoteRevoke as u32 |
+				 ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32)
+				== BOTH_SIDES_SHUTDOWN_MASK &&
+			self.pending_update_fee.is_none()
+	}
+
+	/// Checks if the closing_signed negotiation is making appropriate progress, possibly returning
+	/// an Err if no progress is being made and the channel should be force-closed instead.
+	/// Should be called on a one-minute timer.
+	pub fn timer_check_closing_negotiation_progress(&mut self) -> Result<(), ChannelError> {
+		if self.closing_negotiation_ready() {
+			if self.closing_signed_in_flight {
+				return Err(ChannelError::Close("closing_signed negotiation failed to finish within one minute".to_owned()));
+			} else {
+				self.closing_signed_in_flight = true;
+			}
+		}
+		Ok(())
+	}
+
 	pub fn maybe_propose_first_closing_signed<F: Deref, L: Deref>(&mut self, fee_estimator: &F, logger: &L)
 		-> Result<(Option<msgs::ClosingSigned>, Option<Transaction>), ChannelError>
 		where F::Target: FeeEstimator, L::Target: Logger
 	{
-		if !self.pending_inbound_htlcs.is_empty() || !self.pending_outbound_htlcs.is_empty() ||
-				self.channel_state &
-					(BOTH_SIDES_SHUTDOWN_MASK | ChannelState::AwaitingRemoteRevoke as u32 |
-					 ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32)
-				!= BOTH_SIDES_SHUTDOWN_MASK ||
-				self.last_sent_closing_fee.is_some() || self.pending_update_fee.is_some() {
+		if self.last_sent_closing_fee.is_some() || !self.closing_negotiation_ready() {
 			return Ok((None, None));
 		}
 
@@ -5338,6 +5367,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 			commitment_secrets,
 
 			channel_update_status,
+			closing_signed_in_flight: false,
 
 			announcement_sigs,
 

lightning/src/ln/channelmanager.rs

@@ -2694,46 +2694,56 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				let pending_msg_events = &mut channel_state.pending_msg_events;
 				let short_to_id = &mut channel_state.short_to_id;
 				channel_state.by_id.retain(|chan_id, chan| {
-					match chan.channel_update_status() {
-						ChannelUpdateStatus::Enabled if !chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::DisabledStaged),
-						ChannelUpdateStatus::Disabled if chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::EnabledStaged),
-						ChannelUpdateStatus::DisabledStaged if chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::Enabled),
-						ChannelUpdateStatus::EnabledStaged if !chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::Disabled),
-						ChannelUpdateStatus::DisabledStaged if !chan.is_live() => {
-							if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
-								pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
-									msg: update
-								});
-							}
-							should_persist = NotifyOption::DoPersist;
-							chan.set_channel_update_status(ChannelUpdateStatus::Disabled);
-						},
-						ChannelUpdateStatus::EnabledStaged if chan.is_live() => {
-							if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
-								pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
-									msg: update
-								});
-							}
-							should_persist = NotifyOption::DoPersist;
-							chan.set_channel_update_status(ChannelUpdateStatus::Enabled);
-						},
-						_ => {},
-					}
-
 					let counterparty_node_id = chan.get_counterparty_node_id();
-					let (retain_channel, chan_needs_persist, err) = self.update_channel_fee(short_to_id, pending_msg_events, chan_id, chan, new_feerate);
+					let (mut retain_channel, chan_needs_persist, err) = self.update_channel_fee(short_to_id, pending_msg_events, chan_id, chan, new_feerate);
 					if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; }
 					if err.is_err() {
 						handle_errors.push((err, counterparty_node_id));
 					}
+
+					if retain_channel {
+						if let Err(e) = chan.timer_check_closing_negotiation_progress() {
+							let (needs_close, err) = convert_chan_err!(self, e, short_to_id, chan, chan_id);
+							handle_errors.push((Err(err), chan.get_counterparty_node_id()));
+							if needs_close { retain_channel = false; }
+						}
+					}
+
+					if retain_channel {
+						match chan.channel_update_status() {
+							ChannelUpdateStatus::Enabled if !chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::DisabledStaged),
+							ChannelUpdateStatus::Disabled if chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::EnabledStaged),
+							ChannelUpdateStatus::DisabledStaged if chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::Enabled),
+							ChannelUpdateStatus::EnabledStaged if !chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::Disabled),
+							ChannelUpdateStatus::DisabledStaged if !chan.is_live() => {
+								if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
+									pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
+										msg: update
+									});
+								}
+								should_persist = NotifyOption::DoPersist;
+								chan.set_channel_update_status(ChannelUpdateStatus::Disabled);
+							},
+							ChannelUpdateStatus::EnabledStaged if chan.is_live() => {
+								if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
+									pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
+										msg: update
+									});
+								}
+								should_persist = NotifyOption::DoPersist;
+								chan.set_channel_update_status(ChannelUpdateStatus::Enabled);
+							},
+							_ => {},
+						}
+					}
+
 					retain_channel
 				});
 			}
 
 			for (err, counterparty_node_id) in handle_errors.drain(..) {
 				let _ = handle_error!(self, err, counterparty_node_id);
 			}
-
 			should_persist
 		});