From a3f7b790b45698ccc33d9f08265a1863688c08fe Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 25 Oct 2022 03:15:03 +0000 Subject: [PATCH] Drop A* implementation in the router for simple Dijkstra's As evidenced by the previous commit, it appears our A* router does worse than a more naive approach. This isn't super surpsising, as the A* heuristic calculation requires a map lookup, which is relatively expensive. ``` test routing::router::benches::generate_mpp_routes_with_probabilistic_scorer ... bench: 169,991,943 ns/iter (+/- 30,838,048) test routing::router::benches::generate_mpp_routes_with_zero_penalty_scorer ... bench: 122,144,987 ns/iter (+/- 61,708,911) test routing::router::benches::generate_routes_with_probabilistic_scorer ... bench: 48,546,068 ns/iter (+/- 10,379,642) test routing::router::benches::generate_routes_with_zero_penalty_scorer ... bench: 32,898,557 ns/iter (+/- 14,157,641) ``` --- lightning/src/routing/gossip.rs | 83 ++++----------------------------- lightning/src/routing/router.rs | 49 +++---------------- 2 files changed, 16 insertions(+), 116 deletions(-) diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index 065472aa3..a12b3d563 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -1054,10 +1054,6 @@ impl Readable for NodeAlias { pub struct NodeInfo { /// All valid channels a node has announced pub channels: Vec, - /// Lowest fees enabling routing via any of the enabled, known channels to a node. - /// The two fields (flat and proportional fee) are independent, - /// meaning they don't have to refer to the same channel. - pub lowest_inbound_channel_fees: Option, /// More information about a node from node_announcement. /// Optional because we store a Node entry after learning about it from /// a channel announcement, but before receiving a node announcement. @@ -1066,8 +1062,8 @@ pub struct NodeInfo { impl fmt::Display for NodeInfo { fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { - write!(f, "lowest_inbound_channel_fees: {:?}, channels: {:?}, announcement_info: {:?}", - self.lowest_inbound_channel_fees, &self.channels[..], self.announcement_info)?; + write!(f, " channels: {:?}, announcement_info: {:?}", + &self.channels[..], self.announcement_info)?; Ok(()) } } @@ -1075,7 +1071,7 @@ impl fmt::Display for NodeInfo { impl Writeable for NodeInfo { fn write(&self, writer: &mut W) -> Result<(), io::Error> { write_tlv_fields!(writer, { - (0, self.lowest_inbound_channel_fees, option), + // Note that older versions of LDK wrote the lowest inbound fees here at type 0 (2, self.announcement_info, option), (4, self.channels, vec_type), }); @@ -1103,18 +1099,22 @@ impl MaybeReadable for NodeAnnouncementInfoDeserWrapper { impl Readable for NodeInfo { fn read(reader: &mut R) -> Result { - _init_tlv_field_var!(lowest_inbound_channel_fees, option); + // Historically, we tracked the lowest inbound fees for any node in order to use it as an + // A* heuristic when routing. Sadly, these days many, many nodes have at least one channel + // with zero inbound fees, causing that heuristic to provide little gain. Worse, because it + // requires additional complexity and lookups during routing, it ends up being a + // performance loss. Thus, we simply ignore the old field here and no longer track it. + let mut _lowest_inbound_channel_fees: Option = None; let mut announcement_info_wrap: Option = None; _init_tlv_field_var!(channels, vec_type); read_tlv_fields!(reader, { - (0, lowest_inbound_channel_fees, option), + (0, _lowest_inbound_channel_fees, option), (2, announcement_info_wrap, ignorable), (4, channels, vec_type), }); Ok(NodeInfo { - lowest_inbound_channel_fees: _init_tlv_based_struct_field!(lowest_inbound_channel_fees, option), announcement_info: announcement_info_wrap.map(|w| w.0), channels: _init_tlv_based_struct_field!(channels, vec_type), }) @@ -1175,22 +1175,6 @@ impl ReadableArgs for NetworkGraph where L::Target: Logger { (1, last_rapid_gossip_sync_timestamp, option), }); - // Regenerate inbound fees for all channels. The live-updating of these has been broken in - // various ways historically, so this ensures that we have up-to-date limits. - for (node_id, node) in nodes.iter_mut() { - let mut best_fees = RoutingFees { base_msat: u32::MAX, proportional_millionths: u32::MAX }; - for channel in node.channels.iter() { - if let Some(chan) = channels.get(channel) { - let dir_opt = if *node_id == chan.node_one { &chan.two_to_one } else { &chan.one_to_two }; - if let Some(dir) = dir_opt { - best_fees.base_msat = cmp::min(best_fees.base_msat, dir.fees.base_msat); - best_fees.proportional_millionths = cmp::min(best_fees.proportional_millionths, dir.fees.proportional_millionths); - } - } else { return Err(DecodeError::InvalidValue); } - } - node.lowest_inbound_channel_fees = Some(best_fees); - } - Ok(NetworkGraph { secp_ctx: Secp256k1::verification_only(), genesis_hash, @@ -1430,7 +1414,6 @@ impl NetworkGraph where L::Target: Logger { BtreeEntry::Vacant(node_entry) => { node_entry.insert(NodeInfo { channels: vec!(short_channel_id), - lowest_inbound_channel_fees: None, announcement_info: None, }); } @@ -1731,9 +1714,7 @@ impl NetworkGraph where L::Target: Logger { } fn update_channel_intern(&self, msg: &msgs::UnsignedChannelUpdate, full_msg: Option<&msgs::ChannelUpdate>, sig: Option<&secp256k1::ecdsa::Signature>) -> Result<(), LightningError> { - let dest_node_id; let chan_enabled = msg.flags & (1 << 1) != (1 << 1); - let chan_was_enabled; #[cfg(all(feature = "std", not(test), not(feature = "_test_utils")))] { @@ -1781,9 +1762,6 @@ impl NetworkGraph where L::Target: Logger { } else if existing_chan_info.last_update == msg.timestamp { return Err(LightningError{err: "Update had same timestamp as last processed update".to_owned(), action: ErrorAction::IgnoreDuplicateGossip}); } - chan_was_enabled = existing_chan_info.enabled; - } else { - chan_was_enabled = false; } } } @@ -1811,7 +1789,6 @@ impl NetworkGraph where L::Target: Logger { let msg_hash = hash_to_message!(&Sha256dHash::hash(&msg.encode()[..])[..]); if msg.flags & 1 == 1 { - dest_node_id = channel.node_one.clone(); check_update_latest!(channel.two_to_one); if let Some(sig) = sig { secp_verify_sig!(self.secp_ctx, &msg_hash, &sig, &PublicKey::from_slice(channel.node_two.as_slice()).map_err(|_| LightningError{ @@ -1821,7 +1798,6 @@ impl NetworkGraph where L::Target: Logger { } channel.two_to_one = get_new_channel_info!(); } else { - dest_node_id = channel.node_two.clone(); check_update_latest!(channel.one_to_two); if let Some(sig) = sig { secp_verify_sig!(self.secp_ctx, &msg_hash, &sig, &PublicKey::from_slice(channel.node_one.as_slice()).map_err(|_| LightningError{ @@ -1834,44 +1810,6 @@ impl NetworkGraph where L::Target: Logger { } } - let mut nodes = self.nodes.write().unwrap(); - if chan_enabled { - let node = nodes.get_mut(&dest_node_id).unwrap(); - let mut base_msat = msg.fee_base_msat; - let mut proportional_millionths = msg.fee_proportional_millionths; - if let Some(fees) = node.lowest_inbound_channel_fees { - base_msat = cmp::min(base_msat, fees.base_msat); - proportional_millionths = cmp::min(proportional_millionths, fees.proportional_millionths); - } - node.lowest_inbound_channel_fees = Some(RoutingFees { - base_msat, - proportional_millionths - }); - } else if chan_was_enabled { - let node = nodes.get_mut(&dest_node_id).unwrap(); - let mut lowest_inbound_channel_fees = None; - - for chan_id in node.channels.iter() { - let chan = channels.get(chan_id).unwrap(); - let chan_info_opt; - if chan.node_one == dest_node_id { - chan_info_opt = chan.two_to_one.as_ref(); - } else { - chan_info_opt = chan.one_to_two.as_ref(); - } - if let Some(chan_info) = chan_info_opt { - if chan_info.enabled { - let fees = lowest_inbound_channel_fees.get_or_insert(RoutingFees { - base_msat: u32::max_value(), proportional_millionths: u32::max_value() }); - fees.base_msat = cmp::min(fees.base_msat, chan_info.fees.base_msat); - fees.proportional_millionths = cmp::min(fees.proportional_millionths, chan_info.fees.proportional_millionths); - } - } - } - - node.lowest_inbound_channel_fees = lowest_inbound_channel_fees; - } - Ok(()) } @@ -3291,7 +3229,6 @@ mod tests { // 2. Check we can read a NodeInfo anyways, but set the NodeAnnouncementInfo to None if invalid let valid_node_info = NodeInfo { channels: Vec::new(), - lowest_inbound_channel_fees: None, announcement_info: Some(valid_node_ann_info), }; diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 55d7f0149..e4b95a90d 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -582,7 +582,6 @@ impl_writeable_tlv_based!(RouteHintHop, { #[derive(Eq, PartialEq)] struct RouteGraphNode { node_id: NodeId, - lowest_fee_to_peer_through_node: u64, lowest_fee_to_node: u64, total_cltv_delta: u32, // The maximum value a yet-to-be-constructed payment path might flow through this node. @@ -603,9 +602,9 @@ struct RouteGraphNode { impl cmp::Ord for RouteGraphNode { fn cmp(&self, other: &RouteGraphNode) -> cmp::Ordering { - let other_score = cmp::max(other.lowest_fee_to_peer_through_node, other.path_htlc_minimum_msat) + 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_peer_through_node, self.path_htlc_minimum_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)) } @@ -729,8 +728,6 @@ struct PathBuildingHop<'a> { candidate: CandidateRouteHop<'a>, fee_msat: u64, - /// Minimal fees required to route to the source node of the current hop via any of its inbound channels. - src_lowest_inbound_fees: RoutingFees, /// 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()). @@ -1007,9 +1004,8 @@ where L::Target: Logger { // 8. If our maximum channel saturation limit caused us to pick two identical paths, combine // them so that we're not sending two HTLCs along the same path. - // As for the actual search algorithm, - // we do a payee-to-payer pseudo-Dijkstra's sorting by each node's distance from the payee - // plus the minimum per-HTLC fee to get from it to another node (aka "shitty pseudo-A*"). + // As for the actual search algorithm, we do a payee-to-payer Dijkstra's sorting by each node's + // distance from the payee // // We are not a faithful Dijkstra's implementation because we can change values which impact // earlier nodes while processing later nodes. Specifically, if we reach a channel with a lower @@ -1044,10 +1040,6 @@ where L::Target: Logger { // runtime for little gain. Specifically, the current algorithm rather efficiently explores the // graph for candidate paths, calculating the maximum value which can realistically be sent at // the same time, remaining generic across different payment values. - // - // TODO: There are a few tweaks we could do, including possibly pre-calculating more stuff - // to use as the A* heuristic beyond just the cost to get one node further than the current - // one. let network_channels = network_graph.channels(); let network_nodes = network_graph.nodes(); @@ -1097,7 +1089,7 @@ where L::Target: Logger { } } - // The main heap containing all candidate next-hops sorted by their score (max(A* fee, + // 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. let mut targets: BinaryHeap = BinaryHeap::new(); @@ -1273,20 +1265,10 @@ where L::Target: Logger { // semi-dummy record just to compute the fees to reach the source node. // This will affect our decision on selecting short_channel_id // as a way to reach the $dest_node_id. - let mut fee_base_msat = 0; - let mut fee_proportional_millionths = 0; - if let Some(Some(fees)) = network_nodes.get(&$src_node_id).map(|node| node.lowest_inbound_channel_fees) { - fee_base_msat = fees.base_msat; - fee_proportional_millionths = fees.proportional_millionths; - } PathBuildingHop { node_id: $dest_node_id.clone(), candidate: $candidate.clone(), fee_msat: 0, - src_lowest_inbound_fees: RoutingFees { - base_msat: fee_base_msat, - proportional_millionths: fee_proportional_millionths, - }, next_hops_fee_msat: u64::max_value(), hop_use_fee_msat: u64::max_value(), total_fee_msat: u64::max_value(), @@ -1321,24 +1303,6 @@ where L::Target: Logger { Some(fee_msat) => { hop_use_fee_msat = fee_msat; total_fee_msat += hop_use_fee_msat; - // When calculating the lowest inbound fees to a node, we - // calculate fees here not based on the actual value we think - // will flow over this channel, but on the minimum value that - // we'll accept flowing over it. The minimum accepted value - // is a constant through each path collection run, ensuring - // consistent basis. Otherwise we may later find a - // different path to the source node that is more expensive, - // but which we consider to be cheaper because we are capacity - // constrained and the relative fee becomes lower. - match compute_fees(minimal_value_contribution_msat, old_entry.src_lowest_inbound_fees) - .map(|a| a.checked_add(total_fee_msat)) { - Some(Some(v)) => { - total_fee_msat = v; - }, - _ => { - total_fee_msat = u64::max_value(); - } - }; } } } @@ -1355,8 +1319,7 @@ where L::Target: Logger { .saturating_add(channel_penalty_msat); let new_graph_node = RouteGraphNode { node_id: $src_node_id, - lowest_fee_to_peer_through_node: total_fee_msat, - lowest_fee_to_node: $next_hops_fee_msat as u64 + hop_use_fee_msat, + lowest_fee_to_node: total_fee_msat, total_cltv_delta: hop_total_cltv_delta, value_contribution_msat, path_htlc_minimum_msat, -- 2.39.5