From 358d980e643541ac59dce601fec3d2ac11034a04 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 17 Jan 2023 23:39:41 +0000 Subject: [PATCH 1/2] Always set `_test_utils` when building lightning for some tests This ensures that we hit additional assertions which are intended to always be run in tests. --- fuzz/Cargo.toml | 2 +- lightning-block-sync/Cargo.toml | 1 + lightning-net-tokio/Cargo.toml | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index b0b6910ae2f..5daebb35066 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -18,7 +18,7 @@ libfuzzer_fuzz = ["libfuzzer-sys"] stdin_fuzz = [] [dependencies] -lightning = { path = "../lightning", features = ["regex", "hashbrown"] } +lightning = { path = "../lightning", features = ["regex", "hashbrown", "_test_utils"] } lightning-rapid-gossip-sync = { path = "../lightning-rapid-gossip-sync" } bitcoin = { version = "0.29.0", features = ["secp-lowmemory"] } hex = "0.3" diff --git a/lightning-block-sync/Cargo.toml b/lightning-block-sync/Cargo.toml index 8b185ff3bab..c605f5ef3a2 100644 --- a/lightning-block-sync/Cargo.toml +++ b/lightning-block-sync/Cargo.toml @@ -27,4 +27,5 @@ serde_json = { version = "1.0", optional = true } chunked_transfer = { version = "1.4", optional = true } [dev-dependencies] +lightning = { version = "0.0.113", path = "../lightning", features = ["_test_utils"] } tokio = { version = "~1.14", features = [ "macros", "rt" ] } diff --git a/lightning-net-tokio/Cargo.toml b/lightning-net-tokio/Cargo.toml index 07f9f313307..8a518638c5f 100644 --- a/lightning-net-tokio/Cargo.toml +++ b/lightning-net-tokio/Cargo.toml @@ -21,3 +21,4 @@ tokio = { version = "1.0", features = [ "io-util", "macros", "rt", "sync", "net" [dev-dependencies] tokio = { version = "~1.14", features = [ "io-util", "macros", "rt", "rt-multi-thread", "sync", "net", "time" ] } +lightning = { version = "0.0.113", path = "../lightning", features = ["_test_utils"] } From 7a9bea1bdd6ebda97086d2cd559efb234e4e7e46 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 17 Jan 2023 23:40:44 +0000 Subject: [PATCH 2/2] Use `test`/`_test_utils` to enable single-threaded debug assertions We have a number of debug assertions which are expected to never fire when running in a single thread. This is just fine in tests, and gives us good coverage of our lockorder requirements, but is not-irregularly surprising to users, who may run with their own debug assertions in test environments. Instead, we gate these checks by the `cfg(test)` setting as well as the `_test_utils` feature, ensuring they run in our own tests, but not downstream tests. --- lightning-net-tokio/src/lib.rs | 12 ++++++------ lightning/src/ln/channelmanager.rs | 16 ++++++++-------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/lightning-net-tokio/src/lib.rs b/lightning-net-tokio/src/lib.rs index bd6f13db85c..39452cff034 100644 --- a/lightning-net-tokio/src/lib.rs +++ b/lightning-net-tokio/src/lib.rs @@ -286,7 +286,7 @@ pub fn setup_inbound(peer_manager: Arc(peer_manager: Arc(peer_manager: Arc(peer_manager: Arc Ok(msg), Err(MsgHandleErrInternal { err, chan_id, shutdown_finish }) => { - #[cfg(debug_assertions)] + #[cfg(any(feature = "_test_utils", test))] { // In testing, ensure there are no deadlocks where the lock is already held upon // entering the macro. - assert!($self.pending_events.try_lock().is_ok()); - assert!($self.per_peer_state.try_write().is_ok()); + debug_assert!($self.pending_events.try_lock().is_ok()); + debug_assert!($self.per_peer_state.try_write().is_ok()); } let mut msg_events = Vec::with_capacity(2); @@ -1193,7 +1193,7 @@ macro_rules! handle_error { let mut peer_state = peer_state_mutex.lock().unwrap(); peer_state.pending_msg_events.append(&mut msg_events); } - #[cfg(debug_assertions)] + #[cfg(any(feature = "_test_utils", test))] { if let None = per_peer_state.get(&$counterparty_node_id) { // This shouldn't occour in tests unless an unkown counterparty_node_id @@ -1206,10 +1206,10 @@ macro_rules! handle_error { => { assert_eq!(*data, expected_error_str); if let Some((err_channel_id, _user_channel_id)) = chan_id { - assert_eq!(*channel_id, err_channel_id); + debug_assert_eq!(*channel_id, err_channel_id); } } - _ => panic!("Unexpected event"), + _ => debug_assert!(false, "Unexpected event"), } } } @@ -3565,7 +3565,7 @@ where /// Fails an HTLC backwards to the sender of it to us. /// Note that we do not assume that channels corresponding to failed HTLCs are still available. fn fail_htlc_backwards_internal(&self, source: &HTLCSource, payment_hash: &PaymentHash, onion_error: &HTLCFailReason, destination: HTLCDestination) { - #[cfg(debug_assertions)] + #[cfg(any(feature = "_test_utils", test))] { // Ensure that no peer state channel storage lock is not held when calling this // function. @@ -3574,7 +3574,7 @@ where // this function with any `per_peer_state` peer lock aquired would. let per_peer_state = self.per_peer_state.read().unwrap(); for (_, peer) in per_peer_state.iter() { - assert!(peer.try_lock().is_ok()); + debug_assert!(peer.try_lock().is_ok()); } }