Use infallible conversions from hashes to secp256k1::Message

[Kixunil]

#819 (comment)

And check/fix other cases as well.

[arturomf94]

Hi @Kixunil / @bruteforcecat - is this still an open issue?

I see #851 has been opened as a draft, but I don't know if it's up for grabs now?

[Kixunil]

I think #830 should've been resolved by #1065 and #1066, so feel free to give it a try!

[arturomf94]

Thanks! I think I'll have a stab! Could you assign it to me please? 🙏

[tcharding]

Go for it man! Thanks

[arturomf94]

Hi @tcharding / @Kixunil - I'm having some trouble with this one. I've replaced all instances of secp256k1::Message::from_slice(_).expect(_) to secp256k1::Message::from(_). There is one particular case in which I'm getting an error:

rust-bitcoin/src/util/misc.rs

Line 143 in cd790dc

let msg = secp256k1::Message::from_slice(&msg_hash[..])

The error I'm getting is:

error[E0277]: the trait bound `hashes::sha256d::Hash: secp256k1::ThirtyTwoByteHash` is not satisfied
   --> src/util/misc.rs:143:23
    |
143 |             let msg = secp256k1::Message::from(msg_hash);
    |                       ^^^^^^^^^^^^^^^^^^^^^^^^ the trait `secp256k1::ThirtyTwoByteHash` is not implemented for `hashes::sha256d::Hash`
    |
    = note: required because of the requirements on the impl of `std::convert::From<hashes::sha256d::Hash>` for `secp256k1::Message`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `bitcoin` due to previous error

This is confusing me because such impl does exist.

Digging deeper, I believe this might be due to a mismatch between the versions of bitcoin_hashes used in rust-bitcoin and rust-secp256k1, which are 0.11.0 and 0.10.0, respectively.

Interestingly, I think the approach taken here does not work due to 58f94be.

I'm unsure about which could be the appropriate direction here 🤔 My bet is on matching the bitcoin_hashes version, but I'd love to read your thoughts...

[apoelstra]

We should update rust-secp I guess. Annoying because this will require a major version bump there. We should've coordinated this better ... I think I literally forgot that rust-secp depended on bitcoin_hashes and that we'd have to update things in order.

[arturomf94]

Sorry to be the bearer of these bad news! :/

Question: is there a specific reason why rust-secp256k1 does not have a more relaxed restriction on bitcoin_hashes? Something like, ">=0.10"?

[tcharding]

I think I literally forgot that rust-secp depended on bitcoin_hashes and that we'd have to update things in order

I'll take the rap for that one, I did the release PR without checking if this would break things.

[apoelstra]

Sorry to be the bearer of these bad news! :/

Question: is there a specific reason why rust-secp256k1 does not have a more relaxed restriction on bitcoin_hashes? Something like, ">=0.10"?

Prior to 1.0, cargo treats the minor version number as a major version number. There is no way you can tell it "use 0.10 or 0.11 or...", nor would we want to, because there are breaking changes between those releases.

[arturomf94]

Prior to 1.0, cargo treats the minor version number as a major version number. There is no way you can tell it "use 0.10 or 0.11 or...", nor would we want to, because there are breaking changes between those releases.

Got it. Thanks for the clarification.

[tcharding]

I've tried to clean up my mess, please see:

• Upgrade bitcoin hashes rust-secp256k1#475
• Depend on secp256k1 upcoming release #1103
• Add a test to check bitcoin_hashes dependency #1104

[Kixunil]

@apoelstra

There is no way you can tell it "use 0.10 or 0.11 or..."

You can using >= as @arturomf94 suggested but we definitely don't want to.

@tcharding no need to beat yourself up about this one. It would've easily slipped anyone. The only way I can think of avoiding it is to have a good release checklist with a reminder to use it.

[apoelstra]

I don't think a checklist would've helped, given that this would've been caught by #1104 but only if we'd thought of it beforehand..