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

Improve ergonomics of scalar field multiplication. #443

Merged
merged 7 commits into from
Aug 1, 2022

Conversation

mmaker
Copy link
Member

@mmaker mmaker commented Jul 18, 2022

Hello! This pull request aims to improve the ergonomics of arkworks scalar multiplication by eradicating the plague that into_bigint() has become without sacrificing efficiency.

This change will affect Affine::mul and Projective::mul: instead of accepting as input bits packed into limbs (AsRef<[u64]> or PrimeField::BigInt), we accept aScalarField element. Another function mul_bigint is made available for implementations where conversion for Montgomery form is not necessary.

We believe this will greately improve ergonomicity of arkworks, and since this change will probably affect also other crates in the arkworks ecosystem we'd like to know what's the best way to proceed aligning also the other repositories.

This is partially related (but doesn't conflict) to #294.

asn-d6 and others added 2 commits July 18, 2022 21:41
It will actually take the place of `mul` in the next commit...

Co-authored-by: Michele Orrù <[email protected]>
@asn-d6
Copy link
Contributor

asn-d6 commented Jul 18, 2022

Some questions that appeared while coding the PR:

  • Do we need #[must_use]? If yes, do we also want it in the projective curve?
  • Do we like the name mul_bigint()? It actually multiplies by a ref to u64, so maybe mul_limbs() could be an alternative name.
  • Should we move forward with implementing the Mul trait for the curves, so that we can do B = b*A in arkworks? I know that in the past there were concerns about Mul allowing scalar multiplication from the right (i.e. B = A*b) which is not so nice, but maybe these concerns are not as important as the potential ergonomic benefits?
  • This change will naturally result in many crates building if they complete against master. What's the best way to address such breakage and which packages would you consider essential to be fixed to get this merged?

Thanks!

@weikengchen
Copy link
Member

I would say it is very likely that we need to keep the old one as well.

And Mul for different ways is probably better.

@mmaker
Copy link
Member Author

mmaker commented Jul 31, 2022

@weikengchen what does it mean "the old one"? Mul by bigints? also for implementing the trait Mul, this will be a separate pull request. See https://github.com/mmaker/arkworks-algebra/tree/feature/mul-operand-affine, but I do think that it should be defined only for scalars. Do you have any examples where you'd like to multiply by another type and conversion to/from montgomery form is crucial?

@weikengchen
Copy link
Member

Got it. Just found out this

    + for<'a> Mul<&'a Self::ScalarField, Output = Self>

Then it makes sense.

@Pratyush Pratyush merged commit dd5b9d6 into arkworks-rs:master Aug 1, 2022
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

Successfully merging this pull request may close these issues.

4 participants