Accept feerate increases even if they aren't high enough for us

LND nodes have very broken fee estimators, causing them to suggest
feerates that don't even meet a current mempool minimum feerate
when fees go up over the course of hours. This can cause us to
reject their feerate estimates as they're not high enough, even
though their new feerate is higher than what we had already (which
is the feerate we'll use to broadcast a closing transaction). This
implies we force-close the channel and broadcast something with a
feerate lower than our counterparty was offering.

Here we simply accept such feerates as they are better than what we
had. We really should also close the channel, but only after we
get their signature on the new feerate. That should happen by
checking channel feerates every time we see a new block so is
orthogonal to this code.

Ultimately the fix is anchor outputs plus package-based relay in
Bitcoin Core, however we're still quite some ways from that, so
worth needlessly closing channels for now.

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -1075,8 +1075,9 @@ impl<Signer: Sign> Channel<Signer> {
 		})
 	}
 
-	fn check_remote_fee<F: Deref>(fee_estimator: &LowerBoundedFeeEstimator<F>, feerate_per_kw: u32) -> Result<(), ChannelError>
-		where F::Target: FeeEstimator
+	fn check_remote_fee<F: Deref, L: Deref>(fee_estimator: &LowerBoundedFeeEstimator<F>,
+		feerate_per_kw: u32, cur_feerate_per_kw: Option<u32>, logger: &L)
+		-> Result<(), ChannelError> where F::Target: FeeEstimator, L::Target: Logger,
 	{
 		// We only bound the fee updates on the upper side to prevent completely absurd feerates,
 		// always accepting up to 25 sat/vByte or 10x our fee estimator's "High Priority" fee.
@@ -1093,6 +1094,14 @@ impl<Signer: Sign> Channel<Signer> {
 		// of 1.1 sat/vbyte and a receiver that wants 1.1 rounded up to 2. Thus, we always add 250
 		// sat/kw before the comparison here.
 		if feerate_per_kw + 250 < lower_limit {
+			if let Some(cur_feerate) = cur_feerate_per_kw {
+				if feerate_per_kw > cur_feerate {
+					log_warn!(logger,
+						"Accepting feerate that may prevent us from closing this channel because it's higher than what we have now. Had {} s/kW, now {} s/kW.",
+						cur_feerate, feerate_per_kw);
+					return Ok(());
+				}
+			}
 			return Err(ChannelError::Close(format!("Peer's feerate much too low. Actual: {}. Our expected lower limit: {} (- 250)", feerate_per_kw, lower_limit)));
 		}
 		Ok(())
@@ -1179,7 +1188,7 @@ impl<Signer: Sign> Channel<Signer> {
 		if msg.htlc_minimum_msat >= full_channel_value_msat {
 			return Err(ChannelError::Close(format!("Minimum htlc value ({}) was larger than full channel value ({})", msg.htlc_minimum_msat, full_channel_value_msat)));
 		}
-		Channel::<Signer>::check_remote_fee(fee_estimator, msg.feerate_per_kw)?;
+		Channel::<Signer>::check_remote_fee(fee_estimator, msg.feerate_per_kw, None, logger)?;
 
 		let max_counterparty_selected_contest_delay = u16::min(config.channel_handshake_limits.their_to_self_delay, MAX_LOCAL_BREAKDOWN_TIMEOUT);
 		if msg.to_self_delay > max_counterparty_selected_contest_delay {
@@ -3762,16 +3771,16 @@ impl<Signer: Sign> Channel<Signer> {
 		}
 	}
 
-	pub fn update_fee<F: Deref>(&mut self, fee_estimator: &LowerBoundedFeeEstimator<F>, msg: &msgs::UpdateFee) -> Result<(), ChannelError>
-		where F::Target: FeeEstimator
+	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
 	{
 		if self.is_outbound() {
 			return Err(ChannelError::Close("Non-funding remote tried to update channel fee".to_owned()));
 		}
 		if self.channel_state & (ChannelState::PeerDisconnected as u32) == ChannelState::PeerDisconnected as u32 {
 			return Err(ChannelError::Close("Peer sent update_fee when we needed a channel_reestablish".to_owned()));
 		}
-		Channel::<Signer>::check_remote_fee(fee_estimator, msg.feerate_per_kw)?;
+		Channel::<Signer>::check_remote_fee(fee_estimator, msg.feerate_per_kw, Some(self.feerate_per_kw), logger)?;
 		let feerate_over_dust_buffer = msg.feerate_per_kw > self.get_dust_buffer_feerate(None);
 
 		self.pending_update_fee = Some((msg.feerate_per_kw, FeeUpdateState::RemoteAnnounced));
@@ -6765,7 +6774,8 @@ mod tests {
 		// arithmetic, causing a panic with debug assertions enabled.
 		let fee_est = TestFeeEstimator { fee_est: 42 };
 		let bounded_fee_estimator = LowerBoundedFeeEstimator::new(&fee_est);
-		assert!(Channel::<InMemorySigner>::check_remote_fee(&bounded_fee_estimator, u32::max_value()).is_err());
+		assert!(Channel::<InMemorySigner>::check_remote_fee(&bounded_fee_estimator,
+			u32::max_value(), None, &&test_utils::TestLogger::new()).is_err());
 	}
 
 	struct Keys {

lightning/src/ln/channelmanager.rs

@@ -5189,7 +5189,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 				if chan.get().get_counterparty_node_id() != *counterparty_node_id {
 					return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.channel_id));
 				}
-				try_chan_entry!(self, chan.get_mut().update_fee(&self.fee_estimator, &msg), chan);
+				try_chan_entry!(self, chan.get_mut().update_fee(&self.fee_estimator, &msg, &self.logger), chan);
 			},
 			hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
 		}

lightning/src/ln/functional_tests.rs

@@ -10490,3 +10490,81 @@ fn test_non_final_funding_tx() {
 	assert!(nodes[0].node.funding_transaction_generated(&temp_channel_id, &nodes[1].node.get_our_node_id(), tx.clone()).is_ok());
 	get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
 }
+
+#[test]
+fn accept_busted_but_better_fee() {
+	// If a peer sends us a fee update that is too low, but higher than our previous channel
+	// feerate, we should accept it. In the future we may want to consider closing the channel
+	// later, but for now we only accept the update.
+	let mut chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	create_chan_between_nodes(&nodes[0], &nodes[1], channelmanager::provided_init_features(), channelmanager::provided_init_features());
+
+	// Set nodes[1] to expect 5,000 sat/kW.
+	{
+		let mut feerate_lock = chanmon_cfgs[1].fee_estimator.sat_per_kw.lock().unwrap();
+		*feerate_lock = 5000;
+	}
+
+	// If nodes[0] increases their feerate, even if its not enough, nodes[1] should accept it.
+	{
+		let mut feerate_lock = chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
+		*feerate_lock = 1000;
+	}
+	nodes[0].node.timer_tick_occurred();
+	check_added_monitors!(nodes[0], 1);
+
+	let events = nodes[0].node.get_and_clear_pending_msg_events();
+	assert_eq!(events.len(), 1);
+	match events[0] {
+		MessageSendEvent::UpdateHTLCs { updates: msgs::CommitmentUpdate { ref update_fee, ref commitment_signed, .. }, .. } => {
+			nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), update_fee.as_ref().unwrap());
+			commitment_signed_dance!(nodes[1], nodes[0], commitment_signed, false);
+		},
+		_ => panic!("Unexpected event"),
+	};
+
+	// If nodes[0] increases their feerate further, even if its not enough, nodes[1] should accept
+	// it.
+	{
+		let mut feerate_lock = chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
+		*feerate_lock = 2000;
+	}
+	nodes[0].node.timer_tick_occurred();
+	check_added_monitors!(nodes[0], 1);
+
+	let events = nodes[0].node.get_and_clear_pending_msg_events();
+	assert_eq!(events.len(), 1);
+	match events[0] {
+		MessageSendEvent::UpdateHTLCs { updates: msgs::CommitmentUpdate { ref update_fee, ref commitment_signed, .. }, .. } => {
+			nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), update_fee.as_ref().unwrap());
+			commitment_signed_dance!(nodes[1], nodes[0], commitment_signed, false);
+		},
+		_ => panic!("Unexpected event"),
+	};
+
+	// However, if nodes[0] decreases their feerate, nodes[1] should reject it and close the
+	// channel.
+	{
+		let mut feerate_lock = chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
+		*feerate_lock = 1000;
+	}
+	nodes[0].node.timer_tick_occurred();
+	check_added_monitors!(nodes[0], 1);
+
+	let events = nodes[0].node.get_and_clear_pending_msg_events();
+	assert_eq!(events.len(), 1);
+	match events[0] {
+		MessageSendEvent::UpdateHTLCs { updates: msgs::CommitmentUpdate { ref update_fee, .. }, .. } => {
+			nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), update_fee.as_ref().unwrap());
+			check_closed_event!(nodes[1], 1, ClosureReason::ProcessingError {
+				err: "Peer's feerate much too low. Actual: 1000. Our expected lower limit: 5000 (- 250)".to_owned() });
+			check_closed_broadcast!(nodes[1], true);
+			check_added_monitors!(nodes[1], 1);
+		},
+		_ => panic!("Unexpected event"),
+	};
+}