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

Use ed25519::Signature as the signature type; MSRV 1.60 #6

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tarcieri
Copy link

@tarcieri tarcieri commented Dec 27, 2022

This allows using ed25519-consensus in conjunction with the signature::{Signer, Verifier} traits. These traits are generic around a signature type parameter, which in this case is ed25519::Signature.

Uses namespaced features to activate both dep:serde and ed25519/serde, which requires an MSRV of 1.60. Note this is also the MSRV of ed25519 v2.1+.

@tarcieri
Copy link
Author

Note: this PR doesn't yet impl the Signer or Verifier traits, but should for this PR to actually be useful.

@tarcieri
Copy link
Author

tarcieri commented Dec 27, 2022

Another note: the MSRV bump isn't strictly necessary and can be worked around by renaming serde to serde_crate, although I was having trouble making that work with custom derive, which is hardcoded to serde. Edit: ed25519 v2.1+ is now MSRV 1.60.

README.md Outdated Show resolved Hide resolved
src/signing_key.rs Outdated Show resolved Hide resolved
@tarcieri
Copy link
Author

tarcieri commented Jan 21, 2023

This is basically good to go with one caveat:

I've left the SigningKey::sign and VerificationKey::verify inherent methods, even though the Signer and Verifier traits also define Signer::sign and Verifier::verify methods.

The sign method has a matching signature, however VerificationKey::verify has signature and msg arguments, whereas Verifier::verify has msg and signature arguments in the reverse order.

To land this PR, I would suggest reversing the ordering so they're consistent among the inherent and trait methods (signature v2.0.0 has already shipped so the ordering cannot be reverse there at this point).

Another discrepancy is the error type: Verifier::verify returns a Result with signature::Error type, whereas VerificationKey::verify uses ed25519_consensus::Error, which may be confusing. Generally when inherent methods and trait methods overlap names, my personal preference is for them to be completely identical to prevent confusion.

Another option is to completely remove the inherent methods and always use the traits, although this requires importing them which is a bit more onerous than calling an inherent method.

@hdevalence
Copy link
Member

Thanks for the bump, sorry I missed this in December!

On the argument ordering front, I think we could just swap the argument ordering to be consistent as you suggest, and do a breaking change.

On the error front, it seems like there are two options: either (a), change ed25519_consensus to use the signature::Error type (and re-export it so that callers don't need to pull in another dep), or (b) accept differing error types. I'm not super attached to having a custom error type, so (a) seems like it might be preferable?

Now that signature v2.0.0 has shipped, what are the remaining options in re: fallibility? I'd prefer to keep Signature as a refinement type wrapping bytes and keep all of the consensus rules in one place.

@tarcieri
Copy link
Author

We also just shipped ed25519 v2.0, although it's only been out for 5 days and doesn't have any released transitive dependencies or a large number of downloads, so I suppose it could still be yanked and fallible parsing revisited. That would still be a little messy and we should make it happen ASAP.

@tarcieri
Copy link
Author

I yanked ed25519 v2.0.0. So far we haven't had any complaints.

Here's a PR to switch to infallible parsing: RustCrypto/signatures#623

This allows using `ed25519-consensus` in conjunction with the
`signature::{Signer, Verifier}` traits.

These traits are generic around a signature type parameter which is used
to identify a particular signature algorithm, which in this case is
`ed25519::Signature`. This type has been used to replace the signature
type originally defined in this crate, which is necessary to make
`Signer`/`Verifier` work.

Uses namespaced features to activate both `dep:serde` and
`ed25519/serde`, which requires an MSRV of 1.60. Note this is also the
MSRV of `ed25519` v2.1+.
@tarcieri tarcieri changed the title [WIP] Use ed25519::Signature as the signature type; MSRV 1.60 Use ed25519::Signature as the signature type; MSRV 1.60 Jan 23, 2023
@tarcieri tarcieri marked this pull request as ready for review January 23, 2023 17:21
@tarcieri
Copy link
Author

This is ready for review.

The ed25519 v2.0.1+ (with v2.0.0 yanked) API now supports infallible signature parsing.

I've also changed the argument ordering for VerificationKey::verify to match the signature crate. The return types are still different however, which might be worth addressing.

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.

2 participants