-
Notifications
You must be signed in to change notification settings - Fork 61
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 the need for extraneous generics in downstream code. #83
base: master
Are you sure you want to change the base?
Conversation
is_in_the_normal_form, | ||
target_phantom: PhantomData, | ||
}; | ||
Reducer::<TargetField, BaseField>::post_add_reduce(&mut result)?; |
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.
@weikengchen is this okay? I'm using a single reduction to reduce many additions.
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 think it is better to invoke the existing API add
many times, which, in most cases, also does not introduce constraints.
There are two issues of the change above:
num_of_additions_over_normal_form
seems to be reset to a low number. But maybe we should sum together all thefirst.num_of_additions_over_normal_form
and get the sum as the new element'snum_of_additions_over_normal_form
. Note thatpost_add_reduce
does not always reduce---it only reduces in the extreme cases.post_add_reduce
should better be executed after each addition, in case intermediate additions cause overflow.
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.
in other words, post_add_reduce
are cheap.
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.
num_of_additions_over_normal_form
seems to be reset to a low number. But maybe we should sum together all thefirst.num_of_additions_over_normal_form
and get the sum as the new element'snum_of_additions_over_normal_form
. Note thatpost_add_reduce
does not always reduce---it only reduces in the extreme cases.
Good point, I will sum over the additions too.
post_add_reduce
should better be executed after each addition, in case intermediate additions cause overflow.
Is there a formula for that uses num_of_additions_over_normal_form
to tell me whether a reduction is necessary or not?
Basically I'm trying to avoid generating large lc
s.
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 got your points.
Yes, the following code sets a rule. Computing this surfeit every time may be cumbersome, but it may be the more straightforward approach for this PR.
r1cs-std/src/fields/nonnative/reduce.rs
Line 138 in a2d2d47
let surfeit = overhead!(elem.num_of_additions_over_normal_form + BaseField::one()) + 1; |
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.
Ok so in the latest commit I extract that check into a common function, and now invoke it every iteration to see if a reduction is needed or not. If it is, then I invoke reduce, add this intermediate_result to a list of intermediate_results, which I manually add using the add
operator.
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 see. The new implementation is reasonable. Basically if limbs_iter
is too heavy, we sum everything in the limbs_iter
into a field element that we push to intermediate_result
, which will be handled later.
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.
Did a pass of all the code. Luckily most are refactoring.
And remember to add a CHANGELOG entry. |
due to circular dev-dependencies
to be merged only after curves crates are released
Description
Adds two main APIs:
trait FieldWithVar: Field { type Var: FieldVar<Self, Self::BasePrimeField> }
trait CurveWithVar<ConstraintF>: ProjectiveCurve { type Var: CurveVar<Self, ConstraintF> }
There are refactors to make this work. Primarily: we reimplement everything in terms of
*_assign
operators to prevent too many bounds everywhere.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