Don't generate a commitment if we cannot afford a holding cell feerate

While an `update_fee` is in the holding cell, it is possible for HTLCs
to get added to the commitment transaction such that when we release the
holding cell, we can no longer afford this new feerate.

In that case, we previously would drop the fee update, but still send a
commitment (at the old feerate), which is a break of the specification.

We now stop generating this lonely commitment when the fee update gets
dropped upon release from the holding cell.

Author: tankyleo

lightning/src/ln/channel.rs

@@ -6275,14 +6275,11 @@ impl<SP: Deref> FundedChannel<SP> where
 					}
 				}
 			}
-			if update_add_count == 0 && update_fulfill_count == 0 && update_fail_count == 0 && self.context.holding_cell_update_fee.is_none() {
+			let update_fee = self.context.holding_cell_update_fee.take().and_then(|feerate| self.send_update_fee(feerate, false, fee_estimator, logger));
+
+			if update_add_count == 0 && update_fulfill_count == 0 && update_fail_count == 0 && update_fee.is_none() {
 				return (None, htlcs_to_fail);
 			}
-			let update_fee = if let Some(feerate) = self.context.holding_cell_update_fee.take() {
-				self.send_update_fee(feerate, false, fee_estimator, logger)
-			} else {
-				None
-			};
 
 			let mut additional_update = self.build_commitment_no_status_check(logger);
 			// build_commitment_no_status_check and get_update_fulfill_htlc may bump latest_monitor_id

lightning/src/ln/channelmanager.rs

@@ -6586,7 +6586,7 @@ where
 		NotifyOption::DoPersist
 	}
 
-	#[cfg(fuzzing)]
+	#[cfg(any(test, fuzzing))]
 	/// In chanmon_consistency we want to sometimes do the channel fee updates done in
 	/// timer_tick_occurred, but we can't generate the disabled channel updates as it considers
 	/// these a fuzz failure (as they usually indicate a channel force-close, which is exactly what

lightning/src/ln/update_fee_tests.rs

@@ -1009,3 +1009,134 @@ pub fn accept_busted_but_better_fee() {
 		_ => panic!("Unexpected event"),
 	};
 }
+
+#[xtest(feature = "_externalize_tests")]
+pub fn cant_afford_on_holding_cell_release() {
+	do_cant_afford_on_holding_cell_release(true);
+	do_cant_afford_on_holding_cell_release(false);
+}
+
+pub fn do_cant_afford_on_holding_cell_release(can_afford: bool) {
+	// Test that if we can't afford a feerate update when releasing an
+	// update_fee from its holding cell, we do not generate any msg events
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+
+	let mut default_config = test_default_channel_config();
+	default_config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true;
+	default_config.manually_accept_inbound_channels = true;
+	default_config.channel_handshake_config.max_inbound_htlc_value_in_flight_percent_of_channel = 100;
+
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(default_config.clone()), Some(default_config)]);
+
+	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let node_a_id = nodes[0].node.get_our_node_id();
+	let node_b_id = nodes[1].node.get_our_node_id();
+
+	let target_feerate = 257;
+	let channel_type = ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies();
+	// This is the number of htlcs that `send_update_fee` will account for when checking whether
+	// it can afford the new feerate upon releasing an update_fee from its holding cell,
+	// ie the buffer + the inbound HTLC we will add while the update_fee is in the holding cell
+	let num_htlcs = crate::ln::channel::CONCURRENT_INBOUND_HTLC_FEE_BUFFER + 1;
+	let commit_tx_fee_sat = chan_utils::commit_tx_fee_sat(target_feerate, num_htlcs as usize, &channel_type);
+	let anchor_value_satoshis = 2 * crate::ln::channel::ANCHOR_OUTPUT_VALUE_SATOSHI;
+	let channel_reserve_satoshis = 1000;
+
+	let channel_value_sat = 100_000;
+	let node_0_balance_sat = commit_tx_fee_sat + anchor_value_satoshis + channel_reserve_satoshis - if can_afford { 0 } else { 1 };
+	let node_1_balance_sat = channel_value_sat - node_0_balance_sat;
+
+	let chan_id = create_chan_between_nodes_with_value(&nodes[0], &nodes[1], channel_value_sat, 0).3;
+
+	// Set node 0's balance to the can/can't afford threshold
+	send_payment(&nodes[0], &[&nodes[1]], node_1_balance_sat * 1000);
+
+	{
+		// Sanity check the reserve
+		let per_peer_state_lock;
+		let mut peer_state_lock;
+		let chan =
+			get_channel_ref!(nodes[1], nodes[0], per_peer_state_lock, peer_state_lock, chan_id);
+		assert_eq!(chan.funding().holder_selected_channel_reserve_satoshis, channel_reserve_satoshis);
+	}
+
+	{
+		// Bump the feerate
+		let mut feerate_lock = chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
+		*feerate_lock += 4;
+		assert_eq!(*feerate_lock, target_feerate);
+	}
+
+	// Put the update fee into the holding cell of node 0
+
+	nodes[0].node.maybe_update_chan_fees();
+
+	// While the update_fee is in the holding cell, add an inbound HTLC
+
+	let (route, payment_hash, _, payment_secret) =
+		get_route_and_payment_hash!(nodes[1], nodes[0], 1000 * 1000);
+	let onion = RecipientOnionFields::secret_only(payment_secret);
+	let id = PaymentId(payment_hash.0);
+	nodes[1].node.send_payment_with_route(route, payment_hash, onion, id).unwrap();
+	check_added_monitors(&nodes[1], 1);
+
+	let payment_event = {
+		let mut events_1 = nodes[1].node.get_and_clear_pending_msg_events();
+		assert_eq!(events_1.len(), 1);
+		SendEvent::from_event(events_1.pop().unwrap())
+	};
+	assert_eq!(payment_event.node_id, node_a_id);
+	assert_eq!(payment_event.msgs.len(), 1);
+
+	nodes[0].node.handle_update_add_htlc(node_b_id, &payment_event.msgs[0]);
+	nodes[0].node.handle_commitment_signed(node_b_id, &payment_event.commitment_msg[0]);
+	check_added_monitors(&nodes[0], 1);
+
+	let (revoke_ack, commitment_signed) = get_revoke_commit_msgs!(nodes[0], node_b_id);
+
+	nodes[1].node.handle_revoke_and_ack(node_a_id, &revoke_ack);
+	check_added_monitors(&nodes[1], 1);
+	nodes[1].node.handle_commitment_signed(node_a_id, &commitment_signed[0]);
+	check_added_monitors(&nodes[1], 1);
+
+	let mut events = nodes[1].node.get_and_clear_pending_msg_events();
+	assert_eq!(events.len(), 1);
+
+	if let MessageSendEvent::SendRevokeAndACK { node_id, msg } = events.pop().unwrap() {
+		assert_eq!(node_id, node_a_id);
+		nodes[0].node.handle_revoke_and_ack(node_b_id, &msg);
+		check_added_monitors!(nodes[0], 1);
+	} else {
+		panic!();
+	}
+
+	let events = nodes[0].node.get_and_clear_pending_events();
+	assert_eq!(events.len(), 1);
+	assert!(matches!(events[0], Event::PendingHTLCsForwardable { .. }));
+
+	// Release the update_fee from its holding cell
+
+	let mut events = nodes[0].node.get_and_clear_pending_msg_events();
+	if can_afford {
+		// We could afford the update_fee, sanity check everything
+		if let MessageSendEvent::UpdateHTLCs { node_id, channel_id, updates } = events.pop().unwrap() {
+			assert_eq!(node_id, node_b_id);
+			assert_eq!(updates.commitment_signed.len(), 1);
+			assert_eq!(updates.commitment_signed[0].htlc_signatures.len(), 1);
+			assert_eq!(updates.update_add_htlcs.len(), 0);
+			assert_eq!(updates.update_fulfill_htlcs.len(), 0);
+			assert_eq!(updates.update_fail_htlcs.len(), 0);
+			assert_eq!(updates.update_fail_malformed_htlcs.len(), 0);
+			let update_fee = updates.update_fee.unwrap();
+			assert_eq!(update_fee.channel_id, chan_id);
+			assert_eq!(update_fee.feerate_per_kw, target_feerate);
+		} else {
+			panic!();
+		}
+	} else {
+		// We could not afford the update_fee, no events should be generated
+		assert_eq!(events.len(), 0);
+	}
+}