From: Matt Corallo Date: Mon, 22 Nov 2021 18:00:08 +0000 (+0000) Subject: Seal `scoring::Time` and only use `Instant` or `Eternity` publicly X-Git-Tag: v0.0.104~21^2 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=3539f270c47ec8b525bac65fce2b85c94eb55be9;p=rust-lightning Seal `scoring::Time` and only use `Instant` or `Eternity` publicly `scoring::Time` exists in part to make testing the passage of time in `Scorer` practical. To allow no-std users to provide a time source it was exposed as a trait as well. However, it seems somewhat unlikely that a no-std user is going to have a use for providing their own time source (otherwise they wouldn't be a no-std user), and likely they won't have a graph in memory either. `scoring::Time` as currently written is also exceptionally hard to write C bindings for - the C bindings trait mappings relies on the ability to construct trait implementations at runtime with function pointers (i.e. `dyn Trait`s). `scoring::Time`, on the other hand, is a supertrait of `core::ops::Sub` which requires a `sub` method which takes a type parameter and returns a type parameter. Both of which aren't practical in bindings, especially given the `Sub::Output` associated type is not bound by any trait bounds at all (implying we cannot simply map the `sub` function to return an opaque trait object). Thus, for simplicity, we here simply seal `scoring::Time` and make it effectively-private, ensuring the bindings don't need to bother with it. --- diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs index eb522a48a..e478dbcbe 100644 --- a/lightning/src/routing/scoring.rs +++ b/lightning/src/routing/scoring.rs @@ -46,9 +46,8 @@ //! //! # Note //! -//! If persisting [`Scorer`], it must be restored using the same [`Time`] parameterization. Using a -//! different type results in undefined behavior. Specifically, persisting when built with feature -//! `no-std` and restoring without it, or vice versa, uses different types and thus is undefined. +//! Persisting when built with feature `no-std` and restoring without it, or vice versa, uses +//! different types and thus is undefined. //! //! [`find_route`]: crate::routing::router::find_route @@ -59,9 +58,10 @@ use util::ser::{Readable, Writeable, Writer}; use prelude::*; use core::cell::{RefCell, RefMut}; -use core::ops::{DerefMut, Sub}; +use core::ops::DerefMut; use core::time::Duration; -use io::{self, Read}; use sync::{Mutex, MutexGuard}; +use io::{self, Read}; +use sync::{Mutex, MutexGuard}; /// We define Score ever-so-slightly differently based on whether we are being built for C bindings /// or not. For users, `LockableScore` must somehow be writeable to disk. For Rust users, this is @@ -185,23 +185,33 @@ impl<'a, S: Writeable> Writeable for MutexGuard<'a, S> { /// See [module-level documentation] for usage. /// /// [module-level documentation]: crate::routing::scoring -pub type Scorer = ScorerUsingTime::; - -/// Time used by [`Scorer`]. #[cfg(not(feature = "no-std"))] -pub type DefaultTime = std::time::Instant; - -/// Time used by [`Scorer`]. +pub type Scorer = ScorerUsingTime::; +/// [`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::scoring #[cfg(feature = "no-std")] -pub type DefaultTime = Eternity; +pub type Scorer = ScorerUsingTime::; + +// Note that ideally we'd hide ScorerUsingTime from public view by sealing it as well, but rustdoc +// doesn't handle this well - instead exposing a `Scorer` which has no trait implementation(s) or +// methods at all. -/// [`Score`] implementation parameterized by [`Time`]. +/// [`Score`] implementation. /// /// See [`Scorer`] for details. /// /// # Note /// -/// Mixing [`Time`] types between serialization and deserialization results in undefined behavior. +/// Mixing the `no-std` feature between serialization and deserialization results in undefined +/// behavior. +/// +/// (C-not exported) generally all users should use the [`Scorer`] type alias. pub struct ScorerUsingTime { params: ScoringParameters, // TODO: Remove entries of closed channels. @@ -247,8 +257,8 @@ pub struct ScoringParameters { /// /// # Note /// - /// When time is an [`Eternity`], as is default when enabling feature `no-std`, it will never - /// elapse. Therefore, this penalty will never decay. + /// When built with the `no-std` feature, time will never elapse. Therefore, this penalty will + /// never decay. /// /// [`failure_penalty_msat`]: Self::failure_penalty_msat pub failure_penalty_half_life: Duration, @@ -273,20 +283,6 @@ struct ChannelFailure { last_failed: T, } -/// A measurement of time. -pub trait Time: Sub where Self: Sized { - /// Returns an instance corresponding to the current moment. - fn now() -> Self; - - /// Returns the amount of time elapsed since `self` was created. - fn elapsed(&self) -> Duration; - - /// Returns the amount of time passed since the beginning of [`Time`]. - /// - /// Used during (de-)serialization. - fn duration_since_epoch() -> Duration; -} - impl ScorerUsingTime { /// Creates a new scorer using the given scoring parameters. pub fn new(params: ScoringParameters) -> Self { @@ -383,48 +379,6 @@ impl Score for ScorerUsingTime { } } -#[cfg(not(feature = "no-std"))] -impl Time for std::time::Instant { - fn now() -> Self { - std::time::Instant::now() - } - - fn duration_since_epoch() -> Duration { - use std::time::SystemTime; - SystemTime::now().duration_since(SystemTime::UNIX_EPOCH).unwrap() - } - - fn elapsed(&self) -> Duration { - std::time::Instant::elapsed(self) - } -} - -/// A state in which time has no meaning. -#[derive(Debug, PartialEq, Eq)] -pub struct Eternity; - -impl Time for Eternity { - fn now() -> Self { - Self - } - - fn duration_since_epoch() -> Duration { - Duration::from_secs(0) - } - - fn elapsed(&self) -> Duration { - Duration::from_secs(0) - } -} - -impl Sub for Eternity { - type Output = Self; - - fn sub(self, _other: Duration) -> Self { - self - } -} - impl Writeable for ScorerUsingTime { #[inline] fn write(&self, w: &mut W) -> Result<(), io::Error> { @@ -475,9 +429,72 @@ impl Readable for ChannelFailure { } } +pub(crate) mod time { + use core::ops::Sub; + use core::time::Duration; + /// A measurement of time. + pub trait Time: Sub where Self: Sized { + /// Returns an instance corresponding to the current moment. + fn now() -> Self; + + /// Returns the amount of time elapsed since `self` was created. + fn elapsed(&self) -> Duration; + + /// Returns the amount of time passed since the beginning of [`Time`]. + /// + /// Used during (de-)serialization. + fn duration_since_epoch() -> Duration; + } + + /// A state in which time has no meaning. + #[derive(Debug, PartialEq, Eq)] + pub struct Eternity; + + #[cfg(not(feature = "no-std"))] + impl Time for std::time::Instant { + fn now() -> Self { + std::time::Instant::now() + } + + fn duration_since_epoch() -> Duration { + use std::time::SystemTime; + SystemTime::now().duration_since(SystemTime::UNIX_EPOCH).unwrap() + } + + fn elapsed(&self) -> Duration { + std::time::Instant::elapsed(self) + } + } + + impl Time for Eternity { + fn now() -> Self { + Self + } + + fn duration_since_epoch() -> Duration { + Duration::from_secs(0) + } + + fn elapsed(&self) -> Duration { + Duration::from_secs(0) + } + } + + impl Sub for Eternity { + type Output = Self; + + fn sub(self, _other: Duration) -> Self { + self + } + } +} + +pub(crate) use self::time::Time; + #[cfg(test)] mod tests { - use super::{Eternity, ScoringParameters, ScorerUsingTime, Time}; + use super::{ScoringParameters, ScorerUsingTime, Time}; + use super::time::Eternity; use routing::scoring::Score; use routing::network_graph::NodeId; diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 45f21e0b3..e2e44a2c1 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -21,7 +21,8 @@ use ln::features::{ChannelFeatures, InitFeatures}; use ln::msgs; use ln::msgs::OptionalField; use ln::script::ShutdownScript; -use routing::scoring::{Eternity, ScorerUsingTime}; +use routing::scoring::ScorerUsingTime; +use routing::scoring::time::Eternity; use util::enforcing_trait_impls::{EnforcingSigner, EnforcementState}; use util::events; use util::logger::{Logger, Level, Record};