`rustfmt`: Run on `util/*` (2/2) #3323

tnull · 2024-09-18T07:42:13Z

This is PR 2/2, based on #3324.

The diff is a bit larger for this one, but AFAICT the changes look mostly reasonable (besides the oddity commented below).

FWIW, I had a look at currently open inflight PRs and the conflicts should be minimal if I'm not overlooking something.

codecov · 2024-09-18T07:50:46Z

Codecov Report

Attention: Patch coverage is 82.03883% with 185 lines in your changes missing coverage. Please review.

Project coverage is 88.28%. Comparing base (42cc4e7) to head (7153e35).

Files with missing lines	Patch %	Lines
lightning/src/util/test_channel_signer.rs	50.64%	8 Missing and 69 partials ⚠️
lightning/src/util/test_utils.rs	84.47%	66 Missing and 2 partials ⚠️
lightning/src/util/ser.rs	74.24%	17 Missing ⚠️
lightning/src/util/ser_macros.rs	90.19%	9 Missing and 1 partial ⚠️
lightning/src/util/wakers.rs	93.00%	5 Missing and 2 partials ⚠️
lightning/src/util/transaction_utils.rs	94.17%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3323      +/-   ##
==========================================
- Coverage   88.31%   88.28%   -0.04%     
==========================================
  Files         149      149              
  Lines      112839   113376     +537     
  Branches   112839   113376     +537     
==========================================
+ Hits        99655   100089     +434     
- Misses      10700    10765      +65     
- Partials     2484     2522      +38

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

lightning/src/util/ser.rs

TheBlueMatt · 2024-09-18T13:00:29Z

Ugh, can we split this? 42 commits in one PR is just a lot...

tnull · 2024-09-18T13:10:48Z

Ugh, can we split this? 42 commits in one PR is just a lot...

Alright, happy to if you prefer (although at least half of these commits are one-line diffs). Now split out ~the first half of commits to #3324.

tnull · 2024-09-19T08:11:18Z

Rebased after #3327 landed.

tnull · 2024-10-01T16:02:30Z

Rebased after #3324 landed.

tnull · 2024-10-07T08:25:08Z

Rebased to resolve minor conflicts.

tnull · 2024-11-05T09:14:53Z

Rebased once more to resolve minor conflicts.

Any additional comments here after we landed the split-out commits in #3324, @TheBlueMatt ?

tnull · 2024-11-18T17:57:16Z

Rebased once more to resolve minor conflicts.

TheBlueMatt

Some initial comments. Maybe could have been split across PRs.

lightning/src/util/ser_macros.rs

TheBlueMatt · 2024-12-02T18:39:14Z

lightning/src/util/ser_macros.rs

-		do_test!(concat!("01", "08", "0100000000000000"), Some(72057594037927936), None, None, None);
-		do_test!(concat!("02", "08", "0000000000000226"), None, Some((0 << 30) | (0 << 5) | (550 << 0)), None, None);
+		do_test!(
+			concat!("01", "08", "0100000000000000"),


Not sure how much readable it would be pulling out stuff here into variables? Seems that might just make stuff more confusing?

TheBlueMatt · 2024-12-02T18:47:04Z

lightning/src/util/test_channel_signer.rs

-	}
-
-	fn sign_counterparty_htlc_transaction(&self, htlc_tx: &Transaction, input: usize, amount: u64, per_commitment_point: &PublicKey, htlc: &HTLCOutputInCommitment, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()> {
+		Ok(EcdsaChannelSigner::sign_holder_htlc_transaction(


Can this be less verbose if we use self.inner.sign_holder_htlc_transaction?

Turns out I had to revert the changes as otherwise cargo would complain under cfg(taproot):

error[E0034]: multiple applicable items in scope --> lightning/src/util/test_channel_signer.rs:316:17 | 316 | Ok(self.inner.sign_holder_htlc_transaction(htlc_tx, input, htlc_descriptor, secp_ctx).unwrap()) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ multiple `sign_holder_htlc_transaction` found | note: candidate #1 is defined in an impl of the trait `TaprootChannelSigner` for the type `InMemorySigner` --> lightning/src/sign/mod.rs:1755:2 | 1755 | / fn sign_holder_htlc_transaction( 1756 | | &self, htlc_tx: &Transaction, input: usize, htlc_descriptor: &HTLCDescriptor, 1757 | | secp_ctx: &Secp256k1<All>, 1758 | | ) -> Result<schnorr::Signature, ()> { | |_______________________________________^ note: candidate #2 is defined in an impl of the trait `sign::ecdsa::EcdsaChannelSigner` for the type `InMemorySigner` --> lightning/src/sign/mod.rs:1593:2 | 1593 | / fn sign_holder_htlc_transaction( 1594 | | &self, htlc_tx: &Transaction, input: usize, htlc_descriptor: &HTLCDescriptor, 1595 | | secp_ctx: &Secp256k1<secp256k1::All>, 1596 | | ) -> Result<Signature, ()> { | |______________________________^ help: disambiguate the method for candidate #1 | 316 | Ok(TaprootChannelSigner::sign_holder_htlc_transaction(&self.inner, htlc_tx, input, htlc_descriptor, secp_ctx).unwrap()) | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ help: disambiguate the method for candidate #2 | 316 | Ok(sign::ecdsa::EcdsaChannelSigner::sign_holder_htlc_transaction(&self.inner, htlc_tx, input, htlc_descriptor, secp_ctx).unwrap()) | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

lightning/src/util/transaction_utils.rs

tnull · 2024-12-03T12:28:12Z

Addressed all but one comment.

tnull · 2024-12-18T08:42:57Z

Rebased once more to resolve minor conflict.

@TheBlueMatt any further comments here?

lightning/src/util/test_channel_signer.rs

tnull · 2025-01-17T09:43:49Z

Fixed the missing unwrap and rebased on main as this PR is ancient and I wanted to ensure there were no silent conflicts in the meantime:

> git range-diff 7153e3560...9b420e325 -- lightning/src/util

-:  --------- >  1:  0282b0d96 Drop spurious debug assertion in sweeping logic
 1:  4c70b5676 =  2:  2ca9a955e `rustfmt`: Run on `util/scid_utils.rs`
 2:  09d365b41 =  3:  be298f6df `rustfmt`: Run on `util/ser.rs`
 3:  a29868db1 =  4:  229a712d5 `rustfmt`: Prepare `util/ser_macros.rs`
 4:  60bf9eeaa =  5:  94c0e6aa5 `rustfmt`: Run on `util/ser_macros.rs`
 5:  d372bfdb5 !  6:  defe40494 `rustfmt`: Prepare `util/test_channel_signer.rs`
    @@ lightning/src/util/test_channel_signer.rs: impl EcdsaChannelSigner for TestChann
                        ).unwrap();
                }
     -          Ok(EcdsaChannelSigner::sign_holder_htlc_transaction(&self.inner, htlc_tx, input, htlc_descriptor, secp_ctx).unwrap())
    -+          self.inner.sign_holder_htlc_transaction(htlc_tx, input, htlc_descriptor, secp_ctx)
    ++          Ok(self.inner.sign_holder_htlc_transaction(htlc_tx, input, htlc_descriptor, secp_ctx).unwrap())
        }

        fn sign_counterparty_htlc_transaction(&self, htlc_tx: &Transaction, input: usize, amount: u64, per_commitment_point: &PublicKey, htlc: &HTLCOutputInCommitment, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()> {
 6:  9142bf5f3 !  7:  251f22687 `rustfmt`: Run on `util/test_channel_signer.rs`
    @@ lightning/src/util/test_channel_signer.rs: impl EcdsaChannelSigner for TestChann
     +                          )
     +                          .unwrap();
                }
    -           self.inner.sign_holder_htlc_transaction(htlc_tx, input, htlc_descriptor, secp_ctx)
    +-          Ok(self.inner.sign_holder_htlc_transaction(htlc_tx, input, htlc_descriptor, secp_ctx).unwrap())
    ++          Ok(self
    ++                  .inner
    ++                  .sign_holder_htlc_transaction(htlc_tx, input, htlc_descriptor, secp_ctx)
    ++                  .unwrap())
        }

     -  fn sign_counterparty_htlc_transaction(&self, htlc_tx: &Transaction, input: usize, amount: u64, per_commitment_point: &PublicKey, htlc: &HTLCOutputInCommitment, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()> {
 7:  58b455719 =  8:  95961f81d `rustfmt`: Prepare `util/test_utils.rs`
 8:  cc42fd606 =  9:  87f5694de `rustfmt`: Run on `util/test_utils.rs`
 9:  cd379a425 = 10:  709d89f3f `rustfmt`: Run on `util/time.rs`
10:  0a752b972 = 11:  0c230e1a9 `rustfmt`: Prepare `util/transaction_utils.rs`
11:  664ec5f66 = 12:  b7b882cc5 `rustfmt`: Run on `util/transaction_utils.rs`
12:  e14fafd41 = 13:  bbe6613bb `rustfmt`: Run on `util/wakers.rs`
13:  86a84adf6 = 14:  028fed479 `rustfmt`: Run on `util/mod.rs`

.. we pull out `Mutex` field initialization into dedicated variables as they might otherwise land on the same line when formatting, which might lead to lockorder violation false-positives when compiled with the `backtrace` feature.

TheBlueMatt

Good enough. Some stuff I'll clean up in a followup, some of it pre-existing.

PR lightningdevkit#3323 introduced some extra variables to avoid `rustfmt` making a total mess of our code, but introduced a few that don't make `rustfmt` do dumb things, which we remove here.

tnull commented Sep 18, 2024

View reviewed changes

lightning/src/util/ser.rs Show resolved Hide resolved

tnull force-pushed the 2024-09-rustfmt-util branch 2 times, most recently from 6c4dd83 to 9b991df Compare September 18, 2024 08:42

tnull mentioned this pull request Sep 18, 2024

rustfmt: Run on util/* (1/2) #3324

Merged

tnull added the blocked on dependent pr label Sep 18, 2024

tnull changed the title ~~rustfmt: Run on util/*~~ rustfmt: Run on util/* (2/2) Sep 18, 2024

tnull force-pushed the 2024-09-rustfmt-util branch 2 times, most recently from 60ae0af to e2dcdc7 Compare September 19, 2024 08:10

tnull force-pushed the 2024-09-rustfmt-util branch from e2dcdc7 to bc122e3 Compare October 1, 2024 16:01

tnull removed the blocked on dependent pr label Oct 7, 2024

tnull force-pushed the 2024-09-rustfmt-util branch from bc122e3 to 572f48c Compare October 7, 2024 08:23

tnull force-pushed the 2024-09-rustfmt-util branch from 572f48c to 449a0d6 Compare November 5, 2024 09:13

tnull requested a review from TheBlueMatt November 5, 2024 09:14

tnull force-pushed the 2024-09-rustfmt-util branch from 449a0d6 to b909a7a Compare November 18, 2024 17:57

TheBlueMatt reviewed Dec 2, 2024

View reviewed changes

tnull force-pushed the 2024-09-rustfmt-util branch from b909a7a to 0750550 Compare December 3, 2024 12:27

tnull force-pushed the 2024-09-rustfmt-util branch from 0750550 to 03ccbe8 Compare December 18, 2024 08:42

tnull force-pushed the 2024-09-rustfmt-util branch from 03ccbe8 to 7153e35 Compare December 18, 2024 08:46

TheBlueMatt reviewed Jan 16, 2025

View reviewed changes

lightning/src/util/test_channel_signer.rs Outdated Show resolved Hide resolved

tnull force-pushed the 2024-09-rustfmt-util branch from 7153e35 to d69ca2c Compare January 17, 2025 09:42

tnull force-pushed the 2024-09-rustfmt-util branch from d69ca2c to 9b420e3 Compare January 17, 2025 09:47

tnull added 22 commits January 20, 2025 10:54

rustfmt: Run on util/scid_utils.rs

0fc4877

rustfmt: Drop util/scid_utils.rs from exclusion list

a8a5162

rustfmt: Run on util/ser.rs

d436cf8

rustfmt: Drop util/ser.rs from exclusion list

74d57dc

rustfmt: Prepare util/ser_macros.rs

4568db8

rustfmt: Run on util/ser_macros.rs

1cc309d

rustfmt: Drop util/ser_macros.rs from exclusion list

f4f9535

rustfmt: Run on util/test_channel_signer.rs

fe53ac9

rustfmt: Drop util/test_channel_signer.rs from exclusion list

8f335e0

rustfmt: Prepare util/test_utils.rs

14da64b

.. we pull out `Mutex` field initialization into dedicated variables as they might otherwise land on the same line when formatting, which might lead to lockorder violation false-positives when compiled with the `backtrace` feature.

rustfmt: Run on util/test_utils.rs

12fc953

rustfmt: Drop util/test_utils.rs from exclusion list

767646c

rustfmt: Run on util/time.rs

5a27316

rustfmt: Drop util/time.rs from exclusion list

f2bc902

rustfmt: Prepare util/transaction_utils.rs

2be14b3

rustfmt: Run on util/transaction_utils.rs

b9fc8ba

rustfmt: Drop util/transaction_utils.rs from exclusion list

8e8c925

rustfmt: Run on util/wakers.rs

c349d79

rustfmt: Drop util/wakers.rs from exclusion list

e2aca6b

rustfmt: Run on util/mod.rs

5035638

rustfmt: Drop util/mod.rs from exclusion list

ce50c41

rustfmt: Drop remaining util files from exclusion list

5d6eaec

tnull force-pushed the 2024-09-rustfmt-util branch from 9b420e3 to 5d6eaec Compare January 20, 2025 09:55

TheBlueMatt approved these changes Jan 20, 2025

View reviewed changes

TheBlueMatt merged commit 86308e1 into lightningdevkit:main Jan 20, 2025
24 of 25 checks passed

TheBlueMatt mentioned this pull request Jan 20, 2025

utils cleanups #3552

Merged

rustfmt: Run on util/* (2/2) #3323

rustfmt: Run on util/* (2/2) #3323

Uh oh!

Conversation

tnull commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

TheBlueMatt commented Sep 18, 2024

Uh oh!

tnull commented Sep 18, 2024

Uh oh!

tnull commented Sep 19, 2024

Uh oh!

tnull commented Oct 1, 2024

Uh oh!

tnull commented Oct 7, 2024

Uh oh!

tnull commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull commented Nov 18, 2024

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

tnull Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

tnull Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

tnull Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tnull commented Dec 3, 2024

Uh oh!

tnull commented Dec 18, 2024

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

`rustfmt`: Run on `util/*` (2/2) #3323

`rustfmt`: Run on `util/*` (2/2) #3323

tnull commented Sep 18, 2024 •

edited

Loading

codecov bot commented Sep 18, 2024 •

edited

Loading

tnull commented Nov 5, 2024 •

edited

Loading

tnull commented Jan 17, 2025 •

edited

Loading