-f fix dust htlcs out of fee

Author: Antoine Riard

fuzz/src/chanmon_consistency.rs

@@ -1131,17 +1131,12 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
 					break;
 				}
 
-				// Finally, make sure that at least one end of each channel can make a substantial payment.
-				if out.may_fail.load(atomic::Ordering::Acquire) {
-					return;
-				} else {
-					assert!(
-						send_payment(&nodes[0], &nodes[1], chan_a, 10_000_000, &mut payment_id) ||
-						send_payment(&nodes[1], &nodes[0], chan_a, 10_000_000, &mut payment_id));
-					assert!(
-						send_payment(&nodes[1], &nodes[2], chan_b, 10_000_000, &mut payment_id) ||
-						send_payment(&nodes[2], &nodes[1], chan_b, 10_000_000, &mut payment_id));
-				}
+				assert!(
+					send_payment(&nodes[0], &nodes[1], chan_a, 10_000_000, &mut payment_id) ||
+					send_payment(&nodes[1], &nodes[0], chan_a, 10_000_000, &mut payment_id));
+				assert!(
+					send_payment(&nodes[1], &nodes[2], chan_b, 10_000_000, &mut payment_id) ||
+					send_payment(&nodes[2], &nodes[1], chan_b, 10_000_000, &mut payment_id));
 
 				last_htlc_clear_fee_a = fee_est_a.ret_val.load(atomic::Ordering::Acquire);
 				last_htlc_clear_fee_b = fee_est_b.ret_val.load(atomic::Ordering::Acquire);

lightning/src/ln/channel.rs

@@ -1040,7 +1040,7 @@ impl<Signer: Sign> Channel<Signer> {
 	/// Note that below-dust HTLCs are included in the fourth return value, but not the third, and
 	/// sources are provided only for outbound HTLCs in the fourth return value.
 	#[inline]
-	fn build_commitment_transaction<L: Deref>(&self, commitment_number: u64, keys: &TxCreationKeys, local: bool, generated_by_local: bool, logger: &L) -> (CommitmentTransaction, u32, usize, Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>) where L::Target: Logger {
+	fn build_commitment_transaction<L: Deref>(&self, commitment_number: u64, keys: &TxCreationKeys, local: bool, generated_by_local: bool, htlc_buffer: usize, next_feerate_per_kw: Option<u32>, logger: &L) -> (CommitmentTransaction, u32, usize, Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>, i64, i64) where L::Target: Logger {
 		let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new();
 		let num_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len();
 		let mut included_non_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs);
@@ -1062,6 +1062,9 @@ impl<Signer: Sign> Channel<Signer> {
 				feerate_per_kw = feerate;
 			}
 		}
+		if let Some(feerate) = next_feerate_per_kw {
+			feerate_per_kw = feerate;
+		}
 
 		log_trace!(logger, "Building commitment transaction number {} (really {} xor {}) for channel {} for {}, generated by {} with fee {}...",
 			commitment_number, (INITIAL_COMMITMENT_NUMBER - commitment_number),
@@ -1183,7 +1186,7 @@ impl<Signer: Sign> Channel<Signer> {
 			broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat as u64);
 		}
 
-		let total_fee_sat = Channel::<Signer>::commit_tx_fee_sat(feerate_per_kw, included_non_dust_htlcs.len());
+		let total_fee_sat = Channel::<Signer>::commit_tx_fee_sat(feerate_per_kw, included_non_dust_htlcs.len() + htlc_buffer);
 		let (value_to_self, value_to_remote) = if self.is_outbound() {
 			(value_to_self_msat / 1000 - total_fee_sat as i64, value_to_remote_msat / 1000)
 		} else {
@@ -1231,7 +1234,7 @@ impl<Signer: Sign> Channel<Signer> {
 		htlcs_included.sort_unstable_by_key(|h| h.0.transaction_output_index.unwrap());
 		htlcs_included.append(&mut included_dust_htlcs);
 
-		(tx, feerate_per_kw, num_nondust_htlcs, htlcs_included)
+		(tx, feerate_per_kw, num_nondust_htlcs, htlcs_included, if local { value_to_self } else { value_to_remote }, if local { value_to_remote } else { value_to_self })
 	}
 
 	#[inline]
@@ -1685,7 +1688,7 @@ impl<Signer: Sign> Channel<Signer> {
 		let funding_script = self.get_funding_redeemscript();
 
 		let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number)?;
-		let initial_commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, false, logger).0;
+		let initial_commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, false, 0, None, logger).0;
 		{
 			let trusted_tx = initial_commitment_tx.trust();
 			let initial_commitment_bitcoin_tx = trusted_tx.built_transaction();
@@ -1699,7 +1702,7 @@ impl<Signer: Sign> Channel<Signer> {
 		}
 
 		let counterparty_keys = self.build_remote_transaction_keys()?;
-		let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).0;
+		let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, 0, None, logger).0;
 
 		let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust();
 		let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction();
@@ -1810,15 +1813,15 @@ impl<Signer: Sign> Channel<Signer> {
 		let funding_script = self.get_funding_redeemscript();
 
 		let counterparty_keys = self.build_remote_transaction_keys()?;
-		let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).0;
+		let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, 0, None, logger).0;
 		let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust();
 		let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction();
 
 		log_trace!(logger, "Initial counterparty tx for channel {} is: txid {} tx {}",
 			log_bytes!(self.channel_id()), counterparty_initial_bitcoin_tx.txid, encode::serialize_hex(&counterparty_initial_bitcoin_tx.transaction));
 
 		let holder_signer = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number)?;
-		let initial_commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &holder_signer, true, false, logger).0;
+		let initial_commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &holder_signer, true, false, 0, None, logger).0;
 		{
 			let trusted_tx = initial_commitment_tx.trust();
 			let initial_commitment_bitcoin_tx = trusted_tx.built_transaction();
@@ -2407,8 +2410,8 @@ impl<Signer: Sign> Channel<Signer> {
 
 		let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number).map_err(|e| (None, e))?;
 
-		let (num_htlcs, mut htlcs_cloned, commitment_tx, commitment_txid, feerate_per_kw) = {
-			let commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, false, logger);
+		let (num_htlcs, mut htlcs_cloned, commitment_tx, commitment_txid, feerate_per_kw, _, counterparty_balance) = {
+			let commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, false, 0, None, logger);
 			let commitment_txid = {
 				let trusted_tx = commitment_tx.0.trust();
 				let bitcoin_tx = trusted_tx.built_transaction();
@@ -2424,7 +2427,7 @@ impl<Signer: Sign> Channel<Signer> {
 				bitcoin_tx.txid
 			};
 			let htlcs_cloned: Vec<_> = commitment_tx.3.iter().map(|htlc| (htlc.0.clone(), htlc.1.map(|h| h.clone()))).collect();
-			(commitment_tx.2, htlcs_cloned, commitment_tx.0, commitment_txid, commitment_tx.1)
+			(commitment_tx.2, htlcs_cloned, commitment_tx.0, commitment_txid, commitment_tx.1, commitment_tx.4, commitment_tx.5)
 		};
 
 		// If our counterparty updated the channel fee in this commitment transaction, check that
@@ -2436,7 +2439,7 @@ impl<Signer: Sign> Channel<Signer> {
 		let total_fee_sat = Channel::<Signer>::commit_tx_fee_sat(feerate_per_kw, num_htlcs);
 		if update_fee {
 			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_sat + counterparty_reserve_we_require {
+			if (counterparty_balance as u64) < counterparty_reserve_we_require {
 				return Err((None, ChannelError::Close("Funding remote cannot afford proposed new fee".to_owned())));
 			}
 		}
@@ -2621,7 +2624,7 @@ impl<Signer: Sign> Channel<Signer> {
 				// to rebalance channels.
 				match &htlc_update {
 					&HTLCUpdateAwaitingACK::AddHTLC {amount_msat, cltv_expiry, ref payment_hash, ref source, ref onion_routing_packet, ..} => {
-						match self.send_htlc(amount_msat, *payment_hash, cltv_expiry, source.clone(), onion_routing_packet.clone()) {
+						match self.send_htlc(amount_msat, *payment_hash, cltv_expiry, source.clone(), onion_routing_packet.clone(), logger) {
 							Ok(update_add_msg_option) => update_add_htlcs.push(update_add_msg_option.unwrap()),
 							Err(e) => {
 								match e {
@@ -2983,9 +2986,10 @@ impl<Signer: Sign> Channel<Signer> {
 		// 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_sat = 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_sat).map(|a| a.checked_sub(outbound_stats.pending_htlcs_value_msat / 1000)).unwrap_or(None).unwrap_or(u64::min_value());
-		if holder_balance_after_fee_sub < self.counterparty_selected_channel_reserve_satoshis.unwrap() {
+		let keys = if let Ok(keys) = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number) { keys } else { return None; };
+		let commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, true, 2, Some(feerate_per_kw), logger);
+		println!("holder balance {} vs counterparty reserve {}", commitment_tx.4 as u64, self.counterparty_selected_channel_reserve_satoshis.unwrap());
+		if (commitment_tx.4 as u64) < 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;
@@ -4309,7 +4313,7 @@ impl<Signer: Sign> Channel<Signer> {
 	/// If an Err is returned, it is a ChannelError::Close (for get_outbound_funding_created)
 	fn get_outbound_funding_created_signature<L: Deref>(&mut self, logger: &L) -> Result<Signature, ChannelError> where L::Target: Logger {
 		let counterparty_keys = self.build_remote_transaction_keys()?;
-		let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).0;
+		let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, 0, None, logger).0;
 		Ok(self.holder_signer.sign_counterparty_commitment(&counterparty_initial_commitment_tx, &self.secp_ctx)
 				.map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed".to_owned()))?.0)
 	}
@@ -4528,7 +4532,7 @@ impl<Signer: Sign> Channel<Signer> {
 	/// You MUST call send_commitment prior to calling any other methods on this Channel!
 	///
 	/// If an Err is returned, it's a ChannelError::Ignore!
-	pub fn send_htlc(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket) -> Result<Option<msgs::UpdateAddHTLC>, ChannelError> {
+	pub fn send_htlc<L: Deref>(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket, logger: &L) -> Result<Option<msgs::UpdateAddHTLC>, ChannelError> where L::Target: Logger {
 		if (self.channel_state & (ChannelState::ChannelFunded as u32 | BOTH_SIDES_SHUTDOWN_MASK)) != (ChannelState::ChannelFunded as u32) {
 			return Err(ChannelError::Ignore("Cannot send HTLC until channel is fully established and we haven't started shutting down".to_owned()));
 		}
@@ -4567,11 +4571,11 @@ impl<Signer: Sign> Channel<Signer> {
 
 		if !self.is_outbound() {
 			// Check that we won't violate the remote channel reserve by adding this HTLC.
-			let counterparty_balance_msat = self.channel_value_satoshis * 1000 - self.value_to_self_msat;
-			let holder_selected_chan_reserve_msat = Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis) * 1000;
-			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);
-			if counterparty_balance_msat < holder_selected_chan_reserve_msat + counterparty_commit_tx_fee_msat {
+			let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number)?;
+			let htlc_buffer = if amount_msat / 1000 > (self.feerate_per_kw as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis { 1 } else { 0 };
+			let commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, true, htlc_buffer, None, logger);
+			let holder_selected_chan_reserve = Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis);
+			if (commitment_tx.5 as u64) < holder_selected_chan_reserve {
 				return Err(ChannelError::Ignore("Cannot send value that would put counterparty balance under holder-announced channel reserve value".to_owned()));
 			}
 		}
@@ -4745,7 +4749,7 @@ impl<Signer: Sign> Channel<Signer> {
 	/// when we shouldn't change HTLC/channel state.
 	fn send_commitment_no_state_update<L: Deref>(&self, logger: &L) -> Result<(msgs::CommitmentSigned, (Txid, Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>)), ChannelError> where L::Target: Logger {
 		let counterparty_keys = self.build_remote_transaction_keys()?;
-		let counterparty_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, true, logger);
+		let counterparty_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, true, 0, None, logger);
 		let feerate_per_kw = counterparty_commitment_tx.1;
 		let counterparty_commitment_txid = counterparty_commitment_tx.0.trust().txid();
 		let (signature, htlc_signatures);
@@ -4771,10 +4775,7 @@ impl<Signer: Sign> Channel<Signer> {
 			// 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_sat = 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(Some(pending_feerate.0));
-				let holder_balance_after_fee_sub_sats = (self.value_to_self_msat / 1000).checked_sub(next_total_fee_sat).map(|a| a.checked_sub(outbound_stats.pending_htlcs_value_msat / 1000)).unwrap_or(None).unwrap_or(u64::min_value());
-				if holder_balance_after_fee_sub_sats < self.counterparty_selected_channel_reserve_satoshis.unwrap() {
+				if (counterparty_commitment_tx.5 as u64) < self.counterparty_selected_channel_reserve_satoshis.unwrap() {
 					log_debug!(logger, "Outbound update_fee HTLC buffer overflow - counterparty should force-close this channel");
 				}
 			}
@@ -4817,7 +4818,7 @@ impl<Signer: Sign> Channel<Signer> {
 	/// Shorthand for calling send_htlc() followed by send_commitment(), see docs on those for
 	/// more info.
 	pub fn send_htlc_and_commit<L: Deref>(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket, logger: &L) -> Result<Option<(msgs::UpdateAddHTLC, msgs::CommitmentSigned, ChannelMonitorUpdate)>, ChannelError> where L::Target: Logger {
-		match self.send_htlc(amount_msat, payment_hash, cltv_expiry, source, onion_routing_packet)? {
+		match self.send_htlc(amount_msat, payment_hash, cltv_expiry, source, onion_routing_packet, logger)? {
 			Some(update_add_htlc) => {
 				let (commitment_signed, monitor_update) = self.send_commitment_no_status_check(logger)?;
 				Ok(Some((update_add_htlc, commitment_signed, monitor_update)))
@@ -6011,7 +6012,7 @@ mod tests {
 				$( { $htlc_idx: expr, $counterparty_htlc_sig_hex: expr, $htlc_sig_hex: expr, $htlc_tx_hex: expr } ), *
 			} ) => { {
 				let (commitment_tx, htlcs): (_, Vec<HTLCOutputInCommitment>) = {
-					let mut res = chan.build_commitment_transaction(0xffffffffffff - 42, &keys, true, false, &logger);
+					let mut res = chan.build_commitment_transaction(0xffffffffffff - 42, &keys, true, false, 0, None, &logger);
 
 					let htlcs = res.3.drain(..)
 						.filter_map(|(htlc, _)| if htlc.transaction_output_index.is_some() { Some(htlc) } else { None })

lightning/src/ln/channelmanager.rs

@@ -2509,7 +2509,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 										htlc_id: prev_htlc_id,
 										incoming_packet_shared_secret: incoming_shared_secret,
 									});
-									match chan.get_mut().send_htlc(amt_to_forward, payment_hash, outgoing_cltv_value, htlc_source.clone(), onion_packet) {
+									match chan.get_mut().send_htlc(amt_to_forward, payment_hash, outgoing_cltv_value, htlc_source.clone(), onion_packet, &self.logger) {
 										Err(e) => {
 											if let ChannelError::Ignore(msg) = e {
 												log_trace!(self.logger, "Failed to forward HTLC with payment_hash {}: {}", log_bytes!(payment_hash.0), msg);