Seal `scoring::Time` and only use `Instant` or `Eternity` publicly 2021-11-c-bindings-tweaks
authorMatt Corallo <git@bluematt.me>
Mon, 22 Nov 2021 18:00:08 +0000 (18:00 +0000)
committerMatt Corallo <git@bluematt.me>
Wed, 24 Nov 2021 19:08:12 +0000 (19:08 +0000)
`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.

lightning/src/routing/scoring.rs
lightning/src/util/test_utils.rs

index eb522a48ae59cf1713249e87c43841e0f3d14262..e478dbcbededa9ff5541dd709d1f951e7073fde0 100644 (file)
@@ -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::<DefaultTime>;
-
-/// Time used by [`Scorer`].
 #[cfg(not(feature = "no-std"))]
-pub type DefaultTime = std::time::Instant;
-
-/// Time used by [`Scorer`].
+pub type Scorer = ScorerUsingTime::<std::time::Instant>;
+/// [`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::<time::Eternity>;
+
+// 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<T: Time> {
        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<T: Time> {
        last_failed: T,
 }
 
-/// A measurement of time.
-pub trait Time: Sub<Duration, Output = Self> 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<T: Time> ScorerUsingTime<T> {
        /// Creates a new scorer using the given scoring parameters.
        pub fn new(params: ScoringParameters) -> Self {
@@ -383,48 +379,6 @@ impl<T: Time> Score for ScorerUsingTime<T> {
        }
 }
 
-#[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<Duration> for Eternity {
-       type Output = Self;
-
-       fn sub(self, _other: Duration) -> Self {
-               self
-       }
-}
-
 impl<T: Time> Writeable for ScorerUsingTime<T> {
        #[inline]
        fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
@@ -475,9 +429,72 @@ impl<T: Time> Readable for ChannelFailure<T> {
        }
 }
 
+pub(crate) mod time {
+       use core::ops::Sub;
+       use core::time::Duration;
+       /// A measurement of time.
+       pub trait Time: Sub<Duration, Output = Self> 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<Duration> 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;
index 45f21e0b38c1a39b9105ff5068f1665728253c49..e2e44a2c1a631e5adc8138b6f3ae15965753a83c 100644 (file)
@@ -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};