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.

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 0e304f3074524f56a537681279c558e685a9354d..cf60c280b37632b64a491dd3c36b793b48eccdd4 100644 (file)
--- a/lightning/src/ln/channel.rs
+++ b/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,8 +3771,8 @@ 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()));
@@ -3771,7 +3780,7 @@ impl<Signer: Sign> Channel<Signer> {
                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 {

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index d3fbc244021b059a16d12509510647801ae93549..ba244c6ddc1509d86a811ead23faa8e886cce506 100644 (file)
--- a/lightning/src/ln/channelmanager.rs
+++ b/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))
                }

diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index b95356a3dd7aa8b7375118612fe2faa3c2a8b2e7..f9b9e75bf6d8d4385691f11f5ab5a86830030e9a 100644 (file)
--- a/lightning/src/ln/functional_tests.rs
+++ b/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"),
+       };
+}