-
Notifications
You must be signed in to change notification settings - Fork 264
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
Re-implement public key ordering using underlying FFI functions #449
Conversation
c5786cb
to
9589d0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like #309 (comment) wasn't implemented.
With cfg(fuzzing) now there is no implementation of Ord for PublicKey, which will cause downstream compliation failures during fuzzing.
can you implement some sort of ordering for fuzzable public keys? You can even just auto-derive one.
src/key.rs
Outdated
v if v < 0 => core::cmp::Ordering::Less, | ||
v if v > 0 => core::cmp::Ordering::Greater, | ||
_ => unreachable!() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole match
can be replaced by ret.cmp(&0i32)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ha! So it can :)
9589d0d
to
cbf72bd
Compare
Changes in force push:
|
Instead of selializing the key we can call down to the ffi layer to do ordering. Co-authored-by: Tobin C. Harding <me@tobin.cc>
Feature guard the custom implementations of `Ord` and `PartialOrd` on `cfg(not(fuzzing))`. When fuzzing, auto-derive implementations. Co-authored-by: Tobin C. Harding <me@tobin.cc>
cbf72bd
to
13af519
Compare
I have added/re-written the commit logs of the last two patches and added a |
I don't think we should put |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 13af519
@elichai care to explain why? |
I think any code using production rust-secp should compile under I looked again now and saw |
@elichai ah, clean code, yes. I think derives are a meaningful exception. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 13af519
Re-base #309 for @dr-orlovsky on request by @Kixunil.
To do the rebase I just had to change instances of cfg_attr to use
v0_5_0
instead ofv0_4_1
e.g.,And drop the changes to
src/schnorrsig.rs
, all these changes are covered by the changes inkey.rs
I believe.