Error if the calculated v1 reserve is greater than the channel value

We made the same change to the calculation of the v2 reserve in the
previous commit.

Author: tankyleo

lightning/src/ln/channel.rs

@@ -6761,20 +6761,32 @@ fn get_legacy_default_holder_max_htlc_value_in_flight_msat(channel_value_satoshi
 /// This is used both for outbound and inbound channels and has lower bound
 /// of `MIN_THEIR_CHAN_RESERVE_SATOSHIS`, and the `dust_limit_satoshis` of
 /// the counterparty.
+///
+/// Returns `Err` if `channel_value_satoshis` is smaller than
+/// `MIN_THEIR_CHAN_RESERVE_SATOSHIS` or the `dust_limit_satoshis` of the
+/// counterparty.
 pub(crate) fn get_holder_selected_channel_reserve_satoshis(
 	channel_value_satoshis: u64, their_dust_limit_satoshis: u64, config: &UserConfig,
 	is_0reserve: bool,
-) -> u64 {
+) -> Result<u64, ()> {
+	if channel_value_satoshis < MIN_THEIR_CHAN_RESERVE_SATOSHIS
+		|| channel_value_satoshis < their_dust_limit_satoshis
+	{
+		return Err(());
+	}
 	if is_0reserve {
-		return 0;
+		return Ok(0);
 	}
-	let counterparty_chan_reserve_prop_mil =
-		config.channel_handshake_config.their_channel_reserve_proportional_millionths as u64;
+	// As described in the `ChannelHandshakeConfig` docs, we cap this value at 1_000_000.
+	let counterparty_chan_reserve_prop_mil = cmp::min(
+		config.channel_handshake_config.their_channel_reserve_proportional_millionths as u64,
+		1_000_000,
+	);
 	let calculated_reserve =
 		channel_value_satoshis.saturating_mul(counterparty_chan_reserve_prop_mil) / 1_000_000;
 	let channel_reserve_satoshis = cmp::max(calculated_reserve, MIN_THEIR_CHAN_RESERVE_SATOSHIS);
 	let channel_reserve_satoshis = cmp::max(channel_reserve_satoshis, their_dust_limit_satoshis);
-	cmp::min(channel_value_satoshis, channel_reserve_satoshis)
+	Ok(channel_reserve_satoshis)
 }
 
 /// This is for legacy reasons, present for forward-compatibility.
@@ -14479,12 +14491,19 @@ impl<SP: SignerProvider> OutboundV1Channel<SP> {
 		// a dust limit higher than our selected reserve.
 		let their_dust_limit_satoshis = 0;
 		let is_0reserve = trusted_channel_features.is_some_and(|f| f.is_0reserve());
-		let holder_selected_channel_reserve_satoshis = get_holder_selected_channel_reserve_satoshis(
-			channel_value_satoshis,
-			their_dust_limit_satoshis,
-			config,
-			is_0reserve,
-		);
+		let holder_selected_channel_reserve_satoshis =
+			get_holder_selected_channel_reserve_satoshis(
+				channel_value_satoshis,
+				their_dust_limit_satoshis,
+				config,
+				is_0reserve,
+			)
+			.map_err(|()| APIError::APIMisuseError {
+				err: format!(
+					"The channel value {channel_value_satoshis} is smaller than \
+					{MIN_THEIR_CHAN_RESERVE_SATOSHIS}"
+				),
+			})?;
 		if holder_selected_channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS && !is_0reserve {
 			// Protocol level safety check in place, although it should never happen because
 			// of `MIN_THEIR_CHAN_RESERVE_SATOSHIS` and `MIN_CHANNEL_VALUE_SATOSHIS`
@@ -14876,12 +14895,20 @@ impl<SP: SignerProvider> InboundV1Channel<SP> {
 		let channel_type =
 			channel_type_from_open_channel(&msg.common_fields, our_supported_features)?;
 
-		let holder_selected_channel_reserve_satoshis = get_holder_selected_channel_reserve_satoshis(
-			msg.common_fields.funding_satoshis,
-			msg.common_fields.dust_limit_satoshis,
-			config,
-			trusted_channel_features.is_some_and(|f| f.is_0reserve()),
-		);
+		let holder_selected_channel_reserve_satoshis =
+			get_holder_selected_channel_reserve_satoshis(
+				msg.common_fields.funding_satoshis,
+				msg.common_fields.dust_limit_satoshis,
+				config,
+				trusted_channel_features.is_some_and(|f| f.is_0reserve()),
+			)
+			.map_err(|()| {
+				ChannelError::close(format!(
+					"The channel value {} is smaller than either their dust \
+					limit {}, or {MIN_THEIR_CHAN_RESERVE_SATOSHIS}",
+					msg.common_fields.funding_satoshis, msg.common_fields.dust_limit_satoshis,
+				))
+			})?;
 		let counterparty_pubkeys = ChannelPublicKeys {
 			funding_pubkey: msg.common_fields.funding_pubkey,
 			revocation_basepoint: RevocationBasepoint::from(msg.common_fields.revocation_basepoint),
@@ -17483,6 +17510,10 @@ mod tests {
 		// to channel value
 		test_self_and_counterparty_channel_reserve(10_000_000, 0.50, 0.50);
 		test_self_and_counterparty_channel_reserve(10_000_000, 0.60, 0.50);
+
+		// Make sure we correctly handle reserves greater than the channel value
+		test_self_and_counterparty_channel_reserve(100_000, 1.1, 0.30);
+		test_self_and_counterparty_channel_reserve(100_000, 0.30, 1.1);
 	}
 
 	#[rustfmt::skip]
@@ -17502,7 +17533,19 @@ mod tests {
 		outbound_node_config.channel_handshake_config.their_channel_reserve_proportional_millionths = (outbound_selected_channel_reserve_perc * 1_000_000.0) as u32;
 		let mut chan = OutboundV1Channel::<&TestKeysInterface>::new(&&fee_est, &&keys_provider, &&keys_provider, outbound_node_id, &channelmanager::provided_init_features(&outbound_node_config), channel_value_satoshis, 100_000, 42, &outbound_node_config, 0, 42, None, &logger, None).unwrap();
 
-		let expected_outbound_selected_chan_reserve = cmp::max(MIN_THEIR_CHAN_RESERVE_SATOSHIS, (chan.funding.get_value_satoshis() as f64 * outbound_selected_channel_reserve_perc) as u64);
+		let outbound_capped_reserve_perc = if outbound_selected_channel_reserve_perc.lt(&1.0) {
+			outbound_selected_channel_reserve_perc
+		} else {
+			1.0
+		};
+
+		let inbound_capped_reserve_perc = if inbound_selected_channel_reserve_perc.lt(&1.0) {
+			inbound_selected_channel_reserve_perc
+		} else {
+			1.0
+		};
+
+		let expected_outbound_selected_chan_reserve = cmp::max(MIN_THEIR_CHAN_RESERVE_SATOSHIS, (chan.funding.get_value_satoshis() as f64 * outbound_capped_reserve_perc) as u64);
 		assert_eq!(chan.funding.holder_selected_channel_reserve_satoshis, expected_outbound_selected_chan_reserve);
 
 		let chan_open_channel_msg = chan.get_open_channel(ChainHash::using_genesis_block(network), &&logger).unwrap();
@@ -17512,7 +17555,7 @@ mod tests {
 		if outbound_selected_channel_reserve_perc + inbound_selected_channel_reserve_perc < 1.0 {
 			let chan_inbound_node = InboundV1Channel::<&TestKeysInterface>::new(&&fee_est, &&keys_provider, &&keys_provider, inbound_node_id, &channelmanager::provided_channel_type_features(&inbound_node_config), &channelmanager::provided_init_features(&outbound_node_config), &chan_open_channel_msg, 7, &inbound_node_config, 0, &&logger, None).unwrap();
 
-			let expected_inbound_selected_chan_reserve = cmp::max(MIN_THEIR_CHAN_RESERVE_SATOSHIS, (chan.funding.get_value_satoshis() as f64 * inbound_selected_channel_reserve_perc) as u64);
+			let expected_inbound_selected_chan_reserve = cmp::max(MIN_THEIR_CHAN_RESERVE_SATOSHIS, (chan.funding.get_value_satoshis() as f64 * inbound_capped_reserve_perc) as u64);
 
 			assert_eq!(chan_inbound_node.funding.holder_selected_channel_reserve_satoshis, expected_inbound_selected_chan_reserve);
 			assert_eq!(chan_inbound_node.funding.counterparty_selected_channel_reserve_satoshis.unwrap(), expected_outbound_selected_chan_reserve);

lightning/src/ln/channel_open_tests.rs

@@ -16,7 +16,8 @@ use crate::chain::{self, ChannelMonitorUpdateStatus};
 use crate::events::{ClosureReason, Event, FundingInfo};
 use crate::ln::channel::{
 	get_holder_selected_channel_reserve_satoshis, ChannelError, InboundV1Channel,
-	OutboundV1Channel, COINBASE_MATURITY, UNFUNDED_CHANNEL_AGE_LIMIT_TICKS,
+	OutboundV1Channel, COINBASE_MATURITY, MIN_THEIR_CHAN_RESERVE_SATOSHIS,
+	UNFUNDED_CHANNEL_AGE_LIMIT_TICKS,
 };
 use crate::ln::channelmanager::{
 	self, TrustedChannelFeatures, BREAKDOWN_TIMEOUT, MAX_UNFUNDED_CHANNEL_PEERS,
@@ -473,7 +474,8 @@ pub fn test_insane_channel_opens() {
 	// funding satoshis
 	let channel_value_sat = 31337; // same as funding satoshis
 	let channel_reserve_satoshis =
-		get_holder_selected_channel_reserve_satoshis(channel_value_sat, 0, &legacy_cfg, false);
+		get_holder_selected_channel_reserve_satoshis(channel_value_sat, 0, &legacy_cfg, false)
+			.unwrap();
 	let push_msat = (channel_value_sat - channel_reserve_satoshis) * 1000;
 
 	// Have node0 initiate a channel to node1 with aforementioned parameters
@@ -552,7 +554,13 @@ pub fn test_insane_channel_opens() {
 		},
 	);
 
-	insane_open_helper("Peer never wants payout outputs?", |mut msg| {
+	let crazy_dust_limit = channel_value_sat + 1;
+	let expected_error_str = format!(
+		"Got non-closing error: The channel value \
+		{channel_value_sat} is smaller than either their dust limit {crazy_dust_limit}, or \
+		{MIN_THEIR_CHAN_RESERVE_SATOSHIS}"
+	);
+	insane_open_helper(&expected_error_str, |mut msg| {
 		msg.common_fields.dust_limit_satoshis = msg.common_fields.funding_satoshis + 1;
 		msg
 	});

lightning/src/ln/functional_tests.rs

@@ -415,7 +415,8 @@ pub fn test_inbound_outbound_capacity_is_not_zero() {
 	assert_eq!(channels0.len(), 1);
 	assert_eq!(channels1.len(), 1);
 
-	let reserve = get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false);
+	let reserve =
+		get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false).unwrap();
 	assert_eq!(channels0[0].inbound_capacity_msat, 95000000 - reserve * 1000);
 	assert_eq!(channels1[0].outbound_capacity_msat, 95000000 - reserve * 1000);
 

lightning/src/ln/htlc_reserve_unit_tests.rs

@@ -55,8 +55,9 @@ fn do_test_counterparty_no_reserve(send_from_initiator: bool) {
 	push_amt -= feerate_per_kw as u64
 		* (commitment_tx_base_weight(&channel_type_features) + 4 * COMMITMENT_TX_WEIGHT_PER_HTLC)
 		/ 1000 * 1000;
-	push_amt -=
-		get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false) * 1000;
+	push_amt -= get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false)
+		.unwrap()
+		* 1000;
 
 	let push = if send_from_initiator { 0 } else { push_amt };
 	let temp_channel_id =
@@ -1002,8 +1003,9 @@ pub fn test_chan_reserve_violation_outbound_htlc_inbound_chan() {
 		&channel_type_features,
 	);
 
-	push_amt -=
-		get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false) * 1000;
+	push_amt -= get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false)
+		.unwrap()
+		* 1000;
 
 	let _ = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, push_amt);
 
@@ -1048,8 +1050,9 @@ pub fn test_chan_reserve_dust_inbound_htlcs_outbound_chan() {
 		MIN_AFFORDABLE_HTLC_COUNT as u64,
 		&channel_type_features,
 	);
-	push_amt -=
-		get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false) * 1000;
+	push_amt -= get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false)
+		.unwrap()
+		* 1000;
 	create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, push_amt);
 
 	let (htlc_success_tx_fee_sat, _) =

lightning/src/ln/payment_tests.rs

@@ -5043,7 +5043,7 @@ fn test_htlc_forward_considers_anchor_outputs_value() {
 		create_announced_chan_between_nodes_with_value(&nodes, 1, 2, CHAN_AMT, PUSH_MSAT);
 
 	let channel_reserve_msat =
-		get_holder_selected_channel_reserve_satoshis(CHAN_AMT, 0, &config, false) * 1000;
+		get_holder_selected_channel_reserve_satoshis(CHAN_AMT, 0, &config, false).unwrap() * 1000;
 	let commitment_fee_msat = chan_utils::commit_tx_fee_sat(
 		*nodes[1].fee_estimator.sat_per_kw.lock().unwrap(),
 		2,

lightning/src/ln/update_fee_tests.rs

@@ -410,7 +410,7 @@ pub fn do_test_update_fee_that_funder_cannot_afford(channel_type_features: Chann
 	let channel_id = chan.2;
 	let secp_ctx = Secp256k1::new();
 	let bs_channel_reserve_sats =
-		get_holder_selected_channel_reserve_satoshis(channel_value, 0, &cfg, false);
+		get_holder_selected_channel_reserve_satoshis(channel_value, 0, &cfg, false).unwrap();
 	let (anchor_outputs_value_sats, outputs_num_no_htlcs) =
 		if channel_type_features.supports_anchors_zero_fee_htlc_tx() {
 			(ANCHOR_OUTPUT_VALUE_SATOSHI * 2, 4)
@@ -886,8 +886,9 @@ pub fn test_chan_init_feerate_unaffordability() {
 
 	// 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 -=
-		get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false) * 1000;
+	push_amt -= get_holder_selected_channel_reserve_satoshis(100_000, 0, &default_config, false)
+		.unwrap()
+		* 1000;
 	nodes[0].node.create_channel(node_b_id, 100_000, push_amt, 42, None, None).unwrap();
 	let mut open_channel_msg =
 		get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, node_b_id);