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
Add optional bitcoin_hashes feature to implement ThirtyTwoByteHash #206
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok so far. I'd like to look a bit closer in the situations where this trait is used and how this affects the UX as a rust-secp user.
I might also be tempted to have the feature be default.
@@ -46,6 +46,10 @@ rand_core = "0.4" | |||
serde_test = "1.0" | |||
bitcoin_hashes = "0.7" | |||
|
|||
[dependencies.bitcoin_hashes] | |||
version = "0.7" | |||
optional = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any arguments here against making that default? Annoyingly someone that is depending on bitcoin
and depends on secp and hashes using bitcoin::secp256k1
and bitcoin::hashes
can't add features to the secp dependency. Well, that's a broader Cargo problem, but well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this a lot 😅 (also because of features I might want to activate in secp256k1/bitcoin_hashes
to not cause incompatibilities), turns out that cargo builds dependencies with the union of all the features activated if a dependency exists multiple times in the dependency graph (just tested it with a minimal example). So all they had to do was to specify secp256k1
as a dependency themselves and activate the feature.
On the other hand we could just activate that feature in bitcoin
and use it there too. After taking a closer look we don't seem to have any transaction signing code? At least we don't use secp256k1::Message
anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure? It might be that we use an into()
and that Message
is never mentioned verbatim. I add a usage of this in the Bitcoin Signed Message PR
.
|
||
/// Constructs a `Message` by hashing `data` with hash algorithm `H` | ||
#[cfg(feature = "bitcoin_hashes")] | ||
pub fn from_hashed_data<H: ThirtyTwoByteHash + bitcoin_hashes::Hash>(data: &[u8]) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm how is a method like this used? By explicitly providing the type param? That might be a bit awkward UX?
let msg = Message::from_hashes_data<sha256::Hash>(&my_data[..]);
Or is there another way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that's a bit awkward, but imo still better than having a separate method for each hash function. Given the confusion I should definitely document it better since using a type parameter in such a way is not too common. Alternatively I could just remove it, Message::from(sha256::Hash::hash(&my_data))
will do the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added an example to the docs. I personally find that, although a bit longer, the first version reads better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
75291bf
to
c905de2
Compare
I think this is interesting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
src/lib.rs
Outdated
#[cfg(feature = "bitcoin_hashes")] | ||
pub fn from_hashed_data<H: ThirtyTwoByteHash + bitcoin_hashes::Hash>(data: &[u8]) -> Self { | ||
let hash: H = bitcoin_hashes::Hash::hash(data); | ||
Message::from(hash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: What a missed chance for a cool one-liner!
<H as bitcoin_hashes::Hash>::hash(data).into()
Don't we all love Rust! :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, that's neat, let me sneak it in :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, didn't notice you have the trait imported
#[cfg(feature = "bitcoin_hashes")]
use bitcoin_hashes::Hash;
So it could be shorted. But nvm that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually avoid that on purpose. It collides with std::hash::Hash
too often in my experience. EDIT: thinking about it: where is this import needed? 😄
|
||
/// Constructs a `Message` by hashing `data` with hash algorithm `H` | ||
#[cfg(feature = "bitcoin_hashes")] | ||
pub fn from_hashed_data<H: ThirtyTwoByteHash + bitcoin_hashes::Hash>(data: &[u8]) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
c905de2
to
57526f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 57526f8
I thought about the APIs in place here quite some time while implementing a PoC that uses the secp256k1 curve in ways other than just signing Bitcoin transactions and I found the Here is a proposal for dealing with the whole |
While your implementation looks technically sound and maybe even a bit more idiomatic too I'm not sure if we want to encourage to use arbitrary If there is ever a common and sane need to sign anything other than a hash we can extend the API. |
I tend to agree with that but it is kind of tricky depending on the abstraction level of your library (more low-level APIs tend to be unsafer in their usage). The fact that Currently, the However, the trait documentation mentions things like all-zero messages and ones that overflow the group order. That sounds like we should actually be doing more checks in the |
Having a hashing crate as an optional dependency isn't odd at all since the only I see that yet another dependency to be updated isn't ideal, but it's a trade off I'm willing to make for a safe API. Alternatively there could be a separate crate secp256k1-safe that contains the |
Who said that it's supposed to deal with an elliptic curve? The README states "rust-secp256k1 is a wrapper around libsecp256k1, a C library by Pieter Wuille for producing ECDSA signatures using the SECG curve secp256k1". So the main point of this library is to create (and verify) ECDSA signatures. And ECDSA relies on an elliptic curve and a hash function. People tend to forget about the hash function.
Worse, it's actually not "signing" if you don't do the hash. |
That is a fair point. I guess something like this In any case, this sounds to me like we should just directly remove |
I'm not a big fan of traits like I'm not sure what the best option here is. |
The problem with that is (and why this crate shouldn't have an API like that either) that a naive implementation |
We are not achieving this as long as |
I added documentation that makes it pretty clear to normally use the new API and only use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK.
Is there any opposition to me merging this? I think this is roughly the API we had in mind when we created the ThirtyTwoByteHash
, and I think having bitcoin_hashes be an optional non-default dependency of rust-secp is the least ugly way to manage the dependency tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, Why not. as Andrew said this is probably the best compromise
This PR adds
bitcoin_hashes
as an optional dependency to implement theThirtyTwoByteHash
trait for the relevant hash types. It also adds a shortcut functionMessage::from_hashed_data<H: Hash>(&[u8])
.