Move Score into a scoring module instead of a top-level module

Traits in top-level modules is somewhat confusing - generally
top-level modules are just organizational modules and don't contain
things themselves, instead placing traits and structs in
sub-modules. Further, its incredibly awkward to have a `scorer`
sub-module, but only have a single struct in it, with the relevant
trait it is the only implementation of somewhere else. Not having
`Score` in the `scorer` sub-module is further confusing because
it's the only module anywhere that references scoring at all.

Author: TheBlueMatt

fuzz/src/router.rs

@@ -17,7 +17,7 @@ use lightning::ln::channelmanager::{ChannelDetails, ChannelCounterparty};
 use lightning::ln::features::InitFeatures;
 use lightning::ln::msgs;
 use lightning::routing::router::{find_route, Payee, RouteHint, RouteHintHop, RouteParameters};
-use lightning::routing::scorer::Scorer;
+use lightning::routing::scoring::Scorer;
 use lightning::util::logger::Logger;
 use lightning::util::ser::Readable;
 use lightning::routing::network_graph::{NetworkGraph, RoutingFees};

lightning-invoice/src/payment.rs

@@ -30,7 +30,7 @@
 //! # use lightning::ln::{PaymentHash, PaymentSecret};
 //! # use lightning::ln::channelmanager::{ChannelDetails, PaymentId, PaymentSendFailure};
 //! # use lightning::ln::msgs::LightningError;
-//! # use lightning::routing;
+//! # use lightning::routing::scoring::Score;
 //! # use lightning::routing::network_graph::NodeId;
 //! # use lightning::routing::router::{Route, RouteHop, RouteParameters};
 //! # use lightning::util::events::{Event, EventHandler, EventsProvider};
@@ -59,15 +59,15 @@
 //! # }
 //! #
 //! # struct FakeRouter {};
-//! # impl<S: routing::Score> Router<S> for FakeRouter {
+//! # impl<S: Score> Router<S> for FakeRouter {
 //! #     fn find_route(
 //! #         &self, payer: &PublicKey, params: &RouteParameters,
 //! #         first_hops: Option<&[&ChannelDetails]>, scorer: &S
 //! #     ) -> Result<Route, LightningError> { unimplemented!() }
 //! # }
 //! #
 //! # struct FakeScorer {};
-//! # impl routing::Score for FakeScorer {
+//! # impl Score for FakeScorer {
 //! #     fn channel_penalty_msat(
 //! #         &self, _short_channel_id: u64, _send_amt: u64, _chan_amt: u64, _source: &NodeId, _target: &NodeId
 //! #     ) -> u64 { 0 }
@@ -117,8 +117,7 @@ use bitcoin_hashes::Hash;
 use lightning::ln::{PaymentHash, PaymentSecret};
 use lightning::ln::channelmanager::{ChannelDetails, PaymentId, PaymentSendFailure};
 use lightning::ln::msgs::LightningError;
-use lightning::routing;
-use lightning::routing::{LockableScore, Score};
+use lightning::routing::scoring::{LockableScore, Score};
 use lightning::routing::router::{Payee, Route, RouteParameters};
 use lightning::util::events::{Event, EventHandler};
 use lightning::util::logger::Logger;
@@ -134,8 +133,8 @@ use std::time::{Duration, SystemTime};
 pub struct InvoicePayer<P: Deref, R, S: Deref, L: Deref, E>
 where
 	P::Target: Payer,
-	R: for <'a> Router<<<S as Deref>::Target as routing::LockableScore<'a>>::Locked>,
-	S::Target: for <'a> routing::LockableScore<'a>,
+	R: for <'a> Router<<<S as Deref>::Target as LockableScore<'a>>::Locked>,
+	S::Target: for <'a> LockableScore<'a>,
 	L::Target: Logger,
 	E: EventHandler,
 {
@@ -166,7 +165,7 @@ pub trait Payer {
 }
 
 /// A trait defining behavior for routing an [`Invoice`] payment.
-pub trait Router<S: routing::Score> {
+pub trait Router<S: Score> {
 	/// Finds a [`Route`] between `payer` and `payee` for a payment with the given values.
 	fn find_route(
 		&self, payer: &PublicKey, params: &RouteParameters, first_hops: Option<&[&ChannelDetails]>,
@@ -196,8 +195,8 @@ pub enum PaymentError {
 impl<P: Deref, R, S: Deref, L: Deref, E> InvoicePayer<P, R, S, L, E>
 where
 	P::Target: Payer,
-	R: for <'a> Router<<<S as Deref>::Target as routing::LockableScore<'a>>::Locked>,
-	S::Target: for <'a> routing::LockableScore<'a>,
+	R: for <'a> Router<<<S as Deref>::Target as LockableScore<'a>>::Locked>,
+	S::Target: for <'a> LockableScore<'a>,
 	L::Target: Logger,
 	E: EventHandler,
 {
@@ -404,8 +403,8 @@ fn has_expired(params: &RouteParameters) -> bool {
 impl<P: Deref, R, S: Deref, L: Deref, E> EventHandler for InvoicePayer<P, R, S, L, E>
 where
 	P::Target: Payer,
-	R: for <'a> Router<<<S as Deref>::Target as routing::LockableScore<'a>>::Locked>,
-	S::Target: for <'a> routing::LockableScore<'a>,
+	R: for <'a> Router<<<S as Deref>::Target as LockableScore<'a>>::Locked>,
+	S::Target: for <'a> LockableScore<'a>,
 	L::Target: Logger,
 	E: EventHandler,
 {
@@ -1050,7 +1049,7 @@ mod tests {
 		}
 	}
 
-	impl<S: routing::Score> Router<S> for TestRouter {
+	impl<S: Score> Router<S> for TestRouter {
 		fn find_route(
 			&self,
 			_payer: &PublicKey,
@@ -1066,7 +1065,7 @@ mod tests {
 
 	struct FailingRouter;
 
-	impl<S: routing::Score> Router<S> for FailingRouter {
+	impl<S: Score> Router<S> for FailingRouter {
 		fn find_route(
 			&self,
 			_payer: &PublicKey,
@@ -1095,7 +1094,7 @@ mod tests {
 		}
 	}
 
-	impl routing::Score for TestScorer {
+	impl Score for TestScorer {
 		fn channel_penalty_msat(
 			&self, _short_channel_id: u64, _send_amt: u64, _chan_amt: u64, _source: &NodeId, _target: &NodeId
 		) -> u64 { 0 }
@@ -1217,7 +1216,7 @@ mod tests {
 	// *** Full Featured Functional Tests with a Real ChannelManager ***
 	struct ManualRouter(RefCell<VecDeque<Result<Route, LightningError>>>);
 
-	impl<S: routing::Score> Router<S> for ManualRouter {
+	impl<S: Score> Router<S> for ManualRouter {
 		fn find_route(&self, _payer: &PublicKey, _params: &RouteParameters, _first_hops: Option<&[&ChannelDetails]>, _scorer: &S)
 		-> Result<Route, LightningError> {
 			self.0.borrow_mut().pop_front().unwrap()

lightning-invoice/src/utils.rs

@@ -11,7 +11,7 @@ use lightning::chain::keysinterface::{Sign, KeysInterface};
 use lightning::ln::{PaymentHash, PaymentSecret};
 use lightning::ln::channelmanager::{ChannelDetails, ChannelManager, PaymentId, PaymentSendFailure, MIN_FINAL_CLTV_EXPIRY};
 use lightning::ln::msgs::LightningError;
-use lightning::routing;
+use lightning::routing::scoring::Score;
 use lightning::routing::network_graph::{NetworkGraph, RoutingFees};
 use lightning::routing::router::{Route, RouteHint, RouteHintHop, RouteParameters, find_route};
 use lightning::util::logger::Logger;
@@ -109,7 +109,7 @@ impl<G, L: Deref> DefaultRouter<G, L> where G: Deref<Target = NetworkGraph>, L::
 	}
 }
 
-impl<G, L: Deref, S: routing::Score> Router<S> for DefaultRouter<G, L>
+impl<G, L: Deref, S: Score> Router<S> for DefaultRouter<G, L>
 where G: Deref<Target = NetworkGraph>, L::Target: Logger {
 	fn find_route(
 		&self, payer: &PublicKey, params: &RouteParameters, first_hops: Option<&[&ChannelDetails]>,

lightning/src/routing/mod.rs

@@ -11,72 +11,4 @@
 
 pub mod network_graph;
 pub mod router;
-pub mod scorer;
-
-use routing::network_graph::NodeId;
-use routing::router::RouteHop;
-
-use core::cell::{RefCell, RefMut};
-use core::ops::DerefMut;
-use sync::{Mutex, MutexGuard};
-
-/// An interface used to score payment channels for path finding.
-///
-///	Scoring is in terms of fees willing to be paid in order to avoid routing through a channel.
-pub trait Score {
-	/// Returns the fee in msats willing to be paid to avoid routing through the given channel
-	/// in the direction from `source` to `target`.
-	///
-	/// The maximum amount which may be in the HTLC at the given channel is given by
-	/// `send_amt_sat`.
-	///
-	/// The channel's capacity in satoshis (given other HTLCs to be sent) is given by
-	/// `channel_capacity_sats`. It may be guessed from various sources or assumed from no data.
-	/// Your code should be overflow-safe through a `channel_capacity_sats` of 21 million BTC.
-	fn channel_penalty_msat(&self, short_channel_id: u64, send_amt_msat: u64, channel_capacity_sat: u64, source: &NodeId, target: &NodeId) -> u64;
-
-	/// Handles updating channel penalties after failing to route through a channel.
-	fn payment_path_failed(&mut self, path: &[&RouteHop], short_channel_id: u64);
-}
-
-/// A scorer that is accessed under a lock.
-///
-/// Needed so that calls to [`Score::channel_penalty_msat`] in [`find_route`] can be made while
-/// having shared ownership of a scorer but without requiring internal locking in [`Score`]
-/// implementations. Internal locking would be detrimental to route finding performance and could
-/// result in [`Score::channel_penalty_msat`] returning a different value for the same channel.
-///
-/// [`find_route`]: crate::routing::router::find_route
-pub trait LockableScore<'a> {
-	/// The locked [`Score`] type.
-	type Locked: 'a + Score;
-
-	/// Returns the locked scorer.
-	fn lock(&'a self) -> Self::Locked;
-}
-
-impl<'a, T: 'a + Score> LockableScore<'a> for Mutex<T> {
-	type Locked = MutexGuard<'a, T>;
-
-	fn lock(&'a self) -> MutexGuard<'a, T> {
-		Mutex::lock(self).unwrap()
-	}
-}
-
-impl<'a, T: 'a + Score> LockableScore<'a> for RefCell<T> {
-	type Locked = RefMut<'a, T>;
-
-	fn lock(&'a self) -> RefMut<'a, T> {
-		self.borrow_mut()
-	}
-}
-
-impl<S: Score, T: DerefMut<Target=S>> Score for T {
-	fn channel_penalty_msat(&self, short_channel_id: u64, send_amt_msat: u64, channel_capacity_sat: u64, source: &NodeId, target: &NodeId) -> u64 {
-		self.deref().channel_penalty_msat(short_channel_id, send_amt_msat, channel_capacity_sat, source, target)
-	}
-
-	fn payment_path_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
-		self.deref_mut().payment_path_failed(path, short_channel_id)
-	}
-}
+pub mod scoring;

lightning/src/routing/router.rs

@@ -17,7 +17,7 @@ use bitcoin::secp256k1::key::PublicKey;
 use ln::channelmanager::ChannelDetails;
 use ln::features::{ChannelFeatures, InvoiceFeatures, NodeFeatures};
 use ln::msgs::{DecodeError, ErrorAction, LightningError, MAX_VALUE_MSAT};
-use routing;
+use routing::scoring::Score;
 use routing::network_graph::{NetworkGraph, NodeId, RoutingFees};
 use util::ser::{Writeable, Readable};
 use util::logger::{Level, Logger};
@@ -529,7 +529,7 @@ fn compute_fees(amount_msat: u64, channel_fees: RoutingFees) -> Option<u64> {
 ///
 /// [`ChannelManager::list_usable_channels`]: crate::ln::channelmanager::ChannelManager::list_usable_channels
 /// [`Event::PaymentPathFailed`]: crate::util::events::Event::PaymentPathFailed
-pub fn find_route<L: Deref, S: routing::Score>(
+pub fn find_route<L: Deref, S: Score>(
 	our_node_pubkey: &PublicKey, params: &RouteParameters, network: &NetworkGraph,
 	first_hops: Option<&[&ChannelDetails]>, logger: L, scorer: &S
 ) -> Result<Route, LightningError>
@@ -540,7 +540,7 @@ where L::Target: Logger {
 	)
 }
 
-pub(crate) fn get_route<L: Deref, S: routing::Score>(
+pub(crate) fn get_route<L: Deref, S: Score>(
 	our_node_pubkey: &PublicKey, payee: &Payee, network: &NetworkGraph,
 	first_hops: Option<&[&ChannelDetails]>, final_value_msat: u64, final_cltv_expiry_delta: u32,
 	logger: L, scorer: &S
@@ -1472,7 +1472,7 @@ where L::Target: Logger {
 
 #[cfg(test)]
 mod tests {
-	use routing;
+	use routing::scoring::Score;
 	use routing::network_graph::{NetworkGraph, NetGraphMsgHandler, NodeId};
 	use routing::router::{get_route, Payee, Route, RouteHint, RouteHintHop, RouteHop, RoutingFees};
 	use chain::transaction::OutPoint;
@@ -4549,7 +4549,7 @@ mod tests {
 		short_channel_id: u64,
 	}
 
-	impl routing::Score for BadChannelScorer {
+	impl Score for BadChannelScorer {
 		fn channel_penalty_msat(&self, short_channel_id: u64, _send_amt: u64, _chan_amt: u64, _source: &NodeId, _target: &NodeId) -> u64 {
 			if short_channel_id == self.short_channel_id { u64::max_value() } else { 0 }
 		}
@@ -4561,7 +4561,7 @@ mod tests {
 		node_id: NodeId,
 	}
 
-	impl routing::Score for BadNodeScorer {
+	impl Score for BadNodeScorer {
 		fn channel_penalty_msat(&self, _short_channel_id: u64, _send_amt: u64, _chan_amt: u64, _source: &NodeId, target: &NodeId) -> u64 {
 			if *target == self.node_id { u64::max_value() } else { 0 }
 		}

lightning/src/routing/scorer.rs renamed to lightning/src/routing/scoring.rs

@@ -10,7 +10,7 @@
 //! Utilities for scoring payment channels.
 //!
 //! [`Scorer`] may be given to [`find_route`] to score payment channels during path finding when a
-//! custom [`routing::Score`] implementation is not needed.
+//! custom [`Score`] implementation is not needed.
 //!
 //! # Example
 //!
@@ -19,7 +19,7 @@
 //! #
 //! # use lightning::routing::network_graph::NetworkGraph;
 //! # use lightning::routing::router::{RouteParameters, find_route};
-//! # use lightning::routing::scorer::{Scorer, ScoringParameters};
+//! # use lightning::routing::scoring::{Scorer, ScoringParameters};
 //! # use lightning::util::logger::{Logger, Record};
 //! # use secp256k1::key::PublicKey;
 //! #
@@ -52,19 +52,80 @@
 //!
 //! [`find_route`]: crate::routing::router::find_route
 
-use routing;
-
 use ln::msgs::DecodeError;
 use routing::network_graph::NodeId;
 use routing::router::RouteHop;
 use util::ser::{Readable, Writeable, Writer};
 
 use prelude::*;
-use core::ops::Sub;
+use core::cell::{RefCell, RefMut};
+use core::ops::{DerefMut, Sub};
 use core::time::Duration;
 use io::{self, Read};
+use sync::{Mutex, MutexGuard};
+
+/// An interface used to score payment channels for path finding.
+///
+///	Scoring is in terms of fees willing to be paid in order to avoid routing through a channel.
+pub trait Score {
+	/// Returns the fee in msats willing to be paid to avoid routing through the given channel
+	/// in the direction from `source` to `target`.
+	///
+	/// The maximum amount which may be in the HTLC at the given channel is given by
+	/// `send_amt_sat`.
+	///
+	/// The channel's capacity in satoshis (given other HTLCs to be sent) is given by
+	/// `channel_capacity_sats`. It may be guessed from various sources or assumed from no data.
+	/// Your code should be overflow-safe through a `channel_capacity_sats` of 21 million BTC.
+	fn channel_penalty_msat(&self, short_channel_id: u64, send_amt_msat: u64, channel_capacity_sat: u64, source: &NodeId, target: &NodeId) -> u64;
+
+	/// Handles updating channel penalties after failing to route through a channel.
+	fn payment_path_failed(&mut self, path: &[&RouteHop], short_channel_id: u64);
+}
+
+/// A scorer that is accessed under a lock.
+///
+/// Needed so that calls to [`Score::channel_penalty_msat`] in [`find_route`] can be made while
+/// having shared ownership of a scorer but without requiring internal locking in [`Score`]
+/// implementations. Internal locking would be detrimental to route finding performance and could
+/// result in [`Score::channel_penalty_msat`] returning a different value for the same channel.
+///
+/// [`find_route`]: crate::routing::router::find_route
+pub trait LockableScore<'a> {
+	/// The locked [`Score`] type.
+	type Locked: 'a + Score;
+
+	/// Returns the locked scorer.
+	fn lock(&'a self) -> Self::Locked;
+}
+
+impl<'a, T: 'a + Score> LockableScore<'a> for Mutex<T> {
+	type Locked = MutexGuard<'a, T>;
+
+	fn lock(&'a self) -> MutexGuard<'a, T> {
+		Mutex::lock(self).unwrap()
+	}
+}
+
+impl<'a, T: 'a + Score> LockableScore<'a> for RefCell<T> {
+	type Locked = RefMut<'a, T>;
+
+	fn lock(&'a self) -> RefMut<'a, T> {
+		self.borrow_mut()
+	}
+}
+
+impl<S: Score, T: DerefMut<Target=S>> Score for T {
+	fn channel_penalty_msat(&self, short_channel_id: u64, send_amt_msat: u64, channel_capacity_sat: u64, source: &NodeId, target: &NodeId) -> u64 {
+		self.deref().channel_penalty_msat(short_channel_id, send_amt_msat, channel_capacity_sat, source, target)
+	}
+
+	fn payment_path_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) {
+		self.deref_mut().payment_path_failed(path, short_channel_id)
+	}
+}
 
-/// [`routing::Score`] implementation that provides reasonable default behavior.
+/// [`Score`] implementation that provides reasonable default behavior.
 ///
 /// Used to apply a fixed penalty to each channel, thus avoiding long paths when shorter paths with
 /// slightly higher fees are available. Will further penalize channels that fail to relay payments.
@@ -82,7 +143,7 @@ pub type DefaultTime = std::time::Instant;
 #[cfg(feature = "no-std")]
 pub type DefaultTime = Eternity;
 
-/// [`routing::Score`] implementation parameterized by [`Time`].
+/// [`Score`] implementation parameterized by [`Time`].
 ///
 /// See [`Scorer`] for details.
 ///
@@ -236,7 +297,7 @@ impl Default for ScoringParameters {
 	}
 }
 
-impl<T: Time> routing::Score for ScorerUsingTime<T> {
+impl<T: Time> Score for ScorerUsingTime<T> {
 	fn channel_penalty_msat(
 		&self, short_channel_id: u64, send_amt_msat: u64, chan_capacity_sat: u64, _source: &NodeId, _target: &NodeId
 	) -> u64 {
@@ -361,7 +422,7 @@ impl<T: Time> Readable for ChannelFailure<T> {
 mod tests {
 	use super::{Eternity, ScoringParameters, ScorerUsingTime, Time};
 
-	use routing::Score;
+	use routing::scoring::Score;
 	use routing::network_graph::NodeId;
 	use util::ser::{Readable, Writeable};