Cancel the outbound feerate update if above what we can afford
authorAntoine Riard <dev@ariard.me>
Sat, 21 Aug 2021 22:05:51 +0000 (18:05 -0400)
committerAntoine Riard <dev@ariard.me>
Tue, 9 Nov 2021 00:29:56 +0000 (19:29 -0500)
fuzz/src/chanmon_consistency.rs
fuzz/src/utils/test_logger.rs
lightning/src/ln/channel.rs
lightning/src/ln/functional_tests.rs

index f3b952ff9dba74b10f921be769d06c355747a63e..4995faba6569ebc6ea1cc99c86e047bc17ffb798 100644 (file)
@@ -732,7 +732,10 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
                                                        // force-close which we should detect as an error).
                                                        assert_eq!(msg.contents.flags & 2, 0);
                                                },
-                                               _ => panic!("Unhandled message event {:?}", event),
+                                               _ => if out.locked_contains(&"Outbound update_fee HTLC buffer overflow - counterparty should force-close this channel") {
+                                               } else {
+                                                       panic!("Unhandled message event {:?}", event)
+                                               },
                                        }
                                        if $limit_events != ProcessMessages::AllMessages {
                                                break;
@@ -764,7 +767,10 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
                                                        events::MessageSendEvent::SendChannelUpdate { ref msg, .. } => {
                                                                assert_eq!(msg.contents.flags & 2, 0); // The disable bit must never be set!
                                                        },
-                                                       _ => panic!("Unhandled message event"),
+                                                       _ => if out.locked_contains(&"Outbound update_fee HTLC buffer overflow - counterparty should force-close this channel") {
+                                                       } else {
+                                                               panic!("Unhandled message event")
+                                                       },
                                                }
                                        }
                                        push_excess_b_events!(nodes[1].get_and_clear_pending_msg_events().drain(..), Some(0));
@@ -781,7 +787,10 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
                                                        events::MessageSendEvent::SendChannelUpdate { ref msg, .. } => {
                                                                assert_eq!(msg.contents.flags & 2, 0); // The disable bit must never be set!
                                                        },
-                                                       _ => panic!("Unhandled message event"),
+                                                       _ => if out.locked_contains(&"Outbound update_fee HTLC buffer overflow - counterparty should force-close this channel") {
+                                                       } else {
+                                                               panic!("Unhandled message event")
+                                                       },
                                                }
                                        }
                                        push_excess_b_events!(nodes[1].get_and_clear_pending_msg_events().drain(..), Some(2));
@@ -832,7 +841,10 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
                                                events::Event::PendingHTLCsForwardable { .. } => {
                                                        nodes[$node].process_pending_htlc_forwards();
                                                },
-                                               _ => panic!("Unhandled event"),
+                                               _ => if out.locked_contains(&"Outbound update_fee HTLC buffer overflow - counterparty should force-close this channel") {
+                                               } else {
+                                                       panic!("Unhandled event")
+                                               },
                                        }
                                }
                                had_events
@@ -1116,12 +1128,16 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
                                }
 
                                // Finally, make sure that at least one end of each channel can make a substantial payment.
-                               assert!(
-                                       send_payment(&nodes[0], &nodes[1], chan_a, 10_000_000, &mut payment_id) ||
-                                       send_payment(&nodes[1], &nodes[0], chan_a, 10_000_000, &mut payment_id));
-                               assert!(
-                                       send_payment(&nodes[1], &nodes[2], chan_b, 10_000_000, &mut payment_id) ||
-                                       send_payment(&nodes[2], &nodes[1], chan_b, 10_000_000, &mut payment_id));
+                               // Unless we expected to hit a limitation of LN state machine (see comment in `send_update_fee_and_commit`)
+                               if out.locked_contains(&"Outbound update_fee HTLC buffer overflow - counterparty should force-close this channel") {
+                               } else {
+                                       assert!(
+                                               send_payment(&nodes[0], &nodes[1], chan_a, 10_000_000, &mut payment_id) ||
+                                               send_payment(&nodes[1], &nodes[0], chan_a, 10_000_000, &mut payment_id));
+                                       assert!(
+                                               send_payment(&nodes[1], &nodes[2], chan_b, 10_000_000, &mut payment_id) ||
+                                               send_payment(&nodes[2], &nodes[1], chan_b, 10_000_000, &mut payment_id));
+                               }
 
                                last_htlc_clear_fee_a = fee_est_a.ret_val.load(atomic::Ordering::Acquire);
                                last_htlc_clear_fee_b = fee_est_b.ret_val.load(atomic::Ordering::Acquire);
index f8c96f99bd1c8ddb9c1f9b28d8d7b5fbe67cc27b..ef6344ed4b4c5d3bee894c577a71b46ece685636 100644 (file)
@@ -13,12 +13,16 @@ use std::io::Write;
 
 pub trait Output : Clone  + 'static {
        fn locked_write(&self, data: &[u8]);
+       fn locked_contains(&self, pattern: &str) -> bool;
 }
 
 #[derive(Clone)]
 pub struct DevNull {}
 impl Output for DevNull {
        fn locked_write(&self, _data: &[u8]) {}
+       fn locked_contains(&self, _pattern: &str) -> bool {
+               false
+       }
 }
 #[derive(Clone)]
 pub struct StringBuffer(Arc<Mutex<String>>);
@@ -26,6 +30,9 @@ impl Output for StringBuffer {
        fn locked_write(&self, data: &[u8]) {
                self.0.lock().unwrap().push_str(&String::from_utf8(data.to_vec()).unwrap());
        }
+       fn locked_contains(&self, pattern: &str) -> bool {
+               self.0.lock().unwrap().contains(pattern)
+       }
 }
 impl StringBuffer {
        pub fn new() -> Self {
index 5bd47c329422b1e895a5e7f7df21bafa9d6a5282..23c5a9dbb37143cb9dd6867b076ab3e65d411420 100644 (file)
@@ -61,7 +61,7 @@ pub struct ChannelValueStat {
        pub counterparty_dust_limit_msat: u64,
 }
 
-#[derive(Clone, Copy, PartialEq)]
+#[derive(Debug, Clone, Copy, PartialEq)]
 enum FeeUpdateState {
        // Inbound states mirroring InboundHTLCState
        RemoteAnnounced,
@@ -2957,8 +2957,10 @@ impl<Signer: Sign> Channel<Signer> {
 
        /// Adds a pending update to this channel. See the doc for send_htlc for
        /// further details on the optionness of the return value.
+       /// 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(&mut self, feerate_per_kw: u32) -> Option<msgs::UpdateFee> {
+       fn send_update_fee<L: Deref>(&mut self, feerate_per_kw: u32, logger: &L) -> Option<msgs::UpdateFee> where L::Target: Logger {
                if !self.is_outbound() {
                        panic!("Cannot send fee from inbound channel");
                }
@@ -2969,6 +2971,26 @@ impl<Signer: Sign> Channel<Signer> {
                        panic!("Cannot update fee while peer is disconnected/we're awaiting a monitor update (ChannelManager should have caught this)");
                }
 
+               // Before proposing a feerate update, check that we can actually afford the new fee.
+               let inbound_stats = self.get_inbound_pending_htlc_stats();
+               let outbound_stats = self.get_outbound_pending_htlc_stats();
+               // In case of a concurrent update_add_htlc proposed by our counterparty, we might
+               // not have enough balance value remaining to cover the onchain cost of this new
+               // HTLC weight. If this happens, our counterparty fails the reception of our
+               // commitment_signed including this new HTLC due to infringement on the channel
+               // reserve.
+               // To prevent this case, we compute our outbound update_fee with an HTLC buffer of
+               // size 2. However, if the number of concurrent update_add_htlc is higher, this still
+               // leads to a channel force-close. Ultimately, this is an issue coming from the
+               // design of LN state machines, allowing asynchronous updates.
+               let total_fee_sat = Channel::<Signer>::commit_tx_fee_sat(feerate_per_kw, (inbound_stats.pending_htlcs + /* HTLC feerate buffer */ 2 + outbound_stats.pending_htlcs) as usize);
+               let holder_balance_after_fee_sub = (self.value_to_self_msat / 1000).checked_sub(total_fee_sat).map(|a| a.checked_sub(outbound_stats.pending_htlcs_value_msat / 1000)).unwrap_or(None).unwrap_or(u64::min_value());
+               if holder_balance_after_fee_sub < self.counterparty_selected_channel_reserve_satoshis.unwrap() {
+                       //TODO: auto-close after a number of failures?
+                       log_debug!(logger, "Cannot afford to send new feerate at {}", feerate_per_kw);
+                       return None;
+               }
+
                if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
                        self.holding_cell_update_fee = Some(feerate_per_kw);
                        return None;
@@ -2984,7 +3006,7 @@ 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) {
+               match self.send_update_fee(feerate_per_kw, logger) {
                        Some(update_fee) => {
                                let (commitment_signed, monitor_update) = self.send_commitment_no_status_check(logger)?;
                                Ok(Some((update_fee, commitment_signed, monitor_update)))
@@ -4727,6 +4749,18 @@ impl<Signer: Sign> Channel<Signer> {
                                }
                        }
                }
+               if self.is_outbound() {
+                       // Log if we can't afford next remote commitment tx fee at pending outbound feerate update.
+                       if let Some(pending_feerate) = self.pending_update_fee {
+                               assert_eq!(pending_feerate.1, FeeUpdateState::Outbound);
+                               let next_total_fee_sat = Channel::<Signer>::commit_tx_fee_sat(pending_feerate.0, self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len());
+                               let outbound_stats = self.get_outbound_pending_htlc_stats();
+                               let holder_balance_after_fee_sub_sats = (self.value_to_self_msat / 1000).checked_sub(next_total_fee_sat).map(|a| a.checked_sub(outbound_stats.pending_htlcs_value_msat / 1000)).unwrap_or(None).unwrap_or(u64::min_value());
+                               if holder_balance_after_fee_sub_sats < self.counterparty_selected_channel_reserve_satoshis.unwrap() {
+                                       log_debug!(logger, "Outbound update_fee HTLC buffer overflow - counterparty should force-close this channel");
+                               }
+                       }
+               }
 
                {
                        let mut htlcs = Vec::with_capacity(counterparty_commitment_tx.3.len());
index a8a0e32f1b554660b3458d579e28f6de937357ea..6fa7858f0261e86a302e4c612276a855f63e3449 100644 (file)
@@ -22,7 +22,7 @@ use ln::channel::{COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC};
 use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, PaymentId, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA};
 use ln::channel::{Channel, ChannelError};
 use ln::{chan_utils, onion_utils};
-use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
+use ln::chan_utils::{HTLC_SUCCESS_TX_WEIGHT, HTLCOutputInCommitment};
 use routing::network_graph::{NetworkUpdate, RoutingFees};
 use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route, get_keysend_route};
 use routing::scorer::Scorer;
@@ -585,9 +585,10 @@ fn test_update_fee_that_funder_cannot_afford() {
        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);
-       let channel_value = 1888;
+       let channel_value = 1977;
        let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, channel_value, 700000, InitFeatures::known(), InitFeatures::known());
        let channel_id = chan.2;
+       let secp_ctx = Secp256k1::new();
 
        let feerate = 260;
        {
@@ -622,16 +623,70 @@ fn test_update_fee_that_funder_cannot_afford() {
                *feerate_lock = feerate + 2;
        }
        nodes[0].node.timer_tick_occurred();
-       check_added_monitors!(nodes[0], 1);
+       nodes[0].logger.assert_log("lightning::ln::channel".to_string(), format!("Cannot afford to send new feerate at {}", feerate + 2), 1);
+       check_added_monitors!(nodes[0], 0);
 
-       let update2_msg = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
+       const INITIAL_COMMITMENT_NUMBER: u64 = 281474976710654;
 
-       nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), &update2_msg.update_fee.unwrap());
+       // Get the EnforcingSigner for each channel, which will be used to (1) get the keys
+       // needed to sign the new commitment tx and (2) sign the new commitment tx.
+       let (local_revocation_basepoint, local_htlc_basepoint, local_funding) = {
+               let chan_lock = nodes[0].node.channel_state.lock().unwrap();
+               let local_chan = chan_lock.by_id.get(&chan.2).unwrap();
+               let chan_signer = local_chan.get_signer();
+               let pubkeys = chan_signer.pubkeys();
+               (pubkeys.revocation_basepoint, pubkeys.htlc_basepoint,
+                pubkeys.funding_pubkey)
+       };
+       let (remote_delayed_payment_basepoint, remote_htlc_basepoint,remote_point, remote_funding) = {
+               let chan_lock = nodes[1].node.channel_state.lock().unwrap();
+               let remote_chan = chan_lock.by_id.get(&chan.2).unwrap();
+               let chan_signer = remote_chan.get_signer();
+               let pubkeys = chan_signer.pubkeys();
+               (pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint,
+                chan_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx),
+                pubkeys.funding_pubkey)
+       };
+
+       // Assemble the set of keys we can use for signatures for our commitment_signed message.
+       let commit_tx_keys = chan_utils::TxCreationKeys::derive_new(&secp_ctx, &remote_point, &remote_delayed_payment_basepoint,
+               &remote_htlc_basepoint, &local_revocation_basepoint, &local_htlc_basepoint).unwrap();
+
+       let res = {
+               let local_chan_lock = nodes[0].node.channel_state.lock().unwrap();
+               let local_chan = local_chan_lock.by_id.get(&chan.2).unwrap();
+               let local_chan_signer = local_chan.get_signer();
+               let mut htlcs: Vec<(HTLCOutputInCommitment, ())> = vec![];
+               let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data(
+                       INITIAL_COMMITMENT_NUMBER - 1,
+                       700,
+                       999,
+                       false, local_funding, remote_funding,
+                       commit_tx_keys.clone(),
+                       feerate + 124,
+                       &mut htlcs,
+                       &local_chan.channel_transaction_parameters.as_counterparty_broadcastable()
+               );
+               local_chan_signer.sign_counterparty_commitment(&commitment_tx, &secp_ctx).unwrap()
+       };
+
+       let commit_signed_msg = msgs::CommitmentSigned {
+               channel_id: chan.2,
+               signature: res.0,
+               htlc_signatures: res.1
+       };
+
+       let update_fee = msgs::UpdateFee {
+               channel_id: chan.2,
+               feerate_per_kw: feerate + 124,
+       };
+
+       nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), &update_fee);
 
        //While producing the commitment_signed response after handling a received update_fee request the
        //check to see if the funder, who sent the update_fee request, can afford the new fee (funder_balance >= fee+channel_reserve)
        //Should produce and error.
-       nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &update2_msg.commitment_signed);
+       nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &commit_signed_msg);
        nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Funding remote cannot afford proposed new fee".to_string(), 1);
        check_added_monitors!(nodes[1], 1);
        check_closed_broadcast!(nodes[1], true);