feat: make constraint.Element generic interface#1463
Conversation
|
@gbotrel - ready now. Only thing I'm still thinking, but would require modifying gnark-crypto or bavard is to have option for finite field generator config to force compiling on uint32. Then we could also make |
Yes I guess I took some shortcuts in gnark-crypto and would need to ensure the condition to generate code for koala/babybear is different than the one for fields that are smaller than 32bits . For |
So what I was thinking is to add |
gbotrel
left a comment
gbotrel
left a comment
There was a problem hiding this comment.
great, glad this could work out; minor remarks, and maybe worth it to add U64x4 and U64x6 that would negate any perf changes for most production use cases.
also, not sure we needed to bring in babybear just yet, but since you did it ... :)
|
Feedback addressed imo:
Imo there is two big things we'd need to decide still before merging this PR. First, do we want to split U64 between 4 and 6 element array. I'd prefer not to. Secondly - what to do with We should somehow still address it to avoid any problems in the future. We could either just panic in A bit better solution imo is to use the In the end the result is the same, it is just technical difference (either we panic inside Commit method or we panic as the type assertion doesn't hold). |
There was a problem hiding this comment.
Copilot reviewed 98 out of 98 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
constraint/bls24-317/coeff.go:155
- The field arithmetic functions (e.g. Mul) currently modify the first operand in place. Consider performing the operation on a copy of the input to avoid unexpected side-effects in caller code.
_a.Mul(_a, _b)
constraint/bls12-377/system.go:69
- [nitpick] With the introduction of generics, the blueprint interfaces now include a type parameter (e.g. constraint.BlueprintStateful[constraint.U64]). It would be beneficial to add documentation comments to clarify the new generic usage and its implications for external users.
if b, ok := cs.Blueprints[i].(constraint.BlueprintStateful[constraint.U64]); ok {
Description
With the upcoming support of smallfield fields for compiling the circuits to in conjuction with Linea prover, we need to be able to compile gnark circuits over small fields. gnark-crypto defines small field operations over
[1]uint32instead of[4]uint64and[6]uint64, so we have some incompatibility in marshalling etc.However, the biggest problem is the in-memory size of the compiled circuits - even if we would continue using
[6]uint64for all possible elements and fixing the marshalling, then we would have 12x overhead when defining coefficients. The actual overhead compared with existing circuits would be even bigger, as we need more constraints to represent the same computations (for example when using non-native arithmetic).I have tried to create type aliases so that existing code would compile and run without needing any changes. The only place which has incompatible change is the log-derivate lookup gadget, as previously we returned concrete implementation, and now an interface. As the concrete implementation returned a reference to the lookup table, then we need to dereference when the lookup table type was explicitly defined. I have updated it in gnark, but there may be external users, however the change to fix the interface should be simple (
*logderivlookup.Table--->logderivlookup.Table).Furthermore, it would be nice to change the
tinyfieldimplementation also to useconstraint.U32, as allows to remove some assumptions and hard-coded edge cases.Another thing we could try is to omit code generation for
constraint/package now and use the generic implementation.Type of change
How has this been tested?
Added tests to ensure
constraint.Elementmarshalling and unmarshalling is consistent. Existing tests should run cleanly.How has this been benchmarked?
Macbook, but the variance is high (power saving I guess)
Checklist:
golangci-lintdoes not output errors locally