Skip to content

Use log approximation in ProbabilisticScorer #1347

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 9, 2022
Merged

Use log approximation in ProbabilisticScorer #1347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7f83c55
Use log approximation in ProbabilisticScorer
jkczyz Mar 3, 2022
d70292d
Fix compilation warnings with --features=no-std
jkczyz Mar 3, 2022
f041a64
Check for no-std compatibility across dependencies
jkczyz Mar 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,10 @@ jobs:
cargo test --verbose --color always --no-default-features --features no-std
# check if there is a conflict between no-std and the default std feature
cargo test --verbose --color always --features no-std
# check no-std compatibility across dependencies
Copy link
Member

@devrandom devrandom Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious, did you figure out why compiling in-crate doesn't uncover the issue?

Copy link
Contributor Author

@jkczyz jkczyz Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't unfortunately. In-crate compiling does catch anything using std but apparently not methods unavailable on primitive types. Maybe there is some configuration that would trigger it? I was using cargo check -p lightning --no-default-features --features=no-std.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like something we should maybe report as a rustc bug?

Copy link
Contributor Author

@jkczyz jkczyz Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, I see the compilation fail with a minimal example in Rust playground:

#![no_std]

fn main() {
    1.0_f64.log10();
}
   Compiling playground v0.0.1 (/playground)
error: language item required, but not found: `eh_personality`
  |
  = note: this can occur when a binary crate with `#![no_std]` is compiled for a target where `eh_personality` is defined in the standard library
  = help: you may be able to compile for a target that doesn't need `eh_personality`, specify a target with `--target` or in `.cargo/config`

error: `#[panic_handler]` function required, but not found

error[[E0599]](https://doc.rust-lang.org/stable/error-index.html#E0599): no method named `log10` found for type `f64` in the current scope
 [--> src/main.rs:4:13
](https://play.rust-lang.org/#)  |
4 |     1.0_f64.log10();
  |             ^^^^^ method not found in `f64`

For more information about this error, try `rustc --explain E0599`.
error: could not compile `playground` due to 3 previous errors

If I change the empty lib.rs file in no-std-check to use a minimal example, I only see the last error.

#![no_std]

pub fn log(x: f64) -> f64 {
    x.log10()
}

So I wonder if there is something about our configuration that is causing this to not be caught.

Copy link
Member

@devrandom devrandom Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps the issue is with code that is not reachable from main(), but only from tests?

Copy link
Member

@devrandom devrandom Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignore my last comment, sounds like you did try it without a call from main

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so it sounds like the bug is that rustc doesn't detect this as an issue if we're building for test/check instead of building for run?

Copy link
Contributor Author

@jkczyz jkczyz Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, rustc does catch it when using cd no-std-check && cargo check with the modified lib.rs:

    Checking no-std-check v0.1.0 (/Users/jkczyz/src/rust-lightning/no-std-check)
error[E0599]: no method named `log10` found for type `f64` in the current scope
 --> src/lib.rs:4:7
  |
4 |     x.log10()
  |       ^^^^^ method not found in `f64`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0599`.
error: could not compile `no-std-check`

To learn more, run the command again with --verbose.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so the issue is that some crates in the workspace are not no-std so everything gets built with std?

Copy link
Contributor Author

@jkczyz jkczyz Mar 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... moving the other crates to exclude doesn't seem to make a difference when modifying some code to use log10 and running cargo check --no-default-features --features=no-std. However, it does catch an added use of println.

cd ..
cd no-std-check
cargo check --verbose --color always
cd ..
- name: Test on no-std builds Rust ${{ matrix.toolchain }} and full code-linking for coverage generation
if: "matrix.build-no-std && matrix.coverage"
Expand Down
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ lightning-c-bindings/a.out
Cargo.lock
.idea
lightning/target
no-std-check/target
4 changes: 4 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ members = [
"lightning-background-processor",
]

exclude = [
"no-std-check",
]

# Our tests do actual crypo and lots of work, the tradeoff for -O1 is well worth it.
# Ideally we would only do this in profile.test, but profile.test only applies to
# the test binary, not dependencies, which means most of the critical code still
Expand Down
4 changes: 3 additions & 1 deletion lightning-invoice/src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ use lightning::chain;
use lightning::chain::chaininterface::{BroadcasterInterface, FeeEstimator};
use lightning::chain::keysinterface::{Recipient, KeysInterface, Sign};
use lightning::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
use lightning::ln::channelmanager::{ChannelDetails, ChannelManager, PaymentId, PaymentSendFailure, PhantomRouteHints, MIN_FINAL_CLTV_EXPIRY, MIN_CLTV_EXPIRY_DELTA};
use lightning::ln::channelmanager::{ChannelDetails, ChannelManager, PaymentId, PaymentSendFailure, MIN_FINAL_CLTV_EXPIRY};
#[cfg(feature = "std")]
use lightning::ln::channelmanager::{PhantomRouteHints, MIN_CLTV_EXPIRY_DELTA};
use lightning::ln::msgs::LightningError;
use lightning::routing::scoring::Score;
use lightning::routing::network_graph::{NetworkGraph, RoutingFees};
Expand Down
6 changes: 5 additions & 1 deletion lightning/src/routing/router.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1537,9 +1537,9 @@ where L::Target: Logger {

#[cfg(test)]
mod tests {
use routing::scoring::{ProbabilisticScorer, ProbabilisticScoringParameters, Score};
use routing::network_graph::{NetworkGraph, NetGraphMsgHandler, NodeId};
use routing::router::{get_route, PaymentParameters, Route, RouteHint, RouteHintHop, RouteHop, RoutingFees};
use routing::scoring::Score;
use chain::transaction::OutPoint;
use ln::features::{ChannelFeatures, InitFeatures, InvoiceFeatures, NodeFeatures};
use ln::msgs::{ErrorAction, LightningError, OptionalField, UnsignedChannelAnnouncement, ChannelAnnouncement, RoutingMessageHandler,
Expand Down Expand Up @@ -4970,6 +4970,8 @@ mod tests {
#[test]
#[cfg(not(feature = "no-std"))]
fn generate_routes() {
use routing::scoring::{ProbabilisticScorer, ProbabilisticScoringParameters};

let mut d = match super::test_utils::get_route_file() {
Ok(f) => f,
Err(e) => {
Expand Down Expand Up @@ -5002,6 +5004,8 @@ mod tests {
#[test]
#[cfg(not(feature = "no-std"))]
fn generate_routes_mpp() {
use routing::scoring::{ProbabilisticScorer, ProbabilisticScoringParameters};

let mut d = match super::test_utils::get_route_file() {
Ok(f) => f,
Err(e) => {
Expand Down
215 changes: 165 additions & 50 deletions lightning/src/routing/scoring.rs
View file
Open in desktop

Large diffs are not rendered by default.

6 changes: 6 additions & 0 deletions lightning/src/util/test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,10 @@ impl events::MessageSendEventsProvider for TestRoutingMessageHandler {

pub struct TestLogger {
level: Level,
#[cfg(feature = "std")]
id: String,
#[cfg(not(feature = "std"))]
_id: String,
pub lines: Mutex<HashMap<(String, String), usize>>,
}

Expand All @@ -422,7 +425,10 @@ impl TestLogger {
pub fn with_id(id: String) -> TestLogger {
TestLogger {
level: Level::Trace,
#[cfg(feature = "std")]
id,
#[cfg(not(feature = "std"))]
_id: id,
lines: Mutex::new(HashMap::new())
}
}
Expand Down
11 changes: 11 additions & 0 deletions no-std-check/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
[package]
name = "no-std-check"
version = "0.1.0"
edition = "2018"

[features]
default = ["lightning/no-std", "lightning-invoice/no-std"]

[dependencies]
lightning = { path = "../lightning", default-features = false }
lightning-invoice = { path = "../lightning-invoice", default-features = false }
Empty file added no-std-check/src/lib.rs
View file
Open in desktop
Empty file.