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;
 			}
 		}
 
@@ -2711,16 +2736,16 @@ impl<Signer: Sign> Channel<Signer> {
 	/// Public version of the below, checking relevant preconditions first.
 	/// If we're not in a state where freeing the holding cell makes sense, this is a no-op and
 	/// returns `(None, Vec::new())`.
-	pub fn maybe_free_holding_cell_htlcs<L: Deref>(&mut self, logger: &L) -> Result<(Option<(msgs::CommitmentUpdate, ChannelMonitorUpdate)>, Vec<(HTLCSource, PaymentHash)>), ChannelError> where L::Target: Logger {
+	pub fn maybe_free_holding_cell_htlcs<L: Deref>(&mut self, background_feerate: u32, logger: &L) -> Result<(Option<(msgs::CommitmentUpdate, ChannelMonitorUpdate)>, Vec<(HTLCSource, PaymentHash)>), ChannelError> where L::Target: Logger {
 		if self.channel_state >= ChannelState::ChannelFunded as u32 &&
 		   (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32)) == 0 {
-			self.free_holding_cell_htlcs(logger)
+			self.free_holding_cell_htlcs(background_feerate, logger)
 		} else { Ok((None, Vec::new())) }
 	}
 
 	/// Used to fulfill holding_cell_htlcs when we get a remote ack (or implicitly get it by them
 	/// fulfilling or failing the last pending HTLC)
-	fn free_holding_cell_htlcs<L: Deref>(&mut self, logger: &L) -> Result<(Option<(msgs::CommitmentUpdate, ChannelMonitorUpdate)>, Vec<(HTLCSource, PaymentHash)>), ChannelError> where L::Target: Logger {
+	fn free_holding_cell_htlcs<L: Deref>(&mut self, background_feerate: u32, logger: &L) -> Result<(Option<(msgs::CommitmentUpdate, ChannelMonitorUpdate)>, Vec<(HTLCSource, PaymentHash)>), ChannelError> where L::Target: Logger {
 		assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, 0);
 		if self.holding_cell_htlc_updates.len() != 0 || self.holding_cell_update_fee.is_some() {
 			log_trace!(logger, "Freeing holding cell with {} HTLC updates{} in channel {}", self.holding_cell_htlc_updates.len(),
@@ -2804,7 +2829,7 @@ impl<Signer: Sign> Channel<Signer> {
 				return Ok((None, htlcs_to_fail));
 			}
 			let update_fee = if let Some(feerate) = self.holding_cell_update_fee.take() {
-				self.send_update_fee(feerate, logger)
+				self.send_update_fee(feerate, background_feerate, logger)
 			} else {
 				None
 			};
@@ -2832,13 +2857,35 @@ 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, background_feerate: u32) -> bool {
+
+		// Verify the new feerate is at least superior by 30% of current feerate before to
+		// start a autoclose timer.
+		if new_feerate < current_feerate * 1300 / 1000 && new_feerate < background_feerate {
+			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>
+	pub fn revoke_and_ack<L: Deref, F: Deref>(&mut self, msg: &msgs::RevokeAndACK, logger: &L, fee_estimator: &F) -> Result<RAAUpdates, ChannelError>
 		where L::Target: Logger,
+		F::Target: FeeEstimator
 	{
 		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 +3046,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 +3055,7 @@ impl<Signer: Sign> Channel<Signer> {
 					require_commitment = true;
 					self.feerate_per_kw = feerate;
 					self.pending_update_fee = None;
+					self.autoclose_timer = 0;
 				},
 			}
 		}
@@ -3037,7 +3086,8 @@ impl<Signer: Sign> Channel<Signer> {
 			});
 		}
 
-		match self.free_holding_cell_htlcs(logger)? {
+		let background_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
+		match self.free_holding_cell_htlcs(background_feerate, logger)? {
 			(Some((mut commitment_update, mut additional_update)), htlcs_to_fail) => {
 				commitment_update.update_fail_htlcs.reserve(update_fail_htlcs.len());
 				for fail_msg in update_fail_htlcs.drain(..) {
@@ -3104,7 +3154,7 @@ impl<Signer: Sign> Channel<Signer> {
 	/// If our balance is too low to cover the cost of the next commitment transaction at the
 	/// new feerate, the update is cancelled.
 	/// You MUST call send_commitment prior to any other calls on this Channel
-	fn send_update_fee<L: Deref>(&mut self, feerate_per_kw: u32, logger: &L) -> Option<msgs::UpdateFee> where L::Target: Logger {
+	fn send_update_fee<L: Deref>(&mut self, feerate_per_kw: u32, background_feerate: u32, logger: &L) -> Option<msgs::UpdateFee> where L::Target: Logger {
 		if !self.is_outbound() {
 			panic!("Cannot send fee from inbound channel");
 		}
@@ -3145,6 +3195,8 @@ impl<Signer: Sign> Channel<Signer> {
 			return None;
 		}
 
+		self.maybe_trigger_autoclose_timer(self.get_feerate(), feerate_per_kw, background_feerate);
+
 		debug_assert!(self.pending_update_fee.is_none());
 		self.pending_update_fee = Some((feerate_per_kw, FeeUpdateState::Outbound));
 
@@ -3154,8 +3206,8 @@ impl<Signer: Sign> Channel<Signer> {
 		})
 	}
 
-	pub fn send_update_fee_and_commit<L: Deref>(&mut self, feerate_per_kw: u32, logger: &L) -> Result<Option<(msgs::UpdateFee, msgs::CommitmentSigned, ChannelMonitorUpdate)>, ChannelError> where L::Target: Logger {
-		match self.send_update_fee(feerate_per_kw, logger) {
+	pub fn send_update_fee_and_commit<L: Deref>(&mut self, feerate_per_kw: u32, background_feerate: u32, logger: &L) -> Result<Option<(msgs::UpdateFee, msgs::CommitmentSigned, ChannelMonitorUpdate)>, ChannelError> where L::Target: Logger {
+		match self.send_update_fee(feerate_per_kw, background_feerate, logger) {
 			Some(update_fee) => {
 				let (commitment_signed, monitor_update) = self.send_commitment_no_status_check(logger)?;
 				Ok(Some((update_fee, commitment_signed, monitor_update)))
@@ -3343,6 +3395,26 @@ 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 {
+			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 +3491,10 @@ 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, F: Deref>(&mut self, msg: &msgs::ChannelReestablish, logger: &L, fee_estimator: &F) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, Option<ChannelMonitorUpdate>, RAACommitmentOrder, Vec<(HTLCSource, PaymentHash)>, Option<msgs::Shutdown>), ChannelError>
+		where L::Target: Logger,
+		F::Target: FeeEstimator
+	{
 		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
@@ -3524,7 +3599,8 @@ impl<Signer: Sign> Channel<Signer> {
 				// channel_reestablish should result in them sending a revoke_and_ack), but we may
 				// have received some updates while we were disconnected. Free the holding cell
 				// now!
-				match self.free_holding_cell_htlcs(logger) {
+				let background_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
+				match self.free_holding_cell_htlcs(background_feerate, logger) {
 					Err(ChannelError::Close(msg)) => return Err(ChannelError::Close(msg)),
 					Err(ChannelError::Warn(_)) | Err(ChannelError::Ignore(_)) | Err(ChannelError::CloseDelayBroadcast(_)) =>
 						panic!("Got non-channel-failing result from free_holding_cell_htlcs"),
@@ -4865,6 +4941,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 +5461,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 +5701,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 +5712,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 +5741,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,