From: Matt Corallo <git@bluematt.me>
Date: Mon, 14 Sep 2020 21:39:42 +0000 (-0400)
Subject: Fix two bugs which access the wrong peer's htlc_minimum_msat value
X-Git-Tag: v0.0.12~24^2
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=0f6b0004c490855ad88ce1bb545268eb6561fdc2;p=rust-lightning

Fix two bugs which access the wrong peer's htlc_minimum_msat value

 * Channel::get_counterparty_htlc_minimum_msat() returned
   holder_htlc_minimum_msat, which was obviously incorrect.
 * ChannelManager::get_channel_update set htlc_minimum_msat to
   Channel::get_holder_htlc_minimum_msat(), but the spec explicitly
   states we "MUST set htlc_minimum_msat to the minimum HTLC value
   (in millisatoshi) that the channel peer will accept." This makes
   sense because the reason we're rejecting the HTLC is because our
   counterparty's HTLC minimum value is too small for us to send to
   them, our own HTLC minimum value plays no role. Further, our
   router already expects this - looking at the same directional
   channel info as it does fees.

Finally, we add a test in the existing onion router test cases
which fails if either of the above is incorrect (the second issue
discovered in the process of writing the test).
---

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 2699e4618..dd9170b47 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -3125,6 +3125,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	}
 
 	/// Allowed in any state (including after shutdown)
+	#[cfg(test)]
 	pub fn get_holder_htlc_minimum_msat(&self) -> u64 {
 		self.holder_htlc_minimum_msat
 	}
@@ -3143,7 +3144,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 	/// Allowed in any state (including after shutdown)
 	pub fn get_counterparty_htlc_minimum_msat(&self) -> u64 {
-		self.holder_htlc_minimum_msat
+		self.counterparty_htlc_minimum_msat
 	}
 
 	pub fn get_value_satoshis(&self) -> u64 {
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index cc15861f8..59d513b39 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -1228,7 +1228,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 			timestamp: chan.get_update_time_counter(),
 			flags: (!were_node_one) as u8 | ((!chan.is_live() as u8) << 1),
 			cltv_expiry_delta: CLTV_EXPIRY_DELTA,
-			htlc_minimum_msat: chan.get_holder_htlc_minimum_msat(),
+			htlc_minimum_msat: chan.get_counterparty_htlc_minimum_msat(),
 			htlc_maximum_msat: OptionalField::Present(chan.get_announced_htlc_max_msat()),
 			fee_base_msat: chan.get_holder_fee_base_msat(&self.fee_estimator),
 			fee_proportional_millionths: chan.get_fee_proportional_millionths(),
diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs
index 9a91efa49..3db4c4d52 100644
--- a/lightning/src/ln/onion_route_tests.rs
+++ b/lightning/src/ln/onion_route_tests.rs
@@ -21,6 +21,7 @@ use ln::msgs::{ChannelMessageHandler, HTLCFailChannelUpdate, OptionalField};
 use util::test_utils;
 use util::events::{Event, EventsProvider, MessageSendEvent, MessageSendEventsProvider};
 use util::ser::{Writeable, Writer};
+use util::config::UserConfig;
 
 use bitcoin::blockdata::block::BlockHeader;
 use bitcoin::hash_types::BlockHash;
@@ -257,9 +258,19 @@ fn test_onion_failure() {
 	const NODE: u16 = 0x2000;
 	const UPDATE: u16 = 0x1000;
 
+	// When we check for amount_below_minimum below, we want to test that we're using the *right*
+	// amount, thus we need different htlc_minimum_msat values. We set node[2]'s htlc_minimum_msat
+	// to 2000, which is above the default value of 1000 set in create_node_chanmgrs.
+	// This exposed a previous bug because we were using the wrong value all the way down in
+	// Channel::get_counterparty_htlc_minimum_msat().
+	let mut node_2_cfg: UserConfig = Default::default();
+	node_2_cfg.own_channel_config.our_htlc_minimum_msat = 2000;
+	node_2_cfg.channel_options.announced_channel = true;
+	node_2_cfg.peer_channel_config_limits.force_announced_channel_preference = false;
+
 	let chanmon_cfgs = create_chanmon_cfgs(3);
 	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
-	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
+	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, Some(node_2_cfg)]);
 	let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
 	for node in nodes.iter() {
 		*node.keys_manager.override_session_priv.lock().unwrap() = Some([3; 32]);
@@ -411,6 +422,11 @@ fn test_onion_failure() {
 	bogus_route.paths[0][route_len-1].fee_msat = amt_to_forward;
 	run_onion_failure_test("amount_below_minimum", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));
 
+	// Test a positive test-case with one extra msat, meeting the minimum.
+	bogus_route.paths[0][route_len-1].fee_msat = amt_to_forward + 1;
+	let (preimage, _) = send_along_route(&nodes[0], bogus_route, &[&nodes[1], &nodes[2]], amt_to_forward+1);
+	claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], preimage, amt_to_forward+1);
+
 	//TODO: with new config API, we will be able to generate both valid and
 	//invalid channel_update cases.
 	run_onion_failure_test("fee_insufficient", 0, &nodes, &route, &payment_hash, |msg| {