Force-close channels if their feerate gets stale without any update

For quite some time, LDK has force-closed channels if the peer
sends us a feerate update which is below our `FeeEstimator`'s
concept of a channel lower-bound. This is intended to ensure that
channel feerates are always sufficient to get our commitment
transaction confirmed on-chain if we do need to force-close.

However, we've never checked our channel feerate regularly - if a
peer is offline (or just uninterested in updating the channel
feerate) and the prevailing feerates on-chain go up, we'll simply
ignore it and allow our commitment transaction to sit around with a
feerate too low to get confirmed.

Here we rectify this oversight by force-closing channels with stale
feerates, checking after each block. However, because fee
estimators are often buggy and force-closures piss off users, we
only do so rather conservatively. Specifically, we only force-close
if a channel's feerate is below the minimum `FeeEstimator`-provided
minimum across the last day.

Further, because fee estimators are often especially buggy on
startup (and because peers haven't had a chance to update the
channel feerates yet), we don't force-close channels until we have
a full day of feerate lower-bound history.

This should reduce the incidence of force-closures substantially,
but it is expected this will still increase force-closures somewhat
substantially depending on the users' `FeeEstimator`.

Fixes lightningdevkit#993

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -5303,6 +5303,26 @@ impl<SP: Deref> Channel<SP> where
 		}
 	}
 
+	pub fn check_for_stale_feerate<L: Logger>(&mut self, logger: &L, min_feerate: u32) -> Result<(), ClosureReason> {
+		if self.context.is_outbound() {
+			// While its possible our fee is too low for an outbound channel because we've been
+			// unable to increase the fee, we don't try to force-close directly here.
+			return Ok(());
+		}
+		if self.context.feerate_per_kw < min_feerate {
+			log_info!(logger,
+				"Closing channel as feerate of {} is below required {} (the minimum required rate over the past day)",
+				self.context.feerate_per_kw, min_feerate
+			);
+			Err(ClosureReason::PeerFeerateTooLow {
+				peer_feerate_sat_per_kw: self.context.feerate_per_kw,
+				required_feerate_sat_per_kw: min_feerate,
+			})
+		} else {
+			Ok(())
+		}
+	}
+
 	pub fn update_fee<F: Deref, L: Deref>(&mut self, fee_estimator: &LowerBoundedFeeEstimator<F>, msg: &msgs::UpdateFee, logger: &L) -> Result<(), ChannelError>
 		where F::Target: FeeEstimator, L::Target: Logger
 	{

lightning/src/ln/channelmanager.rs

@@ -962,6 +962,11 @@ pub(super) struct InboundChannelRequest {
 /// accepted. An unaccepted channel that exceeds this limit will be abandoned.
 const UNACCEPTED_INBOUND_CHANNEL_AGE_LIMIT_TICKS: i32 = 2;
 
+/// The number of blocks of historical feerate estimates we keep around and consider when deciding
+/// to force-close a channel for having too-low fees. Also the number of blocks we have to see
+/// after startup before we consider force-closing channels for having too-low fees.
+const FEERATE_TRACKING_BLOCKS: usize = 144;
+
 /// Stores a PaymentSecret and any other data we may need to validate an inbound payment is
 /// actually ours and not some duplicate HTLC sent to us by a node along the route.
 ///
@@ -2096,6 +2101,21 @@ where
 	/// Tracks the message events that are to be broadcasted when we are connected to some peer.
 	pending_broadcast_messages: Mutex<Vec<MessageSendEvent>>,
 
+	/// We only want to force-close our channels on peers based on stale feerates when we're
+	/// confident the feerate on the channel is *really* stale, not just became stale recently.
+	/// Thus, we store the fee estimates we had as of the last [`FEERATE_TRACKING_BLOCKS`] blocks
+	/// (after startup completed) here, and only force-close when channels have a lower feerate
+	/// than we predicted any time in the last [`FEERATE_TRACKING_BLOCKS`] blocks.
+	///
+	/// We only keep this in memory as we assume any feerates we receive immediately after startup
+	/// may be bunk (as they often are if Bitcoin Core crashes) and want to delay taking any
+	/// actions for a day anyway.
+	///
+	/// The first element in the pair is the
+	/// [`ConfirmationTarget::MinAllowedAnchorChannelRemoteFee`] estimate, the second the
+	/// [`ConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee`] estimate.
+	last_days_feerates: Mutex<VecDeque<(u32, u32)>>,
+
 	entropy_source: ES,
 	node_signer: NS,
 	signer_provider: SP,
@@ -3192,6 +3212,8 @@ where
 			pending_offers_messages: Mutex::new(Vec::new()),
 			pending_broadcast_messages: Mutex::new(Vec::new()),
 
+			last_days_feerates: Mutex::new(VecDeque::new()),
+
 			entropy_source,
 			node_signer,
 			signer_provider,
@@ -9414,7 +9436,38 @@ where
 				self, || -> NotifyOption { NotifyOption::DoPersist });
 		*self.best_block.write().unwrap() = BestBlock::new(block_hash, height);
 
-		self.do_chain_event(Some(height), |channel| channel.best_block_updated(height, header.time, self.chain_hash, &self.node_signer, &self.default_configuration, &&WithChannelContext::from(&self.logger, &channel.context, None)));
+		let mut min_anchor_feerate = None;
+		let mut min_non_anchor_feerate = None;
+		if self.background_events_processed_since_startup.load(Ordering::Relaxed) {
+			// If we're past the startup phase, update our feerate cache
+			let mut last_days_feerates = self.last_days_feerates.lock().unwrap();
+			if last_days_feerates.len() >= FEERATE_TRACKING_BLOCKS {
+				last_days_feerates.pop_front();
+			}
+			let anchor_feerate = self.fee_estimator
+				.bounded_sat_per_1000_weight(ConfirmationTarget::MinAllowedAnchorChannelRemoteFee);
+			let non_anchor_feerate = self.fee_estimator
+				.bounded_sat_per_1000_weight(ConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee);
+			last_days_feerates.push_back((anchor_feerate, non_anchor_feerate));
+			if last_days_feerates.len() >= FEERATE_TRACKING_BLOCKS {
+				min_anchor_feerate = last_days_feerates.iter().map(|(f, _)| f).min().copied();
+				min_non_anchor_feerate = last_days_feerates.iter().map(|(_, f)| f).min().copied();
+			}
+		}
+
+		self.do_chain_event(Some(height), |channel| {
+			let logger = WithChannelContext::from(&self.logger, &channel.context, None);
+			if channel.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
+				if let Some(feerate) = min_anchor_feerate {
+					channel.check_for_stale_feerate(&logger, feerate)?;
+				}
+			} else {
+				if let Some(feerate) = min_non_anchor_feerate {
+					channel.check_for_stale_feerate(&logger, feerate)?;
+				}
+			}
+			channel.best_block_updated(height, header.time, self.chain_hash, &self.node_signer, &self.default_configuration, &&WithChannelContext::from(&self.logger, &channel.context, None))
+		});
 
 		macro_rules! max_time {
 			($timestamp: expr) => {
@@ -12375,6 +12428,8 @@ where
 			node_signer: args.node_signer,
 			signer_provider: args.signer_provider,
 
+			last_days_feerates: Mutex::new(VecDeque::new()),
+
 			logger: args.logger,
 			default_configuration: args.default_config,
 		};