-
Notifications
You must be signed in to change notification settings - Fork 409
Misc routing optimization #2803
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
df9c15d
3e90240
98f9e8b
9566c27
bed1fb0
f689e01
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -795,22 +795,32 @@ where | |||||
} | ||||||
} | ||||||
|
||||||
// Fetching values from this struct is very performance sensitive during routefinding. Thus, we | ||||||
// want to ensure that all of the fields we care about (all of them except `last_update_message`) | ||||||
// sit on the same cache line. | ||||||
// | ||||||
// We do this by using `repr(C)`, which forces the struct to be laid out in memory the way we write | ||||||
// it (ensuring `last_update_message` hangs off the end and no fields are reordered after it), and | ||||||
// `align(32)`, ensuring the struct starts either at the start, or in the middle, of a 64b x86-64 | ||||||
// cache line. This ensures the beginning fields (which are 31 bytes) all sit in the same cache | ||||||
// line. | ||||||
#[repr(C, align(32))] | ||||||
#[derive(Clone, Debug, PartialEq, Eq)] | ||||||
/// Details about one direction of a channel as received within a [`ChannelUpdate`]. | ||||||
pub struct ChannelUpdateInfo { | ||||||
/// When the last update to the channel direction was issued. | ||||||
/// Value is opaque, as set in the announcement. | ||||||
pub last_update: u32, | ||||||
/// Whether the channel can be currently used for payments (in this one direction). | ||||||
pub enabled: bool, | ||||||
/// The difference in CLTV values that you must have when routing through this channel. | ||||||
pub cltv_expiry_delta: u16, | ||||||
/// The minimum value, which must be relayed to the next hop via the channel | ||||||
pub htlc_minimum_msat: u64, | ||||||
/// The maximum value which may be relayed to the next hop via the channel. | ||||||
pub htlc_maximum_msat: u64, | ||||||
/// Fees charged when the channel is used for routing | ||||||
pub fees: RoutingFees, | ||||||
/// When the last update to the channel direction was issued. | ||||||
/// Value is opaque, as set in the announcement. | ||||||
pub last_update: u32, | ||||||
/// The difference in CLTV values that you must have when routing through this channel. | ||||||
pub cltv_expiry_delta: u16, | ||||||
/// Whether the channel can be currently used for payments (in this one direction). | ||||||
pub enabled: bool, | ||||||
/// Most recent update for the channel received from the network | ||||||
/// Mostly redundant with the data we store in fields explicitly. | ||||||
/// Everything else is useful only for sending out for initial routing sync. | ||||||
|
@@ -878,22 +888,46 @@ impl Readable for ChannelUpdateInfo { | |||||
} | ||||||
} | ||||||
|
||||||
// Fetching values from this struct is very performance sensitive during routefinding. Thus, we | ||||||
// want to ensure that all of the fields we care about (all of them except `last_update_message` | ||||||
// and `announcement_received_time`) sit on the same cache line. | ||||||
// | ||||||
// Sadly, this is not possible, however we can still do okay - all of the fields before | ||||||
// `one_to_two` and `two_to_one` are just under 128 bytes long, so we can ensure they sit on | ||||||
// adjacent cache lines (which are generally fetched together in x86_64 processors). | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
|
||||||
// | ||||||
// This leaves only the two directional channel info structs on separate cache lines. | ||||||
// | ||||||
// We accomplish this using `repr(C)`, which forces the struct to be laid out in memory the way we | ||||||
// write it (ensuring the fields we care about are at the start of the struct) and `align(128)`, | ||||||
// ensuring the struct starts at the beginning of two adjacent 64b x86-64 cache lines. | ||||||
#[repr(align(128), C)] | ||||||
#[derive(Clone, Debug, Eq)] | ||||||
/// Details about a channel (both directions). | ||||||
/// Received within a channel announcement. | ||||||
pub struct ChannelInfo { | ||||||
/// Protocol features of a channel communicated during its announcement | ||||||
pub features: ChannelFeatures, | ||||||
|
||||||
/// Source node of the first direction of a channel | ||||||
pub node_one: NodeId, | ||||||
/// Details about the first direction of a channel | ||||||
pub one_to_two: Option<ChannelUpdateInfo>, | ||||||
|
||||||
/// Source node of the second direction of a channel | ||||||
pub node_two: NodeId, | ||||||
/// Details about the second direction of a channel | ||||||
pub two_to_one: Option<ChannelUpdateInfo>, | ||||||
|
||||||
/// The [`NodeInfo::node_counter`] of the node pointed to by [`Self::node_one`]. | ||||||
pub(crate) node_one_counter: u32, | ||||||
/// The [`NodeInfo::node_counter`] of the node pointed to by [`Self::node_two`]. | ||||||
pub(crate) node_two_counter: u32, | ||||||
|
||||||
/// The channel capacity as seen on-chain, if chain lookup is available. | ||||||
pub capacity_sats: Option<u64>, | ||||||
|
||||||
/// Details about the first direction of a channel | ||||||
pub one_to_two: Option<ChannelUpdateInfo>, | ||||||
/// Details about the second direction of a channel | ||||||
pub two_to_one: Option<ChannelUpdateInfo>, | ||||||
|
||||||
/// An initial announcement of the channel | ||||||
/// Mostly redundant with the data we store in fields explicitly. | ||||||
/// Everything else is useful only for sending out for initial routing sync. | ||||||
|
@@ -903,11 +937,6 @@ pub struct ChannelInfo { | |||||
/// (which we can probably assume we are - no-std environments probably won't have a full | ||||||
/// network graph in memory!). | ||||||
announcement_received_time: u64, | ||||||
|
||||||
/// The [`NodeInfo::node_counter`] of the node pointed to by [`Self::node_one`]. | ||||||
pub(crate) node_one_counter: u32, | ||||||
/// The [`NodeInfo::node_counter`] of the node pointed to by [`Self::node_two`]. | ||||||
pub(crate) node_two_counter: u32, | ||||||
} | ||||||
|
||||||
impl PartialEq for ChannelInfo { | ||||||
|
@@ -1053,6 +1082,8 @@ impl Readable for ChannelInfo { | |||||
pub struct DirectedChannelInfo<'a> { | ||||||
channel: &'a ChannelInfo, | ||||||
direction: &'a ChannelUpdateInfo, | ||||||
source_counter: u32, | ||||||
target_counter: u32, | ||||||
/// 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, | ||||||
|
@@ -1061,7 +1092,12 @@ pub struct DirectedChannelInfo<'a> { | |||||
impl<'a> DirectedChannelInfo<'a> { | ||||||
#[inline] | ||||||
fn new(channel: &'a ChannelInfo, direction: &'a ChannelUpdateInfo, from_node_one: bool) -> Self { | ||||||
Self { channel, direction, from_node_one } | ||||||
let (source_counter, target_counter) = if from_node_one { | ||||||
(channel.node_one_counter, channel.node_two_counter) | ||||||
} else { | ||||||
(channel.node_two_counter, channel.node_one_counter) | ||||||
}; | ||||||
Self { channel, direction, from_node_one, source_counter, target_counter } | ||||||
} | ||||||
|
||||||
/// Returns information for the channel. | ||||||
|
@@ -1104,12 +1140,12 @@ impl<'a> DirectedChannelInfo<'a> { | |||||
pub fn target(&self) -> &'a NodeId { if self.from_node_one { &self.channel.node_two } else { &self.channel.node_one } } | ||||||
|
||||||
/// Returns the source node's counter | ||||||
#[inline] | ||||||
pub(super) fn source_counter(&self) -> u32 { if self.from_node_one { self.channel.node_one_counter } else { self.channel.node_two_counter } } | ||||||
#[inline(always)] | ||||||
pub(super) fn source_counter(&self) -> u32 { self.source_counter } | ||||||
|
||||||
/// Returns the target node's counter | ||||||
#[inline] | ||||||
pub(super) fn target_counter(&self) -> u32 { if self.from_node_one { self.channel.node_two_counter } else { self.channel.node_one_counter } } | ||||||
#[inline(always)] | ||||||
pub(super) fn target_counter(&self) -> u32 { self.target_counter } | ||||||
} | ||||||
|
||||||
impl<'a> fmt::Debug for DirectedChannelInfo<'a> { | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1437,7 +1437,7 @@ impl<'a> CandidateRouteHop<'a> { | |
} | ||
} | ||
|
||
#[inline] | ||
#[inline(always)] | ||
fn src_node_counter(&self) -> u32 { | ||
match self { | ||
CandidateRouteHop::FirstHop(hop) => hop.payer_node_counter, | ||
|
@@ -1772,6 +1772,14 @@ struct PathBuildingHop<'a> { | |
/// decrease as well. Thus, we have to explicitly track which nodes have been processed and | ||
/// avoid processing them again. | ||
was_processed: bool, | ||
/// When processing a node as the next best-score candidate, we want to quickly check if it is | ||
/// a direct counterparty of ours, using our local channel information immediately if we can. | ||
/// | ||
/// In order to do so efficiently, we cache whether a node is a direct counterparty here at the | ||
/// start of a route-finding pass. Unlike all other fields in this struct, this field is never | ||
/// updated after being initialized - it is set at the start of a route-finding pass and only | ||
/// read thereafter. | ||
is_first_hop_target: bool, | ||
TheBlueMatt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// 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. | ||
|
@@ -1810,6 +1818,7 @@ impl<'a> core::fmt::Debug for PathBuildingHop<'a> { | |
.field("source_node_id", &self.candidate.source()) | ||
.field("target_node_id", &self.candidate.target()) | ||
.field("short_channel_id", &self.candidate.short_channel_id()) | ||
.field("is_first_hop_target", &self.is_first_hop_target) | ||
.field("total_fee_msat", &self.total_fee_msat) | ||
.field("next_hops_fee_msat", &self.next_hops_fee_msat) | ||
.field("hop_use_fee_msat", &self.hop_use_fee_msat) | ||
|
@@ -2383,6 +2392,8 @@ where L::Target: Logger { | |
// if the amount being transferred over this path is lower. | ||
// We do this for now, but this is a subject for removal. | ||
if let Some(mut available_value_contribution_msat) = htlc_maximum_msat.checked_sub($next_hops_fee_msat) { | ||
let cltv_expiry_delta = $candidate.cltv_expiry_delta(); | ||
let htlc_minimum_msat = $candidate.htlc_minimum_msat(); | ||
valentinewallace marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let used_liquidity_msat = used_liquidities | ||
.get(&$candidate.id()) | ||
.map_or(0, |used_liquidity_msat| { | ||
|
@@ -2406,7 +2417,7 @@ where L::Target: Logger { | |
.checked_sub(2*MEDIAN_HOP_CLTV_EXPIRY_DELTA) | ||
.unwrap_or(payment_params.max_total_cltv_expiry_delta - final_cltv_expiry_delta); | ||
let hop_total_cltv_delta = ($next_hops_cltv_delta as u32) | ||
.saturating_add($candidate.cltv_expiry_delta()); | ||
.saturating_add(cltv_expiry_delta); | ||
valentinewallace marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let exceeds_cltv_delta_limit = hop_total_cltv_delta > max_total_cltv_expiry_delta; | ||
|
||
let value_contribution_msat = cmp::min(available_value_contribution_msat, $next_hops_value_contribution); | ||
|
@@ -2417,13 +2428,13 @@ where L::Target: Logger { | |
None => unreachable!(), | ||
}; | ||
#[allow(unused_comparisons)] // $next_hops_path_htlc_minimum_msat is 0 in some calls so rustc complains | ||
let over_path_minimum_msat = amount_to_transfer_over_msat >= $candidate.htlc_minimum_msat() && | ||
let over_path_minimum_msat = amount_to_transfer_over_msat >= htlc_minimum_msat && | ||
amount_to_transfer_over_msat >= $next_hops_path_htlc_minimum_msat; | ||
|
||
#[allow(unused_comparisons)] // $next_hops_path_htlc_minimum_msat is 0 in some calls so rustc complains | ||
let may_overpay_to_meet_path_minimum_msat = | ||
((amount_to_transfer_over_msat < $candidate.htlc_minimum_msat() && | ||
recommended_value_msat >= $candidate.htlc_minimum_msat()) || | ||
((amount_to_transfer_over_msat < htlc_minimum_msat && | ||
recommended_value_msat >= htlc_minimum_msat) || | ||
Comment on lines
+2431
to
+2437
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Review the logic involving |
||
(amount_to_transfer_over_msat < $next_hops_path_htlc_minimum_msat && | ||
recommended_value_msat >= $next_hops_path_htlc_minimum_msat)); | ||
TheBlueMatt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
@@ -2493,12 +2504,14 @@ where L::Target: Logger { | |
// payment path (upstream to the payee). To avoid that, we recompute | ||
// path fees knowing the final path contribution after constructing it. | ||
let curr_min = cmp::max( | ||
$next_hops_path_htlc_minimum_msat, $candidate.htlc_minimum_msat() | ||
$next_hops_path_htlc_minimum_msat, htlc_minimum_msat | ||
); | ||
let path_htlc_minimum_msat = compute_fees_saturating(curr_min, $candidate.fees()) | ||
let candidate_fees = $candidate.fees(); | ||
let src_node_counter = $candidate.src_node_counter(); | ||
let path_htlc_minimum_msat = compute_fees_saturating(curr_min, candidate_fees) | ||
valentinewallace marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.saturating_add(curr_min); | ||
|
||
let dist_entry = &mut dist[$candidate.src_node_counter() as usize]; | ||
let dist_entry = &mut dist[src_node_counter as usize]; | ||
let old_entry = if let Some(hop) = dist_entry { | ||
hop | ||
} else { | ||
|
@@ -2516,6 +2529,7 @@ where L::Target: Logger { | |
path_htlc_minimum_msat, | ||
path_penalty_msat: u64::max_value(), | ||
was_processed: false, | ||
is_first_hop_target: false, | ||
#[cfg(all(not(ldk_bench), any(test, fuzzing)))] | ||
value_contribution_msat, | ||
}); | ||
|
@@ -2540,7 +2554,7 @@ where L::Target: Logger { | |
if src_node_id != our_node_id { | ||
// Note that `u64::max_value` means we'll always fail the | ||
// `old_entry.total_fee_msat > total_fee_msat` check below | ||
hop_use_fee_msat = compute_fees_saturating(amount_to_transfer_over_msat, $candidate.fees()); | ||
hop_use_fee_msat = compute_fees_saturating(amount_to_transfer_over_msat, candidate_fees); | ||
valentinewallace marked this conversation as resolved.
Show resolved
Hide resolved
|
||
total_fee_msat = total_fee_msat.saturating_add(hop_use_fee_msat); | ||
} | ||
|
||
|
@@ -2679,12 +2693,14 @@ where L::Target: Logger { | |
let fee_to_target_msat; | ||
let next_hops_path_htlc_minimum_msat; | ||
let next_hops_path_penalty_msat; | ||
let is_first_hop_target; | ||
let skip_node = if let Some(elem) = &mut dist[$node.node_counter as usize] { | ||
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; | ||
is_first_hop_target = elem.is_first_hop_target; | ||
was_processed | ||
} else { | ||
// Entries are added to dist in add_entry!() when there is a channel from a node. | ||
|
@@ -2695,21 +2711,24 @@ where L::Target: Logger { | |
fee_to_target_msat = 0; | ||
next_hops_path_htlc_minimum_msat = 0; | ||
next_hops_path_penalty_msat = 0; | ||
is_first_hop_target = false; | ||
false | ||
}; | ||
|
||
if !skip_node { | ||
if let Some((first_channels, peer_node_counter)) = first_hop_targets.get(&$node_id) { | ||
for details in first_channels { | ||
debug_assert_eq!(*peer_node_counter, $node.node_counter); | ||
let candidate = CandidateRouteHop::FirstHop(FirstHopCandidate { | ||
details, payer_node_id: &our_node_id, payer_node_counter, | ||
target_node_counter: $node.node_counter, | ||
}); | ||
add_entry!(&candidate, fee_to_target_msat, | ||
$next_hops_value_contribution, | ||
next_hops_path_htlc_minimum_msat, next_hops_path_penalty_msat, | ||
$next_hops_cltv_delta, $next_hops_path_length); | ||
if is_first_hop_target { | ||
if let Some((first_channels, peer_node_counter)) = first_hop_targets.get(&$node_id) { | ||
for details in first_channels { | ||
debug_assert_eq!(*peer_node_counter, $node.node_counter); | ||
let candidate = CandidateRouteHop::FirstHop(FirstHopCandidate { | ||
details, payer_node_id: &our_node_id, payer_node_counter, | ||
target_node_counter: $node.node_counter, | ||
}); | ||
add_entry!(&candidate, fee_to_target_msat, | ||
$next_hops_value_contribution, | ||
next_hops_path_htlc_minimum_msat, next_hops_path_penalty_msat, | ||
$next_hops_cltv_delta, $next_hops_path_length); | ||
} | ||
Comment on lines
+2719
to
+2731
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The handling of first hop targets within the routing algorithm is complex and should be carefully reviewed to ensure correctness. Review the logic for handling first hop targets to ensure correctness. Consider adding more comments to explain how first hop targets are processed in the routing algorithm. |
||
} | ||
} | ||
|
||
|
@@ -2756,6 +2775,32 @@ where L::Target: Logger { | |
for e in dist.iter_mut() { | ||
*e = None; | ||
} | ||
for (_, (chans, peer_node_counter)) in first_hop_targets.iter() { | ||
// In order to avoid looking up whether each node is a first-hop target, we store a | ||
// dummy entry in dist for each first-hop target, allowing us to do this lookup for | ||
// free since we're already looking at the `was_processed` flag. | ||
// | ||
// Note that all the fields (except `is_first_hop_target`) will be overwritten whenever | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be worth adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, it would be, but I'm not sure I know how to write such an assertion? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we could check that if one field is updated, all others are too? But maybe not worth it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I'm not sure how to check on a per-field basis. The likely failure case is we add another field and fail to update it when relevant, but I'm not aware of a way to iterate over the fields of a struct? |
||
// we find a path to the target, so are left as dummies here. | ||
dist[*peer_node_counter as usize] = Some(PathBuildingHop { | ||
candidate: CandidateRouteHop::FirstHop(FirstHopCandidate { | ||
details: &chans[0], | ||
payer_node_id: &our_node_id, | ||
target_node_counter: u32::max_value(), | ||
payer_node_counter: u32::max_value(), | ||
}), | ||
fee_msat: 0, | ||
next_hops_fee_msat: u64::max_value(), | ||
hop_use_fee_msat: u64::max_value(), | ||
total_fee_msat: u64::max_value(), | ||
path_htlc_minimum_msat: u64::max_value(), | ||
path_penalty_msat: u64::max_value(), | ||
was_processed: false, | ||
is_first_hop_target: true, | ||
#[cfg(all(not(ldk_bench), any(test, fuzzing)))] | ||
value_contribution_msat: 0, | ||
}); | ||
} | ||
hit_minimum_limit = false; | ||
|
||
// If first hop is a private channel and the only way to reach the payee, this is the only | ||
|
Uh oh!
There was an error while loading. Please reload this page.