Consider anchor outputs value on inbound HTLCs

This could lead us to accept HTLCs that would put the sender below
their reserve, which must never happen.

Author: wpaulino

lightning/src/ln/channel.rs

@@ -2780,6 +2780,13 @@ impl<SP: Deref> Channel<SP> where
 		if inbound_stats.pending_htlcs_value_msat + msg.amount_msat > self.context.holder_max_htlc_value_in_flight_msat {
 			return Err(ChannelError::Close(format!("Remote HTLC add would put them over our max HTLC value ({})", self.context.holder_max_htlc_value_in_flight_msat)));
 		}
+
+		let anchor_outputs_value_msat = if self.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
+			ANCHOR_OUTPUT_VALUE_SATOSHI * 2 * 1000
+		} else {
+			0
+		};
+
 		// Check holder_selected_channel_reserve_satoshis (we're getting paid, so they have to at least meet
 		// the reserve_satoshis we told them to always have as direct payment so that they lose
 		// something if we punish them for broadcasting an old state).
@@ -2843,11 +2850,11 @@ impl<SP: Deref> Channel<SP> where
 			let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
 			self.context.next_remote_commit_tx_fee_msat(htlc_candidate, None) // Don't include the extra fee spike buffer HTLC in calculations
 		};
-		if pending_remote_value_msat - msg.amount_msat < remote_commit_tx_fee_msat {
+		if pending_remote_value_msat - msg.amount_msat - anchor_outputs_value_msat < remote_commit_tx_fee_msat {
 			return Err(ChannelError::Close("Remote HTLC add would not leave enough to pay for fees".to_owned()));
 		};
 
-		if pending_remote_value_msat - msg.amount_msat - remote_commit_tx_fee_msat < self.context.holder_selected_channel_reserve_satoshis * 1000 {
+		if pending_remote_value_msat - msg.amount_msat - remote_commit_tx_fee_msat - anchor_outputs_value_msat < self.context.holder_selected_channel_reserve_satoshis * 1000 {
 			return Err(ChannelError::Close("Remote HTLC add would put them under remote reserve value".to_owned()));
 		}
 
@@ -2862,7 +2869,7 @@ impl<SP: Deref> Channel<SP> where
 			// sensitive to fee spikes.
 			let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
 			let remote_fee_cost_incl_stuck_buffer_msat = 2 * self.context.next_remote_commit_tx_fee_msat(htlc_candidate, Some(()));
-			if pending_remote_value_msat - msg.amount_msat - self.context.holder_selected_channel_reserve_satoshis * 1000 < remote_fee_cost_incl_stuck_buffer_msat {
+			if pending_remote_value_msat - msg.amount_msat - self.context.holder_selected_channel_reserve_satoshis * 1000 - anchor_outputs_value_msat < remote_fee_cost_incl_stuck_buffer_msat {
 				// Note that if the pending_forward_status is not updated here, then it's because we're already failing
 				// the HTLC, i.e. its status is already set to failing.
 				log_info!(logger, "Attempting to fail HTLC due to fee spike buffer violation in channel {}. Rebalancing is required.", &self.context.channel_id());
@@ -2872,7 +2879,7 @@ impl<SP: Deref> Channel<SP> where
 			// Check that they won't violate our local required channel reserve by adding this HTLC.
 			let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
 			let local_commit_tx_fee_msat = self.context.next_local_commit_tx_fee_msat(htlc_candidate, None);
-			if self.context.value_to_self_msat < self.context.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 + local_commit_tx_fee_msat {
+			if self.context.value_to_self_msat < self.context.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 + local_commit_tx_fee_msat + anchor_outputs_value_msat {
 				return Err(ChannelError::Close("Cannot accept HTLC that would put our balance under counterparty-announced channel reserve value".to_owned()));
 			}
 		}

lightning/src/ln/monitor_tests.rs

@@ -1400,7 +1400,7 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
 
 	// Create some initial channels
 	let (_, _, chan_id, funding_tx) =
-		create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 11_000_000);
+		create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 12_000_000);
 	let funding_outpoint = OutPoint { txid: funding_tx.txid(), index: 0 };
 	assert_eq!(funding_outpoint.to_channel_id(), chan_id);
 
@@ -1410,9 +1410,9 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
 	assert_eq!(revoked_local_txn[0].input.len(), 1);
 	assert_eq!(revoked_local_txn[0].input[0].previous_output.txid, funding_tx.txid());
 	if anchors {
-		assert_eq!(revoked_local_txn[0].output[4].value, 10000); // to_self output
+		assert_eq!(revoked_local_txn[0].output[4].value, 11000); // to_self output
 	} else {
-		assert_eq!(revoked_local_txn[0].output[2].value, 10000); // to_self output
+		assert_eq!(revoked_local_txn[0].output[2].value, 11000); // to_self output
 	}
 
 	// The to-be-revoked commitment tx should have two HTLCs, an output for each side, and an
@@ -1499,11 +1499,11 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
 	let anchor_outputs_value = if anchors { channel::ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 };
 	let as_balances = sorted_vec(vec![Balance::ClaimableAwaitingConfirmations {
 			// to_remote output in B's revoked commitment
-			amount_satoshis: 1_000_000 - 11_000 - 3_000 - commitment_tx_fee - anchor_outputs_value,
+			amount_satoshis: 1_000_000 - 12_000 - 3_000 - commitment_tx_fee - anchor_outputs_value,
 			confirmation_height: to_remote_conf_height,
 		}, Balance::CounterpartyRevokedOutputClaimable {
 			// to_self output in B's revoked commitment
-			amount_satoshis: 10_000,
+			amount_satoshis: 11_000,
 		}, Balance::CounterpartyRevokedOutputClaimable { // HTLC 1
 			amount_satoshis: 3_000,
 		}, Balance::CounterpartyRevokedOutputClaimable { // HTLC 2
@@ -1544,11 +1544,11 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
 	mine_transaction(&nodes[0], &as_htlc_claim_tx[0]);
 	assert_eq!(sorted_vec(vec![Balance::ClaimableAwaitingConfirmations {
 			// to_remote output in B's revoked commitment
-			amount_satoshis: 1_000_000 - 11_000 - 3_000 - commitment_tx_fee - anchor_outputs_value,
+			amount_satoshis: 1_000_000 - 12_000 - 3_000 - commitment_tx_fee - anchor_outputs_value,
 			confirmation_height: to_remote_conf_height,
 		}, Balance::CounterpartyRevokedOutputClaimable {
 			// to_self output in B's revoked commitment
-			amount_satoshis: 10_000,
+			amount_satoshis: 11_000,
 		}, Balance::CounterpartyRevokedOutputClaimable { // HTLC 2
 			amount_satoshis: 1_000,
 		}, Balance::ClaimableAwaitingConfirmations {
@@ -1561,7 +1561,7 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
 	test_spendable_output(&nodes[0], &revoked_local_txn[0], false);
 	assert_eq!(sorted_vec(vec![Balance::CounterpartyRevokedOutputClaimable {
 			// to_self output to B
-			amount_satoshis: 10_000,
+			amount_satoshis: 11_000,
 		}, Balance::CounterpartyRevokedOutputClaimable { // HTLC 2
 			amount_satoshis: 1_000,
 		}, Balance::ClaimableAwaitingConfirmations {
@@ -1574,7 +1574,7 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
 	test_spendable_output(&nodes[0], &as_htlc_claim_tx[0], false);
 	assert_eq!(sorted_vec(vec![Balance::CounterpartyRevokedOutputClaimable {
 			// to_self output in B's revoked commitment
-			amount_satoshis: 10_000,
+			amount_satoshis: 11_000,
 		}, Balance::CounterpartyRevokedOutputClaimable { // HTLC 2
 			amount_satoshis: 1_000,
 		}]),
@@ -1623,7 +1623,7 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
 	connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
 	assert_eq!(sorted_vec(vec![Balance::CounterpartyRevokedOutputClaimable {
 			// to_self output in B's revoked commitment
-			amount_satoshis: 10_000,
+			amount_satoshis: 11_000,
 		}, Balance::CounterpartyRevokedOutputClaimable { // HTLC 2
 			amount_satoshis: 1_000,
 		}]),
@@ -1632,7 +1632,7 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
 	mine_transaction(&nodes[0], &revoked_htlc_timeout_claim);
 	assert_eq!(sorted_vec(vec![Balance::CounterpartyRevokedOutputClaimable {
 			// to_self output in B's revoked commitment
-			amount_satoshis: 10_000,
+			amount_satoshis: 11_000,
 		}, Balance::ClaimableAwaitingConfirmations {
 			amount_satoshis: revoked_htlc_timeout_claim.output[0].value,
 			confirmation_height: nodes[0].best_block_info().1 + ANTI_REORG_DELAY - 1,

lightning/src/ln/payment_tests.rs

@@ -4093,3 +4093,81 @@ fn test_payment_metadata_consistency() {
 	do_test_payment_metadata_consistency(false, true);
 	do_test_payment_metadata_consistency(false, false);
 }
+
+#[test]
+fn  test_htlc_forward_considers_anchor_outputs_value() {
+	// Tests that:
+	//
+	// 1) Forwarding nodes don't forward HTLCs that would cause their balance to dip below the
+	//    reserve when considering the value of anchor outputs.
+	//
+	// 2) Recipients of `update_add_htlc` properly reject HTLCs that would cause the initiator's
+	//    balance to dip below the reserve when considering the value of anchor outputs.
+	let mut anchors_config = test_default_channel_config();
+	anchors_config.channel_handshake_limits.max_funding_satoshis = 100_000_000;
+	anchors_config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true;
+	anchors_config.manually_accept_inbound_channels = true;
+
+	// Set up a test network of three nodes that replicates a production failure leading to the
+	// discovery of this bug.
+	let chanmon_cfgs = create_chanmon_cfgs(3);
+	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[Some(anchors_config), Some(anchors_config), Some(anchors_config)]);
+	let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+
+	*nodes[0].fee_estimator.sat_per_kw.lock().unwrap() = 293;
+	*nodes[1].fee_estimator.sat_per_kw.lock().unwrap() = 293;
+	*nodes[2].fee_estimator.sat_per_kw.lock().unwrap() = 293;
+
+	create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 20_000_000, 0);
+	let (_, _, chan_id_2, _) = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 20_000_000, 19_706_357_000);
+
+	// Send out an HTLC that would cause the forwarding node to dip below its reserve when
+	// considering the value of anchor outputs.
+	let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], 92_761_092);
+	nodes[0].node.send_payment_with_route(
+		&route, payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0)
+	).unwrap();
+	check_added_monitors!(nodes[0], 1);
+
+	let mut events = nodes[0].node.get_and_clear_pending_msg_events();
+	assert_eq!(events.len(), 1);
+	let mut update_add_htlc = if let MessageSendEvent::UpdateHTLCs { updates, .. } = events.pop().unwrap() {
+		nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &updates.update_add_htlcs[0]);
+		check_added_monitors(&nodes[1], 0);
+		commitment_signed_dance!(nodes[1], nodes[0], &updates.commitment_signed, false);
+		updates.update_add_htlcs[0].clone()
+	} else {
+		panic!("Unexpected event");
+	};
+
+	// The forwarding node should reject forwarding it as expected.
+	expect_pending_htlcs_forwardable!(nodes[1]);
+	expect_pending_htlcs_forwardable_and_htlc_handling_failed!(&nodes[1], vec![HTLCDestination::NextHopChannel {
+		node_id: Some(nodes[2].node.get_our_node_id()),
+		channel_id: chan_id_2
+	}]);
+	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::UpdateHTLCs { updates, .. } = events.pop().unwrap() {
+		nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
+		check_added_monitors(&nodes[0], 0);
+		commitment_signed_dance!(nodes[0], nodes[1], &updates.commitment_signed, false);
+	} else {
+		panic!("Unexpected event");
+	}
+
+	expect_payment_failed!(nodes[0], payment_hash, false);
+
+	// Assume that the forwarding node did forward it, and make sure the recipient rejects it as an
+	// invalid update and closes the channel.
+	update_add_htlc.channel_id = chan_id_2;
+	nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &update_add_htlc);
+	check_closed_event(&nodes[2], 1, ClosureReason::ProcessingError {
+		err: "Remote HTLC add would put them under remote reserve value".to_owned()
+	}, false, &[nodes[1].node.get_our_node_id()], 20_000_000);
+	check_closed_broadcast(&nodes[2], 1, true);
+	check_added_monitors(&nodes[2], 1);
+}