-
Notifications
You must be signed in to change notification settings - Fork 158
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
Feat/iop arguments #282
Feat/iop arguments #282
Conversation
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.
looks good, didn't really check the correctness of test, but one thing that bother me is the Form
metadata associated with a Polynomial
and the way the conversions are implemented / tested, I think we can do better to ensure consistency / safety of the APIs.
ecc/bls12-377/fr/iop/polynomial.go
Outdated
// univariate polynomials | ||
|
||
// Enum to tell in which basis a polynomial is represented. | ||
type Basis int64 |
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 enums may find a better home in a dedicated polynomial
package?
If we consider a package importing fr/iop
, they will read as iop.Canonical
iop.Regular
iop.BitReverse
iop.Locked
, ... which is not super clear.
From what I've seen in the PR so far, I'ld suggest (but maybe I'm wrong) to do a more "private enum" like so:
const ( // iota is reset to 0
canonical = iota // canonical == 0
lagrange = iota // lagrange == 1
lagrangeCoset = iota // lagrangeCoset == 2
)
And have methods defined on the Polynomial
(if multiple Polynomial
needs it make it an interface to implement) like IsCanonical() bool
etc. Will be inlined by the compiler, and it makes future changes easier to refactor without user impact.
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 agree on the polynomial package, it's just that currently on Arya's branch for gkr an existing polynomial package is already used, but the plan is to eventually replace it. I'll talk with Arya about that
ecc/bls12-377/fr/iop/polynomial.go
Outdated
} | ||
|
||
// it is supposed that the number of variables matches | ||
func (m monomial) evaluate(x []fr.Element) fr.Element { |
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.
why is this not pointer receiver? m *monomial
would avoid the copy of m.coeff
at each call.
) | ||
|
||
// Form describes the form of a polynomial. | ||
type Form struct { |
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.
would add methods here form.SetBasis()
form.Lock()
.. form.Unlock()
. Then in case debugging is needed, it's easy to add some (temporary or build tag enabled) consistency checks in these APIs, rather than changing the state through polynomial.info.status = lock
ecc/bls12-377/fr/iop/quotient.go
Outdated
if err != nil { | ||
return quotientLagrangeCosetBitReverse, nil | ||
} | ||
|
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.
add a check that domains[...] are correct; for example that domains[n].Cardinality != 0 ?
ecc/bls12-377/fr/iop/utils.go
Outdated
fmt.Printf("]\n") | ||
} | ||
|
||
func printLayout(f Form) { |
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.
add a String()
method to the enums and a String()
method to the Form
type
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.
Yeah those were for debugging that I forgot to delete
ecc/bls12-377/fr/iop/utils.go
Outdated
type modifier func(p *Polynomial, d *fft.Domain) *Polynomial | ||
|
||
// return a copy of p | ||
func copyPoly(p Polynomial) Polynomial { |
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.
add Polynomial.Clone() Polynomial
method?
|
||
// doesn't return any errors, it is a private method, that | ||
// is assumed to be called with correct arguments. | ||
func smallExp(x fr.Element, n int) fr.Element { |
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.
unused?
if used, I think it's less verbose to init 5 big int to avoid many allocations, do a switch on n and call fr.Element.Exp(.., bigINt), perf should be OK.
ecc/bls12-377/fr/iop/utils.go
Outdated
return int(p.Info.Basis)*4 + int(p.Info.Layout)*2 + int(p.Info.Status) | ||
} | ||
|
||
//---------------------------------------------------- |
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.
mmhhh I don't like that part it doesn't seem really robust to changes;
I think the combinatorial is not too crazy and this can be factored in a single method;
isLocked := p.IsLocked()
if isLocked {
p = p.Clone()
}
if targetBasis != p.Basis {
// ...
}
at the minimum, would be nice to have some compiler-enforced consistency checks / safety.
This PR incorporates the routines which are commonly used in the backend of a 'plonk-like iop', namely:
The functions previously described are already hardcoded in gnark in the plonk backend, but hardcoded and not really reusable. The goal here is to make those functions as generic as possible while keeping the efficient we get when they are hardcoded.
To this end, a new polynomial package has been created where it describes a polynomial as a slice of []fr.Element as we implicitly do, but the structure describing the polynomial also contains:
In view of developing custom gates, a simple structure has also been built to support multivariate polynomials. The structure is just a list of monomials which are composed of a coeff + a list of exponents.