From 016eb96fc7170bdbab238292d3cc9338c2a66eb9 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 21 Nov 2021 22:29:48 +0000 Subject: [PATCH 1/3] Support `logger::Record` in C by String-ing the fmt::Arguments This adds a new (non-feature) cfg argument `c_bindings` which will be set when building C bindings. With this, we can (slightly) tweak behavior and API based on whether we are being built for Rust or C users. Ideally we'd never need this, but as long as we can keep the API consistent-enough to avoid material code drift, this gives us a cheap way of doing the "right" thing for both C and Rust when the two are in tension. We also move lightning-background-processor to support the same MSRV as the main lightning crate, instead of only lightning-net-tokio's MSRV. --- .github/workflows/build.yml | 8 ++++++++ lightning/src/util/logger.rs | 25 +++++++++++++++++++------ 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 34337769a26..45084bbd5e0 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -115,6 +115,14 @@ jobs: cargo test --verbose --color always -p lightning cargo test --verbose --color always -p lightning-invoice cargo build --verbose --color always -p lightning-persister + cargo build --verbose --color always -p lightning-background-processor + - name: Test C Bindings Modifications on Rust ${{ matrix.toolchain }} + if: "! matrix.build-net-tokio" + run: | + RUSTFLAGS="--cfg=c_bindings" cargo test --verbose --color always -p lightning + RUSTFLAGS="--cfg=c_bindings" cargo test --verbose --color always -p lightning-invoice + RUSTFLAGS="--cfg=c_bindings" cargo build --verbose --color always -p lightning-persister + RUSTFLAGS="--cfg=c_bindings" cargo build --verbose --color always -p lightning-background-processor - name: Test Block Sync Clients on Rust ${{ matrix.toolchain }} with features if: "matrix.build-net-tokio && !matrix.coverage" run: | diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs index 103f89891cf..f8fc1eda108 100644 --- a/lightning/src/util/logger.rs +++ b/lightning/src/util/logger.rs @@ -86,32 +86,45 @@ impl Level { /// A Record, unit of logging output with Metadata to enable filtering /// Module_path, file, line to inform on log's source -/// (C-not exported) - we convert to a const char* instead -#[derive(Clone,Debug)] +#[derive(Clone, Debug)] pub struct Record<'a> { /// The verbosity level of the message. pub level: Level, + #[cfg(not(c_bindings))] /// The message body. pub args: fmt::Arguments<'a>, + #[cfg(c_bindings)] + /// The message body. + pub args: String, /// The module path of the message. - pub module_path: &'a str, + pub module_path: &'static str, /// The source file containing the message. - pub file: &'a str, + pub file: &'static str, /// The line containing the message. pub line: u32, + + #[cfg(c_bindings)] + /// We don't actually use the lifetime parameter in C bindings (as there is no good way to + /// communicate a lifetime to a C, or worse, Java user). + _phantom: core::marker::PhantomData<&'a ()>, } impl<'a> Record<'a> { /// Returns a new Record. /// (C-not exported) as fmt can't be used in C #[inline] - pub fn new(level: Level, args: fmt::Arguments<'a>, module_path: &'a str, file: &'a str, line: u32) -> Record<'a> { + pub fn new(level: Level, args: fmt::Arguments<'a>, module_path: &'static str, file: &'static str, line: u32) -> Record<'a> { Record { level, + #[cfg(not(c_bindings))] args, + #[cfg(c_bindings)] + args: format!("{}", args), module_path, file, - line + line, + #[cfg(c_bindings)] + _phantom: core::marker::PhantomData, } } } From a173ded03f84193f8da14301e81fdec419c5e441 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 22 Nov 2021 03:27:17 +0000 Subject: [PATCH 2/3] Make `Score : Writeable` in c_bindings and impl on `LockedScore` Ultimately we likely need to wrap the locked `Score` in a struct that exposes writeable somehow, but because all traits have to be fully concretized for C bindings we'll still need `Writeable` on all `Score` in order to expose `Writeable` on the locked score. Otherwise, we'll only have a `LockedScore` with a `Score` visible that only has the `Score` methods, never the original type. --- lightning-invoice/src/payment.rs | 8 +++++ lightning/src/routing/router.rs | 11 ++++++ lightning/src/routing/scoring.rs | 62 ++++++++++++++++++++++++++++---- 3 files changed, 75 insertions(+), 6 deletions(-) diff --git a/lightning-invoice/src/payment.rs b/lightning-invoice/src/payment.rs index a480d40e95d..a141eb2c3e1 100644 --- a/lightning-invoice/src/payment.rs +++ b/lightning-invoice/src/payment.rs @@ -36,6 +36,7 @@ //! # use lightning::routing::router::{Route, RouteHop, RouteParameters}; //! # use lightning::util::events::{Event, EventHandler, EventsProvider}; //! # use lightning::util::logger::{Logger, Record}; +//! # use lightning::util::ser::{Writeable, Writer}; //! # use lightning_invoice::Invoice; //! # use lightning_invoice::payment::{InvoicePayer, Payer, RetryAttempts, Router}; //! # use secp256k1::key::PublicKey; @@ -71,6 +72,9 @@ //! # } //! # //! # struct FakeScorer {}; +//! # impl Writeable for FakeScorer { +//! # fn write(&self, w: &mut W) -> Result<(), std::io::Error> { unimplemented!(); } +//! # } //! # impl Score for FakeScorer { //! # fn channel_penalty_msat( //! # &self, _short_channel_id: u64, _send_amt: u64, _chan_amt: Option, _source: &NodeId, _target: &NodeId @@ -1224,6 +1228,10 @@ mod tests { } } + #[cfg(c_bindings)] + impl lightning::util::ser::Writeable for TestScorer { + fn write(&self, _: &mut W) -> Result<(), std::io::Error> { unreachable!(); } + } impl Score for TestScorer { fn channel_penalty_msat( &self, _short_channel_id: u64, _send_amt: u64, _chan_amt: Option, _source: &NodeId, _target: &NodeId diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 8b2e4d137fd..00c3ee6bceb 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1488,6 +1488,8 @@ mod tests { use ln::channelmanager; use util::test_utils; use util::ser::Writeable; + #[cfg(c_bindings)] + use util::ser::Writer; use bitcoin::hashes::sha256d::Hash as Sha256dHash; use bitcoin::hashes::Hash; @@ -4653,6 +4655,10 @@ mod tests { short_channel_id: u64, } + #[cfg(c_bindings)] + impl Writeable for BadChannelScorer { + fn write(&self, _w: &mut W) -> Result<(), ::io::Error> { unimplemented!() } + } 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 } @@ -4665,6 +4671,11 @@ mod tests { node_id: NodeId, } + #[cfg(c_bindings)] + impl Writeable for BadNodeScorer { + fn write(&self, _w: &mut W) -> Result<(), ::io::Error> { unimplemented!() } + } + 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 } diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs index a2d314665d1..eb522a48ae5 100644 --- a/lightning/src/routing/scoring.rs +++ b/lightning/src/routing/scoring.rs @@ -63,10 +63,22 @@ use core::ops::{DerefMut, Sub}; use core::time::Duration; 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 +/// no problem - you move a `Score` that implements `Writeable` into a `Mutex`, lock it, and now +/// you have the original, concrete, `Score` type, which presumably implements `Writeable`. +/// +/// For C users, once you've moved the `Score` into a `LockableScore` all you have after locking it +/// is an opaque trait object with an opaque pointer with no type info. Users could take the unsafe +/// approach of blindly casting that opaque pointer to a concrete type and calling `Writeable` from +/// there, but other languages downstream of the C bindings (e.g. Java) can't even do that. +/// Instead, we really want `Score` and `LockableScore` to implement `Writeable` directly, which we +/// do here by defining `Score` differently for `cfg(c_bindings)`. +macro_rules! define_score { ($($supertrait: path)*) => { /// 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 { +pub trait Score $(: $supertrait)* { /// 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`. /// @@ -85,6 +97,22 @@ pub trait Score { fn payment_path_failed(&mut self, path: &[&RouteHop], short_channel_id: u64); } +impl $(+ $supertrait)*> 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) + } +} +} } + +#[cfg(c_bindings)] +define_score!(Writeable); +#[cfg(not(c_bindings))] +define_score!(); + /// A scorer that is accessed under a lock. /// /// Needed so that calls to [`Score::channel_penalty_msat`] in [`find_route`] can be made while @@ -101,6 +129,7 @@ pub trait LockableScore<'a> { fn lock(&'a self) -> Self::Locked; } +/// (C-not exported) impl<'a, T: 'a + Score> LockableScore<'a> for Mutex { type Locked = MutexGuard<'a, T>; @@ -117,13 +146,34 @@ impl<'a, T: 'a + Score> LockableScore<'a> for RefCell { } } -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) +#[cfg(c_bindings)] +/// A concrete implementation of [`LockableScore`] which supports multi-threading. +pub struct MultiThreadedLockableScore { + score: Mutex, +} +#[cfg(c_bindings)] +/// (C-not exported) +impl<'a, T: Score + 'a> LockableScore<'a> for MultiThreadedLockableScore { + type Locked = MutexGuard<'a, T>; + + fn lock(&'a self) -> MutexGuard<'a, T> { + Mutex::lock(&self.score).unwrap() } +} - fn payment_path_failed(&mut self, path: &[&RouteHop], short_channel_id: u64) { - self.deref_mut().payment_path_failed(path, short_channel_id) +#[cfg(c_bindings)] +/// (C-not exported) +impl<'a, T: Writeable> Writeable for RefMut<'a, T> { + fn write(&self, writer: &mut W) -> Result<(), io::Error> { + T::write(&**self, writer) + } +} + +#[cfg(c_bindings)] +/// (C-not exported) +impl<'a, S: Writeable> Writeable for MutexGuard<'a, S> { + fn write(&self, writer: &mut W) -> Result<(), io::Error> { + S::write(&**self, writer) } } From 3539f270c47ec8b525bac65fce2b85c94eb55be9 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 22 Nov 2021 18:00:08 +0000 Subject: [PATCH 3/3] 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. --- lightning/src/routing/scoring.rs | 163 +++++++++++++++++-------------- lightning/src/util/test_utils.rs | 3 +- 2 files changed, 92 insertions(+), 74 deletions(-) diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs index eb522a48ae5..e478dbcbede 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 45f21e0b38c..e2e44a2c1a6 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};