Export multiple curves

[survived]

Issue #91

This PR breaks current limitation in exporting only one elliptic curve implementation at one time. All elliptic crypto algorithms defined in curv are going to be generic over a choice of elliptic curve. To do so, significant changes were introduced in API.

[survived]

Hi, @omershlo! My PR is in progress yet, but I believe the most significant changes were made. May I have a short code review? My changes have introduced a lot of .clone(). I think in such a way, in particular, I made some of zeroizing pointless. Check out this function. The problem I worry about is described in this article.

I think this problem can be handled by introducing some more complicated code. Does it worth it? Did the code get already too complicated?

[omershlo]

This looks really promising!

about clone() and zeroize: I suggest to not worry about this at this point and save it for later PR. This PR should be focused on the API change

[omershlo]

question: Do you think we can let go of the ECScalar trait all together ?

[survived]

question: Do you think we can let go of the ECScalar trait all together ?

Are you asking if it's possible to remove ECPoint::Scalar associated type from the trait?

[survived]

@omershlo Why do you use nightly rust when building on CI? This causes pipelines to be always failed with message:

error: component 'rustfmt' for target 'x86_64-unknown-linux-gnu' is unavailable for download for channel nightly
Sometimes not all components are available in any given nightly.

Except for this issue with pipelines, PR is done. Every crypto algorithm is updated.

[survived]

Examples are updated too, but they look a bit weird. Concretely I don't like a part of choosing curve implementation. Can you take a look at it? Should I do something with that? (e.g. here)

[survived]

Also, I found that many tests on curve implementations look very similar. Should we generalize some of the tests and place them separately from curve implementation? Compare tests in jubjub and bls12_381. Most of the tests are semantically equal.

[survived]

And one more thing: should we do something with all these traits: NISigmaProof, ProvePederesen, ProvePederesenBlind, ProveDLog, HomoELGamalDlogProof (not a trait), HomoELGamalProof (not a trait too)? They all have methods solve() and prove(), but somewhere we have a trait (with 3 generic type params or without generic type params), somewhere methods are implemented directly on the type.

According to TODO comment here, you planned to use one trait in every case (as I understood). We could do it as part of this PR or leave it for the future.

[omershlo]

• using nightly in CI - I noticed it as well: I will remove these checks

[omershlo]

Also, I found that many tests on curve implementations look very similar. Should we generalize some of the tests and place them separately from curve implementation? Compare tests in jubjub and bls12_381. Most of the tests are semantically equal.

This PR opens up the possibility to merge all tests. In theory for every curve the same suite of tests should suffice - but as you noticed there are some differences. I will check about about that and get back to you

[omershlo]

And one more thing: should we do something with all these traits: NISigmaProof, ProvePederesen, ProvePederesenBlind, ProveDLog, HomoELGamalDlogProof (not a trait), HomoELGamalProof (not a trait too)? They all have methods solve() and prove(), but somewhere we have a trait (with 3 generic type params or without generic type params), somewhere methods are implemented directly on the type.

According to TODO comment here, you planned to use one trait in every case (as I understood). We could do it as part of this PR or leave it for the future.

To be honest - the different traits are probably unnecessary. At some iterations of the code I wanted to check if there is a way to capture a family of proofs with a trait but in practice so far I don't see a value in doing it. I would advice to remove the traits all together.

[omershlo]

@survived any reason to keep the ecc feature? why not go completely featureless

[doronz2]

Hi @survived. I'de like to add a "meta curve" that consist of two curves (the "curve" bls12_381 actually consist of two curves), that interact with each other (under the pairing function). In particular, I'de like to have the feature that whenever I choose bls12_381, both curves will be instantiated. Is it suitable with this current structure? (I know that's the whole purpose of this PR, I just want to make sure)

[survived]

@survived any reason to keep the ecc feature? why not go completely featureless

No reason. Let's remove all the features then

[survived]

Is it suitable with this current structure? (I know that's the whole purpose of this PR, I just want to make sure)

Hi @doronz2. I guess yes. This PR makes you able to add as many EC implementation as you want. So you can add a "meta curve" as a separate implementation, or change behavior of some other curve implementation with a feature.

[doronz2]

Thanks!

On Wed, Nov 4, 2020, 19:01 survived notifications@github.com wrote:

Is it suitable with this current structure? (I know that's the whole purpose of this PR, I just want to make sure)

Hi @doronz2 https://github.com/doronz2. I guess yes. This PR makes you able to add as many EC implementation as you want. So you can add a "meta curve" as a separate implementation, or change behavior of some other curve implementation with a feature.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ZenGo-X/curv/pull/92#issuecomment-721853814, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABKRANWTV5QTY6TH734WVBLSOGCGHANCNFSM4TDNXSIA .