-
Notifications
You must be signed in to change notification settings - Fork 265
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 #382
Conversation
Not sure I am getting why we can construct parity from arbitrary integer... |
src/key.rs
Outdated
#[derive(Copy, Clone, PartialEq, Eq, Debug, PartialOrd, Ord, Hash)] | ||
pub struct Parity(i32); | ||
pub enum Parity { | ||
/// Ever parity. |
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.
s/ever/even
I agree, I think we should only accept 0/1 and return an error otherwise (but, should those functions even exist? or should users serialize/deserialize it however they want?) |
src/key.rs
Outdated
Even, | ||
/// Odd parity. | ||
Odd, | ||
} |
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.
If you define
Even = 0,
Odd = 1,
then we can just cast this into a i32
using tweaked_parity as i32
idk if this casting is more or less preferred though (I know some people dislike this kind of C-like casts)
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.
I think we should support it, it's really convenient and we don't need to have an opinion about whether it's good style.
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.
FTR the cast works without explicitly setting the variant values. I have however, added the explicit variant values and removed the matching in to_u8
in favour of casting. This puts the mapping of 0/1 -> Even/Odd in a single place only.
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.
Also, a benefit of the explicit values is not relying on the language implementation.
I agree with the comment suggesting we only allow 0 and 1. I also think that, if we keep the |
We can't only allow 0 and 1 (implies fallible conversion) and also keep the We could do this:
|
I've implemented all suggestions. I'm not happy with the
Ideas please? |
@tcharding rather than implementing |
Done, added a deprecation comment as well. Cheers. |
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 511b1e6
Will wait for other ACKs since I know people have opinions about this type :)
This actually still changes the behavior of the code. |
Oof, subtle, thanks @elichai. I'm tempted to just let this slide because I don't see any sensible way to work around it, and it's a fairly small behavior change that would only hit users doing invalid things. |
I tend to agree, I doubt any user is already using the |
src/key.rs
Outdated
fn bitxor(self, rhs: Parity) -> Self::Output { | ||
Parity::from_u8(self.to_u8() ^ rhs.to_u8()).expect("XOR result is always a valid parity value") |
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.
I once asked myself: Why do languages have a bitwise XOR operator but no logical XOR operator? The answer is that have one, it's !=
.
fn bitxor(self, rhs: Parity) -> Self::Output { | |
Parity::from_u8(self.to_u8() ^ rhs.to_u8()).expect("XOR result is always a valid parity value") | |
fn bitxor(self, rhs: Parity) -> Self::Output { | |
self != rhs |
Much simpler but the current code is of course also correct.
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.
Cool, I had never thought of this. This only works in code if parity was an int and the language used 1 for true and 0 for false (i.e., C code). In Rust we'd have to map from true/false -> Parity
. We can do this but I think every dev that reads the code is going to have to scribble down a truth table for XOR to convince themselves that this is the case (well I did anyways).
/// This works because Parity has only two values (i.e. only 1 bit of information).
if self == rhs {
Parity::Even
} else {
Parity::Odd
}
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.
I'm unsure right now which to use?
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.
Yeah, and of course we'd need your version because mine returned bool and not Parity. (In fact we reimplemented bool and that's not surprising: there are not many meaningful types with exactly two members.)
I still think the version with the if is more elegant than the original one. But sorry for starting this bikeshedding...
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.
I used the if version with some additional comments, cheers.
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.
FWIW both compile down to a simple xor
instruction: https://godbolt.org/z/5WzYd8WoY
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.
Wow, I'm amazed at that assembler, all that high level language gone down to just three instructions - that's cool.
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.
God bless Zero Cost Abstractions ;)
Recently we made a wee mess with the `Parity` opaque type. Let's fix it up by doing: - Use an enum with variants `Even` and `Odd`. - Add explicit conversion methods to/from u8 and i32 - Implement `BitXor` Note: This patch is an API breaking change that does _not_ follow the deprecation guidelines. Rust does not allow deprecating `From` impl blocks AFAICT.
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 6fad20e
aa51638 update changelog for 0.22.0 (Andrew Poelstra) d06dd20 update fuzzdummy API to match normal API (Andrew Poelstra) f3d48a2 update "should terminate abnormally" test to trigger a different ARG_CHECK (Andrew Poelstra) 8294ea3 secp256k1-sys: update upstream library (Andrew Poelstra) 2932179 secp256k1-sys: update secp256k1.h.patch (Andrew Poelstra) Pull request description: Should wait on merging until we get a minor release out with #382 and #376. May also want to bundle #380 with this? ACKs for top commit: real-or-random: ACK aa51638 I can't judge if the feature set is meaningful but this release PR is fine Tree-SHA512: e7f48b402378e280a034127f2de58d3127e04303a114f07f294fa3d00c0a083ae0d43375a8a74d226b13ea45fb3fde07d8450790e602bbf9581adc5fd8bc7d29
Recently we made a wee mess with the
Parity
opaque type. Let's fix itup by doing:
Even
andOdd
.BitXor
This PR attempts to follow the plan laid out in the issue but:
to_u8
is requested, I added it anyways.to_i32
is not mentioned but we need it if we are going to pass the parity back to FFI code after receiving it without having to care about the value. And that is the whole point of thisParity
type in the first place.from_xxx
is fallible, when is an integer not even or odd?Please note: This patch is an API breaking change that does not follow the deprecation guidelines. Rust does not allow deprecating
From
impl blocks AFAICT.Fixes: #370