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.

Sign up for GitHub

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

Jump to bottom

Re-implement public key ordering using underlying FFI functions #449

Merged
merged 4 commits into from Jun 15, 2022
Merged

Re-implement public key ordering using underlying FFI functions #449

merged 4 commits into from Jun 15, 2022

Conversation

tcharding
Copy link
Member

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 of v0_4_1 e.g.,

    #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_5_0_xonly_pubkey_cmp")]

And drop the changes to src/schnorrsig.rs, all these changes are covered by the changes in key.rs I believe.

@tcharding tcharding changed the title 06 14 extrakeys cmp Re-implement public key ordering using underlying FFI functions Jun 14, 2022
@tcharding tcharding mentioned this pull request Jun 14, 2022
Merged
Kixunil
Kixunil reviewed Jun 14, 2022
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a 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!()
}
Copy link
Collaborator

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha! So it can :)

@tcharding
Copy link
Member Author

Changes in force push:

  • Added cmp suggestion to the relevant patch (commit: 70a4441 Re-implementing PublicKey ordering using FFI)
  • Added derive(Ord, PartialOrd) to PublicKey using cfg_attr when fuzzing (added to last patch)

dr-orlovsky and others added 2 commits June 15, 2022 08:34
@dr-orlovsky @tcharding
Implement PublicKey ordering using FFI
7396604 
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>
@dr-orlovsky @tcharding
Make key comparison non-fuzzable
13af519 
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>
@tcharding
Copy link
Member Author

I have added/re-written the commit logs of the last two patches and added a co-authored-by tag, not to claim authorship but to accept blame if anything is wrong because these two patches have been non-trivially changed since @dr-orlovsky first posted them.

@elichai
Copy link
Member

elichai commented Jun 15, 2022

I don't think we should put #[cfg(not(fuzzing))] on PartialOrd, instead we should probably add an impl to the fuzzing module (maybe something like just doing == between the arrays?)

Kixunil
Kixunil approved these changes Jun 15, 2022
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 13af519

@Kixunil
Copy link
Collaborator

Kixunil commented Jun 15, 2022

@elichai care to explain why?

@elichai
Copy link
Member

elichai commented Jun 15, 2022

@elichai care to explain why?

I think any code using production rust-secp should compile under fuzz rust-secp, otherwise it makes fuzzing dependent projects much harder

I looked again now and saw #[cfg_attr(feature = "fuzzing", derive(PartialOrd, Ord))] which I missed the first time.
So I guess this is fine, I still think we should try to "contain" the fuzz/production in the FFI modules and not let it leak into the whole codebase.

@Kixunil
Copy link
Collaborator

Kixunil commented Jun 15, 2022

@elichai ah, clean code, yes. I think derives are a meaningful exception.

apoelstra
apoelstra approved these changes Jun 15, 2022
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 13af519

@apoelstra apoelstra merged commit aba2663 into rust-bitcoin:master Jun 15, 2022
@apoelstra apoelstra mentioned this pull request Jun 15, 2022
Closed
@tcharding tcharding deleted the 06-14-extrakeys-cmp branch June 20, 2022 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil approved these changes

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tcharding @elichai @Kixunil @apoelstra @dr-orlovsky