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 2699e461806ff247163ea44e8820d376f42c70a8..dd9170b47e7d8541d88597a358eb7ffa138045a2 100644 (file)
--- 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 cc15861f8772f4556ad2600b431c6b4a54a95a26..59d513b396825eb5e86f7fbf0514691829c4f818 100644 (file)
--- 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 9a91efa498033314188decb11c369f41a8125d95..3db4c4d52f16fcedcb2391e9e6270a0005fe1bbf 100644 (file)
--- 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| {