diff --git a/fuzz/src/router.rs b/fuzz/src/router.rs index 568dcdf0250..fe6f1647f4d 100644 --- a/fuzz/src/router.rs +++ b/fuzz/src/router.rs @@ -227,7 +227,7 @@ pub fn do_test(data: &[u8], out: Out) { }, 4 => { let short_channel_id = slice_to_be64(get_slice!(8)); - net_graph.channel_failed(short_channel_id, false); + net_graph.channel_failed_permanent(short_channel_id); }, _ if node_pks.is_empty() => {}, _ => { diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index b9f36bbd8da..54b6ecdeef8 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -493,21 +493,28 @@ pub(super) fn process_onion_failure(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(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 couldn'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()); } } } diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index 59268b840cd..e5f5e63c938 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -212,7 +212,7 @@ pub enum NetworkUpdate { msg: ChannelUpdate, }, /// An error indicating that a channel failed to route a payment, which should be applied via - /// [`NetworkGraph::channel_failed`]. + /// [`NetworkGraph::channel_failed_permanent`] if permanent. ChannelFailure { /// The short channel id of the closed channel. short_channel_id: u64, @@ -352,9 +352,10 @@ impl NetworkGraph where L::Target: Logger { let _ = self.update_channel(msg); }, NetworkUpdate::ChannelFailure { short_channel_id, is_permanent } => { - let action = if is_permanent { "Removing" } else { "Disabling" }; - log_debug!(self.logger, "{} channel graph entry for {} due to a payment failure.", action, short_channel_id); - self.channel_failed(short_channel_id, is_permanent); + if is_permanent { + log_debug!(self.logger, "Removing channel graph entry for {} due to a payment failure.", short_channel_id); + self.channel_failed_permanent(short_channel_id); + } }, NetworkUpdate::NodeFailure { ref node_id, is_permanent } => { if is_permanent { @@ -1632,40 +1633,27 @@ impl NetworkGraph where L::Target: Logger { Ok(()) } - /// 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. - pub fn channel_failed(&self, short_channel_id: u64, is_permanent: bool) { + /// Marks a channel in the graph as failed permanently. + /// + /// The channel and any node for which this was their last channel are removed from the graph. + pub fn channel_failed_permanent(&self, short_channel_id: u64) { #[cfg(feature = "std")] let current_time_unix = Some(SystemTime::now().duration_since(UNIX_EPOCH).expect("Time must be > 1970").as_secs()); #[cfg(not(feature = "std"))] let current_time_unix = None; - self.channel_failed_with_time(short_channel_id, is_permanent, current_time_unix) + self.channel_failed_permanent_with_time(short_channel_id, current_time_unix) } - /// 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. - fn channel_failed_with_time(&self, short_channel_id: u64, is_permanent: bool, current_time_unix: Option) { + /// Marks a channel in the graph as failed permanently. + /// + /// The channel and any node for which this was their last channel are removed from the graph. + fn channel_failed_permanent_with_time(&self, short_channel_id: u64, current_time_unix: Option) { let mut channels = self.channels.write().unwrap(); - if is_permanent { - if let Some(chan) = channels.remove(&short_channel_id) { - let mut nodes = self.nodes.write().unwrap(); - 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; - } - } + if let Some(chan) = channels.remove(&short_channel_id) { + let mut nodes = self.nodes.write().unwrap(); + self.removed_channels.lock().unwrap().insert(short_channel_id, current_time_unix); + Self::remove_channel_in_nodes(&mut nodes, &chan, short_channel_id); } } @@ -2450,7 +2438,7 @@ pub(crate) mod tests { assert!(network_graph.read_only().channels().get(&short_channel_id).unwrap().one_to_two.is_some()); } - // Non-permanent closing just disables a channel + // Non-permanent failure doesn't touch the channel at all { match network_graph.read_only().channels().get(&short_channel_id) { None => panic!(), @@ -2467,7 +2455,7 @@ pub(crate) mod tests { match network_graph.read_only().channels().get(&short_channel_id) { None => panic!(), Some(channel_info) => { - assert!(!channel_info.one_to_two.as_ref().unwrap().enabled); + assert!(channel_info.one_to_two.as_ref().unwrap().enabled); } }; } @@ -2601,7 +2589,7 @@ pub(crate) mod tests { // Mark the channel as permanently failed. This will also remove the two nodes // and all of the entries will be tracked as removed. - network_graph.channel_failed_with_time(short_channel_id, true, Some(tracking_time)); + network_graph.channel_failed_permanent_with_time(short_channel_id, Some(tracking_time)); // Should not remove from tracking if insufficient time has passed network_graph.remove_stale_channels_and_tracking_with_time( @@ -2634,7 +2622,7 @@ pub(crate) mod tests { // Mark the channel as permanently failed. This will also remove the two nodes // and all of the entries will be tracked as removed. - network_graph.channel_failed(short_channel_id, true); + network_graph.channel_failed_permanent(short_channel_id); // The first time we call the following, the channel will have a removal time assigned. network_graph.remove_stale_channels_and_tracking_with_time(removal_time);