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.

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

Deprecate ThirtyTwoByteHash #686

Merged

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Mar 26, 2024

The implementations of ThirtyTwoByteHash for types from the hashes crate are problematic during upgrades because both bitcoin and secp256k1 depend on hashes and when the versions of hashes get out of sync usage of the trait breaks.

Deprecate the ThirtyTwoByteHash trait and remove the impls for types from bitcoin_hashes.

Add an explanation in the changelog because its too long to go in the deprecation message.

Close: #673

@tcharding tcharding marked this pull request as draft March 26, 2024 19:17
@tcharding tcharding force-pushed the 03-27-rm-ThirtyTwoByteHash-impls branch from 63e737f to 659926c Compare March 26, 2024 19:18
@tcharding tcharding marked this pull request as ready for review March 26, 2024 19:19
@apoelstra
Copy link
Member

concept ACK, but looks like this breaks some tests.

@tcharding
Copy link
Member Author

hmm, the test show that we forgot about SecretKey::from, which takes type that implements ThirtyTwoByteHash. Removing the impl for hashes types makes this API a bit pointless - that feels like a regression.

@apoelstra
Copy link
Member

It is a bit pointless, yes. The idea now is that users can implement ThirtyTwoByteHash for their own types, but really, they could just go ahead and directly implement Into<SecretKey> and Into<Message> instead of going through the trait.

And it is a regression to lose the impl for the sha2 hash types, but we (rust-bitcoin) no longer use those impls, and the impls make the upgrade process harder for users..

But the trait is easier to implement and provides a bit of documentation "I promise that this is actually a hash of some data", so the value isn't zero.

@tcharding tcharding force-pushed the 03-27-rm-ThirtyTwoByteHash-impls branch from 659926c to 9d43945 Compare March 27, 2024 22:46
@tcharding
Copy link
Member Author

Process question please: when a PR changes after review and the description gets re-written it can make the conversation thread go stale. For later reviewers this may be confusing so I"ve been looking for a way to flag that situation. I've tried explicitly saying "discussion thread is now stale", instead here I tried another method - please see PR description and tell me if that is clear - or if this is a non-issue.

@apoelstra
Copy link
Member

I do not think it's clear (from the PR description) that you've changed something, or when you changed it.

@tcharding
Copy link
Member Author

Ok, I'll keep iterating as the situation arises.

@apoelstra
Copy link
Member

ACK 9d43945 but I think we should deprecate from_hashed_data at the same time as we do this. It's a weird function that I've never used (and we've never used in rust-bitcoin) and whose semantics are not super clear versus just having the user compute a hash and sign that.

Also, what if we changed sign_ecdsa and sign_schnorr to take a T: Into<Message> rather than directly taking a Message, so that the user (usually) does not need to do the conversion herself?

@tcharding
Copy link
Member Author

Does it make sense to deprecate while we remove the generic, any user of this code will break anyway and if they are using a hash that is not sha256 its the same as if we took it away - perhaps we should just delete it?

@apoelstra
Copy link
Member

Hmm, yeah, I think you're right. Let's just delete it.

Also let's deprecate the trait. The more I think about this, I think Into<Message> is the right trait for this. And we should make sure the docs on Message suggest implementing this if and only if your type represents a cryptographic hash of 32 bytes in length, preferably one with a known preimage.

Then I think the right approach to type-safing this stuff is to improve the hashes API. Then whatever types we think are "safe to sign", e.g. the sighash types, we can just implement Into<Message> for those.

@tcharding tcharding force-pushed the 03-27-rm-ThirtyTwoByteHash-impls branch from 9d43945 to 5749e13 Compare April 1, 2024 05:56
@tcharding
Copy link
Member Author

I've deprecated ThirtyTwoByteHash and removed both from_hashed_data functions as well as docs mentions and usage.

Leaving the Into<Message> stuff for later so we can get this crate released.

@tcharding tcharding force-pushed the 03-27-rm-ThirtyTwoByteHash-impls branch 2 times, most recently from 4467f2b to 00b66df Compare April 1, 2024 06:13
@@ -372,14 +346,6 @@ impl SecretKey {
}
}

#[cfg(feature = "hashes")]
impl<T: ThirtyTwoByteHash> From<T> for SecretKey {
Copy link
Member

Choose a reason for hiding this comment

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

In 00b66df:

We should wait til after the deprecation cycle to drop these From impls. If we drop them now that has the same effect as just deleting the crate -- sudden compilation failures without a clear path forward.

src/lib.rs Outdated
@@ -198,34 +196,20 @@ pub use crate::scalar::Scalar;
/// Trait describing something that promises to be a 32-byte random number; in particular,
/// it has negligible probability of being zero or overflowing the group order. Such objects
/// may be converted to `Message`s without any error paths.
#[deprecated(since = "0.29.0", note = "this will be removed soon")]
Copy link
Member

Choose a reason for hiding this comment

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

In 00b66df:

Not sure if we can easily put a multi-line message here, but let's suggest that users directly implement From<T> for Message for their type if their goal is to have easy convertibility to messages, and explain that sign_ecdsa and sign_schnorr can now directly take such types rather than requiring a manual conversion to Message.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, ok, we can drop the "no need for manual conversion" since we still need a manual conversion for now. We should suggest people write Message::from though. Later clippy will tell them that they can drop the Message::from

If they just do .into() this will work today but break when we change our signing functions to take generics, because Rust won't know the target type to convert to.

@tcharding tcharding force-pushed the 03-27-rm-ThirtyTwoByteHash-impls branch from 00b66df to 4663198 Compare April 1, 2024 22:11
@tcharding tcharding changed the title Remove impls of ThirtyTwoByteHash for hashes types Deprecate ThirtyTwoByteHash Apr 1, 2024
@tcharding tcharding force-pushed the 03-27-rm-ThirtyTwoByteHash-impls branch from 4663198 to c371561 Compare April 1, 2024 22:14
@tcharding
Copy link
Member Author

Had another go, please re-review the whole thing.

@tcharding tcharding force-pushed the 03-27-rm-ThirtyTwoByteHash-impls branch 2 times, most recently from b372bba to 0f247f4 Compare April 2, 2024 00:08
Recent rustc upgrade introduced some new warnings for incorrect imports,
fix them.
The implementations of `ThirtyTwoByteHash` for types from the `hashes`
crate are problematic during upgrades because both `bitcoin` and
`secp256k1` depend on `hashes` and when the versions of `hashes` get
out of sync usage of the trait breaks.

Deprecate the `ThirtyTwoByteHash` trait and remove the impls for types
from `bitcoin_hashes`.

Add an explanation in the changelog because its too long to go in the
deprecation message.
@tcharding tcharding force-pushed the 03-27-rm-ThirtyTwoByteHash-impls branch from 0f247f4 to 9f28cf6 Compare April 2, 2024 00:11
@tcharding
Copy link
Member Author

tcharding commented Apr 2, 2024

Usage of this PR is shown in rust-bitcoin/rust-bitcoin#2636 (ie., using From<T> for Message instead of ThirtyTwoByteHash). And its a nice improvement.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 9f28cf6

@apoelstra
Copy link
Member

Let's go!

@apoelstra apoelstra merged commit a05078f into rust-bitcoin:master Apr 2, 2024
21 checks passed
@tcharding tcharding deleted the 03-27-rm-ThirtyTwoByteHash-impls branch April 2, 2024 01:39
@Kixunil
Copy link
Collaborator

Kixunil commented Jun 18, 2024

Now that I'm reading this, this was a great change because people won't accidentally sign silly hashes such as Txid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple major revs in hashes dependency doesn't work the way I expected
3 participants