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)
```

Author: TheBlueMatt

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<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,16 +1062,16 @@ 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(())
 	}
 }
 
 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),
 		};
 

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<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,