Skip to content

Work around rustc bug on nightly and make benchmarks not run test code #1301

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.

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 2 commits into from
Feb 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ jobs:
cd ..
- name: Run benchmarks on Rust ${{ matrix.toolchain }}
run: |
cargo bench --features unstable
cargo bench --features _bench_unstable

check_commits:
runs-on: ubuntu-latest
Expand Down
2 changes: 1 addition & 1 deletion lightning-persister/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Utilities to manage Rust-Lightning channel data persistence and retrieval.
"""

[features]
unstable = ["lightning/unstable"]
_bench_unstable = ["lightning/_bench_unstable"]

[dependencies]
bitcoin = "0.27"
Expand Down
6 changes: 3 additions & 3 deletions lightning-persister/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
#![deny(broken_intra_doc_links)]
#![deny(missing_docs)]

#![cfg_attr(all(test, feature = "unstable"), feature(test))]
#[cfg(all(test, feature = "unstable"))] extern crate test;
#![cfg_attr(all(test, feature = "_bench_unstable"), feature(test))]
#[cfg(all(test, feature = "_bench_unstable"))] extern crate test;

mod util;

Expand Down Expand Up @@ -362,7 +362,7 @@ mod tests {
}
}

#[cfg(all(test, feature = "unstable"))]
#[cfg(all(test, feature = "_bench_unstable"))]
pub mod bench {
use test::Bencher;

Expand Down
2 changes: 1 addition & 1 deletion lightning/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ max_level_trace = []
# Allow signing of local transactions that may have been revoked or will be revoked, for functional testing (e.g. justice tx handling).
# This is unsafe to use in production because it may result in the counterparty publishing taking our funds.
unsafe_revoked_tx_signing = []
unstable = []
_bench_unstable = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just call this _bench? Presumably we'd still want this when no longer unstable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well the only reason we have a feature is because its unstable. If cargo bench weren't unstable we'd remove the feature entirely (though there's some indication cargo bench will never be stabilized).


no-std = ["hashbrown", "bitcoin/no-std", "core2/alloc"]
std = ["bitcoin/std"]
Expand Down
4 changes: 2 additions & 2 deletions lightning/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@

#![cfg_attr(all(not(feature = "std"), not(test)), no_std)]

#![cfg_attr(all(any(test, feature = "_test_utils"), feature = "unstable"), feature(test))]
#[cfg(all(any(test, feature = "_test_utils"), feature = "unstable"))] extern crate test;
#![cfg_attr(all(any(test, feature = "_test_utils"), feature = "_bench_unstable"), feature(test))]
#[cfg(all(any(test, feature = "_test_utils"), feature = "_bench_unstable"))] extern crate test;

#[cfg(not(any(feature = "std", feature = "no-std")))]
compile_error!("at least one of the `std` or `no-std` features must be enabled");
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7132,7 +7132,7 @@ mod tests {
}
}

#[cfg(all(any(test, feature = "_test_utils"), feature = "unstable"))]
#[cfg(all(any(test, feature = "_test_utils"), feature = "_bench_unstable"))]
pub mod bench {
use chain::Listen;
use chain::chainmonitor::{ChainMonitor, Persist};
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/ln/functional_test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1137,7 +1137,7 @@ macro_rules! expect_pending_htlcs_forwardable_from_events {
}}
}

#[cfg(any(test, feature = "unstable"))]
#[cfg(any(test, feature = "_bench_unstable"))]
macro_rules! expect_payment_received {
($node: expr, $expected_payment_hash: expr, $expected_payment_secret: expr, $expected_recv_value: expr) => {
let events = $node.node.get_and_clear_pending_events();
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/ln/msgs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -985,7 +985,7 @@ impl fmt::Display for DecodeError {
DecodeError::InvalidValue => f.write_str("Nonsense bytes didn't map to the type they were interpreted as"),
DecodeError::ShortRead => f.write_str("Packet extended beyond the provided bytes"),
DecodeError::BadLengthDescriptor => f.write_str("A length descriptor in the packet didn't describe the later data correctly"),
DecodeError::Io(ref e) => e.fmt(f),
DecodeError::Io(ref e) => fmt::Debug::fmt(e, f),
DecodeError::UnsupportedCompression => f.write_str("We don't support receiving messages with zlib-compressed fields"),
}
}
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/routing/network_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2687,7 +2687,7 @@ mod tests {
}
}

#[cfg(all(test, feature = "unstable"))]
#[cfg(all(test, feature = "_bench_unstable"))]
mod benches {
use super::*;

Expand Down
12 changes: 6 additions & 6 deletions lightning/src/routing/router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ struct PathBuildingHop<'a> {
/// decrease as well. Thus, we have to explicitly track which nodes have been processed and
/// avoid processing them again.
was_processed: bool,
#[cfg(any(test, feature = "fuzztarget"))]
#[cfg(all(not(feature = "_bench_unstable"), any(test, feature = "fuzztarget")))]
// In tests, we apply further sanity checks on cases where we skip nodes we already processed
// to ensure it is specifically in cases where the fee has gone down because of a decrease in
// value_contribution_msat, which requires tracking it here. See comments below where it is
Expand Down Expand Up @@ -896,14 +896,14 @@ where L::Target: Logger {
path_htlc_minimum_msat,
path_penalty_msat: u64::max_value(),
was_processed: false,
#[cfg(any(test, feature = "fuzztarget"))]
#[cfg(all(not(feature = "_bench_unstable"), any(test, feature = "fuzztarget")))]
value_contribution_msat,
}
});

#[allow(unused_mut)] // We only use the mut in cfg(test)
let mut should_process = !old_entry.was_processed;
#[cfg(any(test, feature = "fuzztarget"))]
#[cfg(all(not(feature = "_bench_unstable"), any(test, feature = "fuzztarget")))]
{
// In test/fuzzing builds, we do extra checks to make sure the skipping
// of already-seen nodes only happens in cases we expect (see below).
Expand Down Expand Up @@ -992,13 +992,13 @@ where L::Target: Logger {
old_entry.fee_msat = 0; // This value will be later filled with hop_use_fee_msat of the following channel
old_entry.path_htlc_minimum_msat = path_htlc_minimum_msat;
old_entry.path_penalty_msat = path_penalty_msat;
#[cfg(any(test, feature = "fuzztarget"))]
#[cfg(all(not(feature = "_bench_unstable"), any(test, feature = "fuzztarget")))]
{
old_entry.value_contribution_msat = value_contribution_msat;
}
did_add_update_path_to_src_node = true;
} else if old_entry.was_processed && new_cost < old_cost {
#[cfg(any(test, feature = "fuzztarget"))]
#[cfg(all(not(feature = "_bench_unstable"), any(test, feature = "fuzztarget")))]
{
// If we're skipping processing a node which was previously
// processed even though we found another path to it with a
Expand Down Expand Up @@ -4976,7 +4976,7 @@ pub(crate) mod test_utils {
}
}

#[cfg(all(test, feature = "unstable", not(feature = "no-std")))]
#[cfg(all(test, feature = "_bench_unstable", not(feature = "no-std")))]
mod benches {
use super::*;
use bitcoin::hashes::Hash;
Expand Down