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

@@ -141,7 +141,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;
 
@@ -338,7 +338,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.115", path = "../lightning", default-features = false }

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

@@ -64,10 +64,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;
@@ -287,7 +285,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 = []
 # Override signing to not include randomness when generating signatures for test vectors.
 _test_vectors = []
 

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");
@@ -177,7 +177,7 @@ mod prelude {
 	pub use alloc::string::ToString;
 }
 
-#[cfg(all(not(feature = "_bench_unstable"), feature = "backtrace", feature = "std", test))]
+#[cfg(all(not(bench), feature = "backtrace", feature = "std", test))]
 extern crate backtrace;
 
 mod sync;

lightning/src/ln/channelmanager.rs

@@ -9266,7 +9266,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

@@ -1769,7 +1769,7 @@ macro_rules! get_route_and_payment_hash {
 }
 
 #[macro_export]
-#[cfg(any(test, feature = "_bench_unstable", feature = "_test_utils"))]
+#[cfg(any(test, bench, feature = "_test_utils"))]
 macro_rules! expect_payment_claimable {
 	($node: expr, $expected_payment_hash: expr, $expected_payment_secret: expr, $expected_recv_value: expr) => {
 		expect_payment_claimable!($node, $expected_payment_hash, $expected_payment_secret, $expected_recv_value, None, $node.node.get_our_node_id())
@@ -1796,7 +1796,7 @@ macro_rules! expect_payment_claimable {
 }
 
 #[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();
@@ -1913,7 +1913,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_pending_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);
@@ -1925,7 +1925,7 @@ pub fn expect_channel_pending_event<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>,
 	}
 }
 
-#[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

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

lightning/src/routing/router.rs

@@ -1015,7 +1015,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
@@ -1036,7 +1036,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()
@@ -1570,14 +1570,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).
@@ -1648,13 +1648,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
@@ -6079,7 +6079,7 @@ pub(crate) mod bench_utils {
 	}
 }
 
-#[cfg(all(test, feature = "_bench_unstable", not(feature = "no-std")))]
+#[cfg(all(bench, not(features = "no-std")))]
 mod benches {
 	use super::*;
 	use bitcoin::hashes::Hash;

lightning/src/sign/mod.rs

@@ -1628,7 +1628,7 @@ pub fn dyn_sign() {
 	let _signer: Box<dyn EcdsaChannelSigner>;
 }
 
-#[cfg(all(test, feature = "_bench_unstable", not(feature = "no-std")))]
+#[cfg(all(test, bench, not(feature = "no-std")))]
 mod benches {
 	use std::sync::{Arc, mpsc};
 	use std::sync::mpsc::TryRecvError;

lightning/src/sync/mod.rs

@@ -3,7 +3,7 @@
 pub(crate) enum LockHeldState {
 	HeldByThread,
 	NotHeldByThread,
-	#[cfg(any(feature = "_bench_unstable", not(test)))]
+	#[cfg(any(bench, not(test)))]
 	Unsupported,
 }
 
@@ -20,20 +20,20 @@ pub(crate) trait LockTestExt<'a> {
 	fn unsafe_well_ordered_double_lock_self(&'a self) -> Self::ExclLock;
 }
 
-#[cfg(all(feature = "std", not(feature = "_bench_unstable"), test))]
+#[cfg(all(feature = "std", not(bench), test))]
 mod debug_sync;
-#[cfg(all(feature = "std", not(feature = "_bench_unstable"), test))]
+#[cfg(all(feature = "std", not(bench), test))]
 pub use debug_sync::*;
-#[cfg(all(feature = "std", not(feature = "_bench_unstable"), test))]
+#[cfg(all(feature = "std", not(bench), test))]
 // Note that to make debug_sync's regex work this must not contain `debug_string` in the module name
 mod test_lockorder_checks;
 
-#[cfg(all(feature = "std", any(feature = "_bench_unstable", not(test))))]
+#[cfg(all(feature = "std", any(bench, not(test))))]
 pub(crate) mod fairrwlock;
-#[cfg(all(feature = "std", any(feature = "_bench_unstable", not(test))))]
+#[cfg(all(feature = "std", any(bench, not(test))))]
 pub use {std::sync::{Arc, Mutex, Condvar, MutexGuard, RwLock, RwLockReadGuard, RwLockWriteGuard}, fairrwlock::FairRwLock};
 
-#[cfg(all(feature = "std", any(feature = "_bench_unstable", not(test))))]
+#[cfg(all(feature = "std", any(bench, not(test))))]
 mod ext_impl {
 	use super::*;
 	impl<'a, T: 'a> LockTestExt<'a> for Mutex<T> {