Deprecate from_slice methods in favor of arrays #737

jamillambert · 2024-09-10T19:55:57Z

As brought up in issue rust-bitcoin/rust-bitcoin#3102 support for Rust arrays is now much better so slice-accepting methods that require a fixed length can be replaced with a method that accepts an array.

from_slice() methods have been deprecated and calls to it from within the crate have been changed to use the equivalent array method.

tcharding

Only did shallow review, but everything apart from try_from thing looks good to me, my reviews don't carry much weight in this repo though.

src/key.rs

tcharding · 2024-09-10T22:36:34Z

There are currently not formatting issues on master so there should be no need for a separate formatting patch in this PR bro.

jamillambert · 2024-09-11T06:53:59Z

Third patch removed and incorporated in first two

Kixunil

Hard no deprecating PublicKey::from_slice (but only that one). Other suggestions are not that critical but I'd obviously prefer to fix them.

src/key.rs

jamillambert · 2024-09-11T10:37:18Z

Removed deprecation of PublicKey::from_slice and addressed other suggestions. Thanks.

apoelstra · 2024-09-11T16:17:30Z

code review ack 46e06da except that the expect message is still wrong. Honestly I think you should just unwrap here. It is very obvious that let x: [u8; N] = slice[..N].try_into() will succeed and why.

Functions have been added to PrivateKey, PublicKey and XOnlyPublicKey to allow the creation of a key directly from a byte array.

The PrivateKey and XOnlyPublicKey from_slice functions have been deprecated and calls to them from within the crate have been changed to use the equivalent array function.

jamillambert · 2024-09-11T17:58:33Z

Thanks for the review. I have replaced expect with unwrap.

apoelstra

ACK 537b85b successfully ran local tests

tcharding

ACK 537b85b

My suggestions are just because we all enjoy coding round here and it amused me to find a way to re-write your changes using combinators.

tcharding · 2024-09-12T01:28:52Z

src/key.rs

    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),
        }
+    }


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)) }

It does look quite neat with a combinator

tcharding · 2024-09-12T01:31:16Z

src/ecdh.rs

    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.")]


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))) }

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

apoelstra · 2024-09-12T15:00:17Z

I also prefer the combinator version, though of course I won't hold up the PR over it.

But @Kixunil we need you to take a look at this one again since your status is "change requested".

Kixunil · 2024-09-12T16:28:41Z

I find match cleaner in cases like this where we're transforming stuff in both branches but if we want combinators then I'd use ? instead of and_then.

Kixunil

ACK 537b85b

Kixunil · 2024-09-12T16:40:54Z

src/key.rs

-            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();


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.

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?

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.)

Kixunil · 2024-09-12T16:42:51Z

src/key.rs

        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]),


LOL, the previous code...

…n favor of arrays 537b85b793862096c6a335e0fd14315f64ef1995 Deprecate and replace `from_slice` (Jamil Lambert, PhD) 1661f57d84140b0800552c10d10454d2ad99632e Add `from_byte_array` functions (Jamil Lambert, PhD) Pull request description: As brought up in issue rust-bitcoin/rust-bitcoin#3102 support for Rust arrays is now much better so slice-accepting methods that require a fixed length can be replaced with a method that accepts an array. `from_slice()` methods have been deprecated and calls to it from within the crate have been changed to use the equivalent array method. ACKs for top commit: apoelstra: ACK 537b85b793862096c6a335e0fd14315f64ef1995 successfully ran local tests tcharding: ACK 537b85b793862096c6a335e0fd14315f64ef1995 Kixunil: ACK 537b85b793862096c6a335e0fd14315f64ef1995 Tree-SHA512: 8f8f807af0032130b64a93ff86cae0df1ccee83de052603968be9e30751c27dfc763a6128472e6d0f3a5c2016e7da8e4d06eefc86d8310bdaacc6be0f8fe3822

tcharding reviewed Sep 10, 2024

View reviewed changes

src/key.rs Outdated Show resolved Hide resolved

jamillambert force-pushed the 0906-from_slice branch from fd5649b to d612dc0 Compare September 10, 2024 21:23

jamillambert force-pushed the 0906-from_slice branch from d612dc0 to 0cb6b30 Compare September 11, 2024 06:53

Kixunil requested changes Sep 11, 2024

View reviewed changes

jamillambert force-pushed the 0906-from_slice branch from 0cb6b30 to 1a6339a Compare September 11, 2024 10:31

jamillambert force-pushed the 0906-from_slice branch from 1a6339a to 46e06da Compare September 11, 2024 10:53

jamillambert added 2 commits September 11, 2024 18:56

Add from_byte_array functions

1661f57

Functions have been added to PrivateKey, PublicKey and XOnlyPublicKey to allow the creation of a key directly from a byte array.

Deprecate and replace from_slice

537b85b

The PrivateKey and XOnlyPublicKey from_slice functions have been deprecated and calls to them from within the crate have been changed to use the equivalent array function.

jamillambert force-pushed the 0906-from_slice branch from 46e06da to 537b85b Compare September 11, 2024 17:57

apoelstra approved these changes Sep 11, 2024

View reviewed changes

tcharding approved these changes Sep 12, 2024

View reviewed changes

Kixunil approved these changes Sep 12, 2024

View reviewed changes

apoelstra mentioned this pull request Sep 12, 2024

fix docs for new clippy lint #740

Merged

apoelstra merged commit 3453adb into rust-bitcoin:master Sep 12, 2024

Deprecate from_slice methods in favor of arrays #737

Deprecate from_slice methods in favor of arrays #737

Uh oh!

Conversation

jamillambert commented Sep 10, 2024

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tcharding commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamillambert commented Sep 11, 2024

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jamillambert commented Sep 11, 2024

Uh oh!

apoelstra commented Sep 11, 2024

Uh oh!

jamillambert commented Sep 11, 2024

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Sep 12, 2024

Uh oh!

Kixunil commented Sep 12, 2024

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tcharding commented Sep 10, 2024 •

edited

Loading