search

diwakergupta / stacks-blockchain-tob-audit

GNU General Public License v3.0
0 stars 0 forks source link

PeerHost uses auto-derived PartialEq impl with manually-written Hash impl #26

Open bradlarsen opened 3 years ago

bradlarsen commented 3 years ago

The PeerHost type uses a derived PartialEq implementation:

https://github.com/trailofbits/x-audit-blockstack-core/blob/d35ef465e9fa2ce327a181117f8ca7933b9df075/src/net/mod.rs#L813-L818

But later, a handwritten Hash implementation is provided:

https://github.com/trailofbits/x-audit-blockstack-core/blob/d35ef465e9fa2ce327a181117f8ca7933b9df075/src/net/mod.rs#L838-L853

There doesn't appear to be a problem with this at present.

However, if the PeerHost type or the manual Hash implementation were changed in the future, there is a risk of the two getting out of sync and violating a property that is expected to hold between implementations:

k1 == k2 -> hash(k1) == hash(k2)

It looks like PeerHost could also derive Hash? If there is a good reason why this isn't done now, a one-line comment on the impl Hash for Peerhost could clarify.