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

ECPoint improvement #97

Open
survived opened this issue Dec 22, 2020 · 0 comments
Open

ECPoint improvement #97

survived opened this issue Dec 22, 2020 · 0 comments

Comments

@survived
Copy link
Contributor

I propose the following changes in ECPoint & ECScalar traits aimed at better user experience, reducing unsafe unwrap()s in curve implementations, based on feedback from people who built their solution on top of curv crate.

  1. from_coor(x, y) method should return Result<Self, ErrorKey>.
    Currently it returns Self and panics if point is not on the curve.
  2. Add is_on_curve(x, y) -> bool method.
    It might be helpful for early point validation.
    1. Add is_at_infinity(&self) -> bool method.
      Currently, the only way to check if the point is at infinity (as I understood) - call x_coor()/y_coor(). If any of them are None, then it's a point at infinity. It works, but it looks unclear in the code.
    2. Another way is to restrict ECPoint to never be at infinity. Then we should add methods add_point_checked, sub_point_checked and scalar_mul_checked which should return Option<Self> (as any of them may produce a point at infinity). Also add_point, sub_point and scalar_mulshould be explicitly marked that they will panic if point at infinity occurs.
      x_coor and y_coor methods will return BigInt instead of Option<BigInt> as they are always present (am I right?).
  3. pk_to_key_slice and bytes_compressed_to_big_int methods are a bit confusing.
    They both do the same thing - marshal point to bytes. But the first marshals in uncompressed form (which is unclear until you look at implementation). Also confusing thing is that one method returns Vec<u8>, another returns BigInt. I suggest making both methods to return Vec<u8>. The caller will have to convert to BigInt if it's necessary by calling BigInt::from(...).
    So I suggest to replace both methods with a single one:
    fn to_encoded_point(&self, compressed: bool) -> Vec<u8>
  4. Also we should probably restrict ECScalar not to ever be zero. In such a way, the following changes are required:
    1. from(bytes) -> Self method should return Result<Self>
    2. set_element(&self, ...) method should probably also return Result<()> (as we cannot generally guarantee that every PK is proven to be non-zero)
    3. zero() -> Self method should be removed
    4. Add add_checked, mul_checked, sub_checked methods which return Option<Self>. add, mul, sub methods should be explicitly marked that they will panic if zero scalar occurs.
  5. Should ECScalar trait leak more info about a curve? Currently, it exposes only the order of G (ECScalar::q()). I would add such enum:
    #[non_exhaustive]
    enum CurveParams {
      Weierstrass {
        a: BigInt, b: BigInt, q: BigInt, p: BigInt, ...
      },
      Montgomery {
        a: BigInt, ...
      }
    }
    And then add method to ECScalar:
    fn params() -> &'static CurveParams

It's a big list of suggestions, but every one may be considered independently.

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

No branches or pull requests

1 participant