Run rustfmt on `payment_tests` and `functional_tests` and split up the latter #3751

TheBlueMatt · 2025-04-28T14:52:14Z

functional_tests' time has come, and there's some easy batches of tests to move into separate files. Even at the end its still 10k LoC, but that's better than the 14K it started as (let alone how long it'd be if it were naively rustfmt'd).

`funtional_tests.rs` has gotten incredibly huge over the years, so here we move some channel reserve tests into a new file.

`funtional_tests.rs` has gotten incredibly huge over the years, so here we move some further channel reserve tests into a new file. We do so separately from the previous commit to ensure git identifies the changes as move-only.

`funtional_tests.rs` has gotten incredibly huge over the years, so here we move some simple HTLC unit tests into a new file.

`funtional_tests.rs` has gotten incredibly huge over the years, so here we move all the channel fee-related tests out to their own file.

ldk-reviews-bot · 2025-04-28T14:52:16Z

👋 Thanks for assigning @joostjager as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

In general we shouldn't have tests lying around that are useless as its dead code and may break due to harmless protocol changes. Here, we drop a few useless tests.

joostjager

First of all, I’m incredibly impressed by your ability to stay focused and produce a change set like this. Maybe you used some refactoring tooling or search/replace, but even so, it's impressive. As a reviewer, I’m quite sure I have done a worse job. This kind of diff is hard to stay attentive to, and I haven’t verified every single repeated change in detail.

Secondly, given the potential for merge conflicts, it's probably best not to leave this PR open longer than necessary.

I understand why you want these refactors before applying rustfmt, but I think it’s important to acknowledge that many of these changes are a matter of personal preference rather than necessity. Some of them might even reduce readability, depending on who you ask.

There’s also some risk involved. As mentioned, this kind of work demands a lot of attention from both the author and reviewers. While the impact is lower for tests, I would be hesitant to apply a similar approach to non-test code. Fixing formatting issues in the normal flow of PRs feels much safer.

All that said, this work is done now, and despite the subjective aspects, I believe this PR is a net positive change. Still, it reinforces my opinion that for the rest of the codebase, we should move forward with #3749.

joostjager · 2025-04-29T08:06:52Z

lightning/src/ln/payment_tests.rs

+	let params = route.route_params.clone().unwrap();
+	let onion = RecipientOnionFields::spontaneous_empty();
+	let retry = Retry::Attempts(0);
+	nodes[0].node.send_spontaneous_payment(preimage, onion, payment_id_0, params, retry).unwrap();


Whether extractions into short var names is an improvement or not, is subjective. Especially with an IDE that whispers argument names, I prefer not to have the indirection through these short vars.

I am totally fine with what it was:

Would agree with this, in some cases there's a loss of information in the new var name (e.g. removing _msat suffix, secret vs payment_secret) which doesn't seem worth it. It also makes it less clear that the variable isn't used elsewhere.

joostjager · 2025-04-29T08:09:21Z

lightning/src/ln/payment_tests.rs

-	let third_persister;
-	let third_new_chain_monitor;
+	let persist_1;
+	let chain_1;


This might be considered loss of information / readability. It think it is valid to have a preference for more vertical layout instead.

joostjager · 2025-04-29T08:24:32Z

lightning/src/ln/payment_tests.rs

+	let secret = Some(payment_secret);
+	let path = &[&nodes[1]];
+	pass_along_path(&nodes[0], path, amt_msat, payment_hash, secret, event, true, Some(preimage));
+	claim_payment_along_route(ClaimAlongRouteArgs::new(&nodes[0], &[path], preimage));


For this type PR, an alternative commit structure would be to first 'rustfmt' and then fix problems in a later commit. That way as a reviewer it's possible to see how the formatting was improved by looking at the diff.

For this review, I kept a local 'naively' rustfmt'ed copy on the side to glance at occassionally.

joostjager · 2025-04-29T08:35:27Z

lightning/src/ln/functional_tests.rs


 	let (temp_chan_id, tx, funding_output) =
-		create_funding_transaction(&nodes[0], &nodes[1].node.get_our_node_id(), 1_000_000, 42);
+		create_funding_transaction(&nodes[0], &node_b_id, 1_000_000, 42);


Mixing node numbers and letters isn't ideal I think. node_0_id etc would have been consistent.

joostjager · 2025-04-29T08:38:55Z

lightning/src/ln/htlc_reserve_unit_tests.rs

@@ -505,3 +509,433 @@ pub fn channel_reserve_in_flight_removes() {
 	claim_payment(&nodes[1], &[&nodes[0]], payment_preimage_4);
 	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_3);
 }
+
+#[xtest(feature = "_externalize_tests")]


Commit msg typo, funtional_tests

joostjager · 2025-04-29T09:01:38Z

lightning/src/ln/functional_tests.rs

-	assert!(nodes[0].node.create_channel(node_b_id, channel_value_satoshis, push_msat, 42, None, None).is_ok()); //Create a valid channel
-	let node0_to_1_send_open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, node_b_id);
-	assert!(node0_to_1_send_open_channel.channel_reserve_satoshis>=node0_to_1_send_open_channel.common_fields.dust_limit_satoshis);
+	nodes[0].node.create_channel(node_b_id, 100_000, 0, 42, None, None).unwrap();


Explain in the commit message why it is dead code?

joostjager · 2025-04-29T09:17:05Z

lightning/src/ln/update_fee_tests.rs

@@ -62,7 +62,7 @@ pub fn test_async_inbound_update_fee() {
 		*feerate_lock += 20;
 	}
 	nodes[0].node.timer_tick_occurred();
-	check_added_monitors!(nodes[0], 1);
+	check_added_monitors(&nodes[0], 1);


It's nice to de-macro, but doing it in a separate PR would've been fine too. Just pre-formatting is a lot already.

ldk-reviews-bot · 2025-04-29T09:33:23Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

ldk-reviews-bot · 2025-04-29T11:18:28Z

✅ Added second reviewer: @valentinewallace

valentinewallace · 2025-04-29T18:21:25Z

Needs rebase since #3700 landed

valentinewallace · 2025-04-29T17:45:32Z

lightning/src/ln/payment_tests.rs

+	let params = route.route_params.clone().unwrap();
+	let onion = RecipientOnionFields::spontaneous_empty();
+	let retry = Retry::Attempts(0);
+	nodes[0].node.send_spontaneous_payment(preimage, onion, payment_id_0, params, retry).unwrap();


Would agree with this, in some cases there's a loss of information in the new var name (e.g. removing _msat suffix, secret vs payment_secret) which doesn't seem worth it. It also makes it less clear that the variable isn't used elsewhere.

valentinewallace · 2025-04-29T17:53:33Z

lightning/src/ln/payment_tests.rs

-		assert_eq!(unusable_chan_err , APIError::ChannelUnavailable {
-			err: format!("Channel with id {} for the passed counterparty node_id {} is still opening.",
-				temp_chan_id, node_c_id) });
+		let chan_id = nodes[1].node.create_channel(node_c_id, 100_000, 0, 42, None, None).unwrap();


Prefer s/chan_id/temp_id to make it clearer what's going on

valentinewallace · 2025-04-29T18:24:38Z

lightning/src/ln/functional_tests.rs

-	*nodes[0].connect_style.borrow_mut() = ConnectStyle::BestBlockFirstSkippingBlocks;

+	let node_a_id = nodes[0].node.get_our_node_id();
+	let node_b_id = nodes[1].node.get_our_node_id();
 	let node_c_id = nodes[2].node.get_our_node_id();

+	*nodes[0].connect_style.borrow_mut() = ConnectStyle::BestBlockFirstSkippingBlocks;
+
+	let node_c_id = node_c_id;


nit: unnecessary diff and declared node_c_id twice

valentinewallace · 2025-04-29T18:29:48Z

lightning/src/ln/functional_tests.rs

-fn commit_tx_fee_msat(feerate: u32, num_htlcs: u64, channel_type_features: &ChannelTypeFeatures) -> u64 {
-	(commitment_tx_base_weight(channel_type_features) + num_htlcs * COMMITMENT_TX_WEIGHT_PER_HTLC) * feerate as u64 / 1000 * 1000
-}


nit: this belongs in the commit that adds this method to functional_test_utils

valentinewallace · 2025-04-29T18:45:10Z

lightning/src/ln/functional_tests.rs

-	assert!(nodes[0].node.create_channel(node_b_id, channel_value_satoshis, push_msat, 42, None, None).is_ok()); //Create a valid channel
-	let node0_to_1_send_open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, node_b_id);
-	assert!(node0_to_1_send_open_channel.channel_reserve_satoshis>=node0_to_1_send_open_channel.common_fields.dust_limit_satoshis);
+	nodes[0].node.create_channel(node_b_id, 100_000, 0, 42, None, None).unwrap();


This and the below get_event_msg call need a comment now for what they're testing

valentinewallace · 2025-04-29T19:01:43Z

lightning/src/ln/functional_tests.rs

+use crate::ln::channel::{
+	get_holder_selected_channel_reserve_satoshis, Channel, InboundV1Channel, OutboundV1Channel,
+	COINBASE_MATURITY,
+};
+use crate::ln::channel::{
+	ChannelError, DISCONNECT_PEER_AWAITING_RESPONSE_TICKS, MIN_CHAN_DUST_LIMIT_SATOSHIS,
+};


nit: combine imports here and above

TheBlueMatt added 14 commits April 26, 2025 16:22

Use variables for node IDs in payment_tests

91138f1

Clean up payment_tests.rs in anticipation of rustfmt'ing it

8c714e4

rustfmt payment_tests.rs

dfd277f

Use node_id_* rather than get_our_node_id() in functional_tests

349b077

Move channel reserve tests out of functional_tests.rs into a new file

63b82c4

`funtional_tests.rs` has gotten incredibly huge over the years, so here we move some channel reserve tests into a new file.

Move more chan reserve tests out of functional_tests.rs

3f0d713

`funtional_tests.rs` has gotten incredibly huge over the years, so here we move some further channel reserve tests into a new file. We do so separately from the previous commit to ensure git identifies the changes as move-only.

Move HTLC unit tests out of functional_tests.rs into a new file

429995f

`funtional_tests.rs` has gotten incredibly huge over the years, so here we move some simple HTLC unit tests into a new file.

Clean up htlc_reserve_unit_tests in anticipation of rustfmt

727ab51

De-macro check_added_monitors in htlc_reserve_unit_tests

1b59dec

Run rustfmt on htlc_reserve_unit_tests.rs

5da99fc

Move channel fee-rated tests out of functional_tests.rs

9130acc

`funtional_tests.rs` has gotten incredibly huge over the years, so here we move all the channel fee-related tests out to their own file.

Cleanup update_fee_tests in anticipation of it being rustfmt'd

06ffd24

De-macro check_added_monitors in update_fee_tests.rs

73461d4

Run rustfmt on update_fee_tests

609330f

TheBlueMatt added 4 commits April 28, 2025 14:54

Remove some useless tests in functional_tests and reject huge chans

1855a02

In general we shouldn't have tests lying around that are useless as its dead code and may break due to harmless protocol changes. Here, we drop a few useless tests.

Clean up functional_tests.rs in anticipation of rustfmting it

3e9782e

De-macro check_added_monitors in functional_tests.rs

cdb8d08

Run rustfmt on functional_tests.rs

820cb58

TheBlueMatt force-pushed the 2025-04-rustfmt-tests-1 branch from 293c6af to 820cb58 Compare April 28, 2025 14:54

ldk-reviews-bot requested a review from wpaulino April 28, 2025 15:02

wpaulino requested review from joostjager and removed request for wpaulino April 28, 2025 20:27

joostjager approved these changes Apr 29, 2025

View reviewed changes

ldk-reviews-bot requested a review from valentinewallace April 29, 2025 11:18

valentinewallace reviewed Apr 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run rustfmt on `payment_tests` and `functional_tests` and split up the latter #3751

Run rustfmt on `payment_tests` and `functional_tests` and split up the latter #3751

TheBlueMatt commented Apr 28, 2025

ldk-reviews-bot commented Apr 28, 2025 •

edited

Loading

joostjager left a comment

joostjager Apr 29, 2025

valentinewallace Apr 29, 2025

joostjager Apr 29, 2025

joostjager Apr 29, 2025

joostjager Apr 29, 2025

joostjager Apr 29, 2025

joostjager Apr 29, 2025

joostjager Apr 29, 2025

ldk-reviews-bot commented Apr 29, 2025

ldk-reviews-bot commented Apr 29, 2025

valentinewallace commented Apr 29, 2025

valentinewallace Apr 29, 2025

valentinewallace Apr 29, 2025

valentinewallace Apr 29, 2025

valentinewallace Apr 29, 2025

valentinewallace Apr 29, 2025

valentinewallace Apr 29, 2025

Run rustfmt on payment_tests and functional_tests and split up the latter #3751

Are you sure you want to change the base?

Run rustfmt on payment_tests and functional_tests and split up the latter #3751

Conversation

TheBlueMatt commented Apr 28, 2025

ldk-reviews-bot commented Apr 28, 2025 • edited Loading

joostjager left a 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

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

ldk-reviews-bot commented Apr 29, 2025

ldk-reviews-bot commented Apr 29, 2025

valentinewallace commented Apr 29, 2025

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

Run rustfmt on `payment_tests` and `functional_tests` and split up the latter #3751

Run rustfmt on `payment_tests` and `functional_tests` and split up the latter #3751

ldk-reviews-bot commented Apr 28, 2025 •

edited

Loading