Allow forwarding HTLCs that were constructed for previous config

Author: wpaulino

lightning/src/ln/channelmanager.rs

@@ -2096,6 +2096,26 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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<ChannelHolder<Signer>>) {
 		macro_rules! return_malformed_err {
 			($msg: expr, $err_code: expr) => {
@@ -2230,7 +2250,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
@@ -2246,6 +2266,14 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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<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));
+						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

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);

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<msgs::ChannelUpdate> {
+		prev_update: Option<&msgs::ChannelUpdate>, expire_prev_config: bool| -> Option<msgs::ChannelUpdate> {
 		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.