Force-close if finish closing_signed negotiation takes a full minute

lightning/src/ln/channel.rs

@@ -255,8 +255,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.
@@ -503,6 +501,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.
@@ -740,6 +745,7 @@ impl<Signer: Sign> Channel<Signer> {
 			commitment_secrets: CounterpartyCommitmentSecrets::new(),
 
 			channel_update_status: ChannelUpdateStatus::Enabled,
+			closing_signed_in_flight: false,
 
 			announcement_sigs: None,
 
@@ -1006,6 +1012,7 @@ impl<Signer: Sign> Channel<Signer> {
 			commitment_secrets: CounterpartyCommitmentSecrets::new(),
 
 			channel_update_status: ChannelUpdateStatus::Enabled,
+			closing_signed_in_flight: false,
 
 			announcement_sigs: None,
 
@@ -3413,16 +3420,38 @@ impl<Signer: Sign> Channel<Signer> {
 		self.closing_fee_limits.clone().unwrap()
 	}
 
+	/// 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 two timer ticks".to_owned()));
+			} else {
+				self.closing_signed_in_flight = true;
+			}
+		}
+		Ok(())
+	}
+
 	pub fn maybe_propose_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() ||
+		if self.last_sent_closing_fee.is_some() || !self.closing_negotiation_ready() {
-				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() {
 			return Ok((None, None));
 		}
 
@@ -5463,6 +5492,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

@@ -2715,6 +2715,20 @@ 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| {
+					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);
+					if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; }
+					if err.is_err() {
+						handle_errors.push((err, counterparty_node_id));
+					}
+					if !retain_channel { return false; }
+
+					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 { return false; }
+					}
+
 					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),
@@ -2741,20 +2755,13 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 						_ => {},
 					}
 
-					let counterparty_node_id = chan.get_counterparty_node_id();
+					true
-					let (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));
-					}
-					retain_channel
 				});
 			}
 
 			for (err, counterparty_node_id) in handle_errors.drain(..) {
 				let _ = handle_error!(self, err, counterparty_node_id);
 			}
-
 			should_persist
 		});
 	}