Add RFC 8032 and NIST full validation decompression to EdwardsPoint

[daxpedda]

As discussed in #626. I went ahead and adjusted PrimeField::from_repr() to align with RustCrypto's implementations and provide a constant-time decompression method.

I suggest to get rid of the enum entirely. Users who want to check for prime-order subgroup can do so via is_torsion_free() to simplify the API. WDYT?

Let me know if I can improve the documentation in any way.

Resolves #626.

[daxpedda]

This still needs some work.
Done.

[tarcieri]

As outlined in #626, there are effectively three profiles to consider:

1. default/existing: ZIP-215 rules, which allow unreduced y-coordinates
2. RFC8032 / NIST partial: ensures y-coordinate is reduced
3. NIST full: ensures point belongs to the prime order subgroup

I spitballed a few names in #626 but an important thing to consider in the API design is how to handle the three different cases in ways that make sense.

This is all to say that validated_decompress doesn't really tell you whether it's 2 or 3.

We've spitballed whether different inherent methods or an enum make sense. I guess I'm generally leaning towards methods, maybe ones that include the actual validation criteria?

• decompress (implicitly ZIP-215, loosest)
• decompress_rfc8032
• decompress_nist_full

[daxpedda]

That makes a lot of sense to me.

[daxpedda]

Done!

[nazar-pc]

API-wise this makes sense and is backwards compatible, but my preference would be to not have the "default" implementation, but rather let the user to choose the actual variant explicitly.

For example, in ed25519-dalek there is VerifyingKey struct with from_bytes() method, which will currently uses ZIP-215 and with this PR it will not really be obvious that an alternative exists.

On another hand deserialization with serde will be impacted since it will use the default and if multiple variants exist, then either VerifyingKey needs to be duplicated or made generic or it will not be possible to conveniently deserialize with a specific validation variant, which is error-prone for anyone not using the default.

This PR is certainly a step in the right direction, but it'd be nice to address the problem on a higher level too.

[daxpedda]

API-wise this makes sense and is backwards compatible, but my preference would be to not have the "default" implementation, but rather let the user to choose the actual variant explicitly.

How do you propose that could be done?

Apart from adding cfg flags, which I think would make for a terrible UX imo, the only way I see this could be addressed is by adding methods to all of the types needing it. Absolutely no idea how this could be addressed for Serde implementations other then adding dedicated types.

FWIW: personally I think the default for Serde and other implementations should be RFC 8032.

[nazar-pc]

Feature flag will not work because it is additive and it is likely to have multiple variants enabled by different indirect dependencies. The most straightforward way is to rename decompress() to decompress_zip_215() and similarly for other methods, but that is a lot of churn and I'm not sure maintainers will share this vision.

The issue mentioned an enum, which could allow to retain decompress() method, but add a new required argument to it with enum variants describing how validation should be done.

As for serde, you can't change it without causing a lot of breakage in the ecosystem. If things compile, you generally expect it to work. Even major version bump will still be very frustrating IMHO due to how subtle the change is. And Since ZIP 215 happened first and basically all blockchains adopted this variant due to it being first (but not objectively better), changing the default would force effectively all current users to use non-default, at which point it might make sense to think about breaking API proper and do something better.

Something like VerifyingKey<ValidateZip215> could be a straightforward change that could retain serde and other APIs mostly as is, while supporting all variants, being very explicit and easy to adopt (could get away with a type alias and not change any of the code in downstream project at all).

VerifyingKey could in fact be an alias to some VerifyingKeySomething<T> upstream as well to make it mostly backwards compatible (IIRC there are some cases where type aliases behave slightly differently than the types themselves).

---

So far I think enum with decompress() and the likes, as well as making VerifyingKey generic is my preference.
Note that I'm not a maintainer, just a user that knows very little about this stuff, but is interested in using NIST variant downstream instead of ZIP 215.

[nazar-pc]

@tarcieri could you take another look at this PR, please?

[tarcieri]

Suggested change

	hex-literal = "0.4"
	hex-literal = "1"

[tarcieri]

Not really wild about this. Another way to check for a canonical representative is by decoding, re-encoding, and comparing equality.