Don't remove nodes if there's no channel_update for a temp failure

Previously, we were requiring any `UPDATE` onion errors to include
a `channel_update`, as the spec mandates[1]. If we see an onion
error which is missing one we treat it as a misbehaving node that
isn't behaving according to the spec and simply remove the node.

Sadly, it appears at least some versions of CLN are such nodes, and
opt to not include `channel_update` at all if they're returning a
`temporary_channel_failure`. This causes us to completely remove
CLN nodes from our graph after they fail to forward our HTLC.

While CLN is violating the spec here, there's not a lot of reason
to not allow it, so we go ahead and do so here, treating it simply
as any other failure by letting the scorer handle it.

[1] The spec says `Please note that the channel_update field is
mandatory in messages whose failure_code includes the UPDATE flag`
however doesn't repeat it in the requirements section so its not
crazy that someone missed it when implementing.

Author: TheBlueMatt

lightning/src/ln/onion_utils.rs

@@ -493,21 +493,28 @@ pub(super) fn process_onion_failure<T: secp256k1::Signing, L: Deref>(secp_ctx: &
 									} else {
 										log_trace!(logger, "Failure provided features a channel update without type prefix. Deprecated, but allowing for now.");
 									}
-									if let Ok(chan_update) = msgs::ChannelUpdate::read(&mut Cursor::new(&update_slice)) {
+									let update_opt = msgs::ChannelUpdate::read(&mut Cursor::new(&update_slice));
+									if update_opt.is_ok() || update_slice.is_empty() {
 										// if channel_update should NOT have caused the failure:
 										// MAY treat the channel_update as invalid.
 										let is_chan_update_invalid = match error_code & 0xff {
 											7 => false,
-											11 => amt_to_forward > chan_update.contents.htlc_minimum_msat,
-											12 => amt_to_forward
-												.checked_mul(chan_update.contents.fee_proportional_millionths as u64)
+											11 => update_opt.is_ok() &&
+												amt_to_forward >
+													update_opt.as_ref().unwrap().contents.htlc_minimum_msat,
+											12 => update_opt.is_ok() && amt_to_forward
+												.checked_mul(update_opt.as_ref().unwrap()
+													.contents.fee_proportional_millionths as u64)
 												.map(|prop_fee| prop_fee / 1_000_000)
-												.and_then(|prop_fee| prop_fee.checked_add(chan_update.contents.fee_base_msat as u64))
+												.and_then(|prop_fee| prop_fee.checked_add(
+													update_opt.as_ref().unwrap().contents.fee_base_msat as u64))
 												.map(|fee_msats| route_hop.fee_msat >= fee_msats)
 												.unwrap_or(false),
-											13 => route_hop.cltv_expiry_delta as u16 >= chan_update.contents.cltv_expiry_delta,
+											13 => update_opt.is_ok() &&
+												route_hop.cltv_expiry_delta as u16 >=
+													update_opt.as_ref().unwrap().contents.cltv_expiry_delta,
 											14 => false, // expiry_too_soon; always valid?
-											20 => chan_update.contents.flags & 2 == 0,
+											20 => update_opt.as_ref().unwrap().contents.flags & 2 == 0,
 											_ => false, // unknown error code; take channel_update as valid
 										};
 										if is_chan_update_invalid {
@@ -518,17 +525,31 @@ pub(super) fn process_onion_failure<T: secp256k1::Signing, L: Deref>(secp_ctx: &
 												is_permanent: true,
 											});
 										} else {
-											// Make sure the ChannelUpdate contains the expected
-											// short channel id.
-											if failing_route_hop.short_channel_id == chan_update.contents.short_channel_id {
-												short_channel_id = Some(failing_route_hop.short_channel_id);
+											if let Ok(chan_update) = update_opt {
+												// Make sure the ChannelUpdate contains the expected
+												// short channel id.
+												if failing_route_hop.short_channel_id == chan_update.contents.short_channel_id {
+													short_channel_id = Some(failing_route_hop.short_channel_id);
+												} else {
+													log_info!(logger, "Node provided a channel_update for which it was not authoritative, ignoring.");
+												}
+												network_update = Some(NetworkUpdate::ChannelUpdateMessage {
+													msg: chan_update,
+												})
 											} else {
-												log_info!(logger, "Node provided a channel_update for which it was not authoritative, ignoring.");
+												network_update = Some(NetworkUpdate::ChannelFailure {
+													short_channel_id: route_hop.short_channel_id,
+													is_permanent: false,
+												});
 											}
-											network_update = Some(NetworkUpdate::ChannelUpdateMessage {
-												msg: chan_update,
-											})
 										};
+									} else {
+										// If the channel_update had a non-zero length (i.e. was
+										// present) but we coulnd't read it, treat it as a total
+										// node failure.
+										log_info!(logger,
+											"Failed to read a channel_update of len {} in an onion",
+											update_slice.len());
 									}
 								}
 							}

lightning/src/routing/gossip.rs

@@ -1633,9 +1633,12 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
 	}
 
 	/// Marks a channel in the graph as failed if a corresponding HTLC fail was sent.
+	///
 	/// If permanent, removes a channel from the local storage.
 	/// May cause the removal of nodes too, if this was their last channel.
-	/// If not permanent, makes channels unavailable for routing.
+	///
+	/// If not permanent, no action is taken as such a failure likely indicates the node simply
+	/// lacked liquidity and your scorer should handle this instead.
 	pub fn channel_failed(&self, short_channel_id: u64, is_permanent: bool) {
 		#[cfg(feature = "std")]
 		let current_time_unix = Some(SystemTime::now().duration_since(UNIX_EPOCH).expect("Time must be > 1970").as_secs());
@@ -1646,9 +1649,12 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
 	}
 
 	/// Marks a channel in the graph as failed if a corresponding HTLC fail was sent.
+	///
 	/// If permanent, removes a channel from the local storage.
 	/// May cause the removal of nodes too, if this was their last channel.
-	/// If not permanent, makes channels unavailable for routing.
+	///
+	/// If not permanent, no action is taken as such a failure likely indicates the node simply
+	/// lacked liquidity and your scorer should handle this instead.
 	fn channel_failed_with_time(&self, short_channel_id: u64, is_permanent: bool, current_time_unix: Option<u64>) {
 		let mut channels = self.channels.write().unwrap();
 		if is_permanent {
@@ -1657,15 +1663,6 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
 				self.removed_channels.lock().unwrap().insert(short_channel_id, current_time_unix);
 				Self::remove_channel_in_nodes(&mut nodes, &chan, short_channel_id);
 			}
-		} else {
-			if let Some(chan) = channels.get_mut(&short_channel_id) {
-				if let Some(one_to_two) = chan.one_to_two.as_mut() {
-					one_to_two.enabled = false;
-				}
-				if let Some(two_to_one) = chan.two_to_one.as_mut() {
-					two_to_one.enabled = false;
-				}
-			}
 		}
 	}