elliptic-curve: refactor PublicKey into sec1::EncodedPoint

[tarcieri]

Previously PublicKey was a "wire type" for SEC1 encoded curve points which by design didn't validate the point is on the curve (so as to avoid mandating a curve arithmetic implementation for the type to be useful).

However, this has a pretty big drawback: when we do have a curve arithmetic implementation, there are many fallible operations which could be infallible if we knew the point is valid a priori.

Additionally, "thanks" to typenum-based arithmetic, it has pretty atrocious trait bounds, which are needed anywhere we presently deal with a public key.

This commit moves this encoded point type to sec1::EncodedPoint, which both much more precisely describes what it represents, but also moves it out-of-the-way so we can add a better PublicKey type (e.g. one which is or wraps an AffinePoint in crates which have a curve arithmetic implementation).

[tarcieri]

Opening this as a draft, both to get early review, and because it's a SemVer breaking change and I think there are still some non-breaking, purely additive changes that can be made to the elliptic-curve crate before we bump minor versions again.

[tarcieri]

Something we could do here which would be a significant simplification (especially in regard to the trait bounds) is get rid of CompressedPoint and UncompressedPoint (and CompressedPointSize and UncompressedPointSize) entirely and have EncodedPoint be a struct with a GenericArray buffer which is large enough to contain either a compressed-or-uncompressed point (which can still be disambiguated by the leading tag byte).

I might take a stab at that as part of this PR.

[tarcieri]

I pushed up an additional commit (7a3a0ce) which I think simplifies things greatly by eliminating the CompressedPoint and UncompressedPoint types.

[fjarri]

Can't say I'm a big fan of a mutable EncodedPoint. I would prefer to have the correctness of its state enforced by the compiler.

Also, what would be the opposite of compress()? A FromEncodedPoint implementation for AffinePoint?

[tarcieri]

Can't say I'm a big fan of a mutable EncodedPoint.

I take it that's in regard to compress()?

Also, what would be the opposite of compress()?

I'd also like to conditionally impl decompress() (it came up right away when people were trying to use k256 and match test vectors), and yes, the impl could be bounded by a FromEncodedPoint impl on Arithmetic::AffinePoint, although we may need an additional ToEncodedPoint trait which takes a compress: bool parameter to replace the previous Into bounds.

[fjarri]

I take it that's in regard to compress()?

Yes, and in general for EncodedPoint to have two possible states. If the type conditions for an enum were too complicated, what about having two separate types (uncompressed/compressed encoded point)? This will also allow us to return a GenericArray from to_bytes() (no alloc needed).

[tarcieri]

That's exactly how the current CompressedPoint and UncompressedPoint types presently work (prior to this PR).

However, supporting type-level arithmetic for computing the sizes of both necessitates some pretty nasty trait bounds, and also spreads out and duplicates a lot of logic that can be consolidated under this approach (notice that this PR reduces LOC by ~40%).

Here's an example of how this approach simplifies the bounds.

[newpavlov]

I am not familiar enough with elliptic-curve codebase, so I can't really give a constructive feedback at the moment, but couldn't we solve the logic duplication problem by introducing newtype wrappers over a Point type covering all shared operations?

[tarcieri]

As someone who's a fan of newtype wrappers, I'm not sure how they'd help here.

Concretely I'd break the problem down here as either validating or constructing an encoded document:

• The parser validates the document and then stores it verbatim
• The serializer constructs the document

The previous implementation used separate types for when the document is compressed or uncompressed, and an enum to provide a common type for representing both forms. But really in pretty much all cases the enum is all anything ever interacted with.

However, despite the enum being pretty much the only thing that mattered, the complexity of the compressed-vs-uncompressed type representations was leaking out everywhere in the trait bounds.

[tarcieri]

@fjarri I made compress() return a new point, and with that eliminated all &mut methods other than zeroize().

I also added a bevy of tests which provide ~95% code coverage on sec1.rs (sidebar: it'd be nice to set up codecov on this repo). Even with that, this PR still manages to shave off ~90 LOC versus the previous implementation, which had no tests(!)

I ended up liking this enough that I marked this as "ready for review" as I think it has significantly higher code quality while also simplifying the trait bounds significantly. Earlier I said I thought we might want to hold off on breaking changes, but I changed my mind: this cleans up the trait bounds enough I think I'd like to move forward with it, along with defining a replacement public key type (e.g. associated type e.g. AffinePoint or newtype wrapper thereof)

[fjarri]

That's exactly how the current CompressedPoint and UncompressedPoint types presently work (prior to this PR).

Ah, that's right. So the main reason for the approach in this PR is the simplification of type bounds? I must say, I still prefer the solution with two separate types, especially since decompress() is not trivial. And if UncompressedPublicKey/CompressedPublicKey were separate types and not enum variants, perhaps the bounds could have been kept simple?

But anyway, I do not feel strongly enough about this to insist. It's your call.

@fjarri I made compress() return a new point, and with that eliminated all &mut methods other than zeroize().

I suppose the only issue with this approach might be in some kind of an embedded environment where you really have to fight for every byte - but I don't have much experience in that area.

[tarcieri]

So the main reason for the approach in this PR is the simplification of type bounds?

I'd say it provides three simplifications:

• Significantly simpler bounds
• One "wire type" instead of 3 (EncodedPoint vs PublicKey/CompressedPoint/UncompressedPoint)
• Simpler, more easily testable implementation

I must say, I still prefer the solution with two separate types, especially since decompress() is not trivial.

I think we can add something like a point::Decompress trait that looks something like:

pub trait Decompress<C: Curve> {
    fn decompress(x: &ElementBytes<C>, is_y_odd: bool) -> CtOption<Self>
}

...and implement all of the point decompression logic there. Then the EncodedPoint::decompress() implementation can be made generic with respect to that trait.

As it were, for lack of a nicely encapsulated trait like that, I wound up copying and pasting the point decompression code in the ECDSA public key recovery implementation here:

https://github.com/RustCrypto/elliptic-curves/blob/4ca0987/k256/src/ecdsa/recoverable.rs#L128-L144

And if UncompressedPublicKey/CompressedPublicKey were separate types and not enum variants, perhaps the bounds could have been kept simple?

They are separate types presently:

https://docs.rs/elliptic-curve/0.5.0/elliptic_curve/weierstrass/point/struct.CompressedPoint.html
https://docs.rs/elliptic-curve/0.5.0/elliptic_curve/weierstrass/point/struct.UncompressedPoint.html

Having these two types is the reason for the complex trait bounds, and that wouldn't go away by eliminating the enum, since the source of the complexity isn't the enum itself, but the above types that the variants wrap.

The reason for having an enum over them is to avoid having to use a "write everything twice" approach for handling point compression and making it a user-facing concern everywhere and instead make it as automatic as possible. The current approach allows a reasonable default setting for point compression to be selected on a per-curve basis, e.g.:

• NIST P-256 points are almost always uncompressed
• secp256k1 points are pretty much exclusively compressed (except in e.g. test vectors)

...but in either case, it's still possible to opt-in or opt-out of point compression. For example, age supports compressed P-256 points.

I suppose the only issue with this approach might be in some kind of an embedded environment where you really have to fight for every byte - but I don't have much experience in that area.

It uses marginally more stack space, at least until something like placement by return lands.

[tarcieri]

@fjarri I just noticed I didn't address your request for a to_bytes() method. I'll open a follow-up PR for that.