Deprecate ThirtyTwoByteHash

[tcharding]

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

[apoelstra]

concept ACK, but looks like this breaks some tests.

[tcharding]

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]

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]

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]

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

[tcharding]

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

[apoelstra]

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]

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]

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]

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.

[apoelstra]

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.

[apoelstra]

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.

[apoelstra]

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]

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

[tcharding]

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.

[apoelstra]

ACK 9f28cf6

[apoelstra]

Let's go!

[Kixunil]

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