Pre-scoring test clean-ups #1120

jkczyz · 2021-10-13T17:37:49Z

Replaces get_route with get_route_and_payment_hash in tests wherever possible, since a scorer will need to be passed to get_route. Also includes some fairly trivial test clean ups.

jkczyz · 2021-10-13T17:39:35Z

I left a few TODO comments in the primary commit where I was unsure of the change. So please take a close look at those. Everything else should be fairly mechanical.

codecov · 2021-10-13T17:53:45Z

Codecov Report

Merging #1120 (d4ec090) into main (fe8c10d) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1120      +/-   ##
==========================================
+ Coverage   90.67%   90.73%   +0.06%     
==========================================
  Files          66       66              
  Lines       34732    34808      +76     
==========================================
+ Hits        31494    31584      +90     
+ Misses       3238     3224      -14

Impacted Files	Coverage Δ
lightning-background-processor/src/lib.rs	`94.23% <ø> (ø)`
lightning/src/chain/channelmonitor.rs	`91.26% <ø> (ø)`
lightning/src/ln/functional_test_utils.rs	`95.08% <ø> (-0.01%)`	⬇️
lightning/src/ln/monitor_tests.rs	`100.00% <ø> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.67% <100.00%> (-0.11%)`	⬇️
lightning/src/ln/channelmanager.rs	`85.12% <100.00%> (-0.03%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.33% <100.00%> (-0.15%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`96.60% <100.00%> (-0.05%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.83% <100.00%> (+0.08%)`	⬆️
lightning/src/ln/shutdown_tests.rs	`95.87% <100.00%> (-0.02%)`	⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe8c10d...d4ec090. Read the comment docs.

TheBlueMatt · 2021-10-13T17:55:54Z

The "Fix need for #[allow(unused_mut)] in tests" commit is largely duplicative with "Use Persister to return errors in tests not chain::Watch " in #1108, is it possible to drop that commit here and use the one from 1108?

jkczyz · 2021-10-13T19:15:54Z

The "Fix need for #[allow(unused_mut)] in tests" commit is largely duplicative with "Use Persister to return errors in tests not chain::Watch " in #1108, is it possible to drop that commit here and use the one from 1108?

Dropped the commit.

I left a few TODO comments in the primary commit where I was unsure of the change. So please take a close look at those. Everything else should be fairly mechanical.

Reverted the marked changes to use get_route where applicable for ease of review, as requested offline. Left TODOs about changing them to get_route_and_payment_hash.

TheBlueMatt · 2021-10-13T19:20:55Z

lightning/src/ln/shutdown_tests.rs

@@ -71,6 +71,9 @@ fn pre_funding_lock_shutdown_test() {
 	check_closed_event!(nodes[1], 1, ClosureReason::CooperativeClosure);
 }

+// TODO: Determine why get_route fails when replacing get_payment_preimage_hash with


You have another todo here.

Mainly to note why it may be that a simple replacement doesn't work here. Can drop it if you prefer.

TheBlueMatt · 2021-10-13T19:25:14Z

~~Do you want to take the in-number _ additions from #1113 here? It makes things a bit easier to read in a number of places IMO.~~Actually, I'll just rebase 1113 we can do it later.

TheBlueMatt · 2021-10-13T19:56:04Z

lightning/src/ln/functional_tests.rs

-	let (_, second_payment_hash, second_payment_secret) = get_payment_preimage_hash!(nodes[2]);
+	let sending_node = if forwarded_htlc { &nodes[0] } else { &nodes[1] };
+	let (route, second_payment_hash, _, second_payment_secret) = get_route_and_payment_hash!(sending_node, nodes[2], 100000);
+	// TODO: Why use a different payment secret here?


Cause there's two different payments?

Was unsure because they both used second_payment_hash but a different secret.

Ah, I was comparing it to the first send event further up, its just a typo I believe, feel free to drop it if the test still passes and always use the second secret.

Updated to use the second secret.

TheBlueMatt · 2021-10-13T20:00:13Z

lightning/src/ln/functional_tests.rs

@@ -7906,6 +7740,8 @@ fn test_bump_penalty_txn_on_revoked_htlcs() {
 		assert_ne!(node_txn[0].input[0].previous_output, node_txn[2].input[0].previous_output);
 		assert_ne!(node_txn[1].input[0].previous_output, node_txn[2].input[0].previous_output);

+		// TODO: Determine why replacing get_route with get_route_and_payment_hash above causes


Because the tx ordering is based on the payment hash and we end up with a different hash, I assume? Or something like that, but maybe we should just drop the TODO and open a general "move the remaining explicit route calls to the routing macro" issue?

valentinewallace

ACK mod removing the todos in favor of an issue + squash

lightning/src/ln/onion_route_tests.rs

TheBlueMatt

Needs squash.

TheBlueMatt · 2021-10-13T22:33:41Z

lightning/src/ln/functional_tests.rs

@@ -9376,11 +9189,7 @@ fn test_keysend_payments_to_private_node() {
 	nodes[1].node.peer_connected(&payer_pubkey, &msgs::Init { features: InitFeatures::known() });

 	let _chan = create_chan_between_nodes(&nodes[0], &nodes[1], InitFeatures::known(), InitFeatures::known());
-	let network_graph = &nodes[0].net_graph_msg_handler.network_graph;
-	let first_hops = nodes[0].node.list_usable_channels();
-	let route = get_keysend_route(&payer_pubkey, &network_graph, &payee_pubkey,


This seems to be the only place we call get_keysend_route in tests through a private channel, so I think best to keep it.

Reverted and updated the other keysend test to use get_keysend_route.

The interface for get_route will change to take a scorer. Using get_route_and_payment_hash whenever possible allows for keeping the scorer inside get_route_and_payment_hash rather than at every call site. Replace get_route with get_route_and_payment_hash wherever possible. Additionally, update get_route_and_payment_hash to use the known invoice features and the sending node's logger.

jkczyz added 2 commits October 13, 2021 10:25

Speed up test_timer_tick_called

d7c8c8c

Fix unused variable warnings in fuzzer

3fc0352

jkczyz force-pushed the 2021-10-test-refactors branch from 49e7016 to 7719633 Compare October 13, 2021 19:09

TheBlueMatt mentioned this pull request Oct 13, 2021

Use common macro and local channel view for routing in tests #1113

Closed

TheBlueMatt reviewed Oct 13, 2021

View reviewed changes

valentinewallace reviewed Oct 13, 2021

View reviewed changes

lightning/src/ln/onion_route_tests.rs Outdated Show resolved Hide resolved

TheBlueMatt added this to the 0.0.102 milestone Oct 13, 2021

jkczyz force-pushed the 2021-10-test-refactors branch from 7719633 to b6a3f5c Compare October 13, 2021 22:05

jkczyz mentioned this pull request Oct 13, 2021

Use get_route_and_payment_hash instead of get_route #1122

Open

TheBlueMatt approved these changes Oct 13, 2021

View reviewed changes

jkczyz force-pushed the 2021-10-test-refactors branch from b6a3f5c to d700faa Compare October 13, 2021 22:27

TheBlueMatt reviewed Oct 13, 2021

View reviewed changes

jkczyz force-pushed the 2021-10-test-refactors branch from d700faa to 3e9678c Compare October 13, 2021 23:21

TheBlueMatt approved these changes Oct 13, 2021

View reviewed changes

jkczyz added 2 commits October 13, 2021 18:37

Move mpp_failure test to payment_tests.rs

d4ec090

jkczyz force-pushed the 2021-10-test-refactors branch from 3e9678c to d4ec090 Compare October 13, 2021 23:37

valentinewallace approved these changes Oct 13, 2021

View reviewed changes

TheBlueMatt merged commit da498d7 into lightningdevkit:main Oct 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pre-scoring test clean-ups #1120

Pre-scoring test clean-ups #1120

jkczyz commented Oct 13, 2021

jkczyz commented Oct 13, 2021

codecov bot commented Oct 13, 2021 •

edited

Loading

TheBlueMatt commented Oct 13, 2021

jkczyz commented Oct 13, 2021

TheBlueMatt Oct 13, 2021

jkczyz Oct 13, 2021

TheBlueMatt commented Oct 13, 2021 •

edited

Loading

TheBlueMatt Oct 13, 2021

jkczyz Oct 13, 2021

TheBlueMatt Oct 13, 2021

jkczyz Oct 13, 2021

TheBlueMatt Oct 13, 2021

jkczyz Oct 13, 2021

valentinewallace left a comment

TheBlueMatt left a comment

TheBlueMatt Oct 13, 2021

jkczyz Oct 13, 2021

Pre-scoring test clean-ups #1120

Pre-scoring test clean-ups #1120

Conversation

jkczyz commented Oct 13, 2021

jkczyz commented Oct 13, 2021

codecov bot commented Oct 13, 2021 • edited Loading

Codecov Report

TheBlueMatt commented Oct 13, 2021

jkczyz commented Oct 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 13, 2021 •

edited

Loading

TheBlueMatt commented Oct 13, 2021 •

edited

Loading