search

penumbra-zone / ed25519-consensus

Ed25519 suitable for use in consensus-critical contexts.
45 stars 12 forks source link

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

Open tarcieri opened 1 year ago

tarcieri commented 1 year ago

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 commented 1 year ago

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

tarcieri commented 1 year ago

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.

tarcieri commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Here's a PR to switch to infallible parsing: https://github.com/RustCrypto/signatures/pull/623

tarcieri commented 1 year ago

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.