]> git.bitcoin.ninja Git - rust-lightning/commitdiff
Drop A* implementation in the router for simple Dijkstra's
authorMatt Corallo <git@bluematt.me>
Tue, 25 Oct 2022 03:15:03 +0000 (03:15 +0000)
committerMatt Corallo <git@bluematt.me>
Wed, 25 Jan 2023 17:17:55 +0000 (17:17 +0000)
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
lightning/src/routing/router.rs

index 065472aa3c14158b1e24470490d70d7a917de84a..a12b3d563daaa6353f010224b345314ce8081dc3 100644 (file)
@@ -1054,10 +1054,6 @@ impl Readable for NodeAlias {
 pub struct NodeInfo {
        /// All valid channels a node has announced
        pub channels: Vec<u64>,
-       /// 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<RoutingFees>,
        /// 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<W: crate::util::ser::Writer>(&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<R: io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
-               _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<RoutingFees> = None;
                let mut announcement_info_wrap: Option<NodeAnnouncementInfoDeserWrapper> = 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<L: Deref> ReadableArgs<L> for NetworkGraph<L> 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<L: Deref> NetworkGraph<L> 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<L: Deref> NetworkGraph<L> 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<L: Deref> NetworkGraph<L> 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<L: Deref> NetworkGraph<L> 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<L: Deref> NetworkGraph<L> 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<L: Deref> NetworkGraph<L> 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),
                };
 
index 55d7f01494c6ce0a756a69162c4eb28cb7f13c8e..e4b95a90d3b44ecb18a9da72821f12d2f8e68316 100644 (file)
@@ -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<RouteGraphNode> = 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,