Explicitly support counterparty setting 0 channel reserve

A peer providing a channel_reserve_satoshis of 0 (or less than our
dust limit) is insecure, but only for them. Because some LSPs do it
with some level of trust of the clients (for a substantial UX
improvement), we explicitly allow it. Because its unlikely to
happen often in normal testing, we test it explicitly here.

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -373,9 +373,9 @@ pub(super) struct MonitorRestoreUpdates {
 /// the channel. Sadly, there isn't really a good number for this - if we expect to have no new
 /// HTLCs for days we may need this to suffice for feerate increases across days, but that may
 /// leave the channel less usable as we hold a bigger reserve.
-#[cfg(fuzzing)]
+#[cfg(any(fuzzing, test))]
 pub const FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE: u64 = 2;
-#[cfg(not(fuzzing))]
+#[cfg(not(any(fuzzing, test)))]
 const FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE: u64 = 2;
 
 // TODO: We should refactor this to be an Inbound/OutboundChannel until initial setup handshaking
@@ -477,19 +477,30 @@ pub(super) struct Channel<Signer: Sign> {
 	short_channel_id: Option<u64>,
 
 	counterparty_dust_limit_satoshis: u64,
+
 	#[cfg(test)]
 	pub(super) holder_dust_limit_satoshis: u64,
 	#[cfg(not(test))]
 	holder_dust_limit_satoshis: u64,
+
 	#[cfg(test)]
 	pub(super) counterparty_max_htlc_value_in_flight_msat: u64,
 	#[cfg(not(test))]
 	counterparty_max_htlc_value_in_flight_msat: u64,
+
+	#[cfg(test)]
+	pub(super) holder_max_htlc_value_in_flight_msat: u64,
+	#[cfg(not(test))]
 	holder_max_htlc_value_in_flight_msat: u64,
 
 	/// minimum channel reserve for self to maintain - set by them.
 	counterparty_selected_channel_reserve_satoshis: Option<u64>,
+
+	#[cfg(test)]
+	pub(super) holder_selected_channel_reserve_satoshis: u64,
+	#[cfg(not(test))]
 	holder_selected_channel_reserve_satoshis: u64,
+
 	counterparty_htlc_minimum_msat: u64,
 	holder_htlc_minimum_msat: u64,
 	#[cfg(test)]
@@ -870,9 +881,6 @@ impl<Signer: Sign> Channel<Signer> {
 		if msg.dust_limit_satoshis > msg.funding_satoshis {
 			return Err(ChannelError::Close(format!("dust_limit_satoshis {} was larger than funding_satoshis {}. Peer never wants payout outputs?", msg.dust_limit_satoshis, msg.funding_satoshis)));
 		}
-		if msg.dust_limit_satoshis > msg.channel_reserve_satoshis {
-			return Err(ChannelError::Close(format!("Bogus; channel reserve ({}) is less than dust limit ({})", msg.channel_reserve_satoshis, msg.dust_limit_satoshis)));
-		}
 		let full_channel_value_msat = (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000;
 		if msg.htlc_minimum_msat >= full_channel_value_msat {
 			return Err(ChannelError::Close(format!("Minimum htlc value ({}) was larger than full channel value ({})", msg.htlc_minimum_msat, full_channel_value_msat)));
@@ -928,9 +936,6 @@ impl<Signer: Sign> Channel<Signer> {
 		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)));
 		}
-		if msg.channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS {
-			return Err(ChannelError::Close(format!("channel_reserve_satoshis ({}) is smaller than our dust limit ({})", msg.channel_reserve_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS)));
-		}
 		if holder_selected_channel_reserve_satoshis < msg.dust_limit_satoshis {
 			return Err(ChannelError::Close(format!("Dust limit ({}) too high for the channel reserve we require the remote to keep ({})", msg.dust_limit_satoshis, holder_selected_channel_reserve_satoshis)));
 		}

lightning/src/ln/functional_test_utils.rs

@@ -526,19 +526,16 @@ pub fn create_funding_transaction<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, expected_
 		_ => panic!("Unexpected event"),
 	}
 }
-
-pub fn create_chan_between_nodes_with_value_init<'a, 'b, 'c>(node_a: &Node<'a, 'b, 'c>, node_b: &Node<'a, 'b, 'c>, channel_value: u64, push_msat: u64, a_flags: InitFeatures, b_flags: InitFeatures) -> Transaction {
-	let create_chan_id = node_a.node.create_channel(node_b.node.get_our_node_id(), channel_value, push_msat, 42, None).unwrap();
-	node_b.node.handle_open_channel(&node_a.node.get_our_node_id(), a_flags, &get_event_msg!(node_a, MessageSendEvent::SendOpenChannel, node_b.node.get_our_node_id()));
-	node_a.node.handle_accept_channel(&node_b.node.get_our_node_id(), b_flags, &get_event_msg!(node_b, MessageSendEvent::SendAcceptChannel, node_a.node.get_our_node_id()));
-
+pub fn sign_funding_transaction<'a, 'b, 'c>(node_a: &Node<'a, 'b, 'c>, node_b: &Node<'a, 'b, 'c>, channel_value: u64, expected_temporary_channel_id: [u8; 32]) -> Transaction {
 	let (temporary_channel_id, tx, funding_output) = create_funding_transaction(node_a, channel_value, 42);
-	assert_eq!(temporary_channel_id, create_chan_id);
+	assert_eq!(temporary_channel_id, expected_temporary_channel_id);
 
 	node_a.node.funding_transaction_generated(&temporary_channel_id, tx.clone()).unwrap();
 	check_added_monitors!(node_a, 0);
 
-	node_b.node.handle_funding_created(&node_a.node.get_our_node_id(), &get_event_msg!(node_a, MessageSendEvent::SendFundingCreated, node_b.node.get_our_node_id()));
+	let funding_created_msg = get_event_msg!(node_a, MessageSendEvent::SendFundingCreated, node_b.node.get_our_node_id());
+	assert_eq!(funding_created_msg.temporary_channel_id, expected_temporary_channel_id);
+	node_b.node.handle_funding_created(&node_a.node.get_our_node_id(), &funding_created_msg);
 	{
 		let mut added_monitors = node_b.chain_monitor.added_monitors.lock().unwrap();
 		assert_eq!(added_monitors.len(), 1);
@@ -564,6 +561,18 @@ pub fn create_chan_between_nodes_with_value_init<'a, 'b, 'c>(node_a: &Node<'a, '
 	tx
 }
 
+pub fn create_chan_between_nodes_with_value_init<'a, 'b, 'c>(node_a: &Node<'a, 'b, 'c>, node_b: &Node<'a, 'b, 'c>, channel_value: u64, push_msat: u64, a_flags: InitFeatures, b_flags: InitFeatures) -> Transaction {
+	let create_chan_id = node_a.node.create_channel(node_b.node.get_our_node_id(), channel_value, push_msat, 42, None).unwrap();
+	let open_channel_msg = get_event_msg!(node_a, MessageSendEvent::SendOpenChannel, node_b.node.get_our_node_id());
+	assert_eq!(open_channel_msg.temporary_channel_id, create_chan_id);
+	node_b.node.handle_open_channel(&node_a.node.get_our_node_id(), a_flags, &open_channel_msg);
+	let accept_channel_msg = get_event_msg!(node_b, MessageSendEvent::SendAcceptChannel, node_a.node.get_our_node_id());
+	assert_eq!(accept_channel_msg.temporary_channel_id, create_chan_id);
+	node_a.node.handle_accept_channel(&node_b.node.get_our_node_id(), b_flags, &accept_channel_msg);
+
+	sign_funding_transaction(node_a, node_b, channel_value, create_chan_id)
+}
+
 pub fn create_chan_between_nodes_with_value_confirm_first<'a, 'b, 'c, 'd>(node_recv: &'a Node<'b, 'c, 'c>, node_conf: &'a Node<'b, 'c, 'd>, tx: &Transaction, conf_height: u32) {
 	confirm_transaction_at(node_conf, tx, conf_height);
 	connect_blocks(node_conf, CHAN_CONFIRM_DEPTH - 1);

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, FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE};
 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};
@@ -108,8 +108,6 @@ fn test_insane_channel_opens() {
 
 	insane_open_helper("Peer never wants payout outputs?", |mut msg| { msg.dust_limit_satoshis = msg.funding_satoshis + 1 ; msg });
 
-	insane_open_helper(r"Bogus; channel reserve \(\d+\) is less than dust limit \(\d+\)", |mut msg| { msg.dust_limit_satoshis = msg.channel_reserve_satoshis + 1; msg });
-
 	insane_open_helper(r"Minimum htlc value \(\d+\) was larger than full channel value \(\d+\)", |mut msg| { msg.htlc_minimum_msat = (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000; msg });
 
 	insane_open_helper("They wanted our payments to be delayed by a needlessly long period", |mut msg| { msg.to_self_delay = MAX_LOCAL_BREAKDOWN_TIMEOUT + 1; msg });
@@ -119,6 +117,67 @@ fn test_insane_channel_opens() {
 	insane_open_helper("max_accepted_htlcs was 484. It must not be larger than 483", |mut msg| { msg.max_accepted_htlcs = 484; msg });
 }
 
+fn do_test_counterparty_no_reserve(send_from_initiator: bool) {
+	// A peer providing a channel_reserve_satoshis of 0 (or less than our dust limit) is insecure,
+	// but only for them. Because some LSPs do it with some level of trust of the clients (for a
+	// substantial UX improvement), we explicitly allow it. Because its unlikely to happen often in
+	// normal testing, we test it explicitly here.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	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);
+
+	// Have node0 initiate a channel to node1 with aforementioned parameters
+	let mut push_amt = 100_000_000;
+	let feerate_per_kw = 253;
+	push_amt -= feerate_per_kw as u64 * (COMMITMENT_TX_BASE_WEIGHT + 4 * COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000 * 1000;
+	push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000) * 1000;
+
+	let temp_channel_id = nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100_000, if send_from_initiator { 0 } else { push_amt }, 42, None).unwrap();
+	let mut open_channel_message = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
+	if !send_from_initiator {
+		open_channel_message.channel_reserve_satoshis = 0;
+		open_channel_message.max_htlc_value_in_flight_msat = 100_000_000;
+	}
+	nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &open_channel_message);
+
+	// Extract the channel accept message from node1 to node0
+	let mut accept_channel_message = get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id());
+	if send_from_initiator {
+		accept_channel_message.channel_reserve_satoshis = 0;
+		accept_channel_message.max_htlc_value_in_flight_msat = 100_000_000;
+	}
+	nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), InitFeatures::known(), &accept_channel_message);
+	{
+		let mut lock;
+		let mut chan = get_channel_ref!(if send_from_initiator { &nodes[1] } else { &nodes[0] }, lock, temp_channel_id);
+		chan.holder_selected_channel_reserve_satoshis = 0;
+		chan.holder_max_htlc_value_in_flight_msat = 100_000_000;
+	}
+
+	let funding_tx = sign_funding_transaction(&nodes[0], &nodes[1], 100_000, temp_channel_id);
+	let funding_msgs = create_chan_between_nodes_with_value_confirm(&nodes[0], &nodes[1], &funding_tx);
+	create_chan_between_nodes_with_value_b(&nodes[0], &nodes[1], &funding_msgs.0);
+
+	// nodes[0] should now be able to send the full balance to nodes[1], violating nodes[1]'s
+	// security model if it ever tries to send funds back to nodes[0] (but that's not our problem).
+	if send_from_initiator {
+		send_payment(&nodes[0], &[&nodes[1]], 100_000_000
+			// Note that for outbound channels we have to consider the commitment tx fee and the
+			// "fee spike buffer", which is currently a multiple of the total commitment tx fee as
+			// well as an additional HTLC.
+			- FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * feerate_per_kw * (COMMITMENT_TX_BASE_WEIGHT + 2 * COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000 * 1000);
+	} else {
+		send_payment(&nodes[1], &[&nodes[0]], push_amt);
+	}
+}
+
+#[test]
+fn test_counterparty_no_reserve() {
+	do_test_counterparty_no_reserve(true);
+	do_test_counterparty_no_reserve(false);
+}
+
 #[test]
 fn test_async_inbound_update_fee() {
 	let chanmon_cfgs = create_chanmon_cfgs(2);