Use CandidateRouteHop as input for channel_penalty_msat

  We remove `source`, `target` and `scid` from
  `channel_penalty_msat` to consume them from
  `candidate` of type `CandidateRouteHop`

Author: jbesraa

lightning-background-processor/src/lib.rs

@@ -863,8 +863,8 @@ mod tests {
 	use lightning::ln::msgs::{ChannelMessageHandler, Init};
 	use lightning::ln::peer_handler::{PeerManager, MessageHandler, SocketDescriptor, IgnoringMessageHandler};
 	use lightning::routing::gossip::{NetworkGraph, NodeId, P2PGossipSync};
-	use lightning::routing::router::{DefaultRouter, Path, RouteHop};
 	use lightning::routing::scoring::{ChannelUsage, ScoreUpdate, ScoreLookUp, LockableScore};
+	use lightning::routing::router::{DefaultRouter, Path, RouteHop, CandidateRouteHop};
 	use lightning::util::config::UserConfig;
 	use lightning::util::ser::Writeable;
 	use lightning::util::test_utils;
@@ -1071,7 +1071,7 @@ mod tests {
 	impl ScoreLookUp for TestScorer {
 		type ScoreParams = ();
 		fn channel_penalty_msat(
-			&self, _short_channel_id: u64, _source: &NodeId, _target: &NodeId, _usage: ChannelUsage, _score_params: &Self::ScoreParams
+			&self, _candidate: &CandidateRouteHop, _usage: ChannelUsage, _score_params: &Self::ScoreParams
 		) -> u64 { unimplemented!(); }
 	}
 

lightning/src/routing/router.rs

@@ -130,18 +130,27 @@ impl<'a, S: Deref> ScorerAccountingForInFlightHtlcs<'a, S> where S::Target: Scor
 
 impl<'a, S: Deref> ScoreLookUp for ScorerAccountingForInFlightHtlcs<'a, S> where S::Target: ScoreLookUp {
 	type ScoreParams = <S::Target as ScoreLookUp>::ScoreParams;
-	fn channel_penalty_msat(&self, short_channel_id: u64, source: &NodeId, target: &NodeId, usage: ChannelUsage, score_params: &Self::ScoreParams) -> u64 {
+	fn channel_penalty_msat(&self, candidate: &CandidateRouteHop, usage: ChannelUsage, score_params: &Self::ScoreParams) -> u64 {
+		let target = match candidate.target() {
+			Some(target) => target,
+			None => return self.scorer.channel_penalty_msat(candidate, usage, score_params),
+		};
+		let short_channel_id = match candidate.short_channel_id() {
+			Some(short_channel_id) => short_channel_id,
+			None => return self.scorer.channel_penalty_msat(candidate, usage, score_params),
+		};
+		let source = candidate.source();
 		if let Some(used_liquidity) = self.inflight_htlcs.used_liquidity_msat(
-			source, target, short_channel_id
+			&source, &target, short_channel_id
 		) {
 			let usage = ChannelUsage {
 				inflight_htlc_msat: usage.inflight_htlc_msat.saturating_add(used_liquidity),
 				..usage
 			};
 
-			self.scorer.channel_penalty_msat(short_channel_id, source, target, usage, score_params)
+			self.scorer.channel_penalty_msat(candidate, usage, score_params)
 		} else {
-			self.scorer.channel_penalty_msat(short_channel_id, source, target, usage, score_params)
+			self.scorer.channel_penalty_msat(candidate, usage, score_params)
 		}
 	}
 }
@@ -1065,7 +1074,7 @@ impl<'a> CandidateRouteHop<'a> {
 	/// For `Blinded` and `OneHopBlinded` we return `None` because next hop is not known.
 	pub fn short_channel_id(&self) -> Option<u64> {
 		match self {
-			CandidateRouteHop::FirstHop { details, .. } => Some(details.get_outbound_payment_scid().unwrap()),
+			CandidateRouteHop::FirstHop { details, .. } => details.get_outbound_payment_scid(),
 			CandidateRouteHop::PublicHop { short_channel_id, .. } => Some(*short_channel_id),
 			CandidateRouteHop::PrivateHop { hint, .. } => Some(hint.short_channel_id),
 			CandidateRouteHop::Blinded { .. } => None,
@@ -1171,7 +1180,7 @@ impl<'a> CandidateRouteHop<'a> {
 			CandidateRouteHop::PublicHop { source_node_id, .. } => *source_node_id,
 			CandidateRouteHop::PrivateHop { hint, .. } => hint.src_node_id.into(),
 			CandidateRouteHop::Blinded { hint, .. } => hint.1.introduction_node_id.into(),
-			CandidateRouteHop::OneHopBlinded { hint, .. } => hint.1.introduction_node_id.into()
+			CandidateRouteHop::OneHopBlinded { hint, .. } => hint.1.introduction_node_id.into(),
 		}
 	}
 	/// Returns the target node id of this hop, if known.
@@ -1798,7 +1807,7 @@ where L::Target: Logger {
 	let mut num_ignored_htlc_minimum_msat_limit: u32 = 0;
 
 	macro_rules! add_entry {
-		// Adds entry which goes from $src_node_id to $dest_node_id over the $candidate hop.
+		// Adds entry which goes from candidate.source() to candiadte.target() over the $candidate hop.
 		// $next_hops_fee_msat represents the fees paid for using all the channels *after* this one,
 		// since that value has to be transferred over this channel.
 		// Returns the contribution amount of $candidate if the channel caused an update to `targets`.
@@ -1814,7 +1823,7 @@ where L::Target: Logger {
 			// - for first and last hops early in get_route
 			let src_node_id = $candidate.source();
 			let dest_node_id = $candidate.target().unwrap_or(maybe_dummy_payee_node_id);
-			if src_node_id != dest_node_id {
+			if Some($candidate.source()) != $candidate.target() {
 				let scid_opt = $candidate.short_channel_id();
 				let effective_capacity = $candidate.effective_capacity();
 				let htlc_maximum_msat = max_htlc_from_capacity(effective_capacity, channel_saturation_pow_half);
@@ -1979,9 +1988,10 @@ where L::Target: Logger {
 									inflight_htlc_msat: used_liquidity_msat,
 									effective_capacity,
 								};
-								let channel_penalty_msat = scid_opt.map_or(0,
-									|scid| scorer.channel_penalty_msat(scid, &src_node_id, &dest_node_id,
-										channel_usage, score_params));
+								let channel_penalty_msat =
+									scorer.channel_penalty_msat($candidate,
+										channel_usage,
+										score_params);
 								let path_penalty_msat = $next_hops_path_penalty_msat
 									.saturating_add(channel_penalty_msat);
 								let new_graph_node = RouteGraphNode {
@@ -1994,7 +2004,7 @@ where L::Target: Logger {
 									path_length_to_node,
 								};
 
-								// Update the way of reaching $src_node_id with the given short_channel_id (from $dest_node_id),
+								// Update the way of reaching $candidate.source() with the given short_channel_id (from $candidate.target()),
 								// if this way is cheaper than the already known
 								// (considering the cost to "reach" this channel from the route destination,
 								// the cost of using this channel,
@@ -2288,7 +2298,7 @@ where L::Target: Logger {
 						effective_capacity: candidate.effective_capacity(),
 					};
 					let channel_penalty_msat = scorer.channel_penalty_msat(
-						hop.short_channel_id, &source, &target, channel_usage, score_params
+						&candidate, channel_usage, score_params
 					);
 					aggregate_next_hops_path_penalty_msat = aggregate_next_hops_path_penalty_msat
 						.saturating_add(channel_penalty_msat);
@@ -2652,7 +2662,6 @@ where L::Target: Logger {
 	let mut paths = Vec::new();
 	for payment_path in selected_route {
 		let mut hops = Vec::with_capacity(payment_path.hops.len());
-		let mut prev_hop_node_id = our_node_id;
 		for (hop, node_features) in payment_path.hops.iter()
 			.filter(|(h, _)| h.candidate.short_channel_id().is_some())
 		{
@@ -2669,7 +2678,7 @@ where L::Target: Logger {
 				// an alias, in which case we don't take any chances here.
 				network_graph.node(&hop.node_id).map_or(false, |hop_node|
 					hop_node.channels.iter().any(|scid| network_graph.channel(*scid)
-							.map_or(false, |c| c.as_directed_from(&prev_hop_node_id).is_some()))
+							.map_or(false, |c| c.as_directed_from(&hop.candidate.source()).is_some()))
 				)
 			};
 
@@ -2682,8 +2691,6 @@ where L::Target: Logger {
 				cltv_expiry_delta: hop.candidate.cltv_expiry_delta(),
 				maybe_announced_channel,
 			});
-
-			prev_hop_node_id = hop.node_id;
 		}
 		let mut final_cltv_delta = final_cltv_expiry_delta;
 		let blinded_tail = payment_path.hops.last().and_then(|(h, _)| {
@@ -2846,13 +2853,13 @@ fn build_route_from_hops_internal<L: Deref>(
 
 	impl ScoreLookUp for HopScorer {
 		type ScoreParams = ();
-		fn channel_penalty_msat(&self, _short_channel_id: u64, source: &NodeId, target: &NodeId,
+		fn channel_penalty_msat(&self, candidate: &CandidateRouteHop,
 			_usage: ChannelUsage, _score_params: &Self::ScoreParams) -> u64
 		{
 			let mut cur_id = self.our_node_id;
 			for i in 0..self.hop_ids.len() {
 				if let Some(next_id) = self.hop_ids[i] {
-					if cur_id == *source && next_id == *target {
+					if cur_id == candidate.source() && Some(next_id) == candidate.target() {
 						return 0;
 					}
 					cur_id = next_id;
@@ -2928,6 +2935,8 @@ mod tests {
 
 	use core::convert::TryInto;
 
+	use super::CandidateRouteHop;
+
 	fn get_channel_details(short_channel_id: Option<u64>, node_id: PublicKey,
 			features: InitFeatures, outbound_capacity_msat: u64) -> channelmanager::ChannelDetails {
 		channelmanager::ChannelDetails {
@@ -6200,7 +6209,11 @@ mod tests {
 	}
 	impl ScoreLookUp for BadChannelScorer {
 		type ScoreParams = ();
-		fn channel_penalty_msat(&self, short_channel_id: u64, _: &NodeId, _: &NodeId, _: ChannelUsage, _score_params:&Self::ScoreParams) -> u64 {
+		fn channel_penalty_msat(&self, candidate: &CandidateRouteHop, _: ChannelUsage, _score_params:&Self::ScoreParams) -> u64 {
+			let short_channel_id = match candidate.short_channel_id() {
+				Some(id) => id,
+				None => return 0,
+			};
 			if short_channel_id == self.short_channel_id { u64::max_value() } else { 0 }
 		}
 	}
@@ -6216,8 +6229,8 @@ mod tests {
 
 	impl ScoreLookUp for BadNodeScorer {
 		type ScoreParams = ();
-		fn channel_penalty_msat(&self, _: u64, _: &NodeId, target: &NodeId, _: ChannelUsage, _score_params:&Self::ScoreParams) -> u64 {
-			if *target == self.node_id { u64::max_value() } else { 0 }
+		fn channel_penalty_msat(&self, candidate: &CandidateRouteHop, _: ChannelUsage, _score_params:&Self::ScoreParams) -> u64 {
+			if candidate.target() == Some(self.node_id) { u64::max_value() } else { 0 }
 		}
 	}
 
@@ -6705,26 +6718,34 @@ mod tests {
 		};
 		scorer_params.set_manual_penalty(&NodeId::from_pubkey(&nodes[3]), 123);
 		scorer_params.set_manual_penalty(&NodeId::from_pubkey(&nodes[4]), 456);
-		assert_eq!(scorer.channel_penalty_msat(42, &NodeId::from_pubkey(&nodes[3]), &NodeId::from_pubkey(&nodes[4]), usage, &scorer_params), 456);
+		let network_graph = network_graph.read_only();
+		let channels = network_graph.channels();
+		let channel = channels.get(&5).unwrap();
+		let info = channel.as_directed_from(&NodeId::from_pubkey(&nodes[3])).unwrap();
+		let candidate: CandidateRouteHop = CandidateRouteHop::PublicHop {
+			info: info.0,
+			short_channel_id: 5,
+			source_node_id: NodeId::from_pubkey(&nodes[3]),
+			target_node_id: NodeId::from_pubkey(&nodes[4]),
+		};
+		assert_eq!(scorer.channel_penalty_msat(&candidate, usage, &scorer_params), 456);
 
 		// Then check we can get a normal route
 		let payment_params = PaymentParameters::from_node_id(nodes[10], 42);
 		let route_params = RouteParameters::from_payment_params_and_value(
 			payment_params, 100);
-		let route = get_route(&our_id, &route_params, &network_graph.read_only(), None,
+		let route = get_route(&our_id, &route_params, &network_graph, None,
 			Arc::clone(&logger), &scorer, &scorer_params, &random_seed_bytes);
 		assert!(route.is_ok());
 
 		// Then check that we can't get a route if we ban an intermediate node.
 		scorer_params.add_banned(&NodeId::from_pubkey(&nodes[3]));
-		let route = get_route(&our_id, &route_params, &network_graph.read_only(), None,
-			Arc::clone(&logger), &scorer, &scorer_params, &random_seed_bytes);
+		let route = get_route(&our_id, &route_params, &network_graph, None, Arc::clone(&logger), &scorer, &scorer_params,&random_seed_bytes);
 		assert!(route.is_err());
 
 		// Finally make sure we can route again, when we remove the ban.
 		scorer_params.remove_banned(&NodeId::from_pubkey(&nodes[3]));
-		let route = get_route(&our_id, &route_params, &network_graph.read_only(), None,
-			Arc::clone(&logger), &scorer, &scorer_params, &random_seed_bytes);
+		let route = get_route(&our_id, &route_params, &network_graph, None, Arc::clone(&logger), &scorer, &scorer_params,&random_seed_bytes);
 		assert!(route.is_ok());
 	}