Passing array reference instead of owned array

[uncomputable]

The from_byte_array methods take as parameter an array reference &[u8; LEN] instead of an owned array [u8; LEN].

Looking at the implementation, the input bytes are converted into a C pointer as_c_ptr() and passed to a C function. From that perspective, an array reference seems fitting because it is easy to convert to a C pointer. However, passing an array reference seems like uncanonical Rust code to me. Copying 32 bytes is not a problem on any system.

Affected functions:
SecretKey::from_byte_array, XOnlyPublicKey::from_byte_array, PublicKey::from_byte_array_compressed, PublicKey::from_byte_array_uncompressed.

[apoelstra]

Agreed, this feels un-Rust-like and we should pass 32-byte arrays by value. Same with 64/65-byte arrays. I don't think there are larger ones used by this library.

[Kixunil]

FTR if people are worried about performance we can slap #[inline] on those functions and it should take care of it.

[uncomputable]

I could update the existing methods to take [u8; LEN] and that would be a breaking change.

[Kixunil]

We don't mind doing breaking changes.

[apoelstra]

Yeah, the API in rust-secp is a real mess right now. We are also at a good time right now to make sweeping API changes because in rust-bitcoin we're focused on non-crypto stuff and won't be pressured to cut a new incomplete release of rust-secp anytime soon.

[Kixunil]

and won't be pressured to cut a new incomplete release of rust-secp anytime soon

Frankly, I'm really looking forward to the MuSig stuff, I just also don't have that much time for it yet.

[uncomputable]

The coexistence of schnorr::Signature::as_byte_array and schnorr::Signature::to_byte_array is also questionable. The former returns &[u8; 64] while the latter returns [u8; 64].

[uncomputable]

> git grep '&\[u8;'
src/ecdsa/mod.rs:        noncedata: Option<&[u8; 32]>,
src/ecdsa/mod.rs:        noncedata: &[u8; 32],
src/ecdsa/recovery.rs:        noncedata: &[u8; 32],
src/ellswift.rs:    pub const fn as_secret_bytes(&self) -> &[u8; 32] { &self.0 }
src/key.rs:    fn as_ref(&self) -> &[u8; constants::SECRET_KEY_SIZE] {
src/lib.rs:    pub fn seeded_randomize(&mut self, seed: &[u8; 32]) {
src/scalar.rs:    pub(crate) fn as_be_bytes(&self) -> &[u8; 32] { &self.0 }
src/schnorr.rs:    pub fn as_byte_array(&self) -> &[u8; constants::SCHNORR_SIGNATURE_SIZE] { &self.0 }
src/schnorr.rs:        aux_rand: &[u8; 32],

[apoelstra]

Yeah, I think we should drop (deprecate) as_byte_array.

[Kixunil]

Yeah, I think we should drop (deprecate) as_byte_array.

Definitely not. It's useful when you need to cast a reference to something to a slice reference without dealing with the temporary value causing borrowing issues.

[apoelstra]

Ahh that's a great point.

[uncomputable]

That sounds like AsRef<[u8]>.

[Kixunil]

@uncomputable yes, but having a method too is idiomatic and helpful in some cases. The cost of having the method is near zero.