Update CandidateRouteHop::short_channel_id to be optional

Author: valentinewallace

lightning/src/routing/router.rs

@@ -938,11 +938,11 @@ enum CandidateRouteHop<'a> {
 }
 
 impl<'a> CandidateRouteHop<'a> {
-	fn short_channel_id(&self) -> u64 {
+	fn short_channel_id(&self) -> Option<u64> {
 		match self {
-			CandidateRouteHop::FirstHop { details } => details.get_outbound_payment_scid().unwrap(),
-			CandidateRouteHop::PublicHop { short_channel_id, .. } => *short_channel_id,
-			CandidateRouteHop::PrivateHop { hint } => hint.short_channel_id,
+			CandidateRouteHop::FirstHop { details } => Some(details.get_outbound_payment_scid().unwrap()),
+			CandidateRouteHop::PublicHop { short_channel_id, .. } => Some(*short_channel_id),
+			CandidateRouteHop::PrivateHop { hint } => Some(hint.short_channel_id),
 		}
 	}
 
@@ -994,7 +994,9 @@ impl<'a> CandidateRouteHop<'a> {
 		}
 	}
 	fn id(&self, channel_direction: bool /* src_node_id < target_node_id */) -> CandidateHopId {
-		CandidateHopId::Clear((self.short_channel_id(), channel_direction))
+		match self {
+			_ => CandidateHopId::Clear((self.short_channel_id().unwrap(), channel_direction)),
+		}
 	}
 }
 
@@ -1245,6 +1247,18 @@ impl fmt::Display for LoggedPayeePubkey {
 	}
 }
 
+struct LoggedCandidateHop<'a>(&'a CandidateRouteHop<'a>);
+impl<'a> fmt::Display for LoggedCandidateHop<'a> {
+	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+		match self.0 {
+			_ => {
+				"SCID ".fmt(f)?;
+				self.0.short_channel_id().unwrap().fmt(f)
+			},
+		}
+	}
+}
+
 #[inline]
 fn sort_first_hop_channels(
 	channels: &mut Vec<&ChannelDetails>, used_liquidities: &HashMap<CandidateHopId, u64>,
@@ -1549,7 +1563,7 @@ where L::Target: Logger {
 			// - for regular channels at channel announcement (TODO)
 			// - for first and last hops early in get_route
 			if $src_node_id != $dest_node_id {
-				let short_channel_id = $candidate.short_channel_id();
+				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);
 
@@ -1604,8 +1618,8 @@ where L::Target: Logger {
 						 (amount_to_transfer_over_msat < $next_hops_path_htlc_minimum_msat &&
 						  recommended_value_msat > $next_hops_path_htlc_minimum_msat));
 
-					let payment_failed_on_this_channel =
-						payment_params.previously_failed_channels.contains(&short_channel_id);
+					let payment_failed_on_this_channel = scid_opt.map_or(false,
+						|scid| payment_params.previously_failed_channels.contains(&scid));
 
 					// If HTLC minimum is larger than the amount we're going to transfer, we shouldn't
 					// bother considering this channel. If retrying with recommended_value_msat may
@@ -1674,9 +1688,9 @@ where L::Target: Logger {
 								inflight_htlc_msat: used_liquidity_msat,
 								effective_capacity,
 							};
-							let channel_penalty_msat = scorer.channel_penalty_msat(
-								short_channel_id, &$src_node_id, &$dest_node_id, channel_usage, score_params
-							);
+							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 path_penalty_msat = $next_hops_path_penalty_msat
 								.saturating_add(channel_penalty_msat);
 							let new_graph_node = RouteGraphNode {
@@ -1846,8 +1860,8 @@ where L::Target: Logger {
 				let candidate = CandidateRouteHop::FirstHop { details };
 				let added = add_entry!(candidate, our_node_id, payee, 0, path_value_msat,
 									0, 0u64, 0, 0).is_some();
-				log_trace!(logger, "{} direct route to payee via SCID {}",
-						if added { "Added" } else { "Skipped" }, candidate.short_channel_id());
+				log_trace!(logger, "{} direct route to payee via {}",
+						if added { "Added" } else { "Skipped" }, LoggedCandidateHop(&candidate));
 			}
 		}));
 
@@ -2028,10 +2042,12 @@ where L::Target: Logger {
 					let mut features_set = false;
 					if let Some(first_channels) = first_hop_targets.get(&ordered_hops.last().unwrap().0.node_id) {
 						for details in first_channels {
-							if details.get_outbound_payment_scid().unwrap() == ordered_hops.last().unwrap().0.candidate.short_channel_id() {
-								ordered_hops.last_mut().unwrap().1 = details.counterparty.features.to_context();
-								features_set = true;
-								break;
+							if let Some(scid) = ordered_hops.last().unwrap().0.candidate.short_channel_id() {
+								if details.get_outbound_payment_scid().unwrap() == scid {
+									ordered_hops.last_mut().unwrap().1 = details.counterparty.features.to_context();
+									features_set = true;
+									break;
+								}
 							}
 						}
 					}
@@ -2117,11 +2133,12 @@ where L::Target: Logger {
 					// If we weren't capped by hitting a liquidity limit on a channel in the path,
 					// we'll probably end up picking the same path again on the next iteration.
 					// Decrease the available liquidity of a hop in the middle of the path.
-					let victim_scid = payment_path.hops[(payment_path.hops.len()) / 2].0.candidate.short_channel_id();
+					let victim_candidate = &payment_path.hops[(payment_path.hops.len()) / 2].0.candidate;
 					let exhausted = u64::max_value();
-					log_trace!(logger, "Disabling channel {} for future path building iterations to avoid duplicates.", victim_scid);
-					*used_liquidities.entry(CandidateHopId::Clear((victim_scid, false))).or_default() = exhausted;
-					*used_liquidities.entry(CandidateHopId::Clear((victim_scid, true))).or_default() = exhausted;
+					log_trace!(logger, "Disabling route candidate {} for future path building iterations to
+						avoid duplicates.", LoggedCandidateHop(victim_candidate));
+					*used_liquidities.entry(victim_candidate.id(false)).or_default() = exhausted;
+					*used_liquidities.entry(victim_candidate.id(true)).or_default() = exhausted;
 				}
 
 				// Track the total amount all our collected paths allow to send so that we know
@@ -2251,9 +2268,9 @@ where L::Target: Logger {
 	selected_route.sort_unstable_by_key(|path| {
 		let mut key = [0u64; MAX_PATH_LENGTH_ESTIMATE as usize];
 		debug_assert!(path.hops.len() <= key.len());
-		for (scid, key) in path.hops.iter().map(|h| h.0.candidate.short_channel_id()).zip(key.iter_mut()) {
-			*key = scid;
-		}
+		for (scid, key) in path.hops.iter().filter(|h| h.0.candidate.short_channel_id().is_some())
+			.map(|h| h.0.candidate.short_channel_id().unwrap()).zip(key.iter_mut())
+		{ *key = scid; }
 		key
 	});
 	for idx in 0..(selected_route.len() - 1) {
@@ -2268,15 +2285,16 @@ where L::Target: Logger {
 
 	let mut selected_paths = Vec::<Vec<Result<RouteHop, LightningError>>>::new();
 	for payment_path in selected_route {
-		let mut path = payment_path.hops.iter().map(|(payment_hop, node_features)| {
-			Ok(RouteHop {
-				pubkey: PublicKey::from_slice(payment_hop.node_id.as_slice()).map_err(|_| LightningError{err: format!("Public key {:?} is invalid", &payment_hop.node_id), action: ErrorAction::IgnoreAndLog(Level::Trace)})?,
-				node_features: node_features.clone(),
-				short_channel_id: payment_hop.candidate.short_channel_id(),
-				channel_features: payment_hop.candidate.features(),
-				fee_msat: payment_hop.fee_msat,
-				cltv_expiry_delta: payment_hop.candidate.cltv_expiry_delta(),
-			})
+		let mut path = payment_path.hops.iter().filter(|(h, _)| h.candidate.short_channel_id().is_some())
+			.map(|(payment_hop, node_features)| {
+				Ok(RouteHop {
+					pubkey: PublicKey::from_slice(payment_hop.node_id.as_slice()).map_err(|_| LightningError{err: format!("Public key {:?} is invalid", &payment_hop.node_id), action: ErrorAction::IgnoreAndLog(Level::Trace)})?,
+					node_features: node_features.clone(),
+					short_channel_id: payment_hop.candidate.short_channel_id().unwrap(),
+					channel_features: payment_hop.candidate.features(),
+					fee_msat: payment_hop.fee_msat,
+					cltv_expiry_delta: payment_hop.candidate.cltv_expiry_delta(),
+				})
 		}).collect::<Vec<_>>();
 		// Propagate the cltv_expiry_delta one hop backwards since the delta from the current hop is
 		// applicable for the previous hop.