From 99e4a1fbb63ce05c9923f5645c05d9faf2387238 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 6 Dec 2023 01:13:33 +0000 Subject: [PATCH] Privatise `CandidateRouteHop::short_channel_id` as its a footgun Short channel "ID"s are not globally unique when they come from a BOLT 11 route hint or a first hop (which can be an outbound SCID alias). In those cases, its rather confusing that we have a `short_channel_id` method which mixes them all together, and even more confusing that we have a `CandidateHopId` which is not, in fact returning a unique identifier. In our routing logic this is mostly fine - the cost of a collision isn't super high and we should still do just fine finding a route, however the same can't be true for downstream users, as they may or may not rely on the apparent guarantees. Thus, here, we privatise the SCID and id accessors. --- lightning/src/routing/router.rs | 71 +++++++++++++++++++++++--------- lightning/src/routing/scoring.rs | 12 +++--- lightning/src/util/test_utils.rs | 2 +- 3 files changed, 58 insertions(+), 27 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 6300763e2..ff2dfe1b2 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1055,8 +1055,8 @@ pub enum CandidateRouteHop<'a> { hint: &'a (BlindedPayInfo, BlindedPath), /// Index of the hint in the original list of blinded hints. /// - /// This is used to build a [`CandidateHopId`] that uniquely identifies this blinded path, - /// even though we don't have a short channel ID for this hop. + /// This is used to cheaply uniquely identify this blinded path, even though we don't have + /// a short channel ID for this hop. hint_idx: usize, }, /// Similar to [`Self::Blinded`], but the path here only has one hop. @@ -1078,18 +1078,29 @@ pub enum CandidateRouteHop<'a> { hint: &'a (BlindedPayInfo, BlindedPath), /// Index of the hint in the original list of blinded hints. /// - /// This is used to build a [`CandidateHopId`] that uniquely identifies this blinded path, - /// even though we don't have a short channel ID for this hop. + /// This is used to cheaply uniquely identify this blinded path, even though we don't have + /// a short channel ID for this hop. hint_idx: usize, }, } impl<'a> CandidateRouteHop<'a> { - /// Returns short_channel_id if known. - /// For `FirstHop` we assume [`ChannelDetails::get_outbound_payment_scid`] is always set, this assumption is checked in - /// [`find_route`] method. - /// For `Blinded` and `OneHopBlinded` we return `None` because next hop is not known. - pub fn short_channel_id(&self) -> Option { + /// Returns the short channel ID for this hop, if one is known. + /// + /// This SCID could be an alias or a globally unique SCID, and thus is only expected to + /// uniquely identify this channel in conjunction with the [`CandidateRouteHop::source`]. + /// + /// Returns `Some` as long as the candidate is a [`CandidateRouteHop::PublicHop`], a + /// [`CandidateRouteHop::PrivateHop`] from a BOLT 11 route hint, or a + /// [`CandidateRouteHop::FirstHop`] with a known [`ChannelDetails::get_outbound_payment_scid`] + /// (which is always true for channels which are funded and ready for use). + /// + /// In other words, this should always return `Some` as long as the candidate hop is not a + /// [`CandidateRouteHop::Blinded`] or a [`CandidateRouteHop::OneHopBlinded`]. + /// + /// Note that this is deliberately not public as it is somewhat of a footgun because it doesn't + /// define a global namespace. + fn short_channel_id(&self) -> Option { match self { CandidateRouteHop::FirstHop { details, .. } => details.get_outbound_payment_scid(), CandidateRouteHop::PublicHop { short_channel_id, .. } => Some(*short_channel_id), @@ -1099,6 +1110,21 @@ impl<'a> CandidateRouteHop<'a> { } } + /// Returns the globally unique short channel ID for this hop, if one is known. + /// + /// This only returns `Some` if the channel is public (either our own, or one we've learned + /// from the public network graph), and thus the short channel ID we have for this channel is + /// globally unique and identifies this channel in a global namespace. + pub fn globally_unique_short_channel_id(&self) -> Option { + match self { + CandidateRouteHop::FirstHop { details, .. } => if details.is_public { details.short_channel_id } else { None }, + CandidateRouteHop::PublicHop { short_channel_id, .. } => Some(*short_channel_id), + CandidateRouteHop::PrivateHop { .. } => None, + CandidateRouteHop::Blinded { .. } => None, + CandidateRouteHop::OneHopBlinded { .. } => None, + } + } + // NOTE: This may alloc memory so avoid calling it in a hot code path. fn features(&self) -> ChannelFeatures { match self { @@ -1167,10 +1193,10 @@ impl<'a> CandidateRouteHop<'a> { } } - /// Returns the id of this hop. - /// For `Blinded` and `OneHopBlinded` we return `CandidateHopId::Blinded` with `hint_idx` because we don't know the channel id. - /// For any other option we return `CandidateHopId::Clear` because we know the channel id and the direction. - pub fn id(&self) -> CandidateHopId { + /// Returns an ID describing the given hop. + /// + /// See the docs on [`CandidateHopId`] for when this is, or is not, unique. + fn id(&self) -> CandidateHopId { match self { CandidateRouteHop::Blinded { hint_idx, .. } => CandidateHopId::Blinded(*hint_idx), CandidateRouteHop::OneHopBlinded { hint_idx, .. } => CandidateHopId::Blinded(*hint_idx), @@ -1215,12 +1241,17 @@ impl<'a> CandidateRouteHop<'a> { } } -/// A wrapper around the various hop id representations. +/// A unique(ish) identifier for a specific [`CandidateRouteHop`]. +/// +/// For blinded paths, this ID is unique only within a given [`find_route`] call. +/// +/// For other hops, because SCIDs between private channels and public channels can conflict, this +/// isn't guaranteed to be unique at all. /// -/// `CandidateHopId::Clear` is used to identify a hop with a known short channel id and direction. -/// `CandidateHopId::Blinded` is used to identify a blinded hop `hint_idx`. +/// For our uses, this is generally fine, but it is not public as it is otherwise a rather +/// difficult-to-use API. #[derive(Clone, Copy, Eq, Hash, Ord, PartialOrd, PartialEq)] -pub enum CandidateHopId { +enum CandidateHopId { /// Contains (scid, src_node_id < target_node_id) Clear((u64, bool)), /// Index of the blinded route hint in [`Payee::Blinded::route_hints`]. @@ -2446,8 +2477,10 @@ where L::Target: Logger { let target = ordered_hops.last().unwrap().0.candidate.target().unwrap_or(maybe_dummy_payee_node_id); if let Some(first_channels) = first_hop_targets.get(&target) { for details in first_channels { - if let Some(scid) = ordered_hops.last().unwrap().0.candidate.short_channel_id() { - if details.get_outbound_payment_scid().unwrap() == scid { + if let CandidateRouteHop::FirstHop { details: last_hop_details, .. } + = ordered_hops.last().unwrap().0.candidate + { + if details.get_outbound_payment_scid() == last_hop_details.get_outbound_payment_scid() { ordered_hops.last_mut().unwrap().1 = details.counterparty.features.to_context(); features_set = true; break; diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs index 151ac41e9..80438de67 100644 --- a/lightning/src/routing/scoring.rs +++ b/lightning/src/routing/scoring.rs @@ -1344,13 +1344,11 @@ impl>, L: Deref, T: Time> ScoreLookUp for Prob fn channel_penalty_msat( &self, candidate: &CandidateRouteHop, usage: ChannelUsage, score_params: &ProbabilisticScoringFeeParameters ) -> u64 { - let scid = match candidate.short_channel_id() { - Some(scid) => scid, - None => return 0, - }; - let target = match candidate.target() { - Some(target) => target, - None => return 0, + let (scid, target) = match candidate { + CandidateRouteHop::PublicHop { info, short_channel_id } => { + (short_channel_id, info.target()) + }, + _ => return 0, }; let source = candidate.source(); if let Some(penalty) = score_params.manual_node_penalties.get(&target) { diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 9bbbec0d7..125ba812e 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -1324,7 +1324,7 @@ impl ScoreLookUp for TestScorer { fn channel_penalty_msat( &self, candidate: &CandidateRouteHop, usage: ChannelUsage, _score_params: &Self::ScoreParams ) -> u64 { - let short_channel_id = match candidate.short_channel_id() { + let short_channel_id = match candidate.globally_unique_short_channel_id() { Some(scid) => scid, None => return 0, }; -- 2.39.5