Allow forwarding HTLCs that were constructed for previous config
authorWilmer Paulino <wilmer.paulino@gmail.com>
Wed, 15 Jun 2022 23:33:23 +0000 (16:33 -0700)
committerWilmer Paulino <wilmer.paulino@gmail.com>
Mon, 20 Jun 2022 20:12:57 +0000 (13:12 -0700)
This is mostly motivated by the fact that payments may happen while the
latest `ChannelUpdate` indicating our new `ChannelConfig` is still
propagating throughout the network. By temporarily allowing the previous
config, we can help reduce payment failures across the network.

lightning/src/ln/channel.rs
lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_test_utils.rs
lightning/src/ln/onion_route_tests.rs

index 8c237992ab75ebd64a9c2860fca0b1210b0fbe0e..f46d0aa88aab34057b35c1353b9a3bf069304975 100644 (file)
@@ -4551,6 +4551,43 @@ impl<Signer: Sign> Channel<Signer> {
                did_channel_update
        }
 
+       fn internal_htlc_satisfies_config(
+               &self, htlc: &msgs::UpdateAddHTLC, amt_to_forward: u64, outgoing_cltv_value: u32, config: &ChannelConfig,
+       ) -> Result<(), (&'static str, u16)> {
+               let fee = amt_to_forward.checked_mul(config.forwarding_fee_proportional_millionths as u64)
+                       .and_then(|prop_fee| (prop_fee / 1000000).checked_add(config.forwarding_fee_base_msat as u64));
+               if fee.is_none() || htlc.amount_msat < fee.unwrap() ||
+                       (htlc.amount_msat - fee.unwrap()) < amt_to_forward {
+                       return Err((
+                               "Prior hop has deviated from specified fees parameters or origin node has obsolete ones",
+                               0x1000 | 12, // fee_insufficient
+                       ));
+               }
+               if (htlc.cltv_expiry as u64) < outgoing_cltv_value as u64 + config.cltv_expiry_delta as u64 {
+                       return Err((
+                               "Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta",
+                               0x1000 | 13, // incorrect_cltv_expiry
+                       ));
+               }
+               Ok(())
+       }
+
+       /// Determines whether the parameters of an incoming HTLC to be forwarded satisfy the channel's
+       /// [`ChannelConfig`]. This first looks at the channel's current [`ChannelConfig`], and if
+       /// unsuccessful, falls back to the previous one if one exists.
+       pub fn htlc_satisfies_config(
+               &self, htlc: &msgs::UpdateAddHTLC, amt_to_forward: u64, outgoing_cltv_value: u32,
+       ) -> Result<(), (&'static str, u16)> {
+               self.internal_htlc_satisfies_config(&htlc, amt_to_forward, outgoing_cltv_value, &self.config())
+                       .or_else(|err| {
+                               if let Some(prev_config) = self.prev_config() {
+                                       self.internal_htlc_satisfies_config(htlc, amt_to_forward, outgoing_cltv_value, &prev_config)
+                               } else {
+                                       Err(err)
+                               }
+                       })
+       }
+
        pub fn get_feerate(&self) -> u32 {
                self.feerate_per_kw
        }
index fcd53b502f24946b2d8dee96bb636075834b2e6d..ef37dabc8ade68150602216d2ae42ad29fc67d4d 100644 (file)
@@ -2230,7 +2230,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                },
                                                Some(id) => Some(id.clone()),
                                        };
-                                       let (chan_update_opt, forwardee_cltv_expiry_delta) = if let Some(forwarding_id) = forwarding_id_opt {
+                                       let chan_update_opt = if let Some(forwarding_id) = forwarding_id_opt {
                                                let chan = channel_state.as_mut().unwrap().by_id.get_mut(&forwarding_id).unwrap();
                                                if !chan.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels {
                                                        // Note that the behavior here should be identical to the above block - we
@@ -2257,18 +2257,20 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                if *amt_to_forward < chan.get_counterparty_htlc_minimum_msat() { // amount_below_minimum
                                                        break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, chan_update_opt));
                                                }
-                                               let fee = amt_to_forward.checked_mul(chan.get_fee_proportional_millionths() as u64)
-                                                       .and_then(|prop_fee| { (prop_fee / 1000000)
-                                                       .checked_add(chan.get_outbound_forwarding_fee_base_msat() as u64) });
-                                               if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient
-                                                       break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, chan_update_opt));
+                                               if let Err((err, code)) = chan.htlc_satisfies_config(&msg, *amt_to_forward, *outgoing_cltv_value) {
+                                                       break Some((err, code, chan_update_opt));
                                                }
-                                               (chan_update_opt, chan.get_cltv_expiry_delta())
-                                       } else { (None, MIN_CLTV_EXPIRY_DELTA) };
+                                               chan_update_opt
+                                       } else {
+                                               if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + MIN_CLTV_EXPIRY_DELTA as u64 { // incorrect_cltv_expiry
+                                                       break Some((
+                                                               "Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta",
+                                                               0x1000 | 13, None,
+                                                       ));
+                                               }
+                                               None
+                                       };
 
-                                       if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + forwardee_cltv_expiry_delta as u64 { // incorrect_cltv_expiry
-                                               break Some(("Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta", 0x1000 | 13, chan_update_opt));
-                                       }
                                        let cur_height = self.best_block.read().unwrap().height() + 1;
                                        // Theoretically, channel counterparty shouldn't send us a HTLC expiring now,
                                        // but we want to be robust wrt to counterparty packet sanitization (see
index 38c05998e6a2769db297179d4f0ce0dc76589b9e..fe78395e2f97e685698968b0053d44dbc8a772ce 100644 (file)
@@ -1689,9 +1689,16 @@ pub fn do_claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>,
                        ($node: expr, $prev_node: expr, $next_node: expr, $new_msgs: expr) => {
                                {
                                        $node.node.handle_update_fulfill_htlc(&$prev_node.node.get_our_node_id(), &next_msgs.as_ref().unwrap().0);
-                                       let fee = $node.node.channel_state.lock().unwrap()
-                                               .by_id.get(&next_msgs.as_ref().unwrap().0.channel_id).unwrap()
-                                               .config.options.forwarding_fee_base_msat;
+                                       let fee = {
+                                               let channel_state = $node.node.channel_state.lock().unwrap();
+                                               let channel = channel_state
+                                                       .by_id.get(&next_msgs.as_ref().unwrap().0.channel_id).unwrap();
+                                               if let Some(prev_config) = channel.prev_config() {
+                                                       prev_config.forwarding_fee_base_msat
+                                               } else {
+                                                       channel.config().forwarding_fee_base_msat
+                                               }
+                                       };
                                        expect_payment_forwarded!($node, $next_node, $prev_node, Some(fee as u64), false, false);
                                        expected_total_fee_msat += fee as u64;
                                        check_added_monitors!($node, 1);
index 27c701bcfbd5e755e33e4376ccf104e8fffef14c..c66f45a44923b596e1e31ac0a08d2ee531826128 100644 (file)
@@ -14,6 +14,7 @@
 use chain::channelmonitor::{ChannelMonitor, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS};
 use chain::keysinterface::{KeysInterface, Recipient};
 use ln::{PaymentHash, PaymentSecret};
+use ln::channel::EXPIRE_PREV_CONFIG_TICKS;
 use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, HTLCForwardInfo, CLTV_FAR_FAR_AWAY, MIN_CLTV_EXPIRY_DELTA, PendingHTLCInfo, PendingHTLCRouting};
 use ln::onion_utils;
 use routing::gossip::{NetworkUpdate, RoutingFees, NodeId};
@@ -648,9 +649,16 @@ fn do_test_onion_failure_stale_channel_update(announced_channel: bool) {
                payment_hash, payment_secret);
        claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage);
 
+       // Closure to force expiry of a channel's previous config.
+       let expire_prev_config = || {
+               for _ in 0..EXPIRE_PREV_CONFIG_TICKS {
+                       nodes[1].node.timer_tick_occurred();
+               }
+       };
+
        // Closure to update and retrieve the latest ChannelUpdate.
        let update_and_get_channel_update = |config: &ChannelConfig, expect_new_update: bool,
-               prev_update: Option<&msgs::ChannelUpdate>| -> Option<msgs::ChannelUpdate> {
+               prev_update: Option<&msgs::ChannelUpdate>, should_expire_prev_config: bool| -> Option<msgs::ChannelUpdate> {
                nodes[1].node.update_channel_config(
                        channel_to_update_counterparty, &[channel_to_update.0], config,
                ).unwrap();
@@ -674,6 +682,9 @@ fn do_test_onion_failure_stale_channel_update(announced_channel: bool) {
                if prev_update.is_some() {
                        assert!(new_update.contents.timestamp > prev_update.unwrap().contents.timestamp)
                }
+               if should_expire_prev_config {
+                       expire_prev_config();
+               }
                Some(new_update)
        };
 
@@ -704,28 +715,37 @@ fn do_test_onion_failure_stale_channel_update(announced_channel: bool) {
                .find(|channel| channel.channel_id == channel_to_update.0).unwrap()
                .config.unwrap();
        config.forwarding_fee_base_msat = u32::max_value();
-       let msg = update_and_get_channel_update(&config, true, None).unwrap();
+       let msg = update_and_get_channel_update(&config, true, None, false).unwrap();
+
+       // The old policy should still be in effect until a new block is connected.
+       send_along_route_with_secret(&nodes[0], route.clone(), &[&[&nodes[1], &nodes[2]]], PAYMENT_AMT,
+               payment_hash, payment_secret);
+       claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage);
+
+       // Connect a block, which should expire the previous config, leading to a failure when
+       // forwarding the HTLC.
+       expire_prev_config();
        expect_onion_failure("fee_insufficient", UPDATE|12, &msg);
 
        // Redundant updates should not trigger a new ChannelUpdate.
-       assert!(update_and_get_channel_update(&config, false, None).is_none());
+       assert!(update_and_get_channel_update(&config, false, None, false).is_none());
 
        // Similarly, updates that do not have an affect on ChannelUpdate should not trigger a new one.
        config.force_close_avoidance_max_fee_satoshis *= 2;
-       assert!(update_and_get_channel_update(&config, false, None).is_none());
+       assert!(update_and_get_channel_update(&config, false, None, false).is_none());
 
        // Reset the base fee to the default and increase the proportional fee which should trigger a
        // new ChannelUpdate.
        config.forwarding_fee_base_msat = default_config.forwarding_fee_base_msat;
        config.cltv_expiry_delta = u16::max_value();
-       let msg = update_and_get_channel_update(&config, true, Some(&msg)).unwrap();
+       let msg = update_and_get_channel_update(&config, true, Some(&msg), true).unwrap();
        expect_onion_failure("incorrect_cltv_expiry", UPDATE|13, &msg);
 
        // Reset the proportional fee and increase the CLTV expiry delta which should trigger a new
        // ChannelUpdate.
        config.cltv_expiry_delta = default_config.cltv_expiry_delta;
        config.forwarding_fee_proportional_millionths = u32::max_value();
-       let msg = update_and_get_channel_update(&config, true, Some(&msg)).unwrap();
+       let msg = update_and_get_channel_update(&config, true, Some(&msg), true).unwrap();
        expect_onion_failure("fee_insufficient", UPDATE|12, &msg);
 
        // To test persistence of the updated config, we'll re-initialize the ChannelManager.