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.

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

Impl Ord and PartialOrd for RecoverableSignature #611

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

benthecarman
Copy link
Contributor

No description provided.

@apoelstra
Copy link
Member

Do you need the ordering to be consistent between LE and BE machines? If so, you probably need to serialize them before comparing, #derive will do the wrong thing. (We probably should fix this in ffi::RecoverableSignature actually.)

@benthecarman
Copy link
Contributor Author

Do you need the ordering to be consistent between LE and BE machines? If so, you probably need to serialize them before comparing, #derive will do the wrong thing. (We probably should fix this in ffi::RecoverableSignature actually.)

Yeah that'd be ideal. How do I do that?

@apoelstra
Copy link
Member

Yeah that'd be ideal. How do I do that?

See for example what we did with the regular signatures: https://github.com/rust-bitcoin/rust-secp256k1/blob/master/secp256k1-sys/src/lib.rs#L278-L286

That's in secp256k1-sys, the FFI lib. Then in the main library you can just #derive it like you're doing now.

@benthecarman
Copy link
Contributor Author

@apoelstra sorry never got to this, but looks like they already exist

impl Ord for RecoverableSignature {

@Kixunil
Copy link
Collaborator

Kixunil commented Jan 22, 2024

@benthecarman did you mean to close the PR?

@apoelstra
Copy link
Member

@Kixunil no, it should be merged :). This PR is about deriving the ordering traits on the rust-secp repo. I had said that we couldn't derive these since it would pass thorugh to the rust-secp-sys ordering traits, which were implementation defined. But actually they aren't implementation-defined and there is no problem.

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 dbc5465 oops, sorry!

@Kixunil
Copy link
Collaborator

Kixunil commented Jan 22, 2024

Oh, I thought this one does the same thing as the linked code but the linked is on -sys and this one is in non--sys.

@apoelstra apoelstra merged commit 4dede13 into rust-bitcoin:master Jan 22, 2024
20 checks passed
@benthecarman benthecarman deleted the ord-recoverable-sig branch January 22, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants