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

Allow multiplication of extension field elements directly by the BasePrimeField element #779

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mmagician
Copy link
Member

Description

Note, there was already a method to multiply by the extension field's BaseField: but for a field tower, a BaseField might not necessarily be the BasePrimeField, but rather an extension field itself.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the GitHub PR explorer

@mmagician mmagician requested review from a team as code owners February 13, 2024 16:33
@mmagician mmagician requested review from z-tech, Pratyush and weikengchen and removed request for a team February 13, 2024 16:33
@mmagician
Copy link
Member Author

@suyash67

@@ -350,6 +350,8 @@ pub trait Field:
}
Some(res)
}

fn mul_by_base_field_elem(&self, elem: &Self::BasePrimeField) -> Self;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn mul_by_base_field_elem(&self, elem: &Self::BasePrimeField) -> Self;
fn mul_by_base_prime_field(&self, elem: &Self::BasePrimeField) -> Self;

Also, should we move this into a Mul<Self::BasePrimeField, Output = Self> bound? (And maybe add for<'a> Mul<&'a Self::BasePrimeField, Output = Self>, MulAssign<Self::BasePrimeField>, and for<'a> MulAssign<&'a Self::BasePrimeField> bounds too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this initially, but we already have Mul<Self> coming from a macro, and adding a trait bound as you suggest, and then implementing:


impl<P: QuadExtConfig> Mul<<Self as Field>::BasePrimeField> for QuadExtField<P> {
    type Output = Self;
    ...
}

causes a conflicting implementation error.
I believe this is what you meant, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch that's annoying...

I guess Rust doesn't know that for QuadExtField, BasePrimeField is always different from Self...

But it should be able to figure that out: BasePrimeField: PrimeField, while Self doesn't implement that.

@Pratyush
Copy link
Member

Where is the existing multiplication by base field?

@mmagician
Copy link
Member Author

@Pratyush We have a method directly on QuadExtField:

pub fn mul_assign_by_basefield(&mut self, element: &P::BaseField) {

@mmagician
Copy link
Member Author

Actually, we also have the counterpart for CubicExtField, although the names weren't consistent, notice the underscore in base_field:

pub fn mul_assign_by_base_field(&mut self, value: &P::BaseField) {

I would propose to unify these two as part of this PR, at least when it comes to the name. We could pull that method out into a separate trait, but I'm not sure it's worth it (it is not used anywhere in the arkworks org).

@mmagician mmagician requested a review from Pratyush April 6, 2024 09:55
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.

None yet

2 participants