Cancel the outbound feerate update if above what we can afford

Author: Antoine Riard

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,
@@ -2456,6 +2456,13 @@ impl<Signer: Sign> Channel<Signer> {
 						}
 				}
 			}
+			if update_fee {
+				let total_fee = Channel::<Signer>::commit_tx_fee_sat(feerate_per_kw, num_htlcs + /* HTLC feerate buffer */ 2);
+				let counterparty_reserve_we_require = Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis);
+				if self.channel_value_satoshis - self.value_to_self_msat / 1000 < total_fee + counterparty_reserve_we_require {
+					return Err((None, ChannelError::Close("Funding remote cannot afford proposed new fee with feerate buffer".to_owned())));
+				}
+			}
 		}
 
 		if msg.htlc_signatures.len() != num_htlcs {
@@ -2958,7 +2965,7 @@ 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.
 	/// 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 +2976,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 a 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 = 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).map(|a| a.checked_sub(outbound_stats.pending_htlcs_value_msat / 1000)).unwrap_or(None).unwrap_or(u64::max_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 +3011,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)))
@@ -4728,6 +4755,19 @@ 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 = 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).map(|a| a.checked_sub(outbound_stats.pending_htlcs_value_msat / 1000)).unwrap_or(None).unwrap_or(u64::max_value());
+				if holder_balance_after_fee_sub_sats < self.counterparty_selected_channel_reserve_satoshis.unwrap() {
+					log_trace!(logger, "Outbound update_fee HTLC buffer overflow - counterparty should force-close this channel");
+				}
+			}
+		}
+
 		{
 			let mut htlcs = Vec::with_capacity(counterparty_commitment_tx.3.len());
 			for &(ref htlc, _) in counterparty_commitment_tx.3.iter() {

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,20 +623,74 @@ 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,
+			1088,
+			false, local_funding, remote_funding,
+			commit_tx_keys.clone(),
+			feerate + 2,
+			&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 + 2,
+	};
+
+	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].logger.assert_log("lightning::ln::channelmanager".to_string(), "Funding remote cannot afford proposed new fee".to_string(), 1);
+	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 with feerate buffer".to_string(), 1);
 	check_added_monitors!(nodes[1], 1);
 	check_closed_broadcast!(nodes[1], true);
-	check_closed_event!(nodes[1], 1, ClosureReason::ProcessingError { err: String::from("Funding remote cannot afford proposed new fee") });
+	check_closed_event!(nodes[1], 1, ClosureReason::ProcessingError { err: String::from("Funding remote cannot afford proposed new fee with feerate buffer") });
 }
 
 #[test]