From 5a1cc288b756fa39bcb33563da67841c2f757173 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 5 May 2024 23:22:23 +0000 Subject: [PATCH] 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 #993 --- fuzz/src/full_stack.rs | 20 +++++++++++ lightning/src/ln/channel.rs | 20 +++++++++++ lightning/src/ln/channelmanager.rs | 57 +++++++++++++++++++++++++++++- 3 files changed, 96 insertions(+), 1 deletion(-) diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index 4b2cf17a2..4591888e3 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -921,19 +921,32 @@ mod tests { ext_from_hex("0c005e", &mut test); // the funding transaction ext_from_hex("020000000100000000000000000000000000000000000000000000000000000000000000000000000000ffffffff0150c3000000000000220020ae0000000000000000000000000000000000000000000000000000000000000000000000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection // connect a block with no transactions, one per line ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection // by now client should have sent a channel_ready (CHECK 3: SendChannelReady to 03000000 for chan 3d000000) // inbound read from peer id 0 of len 18 @@ -1303,21 +1316,28 @@ mod tests { ext_from_hex("0c007d", &mut test); // the commitment transaction for channel 3f00000000000000000000000000000000000000000000000000000000000000 ext_from_hex("02000000013a000000000000000000000000000000000000000000000000000000000000000000000000000000800258020000000000002200204b0000000000000000000000000000000000000000000000000000000000000014c0000000000000160014280000000000000000000000000000000000000005000020", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection // // connect a block with one transaction of len 94 ext_from_hex("0c005e", &mut test); // the HTLC timeout transaction ext_from_hex("0200000001730000000000000000000000000000000000000000000000000000000000000000000000000000000001a701000000000000220020b20000000000000000000000000000000000000000000000000000000000000000000000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection // connect a block with no transactions ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection // connect a block with no transactions ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection // connect a block with no transactions ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection // connect a block with no transactions ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection // connect a block with no transactions ext_from_hex("0c0000", &mut test); + ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection // process the now-pending HTLC forward ext_from_hex("07", &mut test); diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 488141fa8..75d1c0949 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -5117,6 +5117,26 @@ impl Channel where } } + pub fn check_for_stale_feerate(&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(&mut self, fee_estimator: &LowerBoundedFeeEstimator, msg: &msgs::UpdateFee, logger: &L) -> Result<(), ChannelError> where F::Target: FeeEstimator, L::Target: Logger { diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index a2647c80c..ef6089d3c 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/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. /// @@ -2098,6 +2103,21 @@ where /// Tracks the message events that are to be broadcasted when we are connected to some peer. pending_broadcast_messages: Mutex>, + /// 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>, + entropy_source: ES, node_signer: NS, signer_provider: SP, @@ -2875,6 +2895,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, @@ -9173,7 +9195,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) => { @@ -11992,6 +12045,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, }; -- 2.39.5