Impl TryFrom<&[u8]> for all compressed point types.

[isislovecruft]

This reduces copy-pasta in downstream users to check the length of the slice beforehand.

[hdevalence]

This seems good, should there also be an implementation for montgomery points?

[hdevalence]

Thinking about this a bit more on the weekend, I have concerns about the use of () as the error type.

I think that this will interact poorly with the type conversions used in the ? operator, which require a tree of error types with From conversions from more-specific to less-specific. But the error type used here is (), which is one of the least-specific types already -- how can another crate use this impl in a method of type -> Result<_, SomeError> without using map_err? They would need to have impl From<()> for SomeError, which a) may not be possible (if SomeError is not defined locally) and b) is wrong, because it doesn't make sense in general to convert () to SomeError.

[isislovecruft]

I can also implement this for Montgomery points. Also, I can make a curve25519 specific error type—I just avoided doing that because I didn't know how much error-handling boiler plate we'd want in this crate.

[isislovecruft]

@hdevalence Alright, you've now got custom error types, with optional dependency and implemenation on the failure crate. The impl TryFrom<&[u8]> for MontgomeryPoint additionally checks that the u-coordinate was a canonicalised field element.

[hdevalence]

I don't think we should depend on failure, because it has its own custom error trait that doesn't play well with the standard library's Error trait, meaning that it's difficult to use in non-failure contexts, and now that Error has been fixed to include the improvements from failure, failure will be less-used anyways. However, the stdlib's Error trait isn't in core, so using it would mean either doing a no_std/std compatibility hack, like the rand crate did, or having a deserialization API that's different in no_std contexts, which seems less than ideal.

Previously, we avoided all of this by using only Option-based APIs, which are easily compatible with any error type in any curve25519-dalek-using crate, as the caller can use ok_or with that crate's preferred error type. I think continuing with that design decision seems like the best option here.

[tarcieri]

The approach we've used in various RustCrypto crates is an opaque struct Error; type, which receives a std::error::Error impl whenever the std feature is enabled. It seems to be working out fairly well for us.

[isislovecruft]

@hdevalence Would you be okay with the std-only impl of std::error::Error for CurveError that @tarcieri suggested?

[isislovecruft]

Hmm.. I'm afraid I don't understand why I'm getting this error, for like the first time ever in Rust: https://travis-ci.org/dalek-cryptography/curve25519-dalek/jobs/602034204#L355

error[E0433]: failed to resolve: use of undeclared type or module `dyn`

  --> src/errors.rs:60:34

   |

60 |     fn source(&self) -> Option<&(dyn ::std::error::Error + 'static)> {

   |                                  ^^^ use of undeclared type or module `dyn`

warning: trait objects without an explicit `dyn` are deprecated

  --> src/errors.rs:60:34

   |

60 |     fn source(&self) -> Option<&(dyn ::std::error::Error + 'static)> {

   |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `dyn`: `dyn dyn ::std::error::Error + 'static`

   |

   = note: `#[warn(bare_trait_objects)]` on by default

error: aborting due to previous error

[WildCryptoFox]

@isislovecruft Try &(dyn std::error::Error + 'static). This is a limitation of Rust's 2015 edition. It works as expected in 2018 edition. Recall the compiler rarely recognizes whitespace. I thought the dyn keyword was introduced in 2018 edition? shrugs

[hdevalence]

Rather than having a stub error type that may or not be std::error::Error, and which is only ever used in one method, I think it would be better to provide an Option-based API, which can be consistent with all of the other functions the library exports. Library consumers can use .ok_or to transform the Option into a Result however they like, we maintain API consistency, and avoid having to deal with error types entirely.

[vlopes11]

Need to use Scalar as one of a generic type set that serializes / deserializes. For that, I need a implementation TryFrom[&[u8]]; was very happy to see this PR 😃

Created the following PR to extend this one for other types:
isislovecruft#1

[isislovecruft]

@hdevalence I'm not sure I precisely understand what you're suggesting.. do you mean, rather than implementing TryFrom, to instead create a custom try_from_bytes() method?

[hdevalence]

No, I'm suggesting that we should provide an Option-based API like the rest of the crate, e.g., making from_slice return an Option instead of panicking. Then callers can use .ok_or with their crate's error type to get something that can be used with ? with no compatibility issues.

[isislovecruft]

Ah, I see. Sure.

[isislovecruft]

Done. Do we also want a similar API for Scalar?

[hdevalence]

I would prefer not to have a similar API for Scalar, because I don't think that there's an appropriate default deserialization check in the same way as there is for the compressed point types. Unfortunately, being compatible with the various X/Ed25519 scalar usecases etc. means that API users need to be aware of their requirements, and I don't think we can fix the broken ecosystem with our API alone, so I would prefer to keep the current design where there are different constructors for different requirements and users can't avoid making an explicit choice.

[isislovecruft]

Sounds good. This branch is ready for review then. Also I can squash it down and remove the history if you like.

[hdevalence]

This is not the right definition of validity; all 32-byte strings are valid representations of elements of the curve+twist pair, and this check should not exist in the source code.

[isislovecruft]

§5.2 of "Montgomery Curves and Their Arithmetic" reads:

both E_{(A,B)}(𝔽_q) and E_{(A,B')}(𝔽_q) have cryptographically strong (i.e. almost prime) group orders. This means that every element of 𝔽_q corresponds to a point on a cryptographically strong Montgomery curve, and implementers need not perform any point validation checks in this protocol.

My reading of that is that we do not need to check that a 32-byte string is a canonical representation of a field element, however we probably should check (which the decoding function explicitly does not do) in order to avoid inputs in [2^255 - 18, 2^255).

[hdevalence]

That is the check that the encoding is canonical, which is why we don't need it.

[isislovecruft]

Yes, I realise that is the the check that the encoding is canonical. I'm arguing that we should do the check to ensure the 32-bytes are a canonical element in 𝔽_q, otherwise the callee must check this if they care about potential "non-contributory" inputs. (My understanding of TryFrom is essentially "here's some random crap, make it absolutely safe if you can".)

[isislovecruft]

I mean, consumers of our library are doing similar checks on their own (cf. tendermint/tmkms#279). I'm arguing that we should give them proper tools to Just Do The Thing And Not Have To Worry, since the whole point of this PR is to reduce copy-pasta.

[isislovecruft]

Uh.. I'm confused, @hdevalence, why you'd unilaterally revert my commits without any given reasoning or explanation? And then furthermore gaslight me by claiming you're not doing this?

Do you have a problem with my code?

[hdevalence]

The reasoning is given in the commit message of the revert commit, linked above in this thread:

These changes are not semver-compatible with the 2.0.0 release.

Searching for "2.0.0" on the issue tracker shows #265 (comment), which was crossreferenced from various other issues tracking the 2.0.0 change.

Regarding "gaslighting", the date on your original message, which you did not include in the screenshot, was November 22; I made the revert commit yesterday afternoon, December 10, because I was concerned that other minor changes would be made on top of the incompatible code in develop and that it would end up in a patch release, breaking downstream code.