Skip to content
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

derive(Hash) for P2P messages #2716

Merged
merged 2 commits into from
Nov 14, 2023

Conversation

TheBlueMatt
Copy link
Collaborator

In other languages (Java and C#, notably), overriding Eq without overriding Hash can lead to surprising or broken behavior. Even in Rust, its usually the case that you actually want both. Here we add missing Hash derivations for P2P messages, to at least address the first pile of warnings the C# compiler dumps.

tnull
tnull previously approved these changes Nov 7, 2023
tnull
tnull previously approved these changes Nov 8, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2023

Codecov Report

Attention: 55 lines in your changes are missing coverage. Please review.

Comparison is base (a025c7c) 88.82% compared to head (26c00ad) 88.96%.
Report is 2 commits behind head on main.

Files Patch % Lines
lightning/src/ln/msgs.rs 1.92% 15 Missing and 36 partials ⚠️
lightning/src/util/ser.rs 0.00% 1 Missing and 1 partial ⚠️
lightning/src/onion_message/packet.rs 0.00% 0 Missing and 1 partial ⚠️
lightning/src/routing/gossip.rs 0.00% 0 Missing and 1 partial ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2716      +/-   ##
==========================================
+ Coverage   88.82%   88.96%   +0.13%     
==========================================
  Files         113      113              
  Lines       89201    91620    +2419     
  Branches    89201    91620    +2419     
==========================================
+ Hits        79235    81509    +2274     
- Misses       7727     7838     +111     
- Partials     2239     2273      +34     

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

@tnull
Copy link
Contributor

tnull commented Nov 8, 2023

Ugh, still fails with

error[E0277]: the trait bound `PublicNonce: std::hash::Hash` is not satisfied
   --> lightning/src/ln/msgs.rs:333:2
    |
293 | #[derive(Clone, Debug, Hash, PartialEq, Eq)]
    |                        ---- in this derive macro expansion
...
333 |     pub next_local_nonce: Option<musig2::types::PublicNonce>,
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::hash::Hash` is not implemented for `PublicNonce`
    |
    = help: the trait `std::hash::Hash` is implemented for `Option<T>`
    = note: required for `Option<PublicNonce>` to implement `std::hash::Hash`
    = note: this error originates in the derive macro `Hash` (in Nightly builds, run with -Z macro-backtrace for more info)

@TheBlueMatt
Copy link
Collaborator Author

@arik-so can you implement Hash in your musig2 library?

G8XSU
G8XSU previously approved these changes Nov 8, 2023
@arik-so
Copy link
Contributor

arik-so commented Nov 13, 2023

arik-so/rust-musig2#9

@TheBlueMatt
Copy link
Collaborator Author

Rebased after the dep bump.

@G8XSU
Copy link
Contributor

G8XSU commented Nov 14, 2023

LGTM! CI is failing

Implementation of standard traits on arrays longer than 32 elements
was shipped in rustc 1.47, which is below our MSRV of 1.48 and we
can use to remove some unnecessary manual implementation of
`PartialEq` on `OnionPacket`.
In other languages (Java and C#, notably), overriding `Eq` without
overriding `Hash` can lead to surprising or broken behavior. Even
in Rust, its usually the case that you actually want both. Here we
add missing `Hash` derivations for P2P messages, to at least
address the first pile of warnings the C# compiler dumps.
@TheBlueMatt
Copy link
Collaborator Author

Yea, sorry, just had to squash that fixup commit.

@tnull tnull merged commit 04b16e7 into lightningdevkit:main Nov 14, 2023
14 of 15 checks passed
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.

None yet

6 participants