Improve handling of parity integer

[tcharding]

Two functions in the FFI secp code return and accept a parity integer.

Currently we are manually converting this to a bool. Doing so forces readers of the code to think what the bool means even though understanding this value is not needed since in is just passed back down to the FFI code.

We initially tried to solve this issue by adding an enum, discussion below refers to that version. Instead of an enum we can solve this issue by adding an opaque type that holds the parity value returned by the FFI function call and then just pass it back down to FFI code without devs needing to know what the value should be. This fully abstracts the value away and removes the boolean conversion code which must otherwise be read by each dev.

• Patch 1 and 2 improve unit tests that test the code path modified by this PR
• Patch 3 trivially changes code to be uniform between two similar methods (tweak_add_assign)
• Patch 4 is the meat and potatoes (main part of PR :)
• Patch 5 is docs improvements to code in the area of this PR

[real-or-random]

I agree that Even/Odd would be nicer in general but:

Draft because I don't know how to map true/false to even/odd?

Parity should mean 0 is even. But yeah, I believe this is the problem with this change. rust-secp256k1 should be compatible with rust-secp256k1, and if upstream uses a parity bit, then we should do the same. Because only then users can be sure that if the bit is the same, they get the same result on rust-secp256k1 and on secp256k1.

Maybe we can convince upstream to rename parity to odd (or even... if I'm wrong above).

[tcharding]

Thanks for the review @real-or-random. I get your intention, users of rust-secp256k1 should be sure that the library matches secp256k1 but we already change the type from an int to a bool so we have already introduced the reliance on us doing that mapping correctly so if your point is to stand, we should change the parity to an integer type and not use a boolean. What about if we used an enum and explicitly set the Even variant to 0 (even though it is already) and then in the mapping of secp256k1's int to rust-secp256k1's enum we explicitly just use the 0 value to set the enum?

Would you be happy with something like this:

/// Parity returned when tweaking a key.
///
/// We explicitly set Even to 0 to ensure that the integer returned by secp256k1 maps to Even if
/// zero and Odd if anything else.
#[derive(Copy, Clone, PartialEq, Eq, Debug, PartialOrd, Ord, Hash)]
pub enum KeyParity {
    Even = 0,
    Odd,
}

impl From<i32> for KeyParity {
    fn from(u: i32) -> Self {
        match u {
            0 => KeyParity::Even,
            _ => KeyParity::Odd,
        }
    }
}

[elichai]

Thanks for the review @real-or-random. I get your intention, users of rust-secp256k1 should be sure that the library matches secp256k1 but we already change the type from an int to a bool so we have already introduced the reliance on us doing that mapping correctly so if your point is to stand, we should change the parity to an integer type and not use a boolean.

nit, boolean values don't exist in C89, so this int actually is equivalent to a bool in rust as it is documented to be used as a bool (only 1/0)
https://github.com/bitcoin-core/secp256k1/blob/f4edfc758142d6e100ca5d086126bf532b8a7020/include/secp256k1_extrakeys.h#L89-L91

I personally wouldn't be against adding an enum, but I'd prefer if we can somehow not expose "even/odd" terminology to the user at all and somehow create an abstraction above this, as users IMO shouldn't know what even/odd means
(note that I don't have an abstraction in mind so sorry that this isn't too helpful)

[apoelstra]

concept ACK using an enum for this.

@tcharding is right that we already change the type from an int to a bool, and this is not a trivial change because in libsecp there is a weird mask that you have to put on the int that you pass, it's not just a zero/nonzero thing. We map this to a bool in rust-secp. It would be better to map it to an enum instead.

Edit: oh, I misunderstood. I thought this was talking about key compressedness rather than parity. For parity I also think we should have an enum, but it needs to have explicit conversion functions between the numeric value (which is encoded in some places) and the enum.

[tcharding]

but it needs to have explicit conversion functions between the numeric value (which is encoded in some places) and the enum.

@apoelstra can you please confirm that I've got all the encodings you refer to. I grepped for the function names involved and I believe I've got them all.

[apoelstra]

ACK ede114f

This looks good to me. I haven't been involved with this API upstream so I'm probably not the best person to ask about having gotten all the conversion sites, but I think you did. (I grepped for some variations of {1} or {true} which would indicate an int->boolean conversion, and didn't find any.)

[elichai]

I actually liked that it was a single byte and that we didn't expose that c_int == i32.

[apoelstra]

I wouldn't mind changing the conversion function to go to/from a u8 rather than an i32, if that's your only concern.

[sanket1729]

I actually liked that it was a single byte and that we didn't expose that c_int == i32.

In rust-bitcoin, i32::from(self.output_key_parity) as u8 | self.leaf_version.as_u8()

This is not the end of the world. Maybe in the next release, we can have a custom enum

[apoelstra]

I think a u8 would be more conceptually clear.

[sanket1729]

Why not a custom enum?

[real-or-random]

post merge review: I'm not entirely convinced that a opaque type is useful here. You can't even XOR two parities which seems to be no so uncommon in advanced signing constructions. (If more is added to tweaking, e.g., musig). Not sure if really the user needs to access the value but on the other hand, I don't see how this opaque value is any safer than a bool.

[apoelstra]

We could implement xor on this type. That's a pretty good idea.

@real-or-random 'because a bool has no intrinsic meaning and does not "naturally" map to the two parity classes of elliptic curve points.

[sanket1729]

I think a u8 would be more conceptually clear.

In which case, we should update the constructors From to be fallible ones.

[apoelstra]

So I think, for now we can simply

• Deprecate the i32 conversion methods
• Add u8 methods (with a fallible From)
• Add an xor method (and operator overload)

And we can do all this in a minor release. Later we can replace the underlying type with an enum so users can directly see "even" or "odd".

[dr-orlovsky]

So shell we have a point release addressing this together with #364?

[apoelstra]

Yep