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

ecdsa::hazmat::DigestPrimitive is not implemented for Secp256k1 #189

Closed
jedisct1 opened this issue Sep 18, 2020 · 14 comments
Closed

ecdsa::hazmat::DigestPrimitive is not implemented for Secp256k1 #189

jedisct1 opened this issue Sep 18, 2020 · 14 comments

Comments

@jedisct1
Copy link
Contributor

Hi,

Since version 0.5, Secp256k1 signatures cannot be created nor verified due to DigestPrimitive not being implemented.

The example code from the documentation fails:

   | let signature: Signature = signing_key.sign(message);
   |                                        ^^^^ the trait `ecdsa::hazmat::DigestPrimitive` is not implemented for `Secp256k1`
   |
   = note: required because of the requirements on the impl of `PrehashSignature` for `ecdsa::Signature<Secp256k1>`
   = note: required because of the requirements on the impl of `Signer<ecdsa::Signature<Secp256k1>>` for `SigningKey`
@tarcieri
Copy link
Member

You need to enable the sha256 feature.

(I see this is not documented at the moment)

@tarcieri
Copy link
Member

I might do a follow-up release that enables sha256 feature as part of the ecdsa feature.

It was removed as some users may only care about other digest functions (e.g. keccak256) but eliminating the superfluous dependency for those users probably isn't worth the confusion it causes.

@jedisct1
Copy link
Contributor Author

Thanks!

Not enabling it as part of the ecdsa feature is fine. People may also want to use a different sha256 implementation.

What is actually confusing is the fact that the sign() and verify() functions are available when the sha256 is not enabled, even though they cannot be used.

@jedisct1
Copy link
Contributor Author

sign_with_rng() also seems to be gone (not sign_digest_with_rng()).

Is that intentional?

@tarcieri
Copy link
Member

I can gate those on the sha256 feature too. That's probably not even a SemVer breaking change, since they're useless without it.

@jedisct1
Copy link
Contributor Author

Yep, gating them would be useful, especially since the Rust error message doesn't really help understand the root cause.

@tarcieri
Copy link
Member

sign_with_rng() also seems to be gone... Is that intentional?

That looks like an oversight

@tarcieri
Copy link
Member

tarcieri commented Sep 18, 2020

Here's a PR to gate the Signer and Verifier impls on sha256: #192

@tarcieri
Copy link
Member

#193 adds a RandomizedSigner impl.

@jedisct1 can you try the latest master and see if it resolves your issues?

I'll also add some more documentation around the sha256 feature.

@jedisct1
Copy link
Contributor Author

Looking good, thank you!

@tarcieri
Copy link
Member

Will cut another release with these changes soon.

@TheRealBluesun
Copy link

When I try using the keccak256 feature instead of sha256, I get this same error. Is this intended?

Basically I want to generate an Ethereum-friendly signature, which is keccak256. Any advice?

@TheRealBluesun
Copy link

I resolved the issue by adding the following to k256/src/ecdsa.rs

#[cfg(all(feature = "ecdsa", feature = "keccak256"))]
impl ecdsa_core::hazmat::DigestPrimitive for Secp256k1 {
    type Digest = sha3::Keccak256;
}

@tarcieri
Copy link
Member

As noted in #269, I think you want k256::ecdsa::recoverable::Signature.

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

No branches or pull requests

3 participants