Merge pull request #1527 from wpaulino/update-htlc-relay-policy

Expose API to update a channel's ChannelConfig

Author: TheBlueMatt

fuzz/src/router.rs

@@ -235,6 +235,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 								next_outbound_htlc_limit_msat: capacity.saturating_mul(1000),
 								inbound_htlc_minimum_msat: None,
 								inbound_htlc_maximum_msat: None,
+								config: None,
 							});
 						}
 						Some(&first_hops_vec[..])

lightning/src/ln/channel.rs

@@ -39,7 +39,7 @@ use util::events::ClosureReason;
 use util::ser::{Readable, ReadableArgs, Writeable, Writer, VecWriter};
 use util::logger::Logger;
 use util::errors::APIError;
-use util::config::{UserConfig, LegacyChannelConfig, ChannelHandshakeConfig, ChannelHandshakeLimits};
+use util::config::{UserConfig, ChannelConfig, LegacyChannelConfig, ChannelHandshakeConfig, ChannelHandshakeLimits};
 use util::scid_utils::scid_from_parts;
 
 use io;
@@ -482,6 +482,16 @@ pub(crate) const CONCURRENT_INBOUND_HTLC_FEE_BUFFER: u32 = 2;
 /// transaction (not counting the value of the HTLCs themselves).
 pub(crate) const MIN_AFFORDABLE_HTLC_COUNT: usize = 4;
 
+/// When a [`Channel`] has its [`ChannelConfig`] updated, its existing one is stashed for up to this
+/// number of ticks to allow forwarding HTLCs by nodes that have yet to receive the new
+/// ChannelUpdate prompted by the config update. This value was determined as follows:
+///
+///   * The expected interval between ticks (1 minute).
+///   * The average convergence delay of updates across the network, i.e., ~300 seconds on average
+///      for a node to see an update as seen on `<https://arxiv.org/pdf/2205.12737.pdf>`.
+///   * `EXPIRE_PREV_CONFIG_TICKS` = convergence_delay / tick_interval
+pub(crate) const EXPIRE_PREV_CONFIG_TICKS: usize = 5;
+
 // TODO: We should refactor this to be an Inbound/OutboundChannel until initial setup handshaking
 // has been completed, and then turn into a Channel to get compiler-time enforcement of things like
 // calling channel_id() before we're set up or things like get_outbound_funding_signed on an
@@ -490,11 +500,13 @@ pub(crate) const MIN_AFFORDABLE_HTLC_COUNT: usize = 4;
 // Holder designates channel data owned for the benefice of the user client.
 // Counterparty designates channel data owned by the another channel participant entity.
 pub(super) struct Channel<Signer: Sign> {
-	#[cfg(any(test, feature = "_test_utils"))]
-	pub(crate) config: LegacyChannelConfig,
-	#[cfg(not(any(test, feature = "_test_utils")))]
 	config: LegacyChannelConfig,
 
+	// Track the previous `ChannelConfig` so that we can continue forwarding HTLCs that were
+	// constructed using it. The second element in the tuple corresponds to the number of ticks that
+	// have elapsed since the update occurred.
+	prev_config: Option<(ChannelConfig, usize)>,
+
 	inbound_handshake_limits_override: Option<ChannelHandshakeLimits>,
 
 	user_id: u64,
@@ -937,6 +949,8 @@ impl<Signer: Sign> Channel<Signer> {
 				commit_upfront_shutdown_pubkey: config.channel_handshake_config.commit_upfront_shutdown_pubkey,
 			},
 
+			prev_config: None,
+
 			inbound_handshake_limits_override: Some(config.channel_handshake_limits.clone()),
 
 			channel_id: keys_provider.get_secure_random_bytes(),
@@ -1264,6 +1278,8 @@ impl<Signer: Sign> Channel<Signer> {
 				commit_upfront_shutdown_pubkey: config.channel_handshake_config.commit_upfront_shutdown_pubkey,
 			},
 
+			prev_config: None,
+
 			inbound_handshake_limits_override: None,
 
 			channel_id: msg.temporary_channel_id,
@@ -4491,6 +4507,84 @@ impl<Signer: Sign> Channel<Signer> {
 		self.config.options.max_dust_htlc_exposure_msat
 	}
 
+	/// Returns the previous [`ChannelConfig`] applied to this channel, if any.
+	pub fn prev_config(&self) -> Option<ChannelConfig> {
+		self.prev_config.map(|prev_config| prev_config.0)
+	}
+
+	/// Tracks the number of ticks elapsed since the previous [`ChannelConfig`] was updated. Once
+	/// [`EXPIRE_PREV_CONFIG_TICKS`] is reached, the previous config is considered expired and will
+	/// no longer be considered when forwarding HTLCs.
+	pub fn maybe_expire_prev_config(&mut self) {
+		if self.prev_config.is_none() {
+			return;
+		}
+		let prev_config = self.prev_config.as_mut().unwrap();
+		prev_config.1 += 1;
+		if prev_config.1 == EXPIRE_PREV_CONFIG_TICKS {
+			self.prev_config = None;
+		}
+	}
+
+	/// Returns the current [`ChannelConfig`] applied to the channel.
+	pub fn config(&self) -> ChannelConfig {
+		self.config.options
+	}
+
+	/// Updates the channel's config. A bool is returned indicating whether the config update
+	/// applied resulted in a new ChannelUpdate message.
+	pub fn update_config(&mut self, config: &ChannelConfig) -> bool {
+		let did_channel_update =
+			self.config.options.forwarding_fee_proportional_millionths != config.forwarding_fee_proportional_millionths ||
+			self.config.options.forwarding_fee_base_msat != config.forwarding_fee_base_msat ||
+			self.config.options.cltv_expiry_delta != config.cltv_expiry_delta;
+		if did_channel_update {
+			self.prev_config = Some((self.config.options, 0));
+			// Update the counter, which backs the ChannelUpdate timestamp, to allow the relay
+			// policy change to propagate throughout the network.
+			self.update_time_counter += 1;
+		}
+		self.config.options = *config;
+		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
 	}
@@ -6339,6 +6433,8 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<Signer>
 
 			config: config.unwrap(),
 
+			prev_config: None,
+
 			// Note that we don't care about serializing handshake limits as we only ever serialize
 			// channel data after the handshake has completed.
 			inbound_handshake_limits_override: None,

lightning/src/ln/channelmanager.rs

@@ -51,7 +51,7 @@ use ln::onion_utils;
 use ln::msgs::{ChannelMessageHandler, DecodeError, LightningError, MAX_VALUE_MSAT, OptionalField};
 use ln::wire::Encode;
 use chain::keysinterface::{Sign, KeysInterface, KeysManager, InMemorySigner, Recipient};
-use util::config::UserConfig;
+use util::config::{UserConfig, ChannelConfig};
 use util::events::{EventHandler, EventsProvider, MessageSendEvent, MessageSendEventsProvider, ClosureReason};
 use util::{byte_utils, events};
 use util::scid_utils::fake_scid;
@@ -1101,6 +1101,10 @@ pub struct ChannelDetails {
 	pub inbound_htlc_minimum_msat: Option<u64>,
 	/// The largest value HTLC (in msat) we currently will accept, for this channel.
 	pub inbound_htlc_maximum_msat: Option<u64>,
+	/// Set of configurable parameters that affect channel operation.
+	///
+	/// This field is only `None` for `ChannelDetails` objects serialized prior to LDK 0.0.109.
+	pub config: Option<ChannelConfig>,
 }
 
 impl ChannelDetails {
@@ -1765,7 +1769,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					is_usable: channel.is_live(),
 					is_public: channel.should_announce(),
 					inbound_htlc_minimum_msat: Some(channel.get_holder_htlc_minimum_msat()),
-					inbound_htlc_maximum_msat: channel.get_holder_htlc_maximum_msat()
+					inbound_htlc_maximum_msat: channel.get_holder_htlc_maximum_msat(),
+					config: Some(channel.config()),
 				});
 			}
 		}
@@ -2231,7 +2236,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
@@ -2258,18 +2263,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
+					} 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,
+							));
 						}
-						(chan_update_opt, chan.get_cltv_expiry_delta())
-					} else { (None, MIN_CLTV_EXPIRY_DELTA) };
+						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
@@ -2940,6 +2947,73 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		}
 	}
 
+	/// Atomically updates the [`ChannelConfig`] for the given channels.
+	///
+	/// Once the updates are applied, each eligible channel (advertised with a known short channel
+	/// ID and a change in [`forwarding_fee_proportional_millionths`], [`forwarding_fee_base_msat`],
+	/// or [`cltv_expiry_delta`]) has a [`BroadcastChannelUpdate`] event message generated
+	/// containing the new [`ChannelUpdate`] message which should be broadcast to the network.
+	///
+	/// Returns [`ChannelUnavailable`] when a channel is not found or an incorrect
+	/// `counterparty_node_id` is provided.
+	///
+	/// Returns [`APIMisuseError`] when a [`cltv_expiry_delta`] update is to be applied with a value
+	/// below [`MIN_CLTV_EXPIRY_DELTA`].
+	///
+	/// If an error is returned, none of the updates should be considered applied.
+	///
+	/// [`forwarding_fee_proportional_millionths`]: ChannelConfig::forwarding_fee_proportional_millionths
+	/// [`forwarding_fee_base_msat`]: ChannelConfig::forwarding_fee_base_msat
+	/// [`cltv_expiry_delta`]: ChannelConfig::cltv_expiry_delta
+	/// [`BroadcastChannelUpdate`]: events::MessageSendEvent::BroadcastChannelUpdate
+	/// [`ChannelUpdate`]: msgs::ChannelUpdate
+	/// [`ChannelUnavailable`]: APIError::ChannelUnavailable
+	/// [`APIMisuseError`]: APIError::APIMisuseError
+	pub fn update_channel_config(
+		&self, counterparty_node_id: &PublicKey, channel_ids: &[[u8; 32]], config: &ChannelConfig,
+	) -> Result<(), APIError> {
+		if config.cltv_expiry_delta < MIN_CLTV_EXPIRY_DELTA {
+			return Err(APIError::APIMisuseError {
+				err: format!("The chosen CLTV expiry delta is below the minimum of {}", MIN_CLTV_EXPIRY_DELTA),
+			});
+		}
+
+		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(
+			&self.total_consistency_lock, &self.persistence_notifier,
+		);
+		{
+			let mut channel_state_lock = self.channel_state.lock().unwrap();
+			let channel_state = &mut *channel_state_lock;
+			for channel_id in channel_ids {
+				let channel_counterparty_node_id = channel_state.by_id.get(channel_id)
+					.ok_or(APIError::ChannelUnavailable {
+						err: format!("Channel with ID {} was not found", log_bytes!(*channel_id)),
+					})?
+					.get_counterparty_node_id();
+				if channel_counterparty_node_id != *counterparty_node_id {
+					return Err(APIError::APIMisuseError {
+						err: "counterparty node id mismatch".to_owned(),
+					});
+				}
+			}
+			for channel_id in channel_ids {
+				let channel = channel_state.by_id.get_mut(channel_id).unwrap();
+				if !channel.update_config(config) {
+					continue;
+				}
+				if let Ok(msg) = self.get_channel_update_for_broadcast(channel) {
+					channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate { msg });
+				} else if let Ok(msg) = self.get_channel_update_for_unicast(channel) {
+					channel_state.pending_msg_events.push(events::MessageSendEvent::SendChannelUpdate {
+						node_id: channel.get_counterparty_node_id(),
+						msg,
+					});
+				}
+			}
+		}
+		Ok(())
+	}
+
 	/// Processes HTLCs which are pending waiting on random forward delay.
 	///
 	/// Should only really ever be called in response to a PendingHTLCsForwardable event.
@@ -3465,6 +3539,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	///  * Broadcasting `ChannelUpdate` messages if we've been disconnected from our peer for more
 	///    than a minute, informing the network that they should no longer attempt to route over
 	///    the channel.
+	///  * Expiring a channel's previous `ChannelConfig` if necessary to only allow forwarding HTLCs
+	///    with the current `ChannelConfig`.
 	///
 	/// Note that this may cause reentrancy through `chain::Watch::update_channel` calls or feerate
 	/// estimate fetches.
@@ -3523,6 +3599,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 						_ => {},
 					}
 
+					chan.maybe_expire_prev_config();
+
 					true
 				});
 
@@ -6115,6 +6193,7 @@ impl_writeable_tlv_based!(ChannelDetails, {
 	(4, counterparty, required),
 	(5, outbound_scid_alias, option),
 	(6, funding_txo, option),
+	(7, config, option),
 	(8, short_channel_id, option),
 	(10, channel_value_satoshis, required),
 	(12, unspendable_punishment_reserve, option),

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