Merge pull request #1162 from TheBlueMatt/2021-11-fix-accept-chan-checks

Correct initial commitment tx fee affordability checks on open

Author: TheBlueMatt

fuzz/src/full_stack.rs

@@ -406,7 +406,12 @@ pub(crate) const FUNDING_CONF_DEADLINE_BLOCKS: u32 = 2016;
 /// 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.
-const CONCURRENT_INBOUND_HTLC_FEE_BUFFER: u32 = 2;
+pub(crate) const CONCURRENT_INBOUND_HTLC_FEE_BUFFER: u32 = 2;
+
+/// When a channel is opened, we check that the funding amount is enough to pay for relevant
+/// commitment transaction fees, with at least this many HTLCs present on the commitment
+/// transaction (not counting the value of the HTLCs themselves).
+pub(crate) const MIN_AFFORDABLE_HTLC_COUNT: usize = 4;
 
 // TODO: We should refactor this to be an Inbound/OutboundChannel until initial setup handshaking
 // has been completed, and then turn into a Channel to get compiler-time enforcement of things like
@@ -712,6 +717,12 @@ impl<Signer: Sign> Channel<Signer> {
 
 		let feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal);
 
+		let value_to_self_msat = channel_value_satoshis * 1000 - push_msat;
+		let commitment_tx_fee = Self::commit_tx_fee_msat(feerate, MIN_AFFORDABLE_HTLC_COUNT);
+		if value_to_self_msat < commitment_tx_fee {
+			return Err(APIError::APIMisuseError{ err: format!("Funding amount ({}) can't even pay fee for initial commitment transaction fee of {}.", value_to_self_msat / 1000, commitment_tx_fee / 1000) });
+		}
+
 		let mut secp_ctx = Secp256k1::new();
 		secp_ctx.seeded_randomize(&keys_provider.get_secure_random_bytes());
 
@@ -742,7 +753,7 @@ impl<Signer: Sign> Channel<Signer> {
 
 			cur_holder_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
 			cur_counterparty_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
-			value_to_self_msat: channel_value_satoshis * 1000 - push_msat,
+			value_to_self_msat,
 
 			pending_inbound_htlcs: Vec::new(),
 			pending_outbound_htlcs: Vec::new(),
@@ -957,8 +968,6 @@ impl<Signer: Sign> Channel<Signer> {
 		// we either accept their preference or the preferences match
 		local_config.announced_channel = announce;
 
-		let background_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
-
 		let holder_selected_channel_reserve_satoshis = Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(msg.funding_satoshis);
 		if holder_selected_channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS {
 			return Err(ChannelError::Close(format!("Suitable channel reserve not found. remote_channel_reserve was ({}). dust_limit_satoshis is ({}).", holder_selected_channel_reserve_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS)));
@@ -971,17 +980,18 @@ impl<Signer: Sign> Channel<Signer> {
 		}
 
 		// check if the funder's amount for the initial commitment tx is sufficient
-		// for full fee payment
+		// for full fee payment plus a few HTLCs to ensure the channel will be useful.
 		let funders_amount_msat = msg.funding_satoshis * 1000 - msg.push_msat;
-		let lower_limit = background_feerate as u64 * COMMITMENT_TX_BASE_WEIGHT;
-		if funders_amount_msat < lower_limit {
-			return Err(ChannelError::Close(format!("Insufficient funding amount ({}) for initial commitment. Must be at least {}", funders_amount_msat, lower_limit)));
+		let commitment_tx_fee = Self::commit_tx_fee_msat(msg.feerate_per_kw, MIN_AFFORDABLE_HTLC_COUNT) / 1000;
+		if funders_amount_msat / 1000 < commitment_tx_fee {
+			return Err(ChannelError::Close(format!("Funding amount ({} sats) can't even pay fee for initial commitment transaction fee of {} sats.", funders_amount_msat / 1000, commitment_tx_fee)));
 		}
 
-		let to_local_msat = msg.push_msat;
-		let to_remote_msat = funders_amount_msat - background_feerate as u64 * COMMITMENT_TX_BASE_WEIGHT;
-		if to_local_msat <= msg.channel_reserve_satoshis * 1000 && to_remote_msat <= holder_selected_channel_reserve_satoshis * 1000 {
-			return Err(ChannelError::Close("Insufficient funding amount for initial commitment".to_owned()));
+		let to_remote_satoshis = funders_amount_msat / 1000 - commitment_tx_fee;
+		// While it's reasonable for us to not meet the channel reserve initially (if they don't
+		// want to push much to us), our counterparty should always have more than our reserve.
+		if to_remote_satoshis < holder_selected_channel_reserve_satoshis {
+			return Err(ChannelError::Close("Insufficient funding amount for initial reserve".to_owned()));
 		}
 
 		let counterparty_shutdown_scriptpubkey = if their_features.supports_upfront_shutdown_script() {
@@ -2115,10 +2125,10 @@ impl<Signer: Sign> Channel<Signer> {
 
 	// Get the fee cost in MSATS of a commitment tx with a given number of HTLC outputs.
 	// Note that num_htlcs should not include dust HTLCs.
-	fn commit_tx_fee_msat(&self, num_htlcs: usize) -> u64 {
+	fn commit_tx_fee_msat(feerate_per_kw: u32, num_htlcs: usize) -> u64 {
 		// Note that we need to divide before multiplying to round properly,
 		// since the lowest denomination of bitcoin on-chain is the satoshi.
-		(COMMITMENT_TX_BASE_WEIGHT + num_htlcs as u64 * COMMITMENT_TX_WEIGHT_PER_HTLC) * self.feerate_per_kw as u64 / 1000 * 1000
+		(COMMITMENT_TX_BASE_WEIGHT + num_htlcs as u64 * COMMITMENT_TX_WEIGHT_PER_HTLC) * feerate_per_kw as u64 / 1000 * 1000
 	}
 
 	// Get the fee cost in SATS of a commitment tx with a given number of HTLC outputs.
@@ -2192,12 +2202,12 @@ impl<Signer: Sign> Channel<Signer> {
 		}
 
 		let num_htlcs = included_htlcs + addl_htlcs;
-		let res = self.commit_tx_fee_msat(num_htlcs);
+		let res = Self::commit_tx_fee_msat(self.feerate_per_kw, num_htlcs);
 		#[cfg(any(test, feature = "fuzztarget"))]
 		{
 			let mut fee = res;
 			if fee_spike_buffer_htlc.is_some() {
-				fee = self.commit_tx_fee_msat(num_htlcs - 1);
+				fee = Self::commit_tx_fee_msat(self.feerate_per_kw, num_htlcs - 1);
 			}
 			let total_pending_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len()
 				+ self.holding_cell_htlc_updates.len();
@@ -2270,12 +2280,12 @@ impl<Signer: Sign> Channel<Signer> {
 		}
 
 		let num_htlcs = included_htlcs + addl_htlcs;
-		let res = self.commit_tx_fee_msat(num_htlcs);
+		let res = Self::commit_tx_fee_msat(self.feerate_per_kw, num_htlcs);
 		#[cfg(any(test, feature = "fuzztarget"))]
 		{
 			let mut fee = res;
 			if fee_spike_buffer_htlc.is_some() {
-				fee = self.commit_tx_fee_msat(num_htlcs - 1);
+				fee = Self::commit_tx_fee_msat(self.feerate_per_kw, num_htlcs - 1);
 			}
 			let total_pending_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len();
 			let commitment_tx_info = CommitmentTxInfoCached {
@@ -4902,7 +4912,7 @@ impl<Signer: Sign> Channel<Signer> {
 						&& info.next_holder_htlc_id == self.next_holder_htlc_id
 						&& info.next_counterparty_htlc_id == self.next_counterparty_htlc_id
 						&& info.feerate == self.feerate_per_kw {
-							let actual_fee = self.commit_tx_fee_msat(commitment_stats.num_nondust_htlcs);
+							let actual_fee = Self::commit_tx_fee_msat(self.feerate_per_kw, commitment_stats.num_nondust_htlcs);
 							assert_eq!(actual_fee, info.fee);
 						}
 				}
@@ -5932,13 +5942,13 @@ mod tests {
 		// the dust limit check.
 		let htlc_candidate = HTLCCandidate::new(htlc_amount_msat, HTLCInitiator::LocalOffered);
 		let local_commit_tx_fee = node_a_chan.next_local_commit_tx_fee_msat(htlc_candidate, None);
-		let local_commit_fee_0_htlcs = node_a_chan.commit_tx_fee_msat(0);
+		let local_commit_fee_0_htlcs = Channel::<EnforcingSigner>::commit_tx_fee_msat(node_a_chan.feerate_per_kw, 0);
 		assert_eq!(local_commit_tx_fee, local_commit_fee_0_htlcs);
 
 		// Finally, make sure that when Node A calculates the remote's commitment transaction fees, all
 		// of the HTLCs are seen to be above the dust limit.
 		node_a_chan.channel_transaction_parameters.is_outbound_from_holder = false;
-		let remote_commit_fee_3_htlcs = node_a_chan.commit_tx_fee_msat(3);
+		let remote_commit_fee_3_htlcs = Channel::<EnforcingSigner>::commit_tx_fee_msat(node_a_chan.feerate_per_kw, 3);
 		let htlc_candidate = HTLCCandidate::new(htlc_amount_msat, HTLCInitiator::LocalOffered);
 		let remote_commit_tx_fee = node_a_chan.next_remote_commit_tx_fee_msat(htlc_candidate, None);
 		assert_eq!(remote_commit_tx_fee, remote_commit_fee_3_htlcs);
@@ -5960,8 +5970,8 @@ mod tests {
 		let config = UserConfig::default();
 		let mut chan = Channel::<EnforcingSigner>::new_outbound(&&fee_est, &&keys_provider, node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0).unwrap();
 
-		let commitment_tx_fee_0_htlcs = chan.commit_tx_fee_msat(0);
-		let commitment_tx_fee_1_htlc = chan.commit_tx_fee_msat(1);
+		let commitment_tx_fee_0_htlcs = Channel::<EnforcingSigner>::commit_tx_fee_msat(chan.feerate_per_kw, 0);
+		let commitment_tx_fee_1_htlc = Channel::<EnforcingSigner>::commit_tx_fee_msat(chan.feerate_per_kw, 1);
 
 		// If HTLC_SUCCESS_TX_WEIGHT and HTLC_TIMEOUT_TX_WEIGHT were swapped: then this HTLC would be
 		// counted as dust when it shouldn't be.