Add auto-close if outbound feerate update don't complete

Author: Antoine Riard

lightning/src/ln/channel.rs

@@ -370,6 +370,12 @@ pub const FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE: u64 = 2;
 #[cfg(not(fuzzing))]
 const FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE: u64 = 2;
 
+/// If after this value tick periods, the channel feerate isn't satisfying, we auto-close the
+/// channel and goes on-chain to avoid unsafe situations where a commitment transaction with
+/// time-sensitive outputs won't confirm due to staling feerate too far away from the upper feerate
+/// groups of network mempools.
+const AUTOCLOSE_TIMEOUT: u16 = 6;
+
 // TODO: We should refactor this to be an Inbound/OutboundChannel until initial setup handshaking
 // has been completed, and then turn into a Channel to get compiler-time enforcement of things like
 // calling channel_id() before we're set up or things like get_outbound_funding_signed on an
@@ -392,6 +398,10 @@ pub(super) struct Channel<Signer: Sign> {
 
 	latest_monitor_update_id: u64,
 
+	// Auto-close timer, if the channel is outbound, and we didn't receive a RAA for counterparty
+	// commitment transaction after `AUTOCLOSE_TIMEOUT` periods, this channel must be force-closed.
+	autoclose_timer: u16,
+
 	holder_signer: Signer,
 	shutdown_scriptpubkey: Option<ShutdownScript>,
 	destination_script: Script,
@@ -682,6 +692,8 @@ impl<Signer: Sign> Channel<Signer> {
 
 			latest_monitor_update_id: 0,
 
+			autoclose_timer: 0,
+
 			holder_signer,
 			shutdown_scriptpubkey,
 			destination_script: keys_provider.get_destination_script(),
@@ -945,6 +957,8 @@ impl<Signer: Sign> Channel<Signer> {
 
 			latest_monitor_update_id: 0,
 
+			autoclose_timer: 0,
+
 			holder_signer,
 			shutdown_scriptpubkey,
 			destination_script: keys_provider.get_destination_script(),
@@ -2720,6 +2734,18 @@ impl<Signer: Sign> Channel<Signer> {
 		}
 	}
 
+	/// Trigger the autoclose timer if it's in the starting position
+	fn maybe_trigger_autoclose_timer(&mut self) {
+		// Start an auto-close timer, if the channel feerate doesn't increase before its
+		// expiration (i.e this outbound feerate update has been committed on both sides),
+		// the channel will be marked as unsafe and force-closed.
+		// If a timer is already pending, no-op, as a higher-feerate `update_fee` will
+		// implicitly override a lower-feerate `update_fee` part of the same update sequence.
+		if self.autoclose_timer == 0 {
+			self.autoclose_timer = 1;
+		}
+	}
+
 	/// Handles receiving a remote's revoke_and_ack. Note that we may return a new
 	/// commitment_signed message here in case we had pending outbound HTLCs to add which were
 	/// waiting on this revoke_and_ack. The generation of this new commitment_signed may also fail,
@@ -2878,6 +2904,7 @@ impl<Signer: Sign> Channel<Signer> {
 					log_trace!(logger, " ...promoting outbound fee update {} to Committed", feerate);
 					self.feerate_per_kw = feerate;
 					self.pending_update_fee = None;
+					self.autoclose_timer = 0;
 				},
 				FeeUpdateState::RemoteAnnounced => { debug_assert!(!self.is_outbound()); },
 				FeeUpdateState::AwaitingRemoteRevokeToAnnounce => {
@@ -2974,6 +3001,8 @@ impl<Signer: Sign> Channel<Signer> {
 			return None;
 		}
 
+		self.maybe_trigger_autoclose_timer();
+
 		debug_assert!(self.pending_update_fee.is_none());
 		self.pending_update_fee = Some((feerate_per_kw, FeeUpdateState::Outbound));
 
@@ -3163,6 +3192,22 @@ impl<Signer: Sign> Channel<Signer> {
 		Ok(())
 	}
 
+	/// If the auto-close timer is reached following the triggering of a auto-close condition
+	/// (i.e a non-satisfying feerate to ensure efficient confirmation), we force-close
+	/// channel, hopefully narrowing the safety risks for the user funds.
+	pub fn check_autoclose(&mut self) -> Result<(), ChannelError> {
+		if self.autoclose_timer	> 0 && self.autoclose_timer < AUTOCLOSE_TIMEOUT {
+			self.autoclose_timer += 1;
+		}
+		if self.autoclose_timer == AUTOCLOSE_TIMEOUT {
+			// If the channel doesn't have pending HTLC outputs to claim on-chain
+			if self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len() > 0 {
+				return Err(ChannelError::Close("Channel has time-sensitive outputs and the auto-close timer has been reached".to_owned()));
+			}
+		}
+		Ok(())
+	}
+
 	fn get_last_revoke_and_ack(&self) -> msgs::RevokeAndACK {
 		let next_per_commitment_point = self.holder_signer.get_per_commitment_point(self.cur_holder_commitment_transaction_number, &self.secp_ctx);
 		let per_commitment_secret = self.holder_signer.release_commitment_secret(self.cur_holder_commitment_transaction_number + 2);
@@ -5178,6 +5223,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 			(5, self.config, required),
 			(7, self.shutdown_scriptpubkey, option),
 			(9, self.target_closing_feerate_sats_per_kw, option),
+			(11, self.autoclose_timer, required),
 		});
 
 		Ok(())
@@ -5411,13 +5457,15 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 
 		let mut announcement_sigs = None;
 		let mut target_closing_feerate_sats_per_kw = None;
+		let mut autoclose_timer = 0;
 		read_tlv_fields!(reader, {
 			(0, announcement_sigs, option),
 			(1, minimum_depth, option),
 			(3, counterparty_selected_channel_reserve_satoshis, option),
 			(5, config, option), // Note that if none is provided we will *not* overwrite the existing one.
 			(7, shutdown_scriptpubkey, option),
 			(9, target_closing_feerate_sats_per_kw, option),
+			(11, autoclose_timer, required),
 		});
 
 		let mut secp_ctx = Secp256k1::new();
@@ -5434,6 +5482,8 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 
 			latest_monitor_update_id,
 
+			autoclose_timer,
+
 			holder_signer,
 			shutdown_scriptpubkey,
 			destination_script,

lightning/src/ln/channelmanager.rs

@@ -626,6 +626,7 @@ const CHECK_CLTV_EXPIRY_SANITY: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRA
 #[allow(dead_code)]
 const CHECK_CLTV_EXPIRY_SANITY_2: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - 2*CLTV_CLAIM_BUFFER;
 
+
 /// Channel parameters which apply to our counterparty. These are split out from [`ChannelDetails`]
 /// to better separate parameters.
 #[derive(Clone, Debug, PartialEq)]
@@ -2660,6 +2661,23 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		(retain_channel, NotifyOption::DoPersist, ret_err)
 	}
 
+	fn check_channel_autoclose(&self, short_to_id: &mut HashMap<u64, [u8; 32]>, chan_id: &[u8; 32], chan: &mut Channel<Signer>) -> (bool, NotifyOption, Result<(), MsgHandleErrInternal>) {
+		let mut retain_channel = true;
+		let mut notify_option = NotifyOption::SkipPersist;
+		let res = match chan.check_autoclose() {
+			Ok(res) => Ok(res),
+			Err(e) => {
+				let (drop, res) = convert_chan_err!(self, e, short_to_id, chan, chan_id);
+				if drop {
+					retain_channel = false;
+					notify_option = NotifyOption::DoPersist;
+				}
+				Err(res)
+			}
+		};
+		(retain_channel, notify_option, res)
+	}
+
 	#[cfg(fuzzing)]
 	/// In chanmon_consistency we want to sometimes do the channel fee updates done in
 	/// timer_tick_occurred, but we can't generate the disabled channel updates as it considers
@@ -2716,6 +2734,14 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				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.check_channel_autoclose(short_to_id, chan_id, chan);
+					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; }
+
 					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() {