diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 22880ec9d07..f1d4c95a1a8 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2096,6 +2096,26 @@ impl ChannelMana }) } + fn check_htlc_forward( + &self, htlc: &msgs::UpdateAddHTLC, amt_to_forward: u64, outgoing_cltv_value: u32, config: &ChannelConfig, + ) -> Option<(&'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 Some(( + "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 Some(( + "Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta", + 0x1000 | 13, // incorrect_cltv_expiry + )); + } + None + } + fn decode_update_add_htlc_onion(&self, msg: &msgs::UpdateAddHTLC) -> (PendingHTLCStatus, MutexGuard>) { macro_rules! return_malformed_err { ($msg: expr, $err_code: expr) => { @@ -2230,7 +2250,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 @@ -2246,6 +2266,14 @@ impl ChannelMana } let chan_update_opt = self.get_channel_update_for_onion(*short_channel_id, chan).ok(); + // We attempt to forward the HTLC with the current policy, or the previous + // for a short period of time. + if let Some(prev_config) = chan.get_prev_config() { + if chan.get_update_time_counter() > prev_config.1 + 1 { + chan.clear_prev_config(); + } + } + // Note that we could technically not return an error yet here and just hope // that the connection is reestablished or monitor updated by the time we get // around to doing the actual forward, but better to fail early if we can and @@ -2257,18 +2285,25 @@ 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)); + match self.check_htlc_forward( + &msg, *amt_to_forward, *outgoing_cltv_value, &chan.get_config(), + ) { + Some((err, code)) => { + if let Some(prev_config) = chan.get_prev_config() { + if let Some((err, code)) = self.check_htlc_forward( + &msg, *amt_to_forward, *outgoing_cltv_value, &prev_config.0 + ) { + break Some((err, code, chan_update_opt)); + } + } else { + break Some((err, code, chan_update_opt)); + } + }, + None => {}, } - (chan_update_opt, chan.get_cltv_expiry_delta()) - } else { (None, MIN_CLTV_EXPIRY_DELTA) }; + chan_update_opt + } else { 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 38c05998e6a..1fb9c356664 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.get_prev_config() { + prev_config.0.forwarding_fee_base_msat + } else { + channel.get_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 77ba18e366b..c98bad199f0 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -616,9 +616,25 @@ fn test_onion_failure_stale_channel_update() { const PAYMENT_AMT: u64 = 40000; send_payment(&nodes[0], &[&nodes[1], &nodes[2]], PAYMENT_AMT); + // Closure to connect a block with a timestamp that will increase a channel's + // `update_time_counter`. + let connect_block = |timestamp: u32| { + connect_block(&nodes[1], &bitcoin::Block { + header: bitcoin::BlockHeader { + version: 2, + prev_blockhash: nodes[1].best_block_hash(), + merkle_root: Default::default(), + time: timestamp, + bits: 42, + nonce: 42, + }, + txdata: vec![], + }); + }; + // 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>, expire_prev_config: bool| -> Option { nodes[1].node.update_channel_config( channel_to_update_counterparty, &[channel_to_update.2], config, ).unwrap(); @@ -634,13 +650,16 @@ fn test_onion_failure_stale_channel_update() { if prev_update.is_some() { assert!(new_update.contents.timestamp > prev_update.unwrap().contents.timestamp) } + if expire_prev_config { + connect_block(new_update.contents.timestamp + 1); + } Some(new_update) }; // We'll be attempting to route payments using the default ChannelUpdate for channels. This will // lead to onion failures at the first hop once we update the HTLC relay parameters for the // second hop. - let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!( + let (route, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!( nodes[0], nodes[2], PAYMENT_AMT ); let expect_onion_failure = |name: &str, error_code: u16, channel_update: &msgs::ChannelUpdate| { @@ -667,28 +686,37 @@ fn test_onion_failure_stale_channel_update() { .find(|channel| channel.channel_id == channel_to_update.2).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. + connect_block(msg.contents.timestamp + 1); 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.