secp-sys: Use NonNull in API instead of *mut T

[tcharding]

Currently we expect non-null pointers when we take *mut T parameters, however we do not check that the pointers are non-null because we never set VERIFY in our C build. We can use the NonNull type to enforce no-null-ness as long as we use NonNull::new. In a couple of instances we manually check that a buffer is not empty and therefore that the pointer to it is non-null so we can safely use NonNull::new_unchecked.

Replace mutable pointer parameters *mut T (e.g. *mut c_void) and return types with NonNull<T>.

Fix #546

Note

The description above fully explains the issue to the best of my knowledge, if the description is not spot on then I'm not fully understanding the issue. Please correct me if this is the case.

One unfortunate thing is that this means that we wouldn't be able to implement CPtr for secp256k1::Context, which is our designated "expose types from secp256k1-sys which is not stable" semver escape hatch.

You've lost me here? secp256k1::Context is a trait did you mean secp256k1::Secp256k1 or secp256k1_sys::Context?

[apoelstra]

Thanks! We could actually go a bit further and NonNull-ize the context pointers in all the FFI functions, even signing and verification etc. Then we wouldn't even need to use ptr.as_ptr() everywhere. (Though at the cost of losing the distinction between *mut and *const ... which sucks.)

You've lost me here? secp256k1::Context is a trait did you mean secp256k1::Secp256k1 or secp256k1_sys::Context?

I meant, on all the specific Secp256k1<C> context objects.

[sanket1729]

Are we sure we want this? My understanding is that FFI sys deliberately should not do anything clever and just translate the C interface. The wrappers can do the clever stuff.

For linker reasons, there should only ever by one sys crate in project build dependency. Anyone building their own high-level wrappers around the sys crate is not bound by our choice. The general design rationale here is to not do anything and deal with raw pointers.

As an example open-ssl-sys: https://github.com/sfackler/rust-openssl/blob/master/openssl-sys/src/handwritten/aes.rs

Even the bindgen autogenerated bindgens follow this principle.

[apoelstra]

Are we sure we want this? My understanding is that FFI sys deliberately should not do anything clever and just translate the C interface. The wrappers can do the clever stuff.

The C interface is "takes a non-null pointer at penalty of UB". Arguably using NonNull rather than *mut T is more correct.

Even the bindgen autogenerated bindgens follow this principle.

That's because bindgen does not (and cannot) understand the contracts required by C code.

[Kixunil]

*const does suck indeed and I'm not sure if we should translate const pointers like that. AFAIK the semantics of NonNull is similar to that of a Box.

[apoelstra]

Ok, I'm happy to take this PR as-is then. Is that what you're suggesting @Kixunil ?

[Kixunil]

Went to double-check and after reading https://internals.rust-lang.org/t/seperating-nonnull-into-nonnullmut-and-nonnullconst/11083 and https://internals.rust-lang.org/t/what-is-the-real-difference-between-const-t-and-mut-t-raw-pointers/6127

I came to the conclusion that neither NonNull<T> nor *const T are great. I wouldn't be against our own #[repr(transparent)] pub struct NonNullConst(NonNull<T>); type but maybe it's too much.

[apoelstra]

@Kixunil nice, thanks for the links -- but this PR only replaces *mut Ts with NonNulls and doesn't touch the *const cases, which I think is ok?

Edit sorry, I'm on the run, I will read the links in a bit and apologize if they answer my question

[Kixunil]

Yes, I think it's OK.

[apoelstra]

ACK 9b07e8e