WIP: Don't apply PathFailure::ChannelUpdateMessage

If we receive a channel update from an intermediary via a failure onion
we shouldn't apply them in a persisted and network-observable way to our
network graph, as this might introduce a privacy leak. Here, we
therefore avoid applying such updates to our network graph.

In order to not reduce our test coverage of channel update appliance too
much, we opt to make `PathFailure::ChannelUpdateMessage`
`#[cfg(test)]`-only where we continue to use it. However, we still don't
apply them to the network graph in tests as otherwise the tested
behavior might diverge too much from the production case.

Author: tnull

lightning/src/ln/functional_test_utils.rs

@@ -2190,6 +2190,7 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
 			if let Some(chan_closed) = conditions.expected_blamed_chan_closed {
 				if let PathFailure::OnPath { network_update: Some(upd) } = failure {
 					match upd {
+						#[cfg(test)]
 						NetworkUpdate::ChannelUpdateMessage { ref msg } if !chan_closed => {
 							if let Some(scid) = conditions.expected_blamed_scid {
 								assert_eq!(msg.contents.short_channel_id, scid);

lightning/src/ln/onion_utils.rs

@@ -635,9 +635,12 @@ pub(super) fn process_onion_failure<T: secp256k1::Signing, L: Deref>(
 								} else {
 									log_info!(logger, "Node provided a channel_update for which it was not authoritative, ignoring.");
 								}
-								network_update = Some(NetworkUpdate::ChannelUpdateMessage {
-									msg: chan_update,
-								})
+								#[cfg(test)]
+								{
+									network_update = Some(NetworkUpdate::ChannelUpdateMessage {
+										msg: chan_update,
+									});
+								}
 							} else {
 								// The node in question intentionally encoded a 0-length channel update. This is
 								// likely due to https://github.com/ElementsProject/lightning/issues/6200.

lightning/src/routing/gossip.rs

@@ -208,6 +208,7 @@ pub struct ReadOnlyNetworkGraph<'a> {
 pub enum NetworkUpdate {
 	/// An error indicating a `channel_update` messages should be applied via
 	/// [`NetworkGraph::update_channel`].
+	#[cfg(test)]
 	ChannelUpdateMessage {
 		/// The update to apply via [`NetworkGraph::update_channel`].
 		msg: ChannelUpdate,
@@ -232,6 +233,7 @@ pub enum NetworkUpdate {
 	}
 }
 
+#[cfg(test)]
 impl_writeable_tlv_based_enum_upgradable!(NetworkUpdate,
 	(0, ChannelUpdateMessage) => {
 		(0, msg, required),
@@ -246,6 +248,19 @@ impl_writeable_tlv_based_enum_upgradable!(NetworkUpdate,
 	},
 );
 
+#[cfg(not(test))]
+impl_writeable_tlv_based_enum_upgradable!(NetworkUpdate,
+	// 0 used to be ChannelUpdateMessage in LDK versions prior to 0.0.118
+	(2, ChannelFailure) => {
+		(0, short_channel_id, required),
+		(2, is_permanent, required),
+	},
+	(4, NodeFailure) => {
+		(0, node_id, required),
+		(2, is_permanent, required),
+	},
+);
+
 /// Receives and validates network updates from peers,
 /// stores authentic and relevant data as a network graph.
 /// This network graph is then used for routing payments.
@@ -345,12 +360,15 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
 	/// [`Event`]: crate::events::Event
 	pub fn handle_network_update(&self, network_update: &NetworkUpdate) {
 		match *network_update {
+			#[cfg(test)]
 			NetworkUpdate::ChannelUpdateMessage { ref msg } => {
 				let short_channel_id = msg.contents.short_channel_id;
 				let is_enabled = msg.contents.flags & (1 << 1) != (1 << 1);
 				let status = if is_enabled { "enabled" } else { "disabled" };
-				log_debug!(self.logger, "Updating channel with channel_update from a payment failure. Channel {} is {}.", short_channel_id, status);
-				let _ = self.update_channel(msg);
+				log_debug!(self.logger, "Skipping application of a channel update from a payment failure. Channel {} is {}.", short_channel_id, status);
+				// While the `ChannelUpdateMessage` variant is available in tests we don't apply
+				// them to the network graph to avoid having our test behavior diverge too much
+				// from the production case.
 			},
 			NetworkUpdate::ChannelFailure { short_channel_id, is_permanent } => {
 				if is_permanent {
@@ -2531,7 +2549,8 @@ pub(crate) mod tests {
 
 		let short_channel_id;
 		{
-			// Announce a channel we will update
+			// Check we won't apply an update via `handle_network_update` for privacy reasons, but
+			// can continue fine if we manually apply it.
 			let valid_channel_announcement = get_signed_channel_announcement(|_| {}, node_1_privkey, node_2_privkey, &secp_ctx);
 			short_channel_id = valid_channel_announcement.contents.short_channel_id;
 			let chain_source: Option<&test_utils::TestChainSource> = None;
@@ -2542,10 +2561,11 @@ pub(crate) mod tests {
 			assert!(network_graph.read_only().channels().get(&short_channel_id).unwrap().one_to_two.is_none());
 
 			network_graph.handle_network_update(&NetworkUpdate::ChannelUpdateMessage {
-				msg: valid_channel_update,
+				msg: valid_channel_update.clone(),
 			});
 
-			assert!(network_graph.read_only().channels().get(&short_channel_id).unwrap().one_to_two.is_some());
+			assert!(network_graph.read_only().channels().get(&short_channel_id).unwrap().one_to_two.is_none());
+			network_graph.update_channel(&valid_channel_update).unwrap();
 		}
 
 		// Non-permanent failure doesn't touch the channel at all