From 0f6b0004c490855ad88ce1bb545268eb6561fdc2 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 14 Sep 2020 17:39:42 -0400 Subject: [PATCH] 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). --- lightning/src/ln/channel.rs | 3 ++- lightning/src/ln/channelmanager.rs | 2 +- lightning/src/ln/onion_route_tests.rs | 18 +++++++++++++++++- 3 files changed, 20 insertions(+), 3 deletions(-) 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 Channel { } /// 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 Channel { /// 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 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| { -- 2.39.5