Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Deprecate from_slice methods in favor of arrays #737

Merged
merged 2 commits into from
Sep 12, 2024
Merged

Deprecate from_slice methods in favor of arrays #737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1661f57
Add `from_byte_array` functions
jamillambert Sep 10, 2024
537b85b
Deprecate and replace `from_slice`
jamillambert Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/ecdh.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ impl SharedSecret {
pub fn from_bytes(bytes: [u8; SHARED_SECRET_SIZE]) -> SharedSecret { SharedSecret(bytes) }

/// Creates a shared secret from `bytes` slice.
#[deprecated(since = "TBD", note = "Use `from_bytes` instead.")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have expected with this addition to see a change to call from_bytes. If you like the combinator style you could use

    pub fn from_slice(bytes: &[u8]) -> Result<SharedSecret, Error> {
        <[u8; SHARED_SECRET_SIZE]>::try_from(bytes)
            .map_err(|_| Error::InvalidSharedSecret)
            .and_then(|a| Ok(Self::from_bytes(a)))
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I need to change anything again I'll add this too, thanks

#[inline]
pub fn from_slice(bytes: &[u8]) -> Result<SharedSecret, Error> {
match bytes.len() {
Expand Down
113 changes: 86 additions & 27 deletions src/key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ impl str::FromStr for SecretKey {
fn from_str(s: &str) -> Result<SecretKey, Error> {
let mut res = [0u8; constants::SECRET_KEY_SIZE];
match from_hex(s, &mut res) {
Ok(constants::SECRET_KEY_SIZE) => SecretKey::from_slice(&res),
Ok(constants::SECRET_KEY_SIZE) => SecretKey::from_byte_array(&res),
_ => Err(Error::InvalidSecretKey),
}
}
Expand All @@ -138,7 +138,7 @@ impl str::FromStr for SecretKey {
/// use secp256k1::{SecretKey, Secp256k1, PublicKey};
///
/// let secp = Secp256k1::new();
/// let secret_key = SecretKey::from_slice(&[0xcd; 32]).expect("32 bytes, within curve order");
/// let secret_key = SecretKey::from_byte_array(&[0xcd; 32]).expect("32 bytes, within curve order");
/// let public_key = PublicKey::from_secret_key(&secp, &secret_key);
/// # }
/// ```
Expand Down Expand Up @@ -168,9 +168,13 @@ impl str::FromStr for PublicKey {
fn from_str(s: &str) -> Result<PublicKey, Error> {
let mut res = [0u8; constants::UNCOMPRESSED_PUBLIC_KEY_SIZE];
match from_hex(s, &mut res) {
Ok(constants::PUBLIC_KEY_SIZE) =>
PublicKey::from_slice(&res[0..constants::PUBLIC_KEY_SIZE]),
Ok(constants::UNCOMPRESSED_PUBLIC_KEY_SIZE) => PublicKey::from_slice(&res),
Ok(constants::PUBLIC_KEY_SIZE) => {
let bytes: [u8; constants::PUBLIC_KEY_SIZE] =
res[0..constants::PUBLIC_KEY_SIZE].try_into().unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking, it'd be better to create a reference here (simply annotating the type with & should work because the conversion is defined) to avoid useless copying but there's a good chance compiler will optimize it out anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying you can convert a &[u8] to a &[u8; N] via try_into and the compiler will just cast the pointers rather than copying?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that's a huge win and I feel like a clippy lint should exist for it. (Though it'd be a bit tough in general to tell when the array is only used by reference.)

PublicKey::from_byte_array_compressed(&bytes)
}
Ok(constants::UNCOMPRESSED_PUBLIC_KEY_SIZE) =>
PublicKey::from_byte_array_uncompressed(&res),
_ => Err(Error::InvalidPublicKey),
}
}
Expand Down Expand Up @@ -203,33 +207,43 @@ impl SecretKey {
SecretKey(data)
}

/// Converts a `SECRET_KEY_SIZE`-byte slice to a secret key.
/// Converts a 32-byte slice to a secret key.
///
/// # Examples
///
/// ```
/// use secp256k1::SecretKey;
/// let sk = SecretKey::from_slice(&[0xcd; 32]).expect("32 bytes, within curve order");
/// ```
#[deprecated(since = "TBD", note = "Use `from_byte_array` instead.")]
#[inline]
pub fn from_slice(data: &[u8]) -> Result<SecretKey, Error> {
match <[u8; constants::SECRET_KEY_SIZE]>::try_from(data) {
Ok(data) => {
unsafe {
if ffi::secp256k1_ec_seckey_verify(
ffi::secp256k1_context_no_precomp,
data.as_c_ptr(),
) == 0
{
return Err(InvalidSecretKey);
}
}
Ok(SecretKey(data))
}
Ok(data) => Self::from_byte_array(&data),
Err(_) => Err(InvalidSecretKey),
}
}

/// Converts a 32-byte array to a secret key.
///
/// # Examples
///
/// ```
/// use secp256k1::SecretKey;
/// let sk = SecretKey::from_byte_array(&[0xcd; 32]).expect("32 bytes, within curve order");
/// ```
#[inline]
pub fn from_byte_array(data: &[u8; constants::SECRET_KEY_SIZE]) -> Result<SecretKey, Error> {
unsafe {
if ffi::secp256k1_ec_seckey_verify(ffi::secp256k1_context_no_precomp, data.as_c_ptr())
== 0
{
return Err(InvalidSecretKey);
}
}
Ok(SecretKey(*data))
}

/// Creates a new secret key using data from BIP-340 [`Keypair`].
///
/// # Examples
Expand Down Expand Up @@ -353,7 +367,7 @@ impl SecretKey {
impl<T: ThirtyTwoByteHash> From<T> for SecretKey {
/// Converts a 32-byte hash directly to a secret key without error paths.
fn from(t: T) -> SecretKey {
SecretKey::from_slice(&t.into_32()).expect("failed to create secret key")
SecretKey::from_byte_array(&t.into_32()).expect("failed to create secret key")
}
}

Expand Down Expand Up @@ -442,17 +456,50 @@ impl PublicKey {
/// Creates a public key directly from a slice.
#[inline]
pub fn from_slice(data: &[u8]) -> Result<PublicKey, Error> {
if data.is_empty() {
return Err(Error::InvalidPublicKey);
match data.len() {
constants::PUBLIC_KEY_SIZE => PublicKey::from_byte_array_compressed(
&<[u8; constants::PUBLIC_KEY_SIZE]>::try_from(data).unwrap(),
),
constants::UNCOMPRESSED_PUBLIC_KEY_SIZE => PublicKey::from_byte_array_uncompressed(
&<[u8; constants::UNCOMPRESSED_PUBLIC_KEY_SIZE]>::try_from(data).unwrap(),
),
_ => Err(InvalidPublicKey),
}
}

/// Creates a public key from a serialized array in compressed format.
#[inline]
pub fn from_byte_array_compressed(
data: &[u8; constants::PUBLIC_KEY_SIZE],
) -> Result<PublicKey, Error> {
unsafe {
let mut pk = ffi::PublicKey::new();
if ffi::secp256k1_ec_pubkey_parse(
ffi::secp256k1_context_no_precomp,
&mut pk,
data.as_c_ptr(),
data.len(),
constants::PUBLIC_KEY_SIZE,
) == 1
{
Ok(PublicKey(pk))
} else {
Err(InvalidPublicKey)
}
}
}

/// Creates a public key from a serialized array in uncompressed format.
#[inline]
pub fn from_byte_array_uncompressed(
data: &[u8; constants::UNCOMPRESSED_PUBLIC_KEY_SIZE],
) -> Result<PublicKey, Error> {
unsafe {
let mut pk = ffi::PublicKey::new();
if ffi::secp256k1_ec_pubkey_parse(
ffi::secp256k1_context_no_precomp,
&mut pk,
data.as_c_ptr(),
constants::UNCOMPRESSED_PUBLIC_KEY_SIZE,
) == 1
{
Ok(PublicKey(pk))
Expand Down Expand Up @@ -500,7 +547,7 @@ impl PublicKey {
};
buf[1..].clone_from_slice(&pk.serialize());

PublicKey::from_slice(&buf).expect("we know the buffer is valid")
PublicKey::from_byte_array_compressed(&buf).expect("we know the buffer is valid")
}

#[inline]
Expand Down Expand Up @@ -1114,8 +1161,7 @@ impl str::FromStr for XOnlyPublicKey {
fn from_str(s: &str) -> Result<XOnlyPublicKey, Error> {
let mut res = [0u8; constants::SCHNORR_PUBLIC_KEY_SIZE];
match from_hex(s, &mut res) {
Ok(constants::SCHNORR_PUBLIC_KEY_SIZE) =>
XOnlyPublicKey::from_slice(&res[0..constants::SCHNORR_PUBLIC_KEY_SIZE]),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL, the previous code...

Ok(constants::SCHNORR_PUBLIC_KEY_SIZE) => XOnlyPublicKey::from_byte_array(&res),
_ => Err(Error::InvalidPublicKey),
}
}
Expand Down Expand Up @@ -1161,12 +1207,25 @@ impl XOnlyPublicKey {
///
/// Returns [`Error::InvalidPublicKey`] if the length of the data slice is not 32 bytes or the
/// slice does not represent a valid Secp256k1 point x coordinate.
#[deprecated(since = "TBD", note = "Use `from_byte_array` instead.")]
#[inline]
pub fn from_slice(data: &[u8]) -> Result<XOnlyPublicKey, Error> {
if data.is_empty() || data.len() != constants::SCHNORR_PUBLIC_KEY_SIZE {
return Err(Error::InvalidPublicKey);
match <[u8; constants::SCHNORR_PUBLIC_KEY_SIZE]>::try_from(data) {
Ok(data) => Self::from_byte_array(&data),
Err(_) => Err(InvalidPublicKey),
}
}
Comment on lines 1212 to +1217
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function could be written with combinators like this:

    pub fn from_slice(data: &[u8]) -> Result<XOnlyPublicKey, Error> {
        <[u8; constants::SCHNORR_PUBLIC_KEY_SIZE]>::try_from(data)
            .map_err(|_| InvalidPublicKey)
            .and_then(|data| Self::from_byte_array(&data))
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does look quite neat with a combinator


/// Creates a schnorr public key directly from a byte array.
///
/// # Errors
///
/// Returns [`Error::InvalidPublicKey`] if the array does not represent a valid Secp256k1 point
/// x coordinate.
#[inline]
pub fn from_byte_array(
data: &[u8; constants::SCHNORR_PUBLIC_KEY_SIZE],
) -> Result<XOnlyPublicKey, Error> {
unsafe {
let mut pk = ffi::XOnlyPublicKey::new();
if ffi::secp256k1_xonly_pubkey_parse(
Expand Down
1 change: 1 addition & 0 deletions src/schnorr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ impl Signature {
pub fn from_byte_array(sig: [u8; constants::SCHNORR_SIGNATURE_SIZE]) -> Self { Self(sig) }

/// Creates a `Signature` directly from a slice.
#[deprecated(since = "TBD", note = "Use `from_byte_array` instead.")]
#[inline]
pub fn from_slice(data: &[u8]) -> Result<Signature, Error> {
match data.len() {
Expand Down
Loading