Manually implement PartialEq, Eq, and Hash for PublicKey
#506
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
Patch 1 looks like it removes the I don't understand though what this has to do with Patch 2, which (correctly) changes the Finally, the current behavior of |
Member
Author
You are right, not sure what I was thinking yesterday. I'll split the first patch off into its own PR. |
tcharding
force-pushed
the
11-07-pk-manual-eq-hash
branch
from
20d0381 to
587e838
Compare
tcharding
changed the title
PartialEq, Eq, and Hash for PublicKey
`PartialEq` and `Eq` should agree with `PartialOrd` and `Ord` but we are deriving `PartialEq`/`Eq` and doing a custom implementation of `PartialOrd` and `Ord` (that calls down to ffi functions). If two keys are equal their hashes should be equal so, we should add a custom implementation of `Hash` also. In order to guarantee the digest will be the same across library versions first serialize the key before hashing it. Add custom implementation of `PartialEq`, `Eq`, and `Hash` when not fuzzing. Please note, this is for the main `PublicKey` type, the patch does not effect the `ffi::PublicKey`, nor do we call methods on the `ffi::PublicKey`.
tcharding
force-pushed
the
11-07-pk-manual-eq-hash
branch
from
587e838 to
5ccf0c8
Compare
apoelstra
approved these changes
Nov 12, 2022
Member
|
I'm gonna go ahead and merge this. If people need high-performance comparisons it's not too difficult to work around this and bypass our |
This was referenced Nov 13, 2022
chain-forgexcr45
added a commit
to chain-forgexcr45/rust-secp256k1
that referenced
this pull request
Sep 28, 2025
…, `Eq`, and `Hash` for `PublicKey`
5ccf0c8db7cd63ab80ff9388565e258d4faec4c6 Manually implement PartialEq, Eq, and Hash for PublicKey (Tobin C. Harding)
Pull request description:
`PartialEq` and `Eq` should agree with `PartialOrd` and `Ord` but we are deriving `PartialEq`/`Eq` and doing a custom implementation of `PartialOrd` and `Ord` (that calls down to ffi functions).
If two keys are equal their hashes should be equal so, we should add a custom implementation of `Hash` also. In order to guarantee the digest will be the same across library versions first serialize the key before hashing it.
Add custom implementation of `PartialEq`, `Eq`, and `Hash` when not fuzzing.
Please note, this is for the main `PublicKey` type, the patch does not effect the `ffi::PublicKey`, nor do we call methods on the `ffi::PublicKey`.
EDIT: Please note the comment below by apoelstra about the possible performance hit introduced by this PR.
ACKs for top commit:
apoelstra:
ACK 5ccf0c8db7cd63ab80ff9388565e258d4faec4c6
Tree-SHA512: 1464308238411d259bb0493dc1eca775ec235036eef10b91f70ef17816174f452d5911ecae3b40434b71f9866be1db54d69e8ed9475a4f2801c07a800aead2b2
william2332-limf
added a commit
to william2332-limf/rust-secp256k1
that referenced
this pull request
Oct 2, 2025
…, `Eq`, and `Hash` for `PublicKey`
5ccf0c8db7cd63ab80ff9388565e258d4faec4c6 Manually implement PartialEq, Eq, and Hash for PublicKey (Tobin C. Harding)
Pull request description:
`PartialEq` and `Eq` should agree with `PartialOrd` and `Ord` but we are deriving `PartialEq`/`Eq` and doing a custom implementation of `PartialOrd` and `Ord` (that calls down to ffi functions).
If two keys are equal their hashes should be equal so, we should add a custom implementation of `Hash` also. In order to guarantee the digest will be the same across library versions first serialize the key before hashing it.
Add custom implementation of `PartialEq`, `Eq`, and `Hash` when not fuzzing.
Please note, this is for the main `PublicKey` type, the patch does not effect the `ffi::PublicKey`, nor do we call methods on the `ffi::PublicKey`.
EDIT: Please note the comment below by apoelstra about the possible performance hit introduced by this PR.
ACKs for top commit:
apoelstra:
ACK 5ccf0c8db7cd63ab80ff9388565e258d4faec4c6
Tree-SHA512: 1464308238411d259bb0493dc1eca775ec235036eef10b91f70ef17816174f452d5911ecae3b40434b71f9866be1db54d69e8ed9475a4f2801c07a800aead2b2
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PartialEqandEqshould agree withPartialOrdandOrdbut we are derivingPartialEq/Eqand doing a custom implementation ofPartialOrdandOrd(that calls down to ffi functions).If two keys are equal their hashes should be equal so, we should add a custom implementation of
Hashalso. In order to guarantee the digest will be the same across library versions first serialize the key before hashing it.Add custom implementation of
PartialEq,Eq, andHashwhen not fuzzing.Please note, this is for the main
PublicKeytype, the patch does not effect theffi::PublicKey, nor do we call methods on theffi::PublicKey.EDIT: Please note the comment below by apoelstra about the possible performance hit introduced by this PR.