Correct non-dust HTLC accounting in next_remote_commit_tx_fee_msat

`next_remote_commit_tx_fee_msat` previously mistakenly classified HTLCs
with values equal to the dust limit as dust.

This did not cause any force closes because the code that builds
commitment transactions for signing correctly trims dust HTLCs.

Nonetheless, this can cause `next_remote_commit_tx_fee_msat` to predict
a weight for the next remote commitment transaction that is
significantly lower than the eventual weight. This allows a malicious
channel funder to create an unbroadcastable commitment for the channel
fundee by adding HTLCs with values equal to the dust limit to the
commitment transaction; according to the fundee, the funder has not
exhausted their reserve because all the added HTLCs are dust, while in
reality all the HTLCs are non-dust, and the funder does not have the
funds to pay the minimum feerate to enter the mempool.

Conflicts resolved in:
 * lightning/src/ln/htlc_reserve_unit_tests.rs which is a new file
   upstream. The new test was instead moved to
   lightning/src/ln/functional_tests.rs and rewritten where the
   upstream API has changed (in some cases nontrivially).

Author: tankyleo

lightning/src/ln/channel.rs

@@ -3216,7 +3216,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 	/// Creates a set of keys for build_commitment_transaction to generate a transaction which we
 	/// will sign and send to our counterparty.
 	/// If an Err is returned, it is a ChannelError::Close (for get_funding_created)
-	fn build_remote_transaction_keys(&self) -> TxCreationKeys {
+	pub fn build_remote_transaction_keys(&self) -> TxCreationKeys {
 		let revocation_basepoint = &self.get_holder_pubkeys().revocation_basepoint;
 		let htlc_basepoint = &self.get_holder_pubkeys().htlc_basepoint;
 		let counterparty_pubkeys = self.get_counterparty_pubkeys();
@@ -3774,14 +3774,14 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
 		// committed outbound HTLCs, see below.
 		let mut included_htlcs = 0;
 		for ref htlc in context.pending_inbound_htlcs.iter() {
-			if htlc.amount_msat / 1000 <= real_dust_limit_timeout_sat {
+			if htlc.amount_msat / 1000 < real_dust_limit_timeout_sat {
 				continue
 			}
 			included_htlcs += 1;
 		}
 
 		for ref htlc in context.pending_outbound_htlcs.iter() {
-			if htlc.amount_msat / 1000 <= real_dust_limit_success_sat {
+			if htlc.amount_msat / 1000 < real_dust_limit_success_sat {
 				continue
 			}
 			// We only include outbound HTLCs if it will not be included in their next commitment_signed,

lightning/src/ln/functional_tests.rs

@@ -11684,3 +11684,293 @@ fn test_funding_signed_event() {
 	nodes[1].node.get_and_clear_pending_msg_events();
 }
 
+#[test]
+pub fn test_dust_limit_fee_accounting() {
+	do_test_dust_limit_fee_accounting(false);
+	do_test_dust_limit_fee_accounting(true);
+}
+
+pub fn do_test_dust_limit_fee_accounting(can_afford: bool) {
+	// Test that when a channel funder sends HTLCs exactly on the dust limit
+	// of the funder, the fundee correctly accounts for the additional fee on the
+	// funder's commitment transaction due to those additional non-dust HTLCs when
+	// checking for any infrigements on the funder's reserve.
+
+	let channel_type = ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies();
+
+	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;
+
+	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();
+
+	// Set a HTLC amount that is equal to the dust limit of the funder
+	const HTLC_AMT_SAT: u64 = 354;
+
+	const CHANNEL_VALUE_SAT: u64 = 100_000;
+
+	const FEERATE_PER_KW: u32 = 253;
+
+	let commit_tx_fee_sat =
+		chan_utils::commit_tx_fee_sat(FEERATE_PER_KW, MIN_AFFORDABLE_HTLC_COUNT, &channel_type);
+
+	// By default the reserve is set to 1% or 1000sat, whichever is higher
+	let channel_reserve_satoshis = 1_000;
+
+	// Set node 0's balance to pay for exactly MIN_AFFORDABLE_HTLC_COUNT non-dust HTLCs on the channel, minus some offset
+	let node_0_balance_sat = commit_tx_fee_sat
+		+ channel_reserve_satoshis
+		+ 2 * crate::ln::channel::ANCHOR_OUTPUT_VALUE_SATOSHI
+		+ MIN_AFFORDABLE_HTLC_COUNT as u64 * HTLC_AMT_SAT
+		- if can_afford { 0 } else { 1 };
+	let mut 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,
+		node_1_balance_sat * 1000,
+	)
+	.3;
+
+	{
+		// Double check the reserve that node 0 has to maintain here
+		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.context().holder_selected_channel_reserve_satoshis,
+			channel_reserve_satoshis
+		);
+	}
+	{
+		// Double check the dust limit on node 0's commitment transactions; when node 0
+		// adds a HTLC, node 1 will check that the fee on node 0's commitment transaction
+		// does not dip under the node 1 selected reserve.
+		let per_peer_state_lock;
+		let mut peer_state_lock;
+		let chan =
+			get_channel_ref!(nodes[0], nodes[1], per_peer_state_lock, peer_state_lock, chan_id);
+		assert_eq!(chan.context().holder_dust_limit_satoshis, HTLC_AMT_SAT);
+	}
+
+	// Precompute the route to skip any router complaints when sending the last HTLC
+	let (route_0_1, payment_hash_0_1, _, payment_secret_0_1) =
+		get_route_and_payment_hash!(nodes[0], nodes[1], HTLC_AMT_SAT * 1000);
+
+	let mut htlcs = Vec::new();
+	for _ in 0..MIN_AFFORDABLE_HTLC_COUNT - 1 {
+		let (_payment_preimage, payment_hash, ..) =
+			route_payment(&nodes[0], &[&nodes[1]], HTLC_AMT_SAT * 1000);
+		// Grab a snapshot of these HTLCs to manually build the commitment transaction later...
+		let accepted_htlc = chan_utils::HTLCOutputInCommitment {
+			offered: false,
+			amount_msat: HTLC_AMT_SAT * 1000,
+			// Hard-coded to match the expected value
+			cltv_expiry: 81,
+			payment_hash,
+			transaction_output_index: None,
+		};
+		htlcs.push((accepted_htlc, ()));
+	}
+
+	// Need to manually create the update_add_htlc message to go around the channel reserve check in send_htlc()
+	let secp_ctx = Secp256k1::new();
+	let session_priv = SecretKey::from_slice(&[42; 32]).expect("RNG is bad!");
+
+	let cur_height = nodes[1].node.best_block.read().unwrap().height + 1;
+
+	let onion_keys =
+		onion_utils::construct_onion_keys(&secp_ctx, &route_0_1.paths[0], &session_priv).unwrap();
+	let recipient_onion_fields = RecipientOnionFields::secret_only(payment_secret_0_1);
+	let (onion_payloads, amount_msat, cltv_expiry) = onion_utils::build_onion_payloads(
+		&route_0_1.paths[0],
+		HTLC_AMT_SAT * 1000,
+		&recipient_onion_fields,
+		cur_height,
+		&None,
+		None,
+	)
+	.unwrap();
+	let onion_routing_packet =
+		onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash_0_1)
+			.unwrap();
+	// Double check the hard-coded value
+	assert_eq!(cltv_expiry, 81);
+	let msg = msgs::UpdateAddHTLC {
+		channel_id: chan_id,
+		htlc_id: MIN_AFFORDABLE_HTLC_COUNT as u64 - 1,
+		amount_msat,
+		payment_hash: payment_hash_0_1,
+		cltv_expiry,
+		onion_routing_packet,
+		skimmed_fee_msat: None,
+		blinding_point: None,
+	};
+
+	nodes[1].node.handle_update_add_htlc(node_a_id, &msg);
+
+	if !can_afford {
+		let err = "Remote HTLC add would put them under remote reserve value".to_string();
+		nodes[1].logger.assert_log_contains("lightning::ln::channelmanager", &err, 3);
+		let events = nodes[1].node.get_and_clear_pending_msg_events();
+		assert_eq!(events.len(), 2);
+		let reason = ClosureReason::ProcessingError { err };
+		check_closed_event(&nodes[1], 1, reason, false, &[node_a_id], CHANNEL_VALUE_SAT);
+		check_added_monitors(&nodes[1], 1);
+	} else {
+		// Now manually create the commitment_signed message corresponding to the update_add
+		// nodes[0] just sent. In the code for construction of this message, "local" refers
+		// to the sender of the message, and "remote" refers to the receiver.
+
+		const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1;
+
+		let (local_secret, next_local_point) = {
+			let per_peer_lock;
+			let mut peer_state_lock;
+
+			let channel =
+				get_channel_ref!(nodes[0], nodes[1], per_peer_lock, peer_state_lock, chan_id);
+			let local_chan = if let ChannelPhase::Funded(chan) = &*channel {
+				chan
+			} else {
+				panic!();
+			};
+			let chan_signer = local_chan.get_signer();
+			// Make the signer believe we validated another commitment, so we can release the secret
+			chan_signer.as_ecdsa().unwrap().get_enforcement_state().last_holder_commitment -= 1;
+
+			(
+				chan_signer
+					.as_ref()
+					.release_commitment_secret(
+						INITIAL_COMMITMENT_NUMBER - MIN_AFFORDABLE_HTLC_COUNT as u64 + 1,
+					)
+					.unwrap(),
+				chan_signer
+					.as_ref()
+					.get_per_commitment_point(
+						INITIAL_COMMITMENT_NUMBER - MIN_AFFORDABLE_HTLC_COUNT as u64,
+						&secp_ctx,
+					)
+					.unwrap(),
+			)
+		};
+
+		// Build the remote commitment transaction so we can sign it, and then later use the
+		// signature for the commitment_signed message.
+		let local_chan_balance = node_0_balance_sat
+			- HTLC_AMT_SAT * MIN_AFFORDABLE_HTLC_COUNT as u64
+			- 2 * crate::ln::channel::ANCHOR_OUTPUT_VALUE_SATOSHI
+			- chan_utils::commit_tx_fee_sat(
+				FEERATE_PER_KW,
+				MIN_AFFORDABLE_HTLC_COUNT,
+				&channel_type,
+			);
+
+		let accepted_htlc_info = chan_utils::HTLCOutputInCommitment {
+			offered: false,
+			amount_msat: HTLC_AMT_SAT * 1000,
+			cltv_expiry,
+			payment_hash: payment_hash_0_1,
+			transaction_output_index: None,
+		};
+		htlcs.push((accepted_htlc_info, ()));
+
+		let commitment_number = INITIAL_COMMITMENT_NUMBER - MIN_AFFORDABLE_HTLC_COUNT as u64;
+
+		let res = {
+			let per_peer_lock;
+			let mut peer_state_lock;
+
+			let channel =
+				get_channel_ref!(nodes[0], nodes[1], per_peer_lock, peer_state_lock, chan_id);
+			let chan_signer = if let ChannelPhase::Funded(chan) = &*channel {
+				chan.get_signer()
+			} else {
+				panic!();
+			};
+
+			let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data(
+				commitment_number,
+				node_1_balance_sat,
+				local_chan_balance,
+				channel.context().channel_transaction_parameters.counterparty_parameters.as_ref().unwrap().pubkeys.funding_pubkey,
+				channel.context().channel_transaction_parameters.holder_pubkeys.funding_pubkey,
+				channel.context().build_remote_transaction_keys(),
+				FEERATE_PER_KW,
+				&mut htlcs,
+				&channel.context().channel_transaction_parameters.as_counterparty_broadcastable(),
+			);
+			chan_signer
+				.as_ecdsa()
+				.unwrap()
+				.sign_counterparty_commitment(
+					&commitment_tx,
+					Vec::new(),
+					Vec::new(),
+					&secp_ctx,
+				)
+				.unwrap()
+		};
+
+		let commit_signed_msg = msgs::CommitmentSigned {
+			channel_id: chan_id,
+			signature: res.0,
+			htlc_signatures: res.1,
+			batch: None,
+			#[cfg(taproot)]
+			partial_signature_with_nonce: None,
+		};
+
+		// Send the commitment_signed message to the nodes[1].
+		nodes[1].node.handle_commitment_signed(node_a_id, &commit_signed_msg);
+		let _ = nodes[1].node.get_and_clear_pending_msg_events();
+
+		// Send the RAA to nodes[1].
+		let raa_msg = msgs::RevokeAndACK {
+			channel_id: chan_id,
+			per_commitment_secret: local_secret,
+			next_per_commitment_point: next_local_point,
+			#[cfg(taproot)]
+			next_local_nonce: None,
+		};
+		nodes[1].node.handle_revoke_and_ack(node_a_id, &raa_msg);
+
+		// The HTLC actually fails here in `fn validate_commitment_signed` due to a fee spike buffer
+		// violation. It nonetheless passed all checks in `fn validate_update_add_htlc`.
+
+		//expect_pending_htlcs_forwardable!(nodes[1]);
+		expect_htlc_handling_failed_destinations!(
+			nodes[1].node.get_and_clear_pending_events(),
+			&[HTLCDestination::FailedPayment { payment_hash: payment_hash_0_1 }]
+		);
+
+		let events = nodes[1].node.get_and_clear_pending_msg_events();
+		assert_eq!(events.len(), 1);
+		// Make sure the HTLC failed in the way we expect.
+		match events[0] {
+			MessageSendEvent::UpdateHTLCs {
+				updates: msgs::CommitmentUpdate { ref update_fail_htlcs, .. },
+				..
+			} => {
+				assert_eq!(update_fail_htlcs.len(), 1);
+				update_fail_htlcs[0].clone()
+			},
+			_ => panic!("Unexpected event"),
+		};
+		nodes[1].logger.assert_log("lightning::ln::channel",
+			format!("Attempting to fail HTLC due to fee spike buffer violation in channel {}. Rebalancing is required.", raa_msg.channel_id), 1);
+
+		check_added_monitors(&nodes[1], 2);
+	}
+}