Allow SharedSecret to be created from byte array #417

dspicher · 2022-03-10T15:00:16Z

This was accidentally removed in 8b2edad. See also the discussion
on #402

Closes #416.

apoelstra

ACK 16e37ec

Can you also add a commit which bumps the minor version number in Cargo.toml and adds an entry to CHANGELOG.md? This will let me quickly publish a new version.

apoelstra · 2022-03-10T15:14:58Z

Also can you make sure this compiles with rustc 1.29? Sorry, we'll eliminate this requirement in the next major version I think.

elichai

tACK 78ed572

Kixunil · 2022-03-10T18:47:55Z

src/ecdh.rs

+    fn from(arr: [u8; SHARED_SECRET_SIZE]) -> Self {
+        SharedSecret(arr)
+    }
+}


I'm personally not keen on adding impl From<Inner> for Newtype in case there is struct Newtype(Inner); It encourages just calling into() which hides the newtype.

I suggest from_bytes(bytes: [u8; SHARED_SECRET_SIZE]) -> Self method instead.

I don't have a strong preference here. I'm not sure what you mean by "hiding" .. a call to .into is pretty explicit.

It's not clear which type is converted into which until you review a bigger chunk of code.

Done. I'm not very familiar with #[inline] usage, let me know if this is needed here as well.

Another data point: we have secret_bytes() -> [u8; SHARED_SECRET_SIZE] already which is slightly strangely named, the reason is that its the same method name used for various keys, especially KeyPair (for which it makes most sense). In light of this I'd find from_bytes the least surprising name so as to kind of match with secret_bytes.

#[inline] could be useful/nice but not super-important. Especially if one uses LTO.

@Kixunil @apoelstra @dspicher

I think it is a poor decision to avoid implementing traits from the standard library which are even part of the standard prelude. The From trait defines a standard interface that allows for better composability, e.g. you can define functions like this

fn process<T: Into<Foo>>(t: T)

and it will compile for all types that can be converted into Foo. Also, you can always opt-in for doing the conversion explicitly:

let bar: Bar = foo.into()

I really don't understand why we need to segue here out of the standard rust interface for doing conversions.

apoelstra

ACK 78ed572

But will hold off merging until @Kixunil is satisified.

This was accidentally removed in 8b2edad. See also the discussion on rust-bitcoin#402

apoelstra

ACK 463148f

apoelstra · 2022-03-11T17:44:28Z

Looks like the tests have actually passed -- not sure why CI reporting is being weird.

apoelstra · 2022-03-11T17:45:43Z

Holding off on publishing new version until #418.

Kixunil

Post-merge ACK :)

@dspicher

de65fb2 Implement de/serialization for SharedSecret (Tobin Harding) Pull request description: As we do for other keys implement serde de/serialization for the `SharedSecret`. Please note, this adds `from_slice` and `from_bytes` (borrowed and owner respectively) because I needed to use them. Doing so treads on @dspicher's toes because he is in the process of implementing an owned conversion method for `SharedSecret`. The fair thing to do would be let #417 resolve and merge before merging this one (I can rebase). ~Side note, its kind of rubbish that `BytesVisitor` deserializes into a buffer (presumably) then we reallocate and copy the buffer to use the borrowed conversion method due to the serde function signature `fn visit_bytes<E: de::Error>(self, v: &[u8]) -> Result<Self::Value, E>`~ (I was bumbling nonsense.) Closes: #416 ACKs for top commit: apoelstra: ACK de65fb2 Tree-SHA512: 3d484f160d8a459a867f645736713984bad982429236ac5351c20b6c21b42cec86e68009fe7adf062912037cf7e747e5b15357a5fd7900e52169f208a4e56710

…ed from byte array 463148f9a0a316c5f2cf5cb89078ed59d87dfdf6 bump version to 0.22.1 (Dominik Spicher) 9be8e7410751f47e8b5d2ab90945a7618482bd0e Allow SharedSecret to be created from byte array (Dominik Spicher) Pull request description: This was accidentally removed in 8b2edad. See also the discussion on rust-bitcoin/rust-secp256k1#402 Closes #416. ACKs for top commit: apoelstra: ACK 463148f9a0a316c5f2cf5cb89078ed59d87dfdf6 Tree-SHA512: 04e16226efa2cf6fd461eabb0c78e5b00f347c78e20c1c7561591ffa74a7259fb3265b49a9d7326caf70e4d5ce32a620485f1bd5538c292654f91eb68c2a57dc

@dspicher

… SharedSecret de65fb2f1e4d55ecb04afa621044a96840d449bf Implement de/serialization for SharedSecret (Tobin Harding) Pull request description: As we do for other keys implement serde de/serialization for the `SharedSecret`. Please note, this adds `from_slice` and `from_bytes` (borrowed and owner respectively) because I needed to use them. Doing so treads on @dspicher's toes because he is in the process of implementing an owned conversion method for `SharedSecret`. The fair thing to do would be let rust-bitcoin/rust-secp256k1#417 resolve and merge before merging this one (I can rebase). ~Side note, its kind of rubbish that `BytesVisitor` deserializes into a buffer (presumably) then we reallocate and copy the buffer to use the borrowed conversion method due to the serde function signature `fn visit_bytes<E: de::Error>(self, v: &[u8]) -> Result<Self::Value, E>`~ (I was bumbling nonsense.) Closes: #416 ACKs for top commit: apoelstra: ACK de65fb2f1e4d55ecb04afa621044a96840d449bf Tree-SHA512: 3d484f160d8a459a867f645736713984bad982429236ac5351c20b6c21b42cec86e68009fe7adf062912037cf7e747e5b15357a5fd7900e52169f208a4e56710

…ed from byte array 463148f9a0a316c5f2cf5cb89078ed59d87dfdf6 bump version to 0.22.1 (Dominik Spicher) 9be8e7410751f47e8b5d2ab90945a7618482bd0e Allow SharedSecret to be created from byte array (Dominik Spicher) Pull request description: This was accidentally removed in 8b2edad. See also the discussion on rust-bitcoin/rust-secp256k1#402 Closes #416. ACKs for top commit: apoelstra: ACK 463148f9a0a316c5f2cf5cb89078ed59d87dfdf6 Tree-SHA512: 04e16226efa2cf6fd461eabb0c78e5b00f347c78e20c1c7561591ffa74a7259fb3265b49a9d7326caf70e4d5ce32a620485f1bd5538c292654f91eb68c2a57dc

@dspicher

… SharedSecret de65fb2f1e4d55ecb04afa621044a96840d449bf Implement de/serialization for SharedSecret (Tobin Harding) Pull request description: As we do for other keys implement serde de/serialization for the `SharedSecret`. Please note, this adds `from_slice` and `from_bytes` (borrowed and owner respectively) because I needed to use them. Doing so treads on @dspicher's toes because he is in the process of implementing an owned conversion method for `SharedSecret`. The fair thing to do would be let rust-bitcoin/rust-secp256k1#417 resolve and merge before merging this one (I can rebase). ~Side note, its kind of rubbish that `BytesVisitor` deserializes into a buffer (presumably) then we reallocate and copy the buffer to use the borrowed conversion method due to the serde function signature `fn visit_bytes<E: de::Error>(self, v: &[u8]) -> Result<Self::Value, E>`~ (I was bumbling nonsense.) Closes: #416 ACKs for top commit: apoelstra: ACK de65fb2f1e4d55ecb04afa621044a96840d449bf Tree-SHA512: 3d484f160d8a459a867f645736713984bad982429236ac5351c20b6c21b42cec86e68009fe7adf062912037cf7e747e5b15357a5fd7900e52169f208a4e56710

dspicher force-pushed the master branch from 6b2c4d4 to 16e37ec Compare March 10, 2022 15:05

apoelstra previously approved these changes Mar 10, 2022

View reviewed changes

dspicher dismissed apoelstra’s stale review via 78ed572 March 10, 2022 15:29

dspicher force-pushed the master branch from 16e37ec to 78ed572 Compare March 10, 2022 15:29

elichai previously approved these changes Mar 10, 2022

View reviewed changes

Kixunil reviewed Mar 10, 2022

View reviewed changes

apoelstra previously approved these changes Mar 10, 2022

View reviewed changes

dspicher dismissed stale reviews from apoelstra and elichai via e3da64a March 10, 2022 20:55

dspicher force-pushed the master branch from 78ed572 to e3da64a Compare March 10, 2022 20:55

dspicher added 2 commits March 10, 2022 22:38

Allow SharedSecret to be created from byte array

9be8e74

This was accidentally removed in 8b2edad. See also the discussion on rust-bitcoin#402

bump version to 0.22.1

463148f

dspicher force-pushed the master branch from e3da64a to 463148f Compare March 10, 2022 21:38

This was referenced Mar 10, 2022

Update secp256k1 dependency rust-bitcoin/rust-bitcoin#875

Merged

Implement de/serialization for SharedSecret #418

Merged

apoelstra approved these changes Mar 11, 2022

View reviewed changes

apoelstra merged commit 330c91b into rust-bitcoin:master Mar 11, 2022

Kixunil reviewed Mar 11, 2022

View reviewed changes

Allow SharedSecret to be created from byte array #417

Allow SharedSecret to be created from byte array #417

Uh oh!

Conversation

dspicher commented Mar 10, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Mar 10, 2022

Uh oh!

elichai left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil Mar 10, 2022

Choose a reason for hiding this comment

Uh oh!

apoelstra Mar 10, 2022

Choose a reason for hiding this comment

Uh oh!

Kixunil Mar 10, 2022

Choose a reason for hiding this comment

Uh oh!

dspicher Mar 10, 2022

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kixunil Mar 11, 2022

Choose a reason for hiding this comment

Uh oh!

toxeus Mar 13, 2022

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Mar 11, 2022

Uh oh!

apoelstra commented Mar 11, 2022

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tcharding Mar 10, 2022 •

edited

Loading