-
Notifications
You must be signed in to change notification settings - Fork 59
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
Implement pow-by-constant with NAF for FpVar
#72
base: master
Are you sure you want to change the base?
Conversation
The need to support zero bases for a non-zero exp when using NAF has something to do with Poseidon. It is very possible for Poseidon to be powering "0" to alpha. If alpha has a NAF form that contains "-1", it should not make the constraint system unsatisfiable, which needs the |
The zero-base zero-pow issue is also discussed in arkworks-rs/algebra#303. If we decide that 0^0 = 1, then this PR can also be simplified somehow (note, only the beginning part). |
use ark_ff::biginteger::arithmetic::find_wnaf; | ||
|
||
// first check if exp = 0 | ||
let mut is_nonzero = false; |
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.
Is this something we can move to a function in utils?
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.
This is tricky since that the optimization here is very specific to constraints (e.g., count inversion at a cost of 3 constraints).
let standard_cost = | ||
standard_be_bits.len() + standard_be_bits.iter().filter(|x| **x == true).count(); |
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.
Does it make sense to make a pow_by_const trait, with two impls, but eventually three, naf, standard, and windowed?
Then we can have four methods, pow
, pow_r1cs
, pow_native_cost
, and pow_r1cs_cost
?
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.
Or does that seem like a premature abstraction? If so, I at least recommend splitting out the pow
and r1cs_cost
into separate functions for code readability & reuse in the future.
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 the idea. It seems better to have a separate function that simply returns what is the best sequence, and then this function in fp/mod should just execute it.
I agree and will make the change. Let me think about where to put this function since it is naturally r1cs...
(Note: later it might also be used for group elements)
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.
Let me try to split it and will keep you posted.
Description
Warning: this PR is more complicated than one may expect.
This PR changes
FpVar
's implementation ofpow_by_constant
to a special version that uses NAF.However, there are several subtle issues.
First, for correctness and soundness,
base
= 0,exp
!= 0, the result should be zero, and the constraint system should be satisfied. This means that one cannot simply useinverse
to compute the inverse of the base, as it may create an unsatisfiable constraint.base
= 0,exp
= 0, there should be panic, and the constraint system is guaranteed to be unsatisfied.Second, for efficiency,
closes: #55
Note that currently
algebra
'spow
does not panic whenbase
= 0 andexp
= 0, which will be handled in a separate PR.This PR is associated with four tests.
For review, this PR will need some effort, as it appears to be quite complicated.
If you find any good ideas to simplify the code, please propose. The current code is a little bit longer than expected (due to the handling of soundness in respect to the corner cases).
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