Remove derives for ffi wrapped types #515
Closed
Conversation
This file contains 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
tcharding
force-pushed
the
11-16-rm-autoderive-hash
branch
from
1b552bb
to
1bc1e01
Compare
|
Uses the derives when fuzzing like |
tcharding
force-pushed
the
11-16-rm-autoderive-hash
branch
2 times, most recently
from
a29f91e
to
64a22fe
Compare
Currently we are deriving various traits that call down to the traits implemented in secp265k1-sys that in turn call down to implementations on the inner byte array. Since we cannot guarantee the inner byte arrays in secp256k1-sys this results in trait implementations that may not be stable across versions of the library. For `XOnlyPublicKey`, `KeyPair`, and `ecdsa::Signature` manually implement `PartialEq`, `Eq`, `PartialOrd`, `Ord`, and `Hash` by first converting the type into a form that is guaranteed to be stable across library versions e.g., by serializing it to a known format.
tcharding
force-pushed
the
11-16-rm-autoderive-hash
branch
from
64a22fe
to
86608c2
Compare
We recently improved the comparison implementations to be stable across library versions by first serializing the data types before comparing them instead of just comparing the inner bytes. This had the downside that the comparisons are now slower. Add public methods `cmp_fast_unstable` and `eq_fast_unstable` to each of the types that wraps an FFI type.
|
Converting to draft while I work out the resolution to the second part of this comment: #463 (comment) |
|
Superseded by #518 |
apoelstra
added a commit
that referenced
this pull request
Nov 21, 2022
9850550 Move AsRef impl block next to Index (Tobin C. Harding) 4d42e8e Derive Copy and Clone (Tobin C. Harding) b38ae97 Implement stable comparison functionality (Tobin C. Harding) 630fc1f Remove len and is_empty from impl_array_newtype macros (Tobin C. Harding) 9788b6d Remove leading colons from impl_array_newtype methods (Tobin C. Harding) 2bb08c2 Remove as_[mut_]ptr from impl_array_newtype macros (Tobin C. Harding) 3e28070 Duplicate impl_array_newtype (Tobin C. Harding) 6358903 Add newline to end of file (Tobin C. Harding) Pull request description: Supersedes #515 The primary aim of this PR is to fix the fact that we currently implement various comparison traits (`Ord`, `PartialEq`) by comparing the inner byte arrays. These bytes are meant to be opaque and are not guaranteed across versions of `libsecp256k1`. This PR is a bit involved because I had to detangle all the various types (across both `secp256k1` and `secp256k1-sys`) that use the `impl_array_newtype` macro. - Patch 1: is trivial cleanup - Patch 2: Step one of the PR is duplicating the macro into both crates so we can tease them apart. - Patch 3-5: Are cleanup of the now duplicated `impl_array_newtype` macros - Patch 6: Is the meat and potatoes - Patch 7,8: Further minor clean ups to the macro I had a lot of fun with this PR, I hope you enjoy it too. Fix: #463 ACKs for top commit: apoelstra: ACK 9850550 Tree-SHA512: 160381e53972ff882ceb1d2d47bac56a7301a2d13bfe75d3f6ff658ab2c6fbe516ad856587c4d23f98524205918ca1a5f9b737e35c23c7a01b476c92d8d1792f
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Currently we are deriving various traits that call down to the traits implemented in
secp265k1-systhat in turn call down to implementations on the inner byte array. Since we cannot guarantee the inner byte arrays insecp256k1-systhis results in trait implementations that may not be stable across versions of the library.For
XOnlyPublicKey,KeyPair, andecdsa::Signaturemanually implementPartialEq,Eq,PartialOrd,Ord, andHashby first converting the type into a form that is guaranteed to be stable across library versions e.g., by serializing it to a known format.EDIT: Now includes a patch to add fast unstable comparisons.
Review notes
Hashimplementation forKeyPair- I am not a cryptographer.PublicKeyalready in Manually implementPartialEq,Eq, andHashforPublicKey#506