-
Notifications
You must be signed in to change notification settings - Fork 66
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
Let Radix2Domain::offset
to be FpVar
instead of F
#65
Conversation
The change looks good. Although this is a breaking change, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for review! fixed 2 of 3
will wait for @ValarDragon for a final review |
/// Defines an evaluation domain over a prime field. The domain is a coset of size `1<<dim`. | ||
/// | ||
/// Native code corresponds to `ark-poly::univariate::domain::radix2`, but `ark-poly` only supports | ||
/// subgroup for now. | ||
/// | ||
/// TODO: support cosets in `ark-poly`. | ||
pub struct Radix2Domain<F: PrimeField> { | ||
pub struct Radix2DomainVar<F: PrimeField> { | ||
/// generator of subgroup g | ||
pub gen: F, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we plan to make this FpVar in a future release as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some preliminary attempts. It is possible but would require a lot of changes. I guess we would leave it to the next release.
There are some potential use cases. For example, a Marlin gadget verifies proofs of varying domain sizes, e.g., if used as layer-2 of ZEXE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I will put this on my stack, and will probably do so after I finish up FRI protocol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a suggestion for one minor function rename
* restructure code * done * add changelog * add the changelog to mark this as a breaking change * add the CHANGELOG * tweak * add `EqGadget` * rename generate_interpolate_cache to generate_interpolation_cache * address the comment Co-authored-by: weikeng <[email protected]>
Description
Current
Radix2Domain
does not support non-constant offset. This PR adds that functionality.Highlighted Changes:
Radix2Domain
will be renamed toRadix2DomainVar
Radix2DomainVar
will offset that isFpVar
instead ofF
Radix2DomainVar::interpolate_and_evaluate
will specialize according to whetherself.offset
is constant or not.closes: #64
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.
Pending
section inCHANGELOG.md
Files changed
in the Github PR explorer