Run rustfmt on `chain/transaction`, `chain/chaininterface`, `max_payment_path_len_tests`, and `chanmon_update_fail_tests` #3783

TheBlueMatt · 2025-05-18T21:36:58Z

This formats ~36% of the remaining lines in */_tests.rs

ldk-reviews-bot · 2025-05-18T21:37:00Z

👋 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.

joostjager

Ack, though I'd say that these PRs carry some risk, and the potential benefits may not justify the costs.

joostjager · 2025-05-19T07:29:45Z

lightning/src/ln/max_payment_path_len_tests.rs

 		.unwrap_err();
 	assert_eq!(err, RetryableSendFailure::RouteNotFound);

 	// If our payment_metadata contains 1 additional byte, we'll fail prior to pathfinding.
-	let mut recipient_onion_too_large_md = recipient_onion_max_md_size.clone();
-	recipient_onion_too_large_md.payment_metadata.as_mut().map(|mut md| md.push(42));
-	let err = nodes[0]


For these formatting PRs, I think it is better to keep the formatting strictly separate from any other change. So that reviewers know what to look for in a commit.

This send_payment call is moved down, but as a reviewer I question myself whether it is just formatting or also something that changed functionally. I see it is now called with payment_hash_2 rather than payment_hash.

I'm confused, the commit "Run rustfmt on max_payment_path_len_tests.rs" only formats the file, changes happen in a separate commit.

I understand the confusion. Rustfmt is indeed separate and fine.

The comment was directly specifically at the clean up in this commit. Most of the changes are variable extractions for which it is easy to see that nothing changed functionally.

The change here is also moving things around. To review that, a more careful look is needed. Can't just look at the lines touched, but also need the context around it.

My point is that it could be helpful for reviewers to separate those two types of changes.

joostjager · 2025-05-19T07:37:20Z

lightning/src/ln/max_payment_path_len_tests.rs

-			route_params.clone(),
-			Retry::Attempts(0),
-		)
+		.send_payment(payment_hash, max_sized_onion.clone(), id, route_params.clone(), no_retry)


My opinion remains unchanged that this isn't necessary to reformat.

joostjager · 2025-05-19T07:52:18Z

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -48,6 +48,9 @@ fn test_monitor_and_persister_update_fail() {
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
 	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);

+	let node_a_id = nodes[0].node.get_our_node_id();


This commit, I think the change isn't worth it. A lot of manual checking for a modest reduction of lines of code (less than 10% on this file) and with changes that not everyone may agree benefit readability.

I've only spot-checked this commit. It looks fine, but we need to be aware that it carries some risk. If I had to produce a change like this I'd probably use search/replace/regex and then adding vars where the compiler complains. This may be easier than reviewing all of the 650 lines in this commit that have near-identical changes.

Not that reviewing needs to be easier than authoring necessarily, but it does raise a flag for me whether we should do it at all.

Hmm, it should be doable to validate the commit by running a regex to replace the new variables with the original copy then running a regex to remove the (correct) initialization lines (eg `s/let nodes[0].node.get_our_node_id() = nodes[0].node.get_our_node_id();$//' assuming the first regex replaced the variable name) and then check that the remaining commit is empty. Should be pretty easy to validate, no?

Reverse-engineering the replace doesn't sound particularly attractive to me. Then I'd much rather have commits that isolate search/replaces, with the actual commands in the commit message.

joostjager · 2025-05-19T07:53:38Z

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -38,6 +38,13 @@ use crate::prelude::*;
 use crate::sync::{Arc, Mutex};
 use bitcoin::hashes::Hash;

+fn get_latest_mon_update_id<'a, 'b, 'c>(


ldk-reviews-bot · 2025-05-19T08:15:02Z

👋 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.

tnull

This needs a rebase it seems

tnull

Also, seems fuzz is failing with:

thread '<unnamed>' panicked at /home/tnull/workspace/rust-lightning/lightning/src/chain/channelmonitor.rs:4439:7:
An unmature HTLC transaction conflicts with a maturing one; failed to call either transaction_unconfirmed for the conflicting transaction or block_disconnected for a block containing it.

`assert_eq` is generally much better than `assert` for equality tests as it provides debug output of `a` and `b`.

TheBlueMatt · 2025-06-11T23:43:49Z

Apologies for the delay, rebased. The fuzz failure is #3756

joostjager

LGTM with the side note that I only spot checked the mass edits. Assuming these were done mechanically and this is "just" test code, it might be sufficient?

tnull

LGTM

I went through the edit commits and verified that the rustfmt commits didn't introduce any unexpected changes. I have to say the sheer size of the diff made this one somewhat tedious and painful to review (and I hope I didn't overlook anything).

IMO it would be preferable to go with the function-level rustfmt::skip approach for the remainder of the codebase, and make some more fine-grained progress over time before we bite the bullet and remove all remaining skips at some point.

Going ahead and landing this in-before it might get stale again.

tnull · 2025-06-12T09:16:42Z

lightning/src/ln/chanmon_update_fail_tests.rs

-			)
-			.unwrap();
+		let init_msg = msgs::Init {
+			features: nodes[1].node.init_features(),


This now reuses the nodes[1].init_features for both connections. Though, I assume this behavior change was introduced on pupose (also in a few other places).

Yea, was careful to only do it in cases where the configs for both nodes are identical.

TheBlueMatt requested a review from joostjager May 18, 2025 21:37

joostjager previously approved these changes May 19, 2025

View reviewed changes

joostjager added the rustfmt label Jun 11, 2025

joostjager mentioned this pull request Jun 11, 2025

Convert file-level rustfmt skip to fn level skips #3809

Merged

tnull reviewed Jun 11, 2025

View reviewed changes

TheBlueMatt added 12 commits June 11, 2025 23:16

Clean up chain/transaction.rs tests marginally before rustfmt

d4c6681

Run rustfmt on chain/transaction.rs

1faaab6

Run rustfmt on chain/chaininterface.rs

4349898

Run rustfmt on max_payment_path_len_tests.rs

6d0c940

Cleanup max_payment_path_len_tests somewhat after rustfmting it

cd135f6

Run rustfmt on chanmon_update_fail_tests.rs

1d36d1d

Remove some unnecessary vecs from chanmon_update_fail_tests

dd30a77

Avoid assert!(a == b) in chanmon_update_fail_tests

a521e15

`assert_eq` is generally much better than `assert` for equality tests as it provides debug output of `a` and `b`.

Use node_*_id variables in chanmon_update_fail_tests

b0de677

Re-rustfmt chanmon_update_fail_tests

df1802a

Introduce a fn to fetch the monitor update id in chanmon tests

8ca43a1

Clean up various overly vertical nonsense in chanmon fail tests

b9d0d60

TheBlueMatt dismissed joostjager’s stale review via b9d0d60 June 11, 2025 23:43

TheBlueMatt force-pushed the 2025-05-rustfmt-tests-2 branch from 1c48bf0 to b9d0d60 Compare June 11, 2025 23:43

joostjager approved these changes Jun 12, 2025

View reviewed changes

tnull approved these changes Jun 12, 2025

View reviewed changes

tnull merged commit 217a5b0 into lightningdevkit:main Jun 12, 2025
28 checks passed

Run rustfmt on chain/transaction, chain/chaininterface, max_payment_path_len_tests, and chanmon_update_fail_tests #3783

Run rustfmt on chain/transaction, chain/chaininterface, max_payment_path_len_tests, and chanmon_update_fail_tests #3783

Uh oh!

Conversation

TheBlueMatt commented May 18, 2025

Uh oh!

ldk-reviews-bot commented May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

joostjager May 19, 2025

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

joostjager Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager May 19, 2025

Choose a reason for hiding this comment

Uh oh!

joostjager May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

joostjager Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

joostjager May 19, 2025

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented May 19, 2025

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jun 11, 2025

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

tnull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Run rustfmt on `chain/transaction`, `chain/chaininterface`, `max_payment_path_len_tests`, and `chanmon_update_fail_tests` #3783

Run rustfmt on `chain/transaction`, `chain/chaininterface`, `max_payment_path_len_tests`, and `chanmon_update_fail_tests` #3783

ldk-reviews-bot commented May 18, 2025 •

edited

Loading

joostjager Jun 12, 2025 •

edited

Loading

joostjager May 19, 2025 •

edited

Loading

tnull left a comment •

edited

Loading

tnull Jun 12, 2025 •

edited

Loading