From 9973331d8ef5a71a56f78c4d7788f54e2f201cad Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 6 Dec 2023 01:22:07 +0000 Subject: [PATCH 01/14] Rewrite docs in `CandidateRouteHop` to be somewhat more descriptive --- lightning/src/routing/router.rs | 92 +++++++++++++++++++-------------- 1 file changed, 53 insertions(+), 39 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index b4cbf3cb234..6300763e2a2 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -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 @@ -1009,63 +1013,73 @@ 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. + node_id: 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. + /// Node id of the next hop in BOLT 11 route hint. target_node_id: 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 build a [`CandidateHopId`] that uniquely identifies 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 build a [`CandidateHopId`] that uniquely identifies this blinded path, + /// even though we don't have a short channel ID for this hop. hint_idx: usize, }, } From 98ed285b9cabf19097665aef0e9d8d1688ba5b39 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 6 Dec 2023 17:12:28 +0000 Subject: [PATCH 02/14] Rename `DirectedChannelInfo::outbound` to `from_node_one` ...to give a bit more readability on accessing sites. --- lightning/src/routing/gossip.rs | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index edb37261697..97406289179 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -992,19 +992,14 @@ pub struct DirectedChannelInfo<'a> { 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 { + fn new(channel: &'a ChannelInfo, direction: &'a ChannelUpdateInfo, from_node_one: bool) -> Self { let mut htlc_maximum_msat = direction.htlc_maximum_msat; let capacity_msat = channel.capacity_sats.map(|capacity_sats| capacity_sats * 1000); @@ -1017,7 +1012,7 @@ impl<'a> DirectedChannelInfo<'a> { }; Self { - channel, direction, htlc_maximum_msat, effective_capacity, outbound + channel, direction, htlc_maximum_msat, effective_capacity, from_node_one, } } @@ -1047,12 +1042,12 @@ 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 } } + 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 } } + 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> { From 2caccc5cd6e9a64c115d86f1109415ecaa23bc06 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 6 Dec 2023 17:48:51 +0000 Subject: [PATCH 03/14] Fix and re-enable the `remembers_historical_failures` test f0ecc3ec73dcdb9303b1bd5ac687a361decce2dd introduced a regression in the `remembers_historical_failures` test, and disabled it by simply removing the `#[test]` annotation. This fixes the test and marks it as a test again. --- lightning/src/routing/scoring.rs | 89 ++++++++++++++++++++++++-------- 1 file changed, 67 insertions(+), 22 deletions(-) diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs index 5d637a85990..151ac41e990 100644 --- a/lightning/src/routing/scoring.rs +++ b/lightning/src/routing/scoring.rs @@ -3388,6 +3388,7 @@ mod tests { assert_eq!(scorer.channel_penalty_msat(&candidate, usage, ¶ms), u64::max_value()); } + #[test] fn remembers_historical_failures() { let logger = TestLogger::new(); let network_graph = network_graph(&logger); @@ -3402,6 +3403,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 +3415,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, ¶ms), 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, ¶ms), 168); + } assert_eq!(scorer.historical_estimated_channel_liquidity_probabilities(42, &target), None); assert_eq!(scorer.historical_estimated_payment_success_probability(42, &target, 42, ¶ms), None); scorer.payment_path_failed(&payment_path_for_amount(1), 42); - assert_eq!(scorer.channel_penalty_msat(&candidate, usage, ¶ms), 2048); - assert_eq!(scorer.channel_penalty_msat(&candidate, usage_1, ¶ms), 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, ¶ms), 2048); + assert_eq!(scorer.channel_penalty_msat(&candidate, usage_1, ¶ms), 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 +3459,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, ¶ms), 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, ¶ms), 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 +3489,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, ¶ms), 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, ¶ms), 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 +3512,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, ¶ms), 2050); - usage.inflight_htlc_msat = 0; - assert_eq!(scorer.channel_penalty_msat(&candidate, usage, ¶ms), 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, ¶ms), 2048); + assert_eq!(scorer.channel_penalty_msat(&candidate, usage, ¶ms), 2050); + usage.inflight_htlc_msat = 0; + assert_eq!(scorer.channel_penalty_msat(&candidate, usage, ¶ms), 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, ¶ms), 2048); + } // Advance to decay all liquidity offsets to zero. SinceEpoch::advance(Duration::from_secs(60 * 60 * 10)); From 99e4a1fbb63ce05c9923f5645c05d9faf2387238 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 6 Dec 2023 01:13:33 +0000 Subject: [PATCH 04/14] 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 6300763e2a2..ff2dfe1b25e 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 151ac41e990..80438de67bf 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 9bbbec0d78e..125ba812e78 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, }; From fc44e84ca8a6a5bc70e3808e9657d90e20fa3c57 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 6 Dec 2023 17:17:27 +0000 Subject: [PATCH 05/14] Fix new unused warnings in `scoring.rs` --- lightning/src/routing/scoring.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs index 80438de67bf..337fd8eec7a 100644 --- a/lightning/src/routing/scoring.rs +++ b/lightning/src/routing/scoring.rs @@ -2242,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()) } @@ -2900,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, @@ -3000,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, From 57857fd52045abe0988d2d3efe55341be2fe33d5 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 6 Dec 2023 01:17:48 +0000 Subject: [PATCH 06/14] `#[inline]` `CandidateRouteHop` accessors These are used in the performance-critical routing and scoring operations, which may happen outside of our crate. Thus, we really need to allow downstream crates to inline these accessors into their code, which we do here. --- lightning/src/routing/router.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index ff2dfe1b25e..6138b87bc9a 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1100,6 +1100,7 @@ impl<'a> CandidateRouteHop<'a> { /// /// 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 { match self { CandidateRouteHop::FirstHop { details, .. } => details.get_outbound_payment_scid(), @@ -1115,6 +1116,7 @@ impl<'a> CandidateRouteHop<'a> { /// 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 { match self { CandidateRouteHop::FirstHop { details, .. } => if details.is_public { details.short_channel_id } else { None }, @@ -1137,6 +1139,7 @@ impl<'a> CandidateRouteHop<'a> { } /// Returns cltv_expiry_delta for this hop. + #[inline] pub fn cltv_expiry_delta(&self) -> u32 { match self { CandidateRouteHop::FirstHop { .. } => 0, @@ -1148,6 +1151,7 @@ impl<'a> CandidateRouteHop<'a> { } /// Returns the htlc_minimum_msat for this hop. + #[inline] pub fn htlc_minimum_msat(&self) -> u64 { match self { CandidateRouteHop::FirstHop { details, .. } => details.next_outbound_htlc_minimum_msat, @@ -1159,6 +1163,7 @@ impl<'a> CandidateRouteHop<'a> { } /// Returns the fees for this hop. + #[inline] pub fn fees(&self) -> RoutingFees { match self { CandidateRouteHop::FirstHop { .. } => RoutingFees { @@ -1196,6 +1201,7 @@ impl<'a> CandidateRouteHop<'a> { /// 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), @@ -1216,6 +1222,7 @@ impl<'a> CandidateRouteHop<'a> { /// Source node id refers to the hop forwarding the payment. /// /// For `FirstHop` we return payer's node id. + #[inline] pub fn source(&self) -> NodeId { match self { CandidateRouteHop::FirstHop { node_id, .. } => *node_id, @@ -1230,7 +1237,8 @@ impl<'a> CandidateRouteHop<'a> { /// Target node id refers to the hop receiving the payment. /// /// For `Blinded` and `OneHopBlinded` we return `None` because next hop is blinded. - pub fn target(&self) -> Option { + #[inline] + pub fn target(&self) -> Option { match self { CandidateRouteHop::FirstHop { details, .. } => Some(details.counterparty.node_id.into()), CandidateRouteHop::PublicHop { info, .. } => Some(*info.target()), From cc4bc1df5a1ab6c363fac3c1b7eddce362e167c0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 6 Dec 2023 01:22:21 +0000 Subject: [PATCH 07/14] Rename `CandidateRouteHop::FirstHop::node_id` and make it a ref Rather than calling `CandidateRouteHop::FirstHop::node_id` just `node_id`, we should call it `payer_node_id` to provide more context. We also take this opportunity to make it a reference, avoiding bloating `CandidateRouteHop`. --- lightning/src/routing/router.rs | 24 +++++++++++++++++------- lightning/src/util/test_utils.rs | 3 ++- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 6138b87bc9a..227d3f6fd5d 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1020,7 +1020,7 @@ pub enum CandidateRouteHop<'a> { /// [`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. - node_id: NodeId, + payer_node_id: &'a NodeId, }, /// A hop found in the [`ReadOnlyNetworkGraph`]. PublicHop { @@ -1225,7 +1225,7 @@ impl<'a> CandidateRouteHop<'a> { #[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(), @@ -2198,7 +2198,9 @@ where L::Target: Logger { 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 }; + 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, @@ -2253,7 +2255,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 {}", @@ -2300,7 +2304,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 @@ -2397,7 +2403,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, @@ -2442,7 +2450,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, diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 125ba812e78..387f9df677e 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -148,9 +148,10 @@ impl<'a> Router for TestRouter<'a> { continue; } let idx = if first_hops.len() > 1 { route.paths.iter().position(|p| p == path).unwrap_or(0) } else { 0 }; + let node_id = NodeId::from_pubkey(payer); let candidate = CandidateRouteHop::FirstHop { details: first_hops[idx], - node_id: NodeId::from_pubkey(payer) + payer_node_id: &node_id, }; scorer.channel_penalty_msat(&candidate, usage, &()); } else { From e9bad1b95c77387d81508b958288b46c718b28ec Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 6 Dec 2023 01:19:17 +0000 Subject: [PATCH 08/14] Fix indentation in `router.rs` broken in a1d15ac1926f70aa5ab4f6686f --- lightning/src/routing/router.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 227d3f6fd5d..7dc3be1cb5b 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -2747,8 +2747,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); From 6ae051694d016ebd4c4b62ae63c92765b38ff069 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 6 Dec 2023 02:23:30 +0000 Subject: [PATCH 09/14] Make `CandidateRouteHop` method docs somewhat more descriptive --- lightning/src/routing/router.rs | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 7dc3be1cb5b..52678c42c8e 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1138,7 +1138,11 @@ 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 { @@ -1150,7 +1154,7 @@ 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 { @@ -1162,7 +1166,7 @@ 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 { @@ -1219,9 +1223,9 @@ 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 { @@ -1234,9 +1238,13 @@ 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. + /// 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 { match self { From 2b7d097dc76805a814eef93b4610a9eebc68e20e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 6 Dec 2023 17:47:00 +0000 Subject: [PATCH 10/14] Simplify and make scoring calls in `TestRouter` more complete `TestRouter` tries to make scoring calls that mimic what an actual router would do, but the changes in f0ecc3ec73dcdb9303b1bd5ac687a36 failed to make scoring calls for private hints or if we take a public hop for the last hop. This fixes those regressions, though no tests currently depend on this behavior. --- lightning/src/util/test_utils.rs | 58 ++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 26 deletions(-) diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 387f9df677e..1ae28b40e37 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -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,39 +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 node_id = NodeId::from_pubkey(payer); - let candidate = CandidateRouteHop::FirstHop { - details: first_hops[idx], - payer_node_id: &node_id, + } + 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; } } } From d0084c22d4d85b95ddf468953543e38e42c00fe0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 6 Dec 2023 03:54:28 +0000 Subject: [PATCH 11/14] Make `CandidateRouteHop::PrivateHop::target_node_id` a reference This avoids bloating `CandidateRouteHop` with a full 33-byte node_id (and avoids repeated public key serialization when we do multiple pathfinding passes). --- lightning/src/routing/router.rs | 21 +++++++++++++++++---- lightning/src/util/test_utils.rs | 2 +- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 52678c42c8e..7b1df46fb65 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1039,7 +1039,7 @@ pub enum CandidateRouteHop<'a> { /// Information about the private hop communicated via BOLT 11. hint: &'a RouteHintHop, /// Node id of the next hop in BOLT 11 route hint. - target_node_id: NodeId + target_node_id: &'a NodeId }, /// A blinded path which starts with an introduction point and ultimately terminates with the /// payee. @@ -1250,7 +1250,7 @@ impl<'a> CandidateRouteHop<'a> { 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, } @@ -1795,6 +1795,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. @@ -2353,8 +2367,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)) { diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 1ae28b40e37..664deb9fa15 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -171,7 +171,7 @@ impl<'a> Router for TestRouter<'a> { }; let candidate = CandidateRouteHop::PrivateHop { hint: &route_hint, - target_node_id: target_node_id, + target_node_id: &target_node_id, }; scorer.channel_penalty_msat(&candidate, usage, &()); } From e2f34cb12216f2b2f6292615bb41bad882eec742 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 6 Dec 2023 06:02:37 +0000 Subject: [PATCH 12/14] Make `find_route`'s `dist` map elements fit in 128 bytes We'd previously aggressively cached elements in the `PathBuildingHop` struct (and its sub-structs), which resulted in a rather bloated size. This implied cache misses as we read from and write to multiple cache lines during processing of a single channel. Here, we reduce caching in `DirectedChannelInfo`, fitting the `(NodeId, PathBuildingHop)` tuple in exactly 128 bytes. While this should fit in a single cache line, it sadly does not generally lie in only two lines, as glibc returns large buffers from `malloc` which are very well aligned, plus 16 bytes (for its own allocation tracking). Thus, we try to avoid reading from the last 16 bytes of a `PathBuildingHop`, but luckily that isn't super hard. Note that here we make accessing `DirectedChannelInfo::effective_capacity` somewhat slower, but that's okay as its only ever done once per `DirectedChannelInfo` anyway. While our routing benchmarks are quite noisy, this appears to result in between a 5% and 15% performance improvement in the probabilistic scoring benchmarks. --- lightning/src/routing/gossip.rs | 37 +++++++++++++-------------------- lightning/src/routing/router.rs | 16 ++++++++++++++ 2 files changed, 30 insertions(+), 23 deletions(-) diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index 97406289179..9b4e41ae174 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -990,8 +990,6 @@ impl Readable for ChannelInfo { pub struct DirectedChannelInfo<'a> { channel: &'a ChannelInfo, direction: &'a ChannelUpdateInfo, - htlc_maximum_msat: u64, - effective_capacity: EffectiveCapacity, /// 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, @@ -1000,39 +998,30 @@ pub struct DirectedChannelInfo<'a> { impl<'a> DirectedChannelInfo<'a> { #[inline] fn new(channel: &'a ChannelInfo, direction: &'a ChannelUpdateInfo, from_node_one: 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, from_node_one, - } + 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. @@ -1042,11 +1031,13 @@ impl<'a> DirectedChannelInfo<'a> { /// Returns the `node_id` of the source hop. /// /// Refers to the `node_id` forwarding the payment to the next hop. + #[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. + #[inline] pub(super) fn target(&self) -> &'a NodeId { if self.from_node_one { &self.channel.node_two } else { &self.channel.node_one } } } diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 7b1df46fb65..652c8cd32f2 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1186,6 +1186,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 { @@ -1341,6 +1345,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"); From 8ba3e83bb0a38b9fc8a3e492f70e6f78bd575d73 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 6 Dec 2023 05:02:07 +0000 Subject: [PATCH 13/14] Reorder `PathBuildingHop` fields somewhat Given `PathBuildingHop` is now an even multiple of cache lines, we can pick which fields "fall off" the cache line we have visible when dealing with hops, which we do here. --- lightning/src/routing/router.rs | 34 ++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 652c8cd32f2..e2ccd1f73ed 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1311,15 +1311,15 @@ fn iter_equal(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. @@ -1331,12 +1331,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 From 1171bc191349442eb9029ba3a6c8b0962a8455c3 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 6 Dec 2023 05:29:28 +0000 Subject: [PATCH 14/14] Pre-calculate heap element scores (retaining RouteGraphNode size) `RouteGraphNode` currently recalculates scores in its `Ord` implementation, wasting time while sorting the main Dijkstra's heap. Further, some time ago, when implementing the `htlc_maximum_msat` amount reduction while walking the graph, we added `PathBuildingHop::was_processed`, looking up the source node in `dist` each time we pop'ed an element off of the binary heap. As a result, we now have a reference to our `PathBuildingHop` when processing a best-node's channels, leading to several fields in `RouteGraphNode` being entirely redundant. Here we drop those fields, but add a pre-calculated score field, as well as force a suboptimal `RouteGraphNode` layout, retaining its existing 64 byte size. Without the suboptimal layout, performance is very mixed, but with it performance is mostly improved, by around 10% in most tests. --- lightning/src/routing/router.rs | 73 ++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 33 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index e2ccd1f73ed..9423078749d 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -968,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)) } } @@ -1004,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::(); +#[cfg(any(ldk_bench, not(any(test, fuzzing))))] +const _GRAPH_NODE_FIXED_SIZE: usize = core::mem::size_of::() - 64; + /// A wrapper around the various hop representations. /// /// Can be used to examine the properties of a hop, @@ -2120,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()), @@ -2153,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; @@ -2226,18 +2225,26 @@ 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 }; @@ -2247,9 +2254,9 @@ where L::Target: Logger { let candidate = CandidateRouteHop::FirstHop { details, payer_node_id: &our_node_id, }; - add_entry!(&candidate, $fee_to_target_msat, + 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); } } @@ -2272,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); } } @@ -2320,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); }, }); @@ -2527,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. @@ -2662,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); }, }