Merge pull request #2774 from TheBlueMatt/2023-12-2551-followups
authorElias Rohrer <dev@tnull.de>
Fri, 8 Dec 2023 22:46:43 +0000 (23:46 +0100)
committerGitHub <noreply@github.com>
Fri, 8 Dec 2023 22:46:43 +0000 (23:46 +0100)
Doc and performance followups to #2551

lightning/src/routing/gossip.rs
lightning/src/routing/router.rs
lightning/src/routing/scoring.rs
lightning/src/util/test_utils.rs

index edb372616972770fc1481cd576d99718c32002b0..9b4e41ae174d7b8a73cb8f64b7918f62a0dbf01b 100644 (file)
@@ -990,54 +990,38 @@ impl Readable for ChannelInfo {
 pub struct DirectedChannelInfo<'a> {
        channel: &'a ChannelInfo,
        direction: &'a ChannelUpdateInfo,
-       htlc_maximum_msat: u64,
-       effective_capacity: EffectiveCapacity,
-       /// Outbound from the perspective of `node_one`.
-       ///
-       /// If true, the channel is considered to be outbound from `node_one` perspective.
-       /// If false, the channel is considered to be outbound from `node_two` perspective.
-       ///
-       /// [`ChannelInfo::node_one`]
-       /// [`ChannelInfo::node_two`]
-       outbound: bool,
+       /// The direction this channel is in - if set, it indicates that we're traversing the channel
+       /// from [`ChannelInfo::node_one`] to [`ChannelInfo::node_two`].
+       from_node_one: bool,
 }
 
 impl<'a> DirectedChannelInfo<'a> {
        #[inline]
-       fn new(channel: &'a ChannelInfo, direction: &'a ChannelUpdateInfo, outbound: bool) -> Self {
-               let mut htlc_maximum_msat = direction.htlc_maximum_msat;
-               let capacity_msat = channel.capacity_sats.map(|capacity_sats| capacity_sats * 1000);
-
-               let effective_capacity = match capacity_msat {
-                       Some(capacity_msat) => {
-                               htlc_maximum_msat = cmp::min(htlc_maximum_msat, capacity_msat);
-                               EffectiveCapacity::Total { capacity_msat, htlc_maximum_msat }
-                       },
-                       None => EffectiveCapacity::AdvertisedMaxHTLC { amount_msat: htlc_maximum_msat },
-               };
-
-               Self {
-                       channel, direction, htlc_maximum_msat, effective_capacity, outbound
-               }
+       fn new(channel: &'a ChannelInfo, direction: &'a ChannelUpdateInfo, from_node_one: bool) -> Self {
+               Self { channel, direction, from_node_one }
        }
 
        /// Returns information for the channel.
        #[inline]
        pub fn channel(&self) -> &'a ChannelInfo { self.channel }
 
-       /// Returns the maximum HTLC amount allowed over the channel in the direction.
-       #[inline]
-       pub fn htlc_maximum_msat(&self) -> u64 {
-               self.htlc_maximum_msat
-       }
-
        /// Returns the [`EffectiveCapacity`] of the channel in the direction.
        ///
        /// This is either the total capacity from the funding transaction, if known, or the
        /// `htlc_maximum_msat` for the direction as advertised by the gossip network, if known,
        /// otherwise.
+       #[inline]
        pub fn effective_capacity(&self) -> EffectiveCapacity {
-               self.effective_capacity
+               let mut htlc_maximum_msat = self.direction().htlc_maximum_msat;
+               let capacity_msat = self.channel.capacity_sats.map(|capacity_sats| capacity_sats * 1000);
+
+               match capacity_msat {
+                       Some(capacity_msat) => {
+                               htlc_maximum_msat = cmp::min(htlc_maximum_msat, capacity_msat);
+                               EffectiveCapacity::Total { capacity_msat, htlc_maximum_msat }
+                       },
+                       None => EffectiveCapacity::AdvertisedMaxHTLC { amount_msat: htlc_maximum_msat },
+               }
        }
 
        /// Returns information for the direction.
@@ -1047,12 +1031,14 @@ impl<'a> DirectedChannelInfo<'a> {
        /// Returns the `node_id` of the source hop.
        ///
        /// Refers to the `node_id` forwarding the payment to the next hop.
-       pub(super) fn source(&self) -> &'a NodeId { if self.outbound { &self.channel.node_one } else { &self.channel.node_two } }
+       #[inline]
+       pub(super) fn source(&self) -> &'a NodeId { if self.from_node_one { &self.channel.node_one } else { &self.channel.node_two } }
 
        /// Returns the `node_id` of the target hop.
        ///
        /// Refers to the `node_id` receiving the payment from the previous hop.
-       pub(super) fn target(&self) -> &'a NodeId { if self.outbound { &self.channel.node_two } else { &self.channel.node_one } }
+       #[inline]
+       pub(super) fn target(&self) -> &'a NodeId { if self.from_node_one { &self.channel.node_two } else { &self.channel.node_one } }
 }
 
 impl<'a> fmt::Debug for DirectedChannelInfo<'a> {
index b4cbf3cb234c9eaae6c0757131b596f88c5e7c56..9423078749d39175790a6f298831048db5482a63 100644 (file)
@@ -938,6 +938,10 @@ impl Readable for RouteHint {
 }
 
 /// A channel descriptor for a hop along a payment path.
+///
+/// While this generally comes from BOLT 11's `r` field, this struct includes more fields than are
+/// available in BOLT 11. Thus, encoding and decoding this via `lightning-invoice` is lossy, as
+/// fields not supported in BOLT 11 will be stripped.
 #[derive(Clone, Debug, Hash, Eq, PartialEq, Ord, PartialOrd)]
 pub struct RouteHintHop {
        /// The node_id of the non-target end of the route
@@ -964,33 +968,24 @@ impl_writeable_tlv_based!(RouteHintHop, {
 });
 
 #[derive(Eq, PartialEq)]
+#[repr(align(64))] // Force the size to 64 bytes
 struct RouteGraphNode {
        node_id: NodeId,
-       lowest_fee_to_node: u64,
-       total_cltv_delta: u32,
+       score: u64,
        // The maximum value a yet-to-be-constructed payment path might flow through this node.
        // This value is upper-bounded by us by:
        // - how much is needed for a path being constructed
        // - how much value can channels following this node (up to the destination) can contribute,
        //   considering their capacity and fees
        value_contribution_msat: u64,
-       /// The effective htlc_minimum_msat at this hop. If a later hop on the path had a higher HTLC
-       /// minimum, we use it, plus the fees required at each earlier hop to meet it.
-       path_htlc_minimum_msat: u64,
-       /// All penalties incurred from this hop on the way to the destination, as calculated using
-       /// channel scoring.
-       path_penalty_msat: u64,
+       total_cltv_delta: u32,
        /// The number of hops walked up to this node.
        path_length_to_node: u8,
 }
 
 impl cmp::Ord for RouteGraphNode {
        fn cmp(&self, other: &RouteGraphNode) -> cmp::Ordering {
-               let other_score = cmp::max(other.lowest_fee_to_node, other.path_htlc_minimum_msat)
-                       .saturating_add(other.path_penalty_msat);
-               let self_score = cmp::max(self.lowest_fee_to_node, self.path_htlc_minimum_msat)
-                       .saturating_add(self.path_penalty_msat);
-               other_score.cmp(&self_score).then_with(|| other.node_id.cmp(&self.node_id))
+               other.score.cmp(&self.score).then_with(|| other.node_id.cmp(&self.node_id))
        }
 }
 
@@ -1000,6 +995,16 @@ impl cmp::PartialOrd for RouteGraphNode {
        }
 }
 
+// While RouteGraphNode can be laid out with fewer bytes, performance appears to be improved
+// substantially when it is laid out at exactly 64 bytes.
+//
+// Thus, we use `#[repr(C)]` on the struct to force a suboptimal layout and check that it stays 64
+// bytes here.
+#[cfg(any(ldk_bench, not(any(test, fuzzing))))]
+const _GRAPH_NODE_SMALL: usize = 64 - core::mem::size_of::<RouteGraphNode>();
+#[cfg(any(ldk_bench, not(any(test, fuzzing))))]
+const _GRAPH_NODE_FIXED_SIZE: usize = core::mem::size_of::<RouteGraphNode>() - 64;
+
 /// A wrapper around the various hop representations.
 ///
 /// Can be used to examine the properties of a hop,
@@ -1009,73 +1014,95 @@ pub enum CandidateRouteHop<'a> {
        /// A hop from the payer, where the outbound liquidity is known.
        FirstHop {
                /// Channel details of the first hop
-               /// [`ChannelDetails::get_outbound_payment_scid`] is assumed
-               /// to always return `Some(scid)`
-               /// this assumption is checked in [`find_route`] method.
-               details: &'a ChannelDetails,
-               /// The node id of the payer.
                ///
-               /// Can be accessed via `source` method.
-               node_id: NodeId
+               /// [`ChannelDetails::get_outbound_payment_scid`] MUST be `Some` (indicating the channel
+               /// has been funded and is able to pay), and accessor methods may panic otherwise.
+               ///
+               /// [`find_route`] validates this prior to constructing a [`CandidateRouteHop`].
+               details: &'a ChannelDetails,
+               /// The node id of the payer, which is also the source side of this candidate route hop.
+               payer_node_id: &'a NodeId,
        },
-       /// A hop found in the [`ReadOnlyNetworkGraph`],
-       /// where the channel capacity may be unknown.
+       /// A hop found in the [`ReadOnlyNetworkGraph`].
        PublicHop {
-               /// channel info of the hop.
+               /// Information about the channel, including potentially its capacity and
+               /// direction-specific information.
                info: DirectedChannelInfo<'a>,
-               /// short_channel_id of the channel.
+               /// The short channel ID of the channel, i.e. the identifier by which we refer to this
+               /// channel.
                short_channel_id: u64,
        },
-       /// A hop to the payee found in the BOLT 11 payment invoice,
-       /// though not necessarily a direct
-       /// channel.
+       /// A private hop communicated by the payee, generally via a BOLT 11 invoice.
+       ///
+       /// Because BOLT 11 route hints can take multiple hops to get to the destination, this may not
+       /// terminate at the payee.
        PrivateHop {
-               /// Hint provides information about a private hop,
-               /// needed while routing through a private
-               /// channel.
+               /// Information about the private hop communicated via BOLT 11.
                hint: &'a RouteHintHop,
-               /// Node id of the next hop in route.
-               target_node_id: NodeId
+               /// Node id of the next hop in BOLT 11 route hint.
+               target_node_id: &'a NodeId
        },
-       /// The payee's identity is concealed behind
-       /// blinded paths provided in a BOLT 12 invoice.
+       /// A blinded path which starts with an introduction point and ultimately terminates with the
+       /// payee.
+       ///
+       /// Because we don't know the payee's identity, [`CandidateRouteHop::target`] will return
+       /// `None` in this state.
+       ///
+       /// Because blinded paths are "all or nothing", and we cannot use just one part of a blinded
+       /// path, the full path is treated as a single [`CandidateRouteHop`].
        Blinded {
-               /// Hint provides information about a blinded hop,
-               /// needed while routing through a blinded path.
-               /// `BlindedPayInfo` provides information needed about the
-               /// payment while routing through a blinded path.
-               /// `BlindedPath` is the blinded path to the destination.
+               /// Information about the blinded path including the fee, HTLC amount limits, and
+               /// cryptographic material required to build an HTLC through the given path.
                hint: &'a (BlindedPayInfo, BlindedPath),
                /// Index of the hint in the original list of blinded hints.
-               /// Provided to uniquely identify a hop as we are
-               /// route building.
+               ///
+               /// 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
-       /// has 1 blinded hop. `BlindedPayInfo` provided
-       /// for 1-hop blinded paths is ignored
-       /// because it is meant to apply to the hops *between* the
-       /// introduction node and the destination.
-       /// Useful for tracking that we need to include a blinded
-       /// path at the end of our [`Route`].
+       /// Similar to [`Self::Blinded`], but the path here only has one hop.
+       ///
+       /// While we treat this similarly to [`CandidateRouteHop::Blinded`] in many respects (e.g.
+       /// returning `None` from [`CandidateRouteHop::target`]), in this case we do actually know the
+       /// payee's identity - it's the introduction point!
+       ///
+       /// [`BlindedPayInfo`] provided for 1-hop blinded paths is ignored because it is meant to apply
+       /// to the hops *between* the introduction node and the destination.
+       ///
+       /// This primarily exists to track that we need to included a blinded path at the end of our
+       /// [`Route`], even though it doesn't actually add an additional hop in the payment.
        OneHopBlinded {
-               /// Hint provides information about a single blinded hop,
-               /// needed while routing through a one hop blinded path.
-               /// `BlindedPayInfo` is ignored here.
-               /// `BlindedPath` is the blinded path to the destination.
+               /// Information about the blinded path including the fee, HTLC amount limits, and
+               /// cryptographic material required to build an HTLC terminating with the given path.
+               ///
+               /// Note that the [`BlindedPayInfo`] is ignored here.
                hint: &'a (BlindedPayInfo, BlindedPath),
                /// Index of the hint in the original list of blinded hints.
-               /// Provided to uniquely identify a hop as we are route building.
+               ///
+               /// 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.
+       #[inline]
+       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),
@@ -1085,6 +1112,22 @@ 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.
+       #[inline]
+       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 {
@@ -1096,7 +1139,12 @@ impl<'a> CandidateRouteHop<'a> {
                }
        }
 
-       /// Returns cltv_expiry_delta for this hop.
+       /// Returns the required difference in HTLC CLTV expiry between the [`Self::source`] and the
+       /// next-hop for an HTLC taking this hop.
+       ///
+       /// This is the time that the node(s) in this hop have to claim the HTLC on-chain if the
+       /// next-hop goes on chain with a payment preimage.
+       #[inline]
        pub fn cltv_expiry_delta(&self) -> u32 {
                match self {
                        CandidateRouteHop::FirstHop { .. } => 0,
@@ -1107,7 +1155,8 @@ impl<'a> CandidateRouteHop<'a> {
                }
        }
 
-       /// Returns the htlc_minimum_msat for this hop.
+       /// Returns the minimum amount that can be sent over this hop, in millisatoshis.
+       #[inline]
        pub fn htlc_minimum_msat(&self) -> u64 {
                match self {
                        CandidateRouteHop::FirstHop { details, .. } => details.next_outbound_htlc_minimum_msat,
@@ -1118,7 +1167,8 @@ impl<'a> CandidateRouteHop<'a> {
                }
        }
 
-       /// Returns the fees for this hop.
+       /// Returns the fees that must be paid to route an HTLC over this channel.
+       #[inline]
        pub fn fees(&self) -> RoutingFees {
                match self {
                        CandidateRouteHop::FirstHop { .. } => RoutingFees {
@@ -1137,6 +1187,10 @@ impl<'a> CandidateRouteHop<'a> {
                }
        }
 
+       /// Fetch the effective capacity of this hop.
+       ///
+       /// Note that this may be somewhat expensive, so calls to this should be limited and results
+       /// cached!
        fn effective_capacity(&self) -> EffectiveCapacity {
                match self {
                        CandidateRouteHop::FirstHop { details, .. } => EffectiveCapacity::ExactLiquidity {
@@ -1153,10 +1207,11 @@ 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.
+       #[inline]
+       fn id(&self) -> CandidateHopId {
                match self {
                        CandidateRouteHop::Blinded { hint_idx, .. } => CandidateHopId::Blinded(*hint_idx),
                        CandidateRouteHop::OneHopBlinded { hint_idx, .. } => CandidateHopId::Blinded(*hint_idx),
@@ -1173,12 +1228,13 @@ impl<'a> CandidateRouteHop<'a> {
        }
        /// Returns the source node id of current hop.
        ///
-       /// Source node id refers to the hop forwarding the payment.
+       /// Source node id refers to the node forwarding the HTLC through this hop.
        ///
-       /// For `FirstHop` we return payer's node id.
+       /// For [`Self::FirstHop`] we return payer's node id.
+       #[inline]
        pub fn source(&self) -> NodeId {
                match self {
-                       CandidateRouteHop::FirstHop { node_id, .. } => *node_id,
+                       CandidateRouteHop::FirstHop { payer_node_id, .. } => **payer_node_id,
                        CandidateRouteHop::PublicHop { info, .. } => *info.source(),
                        CandidateRouteHop::PrivateHop { hint, .. } => hint.src_node_id.into(),
                        CandidateRouteHop::Blinded { hint, .. } => hint.1.introduction_node_id.into(),
@@ -1187,26 +1243,36 @@ impl<'a> CandidateRouteHop<'a> {
        }
        /// Returns the target node id of this hop, if known.
        ///
-       /// Target node id refers to the hop receiving the payment.
+       /// Target node id refers to the node receiving the HTLC after this hop.
+       ///
+       /// For [`Self::Blinded`] we return `None` because the ultimate destination after the blinded
+       /// path is unknown.
        ///
-       /// For `Blinded` and `OneHopBlinded` we return `None` because next hop is blinded.
-       pub fn target(&self) -> Option<NodeId> {
+       /// For [`Self::OneHopBlinded`] we return `None` because the target is the same as the source,
+       /// and such a return value would be somewhat nonsensical.
+       #[inline]
+       pub fn target(&self) -> Option<NodeId> {
                match self {
                        CandidateRouteHop::FirstHop { details, .. } => Some(details.counterparty.node_id.into()),
                        CandidateRouteHop::PublicHop { info, .. } => Some(*info.target()),
-                       CandidateRouteHop::PrivateHop { target_node_id, .. } => Some(*target_node_id),
+                       CandidateRouteHop::PrivateHop { target_node_id, .. } => Some(**target_node_id),
                        CandidateRouteHop::Blinded { .. } => None,
                        CandidateRouteHop::OneHopBlinded { .. } => None,
                }
        }
 }
 
-/// 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`].
@@ -1246,15 +1312,15 @@ fn iter_equal<I1: Iterator, I2: Iterator>(mut iter_a: I1, mut iter_b: I2)
 /// Fee values should be updated only in the context of the whole path, see update_value_and_recompute_fees.
 /// These fee values are useful to choose hops as we traverse the graph "payee-to-payer".
 #[derive(Clone)]
+#[repr(C)] // Force fields to appear in the order we define them.
 struct PathBuildingHop<'a> {
        candidate: CandidateRouteHop<'a>,
-       fee_msat: u64,
-
-       /// All the fees paid *after* this channel on the way to the destination
-       next_hops_fee_msat: u64,
-       /// Fee paid for the use of the current channel (see candidate.fees()).
-       /// The value will be actually deducted from the counterparty balance on the previous link.
-       hop_use_fee_msat: u64,
+       /// If we've already processed a node as the best node, we shouldn't process it again. Normally
+       /// we'd just ignore it if we did as all channels would have a higher new fee, but because we
+       /// may decrease the amounts in use as we walk the graph, the actual calculated fee may
+       /// decrease as well. Thus, we have to explicitly track which nodes have been processed and
+       /// avoid processing them again.
+       was_processed: bool,
        /// Used to compare channels when choosing the for routing.
        /// Includes paying for the use of a hop and the following hops, as well as
        /// an estimated cost of reaching this hop.
@@ -1266,12 +1332,20 @@ struct PathBuildingHop<'a> {
        /// All penalties incurred from this channel on the way to the destination, as calculated using
        /// channel scoring.
        path_penalty_msat: u64,
-       /// If we've already processed a node as the best node, we shouldn't process it again. Normally
-       /// we'd just ignore it if we did as all channels would have a higher new fee, but because we
-       /// may decrease the amounts in use as we walk the graph, the actual calculated fee may
-       /// decrease as well. Thus, we have to explicitly track which nodes have been processed and
-       /// avoid processing them again.
-       was_processed: bool,
+
+       // The last 16 bytes are on the next cache line by default in glibc's malloc. Thus, we should
+       // only place fields which are not hot there. Luckily, the next three fields are only read if
+       // we end up on the selected path, and only in the final path layout phase, so we don't care
+       // too much if reading them is slow.
+
+       fee_msat: u64,
+
+       /// All the fees paid *after* this channel on the way to the destination
+       next_hops_fee_msat: u64,
+       /// Fee paid for the use of the current channel (see candidate.fees()).
+       /// The value will be actually deducted from the counterparty balance on the previous link.
+       hop_use_fee_msat: u64,
+
        #[cfg(all(not(ldk_bench), any(test, fuzzing)))]
        // In tests, we apply further sanity checks on cases where we skip nodes we already processed
        // to ensure it is specifically in cases where the fee has gone down because of a decrease in
@@ -1280,6 +1354,18 @@ struct PathBuildingHop<'a> {
        value_contribution_msat: u64,
 }
 
+// Checks that the entries in the `find_route` `dist` map fit in (exactly) two standard x86-64
+// cache lines. Sadly, they're not guaranteed to actually lie on a cache line (and in fact,
+// generally won't, because at least glibc's malloc will align to a nice, big, round
+// boundary...plus 16), but at least it will reduce the amount of data we'll need to load.
+//
+// Note that these assertions only pass on somewhat recent rustc, and thus are gated on the
+// ldk_bench flag.
+#[cfg(ldk_bench)]
+const _NODE_MAP_SIZE_TWO_CACHE_LINES: usize = 128 - core::mem::size_of::<(NodeId, PathBuildingHop)>();
+#[cfg(ldk_bench)]
+const _NODE_MAP_SIZE_EXACTLY_CACHE_LINES: usize = core::mem::size_of::<(NodeId, PathBuildingHop)>() - 128;
+
 impl<'a> core::fmt::Debug for PathBuildingHop<'a> {
        fn fmt(&self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> {
                let mut debug_struct = f.debug_struct("PathBuildingHop");
@@ -1734,6 +1820,20 @@ where L::Target: Logger {
                }
        }
 
+       let mut private_hop_key_cache = HashMap::with_capacity(
+               payment_params.payee.unblinded_route_hints().iter().map(|path| path.0.len()).sum()
+       );
+
+       // Because we store references to private hop node_ids in `dist`, below, we need them to exist
+       // (as `NodeId`, not `PublicKey`) for the lifetime of `dist`. Thus, we calculate all the keys
+       // we'll need here and simply fetch them when routing.
+       private_hop_key_cache.insert(maybe_dummy_payee_pk, NodeId::from_pubkey(&maybe_dummy_payee_pk));
+       for route in payment_params.payee.unblinded_route_hints().iter() {
+               for hop in route.0.iter() {
+                       private_hop_key_cache.insert(hop.src_node_id, NodeId::from_pubkey(&hop.src_node_id));
+               }
+       }
+
        // The main heap containing all candidate next-hops sorted by their score (max(fee,
        // htlc_minimum)). Ideally this would be a heap which allowed cheap score reduction instead of
        // adding duplicate entries when we find a better path to a given node.
@@ -2021,15 +2121,6 @@ where L::Target: Logger {
                                                                                score_params);
                                                                let path_penalty_msat = $next_hops_path_penalty_msat
                                                                        .saturating_add(channel_penalty_msat);
-                                                               let new_graph_node = RouteGraphNode {
-                                                                       node_id: src_node_id,
-                                                                       lowest_fee_to_node: total_fee_msat,
-                                                                       total_cltv_delta: hop_total_cltv_delta,
-                                                                       value_contribution_msat,
-                                                                       path_htlc_minimum_msat,
-                                                                       path_penalty_msat,
-                                                                       path_length_to_node,
-                                                               };
 
                                                                // Update the way of reaching $candidate.source()
                                                                // with the given short_channel_id (from $candidate.target()),
@@ -2054,6 +2145,13 @@ where L::Target: Logger {
                                                                        .saturating_add(path_penalty_msat);
 
                                                                if !old_entry.was_processed && new_cost < old_cost {
+                                                                       let new_graph_node = RouteGraphNode {
+                                                                               node_id: src_node_id,
+                                                                               score: cmp::max(total_fee_msat, path_htlc_minimum_msat).saturating_add(path_penalty_msat),
+                                                                               total_cltv_delta: hop_total_cltv_delta,
+                                                                               value_contribution_msat,
+                                                                               path_length_to_node,
+                                                                       };
                                                                        targets.push(new_graph_node);
                                                                        old_entry.next_hops_fee_msat = $next_hops_fee_msat;
                                                                        old_entry.hop_use_fee_msat = hop_use_fee_msat;
@@ -2127,28 +2225,38 @@ where L::Target: Logger {
        // meaning how much will be paid in fees after this node (to the best of our knowledge).
        // This data can later be helpful to optimize routing (pay lower fees).
        macro_rules! add_entries_to_cheapest_to_target_node {
-               ( $node: expr, $node_id: expr, $fee_to_target_msat: expr, $next_hops_value_contribution: expr,
-                 $next_hops_path_htlc_minimum_msat: expr, $next_hops_path_penalty_msat: expr,
+               ( $node: expr, $node_id: expr, $next_hops_value_contribution: expr,
                  $next_hops_cltv_delta: expr, $next_hops_path_length: expr ) => {
+                       let fee_to_target_msat;
+                       let next_hops_path_htlc_minimum_msat;
+                       let next_hops_path_penalty_msat;
                        let skip_node = if let Some(elem) = dist.get_mut(&$node_id) {
                                let was_processed = elem.was_processed;
                                elem.was_processed = true;
+                               fee_to_target_msat = elem.total_fee_msat;
+                               next_hops_path_htlc_minimum_msat = elem.path_htlc_minimum_msat;
+                               next_hops_path_penalty_msat = elem.path_penalty_msat;
                                was_processed
                        } else {
                                // Entries are added to dist in add_entry!() when there is a channel from a node.
                                // Because there are no channels from payee, it will not have a dist entry at this point.
                                // If we're processing any other node, it is always be the result of a channel from it.
                                debug_assert_eq!($node_id, maybe_dummy_payee_node_id);
+                               fee_to_target_msat = 0;
+                               next_hops_path_htlc_minimum_msat = 0;
+                               next_hops_path_penalty_msat = 0;
                                false
                        };
 
                        if !skip_node {
                                if let Some(first_channels) = first_hop_targets.get(&$node_id) {
                                        for details in first_channels {
-                                               let candidate = CandidateRouteHop::FirstHop { details, node_id: our_node_id };
-                                               add_entry!(&candidate, $fee_to_target_msat,
+                                               let candidate = CandidateRouteHop::FirstHop {
+                                                       details, payer_node_id: &our_node_id,
+                                               };
+                                               add_entry!(&candidate, fee_to_target_msat,
                                                        $next_hops_value_contribution,
-                                                       $next_hops_path_htlc_minimum_msat, $next_hops_path_penalty_msat,
+                                                       next_hops_path_htlc_minimum_msat, next_hops_path_penalty_msat,
                                                        $next_hops_cltv_delta, $next_hops_path_length);
                                        }
                                }
@@ -2171,10 +2279,10 @@ where L::Target: Logger {
                                                                                        short_channel_id: *chan_id,
                                                                                };
                                                                                add_entry!(&candidate,
-                                                                                       $fee_to_target_msat,
+                                                                                       fee_to_target_msat,
                                                                                        $next_hops_value_contribution,
-                                                                                       $next_hops_path_htlc_minimum_msat,
-                                                                                       $next_hops_path_penalty_msat,
+                                                                                       next_hops_path_htlc_minimum_msat,
+                                                                                       next_hops_path_penalty_msat,
                                                                                        $next_hops_cltv_delta, $next_hops_path_length);
                                                                        }
                                                                }
@@ -2200,7 +2308,9 @@ where L::Target: Logger {
                // place where it could be added.
                payee_node_id_opt.map(|payee| first_hop_targets.get(&payee).map(|first_channels| {
                        for details in first_channels {
-                               let candidate = CandidateRouteHop::FirstHop { details, node_id: our_node_id };
+                               let candidate = CandidateRouteHop::FirstHop {
+                                       details, payer_node_id: &our_node_id,
+                               };
                                let added = add_entry!(&candidate, 0, path_value_msat,
                                                                        0, 0u64, 0, 0).is_some();
                                log_trace!(logger, "{} direct route to payee via {}",
@@ -2217,7 +2327,7 @@ where L::Target: Logger {
                        // If not, targets.pop() will not even let us enter the loop in step 2.
                        None => {},
                        Some(node) => {
-                               add_entries_to_cheapest_to_target_node!(node, payee, 0, path_value_msat, 0, 0u64, 0, 0);
+                               add_entries_to_cheapest_to_target_node!(node, payee, path_value_msat, 0, 0);
                        },
                });
 
@@ -2247,7 +2357,9 @@ where L::Target: Logger {
                                sort_first_hop_channels(first_channels, &used_liquidities, recommended_value_msat,
                                        our_node_pubkey);
                                for details in first_channels {
-                                       let first_hop_candidate = CandidateRouteHop::FirstHop { details, node_id: our_node_id};
+                                       let first_hop_candidate = CandidateRouteHop::FirstHop {
+                                               details, payer_node_id: &our_node_id,
+                                       };
                                        let blinded_path_fee = match compute_fees(path_contribution_msat, candidate.fees()) {
                                                Some(fee) => fee,
                                                None => continue
@@ -2286,8 +2398,7 @@ where L::Target: Logger {
                                let mut aggregate_path_contribution_msat = path_value_msat;
 
                                for (idx, (hop, prev_hop_id)) in hop_iter.zip(prev_hop_iter).enumerate() {
-                                       let source = NodeId::from_pubkey(&hop.src_node_id);
-                                       let target = NodeId::from_pubkey(&prev_hop_id);
+                                       let target = private_hop_key_cache.get(&prev_hop_id).unwrap();
 
                                        if let Some(first_channels) = first_hop_targets.get(&target) {
                                                if first_channels.iter().any(|d| d.outbound_scid_alias == Some(hop.short_channel_id)) {
@@ -2344,7 +2455,9 @@ where L::Target: Logger {
                                                sort_first_hop_channels(first_channels, &used_liquidities,
                                                        recommended_value_msat, our_node_pubkey);
                                                for details in first_channels {
-                                                       let first_hop_candidate = CandidateRouteHop::FirstHop { details, node_id: our_node_id};
+                                                       let first_hop_candidate = CandidateRouteHop::FirstHop {
+                                                               details, payer_node_id: &our_node_id,
+                                                       };
                                                        add_entry!(&first_hop_candidate,
                                                                aggregate_next_hops_fee_msat, aggregate_path_contribution_msat,
                                                                aggregate_next_hops_path_htlc_minimum_msat, aggregate_next_hops_path_penalty_msat,
@@ -2389,7 +2502,9 @@ where L::Target: Logger {
                                                        sort_first_hop_channels(first_channels, &used_liquidities,
                                                                recommended_value_msat, our_node_pubkey);
                                                        for details in first_channels {
-                                                               let first_hop_candidate = CandidateRouteHop::FirstHop { details, node_id: our_node_id};
+                                                               let first_hop_candidate = CandidateRouteHop::FirstHop {
+                                                                       details, payer_node_id: &our_node_id,
+                                                               };
                                                                add_entry!(&first_hop_candidate,
                                                                        aggregate_next_hops_fee_msat,
                                                                        aggregate_path_contribution_msat,
@@ -2419,7 +2534,7 @@ where L::Target: Logger {
                // Both these cases (and other cases except reaching recommended_value_msat) mean that
                // paths_collection will be stopped because found_new_path==false.
                // This is not necessarily a routing failure.
-               'path_construction: while let Some(RouteGraphNode { node_id, lowest_fee_to_node, total_cltv_delta, mut value_contribution_msat, path_htlc_minimum_msat, path_penalty_msat, path_length_to_node, .. }) = targets.pop() {
+               'path_construction: while let Some(RouteGraphNode { node_id, total_cltv_delta, mut value_contribution_msat, path_length_to_node, .. }) = targets.pop() {
 
                        // Since we're going payee-to-payer, hitting our node as a target means we should stop
                        // traversing the graph and arrange the path out of what we found.
@@ -2432,8 +2547,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;
@@ -2552,8 +2669,8 @@ where L::Target: Logger {
                        match network_nodes.get(&node_id) {
                                None => {},
                                Some(node) => {
-                                       add_entries_to_cheapest_to_target_node!(node, node_id, lowest_fee_to_node,
-                                               value_contribution_msat, path_htlc_minimum_msat, path_penalty_msat,
+                                       add_entries_to_cheapest_to_target_node!(node, node_id,
+                                               value_contribution_msat,
                                                total_cltv_delta, path_length_to_node);
                                },
                        }
@@ -2682,8 +2799,8 @@ where L::Target: Logger {
        });
        for idx in 0..(selected_route.len() - 1) {
                if idx + 1 >= selected_route.len() { break; }
-               if iter_equal(selected_route[idx].hops.iter().map(|h| (h.0.candidate.id(), h.0.candidate.target())),
-                                                                       selected_route[idx + 1].hops.iter().map(|h| (h.0.candidate.id(), h.0.candidate.target()))) {
+               if iter_equal(selected_route[idx    ].hops.iter().map(|h| (h.0.candidate.id(), h.0.candidate.target())),
+                             selected_route[idx + 1].hops.iter().map(|h| (h.0.candidate.id(), h.0.candidate.target()))) {
                        let new_value = selected_route[idx].get_value_msat() + selected_route[idx + 1].get_value_msat();
                        selected_route[idx].update_value_and_recompute_fees(new_value);
                        selected_route.remove(idx + 1);
index 5d637a859903bfa34f849328ac366517b541394b..337fd8eec7a895ce5198edb3a9774267048de03d 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) {
@@ -2244,10 +2242,6 @@ mod tests {
                PublicKey::from_secret_key(&secp_ctx, &recipient_privkey())
        }
 
-       fn sender_node_id() -> NodeId {
-               NodeId::from_pubkey(&sender_pubkey())
-       }
-
        fn recipient_node_id() -> NodeId {
                NodeId::from_pubkey(&recipient_pubkey())
        }
@@ -2902,7 +2896,7 @@ mod tests {
                        effective_capacity: EffectiveCapacity::Total { capacity_msat: 1_024, htlc_maximum_msat: 1_024 },
                };
                let channel = network_graph.read_only().channel(42).unwrap().to_owned();
-               let (info, target) = channel.as_directed_from(&source).unwrap();
+               let (info, _) = channel.as_directed_from(&source).unwrap();
                let candidate = CandidateRouteHop::PublicHop {
                        info,
                        short_channel_id: 42,
@@ -3002,7 +2996,7 @@ mod tests {
                        effective_capacity: EffectiveCapacity::Total { capacity_msat: 1_024, htlc_maximum_msat: 1_000 },
                };
                let channel = network_graph.read_only().channel(42).unwrap().to_owned();
-               let (info, target) = channel.as_directed_from(&source).unwrap();
+               let (info, _) = channel.as_directed_from(&source).unwrap();
                let candidate = CandidateRouteHop::PublicHop {
                        info,
                        short_channel_id: 42,
@@ -3388,6 +3382,7 @@ mod tests {
                assert_eq!(scorer.channel_penalty_msat(&candidate, usage, &params), u64::max_value());
        }
 
+       #[test]
        fn remembers_historical_failures() {
                let logger = TestLogger::new();
                let network_graph = network_graph(&logger);
@@ -3402,6 +3397,7 @@ mod tests {
                };
                let mut scorer = ProbabilisticScorer::new(decay_params, &network_graph, &logger);
                let source = source_node_id();
+               let target = target_node_id();
 
                let usage = ChannelUsage {
                        amount_msat: 100,
@@ -3413,24 +3409,37 @@ mod tests {
                        inflight_htlc_msat: 0,
                        effective_capacity: EffectiveCapacity::Total { capacity_msat: 1_024, htlc_maximum_msat: 1_024 },
                };
-               let network_graph = network_graph.read_only();
-               let channel = network_graph.channel(42).unwrap();
-               let (info, target) = channel.as_directed_from(&source).unwrap();
-               let candidate = CandidateRouteHop::PublicHop {
-                       info,
-                       short_channel_id: 42,
-               };
 
-               // With no historical data the normal liquidity penalty calculation is used.
-               assert_eq!(scorer.channel_penalty_msat(&candidate, usage, &params), 168);
+               {
+                       let network_graph = network_graph.read_only();
+                       let channel = network_graph.channel(42).unwrap();
+                       let (info, _) = channel.as_directed_from(&source).unwrap();
+                       let candidate = CandidateRouteHop::PublicHop {
+                               info,
+                               short_channel_id: 42,
+                       };
+
+                       // With no historical data the normal liquidity penalty calculation is used.
+                       assert_eq!(scorer.channel_penalty_msat(&candidate, usage, &params), 168);
+               }
                assert_eq!(scorer.historical_estimated_channel_liquidity_probabilities(42, &target),
                None);
                assert_eq!(scorer.historical_estimated_payment_success_probability(42, &target, 42, &params),
                None);
 
                scorer.payment_path_failed(&payment_path_for_amount(1), 42);
-               assert_eq!(scorer.channel_penalty_msat(&candidate, usage, &params), 2048);
-               assert_eq!(scorer.channel_penalty_msat(&candidate, usage_1, &params), 249);
+               {
+                       let network_graph = network_graph.read_only();
+                       let channel = network_graph.channel(42).unwrap();
+                       let (info, _) = channel.as_directed_from(&source).unwrap();
+                       let candidate = CandidateRouteHop::PublicHop {
+                               info,
+                               short_channel_id: 42,
+                       };
+
+                       assert_eq!(scorer.channel_penalty_msat(&candidate, usage, &params), 2048);
+                       assert_eq!(scorer.channel_penalty_msat(&candidate, usage_1, &params), 249);
+               }
                // The "it failed" increment is 32, where the probability should lie several buckets into
                // the first octile.
                assert_eq!(scorer.historical_estimated_channel_liquidity_probabilities(42, &target),
@@ -3444,7 +3453,17 @@ mod tests {
                // Even after we tell the scorer we definitely have enough available liquidity, it will
                // still remember that there was some failure in the past, and assign a non-0 penalty.
                scorer.payment_path_failed(&payment_path_for_amount(1000), 43);
-               assert_eq!(scorer.channel_penalty_msat(&candidate, usage, &params), 105);
+               {
+                       let network_graph = network_graph.read_only();
+                       let channel = network_graph.channel(42).unwrap();
+                       let (info, _) = channel.as_directed_from(&source).unwrap();
+                       let candidate = CandidateRouteHop::PublicHop {
+                               info,
+                               short_channel_id: 42,
+                       };
+
+                       assert_eq!(scorer.channel_penalty_msat(&candidate, usage, &params), 105);
+               }
                // The first points should be decayed just slightly and the last bucket has a new point.
                assert_eq!(scorer.historical_estimated_channel_liquidity_probabilities(42, &target),
                        Some(([31, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 32, 0, 0, 0, 0, 0],
@@ -3464,7 +3483,17 @@ mod tests {
                // Advance the time forward 16 half-lives (which the docs claim will ensure all data is
                // gone), and check that we're back to where we started.
                SinceEpoch::advance(Duration::from_secs(10 * 16));
-               assert_eq!(scorer.channel_penalty_msat(&candidate, usage, &params), 168);
+               {
+                       let network_graph = network_graph.read_only();
+                       let channel = network_graph.channel(42).unwrap();
+                       let (info, _) = channel.as_directed_from(&source).unwrap();
+                       let candidate = CandidateRouteHop::PublicHop {
+                               info,
+                               short_channel_id: 42,
+                       };
+
+                       assert_eq!(scorer.channel_penalty_msat(&candidate, usage, &params), 168);
+               }
                // Once fully decayed we still have data, but its all-0s. In the future we may remove the
                // data entirely instead.
                assert_eq!(scorer.historical_estimated_channel_liquidity_probabilities(42, &target),
@@ -3477,16 +3506,26 @@ mod tests {
                        effective_capacity: EffectiveCapacity::Total { capacity_msat: 1_024, htlc_maximum_msat: 1_024 },
                };
                scorer.payment_path_failed(&payment_path_for_amount(1), 42);
-               assert_eq!(scorer.channel_penalty_msat(&candidate, usage, &params), 2050);
-               usage.inflight_htlc_msat = 0;
-               assert_eq!(scorer.channel_penalty_msat(&candidate, usage, &params), 866);
+               {
+                       let network_graph = network_graph.read_only();
+                       let channel = network_graph.channel(42).unwrap();
+                       let (info, _) = channel.as_directed_from(&source).unwrap();
+                       let candidate = CandidateRouteHop::PublicHop {
+                               info,
+                               short_channel_id: 42,
+                       };
 
-               let usage = ChannelUsage {
-                       amount_msat: 1,
-                       inflight_htlc_msat: 0,
-                       effective_capacity: EffectiveCapacity::AdvertisedMaxHTLC { amount_msat: 0 },
-               };
-               assert_eq!(scorer.channel_penalty_msat(&candidate, usage, &params), 2048);
+                       assert_eq!(scorer.channel_penalty_msat(&candidate, usage, &params), 2050);
+                       usage.inflight_htlc_msat = 0;
+                       assert_eq!(scorer.channel_penalty_msat(&candidate, usage, &params), 866);
+
+                       let usage = ChannelUsage {
+                               amount_msat: 1,
+                               inflight_htlc_msat: 0,
+                               effective_capacity: EffectiveCapacity::AdvertisedMaxHTLC { amount_msat: 0 },
+                       };
+                       assert_eq!(scorer.channel_penalty_msat(&candidate, usage, &params), 2048);
+               }
 
                // Advance to decay all liquidity offsets to zero.
                SinceEpoch::advance(Duration::from_secs(60 * 60 * 10));
index 9bbbec0d78e6a182e6e8e2553e60642db9645f52..664deb9fa1566f6aa2628150458385673fd16cc8 100644 (file)
@@ -30,9 +30,9 @@ use crate::ln::msgs::LightningError;
 use crate::ln::script::ShutdownScript;
 use crate::offers::invoice::UnsignedBolt12Invoice;
 use crate::offers::invoice_request::UnsignedInvoiceRequest;
-use crate::routing::gossip::{EffectiveCapacity, NetworkGraph, NodeId};
+use crate::routing::gossip::{EffectiveCapacity, NetworkGraph, NodeId, RoutingFees};
 use crate::routing::utxo::{UtxoLookup, UtxoLookupError, UtxoResult};
-use crate::routing::router::{find_route, InFlightHtlcs, Path, Route, RouteParameters, Router, ScorerAccountingForInFlightHtlcs};
+use crate::routing::router::{find_route, InFlightHtlcs, Path, Route, RouteParameters, RouteHintHop, Router, ScorerAccountingForInFlightHtlcs};
 use crate::routing::scoring::{ChannelUsage, ScoreUpdate, ScoreLookUp};
 use crate::sync::RwLock;
 use crate::util::config::UserConfig;
@@ -129,6 +129,7 @@ impl<'a> Router for TestRouter<'a> {
                                let scorer = ScorerAccountingForInFlightHtlcs::new(scorer, &inflight_htlcs);
                                for path in &route.paths {
                                        let mut aggregate_msat = 0u64;
+                                       let mut prev_hop_node = payer;
                                        for (idx, hop) in path.hops.iter().rev().enumerate() {
                                                aggregate_msat += hop.fee_msat;
                                                let usage = ChannelUsage {
@@ -137,38 +138,44 @@ impl<'a> Router for TestRouter<'a> {
                                                        effective_capacity: EffectiveCapacity::Unknown,
                                                };
 
-                                               // Since the path is reversed, the last element in our iteration is the first
-                                               // hop.
                                                if idx == path.hops.len() - 1 {
-                                                       let first_hops = match first_hops {
-                                                               Some(hops) => hops,
-                                                               None => continue,
-                                                       };
-                                                       if first_hops.len() == 0 {
-                                                               continue;
+                                                       if let Some(first_hops) = first_hops {
+                                                               if let Some(idx) = first_hops.iter().position(|h| h.get_outbound_payment_scid() == Some(hop.short_channel_id)) {
+                                                                       let node_id = NodeId::from_pubkey(payer);
+                                                                       let candidate = CandidateRouteHop::FirstHop {
+                                                                               details: first_hops[idx],
+                                                                               payer_node_id: &node_id,
+                                                                       };
+                                                                       scorer.channel_penalty_msat(&candidate, usage, &());
+                                                                       continue;
+                                                               }
                                                        }
-                                                       let idx = if first_hops.len() > 1 { route.paths.iter().position(|p| p == path).unwrap_or(0) } else { 0 };
-                                                       let candidate = CandidateRouteHop::FirstHop {
-                                                               details: first_hops[idx],
-                                                               node_id: NodeId::from_pubkey(payer)
+                                               }
+                                               let network_graph = self.network_graph.read_only();
+                                               if let Some(channel) = network_graph.channel(hop.short_channel_id) {
+                                                       let (directed, _) = channel.as_directed_to(&NodeId::from_pubkey(&hop.pubkey)).unwrap();
+                                                       let candidate = CandidateRouteHop::PublicHop {
+                                                               info: directed,
+                                                               short_channel_id: hop.short_channel_id,
                                                        };
                                                        scorer.channel_penalty_msat(&candidate, usage, &());
                                                } else {
-                                                       let network_graph = self.network_graph.read_only();
-                                                       let channel = match network_graph.channel(hop.short_channel_id) {
-                                                               Some(channel) => channel,
-                                                               None => continue,
-                                                       };
-                                                       let channel = match channel.as_directed_to(&NodeId::from_pubkey(&hop.pubkey)) {
-                                                               Some(channel) => channel,
-                                                               None => panic!("Channel directed to {} was not found", hop.pubkey),
-                                                       };
-                                                       let candidate = CandidateRouteHop::PublicHop {
-                                                               info: channel.0,
+                                                       let target_node_id = NodeId::from_pubkey(&hop.pubkey);
+                                                       let route_hint = RouteHintHop {
+                                                               src_node_id: *prev_hop_node,
                                                                short_channel_id: hop.short_channel_id,
+                                                               fees: RoutingFees { base_msat: 0, proportional_millionths: 0 },
+                                                               cltv_expiry_delta: 0,
+                                                               htlc_minimum_msat: None,
+                                                               htlc_maximum_msat: None,
+                                                       };
+                                                       let candidate = CandidateRouteHop::PrivateHop {
+                                                               hint: &route_hint,
+                                                               target_node_id: &target_node_id,
                                                        };
                                                        scorer.channel_penalty_msat(&candidate, usage, &());
                                                }
+                                               prev_hop_node = &hop.pubkey;
                                        }
                                }
                        }
@@ -1324,7 +1331,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,
                };