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

Reduce code duplication in field implementations #49

Open
str4d opened this issue Oct 13, 2022 · 3 comments
Open

Reduce code duplication in field implementations #49

str4d opened this issue Oct 13, 2022 · 3 comments

Comments

@str4d
Copy link
Contributor

str4d commented Oct 13, 2022

Currently this library has two field implementations, the Pallas field and the Vesta field. They only differ in their prime modulus, and are otherwise almost identically implemented. This makes the library harder to maintain, as every change needs to be duplicated.

There are several possible approaches we could take to improve the situation:

  • Use a declarative macro.
    • We already use a declarative macro for implementing the curves, so it would be in keeping with the rest of the library.
    • This has been proposed and implemented here: e150cc5
  • Use a procedural macro, i.e. ff_derive
    • This is how we originally implemented BLS12-381 in the pairing crate, but we replaced that with a direct implementation of the field in the bls12_381 crate, and ff_derive hasn't been as effectively maintained since. If we switched to it, the majority of our field maintenance effort could go into ff_derive instead of here.
  • Use crypto-bigint to implement the internal field details.
    • This would reduce the amount of code we need to maintain, which would just be the mapping from a big integer element to a prime field element.
    • This would be quite a divergence from previous approaches. I also don't know how feature-complete crypto-bigint is, though progress is being made.
    • If this is a practical and performant approach, we could also combine it with either the declarative macro or ff_derive approach to make it easier to maintain as well.
@str4d
Copy link
Contributor Author

str4d commented Oct 13, 2022

Something I want to consider for this is how easy each of the approaches makes implementing "backends": specialisations for different targets for improved performance (e.g. AVX2).

@ashWhiteHat
Copy link

And I also have idea about code duplication.

We can separate interface and arithmetic(algorithm).
The arithmetic is often the same for field and curve.
We can implement it separately like following.
https://github.com/zero-network/zero/tree/master/primitive/crypto/src/arithmetic
And interface is just passing its arguments to arithmetic function.
https://github.com/zero-network/zero/blob/master/primitive/crypto/src/dress/curve/edwards/group.rs#L76

If we apply the optimization to arithmetic, it affects for every component.
How about creating zkcrypto/crypto-arithmetic and import arithmetic from components?
It will be a big refactoring but we can reduce code duplication and synchronizing workload.

@ashWhiteHat
Copy link

I think macro is not good way to reduce code because it makes visibility bad.
but we can use macro for the parts where visibility is not necessary as in RefOps.
We can also implement it as module with the same way with above arithmetic.

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

2 participants