Fix channel reserve calculation on the sending side

As the variable name implies holder_selected_chan_reserve_msat is
intended to be in millisatoshis, but is instead calculated in
satoshis.

We fix that error here and update the relevant tests to more
accurately calculate the expected reserve value and test both
success and failure cases.

Bug discovered by chanmon_consistency fuzz target.

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -251,6 +251,7 @@ fn check_api_err(api_err: APIError) {
 				_ if err.starts_with("Cannot push more than their max accepted HTLCs ") => {},
 				_ if err.starts_with("Cannot send value that would put us over the max HTLC value in flight our peer will accept ") => {},
 				_ if err.starts_with("Cannot send value that would put our balance under counterparty-announced channel reserve value") => {},
+				_ if err.starts_with("Cannot send value that would put counterparty balance under holder-announced channel reserve value") => {},
 				_ if err.starts_with("Cannot send value that would overdraw remaining funds.") => {},
 				_ if err.starts_with("Cannot send value that would not leave enough to pay for fees.") => {},
 				_ => panic!("{}", err),

lightning/src/ln/channel.rs

@@ -4070,7 +4070,7 @@ 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);
+			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 {

lightning/src/ln/functional_tests.rs

@@ -22,6 +22,7 @@ use ln::channel::{COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC};
 use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA};
 use ln::channel::{Channel, ChannelError};
 use ln::{chan_utils, onion_utils};
+use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
 use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route};
 use routing::network_graph::RoutingFees;
 use ln::features::{ChannelFeatures, InitFeatures, InvoiceFeatures, NodeFeatures};
@@ -1734,14 +1735,24 @@ 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.
-	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 feerate_per_kw = 253;
+	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 _ = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 95000000, InitFeatures::known(), InitFeatures::known());
 
-	let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 4843000);
+	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 -= 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);
+
+	// 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);
 	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"));
 	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
@@ -1793,21 +1804,34 @@ fn test_chan_reserve_violation_inbound_htlc_outbound_channel() {
 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 chanmon_cfgs = create_chanmon_cfgs(2);
+	let mut chanmon_cfgs = create_chanmon_cfgs(2);
+	let feerate_per_kw = 253;
+	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]);
 	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
 	// 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)).
-	create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 98817000, InitFeatures::known(), InitFeatures::known());
+	let mut push_amt = 100_000_000;
+	push_amt -= feerate_per_kw as u64 * (COMMITMENT_TX_BASE_WEIGHT) / 1000 * 1000;
+	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());
 
-	let dust_amt = 329000; // Dust amount
+	let dust_amt = crate::ln::channel::MIN_DUST_LIMIT_SATOSHIS * 1000
+		+ feerate_per_kw as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000 * 1000 - 1;
 	// In the previous code, routing this dust payment would cause nodes[0] to perceive a channel
 	// reserve violation even though it's a dust HTLC and therefore shouldn't count towards the
 	// commitment transaction fee.
 	let (_, _, _) = route_payment(&nodes[1], &[&nodes[0]], dust_amt);
+
+	// 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]