Skip to content

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
@benthecarman benthecarman deleted the ord-recoverable-sig branch January 22, 2024 19:22
chain-forgexcr45 added a commit to chain-forgexcr45/rust-secp256k1 that referenced this pull request Sep 28, 2025
…coverableSignature

dbc546596f4c7b2d4d1e489aa7e91775eaf54bb0 Impl Ord and PartialOrd for RecoverableSignature (benthecarman)

Pull request description:

ACKs for top commit:
  apoelstra:
    ACK dbc546596f4c7b2d4d1e489aa7e91775eaf54bb0 oops, sorry!

Tree-SHA512: decda6b6e7a4929147f5ca00cb2802037c6a297aa816c253e181f1a85ec9e46958469b9d481e2b4aba2c6d86def80bba33a62c507459d82c86cfed4271a23eea
william2332-limf added a commit to william2332-limf/rust-secp256k1 that referenced this pull request Oct 2, 2025
…coverableSignature

dbc546596f4c7b2d4d1e489aa7e91775eaf54bb0 Impl Ord and PartialOrd for RecoverableSignature (benthecarman)

Pull request description:

ACKs for top commit:
  apoelstra:
    ACK dbc546596f4c7b2d4d1e489aa7e91775eaf54bb0 oops, sorry!

Tree-SHA512: decda6b6e7a4929147f5ca00cb2802037c6a297aa816c253e181f1a85ec9e46958469b9d481e2b4aba2c6d86def80bba33a62c507459d82c86cfed4271a23eea
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