Make router benchmarks more realistic by not running test-only code

`cargo bench` sets `cfg(test)`, causing us to hit some test-only
code in the router when benchmarking, throwing off our benchmarks
substantially. Here we swap from the `unstable` feature to a more
clearly internal feature (`_bench_unstable`) and also checking for
it when enabling test-only code.

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index fef042beee6a79f7c2e5812890b698641e093834..c982060a8c5cab14c7d077ff123314e4607d91ab 100644 (file)
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -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

diff --git a/lightning-persister/Cargo.toml b/lightning-persister/Cargo.toml
index 27f772cf326e84574bfd2cbfd70c6c9360d84ccf..79ffeeeb281ad1e28ef190212c5185b56225f4a3 100644 (file)
--- a/lightning-persister/Cargo.toml
+++ b/lightning-persister/Cargo.toml
@@ -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"

diff --git a/lightning-persister/src/lib.rs b/lightning-persister/src/lib.rs
index b853b5796b6e7e226fb2efae1d00495ef5de389d..558f4b8fe3cee9d56b75b2230f203ccca6608e9c 100644 (file)
--- a/lightning-persister/src/lib.rs
+++ b/lightning-persister/src/lib.rs
@@ -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;
 
@@ -362,7 +362,7 @@ mod tests {
        }
 }
 
-#[cfg(all(test, feature = "unstable"))]
+#[cfg(all(test, feature = "_bench_unstable"))]
 pub mod bench {
        use test::Bencher;
 

diff --git a/lightning/Cargo.toml b/lightning/Cargo.toml
index 135d42a6a81ced0a147775ca9e352d8dcaf0fdc8..c5d0c5564afb9249784276c2df2567a57ebcf084 100644 (file)
--- a/lightning/Cargo.toml
+++ b/lightning/Cargo.toml
@@ -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 = []
 
 no-std = ["hashbrown", "bitcoin/no-std", "core2/alloc"]
 std = ["bitcoin/std"]

diff --git a/lightning/src/lib.rs b/lightning/src/lib.rs
index ca18d7bb5cc0ba5ae64eadb5310e023d62b634b2..8fdf63ffad0e7f370359a9a3922490f939d05587 100644 (file)
--- a/lightning/src/lib.rs
+++ b/lightning/src/lib.rs
@@ -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");

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 451d6d5f8efab76b6273f91927eee7c091c004e1..2b0e4a0becca6fed78ad6f4cbc56e2df0c03b07e 100644 (file)
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -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};

diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs
index e8931c1edc00f9f0f3156e3515c512e594e963e2..b0ec079479a2f3afc71ea95e7bc767cfdea0239d 100644 (file)
--- a/lightning/src/ln/functional_test_utils.rs
+++ b/lightning/src/ln/functional_test_utils.rs
@@ -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();

diff --git a/lightning/src/routing/network_graph.rs b/lightning/src/routing/network_graph.rs
index ee310830afa93ee8e4bc55cccba603cf26897ac2..5a84f35c28dd4f1990ef0eda4611c53a139cb615 100644 (file)
--- a/lightning/src/routing/network_graph.rs
+++ b/lightning/src/routing/network_graph.rs
@@ -2687,7 +2687,7 @@ mod tests {
        }
 }
 
-#[cfg(all(test, feature = "unstable"))]
+#[cfg(all(test, feature = "_bench_unstable"))]
 mod benches {
        use super::*;
 

diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index a21cf79a144dfecf47603da633f887992b068d5b..dc2f8fd1196a1848af7390af2bc7642c9265f1df 100644 (file)
--- a/lightning/src/routing/router.rs
+++ b/lightning/src/routing/router.rs
@@ -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
@@ -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).
@@ -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
@@ -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;