From: Wilmer Paulino Date: Wed, 15 Jun 2022 23:33:23 +0000 (-0700) Subject: Allow forwarding HTLCs that were constructed for previous config X-Git-Tag: v0.0.109~10^2~1 X-Git-Url: http://git.bitcoin.ninja/?a=commitdiff_plain;h=e14f25ce0c01059c2d2d1b08f7ddc0fb925245ee;p=rust-lightning Allow forwarding HTLCs that were constructed for previous config 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. --- diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 8c237992a..f46d0aa88 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4551,6 +4551,43 @@ impl Channel { 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 } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index fcd53b502..ef37dabc8 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2230,7 +2230,7 @@ impl 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 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 diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 38c05998e..fe78395e2 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -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); diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index 27c701bcf..c66f45a44 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -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 { + prev_update: Option<&msgs::ChannelUpdate>, should_expire_prev_config: bool| -> Option { 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.