Tests: take netgraph locks after peer_state
What the developer wrote
Authored by Valentine Wallace
Tests: take netgraph locks after peer_state
Previously we were taking the NetworkGraph::{channels,nodes} locks before
ChannelManager::per_peer_state's peer_state locks in some tests, which violated
a lock order requirement we have in ChannelManager to take netgraph locks
*after* peer_state locks. See OffersMessageFlow::path_for_release_htlc which is
called while a peer_state lock is held and takes netgraph locks while creating
blinded paths.
What changed, and why it matters
This commit fixes the order in which internal locks are acquired inside test code only. It prevents a potential deadlock during tests, but does not change any production code that an attacker could exploit. The change makes tests follow the same lock-ordering rule already required by the main ChannelManager code.
No production action required. Ensure the project's CI and test suite run these updated tests to confirm the lock-order fix eliminates any intermittent test deadlocks. Consider adding a lint or runtime lock-order checker if not already present to catch similar test-only violations.
Security signals we found
Lock-order correction in test code
Potential deadlock avoidance (test-only)
Alignment of test helpers with production lock-order invariant
No production code changes
Evidence from the diff
The patch moves calls to list_usable_channels() (which takes ChannelManager::per_peer_state peer_state locks) before network_graph.read_only() (which takes NetworkGraph locks) in three test files. Previously the lock order was netgraph-first in these tests, which violated the production rule that peer_state must be locked before netgraph locks. The production code path OffersMessageFlow::path_for_release_htlc already relies on this ordering when creating blinded paths while holding a peer_state lock. The change is purely in test helpers/macros and reduces the risk of test deadlocks or of tests masking real lock-order bugs.
Changed components
lightning/src/ln/functional_test_utils.rslightning/src/ln/onion_route_tests.rslightning/src/ln/payment_tests.rsInspect captured patch +6 / −3
diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs
index 564edaa..5dd1414 100644
--- a/lightning/src/ln/functional_test_utils.rs
+++ b/lightning/src/ln/functional_test_utils.rs
@@ -2647,11 +2647,12 @@ pub fn get_route(send_node: &Node, route_params: &RouteParameters) -> Result<Rou
let scorer = TestScorer::new();
let keys_manager = TestKeysInterface::new(&[0u8; 32], Network::Testnet);
let random_seed_bytes = keys_manager.get_secure_random_bytes();
+ let first_hops = send_node.node.list_usable_channels();
router::get_route(
&send_node.node.get_our_node_id(),
route_params,
&send_node.network_graph.read_only(),
- Some(&send_node.node.list_usable_channels().iter().collect::<Vec<_>>()),
+ Some(&first_hops.iter().collect::<Vec<_>>()),
send_node.logger,
&scorer,
&Default::default(),
diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs
index 28b2151..f4cfb9e 100644
--- a/lightning/src/ln/onion_route_tests.rs
+++ b/lightning/src/ln/onion_route_tests.rs
@@ -2392,6 +2392,7 @@ macro_rules! get_phantom_route {
])])
.unwrap();
let scorer = test_utils::TestScorer::new();
+ let first_hops = $nodes[0].node.list_usable_channels();
let network_graph = $nodes[0].network_graph.read_only();
let route_params = RouteParameters::from_payment_params_and_value(payment_params, $amt);
(
@@ -2399,7 +2400,7 @@ macro_rules! get_phantom_route {
&$nodes[0].node.get_our_node_id(),
&route_params,
&network_graph,
- Some(&$nodes[0].node.list_usable_channels().iter().collect::<Vec<_>>()),
+ Some(&first_hops.iter().collect::<Vec<_>>()),
$nodes[0].logger,
&scorer,
&Default::default(),
diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs
index 18fb33a..0867fef 100644
--- a/lightning/src/ln/payment_tests.rs
+++ b/lightning/src/ln/payment_tests.rs
@@ -1495,11 +1495,12 @@ fn get_ldk_payment_preimage() {
let keys_manager = test_utils::TestKeysInterface::new(&[0u8; 32], Network::Testnet);
let random_seed_bytes = keys_manager.get_secure_random_bytes();
let route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat);
+ let first_hops = nodes[0].node.list_usable_channels();
let route = get_route(
&node_a_id,
&route_params,
&nodes[0].network_graph.read_only(),
- Some(&nodes[0].node.list_usable_channels().iter().collect::<Vec<_>>()),
+ Some(&first_hops.iter().collect::<Vec<_>>()),
nodes[0].logger,
&scorer,
&Default::default(),
Why this scored 17/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.