Pass EffectiveCapacity through to scorer instead of a u64

There is little reason to take the `EffectiveCapacity`, which has
a bunch of information about the type of channel, and distill it
down to a `u64` when scoring channels.

Instead, we here pass the full information we know, in the form of
the original `EffectiveCapacity`. This does create more branching
in the main router loop, which appears to have a very slight (1-2%)
performance loss, but that may well be within noise.

Much more importantly, this resolves a panic in our log
approximation where we can accidentally call `log(0)` when the
channel's effective capacity is `u64::max_value()`.

Author: TheBlueMatt

lightning-invoice/src/payment.rs

@@ -39,7 +39,7 @@
 //! # use lightning::ln::channelmanager::{ChannelDetails, PaymentId, PaymentSendFailure};
 //! # use lightning::ln::msgs::LightningError;
 //! # use lightning::routing::scoring::Score;
-//! # use lightning::routing::network_graph::NodeId;
+//! # use lightning::routing::network_graph::{EffectiveCapacity, NodeId};
 //! # use lightning::routing::router::{Route, RouteHop, RouteParameters};
 //! # use lightning::util::events::{Event, EventHandler, EventsProvider};
 //! # use lightning::util::logger::{Logger, Record};
@@ -90,7 +90,7 @@
 //! # }
 //! # impl Score for FakeScorer {
 //! #     fn channel_penalty_msat(
-//! #         &self, _short_channel_id: u64, _send_amt: u64, _chan_amt: u64, _source: &NodeId, _target: &NodeId
+//! #         &self, _short_channel_id: u64, _send_amt: u64, _cap: EffectiveCapacity, _source: &NodeId, _target: &NodeId
 //! #     ) -> u64 { 0 }
 //! #     fn payment_path_failed(&mut self, _path: &[&RouteHop], _short_channel_id: u64) {}
 //! #     fn payment_path_successful(&mut self, _path: &[&RouteHop]) {}
@@ -532,7 +532,7 @@ mod tests {
 	use lightning::ln::features::{ChannelFeatures, NodeFeatures, InitFeatures};
 	use lightning::ln::functional_test_utils::*;
 	use lightning::ln::msgs::{ChannelMessageHandler, ErrorAction, LightningError};
-	use lightning::routing::network_graph::NodeId;
+	use lightning::routing::network_graph::{EffectiveCapacity, NodeId};
 	use lightning::routing::router::{PaymentParameters, Route, RouteHop};
 	use lightning::util::test_utils::TestLogger;
 	use lightning::util::errors::APIError;
@@ -1327,7 +1327,7 @@ mod tests {
 
 	impl Score for TestScorer {
 		fn channel_penalty_msat(
-			&self, _short_channel_id: u64, _send_amt: u64, _chan_amt: u64, _source: &NodeId, _target: &NodeId
+			&self, _short_channel_id: u64, _send_amt: u64, _capacity: EffectiveCapacity, _source: &NodeId, _target: &NodeId
 		) -> u64 { 0 }
 
 		fn payment_path_failed(&mut self, actual_path: &[&RouteHop], actual_short_channel_id: u64) {

lightning/src/routing/network_graph.rs

@@ -758,7 +758,7 @@ impl<'a> DirectedChannelInfo<'a> {
 			})
 			.or_else(|| capacity_msat.map(|capacity_msat|
 					EffectiveCapacity::Total { capacity_msat }))
-			.unwrap_or(EffectiveCapacity::Unknown)
+			.unwrap_or(EffectiveCapacity::Unknown { previously_used_msat: 0 })
 	}
 
 	/// Returns `Some` if [`ChannelUpdateInfo`] is available in the direction.
@@ -808,6 +808,7 @@ impl<'a> fmt::Debug for DirectedChannelInfoWithUpdate<'a> {
 ///
 /// While this may be smaller than the actual channel capacity, amounts greater than
 /// [`Self::as_msat`] should not be routed through the channel.
+#[derive(Copy, Clone, PartialEq)]
 pub enum EffectiveCapacity {
 	/// The available liquidity in the channel known from being a channel counterparty, and thus a
 	/// direct hop.
@@ -831,22 +832,62 @@ pub enum EffectiveCapacity {
 	Infinite,
 	/// A capacity that is unknown possibly because either the chain state is unavailable to know
 	/// the total capacity or the `htlc_maximum_msat` was not advertised on the gossip network.
-	Unknown,
+	Unknown {
+		/// An amount which should be considered "already used". Used during routing to keep track
+		/// of how much we're already considering routing through this channel.
+		previously_used_msat: u64,
+	},
 }
 
 /// The presumed channel capacity denominated in millisatoshi for [`EffectiveCapacity::Unknown`] to
 /// use when making routing decisions.
 pub const UNKNOWN_CHANNEL_CAPACITY_MSAT: u64 = 250_000 * 1000;
 
 impl EffectiveCapacity {
-	/// Returns the effective capacity denominated in millisatoshi.
-	pub fn as_msat(&self) -> u64 {
+	#[inline]
+	pub(crate) fn as_msat_without_bounds(&self) -> u64 {
 		match self {
 			EffectiveCapacity::ExactLiquidity { liquidity_msat } => *liquidity_msat,
 			EffectiveCapacity::MaximumHTLC { amount_msat } => *amount_msat,
 			EffectiveCapacity::Total { capacity_msat } => *capacity_msat,
 			EffectiveCapacity::Infinite => u64::max_value(),
-			EffectiveCapacity::Unknown => UNKNOWN_CHANNEL_CAPACITY_MSAT,
+			EffectiveCapacity::Unknown { previously_used_msat } => UNKNOWN_CHANNEL_CAPACITY_MSAT.saturating_sub(*previously_used_msat),
+		}
+	}
+
+	/// Returns the effective capacity denominated in millisatoshi.
+	#[inline]
+	pub fn as_msat_with_default(&self) -> Option<u64> {
+		match self {
+			EffectiveCapacity::ExactLiquidity { liquidity_msat } => Some(*liquidity_msat),
+			EffectiveCapacity::MaximumHTLC { amount_msat } => Some(*amount_msat),
+			EffectiveCapacity::Total { capacity_msat } => Some(*capacity_msat),
+			EffectiveCapacity::Infinite => None,
+			EffectiveCapacity::Unknown { previously_used_msat } => Some(UNKNOWN_CHANNEL_CAPACITY_MSAT.saturating_sub(*previously_used_msat)),
+		}
+	}
+
+	/// Returns a new [`EffectiveCapacity`] which is reduced by the given number of millisatoshis
+	#[inline]
+	pub fn checked_sub(&self, msats: u64) -> Option<EffectiveCapacity> {
+		match self {
+			Self::ExactLiquidity { liquidity_msat } => match liquidity_msat.checked_sub(msats) {
+				Some(liquidity_msat) => Some(Self::ExactLiquidity { liquidity_msat }),
+				None => None,
+			},
+			Self::MaximumHTLC { amount_msat } => match amount_msat.checked_sub(msats) {
+				Some(amount_msat) => Some(Self::MaximumHTLC { amount_msat }),
+				None => None,
+			},
+			Self::Total { capacity_msat } => match capacity_msat.checked_sub(msats) {
+				Some(capacity_msat) => Some(Self::Total { capacity_msat }),
+				None => None,
+			},
+			Self::Infinite => Some(Self::Infinite),
+			Self::Unknown { previously_used_msat } => match previously_used_msat.checked_add(msats) {
+				Some(previously_used_msat) => Some(Self::Unknown { previously_used_msat }),
+				None => None,
+			},
 		}
 	}
 }

lightning/src/routing/router.rs

@@ -854,7 +854,7 @@ where L::Target: Logger {
 				let short_channel_id = $candidate.short_channel_id();
 				let available_liquidity_msat = bookkept_channels_liquidity_available_msat
 					.entry(short_channel_id)
-					.or_insert_with(|| $candidate.effective_capacity().as_msat());
+					.or_insert_with(|| $candidate.effective_capacity());
 
 				// It is tricky to substract $next_hops_fee_msat from available liquidity here.
 				// It may be misleading because we might later choose to reduce the value transferred
@@ -863,7 +863,7 @@ where L::Target: Logger {
 				// fees caused by one expensive channel, but then this channel could have been used
 				// if the amount being transferred over this path is lower.
 				// We do this for now, but this is a subject for removal.
-				if let Some(available_value_contribution_msat) = available_liquidity_msat.checked_sub($next_hops_fee_msat) {
+				if let Some(available_value_contribution) = available_liquidity_msat.checked_sub($next_hops_fee_msat) {
 
 					// Routing Fragmentation Mitigation heuristic:
 					//
@@ -886,6 +886,7 @@ where L::Target: Logger {
 						final_value_msat
 					};
 					// Verify the liquidity offered by this channel complies to the minimal contribution.
+					let available_value_contribution_msat = available_value_contribution.as_msat_without_bounds();
 					let contributes_sufficient_value = available_value_contribution_msat >= minimal_value_contribution_msat;
 
 					// Do not consider candidates that exceed the maximum total cltv expiry limit.
@@ -1220,9 +1221,8 @@ where L::Target: Logger {
 							short_channel_id: hop.short_channel_id,
 						})
 						.unwrap_or_else(|| CandidateRouteHop::PrivateHop { hint: hop });
-					let capacity_msat = candidate.effective_capacity().as_msat();
 					aggregate_next_hops_path_penalty_msat = aggregate_next_hops_path_penalty_msat
-						.checked_add(scorer.channel_penalty_msat(hop.short_channel_id, final_value_msat, capacity_msat, &source, &target))
+						.checked_add(scorer.channel_penalty_msat(hop.short_channel_id, path_value_msat, candidate.effective_capacity(), &source, &target))
 						.unwrap_or_else(|| u64::max_value());
 
 					aggregate_next_hops_cltv_delta = aggregate_next_hops_cltv_delta
@@ -1382,12 +1382,13 @@ where L::Target: Logger {
 					let mut spent_on_hop_msat = value_contribution_msat;
 					let next_hops_fee_msat = payment_hop.next_hops_fee_msat;
 					spent_on_hop_msat += next_hops_fee_msat;
-					if spent_on_hop_msat == *channel_liquidity_available_msat {
+					if spent_on_hop_msat == channel_liquidity_available_msat.as_msat_without_bounds() {
 						// If this path used all of this channel's available liquidity, we know
 						// this path will not be selected again in the next loop iteration.
 						prevented_redundant_path_selection = true;
 					}
-					*channel_liquidity_available_msat -= spent_on_hop_msat;
+					*channel_liquidity_available_msat = channel_liquidity_available_msat
+						.checked_sub(spent_on_hop_msat).unwrap_or(EffectiveCapacity::ExactLiquidity { liquidity_msat: 0 });
 				}
 				if !prevented_redundant_path_selection {
 					// If we weren't capped by hitting a liquidity limit on a channel in the path,
@@ -1396,7 +1397,7 @@ where L::Target: Logger {
 					let victim_scid = payment_path.hops[(payment_path.hops.len() - 1) / 2].0.candidate.short_channel_id();
 					log_trace!(logger, "Disabling channel {} for future path building iterations to avoid duplicates.", victim_scid);
 					let victim_liquidity = bookkept_channels_liquidity_available_msat.get_mut(&victim_scid).unwrap();
-					*victim_liquidity = 0;
+					*victim_liquidity = EffectiveCapacity::ExactLiquidity { liquidity_msat: 0 };
 				}
 
 				// Track the total amount all our collected paths allow to send so that we:
@@ -1664,7 +1665,7 @@ fn add_random_cltv_offset(route: &mut Route, payment_params: &PaymentParameters,
 
 #[cfg(test)]
 mod tests {
-	use routing::network_graph::{NetworkGraph, NetGraphMsgHandler, NodeId};
+	use routing::network_graph::{EffectiveCapacity, NetworkGraph, NetGraphMsgHandler, NodeId};
 	use routing::router::{get_route, add_random_cltv_offset, PaymentParameters, Route, RouteHint, RouteHintHop, RouteHop, RoutingFees, DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA};
 	use routing::scoring::Score;
 	use chain::transaction::OutPoint;
@@ -5001,7 +5002,7 @@ mod tests {
 		fn write<W: Writer>(&self, _w: &mut W) -> Result<(), ::io::Error> { unimplemented!() }
 	}
 	impl Score for BadChannelScorer {
-		fn channel_penalty_msat(&self, short_channel_id: u64, _send_amt: u64, _capacity_msat: u64, _source: &NodeId, _target: &NodeId) -> u64 {
+		fn channel_penalty_msat(&self, short_channel_id: u64, _send_amt: u64, _capacity: EffectiveCapacity, _source: &NodeId, _target: &NodeId) -> u64 {
 			if short_channel_id == self.short_channel_id { u64::max_value() } else { 0 }
 		}
 
@@ -5019,7 +5020,7 @@ mod tests {
 	}
 
 	impl Score for BadNodeScorer {
-		fn channel_penalty_msat(&self, _short_channel_id: u64, _send_amt: u64, _capacity_msat: u64, _source: &NodeId, target: &NodeId) -> u64 {
+		fn channel_penalty_msat(&self, _short_channel_id: u64, _send_amt: u64, _capacity: EffectiveCapacity, _source: &NodeId, target: &NodeId) -> u64 {
 			if *target == self.node_id { u64::max_value() } else { 0 }
 		}