Add auto-close if outbound feerate don't complete

According to the lightning security model, commitment transactions
must offer a compelling feerate at anytime to maximize odds of
block inclusion, and thus claim on-chain time-sensitive HTLCs.

This fee efficiency is currently guarantee through the update_fee
mechanism, a cooperative update of the channel feerate. However,
if the counterparty is malicious or offline, this update might not
succeed, therefore exposing our balance at risk.

To lighten this risk, we add a new "auto-close" mechanism. If we
detect that a update_fee should have been committed but never
effectively finalized, we start a timer. At expiration, the
channel is force-close, if they're pending *included HTLCs.

We choose 60 ticks as "auto-close" timer duration. The mechanism
can be disabled through `should_autoclose` (default: true)

Author: Antoine Riard

lightning/src/ln/channel.rs

@@ -295,6 +295,7 @@ struct HTLCStats {
 	on_holder_tx_dust_exposure_msat: u64,
 	holding_cell_msat: u64,
 	on_holder_tx_holding_cell_htlcs_count: u32, // dust HTLCs *non*-included
+	on_holder_tx_included_htlcs: u32,
 }
 
 /// An enum gathering stats on commitment transaction, either local or remote.
@@ -413,6 +414,12 @@ pub(crate) const CONCURRENT_INBOUND_HTLC_FEE_BUFFER: u32 = 2;
 /// transaction (not counting the value of the HTLCs themselves).
 pub(crate) const MIN_AFFORDABLE_HTLC_COUNT: usize = 4;
 
+/// 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 = 60;
+
 // 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
@@ -435,6 +442,14 @@ pub(super) struct Channel<Signer: Sign> {
 
 	latest_monitor_update_id: u64,
 
+	// Auto-close timer, if the channel is outbound, we sent a `update_fee`, and we didn't
+	// receive a RAA from counterparty committing this state after `AUTOCLOSE_TIMEOUT` periods,
+	// this channel must be force-closed.
+	// If the channel is inbound, it has been observed the channel feerate should increase,
+	// and we didn't receive a RAA from counterparty committing an `update_fee` after
+	// `AUTOCLOSE_TIMEOUT` periods, this channel must be force-closed.
+	autoclose_timer: u16,
+
 	holder_signer: Signer,
 	shutdown_scriptpubkey: Option<ShutdownScript>,
 	destination_script: Script,
@@ -747,6 +762,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(),
@@ -1037,6 +1054,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(),
@@ -2042,6 +2061,7 @@ impl<Signer: Sign> Channel<Signer> {
 			on_holder_tx_dust_exposure_msat: 0,
 			holding_cell_msat: 0,
 			on_holder_tx_holding_cell_htlcs_count: 0,
+			on_holder_tx_included_htlcs: 0,
 		};
 
 		let counterparty_dust_limit_timeout_sat = (self.get_dust_buffer_feerate(outbound_feerate_update) as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis;
@@ -2053,6 +2073,8 @@ impl<Signer: Sign> Channel<Signer> {
 			}
 			if htlc.amount_msat / 1000 < holder_dust_limit_success_sat {
 				stats.on_holder_tx_dust_exposure_msat += htlc.amount_msat;
+			} else {
+				stats.on_holder_tx_included_htlcs += 1;
 			}
 		}
 		stats
@@ -2067,6 +2089,7 @@ impl<Signer: Sign> Channel<Signer> {
 			on_holder_tx_dust_exposure_msat: 0,
 			holding_cell_msat: 0,
 			on_holder_tx_holding_cell_htlcs_count: 0,
+			on_holder_tx_included_htlcs: 0,
 		};
 
 		let counterparty_dust_limit_success_sat = (self.get_dust_buffer_feerate(outbound_feerate_update) as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis;
@@ -2078,6 +2101,8 @@ impl<Signer: Sign> Channel<Signer> {
 			}
 			if htlc.amount_msat / 1000 < holder_dust_limit_timeout_sat {
 				stats.on_holder_tx_dust_exposure_msat += htlc.amount_msat;
+			} else {
+				stats.on_holder_tx_included_htlcs += 1;
 			}
 		}
 
@@ -2832,13 +2857,33 @@ impl<Signer: Sign> Channel<Signer> {
 		}
 	}
 
+	/// Trigger the autoclose timer if it's in the starting position
+	pub fn maybe_trigger_autoclose_timer(&mut self, current_feerate: u32, new_feerate: u32) -> bool {
+
+		// Verify the new feerate is at least superior by 30% of current feerate before to
+		// start a autoclose timer and current feerate has fallen behind background feerate.
+		if new_feerate > current_feerate * 1300 / 1000 {
+			return false;
+		}
+
+		// 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;
+			return true;
+		}
+		return false;
+	}
+
 	/// 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,
 	/// generating an appropriate error *after* the channel state has been updated based on the
 	/// revoke_and_ack message.
-	pub fn revoke_and_ack<L: Deref>(&mut self, msg: &msgs::RevokeAndACK, logger: &L) -> Result<RAAUpdates, ChannelError>
-		where L::Target: Logger,
+	pub fn revoke_and_ack<L: Deref>(&mut self, msg: &msgs::RevokeAndACK, logger: &L) -> Result<RAAUpdates, ChannelError> where L::Target: Logger
 	{
 		if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) {
 			return Err(ChannelError::Close("Got revoke/ACK message when channel was not in an operational state".to_owned()));
@@ -2999,6 +3044,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 => {
@@ -3007,6 +3053,7 @@ impl<Signer: Sign> Channel<Signer> {
 					require_commitment = true;
 					self.feerate_per_kw = feerate;
 					self.pending_update_fee = None;
+					self.autoclose_timer = 0;
 				},
 			}
 		}
@@ -3145,6 +3192,8 @@ impl<Signer: Sign> Channel<Signer> {
 			return None;
 		}
 
+		self.maybe_trigger_autoclose_timer(self.get_feerate(), feerate_per_kw);
+
 		debug_assert!(self.pending_update_fee.is_none());
 		self.pending_update_fee = Some((feerate_per_kw, FeeUpdateState::Outbound));
 
@@ -3343,6 +3392,33 @@ 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, new_feerate: u32) -> Result<(), ChannelError> {
+		if self.autoclose_timer	> 0 && self.autoclose_timer < AUTOCLOSE_TIMEOUT {
+			// Again, check at each autoclose tick that mempool feerate hasn't fall back
+			// more in-sync to the current channel feerate. Otherwise, reset autoclose
+			// timer from scratch.
+			if new_feerate < self.get_feerate() * 1300 / 1000 {
+				self.autoclose_timer += 1;
+			} else {
+				self.autoclose_timer = 0;
+			}
+		}
+		if self.autoclose_timer == AUTOCLOSE_TIMEOUT {
+			let inbound_stats = self.get_inbound_pending_htlc_stats(None);
+			let outbound_stats = self.get_outbound_pending_htlc_stats(None);
+			// If the channel has pending *included* HTLCs to claim on-chain and `should_autoclose`=y, close the channel
+			if inbound_stats.on_holder_tx_included_htlcs + outbound_stats.on_holder_tx_included_htlcs > 0 && self.config.should_autoclose {
+				return Err(ChannelError::Close("Channel has time-sensitive outputs and the auto-close timer has been reached".to_owned()));
+			}
+			// Otherwise, do not reset the autoclose timer as a substantial HTLC can be committed
+			// at anytime on the still low-feerate channel.
+		}
+		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);
@@ -3419,7 +3495,8 @@ impl<Signer: Sign> Channel<Signer> {
 
 	/// May panic if some calls other than message-handling calls (which will all Err immediately)
 	/// have been called between remove_uncommitted_htlcs_and_mark_paused and this call.
-	pub fn channel_reestablish<L: Deref>(&mut self, msg: &msgs::ChannelReestablish, logger: &L) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, Option<ChannelMonitorUpdate>, RAACommitmentOrder, Vec<(HTLCSource, PaymentHash)>, Option<msgs::Shutdown>), ChannelError> where L::Target: Logger {
+	pub fn channel_reestablish<L: Deref>(&mut self, msg: &msgs::ChannelReestablish, logger: &L) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, Option<ChannelMonitorUpdate>, RAACommitmentOrder, Vec<(HTLCSource, PaymentHash)>, Option<msgs::Shutdown>), ChannelError> where L::Target: Logger,
+	{
 		if self.channel_state & (ChannelState::PeerDisconnected as u32) == 0 {
 			// While BOLT 2 doesn't indicate explicitly we should error this channel here, it
 			// almost certainly indicates we are going to end up out-of-sync in some way, so we
@@ -4865,6 +4942,7 @@ impl<Signer: Sign> Channel<Signer> {
 				log_trace!(logger, " ...promoting inbound AwaitingRemoteRevokeToAnnounce fee update {} to Committed", feerate);
 				self.feerate_per_kw = feerate;
 				self.pending_update_fee = None;
+				self.autoclose_timer = 0;
 			}
 		}
 		self.resend_order = RAACommitmentOrder::RevokeAndACKFirst;
@@ -5384,6 +5462,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 			(9, self.target_closing_feerate_sats_per_kw, option),
 			(11, self.monitor_pending_finalized_fulfills, vec_type),
 			(13, self.channel_creation_height, required),
+			(15, self.autoclose_timer, required),
 		});
 
 		Ok(())
@@ -5623,6 +5702,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<Signer>
 		// only, so we default to that if none was written.
 		let mut channel_type = Some(ChannelTypeFeatures::only_static_remote_key());
 		let mut channel_creation_height = Some(serialized_height);
+		let mut autoclose_timer = 0;
 		read_tlv_fields!(reader, {
 			(0, announcement_sigs, option),
 			(1, minimum_depth, option),
@@ -5633,6 +5713,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<Signer>
 			(9, target_closing_feerate_sats_per_kw, option),
 			(11, monitor_pending_finalized_fulfills, vec_type),
 			(13, channel_creation_height, option),
+			(15, autoclose_timer, required),
 		});
 
 		let chan_features = channel_type.as_ref().unwrap();
@@ -5661,6 +5742,8 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<Signer>
 
 			latest_monitor_update_id,
 
+			autoclose_timer,
+
 			holder_signer,
 			shutdown_scriptpubkey,
 			destination_script,

lightning/src/ln/channelmanager.rs

@@ -3021,6 +3021,24 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		(retain_channel, NotifyOption::DoPersist, ret_err)
 	}
 
+	/// Close the channel if the feerate is non-satisfying to ensure efficient confirmation
+	fn check_channel_autoclose(&self, short_to_id: &mut HashMap<u64, [u8; 32]>, chan_id: &[u8; 32], chan: &mut Channel<Signer>, new_feerate: u32) -> (bool, NotifyOption, Result<(), MsgHandleErrInternal>) {
+		let mut retain_channel = true;
+		let mut notify_option = NotifyOption::SkipPersist;
+		let res = match chan.check_autoclose(new_feerate) {
+			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
@@ -3077,6 +3095,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, 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; }
+
 					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() {

lightning/src/util/config.rs

@@ -246,6 +246,10 @@ pub struct ChannelConfig {
 	/// [`Normal`]: crate::chain::chaininterface::ConfirmationTarget::Normal
 	/// [`Background`]: crate::chain::chaininterface::ConfirmationTarget::Background
 	pub force_close_avoidance_max_fee_satoshis: u64,
+	/// If this is set to false, we do not auto-close channel with pending time-sensitive outputs,
+	/// of which the feerate has been detected as stalling.
+	/// Default value: true.
+	pub should_autoclose: bool,
 }
 
 impl Default for ChannelConfig {
@@ -259,6 +263,7 @@ impl Default for ChannelConfig {
 			commit_upfront_shutdown_pubkey: true,
 			max_dust_htlc_exposure_msat: 5_000_000,
 			force_close_avoidance_max_fee_satoshis: 1000,
+			should_autoclose: true,
 		}
 	}
 }
@@ -271,6 +276,7 @@ impl_writeable_tlv_based!(ChannelConfig, {
 	(4, announced_channel, required),
 	(6, commit_upfront_shutdown_pubkey, required),
 	(8, forwarding_fee_base_msat, required),
+	(10, should_autoclose, (default_value, true)),
 });
 
 /// Top-level config which holds ChannelHandshakeLimits and ChannelConfig.