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

Correct initial commitment tx fee affordability checks on open

lightning/src/ln/channel.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;
+		let commitment_tx_fee = Self::commit_tx_fee_msat(msg.feerate_per_kw, MIN_AFFORDABLE_HTLC_COUNT) / 1000;
-		if funders_amount_msat < lower_limit {
+		if funders_amount_msat / 1000 < commitment_tx_fee {
-			return Err(ChannelError::Close(format!("Insufficient funding amount ({}) for initial commitment. Must be at least {}", funders_amount_msat, lower_limit)));
+			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_satoshis = funders_amount_msat / 1000 - commitment_tx_fee;
-		let to_remote_msat = funders_amount_msat - background_feerate as u64 * COMMITMENT_TX_BASE_WEIGHT;
+		// While it's reasonable for us to not meet the channel reserve initially (if they don't
-		if to_local_msat <= msg.channel_reserve_satoshis * 1000 && to_remote_msat <= holder_selected_channel_reserve_satoshis * 1000 {
+		// want to push much to us), our counterparty should always have more than our reserve.
-			return Err(ChannelError::Close("Insufficient funding amount for initial commitment".to_owned()));
+		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_0_htlcs = Channel::<EnforcingSigner>::commit_tx_fee_msat(chan.feerate_per_kw, 0);
-		let commitment_tx_fee_1_htlc = chan.commit_tx_fee_msat(1);
+		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.

lightning/src/ln/functional_tests.rs

@@ -18,7 +18,7 @@ use chain::channelmonitor::{ChannelMonitor, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PER
 use chain::transaction::OutPoint;
 use chain::keysinterface::BaseSign;
 use ln::{PaymentPreimage, PaymentSecret, PaymentHash};
-use ln::channel::{COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC};
+use ln::channel::{COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC, CONCURRENT_INBOUND_HTLC_FEE_BUFFER, MIN_AFFORDABLE_HTLC_COUNT};
 use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, PaymentId, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA};
 use ln::channel::{Channel, ChannelError};
 use ln::{chan_utils, onion_utils};
@@ -584,12 +584,19 @@ fn test_update_fee_that_funder_cannot_afford() {
 	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);
-	let channel_value = 1977;
+	let channel_value = 5000;
-	let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, channel_value, 700000, InitFeatures::known(), InitFeatures::known());
+	let push_sats = 700;
+	let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, channel_value, push_sats * 1000, InitFeatures::known(), InitFeatures::known());
 	let channel_id = chan.2;
 	let secp_ctx = Secp256k1::new();
+	let bs_channel_reserve_sats = Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(channel_value);
 
-	let feerate = 260;
+	// Calculate the maximum feerate that A can afford. Note that we don't send an update_fee
+	// CONCURRENT_INBOUND_HTLC_FEE_BUFFER HTLCs before actually running out of local balance, so we
+	// calculate two different feerates here - the expected local limit as well as the expected
+	// remote limit.
+	let feerate = ((channel_value - bs_channel_reserve_sats - push_sats) * 1000 / (COMMITMENT_TX_BASE_WEIGHT + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as u64 * COMMITMENT_TX_WEIGHT_PER_HTLC)) as u32;
+	let non_buffer_feerate = ((channel_value - bs_channel_reserve_sats - push_sats) * 1000 / COMMITMENT_TX_BASE_WEIGHT) as u32;
 	{
 		let mut feerate_lock = chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
 		*feerate_lock = feerate;
@@ -602,27 +609,25 @@ fn test_update_fee_that_funder_cannot_afford() {
 
 	commitment_signed_dance!(nodes[1], nodes[0], update_msg.commitment_signed, false);
 
-	//Confirm that the new fee based on the last local commitment txn is what we expected based on the feerate of 260 set above.
+	// Confirm that the new fee based on the last local commitment txn is what we expected based on the feerate set above.
-	//This value results in a fee that is exactly what the funder can afford (277 sat + 1000 sat channel reserve)
 	{
 		let commitment_tx = get_local_commitment_txn!(nodes[1], channel_id)[0].clone();
 
-		//We made sure neither party's funds are below the dust limit so -2 non-HTLC txns from number of outputs
+		//We made sure neither party's funds are below the dust limit and there are no HTLCs here
-		let num_htlcs = commitment_tx.output.len() - 2;
+		assert_eq!(commitment_tx.output.len(), 2);
-		let total_fee: u64 = feerate as u64 * (COMMITMENT_TX_BASE_WEIGHT + (num_htlcs as u64) * COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000;
+		let total_fee: u64 = commit_tx_fee_msat(feerate, 0) / 1000;
 		let mut actual_fee = commitment_tx.output.iter().fold(0, |acc, output| acc + output.value);
 		actual_fee = channel_value - actual_fee;
 		assert_eq!(total_fee, actual_fee);
 	}
 
-	//Add 2 to the previous fee rate to the final fee increases by 1 (with no HTLCs the fee is essentially
-	//fee_rate*(724/1000) so the increment of 1*0.724 is rounded back down)
 	{
+		// Increment the feerate by a small constant, accounting for rounding errors
 		let mut feerate_lock = chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
-		*feerate_lock = feerate + 2;
+		*feerate_lock += 4;
 	}
 	nodes[0].node.timer_tick_occurred();
-	nodes[0].logger.assert_log("lightning::ln::channel".to_string(), format!("Cannot afford to send new feerate at {}", feerate + 2), 1);
+	nodes[0].logger.assert_log("lightning::ln::channel".to_string(), format!("Cannot afford to send new feerate at {}", feerate + 4), 1);
 	check_added_monitors!(nodes[0], 0);
 
 	const INITIAL_COMMITMENT_NUMBER: u64 = 281474976710654;
@@ -658,11 +663,11 @@ fn test_update_fee_that_funder_cannot_afford() {
 		let mut htlcs: Vec<(HTLCOutputInCommitment, ())> = vec![];
 		let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data(
 			INITIAL_COMMITMENT_NUMBER - 1,
-			700,
+			push_sats,
-			999,
+			channel_value - push_sats - commit_tx_fee_msat(non_buffer_feerate + 4, 0) / 1000,
 			false, local_funding, remote_funding,
 			commit_tx_keys.clone(),
-			feerate + 124,
+			non_buffer_feerate + 4,
 			&mut htlcs,
 			&local_chan.channel_transaction_parameters.as_counterparty_broadcastable()
 		);
@@ -677,7 +682,7 @@ fn test_update_fee_that_funder_cannot_afford() {
 
 	let update_fee = msgs::UpdateFee {
 		channel_id: chan.2,
-		feerate_per_kw: feerate + 124,
+		feerate_per_kw: non_buffer_feerate + 4,
 	};
 
 	nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), &update_fee);
@@ -1446,21 +1451,21 @@ fn test_chan_reserve_violation_outbound_htlc_inbound_chan() {
 	// sending any above-dust amount would result in a channel reserve violation.
 	// In this test we check that we would be prevented from sending an HTLC in
 	// this situation.
-	let feerate_per_kw = 253;
+	let feerate_per_kw = *chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
-	chanmon_cfgs[0].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(feerate_per_kw) };
-	chanmon_cfgs[1].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(feerate_per_kw) };
 	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
 	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
 	let mut push_amt = 100_000_000;
-	push_amt -= feerate_per_kw as u64 * (COMMITMENT_TX_BASE_WEIGHT + COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000 * 1000;
+	push_amt -= commit_tx_fee_msat(feerate_per_kw, MIN_AFFORDABLE_HTLC_COUNT as u64);
 	push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000) * 1000;
 
 	let _ = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, push_amt, InitFeatures::known(), InitFeatures::known());
 
 	// Sending exactly enough to hit the reserve amount should be accepted
-	let (_, _, _) = route_payment(&nodes[1], &[&nodes[0]], 1_000_000);
+	for _ in 0..MIN_AFFORDABLE_HTLC_COUNT {
+		let (_, _, _) = route_payment(&nodes[1], &[&nodes[0]], 1_000_000);
+	}
 
 	// However one more HTLC should be significantly over the reserve amount and fail.
 	let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 1_000_000);
@@ -1473,30 +1478,36 @@ fn test_chan_reserve_violation_outbound_htlc_inbound_chan() {
 #[test]
 fn test_chan_reserve_violation_inbound_htlc_outbound_channel() {
 	let mut chanmon_cfgs = create_chanmon_cfgs(2);
-	// Set the fee rate for the channel very high, to the point where the funder
+	let feerate_per_kw = *chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
-	// receiving 1 update_add_htlc would result in them closing the channel due
-	// to channel reserve violation. This close could also happen if the fee went
-	// up a more realistic amount, but many HTLCs were outstanding at the time of
-	// the update_add_htlc.
-	chanmon_cfgs[0].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(6000) };
-	chanmon_cfgs[1].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(6000) };
 	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
 	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
-	let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 95000000, InitFeatures::known(), InitFeatures::known());
 
-	let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 1000);
+	// Set nodes[0]'s balance such that they will consider any above-dust received HTLC to be a
+	// channel reserve violation (so their balance is channel reserve (1000 sats) + commitment
+	// transaction fee with 0 HTLCs (183 sats)).
+	let mut push_amt = 100_000_000;
+	push_amt -= commit_tx_fee_msat(feerate_per_kw, MIN_AFFORDABLE_HTLC_COUNT as u64);
+	push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000) * 1000;
+	let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, push_amt, InitFeatures::known(), InitFeatures::known());
+
+	// Send four HTLCs to cover the initial push_msat buffer we're required to include
+	for _ in 0..MIN_AFFORDABLE_HTLC_COUNT {
+		let (_, _, _) = route_payment(&nodes[1], &[&nodes[0]], 1_000_000);
+	}
+
+	let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 700_000);
 	// 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]).unwrap();
 	let cur_height = nodes[1].node.best_block.read().unwrap().height() + 1;
 	let onion_keys = onion_utils::construct_onion_keys(&secp_ctx, &route.paths[0], &session_priv).unwrap();
-	let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(&route.paths[0], 1000, &Some(payment_secret), cur_height, &None).unwrap();
+	let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(&route.paths[0], 700_000, &Some(payment_secret), cur_height, &None).unwrap();
 	let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash);
 	let msg = msgs::UpdateAddHTLC {
 		channel_id: chan.2,
-		htlc_id: 1,
+		htlc_id: MIN_AFFORDABLE_HTLC_COUNT as u64,
-		amount_msat: htlc_msat + 1,
+		amount_msat: htlc_msat,
 		payment_hash: payment_hash,
 		cltv_expiry: htlc_cltv,
 		onion_routing_packet: onion_packet,
@@ -1517,9 +1528,7 @@ fn test_chan_reserve_dust_inbound_htlcs_outbound_chan() {
 	// Test that if we receive many dust HTLCs over an outbound channel, they don't count when
 	// calculating our commitment transaction fee (this was previously broken).
 	let mut chanmon_cfgs = create_chanmon_cfgs(2);
-	let feerate_per_kw = 253;
+	let feerate_per_kw = *chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
-	chanmon_cfgs[0].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(feerate_per_kw) };
-	chanmon_cfgs[1].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(feerate_per_kw) };
 
 	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None, None]);
@@ -1529,7 +1538,7 @@ fn test_chan_reserve_dust_inbound_htlcs_outbound_chan() {
 	// channel reserve violation (so their balance is channel reserve (1000 sats) + commitment
 	// transaction fee with 0 HTLCs (183 sats)).
 	let mut push_amt = 100_000_000;
-	push_amt -= feerate_per_kw as u64 * (COMMITMENT_TX_BASE_WEIGHT) / 1000 * 1000;
+	push_amt -= commit_tx_fee_msat(feerate_per_kw, MIN_AFFORDABLE_HTLC_COUNT as u64);
 	push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000) * 1000;
 	create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, push_amt, InitFeatures::known(), InitFeatures::known());
 
@@ -1540,12 +1549,52 @@ fn test_chan_reserve_dust_inbound_htlcs_outbound_chan() {
 	// commitment transaction fee.
 	let (_, _, _) = route_payment(&nodes[1], &[&nodes[0]], dust_amt);
 
+	// Send four HTLCs to cover the initial push_msat buffer we're required to include
+	for _ in 0..MIN_AFFORDABLE_HTLC_COUNT {
+		let (_, _, _) = route_payment(&nodes[1], &[&nodes[0]], 1_000_000);
+	}
+
 	// One more than the dust amt should fail, however.
 	let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], dust_amt + 1);
 	unwrap_send_err!(nodes[1].node.send_payment(&route, our_payment_hash, &Some(our_payment_secret)), true, APIError::ChannelUnavailable { ref err },
 		assert_eq!(err, "Cannot send value that would put counterparty balance under holder-announced channel reserve value"));
 }
 
+#[test]
+fn test_chan_init_feerate_unaffordability() {
+	// Test that we will reject channel opens which do not leave enough to pay for any HTLCs due to
+	// channel reserve and feerate requirements.
+	let mut chanmon_cfgs = create_chanmon_cfgs(2);
+	let feerate_per_kw = *chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	// Set the push_msat amount such that nodes[0] will not be able to afford to add even a single
+	// HTLC.
+	let mut push_amt = 100_000_000;
+	push_amt -= commit_tx_fee_msat(feerate_per_kw, MIN_AFFORDABLE_HTLC_COUNT as u64);
+	assert_eq!(nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100_000, push_amt + 1, 42, None).unwrap_err(),
+		APIError::APIMisuseError { err: "Funding amount (356) can't even pay fee for initial commitment transaction fee of 357.".to_string() });
+
+	// During open, we don't have a "counterparty channel reserve" to check against, so that
+	// requirement only comes into play on the open_channel handling side.
+	push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000) * 1000;
+	nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100_000, push_amt, 42, None).unwrap();
+	let mut open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
+	open_channel_msg.push_msat += 1;
+	nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &open_channel_msg);
+
+	let msg_events = nodes[1].node.get_and_clear_pending_msg_events();
+	assert_eq!(msg_events.len(), 1);
+	match msg_events[0] {
+		MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { ref msg }, node_id: _ } => {
+			assert_eq!(msg.data, "Insufficient funding amount for initial reserve");
+		},
+		_ => panic!("Unexpected event"),
+	}
+}
+
 #[test]
 fn test_chan_reserve_dust_inbound_htlcs_inbound_chan() {
 	// Test that if we receive many dust HTLCs over an inbound channel, they don't count when
@@ -4508,7 +4557,7 @@ fn test_claim_sizeable_push_msat() {
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
 	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
-	let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 99000000, InitFeatures::known(), InitFeatures::known());
+	let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 98_000_000, InitFeatures::known(), InitFeatures::known());
 	nodes[1].node.force_close_channel(&chan.2).unwrap();
 	check_closed_broadcast!(nodes[1], true);
 	check_added_monitors!(nodes[1], 1);
@@ -4537,7 +4586,7 @@ fn test_claim_on_remote_sizeable_push_msat() {
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
 	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
-	let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 99000000, InitFeatures::known(), InitFeatures::known());
+	let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 98_000_000, InitFeatures::known(), InitFeatures::known());
 	nodes[0].node.force_close_channel(&chan.2).unwrap();
 	check_closed_broadcast!(nodes[0], true);
 	check_added_monitors!(nodes[0], 1);

lightning/src/ln/script.rs

@@ -17,7 +17,7 @@ use io;
 /// A script pubkey for shutting down a channel as defined by [BOLT #2].
 ///
 /// [BOLT #2]: https://github.com/lightningnetwork/lightning-rfc/blob/master/02-peer-protocol.md
-#[derive(Clone)]
+#[derive(Clone, PartialEq)]
 pub struct ShutdownScript(ShutdownScriptImpl);
 
 /// An error occurring when converting from [`Script`] to [`ShutdownScript`].
@@ -29,7 +29,7 @@ pub struct InvalidShutdownScript {
 	pub script: Script
 }
 
-#[derive(Clone)]
+#[derive(Clone, PartialEq)]
 enum ShutdownScriptImpl {
 	/// [`PublicKey`] used to form a P2WPKH script pubkey. Used to support backward-compatible
 	/// serialization.

lightning/src/util/errors.rs

@@ -16,7 +16,7 @@ use core::fmt;
 
 /// Indicates an error on the client's part (usually some variant of attempting to use too-low or
 /// too-high values)
-#[derive(Clone)]
+#[derive(Clone, PartialEq)]
 pub enum APIError {
 	/// Indicates the API was wholly misused (see err for more). Cases where these can be returned
 	/// are documented, but generally indicates some precondition of a function was violated.