Remove low-hanging fruit `rustfmt::skips` in ChannelManager pt. 2 by valentinewallace · Pull Request #3850 · lightningdevkit/rust-lightning

valentinewallace · 2025-06-12T14:47:12Z

After #3767 landed, there were a bunch of rustfmt::skips added to ChannelManager that could easily be removed.

Here we pick some of that low-hanging fruit by removing the skips for methods that are less than ~10 lines long.

Follows on from #3844, since that one landed so easily.

ldk-reviews-bot · 2025-06-12T14:47:15Z

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

codecov · 2025-06-12T16:51:28Z

Codecov Report

Attention: Patch coverage is 91.02990% with 27 lines in your changes missing coverage. Please review.

Project coverage is 89.93%. Comparing base (3333b6d) to head (a27c1cb).
Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	91.02%	22 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3850 +/- ##
==========================================
- Coverage 89.94% 89.93% -0.02% 
==========================================
Files 163 163 Lines 131655 131826 +171 Branches 131655 131826 +171 ==========================================
+ Hits 118421 118556 +135 - Misses 10550 10582 +32 - Partials 2684 2688 +4

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

joostjager

Checked the fmt fixes, LGTM. But still I think we have to be super careful with these "random" refactorings outside the context of a functional PR. Maybe it is okay to not touch it also...

joostjager · 2025-06-12T15:01:00Z

+ best_block_height,
+ &self.logger,
+ &self.pending_events,
+ |args| self.send_payment_along_path(args),


Pretty vertical, but also fairly static code, and probably not so easy to refactor.

tnull

This needs a rebase now.

rustfmt::skips were added recently all over channelmanager in order for formatting to be enforced for all new code added to the file. Here we remove a bunch of those skips that aren't necessary because the methods that are skipped are so short. Does not include cfg(splicing) code. Here we just remove the rustfmt skips, in the next commit we'll clean up the code that rustfmt made too vertical.

In the previous commit we formatted a bunch of short methods. Here we clean up the default formatting that rustfmt applied by extracting code into variables.

valentinewallace · 2025-06-13T13:49:59Z

Rebased

joostjager

I do feel the urge to again emphasize that we need to be so careful with "random" refactoring. Especially if we only save a couple of lines compared to naive rustfmt. It doesn't take much for something to slip through.

tnull

LGTM

TheBlueMatt · 2025-06-16T13:42:49Z

Ugh, this formatted a handful of lines that #3741 was about to rewrite.

joostjager · 2025-06-16T18:49:11Z

Ugh, this formatted a handful of lines that #3741 was about to rewrite.

Isn't that easily solved by first formatting the lines in #3741 too before rebasing?

TheBlueMatt · 2025-06-16T19:01:05Z

Its not hard enough to solve either way, but it does mean CI gets delayed a day (luckily not an issue here) and it does mean invalidating existing ACKs and yet more for reviewers to look at.

joostjager · 2025-06-16T19:19:41Z

You'd have preferred more coordination with a large PR ready to land, is that what you'd suggest doing next time?

valentinewallace · 2025-06-12T14:47:12Z

After #3767 landed, there were a bunch of rustfmt::skips added to ChannelManager that could easily be removed.

Here we pick some of that low-hanging fruit by removing the skips for methods that are less than ~10 lines long.

Follows on from #3844, since that one landed so easily.

ldk-reviews-bot · 2025-06-12T14:47:15Z

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

codecov · 2025-06-12T16:51:28Z

Codecov Report

Attention: Patch coverage is 91.02990% with 27 lines in your changes missing coverage. Please review.

Project coverage is 89.93%. Comparing base (3333b6d) to head (a27c1cb).
Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	91.02%	22 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3850 +/- ##
==========================================
- Coverage 89.94% 89.93% -0.02% 
==========================================
Files 163 163 Lines 131655 131826 +171 Branches 131655 131826 +171 ==========================================
+ Hits 118421 118556 +135 - Misses 10550 10582 +32 - Partials 2684 2688 +4

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

joostjager

Checked the fmt fixes, LGTM. But still I think we have to be super careful with these "random" refactorings outside the context of a functional PR. Maybe it is okay to not touch it also...

joostjager · 2025-06-12T15:01:00Z

+ best_block_height,
+ &self.logger,
+ &self.pending_events,
+ |args| self.send_payment_along_path(args),


Pretty vertical, but also fairly static code, and probably not so easy to refactor.

tnull

This needs a rebase now.

rustfmt::skips were added recently all over channelmanager in order for formatting to be enforced for all new code added to the file. Here we remove a bunch of those skips that aren't necessary because the methods that are skipped are so short. Does not include cfg(splicing) code. Here we just remove the rustfmt skips, in the next commit we'll clean up the code that rustfmt made too vertical.

In the previous commit we formatted a bunch of short methods. Here we clean up the default formatting that rustfmt applied by extracting code into variables.

valentinewallace · 2025-06-13T13:49:59Z

Rebased

joostjager

I do feel the urge to again emphasize that we need to be so careful with "random" refactoring. Especially if we only save a couple of lines compared to naive rustfmt. It doesn't take much for something to slip through.

tnull

LGTM

TheBlueMatt · 2025-06-16T13:42:49Z

Ugh, this formatted a handful of lines that #3741 was about to rewrite.

joostjager · 2025-06-16T18:49:11Z

Ugh, this formatted a handful of lines that #3741 was about to rewrite.

Isn't that easily solved by first formatting the lines in #3741 too before rebasing?

TheBlueMatt · 2025-06-16T19:01:05Z

Its not hard enough to solve either way, but it does mean CI gets delayed a day (luckily not an issue here) and it does mean invalidating existing ACKs and yet more for reviewers to look at.

joostjager · 2025-06-16T19:19:41Z

You'd have preferred more coordination with a large PR ready to land, is that what you'd suggest doing next time?

valentinewallace · 2025-06-12T14:47:12Z

After #3767 landed, there were a bunch of rustfmt::skips added to ChannelManager that could easily be removed.

Here we pick some of that low-hanging fruit by removing the skips for methods that are less than ~10 lines long.

Follows on from #3844, since that one landed so easily.

ldk-reviews-bot · 2025-06-12T14:47:15Z

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

codecov · 2025-06-12T16:51:28Z

Codecov Report

Attention: Patch coverage is 91.02990% with 27 lines in your changes missing coverage. Please review.

Project coverage is 89.93%. Comparing base (3333b6d) to head (a27c1cb).
Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	91.02%	22 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3850 +/- ##
==========================================
- Coverage 89.94% 89.93% -0.02% 
==========================================
Files 163 163 Lines 131655 131826 +171 Branches 131655 131826 +171 ==========================================
+ Hits 118421 118556 +135 - Misses 10550 10582 +32 - Partials 2684 2688 +4

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

joostjager

Checked the fmt fixes, LGTM. But still I think we have to be super careful with these "random" refactorings outside the context of a functional PR. Maybe it is okay to not touch it also...

joostjager · 2025-06-12T15:01:00Z

+ best_block_height,
+ &self.logger,
+ &self.pending_events,
+ |args| self.send_payment_along_path(args),


Pretty vertical, but also fairly static code, and probably not so easy to refactor.

tnull

This needs a rebase now.

rustfmt::skips were added recently all over channelmanager in order for formatting to be enforced for all new code added to the file. Here we remove a bunch of those skips that aren't necessary because the methods that are skipped are so short. Does not include cfg(splicing) code. Here we just remove the rustfmt skips, in the next commit we'll clean up the code that rustfmt made too vertical.

In the previous commit we formatted a bunch of short methods. Here we clean up the default formatting that rustfmt applied by extracting code into variables.

valentinewallace · 2025-06-13T13:49:59Z

Rebased

joostjager

I do feel the urge to again emphasize that we need to be so careful with "random" refactoring. Especially if we only save a couple of lines compared to naive rustfmt. It doesn't take much for something to slip through.

tnull

LGTM

TheBlueMatt · 2025-06-16T13:42:49Z

Ugh, this formatted a handful of lines that #3741 was about to rewrite.

joostjager · 2025-06-16T18:49:11Z

Ugh, this formatted a handful of lines that #3741 was about to rewrite.

Isn't that easily solved by first formatting the lines in #3741 too before rebasing?

TheBlueMatt · 2025-06-16T19:01:05Z

Its not hard enough to solve either way, but it does mean CI gets delayed a day (luckily not an issue here) and it does mean invalidating existing ACKs and yet more for reviewers to look at.

joostjager · 2025-06-16T19:19:41Z

You'd have preferred more coordination with a large PR ready to land, is that what you'd suggest doing next time?

valentinewallace · 2025-06-12T14:47:12Z

After #3767 landed, there were a bunch of rustfmt::skips added to ChannelManager that could easily be removed.

Here we pick some of that low-hanging fruit by removing the skips for methods that are less than ~10 lines long.

Follows on from #3844, since that one landed so easily.

ldk-reviews-bot · 2025-06-12T14:47:15Z

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

codecov · 2025-06-12T16:51:28Z

Codecov Report

Attention: Patch coverage is 91.02990% with 27 lines in your changes missing coverage. Please review.

Project coverage is 89.93%. Comparing base (3333b6d) to head (a27c1cb).
Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	91.02%	22 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3850 +/- ##
==========================================
- Coverage 89.94% 89.93% -0.02% 
==========================================
Files 163 163 Lines 131655 131826 +171 Branches 131655 131826 +171 ==========================================
+ Hits 118421 118556 +135 - Misses 10550 10582 +32 - Partials 2684 2688 +4

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

joostjager

Checked the fmt fixes, LGTM. But still I think we have to be super careful with these "random" refactorings outside the context of a functional PR. Maybe it is okay to not touch it also...

joostjager · 2025-06-12T15:01:00Z

+ best_block_height,
+ &self.logger,
+ &self.pending_events,
+ |args| self.send_payment_along_path(args),


Pretty vertical, but also fairly static code, and probably not so easy to refactor.

tnull

This needs a rebase now.

rustfmt::skips were added recently all over channelmanager in order for formatting to be enforced for all new code added to the file. Here we remove a bunch of those skips that aren't necessary because the methods that are skipped are so short. Does not include cfg(splicing) code. Here we just remove the rustfmt skips, in the next commit we'll clean up the code that rustfmt made too vertical.

In the previous commit we formatted a bunch of short methods. Here we clean up the default formatting that rustfmt applied by extracting code into variables.

valentinewallace · 2025-06-13T13:49:59Z

Rebased

joostjager

I do feel the urge to again emphasize that we need to be so careful with "random" refactoring. Especially if we only save a couple of lines compared to naive rustfmt. It doesn't take much for something to slip through.

tnull

LGTM

TheBlueMatt · 2025-06-16T13:42:49Z

Ugh, this formatted a handful of lines that #3741 was about to rewrite.

joostjager · 2025-06-16T18:49:11Z

Ugh, this formatted a handful of lines that #3741 was about to rewrite.

Isn't that easily solved by first formatting the lines in #3741 too before rebasing?

TheBlueMatt · 2025-06-16T19:01:05Z

Its not hard enough to solve either way, but it does mean CI gets delayed a day (luckily not an issue here) and it does mean invalidating existing ACKs and yet more for reviewers to look at.

joostjager · 2025-06-16T19:19:41Z

You'd have preferred more coordination with a large PR ready to land, is that what you'd suggest doing next time?

valentinewallace · 2025-06-12T14:47:12Z

After #3767 landed, there were a bunch of rustfmt::skips added to ChannelManager that could easily be removed.

Here we pick some of that low-hanging fruit by removing the skips for methods that are less than ~10 lines long.

Follows on from #3844, since that one landed so easily.

ldk-reviews-bot · 2025-06-12T14:47:15Z

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

codecov · 2025-06-12T16:51:28Z

Codecov Report

Attention: Patch coverage is 91.02990% with 27 lines in your changes missing coverage. Please review.

Project coverage is 89.93%. Comparing base (3333b6d) to head (a27c1cb).
Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	91.02%	22 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3850 +/- ##
==========================================
- Coverage 89.94% 89.93% -0.02% 
==========================================
Files 163 163 Lines 131655 131826 +171 Branches 131655 131826 +171 ==========================================
+ Hits 118421 118556 +135 - Misses 10550 10582 +32 - Partials 2684 2688 +4

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

joostjager

Checked the fmt fixes, LGTM. But still I think we have to be super careful with these "random" refactorings outside the context of a functional PR. Maybe it is okay to not touch it also...

joostjager · 2025-06-12T15:01:00Z

+ best_block_height,
+ &self.logger,
+ &self.pending_events,
+ |args| self.send_payment_along_path(args),


Pretty vertical, but also fairly static code, and probably not so easy to refactor.

tnull

This needs a rebase now.

rustfmt::skips were added recently all over channelmanager in order for formatting to be enforced for all new code added to the file. Here we remove a bunch of those skips that aren't necessary because the methods that are skipped are so short. Does not include cfg(splicing) code. Here we just remove the rustfmt skips, in the next commit we'll clean up the code that rustfmt made too vertical.

In the previous commit we formatted a bunch of short methods. Here we clean up the default formatting that rustfmt applied by extracting code into variables.

valentinewallace · 2025-06-13T13:49:59Z

Rebased

joostjager

I do feel the urge to again emphasize that we need to be so careful with "random" refactoring. Especially if we only save a couple of lines compared to naive rustfmt. It doesn't take much for something to slip through.

tnull

LGTM

TheBlueMatt · 2025-06-16T13:42:49Z

Ugh, this formatted a handful of lines that #3741 was about to rewrite.

joostjager · 2025-06-16T18:49:11Z

Ugh, this formatted a handful of lines that #3741 was about to rewrite.

Isn't that easily solved by first formatting the lines in #3741 too before rebasing?

TheBlueMatt · 2025-06-16T19:01:05Z

Its not hard enough to solve either way, but it does mean CI gets delayed a day (luckily not an issue here) and it does mean invalidating existing ACKs and yet more for reviewers to look at.

joostjager · 2025-06-16T19:19:41Z

You'd have preferred more coordination with a large PR ready to land, is that what you'd suggest doing next time?

valentinewallace · 2025-06-12T14:47:12Z

After #3767 landed, there were a bunch of rustfmt::skips added to ChannelManager that could easily be removed.

Here we pick some of that low-hanging fruit by removing the skips for methods that are less than ~10 lines long.

Follows on from #3844, since that one landed so easily.

ldk-reviews-bot · 2025-06-12T14:47:15Z

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

codecov · 2025-06-12T16:51:28Z

Codecov Report

Attention: Patch coverage is 91.02990% with 27 lines in your changes missing coverage. Please review.

Project coverage is 89.93%. Comparing base (3333b6d) to head (a27c1cb).
Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	91.02%	22 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3850 +/- ##
==========================================
- Coverage 89.94% 89.93% -0.02% 
==========================================
Files 163 163 Lines 131655 131826 +171 Branches 131655 131826 +171 ==========================================
+ Hits 118421 118556 +135 - Misses 10550 10582 +32 - Partials 2684 2688 +4

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

joostjager

Checked the fmt fixes, LGTM. But still I think we have to be super careful with these "random" refactorings outside the context of a functional PR. Maybe it is okay to not touch it also...

joostjager · 2025-06-12T15:01:00Z

+ best_block_height,
+ &self.logger,
+ &self.pending_events,
+ |args| self.send_payment_along_path(args),


Pretty vertical, but also fairly static code, and probably not so easy to refactor.

tnull

This needs a rebase now.

rustfmt::skips were added recently all over channelmanager in order for formatting to be enforced for all new code added to the file. Here we remove a bunch of those skips that aren't necessary because the methods that are skipped are so short. Does not include cfg(splicing) code. Here we just remove the rustfmt skips, in the next commit we'll clean up the code that rustfmt made too vertical.

In the previous commit we formatted a bunch of short methods. Here we clean up the default formatting that rustfmt applied by extracting code into variables.

valentinewallace · 2025-06-13T13:49:59Z

Rebased

joostjager

I do feel the urge to again emphasize that we need to be so careful with "random" refactoring. Especially if we only save a couple of lines compared to naive rustfmt. It doesn't take much for something to slip through.

tnull

LGTM

TheBlueMatt · 2025-06-16T13:42:49Z

Ugh, this formatted a handful of lines that #3741 was about to rewrite.

joostjager · 2025-06-16T18:49:11Z

Ugh, this formatted a handful of lines that #3741 was about to rewrite.

Isn't that easily solved by first formatting the lines in #3741 too before rebasing?

TheBlueMatt · 2025-06-16T19:01:05Z

Its not hard enough to solve either way, but it does mean CI gets delayed a day (luckily not an issue here) and it does mean invalidating existing ACKs and yet more for reviewers to look at.

joostjager · 2025-06-16T19:19:41Z

You'd have preferred more coordination with a large PR ready to land, is that what you'd suggest doing next time?

valentinewallace · 2025-06-12T14:47:12Z

After #3767 landed, there were a bunch of rustfmt::skips added to ChannelManager that could easily be removed.

Here we pick some of that low-hanging fruit by removing the skips for methods that are less than ~10 lines long.

Follows on from #3844, since that one landed so easily.

ldk-reviews-bot · 2025-06-12T14:47:15Z

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

codecov · 2025-06-12T16:51:28Z

Codecov Report

Attention: Patch coverage is 91.02990% with 27 lines in your changes missing coverage. Please review.

Project coverage is 89.93%. Comparing base (3333b6d) to head (a27c1cb).
Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	91.02%	22 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3850 +/- ##
==========================================
- Coverage 89.94% 89.93% -0.02% 
==========================================
Files 163 163 Lines 131655 131826 +171 Branches 131655 131826 +171 ==========================================
+ Hits 118421 118556 +135 - Misses 10550 10582 +32 - Partials 2684 2688 +4

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

joostjager

Checked the fmt fixes, LGTM. But still I think we have to be super careful with these "random" refactorings outside the context of a functional PR. Maybe it is okay to not touch it also...

joostjager · 2025-06-12T15:01:00Z

+ best_block_height,
+ &self.logger,
+ &self.pending_events,
+ |args| self.send_payment_along_path(args),


Pretty vertical, but also fairly static code, and probably not so easy to refactor.

tnull

This needs a rebase now.

rustfmt::skips were added recently all over channelmanager in order for formatting to be enforced for all new code added to the file. Here we remove a bunch of those skips that aren't necessary because the methods that are skipped are so short. Does not include cfg(splicing) code. Here we just remove the rustfmt skips, in the next commit we'll clean up the code that rustfmt made too vertical.

In the previous commit we formatted a bunch of short methods. Here we clean up the default formatting that rustfmt applied by extracting code into variables.

valentinewallace · 2025-06-13T13:49:59Z

Rebased

joostjager

I do feel the urge to again emphasize that we need to be so careful with "random" refactoring. Especially if we only save a couple of lines compared to naive rustfmt. It doesn't take much for something to slip through.

tnull

LGTM

TheBlueMatt · 2025-06-16T13:42:49Z

Ugh, this formatted a handful of lines that #3741 was about to rewrite.

joostjager · 2025-06-16T18:49:11Z

Ugh, this formatted a handful of lines that #3741 was about to rewrite.

Isn't that easily solved by first formatting the lines in #3741 too before rebasing?

TheBlueMatt · 2025-06-16T19:01:05Z

Its not hard enough to solve either way, but it does mean CI gets delayed a day (luckily not an issue here) and it does mean invalidating existing ACKs and yet more for reviewers to look at.

joostjager · 2025-06-16T19:19:41Z

You'd have preferred more coordination with a large PR ready to land, is that what you'd suggest doing next time?

valentinewallace · 2025-06-12T14:47:12Z

After #3767 landed, there were a bunch of rustfmt::skips added to ChannelManager that could easily be removed.

Here we pick some of that low-hanging fruit by removing the skips for methods that are less than ~10 lines long.

Follows on from #3844, since that one landed so easily.

ldk-reviews-bot · 2025-06-12T14:47:15Z

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

codecov · 2025-06-12T16:51:28Z

Codecov Report

Attention: Patch coverage is 91.02990% with 27 lines in your changes missing coverage. Please review.

Project coverage is 89.93%. Comparing base (3333b6d) to head (a27c1cb).
Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	91.02%	22 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3850 +/- ##
==========================================
- Coverage 89.94% 89.93% -0.02% 
==========================================
Files 163 163 Lines 131655 131826 +171 Branches 131655 131826 +171 ==========================================
+ Hits 118421 118556 +135 - Misses 10550 10582 +32 - Partials 2684 2688 +4

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

joostjager

Checked the fmt fixes, LGTM. But still I think we have to be super careful with these "random" refactorings outside the context of a functional PR. Maybe it is okay to not touch it also...

joostjager · 2025-06-12T15:01:00Z

+ best_block_height,
+ &self.logger,
+ &self.pending_events,
+ |args| self.send_payment_along_path(args),


Pretty vertical, but also fairly static code, and probably not so easy to refactor.

tnull

This needs a rebase now.

rustfmt::skips were added recently all over channelmanager in order for formatting to be enforced for all new code added to the file. Here we remove a bunch of those skips that aren't necessary because the methods that are skipped are so short. Does not include cfg(splicing) code. Here we just remove the rustfmt skips, in the next commit we'll clean up the code that rustfmt made too vertical.

In the previous commit we formatted a bunch of short methods. Here we clean up the default formatting that rustfmt applied by extracting code into variables.

valentinewallace · 2025-06-13T13:49:59Z

Rebased

joostjager

I do feel the urge to again emphasize that we need to be so careful with "random" refactoring. Especially if we only save a couple of lines compared to naive rustfmt. It doesn't take much for something to slip through.

tnull

LGTM

TheBlueMatt · 2025-06-16T13:42:49Z

Ugh, this formatted a handful of lines that #3741 was about to rewrite.

joostjager · 2025-06-16T18:49:11Z

Ugh, this formatted a handful of lines that #3741 was about to rewrite.

Isn't that easily solved by first formatting the lines in #3741 too before rebasing?

TheBlueMatt · 2025-06-16T19:01:05Z

Its not hard enough to solve either way, but it does mean CI gets delayed a day (luckily not an issue here) and it does mean invalidating existing ACKs and yet more for reviewers to look at.

joostjager · 2025-06-16T19:19:41Z

You'd have preferred more coordination with a large PR ready to land, is that what you'd suggest doing next time?

valentinewallace commented Jun 12, 2025
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

valentinewallace requested review from joostjager and tnull June 12, 2025 14:48

joostjager added the rustfmt label Jun 12, 2025

joostjager previously approved these changes Jun 12, 2025
View reviewed changes

valentinewallace dismissed joostjager’s stale review via 047a256June 12, 2025 19:43

valentinewallace force-pushed the 2025-06-fmt-chanmgr-even-more branch from d034929 to 047a256Compare June 12, 2025 19:43

valentinewallace requested a review from joostjager June 12, 2025 19:44

valentinewallace mentioned this pull request Jun 12, 2025
Remove low-hanging fruit rustfmt::skips in Channel #3854
Merged

joostjager previously approved these changes Jun 12, 2025
View reviewed changes

tnull reviewed Jun 13, 2025
View reviewed changes

valentinewallace added 2 commits June 13, 2025 09:49

Fix rustfmt'd short ChannelManager methods
a27c1cb
In the previous commit we formatted a bunch of short methods. Here we clean up the default formatting that rustfmt applied by extracting code into variables.

valentinewallace dismissed joostjager’s stale review via a27c1cbJune 13, 2025 13:49

valentinewallace force-pushed the 2025-06-fmt-chanmgr-even-more branch from 047a256 to a27c1cbCompare June 13, 2025 13:49

joostjager approved these changes Jun 13, 2025
View reviewed changes

tnull approved these changes Jun 16, 2025
View reviewed changes

tnull merged commit 30ab411 into lightningdevkit:mainJun 16, 2025

valentinewallace commented Jun 12, 2025
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

valentinewallace requested review from joostjager and tnull June 12, 2025 14:48

joostjager added the rustfmt label Jun 12, 2025

joostjager previously approved these changes Jun 12, 2025
View reviewed changes

valentinewallace dismissed joostjager’s stale review via 047a256June 12, 2025 19:43

valentinewallace force-pushed the 2025-06-fmt-chanmgr-even-more branch from d034929 to 047a256Compare June 12, 2025 19:43

valentinewallace requested a review from joostjager June 12, 2025 19:44

valentinewallace mentioned this pull request Jun 12, 2025
Remove low-hanging fruit rustfmt::skips in Channel #3854
Merged

joostjager previously approved these changes Jun 12, 2025
View reviewed changes

tnull reviewed Jun 13, 2025
View reviewed changes

valentinewallace added 2 commits June 13, 2025 09:49

Fix rustfmt'd short ChannelManager methods
a27c1cb
In the previous commit we formatted a bunch of short methods. Here we clean up the default formatting that rustfmt applied by extracting code into variables.

valentinewallace dismissed joostjager’s stale review via a27c1cbJune 13, 2025 13:49

valentinewallace force-pushed the 2025-06-fmt-chanmgr-even-more branch from 047a256 to a27c1cbCompare June 13, 2025 13:49

joostjager approved these changes Jun 13, 2025
View reviewed changes

tnull approved these changes Jun 16, 2025
View reviewed changes

tnull merged commit 30ab411 into lightningdevkit:mainJun 16, 2025

valentinewallace commented Jun 12, 2025
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

valentinewallace requested review from joostjager and tnull June 12, 2025 14:48

joostjager added the rustfmt label Jun 12, 2025

joostjager previously approved these changes Jun 12, 2025
View reviewed changes

valentinewallace dismissed joostjager’s stale review via 047a256June 12, 2025 19:43

valentinewallace force-pushed the 2025-06-fmt-chanmgr-even-more branch from d034929 to 047a256Compare June 12, 2025 19:43

valentinewallace requested a review from joostjager June 12, 2025 19:44

valentinewallace mentioned this pull request Jun 12, 2025
Remove low-hanging fruit rustfmt::skips in Channel #3854
Merged

joostjager previously approved these changes Jun 12, 2025
View reviewed changes

tnull reviewed Jun 13, 2025
View reviewed changes

valentinewallace added 2 commits June 13, 2025 09:49

Fix rustfmt'd short ChannelManager methods
a27c1cb
In the previous commit we formatted a bunch of short methods. Here we clean up the default formatting that rustfmt applied by extracting code into variables.

valentinewallace dismissed joostjager’s stale review via a27c1cbJune 13, 2025 13:49

valentinewallace force-pushed the 2025-06-fmt-chanmgr-even-more branch from 047a256 to a27c1cbCompare June 13, 2025 13:49

joostjager approved these changes Jun 13, 2025
View reviewed changes

tnull approved these changes Jun 16, 2025
View reviewed changes

tnull merged commit 30ab411 into lightningdevkit:mainJun 16, 2025

valentinewallace commented Jun 12, 2025
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

valentinewallace requested review from joostjager and tnull June 12, 2025 14:48

joostjager added the rustfmt label Jun 12, 2025

joostjager previously approved these changes Jun 12, 2025
View reviewed changes

valentinewallace dismissed joostjager’s stale review via 047a256June 12, 2025 19:43

valentinewallace force-pushed the 2025-06-fmt-chanmgr-even-more branch from d034929 to 047a256Compare June 12, 2025 19:43

valentinewallace requested a review from joostjager June 12, 2025 19:44

valentinewallace mentioned this pull request Jun 12, 2025
Remove low-hanging fruit rustfmt::skips in Channel #3854
Merged

joostjager previously approved these changes Jun 12, 2025
View reviewed changes

tnull reviewed Jun 13, 2025
View reviewed changes

valentinewallace added 2 commits June 13, 2025 09:49

Fix rustfmt'd short ChannelManager methods
a27c1cb
In the previous commit we formatted a bunch of short methods. Here we clean up the default formatting that rustfmt applied by extracting code into variables.

valentinewallace dismissed joostjager’s stale review via a27c1cbJune 13, 2025 13:49

valentinewallace force-pushed the 2025-06-fmt-chanmgr-even-more branch from 047a256 to a27c1cbCompare June 13, 2025 13:49

joostjager approved these changes Jun 13, 2025
View reviewed changes

tnull approved these changes Jun 16, 2025
View reviewed changes

tnull merged commit 30ab411 into lightningdevkit:mainJun 16, 2025

valentinewallace commented Jun 12, 2025
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

valentinewallace requested review from joostjager and tnull June 12, 2025 14:48

joostjager added the rustfmt label Jun 12, 2025

joostjager previously approved these changes Jun 12, 2025
View reviewed changes

valentinewallace dismissed joostjager’s stale review via 047a256June 12, 2025 19:43

valentinewallace force-pushed the 2025-06-fmt-chanmgr-even-more branch from d034929 to 047a256Compare June 12, 2025 19:43

valentinewallace requested a review from joostjager June 12, 2025 19:44

valentinewallace mentioned this pull request Jun 12, 2025
Remove low-hanging fruit rustfmt::skips in Channel #3854
Merged

joostjager previously approved these changes Jun 12, 2025
View reviewed changes

tnull reviewed Jun 13, 2025
View reviewed changes

valentinewallace added 2 commits June 13, 2025 09:49

Fix rustfmt'd short ChannelManager methods
a27c1cb
In the previous commit we formatted a bunch of short methods. Here we clean up the default formatting that rustfmt applied by extracting code into variables.

valentinewallace dismissed joostjager’s stale review via a27c1cbJune 13, 2025 13:49

valentinewallace force-pushed the 2025-06-fmt-chanmgr-even-more branch from 047a256 to a27c1cbCompare June 13, 2025 13:49

joostjager approved these changes Jun 13, 2025
View reviewed changes

tnull approved these changes Jun 16, 2025
View reviewed changes

tnull merged commit 30ab411 into lightningdevkit:mainJun 16, 2025

valentinewallace commented Jun 12, 2025
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

valentinewallace requested review from joostjager and tnull June 12, 2025 14:48

joostjager added the rustfmt label Jun 12, 2025

joostjager previously approved these changes Jun 12, 2025
View reviewed changes

valentinewallace dismissed joostjager’s stale review via 047a256June 12, 2025 19:43

valentinewallace force-pushed the 2025-06-fmt-chanmgr-even-more branch from d034929 to 047a256Compare June 12, 2025 19:43

valentinewallace requested a review from joostjager June 12, 2025 19:44

valentinewallace mentioned this pull request Jun 12, 2025
Remove low-hanging fruit rustfmt::skips in Channel #3854
Merged

joostjager previously approved these changes Jun 12, 2025
View reviewed changes

tnull reviewed Jun 13, 2025
View reviewed changes

valentinewallace added 2 commits June 13, 2025 09:49

Fix rustfmt'd short ChannelManager methods
a27c1cb
In the previous commit we formatted a bunch of short methods. Here we clean up the default formatting that rustfmt applied by extracting code into variables.

valentinewallace dismissed joostjager’s stale review via a27c1cbJune 13, 2025 13:49

valentinewallace force-pushed the 2025-06-fmt-chanmgr-even-more branch from 047a256 to a27c1cbCompare June 13, 2025 13:49

joostjager approved these changes Jun 13, 2025
View reviewed changes

tnull approved these changes Jun 16, 2025
View reviewed changes

tnull merged commit 30ab411 into lightningdevkit:mainJun 16, 2025

valentinewallace commented Jun 12, 2025
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

valentinewallace requested review from joostjager and tnull June 12, 2025 14:48

joostjager added the rustfmt label Jun 12, 2025

joostjager previously approved these changes Jun 12, 2025
View reviewed changes

valentinewallace dismissed joostjager’s stale review via 047a256June 12, 2025 19:43

valentinewallace force-pushed the 2025-06-fmt-chanmgr-even-more branch from d034929 to 047a256Compare June 12, 2025 19:43

valentinewallace requested a review from joostjager June 12, 2025 19:44

valentinewallace mentioned this pull request Jun 12, 2025
Remove low-hanging fruit rustfmt::skips in Channel #3854
Merged

joostjager previously approved these changes Jun 12, 2025
View reviewed changes

tnull reviewed Jun 13, 2025
View reviewed changes

valentinewallace added 2 commits June 13, 2025 09:49

Fix rustfmt'd short ChannelManager methods
a27c1cb
In the previous commit we formatted a bunch of short methods. Here we clean up the default formatting that rustfmt applied by extracting code into variables.

valentinewallace dismissed joostjager’s stale review via a27c1cbJune 13, 2025 13:49

valentinewallace force-pushed the 2025-06-fmt-chanmgr-even-more branch from 047a256 to a27c1cbCompare June 13, 2025 13:49

joostjager approved these changes Jun 13, 2025
View reviewed changes

tnull approved these changes Jun 16, 2025
View reviewed changes

tnull merged commit 30ab411 into lightningdevkit:mainJun 16, 2025

valentinewallace commented Jun 12, 2025
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

valentinewallace requested review from joostjager and tnull June 12, 2025 14:48

joostjager added the rustfmt label Jun 12, 2025

joostjager previously approved these changes Jun 12, 2025
View reviewed changes

valentinewallace dismissed joostjager’s stale review via 047a256June 12, 2025 19:43

valentinewallace force-pushed the 2025-06-fmt-chanmgr-even-more branch from d034929 to 047a256Compare June 12, 2025 19:43

valentinewallace requested a review from joostjager June 12, 2025 19:44

valentinewallace mentioned this pull request Jun 12, 2025
Remove low-hanging fruit rustfmt::skips in Channel #3854
Merged

joostjager previously approved these changes Jun 12, 2025
View reviewed changes

tnull reviewed Jun 13, 2025
View reviewed changes

valentinewallace added 2 commits June 13, 2025 09:49

Fix rustfmt'd short ChannelManager methods
a27c1cb
In the previous commit we formatted a bunch of short methods. Here we clean up the default formatting that rustfmt applied by extracting code into variables.

valentinewallace dismissed joostjager’s stale review via a27c1cbJune 13, 2025 13:49

valentinewallace force-pushed the 2025-06-fmt-chanmgr-even-more branch from 047a256 to a27c1cbCompare June 13, 2025 13:49

joostjager approved these changes Jun 13, 2025
View reviewed changes

tnull approved these changes Jun 16, 2025
View reviewed changes

tnull merged commit 30ab411 into lightningdevkit:mainJun 16, 2025

Uh oh!

Conversation

valentinewallace commented Jun 12, 2025

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

joostjagerJun 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

valentinewallace commented Jun 13, 2025

Uh oh!

joostjager 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 16, 2025

Uh oh!

joostjager commented Jun 16, 2025

Uh oh!

TheBlueMatt commented Jun 16, 2025

Uh oh!

joostjager commented Jun 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

valentinewallace commented Jun 12, 2025

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

joostjagerJun 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

valentinewallace commented Jun 13, 2025

Uh oh!

joostjager 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 16, 2025

Uh oh!

joostjager commented Jun 16, 2025

Uh oh!

ldk-reviews-bot commented Jun 12, 2025•
edited
Loading

codecovBot commented Jun 12, 2025•
edited
Loading

ldk-reviews-bot commented Jun 12, 2025•
edited
Loading

codecovBot commented Jun 12, 2025•
edited
Loading

ldk-reviews-bot commented Jun 12, 2025•
edited
Loading

codecovBot commented Jun 12, 2025•
edited
Loading