]> git.bitcoin.ninja Git - rust-lightning/commitdiff
Privatise `CandidateRouteHop::short_channel_id` as its a footgun
authorMatt Corallo <git@bluematt.me>
Wed, 6 Dec 2023 01:13:33 +0000 (01:13 +0000)
committerMatt Corallo <git@bluematt.me>
Fri, 8 Dec 2023 20:45:06 +0000 (20:45 +0000)
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
lightning/src/routing/scoring.rs
lightning/src/util/test_utils.rs

index 6300763e2a20e34c90850e12c9965524a82bda95..ff2dfe1b25e631cd828b121b91c0802d9bf44284 100644 (file)
@@ -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<u64> {
+       /// 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<u64> {
                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<u64> {
+               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;
index 151ac41e990b5361379308e56af4192d0d198e61..80438de67bfa496e7702a8d5f551c75cc9e2d39f 100644 (file)
@@ -1344,13 +1344,11 @@ impl<G: Deref<Target = NetworkGraph<L>>, 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) {
index 9bbbec0d78e6a182e6e8e2553e60642db9645f52..125ba812e78007b7b1872b2efec2c5e51af25a17 100644 (file)
@@ -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,
                };