-
Notifications
You must be signed in to change notification settings - Fork 134
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
Activate custom gates in JS and add rangeCheck64 gadget #1176
Conversation
…names duplicated :/
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 great to me! 🎉
lookup: false, | ||
runtimeTables: false, | ||
}; | ||
for (let gate of gates) { |
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.
nit: This is a superficial suggestion, but you could add some type safety here instead of the switch statement
const gateTypeToFlagKey: Record<GateType, keyof FeatureFlags> = {
RangeCheck0: 'rangeCheck0',
RangeCheck1: 'rangeCheck1',
ForeignFieldAdd: 'foreignFieldAdd',
ForeignFieldMul: 'foreignFieldMul',
Xor16: 'xor',
Rot64: 'rot',
Lookup: 'lookup',
...
};
for (let gate of gates) {
const flagKey = gateTypeToFlagKey[gate.type];
if (flagKey) {
flags[flagKey] = true;
}
};
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.
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 went with the object approach. This is the type signature you need to encode that some but not all gate types cause a feature flag to be set (same as the switch statement with the default branch):
const gateToFlag: Partial<Record<GateType, keyof FeatureFlags>> = {
RangeCheck0: 'rangeCheck0',
RangeCheck1: 'rangeCheck1',
// ...
x.value, | ||
[0, FieldVar[0], FieldVar[0], x52, x40, x28, x16], | ||
[0, x14, x12, x10, x8, x6, x4, x2, x0], | ||
// not using compact mode |
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.
// not using compact mode
why not? 😅
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 just a different "mode" of the gate which is used in specific circumstances (when combining this with other gates to do a 2x88 + 1x88-bit range check where the 2x88-bit value is what is called "compact")
closes #1172
bindings: o1-labs/o1js-bindings#179
This PR
=> makes Kimchi verification work for proofs that use custom gates
Pickles.compile
based on the gates used in a circuit=> makes Pickles proofs work with custom gates
rangeCheck0
gate to JS, and wraps it in a 64-bit range check gadgetUInt64
, which is very well covered by existing testsTODOs:
UInt64
again, or move it behind a customization flagUPDATE
Following internal discussion, I didn't use the
rangeCheck64
gate inUInt64
for now. Thus, there is no more test in this PR that everything works (but CI for a previous commit shows it working)However, there is now a follow-up PR which exposes the new gate directly (for advanced users), and also adds a unit test: #1181