Use RUSTFLAGS instead of feature to gate benching

Currently we use the `"_bench_unstable"` feature to gate benching code.
This results in features not being additive for stable toolchains i.e.,
one cannot currently run `cargo +stable check --all-features`. Although
this method of gating bench code is widely proposed as a good solution
online there is another solution that keeps all features additive.

Guard the bench code with `#[cfg(bench)]` and when running benches use
`RUSTFLAGS='--cfg=bench'` to turn on `bench`.

Author: tcharding

.github/workflows/build.yml

@@ -273,7 +273,8 @@ jobs:
           cd ..
       - name: Run benchmarks on Rust ${{ matrix.toolchain }}
         run: |
-          RUSTC_BOOTSTRAP=1 cargo bench --features _bench_unstable
+
+          RUSTC_BOOTSTRAP=1 RUSTFLAGS='--cfg=bench' cargo bench
 
   check_commits:
     runs-on: ubuntu-latest

lightning-persister/Cargo.toml

@@ -14,7 +14,6 @@ all-features = true
 rustdoc-args = ["--cfg", "docsrs"]
 
 [features]
-_bench_unstable = ["lightning/_bench_unstable"]
 
 [dependencies]
 bitcoin = "0.29.0"

lightning-persister/src/lib.rs

@@ -8,8 +8,8 @@
 
 #![cfg_attr(docsrs, feature(doc_auto_cfg))]
 
-#![cfg_attr(all(test, feature = "_bench_unstable"), feature(test))]
-#[cfg(all(test, feature = "_bench_unstable"))] extern crate test;
+#![cfg_attr(bench, feature(test))]
+#[cfg(bench)] extern crate test;
 
 mod util;
 
@@ -316,7 +316,7 @@ mod tests {
 	}
 }
 
-#[cfg(all(test, feature = "_bench_unstable"))]
+#[cfg(bench)]
 pub mod bench {
 	use test::Bencher;
 

lightning-rapid-gossip-sync/Cargo.toml

@@ -13,7 +13,6 @@ Utility to process gossip routing data from Rapid Gossip Sync Server.
 default = ["std"]
 no-std = ["lightning/no-std"]
 std = ["lightning/std"]
-_bench_unstable = []
 
 [dependencies]
 lightning = { version = "0.0.112", path = "../lightning", default-features = false }

lightning-rapid-gossip-sync/src/lib.rs

@@ -62,10 +62,8 @@
 
 #![cfg_attr(all(not(feature = "std"), not(test)), no_std)]
 
-// Allow and import test features for benching
-#![cfg_attr(all(test, feature = "_bench_unstable"), feature(test))]
-#[cfg(all(test, feature = "_bench_unstable"))]
-extern crate test;
+#![cfg_attr(bench, feature(test))]
+#[cfg(bench)] extern crate test;
 
 #[cfg(not(feature = "std"))]
 extern crate alloc;
@@ -278,7 +276,7 @@ mod tests {
 	}
 }
 
-#[cfg(all(test, feature = "_bench_unstable"))]
+#[cfg(bench)]
 pub mod bench {
 	use test::Bencher;
 

lightning/Cargo.toml

@@ -28,7 +28,6 @@ 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 = []
-_bench_unstable = []
 
 no-std = ["hashbrown", "bitcoin/no-std", "core2/alloc"]
 std = ["bitcoin/std"]

lightning/src/lib.rs

@@ -54,8 +54,8 @@
 
 #![cfg_attr(all(not(feature = "std"), not(test)), no_std)]
 
-#![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_attr(bench), feature(test)]
+#[cfg(bench)] 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");
@@ -175,18 +175,18 @@ mod prelude {
 	pub use alloc::string::ToString;
 }
 
-#[cfg(all(not(feature = "_bench_unstable"), feature = "std", test))]
+#[cfg(all(not(bench), feature = "std", test))]
 mod debug_sync;
-#[cfg(all(not(feature = "_bench_unstable"), feature = "backtrace", feature = "std", test))]
+#[cfg(all(not(bench), feature = "backtrace", feature = "std", test))]
 extern crate backtrace;
 
 #[cfg(feature = "std")]
 mod sync {
-	#[cfg(all(not(feature = "_bench_unstable"), test))]
+	#[cfg(all(not(bench), test))]
 	pub use crate::debug_sync::*;
-	#[cfg(any(feature = "_bench_unstable", not(test)))]
+	#[cfg(any(bench, not(test)))]
 	pub use ::std::sync::{Arc, Mutex, Condvar, MutexGuard, RwLock, RwLockReadGuard, RwLockWriteGuard};
-	#[cfg(any(feature = "_bench_unstable", not(test)))]
+	#[cfg(any(bench, not(test)))]
 	pub use crate::util::fairrwlock::FairRwLock;
 }
 

lightning/src/ln/channelmanager.rs

@@ -8139,7 +8139,7 @@ mod tests {
 	}
 }
 
-#[cfg(all(any(test, feature = "_test_utils"), feature = "_bench_unstable"))]
+#[cfg(bench)]
 pub mod bench {
 	use crate::chain::Listen;
 	use crate::chain::chainmonitor::{ChainMonitor, Persist};

lightning/src/ln/functional_test_utils.rs

@@ -1470,7 +1470,7 @@ macro_rules! expect_pending_htlcs_forwardable_from_events {
 	}}
 }
 #[macro_export]
-#[cfg(any(test, feature = "_bench_unstable", feature = "_test_utils"))]
+#[cfg(any(test, bench, feature = "_test_utils"))]
 macro_rules! expect_payment_received {
 	($node: expr, $expected_payment_hash: expr, $expected_payment_secret: expr, $expected_recv_value: expr) => {
 		expect_payment_received!($node, $expected_payment_hash, $expected_payment_secret, $expected_recv_value, None, $node.node.get_our_node_id())
@@ -1497,7 +1497,7 @@ macro_rules! expect_payment_received {
 }
 
 #[macro_export]
-#[cfg(any(test, feature = "_bench_unstable", feature = "_test_utils"))]
+#[cfg(any(test, bench, feature = "_test_utils"))]
 macro_rules! expect_payment_claimed {
 	($node: expr, $expected_payment_hash: expr, $expected_recv_value: expr) => {
 		let events = $node.node.get_and_clear_pending_events();
@@ -1603,7 +1603,7 @@ macro_rules! expect_payment_forwarded {
 	}
 }
 
-#[cfg(any(test, feature = "_bench_unstable", feature = "_test_utils"))]
+#[cfg(any(test, bench, feature = "_test_utils"))]
 pub fn expect_channel_ready_event<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, expected_counterparty_node_id: &PublicKey) {
 	let events = node.node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 1);

lightning/src/routing/gossip.rs

@@ -3280,7 +3280,7 @@ mod tests {
 	}
 }
 
-#[cfg(all(test, feature = "_bench_unstable"))]
+#[cfg(bench)]
 mod benches {
 	use super::*;
 

lightning/src/routing/router.rs

@@ -611,7 +611,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(all(not(feature = "_bench_unstable"), any(test, fuzzing)))]
+	#[cfg(all(not(bench), any(test, fuzzing)))]
 	// 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
@@ -632,7 +632,7 @@ impl<'a> core::fmt::Debug for PathBuildingHop<'a> {
 			.field("path_penalty_msat", &self.path_penalty_msat)
 			.field("path_htlc_minimum_msat", &self.path_htlc_minimum_msat)
 			.field("cltv_expiry_delta", &self.candidate.cltv_expiry_delta());
-		#[cfg(all(not(feature = "_bench_unstable"), any(test, fuzzing)))]
+		#[cfg(all(not(bench), any(test, fuzzing)))]
 		let debug_struct = debug_struct
 			.field("value_contribution_msat", &self.value_contribution_msat);
 		debug_struct.finish()
@@ -1151,14 +1151,14 @@ where L::Target: Logger {
 								path_htlc_minimum_msat,
 								path_penalty_msat: u64::max_value(),
 								was_processed: false,
-								#[cfg(all(not(feature = "_bench_unstable"), any(test, fuzzing)))]
+								#[cfg(all(not(bench), any(test, fuzzing)))]
 								value_contribution_msat,
 							}
 						});
 
 						#[allow(unused_mut)] // We only use the mut in cfg(test)
 						let mut should_process = !old_entry.was_processed;
-						#[cfg(all(not(feature = "_bench_unstable"), any(test, fuzzing)))]
+						#[cfg(all(not(bench), any(test, fuzzing)))]
 						{
 							// 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).
@@ -1253,13 +1253,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(all(not(feature = "_bench_unstable"), any(test, fuzzing)))]
+								#[cfg(all(not(bench), any(test, fuzzing)))]
 								{
 									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(all(not(feature = "_bench_unstable"), any(test, fuzzing)))]
+								#[cfg(all(not(bench), any(test, fuzzing)))]
 								{
 									// If we're skipping processing a node which was previously
 									// processed even though we found another path to it with a
@@ -5504,7 +5504,7 @@ pub(crate) mod bench_utils {
 	}
 }
 
-#[cfg(all(test, feature = "_bench_unstable", not(feature = "no-std")))]
+#[cfg(all(test, bench, not(features = "no-std")))]
 mod benches {
 	use super::*;
 	use bitcoin::hashes::Hash;

lightning/src/util/mod.rs

@@ -27,7 +27,7 @@ pub mod wakers;
 pub(crate) mod atomic_counter;
 pub(crate) mod byte_utils;
 pub(crate) mod chacha20;
-#[cfg(all(any(feature = "_bench_unstable", not(test)), feature = "std"))]
+#[cfg(all(any(bench, not(test), features = "std")))]
 pub(crate) mod fairrwlock;
 #[cfg(fuzzing)]
 pub mod zbase32;