Skip to content

Avoid unnecessarily cloning unsigned Transaction when broadcasting #2582

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

Our Trusted* wrappers in chan_utils expose additional inner
fields by reference. However, because they were not explicitly
marked as returning a reference with the wrapped struct's
lifetimes, rustc was considering them to return a reference with
the wrapper struct's lifetime.

This is unnecessarily restrictive, and resulted in the addition of
a clone in 9850c58 which we remove
here.

Our `Trusted*` wrappers in `chan_utils` expose additional inner
fields by reference. However, because they were not explicitly
marked as returning a reference with the wrapped struct's
lifetimes, rustc was considering them to return a reference with
the wrapper struct's lifetime.

This is unnecessarily restrictive, and resulted in the addition of
a clone in 9850c58 which we remove
here.
@@ -1668,17 +1668,17 @@ impl<'a> TrustedCommitmentTransaction<'a> {
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't this apply to Txid?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not returned by reference, its returned by copy, so there's no need to add the lifetime.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I meant its equivalent to clone right ? 😅
But yeah since its already like that, nbd i guess.

@G8XSU
Copy link
Contributor

G8XSU commented Sep 15, 2023

Otherwise, LGTM.

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 91.66% and project coverage change: -0.01% ⚠️

Comparison is base (89fb5a3) 90.63% compared to head (53c8f89) 90.63%.
Report is 10 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2582      +/-   ##
==========================================
- Coverage   90.63%   90.63%   -0.01%     
==========================================
  Files         113      113              
  Lines       59054    59057       +3     
  Branches    59054    59057       +3     
==========================================
+ Hits        53522    53524       +2     
- Misses       5532     5533       +1     
Files Changed Coverage Δ
lightning/src/ln/chan_utils.rs 95.03% <85.71%> (ø)
lightning/src/chain/channelmonitor.rs 94.59% <100.00%> (-0.06%) ⬇️
lightning/src/chain/onchaintx.rs 90.95% <100.00%> (+0.04%) ⬆️

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

Copy link
Contributor

@G8XSU G8XSU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@@ -1668,17 +1668,17 @@ impl<'a> TrustedCommitmentTransaction<'a> {
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I meant its equivalent to clone right ? 😅
But yeah since its already like that, nbd i guess.

@TheBlueMatt TheBlueMatt merged commit f97d520 into lightningdevkit:main Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants