Add as_mut_ptr to PublicKey

[jonasnick]

I'm working on rust-secp256k1-zkp and there's a function that MuSig-combines a bunch of pubkeys into a single pubkey. In principle returning the pubkey could work very similar to how from_secret_key works (https://github.com/rust-bitcoin/rust-secp256k1/compare/master...jonasnick:pk_as_mut_ptr?expand=1#diff-6fb5c8f31a9b0420853a8e3f5af3e391L227). But that doesn't work because we don't have access to the PublicKey constructor from outside the module because the inner field is private. Instead the same effect is achieved with as_mut_ptr (jonasnick/rust-secp256k1-zkp@9428b6c#diff-21a76c1f1f03ad91cc16d9ab499374ddR44).

[real-or-random]

I think we can do that.

Actually we should make both PublicKey and SecretKey repr(C). Then you could just create a [u8; 64] and transmute it into a PublicKey. (Not saying this is better. Maybe. I don't know.)

[apoelstra]

I don't think we should use repr(C) or encourage the transmutation of the non-FFI types.

[real-or-random]

Okay I'm late here but couldn't you use PublicKey::from_slice() too?

Some off-topic stuff:
With @jonasnick's code in rust-secp256k1-zkp in mind, I wanted to argue why repr(C) is safer than not having it. After wasting my morning on reading up about the still unspecified semantics of Rust, I don't think that's true but here are some random notes that may be of interest to us and that I want to write down:

• Storing mem::uninitialized() in a variable is probably* already UB, see https://doc.rust-lang.org/nightly/core/mem/union.MaybeUninit.html. The problem is that you lie to the compiler by claiming that the value has been initialized while it hasn't. (It's actually unclear if there will be an exception for the case of integers and thus integer arrays like in our case here but it's better to assume for the moment that all of this invokes UB.)
• But everyone abuses mem::uninitialized() now and that's okay because the compiler does not exploit it.
• MaybeUninit<T> (unstable) will replace mem::uninitialized() in the future. It gives you a way to a) tell the compiler explicitly "give me fresh memory for T and please be aware it is not initialized", b) then initialize the contents safely, c) and then tell the compiler "okay, now please give me the inner T because I now promise you it's a valid T".
• I guess that's not news but for all of our (lower-level) FFI wrapper types, we actually want repr(transparent) and not repr(C) because for calling into FFI code not only the data layout is relevant but also the ABI conventions. See https://doc.rust-lang.org/1.26.2/unstable-book/language-features/repr-transparent.html for details. We just can't use it at the moment because repr(transparent) was only introduced in Rust 1.25.0 and stabilized in 1.28.0.

*I say probably just because the semantics are not formalizing but people are converging into that direction.

Independent of all of this, I still think we should add still repr(C) to PublicKey, if just to make it consistent with SecretKey. SecretKey is also repr(C) and I don't see a reason to treat differently. There are invalid values patterns for SecretKey too.

[apoelstra]

Cool, thanks for the research!

My understanding, last I checked, is that it's not UB to have uninitialized primitive integer types (though just like in C, if you try to read from them that would be UB). Essentially the compiler is allowed to assume that uninitialized memory has any legal value.

Where MaybeUninit comes in is for types where "any legal value" is a smaller set than the actual in-memory representations. For example an Option<Box<T>> set to Some(mem::uninitialized()) may actually be set to all-bits-zero, which is the representation of None, so if you call .is_none() on this value then you'll get the "wrong" answer even though you "didn't read" the uninitialized memory.

It gets even worse with uninhabited types like enum Enum { } or ! where there are no legal values, meaning the compiler can assume that any code that can access one of these objects is unreachable. Then you get absolutely crazy results even if you don't touch anything related to the uninitialized object.

But u8 should be perfectly fine.

[apoelstra]

Also I don't understand your comment about PublicKey::from_slice. This function actually calls secp256k1_pubkey_parse internally which is a bit expensive as in decompresses the serialized point.

[real-or-random]

Well for integer types like u8, the docs explicitly say this:

Moreover, uninitialized memory is special in that the compiler knows that it does not have a fixed value. This makes it undefined behavior to have uninitialized data in a variable even if that variable has an integer type, which otherwise can hold any bit pattern: [...]
(Notice that the rules around uninitialized integers are not finalized yet, but until they are, it is advisable to avoid them.)

(https://doc.rust-lang.org/nightly/core/mem/union.MaybeUninit.html)

As I understand, this moved only recently towards that direction. That's also why they're currently deprecating mem::uninitiated(), it just does not make a lot of sense (rust-lang/rust#60445, which was opened today after I poked @RalfJung with my questions...)

re from_slice(): scratch my comment.

[RalfJung]

Things like uninitialized bits in a u8 are also discussed by the Unsafe Code Guidelines WG at rust-lang/unsafe-code-guidelines#71. No conclusion has been reached yet though, which is why the docs say you shouldn't do this. (There may be other places where the docs still say that is okay; please let me know if you find any so that we can fix them.)

As I understand, this moved only recently towards that direction

Well, rust-lang/rfcs#1892 was opened early 2017. So it's not exactly new either. But it has been moving very slowly.