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

Fix the mess around Parity #370

Closed
tcharding opened this issue Jan 10, 2022 · 6 comments · Fixed by #382
Closed

Fix the mess around Parity #370

tcharding opened this issue Jan 10, 2022 · 6 comments · Fixed by #382

Comments

@tcharding
Copy link
Member

Recently I opened up a hornets nest by changing the parity boolean into an opaque type (#344). This PR was also flawed, @sanket1729 came to the rescue in #360 and #361.

There has now been much talk and disagreement on this, listing links here in an attempt to collect all the comments (in no particular order):

@apoelstra
Copy link
Member

My proposed solution:

  • Redefine Parity to be an enum Even/Odd rather than an opaque type (not a breaking change)
  • Add to_u8 and fallable from_u8 methods (we can bikeshed on names, I don't care) (not a breaking change)
  • Add an xor method between two Paritys/overload the ^ operator (not a breaking change)
  • Deprecate the existing i32 conversion methods
  • Make sure we have all the necessary #derives :) (I think this is done)

So I think we can clean this all up with another minor version.

@tcharding
Copy link
Member Author

Just noting that the first step is where we got stuck last time, the naming of Even/Odd and not knowing how this maps to the integer value.

@dr-orlovsky
Copy link
Contributor

dr-orlovsky commented Jan 10, 2022

Hm, isn't it quite obvious: odd should be a first odd integer value - and even should be a first even integer value, i.e. odd is 1 and even is 0.

0 is indeed even, since 0 % 2 == 0, and by definition even are the integers which divide by 2 without reminder.

@apoelstra
Copy link
Member

I agree that the mapping between 0/1 and even/odd is natural .. I'm not sure how this maps to the 02/03 prefixes on full keys, but I should hope also in the natural way!

@elichai
Copy link
Member

elichai commented Jan 12, 2022

Yeah, in libsecp 0 is also used for even and 1 for odd.
(and in compressed 0x02 is for even and 0x03 is for odd)

I'm still somewhat uncomfortable exposing to users that public keys can even be "Even" or "Odd", but maybe this ship has long sailed.

@tcharding
Copy link
Member Author

TIL: Cannot deprecate implementation of From.

#[deprecated(since = "0.21.3", note = "Please use Parity::from_i32")]
impl From<i32> for Parity {
    fn from(parity: i32) -> Parity {
        Parity::from_i32(parity)
    }
}

Results in:
error: this #[deprecated] annotation has no effect
--> src/key.rs:928:1
|
928 | #[deprecated(since = "0.21.3", note = "Please use Parity::from_i32")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the unnecessary deprecation attribute
|
= note: #[deny(useless_deprecated)] on by default

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 a pull request may close this issue.

4 participants