From 34fd833b72cd231843684138cbaa7cd63ee98c3e Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Sat, 21 Aug 2021 18:05:51 -0400 Subject: [PATCH] Cancel the outbound feerate update if above what we can afford --- fuzz/src/chanmon_consistency.rs | 36 ++++++++++----- fuzz/src/utils/test_logger.rs | 7 +++ lightning/src/ln/channel.rs | 40 +++++++++++++++-- lightning/src/ln/functional_tests.rs | 67 +++++++++++++++++++++++++--- 4 files changed, 131 insertions(+), 19 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index f3b952ff9..4995faba6 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -732,7 +732,10 @@ pub fn do_test(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(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(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(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(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); diff --git a/fuzz/src/utils/test_logger.rs b/fuzz/src/utils/test_logger.rs index f8c96f99b..ef6344ed4 100644 --- a/fuzz/src/utils/test_logger.rs +++ b/fuzz/src/utils/test_logger.rs @@ -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>); @@ -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 { diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 5bd47c329..23c5a9dbb 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -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 Channel { /// 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 { + fn send_update_fee(&mut self, feerate_per_kw: u32, logger: &L) -> Option where L::Target: Logger { if !self.is_outbound() { panic!("Cannot send fee from inbound channel"); } @@ -2969,6 +2971,26 @@ impl Channel { 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::::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 Channel { } pub fn send_update_fee_and_commit(&mut self, feerate_per_kw: u32, logger: &L) -> Result, 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 Channel { } } } + 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::::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()); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index a8a0e32f1..6fa7858f0 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -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); -- 2.39.5