Skip to content

Correct test lifetimes #2500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

This fixes two test lifetime issues which caused #2441 to break build (for reasons we don't quite understand).

For some reason an unrelated PR caused all our tests with
`reload_node` calls to fail to compile. This is due, in part, to
the lifetimes on `reload_node` implying that the new and original
`ChannelManager` (or some of the structs they reference) must live
for the same lifetime.

This fixes that issue by correcting the lifetimes to be consistent
across `Node` and `_reload_node`.
arik-so
arik-so previously approved these changes Aug 15, 2023
Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but we're missing a few more:

diff --git a/lightning/src/ln/priv_short_conf_tests.rs b/lightning/src/ln/priv_short_conf_tests.rs
index 72399c83..4b57efae 100644
--- a/lightning/src/ln/priv_short_conf_tests.rs
+++ b/lightning/src/ln/priv_short_conf_tests.rs
@@ -42,9 +42,9 @@ fn test_priv_forwarding_rejection() {
 	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
 	let mut no_announce_cfg = test_default_channel_config();
 	no_announce_cfg.accept_forwards_to_priv_channels = false;
-	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, Some(no_announce_cfg), None]);
 	let persister: test_utils::TestPersister;
 	let new_chain_monitor: test_utils::TestChainMonitor;
+	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, Some(no_announce_cfg), None]);
 	let nodes_1_deserialized: ChannelManager<&test_utils::TestChainMonitor, &test_utils::TestBroadcaster, &test_utils::TestKeysInterface, &test_utils::TestKeysInterface, &test_utils::TestKeysInterface, &test_utils::TestFeeEstimator, &test_utils::TestRouter, &test_utils::TestLogger>;
 	let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
 
diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs
index d2ab2a86..0a54b3c7 100644
--- a/lightning/src/ln/reload_tests.rs
+++ b/lightning/src/ln/reload_tests.rs
@@ -502,11 +502,11 @@ fn do_test_data_loss_protect(reconnect_panicing: bool) {
 	// We broadcast during Drop because chanmon is out of sync with chanmgr, which would cause a panic
 	// during signing due to revoked tx
 	chanmon_cfgs[0].keys_manager.disable_revocation_policy_check = true;
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 	let persister;
 	let new_chain_monitor;
-	let nodes_0_deserialized;
-	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let nodes_0_deserialized;
 	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
 	let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1000000, 1000000);
@@ -629,9 +629,9 @@ fn test_forwardable_regen() {
 
 	let chanmon_cfgs = create_chanmon_cfgs(3);
 	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
-	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
 	let persister: test_utils::TestPersister;
 	let new_chain_monitor: test_utils::TestChainMonitor;
+	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
 	let nodes_1_deserialized: ChannelManager<&test_utils::TestChainMonitor, &test_utils::TestBroadcaster, &test_utils::TestKeysInterface, &test_utils::TestKeysInterface, &test_utils::TestKeysInterface, &test_utils::TestFeeEstimator, &test_utils::TestRouter, &test_utils::TestLogger>;
 	let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
 	let chan_id_1 = create_announced_chan_between_nodes(&nodes, 0, 1).2;
@@ -716,10 +716,10 @@ fn do_test_partial_claim_before_restart(persist_both_monitors: bool) {
 	// definitely claimed.
 	let chanmon_cfgs = create_chanmon_cfgs(4);
 	let node_cfgs = create_node_cfgs(4, &chanmon_cfgs);
-	let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]);
-
 	let persister: test_utils::TestPersister;
 	let new_chain_monitor: test_utils::TestChainMonitor;
+	let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]);
+
 	let nodes_3_deserialized: ChannelManager<&test_utils::TestChainMonitor, &test_utils::TestBroadcaster, &test_utils::TestKeysInterface, &test_utils::TestKeysInterface, &test_utils::TestKeysInterface, &test_utils::TestFeeEstimator, &test_utils::TestRouter, &test_utils::TestLogger>;
 
 	let mut nodes = create_network(4, &node_cfgs, &node_chanmgrs);

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 71.42% and no project coverage change.

Comparison is base (a24626b) 90.40% compared to head (5f44887) 90.41%.
Report is 2 commits behind head on main.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2500   +/-   ##
=======================================
  Coverage   90.40%   90.41%           
=======================================
  Files         106      106           
  Lines       56328    56319    -9     
  Branches    56328    56319    -9     
=======================================
- Hits        50926    50923    -3     
+ Misses       5402     5396    -6     
Files Changed Coverage Δ
lightning/src/ln/chanmon_update_fail_tests.rs 98.69% <ø> (ø)
lightning/src/ln/reorg_tests.rs 100.00% <ø> (ø)
lightning/src/ln/functional_test_utils.rs 88.88% <42.85%> (+0.53%) ⬆️
lightning/src/ln/monitor_tests.rs 98.52% <100.00%> (ø)
lightning/src/ln/onion_route_tests.rs 98.51% <100.00%> (ø)
lightning/src/ln/payment_tests.rs 97.40% <100.00%> (ø)
lightning/src/ln/reload_tests.rs 95.69% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

When reloading a node in the test framework, we end up with a new
`ChannelManager` that has references to various test util structs.
In order for the tests to compile reliably in the face of unrelated
changes, those test structs need to always be initialized before
both the new but also the original `ChannelManager`.

Here we make that change.
@TheBlueMatt
Copy link
Collaborator Author

Grrr, I dunno how I did that, sorry about that. Pushed a pre-squashed version fixing those 4 cases.

@TheBlueMatt TheBlueMatt merged commit 9b13862 into lightningdevkit:main Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants