Disallow sending an HTLC when the balance needed is pending removal

While its nice to be able to push an HTLC which spends balance that
is removed in our local commitment transaction but awaiting an RAA
from our peer for final removal its by no means a critical feature.

Because peers should really be sending RAAs quickly after we send
a commitment, this should be an exceedingly rare case, and we
already don't expose this as available balance when routing, so
this isn't even made available when sending, only forwarding.

Note that `test_pending_claimed_htlc_no_balance_underflow` is
removed as it tested a case which was only possible because of this
and now is no longer possible.

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -5857,14 +5857,13 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 			return Err(ChannelError::Ignore(format!("Cannot send value that would put us over the max HTLC value in flight our peer will accept ({})", self.counterparty_max_htlc_value_in_flight_msat)));
 		}
 
-		let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number);
-		let commitment_stats = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, true, logger);
 		if !self.is_outbound() {
 			// Check that we won't violate the remote channel reserve by adding this HTLC.
 			let htlc_candidate = HTLCCandidate::new(amount_msat, HTLCInitiator::LocalOffered);
 			let counterparty_commit_tx_fee_msat = self.next_remote_commit_tx_fee_msat(htlc_candidate, None);
 			let holder_selected_chan_reserve_msat = self.holder_selected_channel_reserve_satoshis * 1000;
-			if commitment_stats.remote_balance_msat < counterparty_commit_tx_fee_msat + holder_selected_chan_reserve_msat {
+			let remote_balance_msat = (self.channel_value_satoshis * 1000 - self.value_to_self_msat).saturating_sub(inbound_stats.pending_htlcs_value_msat);
+			if remote_balance_msat < counterparty_commit_tx_fee_msat + holder_selected_chan_reserve_msat {
 				return Err(ChannelError::Ignore("Cannot send value that would put counterparty balance under holder-announced channel reserve value".to_owned()));
 			}
 		}
@@ -5894,7 +5893,8 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 			}
 		}
 
-		let holder_balance_msat = commitment_stats.local_balance_msat - outbound_stats.holding_cell_msat;
+		let holder_balance_msat = self.value_to_self_msat
+			.saturating_sub(outbound_stats.pending_htlcs_value_msat);
 		if holder_balance_msat < amount_msat {
 			return Err(ChannelError::Ignore(format!("Cannot send value that would overdraw remaining funds. Amount: {}, pending value to self {}", amount_msat, holder_balance_msat)));
 		}
@@ -5915,7 +5915,13 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 			return Err(ChannelError::Ignore(format!("Cannot send value that would put our balance under counterparty-announced channel reserve value ({})", chan_reserve_msat)));
 		}
 
-		if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateInProgress as u32)) != 0 {
+		let need_holding_cell = (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateInProgress as u32)) != 0;
+		log_debug!(logger, "Pushing new outbound HTLC for {} msat {}", amount_msat,
+			if force_holding_cell { "into holding cell" }
+			else if need_holding_cell { "into holding cell as we're awaiting an RAA or monitor" }
+			else { "to peer" });
+
+		if need_holding_cell {
 			force_holding_cell = true;
 		}
 

lightning/src/ln/functional_tests.rs

@@ -7618,45 +7618,6 @@ fn test_bump_txn_sanitize_tracking_maps() {
 	}
 }
 
-#[test]
-fn test_pending_claimed_htlc_no_balance_underflow() {
-	// Tests that if we have a pending outbound HTLC as well as a claimed-but-not-fully-removed
-	// HTLC we will not underflow when we call `Channel::get_balance_msat()`.
-	let 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_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 0);
-
-	let (payment_preimage, payment_hash, _) = route_payment(&nodes[0], &[&nodes[1]], 1_010_000);
-	nodes[1].node.claim_funds(payment_preimage);
-	expect_payment_claimed!(nodes[1], payment_hash, 1_010_000);
-	check_added_monitors!(nodes[1], 1);
-	let fulfill_ev = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
-
-	nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &fulfill_ev.update_fulfill_htlcs[0]);
-	expect_payment_sent_without_paths!(nodes[0], payment_preimage);
-	nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &fulfill_ev.commitment_signed);
-	check_added_monitors!(nodes[0], 1);
-	let (_raa, _cs) = get_revoke_commit_msgs!(nodes[0], nodes[1].node.get_our_node_id());
-
-	// At this point nodes[1] has received 1,010k msat (10k msat more than their reserve) and can
-	// send an HTLC back (though it will go in the holding cell). Send an HTLC back and check we
-	// can get our balance.
-
-	// Get a route from nodes[1] to nodes[0] by getting a route going the other way and then flip
-	// the public key of the only hop. This works around ChannelDetails not showing the
-	// almost-claimed HTLC as available balance.
-	let (mut route, _, _, _) = get_route_and_payment_hash!(nodes[0], nodes[1], 10_000);
-	route.payment_params = None; // This is all wrong, but unnecessary
-	route.paths[0].hops[0].pubkey = nodes[0].node.get_our_node_id();
-	let (_, payment_hash_2, payment_secret_2) = get_payment_preimage_hash!(nodes[0]);
-	nodes[1].node.send_payment_with_route(&route, payment_hash_2,
-		RecipientOnionFields::secret_only(payment_secret_2), PaymentId(payment_hash_2.0)).unwrap();
-
-	assert_eq!(nodes[1].node.list_channels()[0].balance_msat, 1_000_000);
-}
-
 #[test]
 fn test_channel_conf_timeout() {
 	// Tests that, for inbound channels, we give up on them if the funding transaction does not