From 42ebf774155632b5656fd5820eb8c28d0003d9b6 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 12 Nov 2021 15:52:59 +0000 Subject: [PATCH] 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. --- fuzz/src/full_stack.rs | 2 +- fuzz/src/router.rs | 2 +- lightning-invoice/src/payment.rs | 31 ++++--- lightning-invoice/src/utils.rs | 4 +- lightning/src/ln/channelmanager.rs | 2 +- lightning/src/routing/mod.rs | 73 +--------------- lightning/src/routing/router.rs | 14 +-- .../src/routing/{scorer.rs => scoring.rs} | 85 ++++++++++++++++--- lightning/src/util/test_utils.rs | 2 +- 9 files changed, 103 insertions(+), 112 deletions(-) rename lightning/src/routing/{scorer.rs => scoring.rs} (85%) diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index 829ef20b0..81408b85b 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -39,7 +39,7 @@ use lightning::ln::msgs::DecodeError; use lightning::ln::script::ShutdownScript; use lightning::routing::network_graph::{NetGraphMsgHandler, NetworkGraph}; use lightning::routing::router::{find_route, Payee, RouteParameters}; -use lightning::routing::scorer::Scorer; +use lightning::routing::scoring::Scorer; use lightning::util::config::UserConfig; use lightning::util::errors::APIError; use lightning::util::events::Event; diff --git a/fuzz/src/router.rs b/fuzz/src/router.rs index 8c9b4b7d8..149d40134 100644 --- a/fuzz/src/router.rs +++ b/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}; diff --git a/lightning-invoice/src/payment.rs b/lightning-invoice/src/payment.rs index 4099afbaa..a480d40e9 100644 --- a/lightning-invoice/src/payment.rs +++ b/lightning-invoice/src/payment.rs @@ -31,7 +31,7 @@ //! # use lightning::ln::{PaymentHash, PaymentPreimage, 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}; @@ -63,7 +63,7 @@ //! # } //! # //! # struct FakeRouter {}; -//! # impl Router for FakeRouter { +//! # impl Router for FakeRouter { //! # fn find_route( //! # &self, payer: &PublicKey, params: &RouteParameters, payment_hash: &PaymentHash, //! # first_hops: Option<&[&ChannelDetails]>, scorer: &S @@ -71,7 +71,7 @@ //! # } //! # //! # 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: Option, _source: &NodeId, _target: &NodeId //! # ) -> u64 { 0 } @@ -122,8 +122,7 @@ use bitcoin_hashes::sha256::Hash as Sha256; use lightning::ln::{PaymentHash, PaymentPreimage, 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; @@ -139,8 +138,8 @@ use std::time::{Duration, SystemTime}; pub struct InvoicePayer where P::Target: Payer, - R: for <'a> Router<<::Target as routing::LockableScore<'a>>::Locked>, - S::Target: for <'a> routing::LockableScore<'a>, + R: for <'a> Router<<::Target as LockableScore<'a>>::Locked>, + S::Target: for <'a> LockableScore<'a>, L::Target: Logger, E: EventHandler, { @@ -177,7 +176,7 @@ pub trait Payer { } /// A trait defining behavior for routing an [`Invoice`] payment. -pub trait Router { +pub trait Router { /// Finds a [`Route`] between `payer` and `payee` for a payment with the given values. fn find_route( &self, payer: &PublicKey, params: &RouteParameters, payment_hash: &PaymentHash, @@ -207,8 +206,8 @@ pub enum PaymentError { impl InvoicePayer where P::Target: Payer, - R: for <'a> Router<<::Target as routing::LockableScore<'a>>::Locked>, - S::Target: for <'a> routing::LockableScore<'a>, + R: for <'a> Router<<::Target as LockableScore<'a>>::Locked>, + S::Target: for <'a> LockableScore<'a>, L::Target: Logger, E: EventHandler, { @@ -441,8 +440,8 @@ fn has_expired(params: &RouteParameters) -> bool { impl EventHandler for InvoicePayer where P::Target: Payer, - R: for <'a> Router<<::Target as routing::LockableScore<'a>>::Locked>, - S::Target: for <'a> routing::LockableScore<'a>, + R: for <'a> Router<<::Target as LockableScore<'a>>::Locked>, + S::Target: for <'a> LockableScore<'a>, L::Target: Logger, E: EventHandler, { @@ -1186,7 +1185,7 @@ mod tests { } } - impl Router for TestRouter { + impl Router for TestRouter { fn find_route( &self, _payer: &PublicKey, params: &RouteParameters, _payment_hash: &PaymentHash, _first_hops: Option<&[&ChannelDetails]>, _scorer: &S @@ -1199,7 +1198,7 @@ mod tests { struct FailingRouter; - impl Router for FailingRouter { + impl Router for FailingRouter { fn find_route( &self, _payer: &PublicKey, _params: &RouteParameters, _payment_hash: &PaymentHash, _first_hops: Option<&[&ChannelDetails]>, _scorer: &S @@ -1225,7 +1224,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: Option, _source: &NodeId, _target: &NodeId ) -> u64 { 0 } @@ -1364,7 +1363,7 @@ mod tests { // *** Full Featured Functional Tests with a Real ChannelManager *** struct ManualRouter(RefCell>>); - impl Router for ManualRouter { + impl Router for ManualRouter { fn find_route( &self, _payer: &PublicKey, _params: &RouteParameters, _payment_hash: &PaymentHash, _first_hops: Option<&[&ChannelDetails]>, _scorer: &S diff --git a/lightning-invoice/src/utils.rs b/lightning-invoice/src/utils.rs index ad94b0804..b7fdb73f2 100644 --- a/lightning-invoice/src/utils.rs +++ b/lightning-invoice/src/utils.rs @@ -11,7 +11,7 @@ use lightning::chain::keysinterface::{Sign, KeysInterface}; use lightning::ln::{PaymentHash, PaymentPreimage, 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 DefaultRouter where G: Deref, L:: } } -impl Router for DefaultRouter +impl Router for DefaultRouter where G: Deref, L::Target: Logger { fn find_route( &self, payer: &PublicKey, params: &RouteParameters, _payment_hash: &PaymentHash, diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 631edfe6c..03522e24c 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -6552,7 +6552,7 @@ pub mod bench { use ln::msgs::{ChannelMessageHandler, Init}; use routing::network_graph::NetworkGraph; use routing::router::{Payee, get_route}; - use routing::scorer::Scorer; + use routing::scoring::Scorer; use util::test_utils; use util::config::UserConfig; use util::events::{Event, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose}; diff --git a/lightning/src/routing/mod.rs b/lightning/src/routing/mod.rs index 91478bafc..a3ab6c0c1 100644 --- a/lightning/src/routing/mod.rs +++ b/lightning/src/routing/mod.rs @@ -11,75 +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 `send_amt_msat` through the - /// given channel in the direction from `source` to `target`. - /// - /// The channel's capacity (less any other MPP parts which are also being considered for use in - /// the same payment) is given by `channel_capacity_msat`. It may be guessed from various - /// sources or assumed from no data at all. - /// - /// For hints provided in the invoice, we assume the channel has sufficient capacity to accept - /// the invoice's full amount, and provide a `channel_capacity_msat` of `None`. In all other - /// cases it is set to `Some`, even if we're guessing at the channel value. - /// - /// Your code should be overflow-safe through a `channel_capacity_msat` of 21 million BTC. - fn channel_penalty_msat(&self, short_channel_id: u64, send_amt_msat: u64, channel_capacity_msat: Option, 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 { - 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 { - type Locked = RefMut<'a, T>; - - fn lock(&'a self) -> RefMut<'a, T> { - self.borrow_mut() - } -} - -impl> Score for T { - fn channel_penalty_msat(&self, short_channel_id: u64, send_amt_msat: u64, channel_capacity_msat: Option, source: &NodeId, target: &NodeId) -> u64 { - self.deref().channel_penalty_msat(short_channel_id, send_amt_msat, channel_capacity_msat, 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; diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index a98fb9912..90c01ec0d 100644 --- a/lightning/src/routing/router.rs +++ b/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 { /// /// [`ChannelManager::list_usable_channels`]: crate::ln::channelmanager::ChannelManager::list_usable_channels /// [`Event::PaymentPathFailed`]: crate::util::events::Event::PaymentPathFailed -pub fn find_route( +pub fn find_route( our_node_pubkey: &PublicKey, params: &RouteParameters, network: &NetworkGraph, first_hops: Option<&[&ChannelDetails]>, logger: L, scorer: &S ) -> Result @@ -540,7 +540,7 @@ where L::Target: Logger { ) } -pub(crate) fn get_route( +pub(crate) fn get_route( 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: Option, _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: Option, _source: &NodeId, target: &NodeId) -> u64 { if *target == self.node_id { u64::max_value() } else { 0 } } @@ -4787,7 +4787,7 @@ pub(crate) mod test_utils { #[cfg(all(test, feature = "unstable", not(feature = "no-std")))] mod benches { use super::*; - use routing::scorer::Scorer; + use routing::scoring::Scorer; use util::logger::{Logger, Record}; use test::Bencher; diff --git a/lightning/src/routing/scorer.rs b/lightning/src/routing/scoring.rs similarity index 85% rename from lightning/src/routing/scorer.rs rename to lightning/src/routing/scoring.rs index 382eae25b..a2d314665 100644 --- a/lightning/src/routing/scorer.rs +++ b/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,26 +52,89 @@ //! //! [`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 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 `send_amt_msat` through the + /// given channel in the direction from `source` to `target`. + /// + /// The channel's capacity (less any other MPP parts which are also being considered for use in + /// the same payment) is given by `channel_capacity_msat`. It may be guessed from various + /// sources or assumed from no data at all. + /// + /// For hints provided in the invoice, we assume the channel has sufficient capacity to accept + /// the invoice's full amount, and provide a `channel_capacity_msat` of `None`. In all other + /// cases it is set to `Some`, even if we're guessing at the channel value. + /// + /// Your code should be overflow-safe through a `channel_capacity_msat` of 21 million BTC. + fn channel_penalty_msat(&self, short_channel_id: u64, send_amt_msat: u64, channel_capacity_msat: Option, 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 { + 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 { + type Locked = RefMut<'a, T>; + + fn lock(&'a self) -> RefMut<'a, T> { + self.borrow_mut() + } +} + +impl> Score for T { + fn channel_penalty_msat(&self, short_channel_id: u64, send_amt_msat: u64, channel_capacity_msat: Option, source: &NodeId, target: &NodeId) -> u64 { + self.deref().channel_penalty_msat(short_channel_id, send_amt_msat, channel_capacity_msat, 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. /// /// See [module-level documentation] for usage. /// -/// [module-level documentation]: crate::routing::scorer +/// [module-level documentation]: crate::routing::scoring pub type Scorer = ScorerUsingTime::; /// Time used by [`Scorer`]. @@ -82,7 +145,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 +299,7 @@ impl Default for ScoringParameters { } } -impl routing::Score for ScorerUsingTime { +impl Score for ScorerUsingTime { fn channel_penalty_msat( &self, short_channel_id: u64, send_amt_msat: u64, chan_capacity_opt: Option, _source: &NodeId, _target: &NodeId ) -> u64 { @@ -366,7 +429,7 @@ impl Readable for ChannelFailure { 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}; diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 4734a1cb4..45f21e0b3 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -21,7 +21,7 @@ use ln::features::{ChannelFeatures, InitFeatures}; use ln::msgs; use ln::msgs::OptionalField; use ln::script::ShutdownScript; -use routing::scorer::{Eternity, ScorerUsingTime}; +use routing::scoring::{Eternity, ScorerUsingTime}; use util::enforcing_trait_impls::{EnforcingSigner, EnforcementState}; use util::events; use util::logger::{Logger, Level, Record}; -- 2.39.5