-
Notifications
You must be signed in to change notification settings - Fork 118
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
Implements AND gadget #1193
Implements AND gadget #1193
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.
Ok! I added some questions and tried to bring attention to anything I was unsure of!
Sorry that it took me a little while to get this out! I had to split attention between several things last week, but I hope it looks ok, and I enjoyed working on it! :)
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.
Very well done! Please, make sure to read the comments I left, as I believe they can be useful to understand a bit of how kimchi works behind the scenes 👍🏻
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.
Great job, Jack! I will continue with the review tomorrow, but here are a few comments to get started :D
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.
Awesome work Jack! I have some feedback
src/lib/gates.ts
Outdated
@@ -81,6 +81,22 @@ function xor( | |||
); | |||
} | |||
|
|||
/** | |||
* Generic gate |
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.
It's great that you expose this, but I think it could use a bit of polishing:
- instead of
FieldConst
, this should takebigint
and convert them so that it's more natural to use from o1js (I'd considerFieldConst
a purely internal type which exists for OCaml compatibility, but really represents a bigint modulo the field size) - The doccomment should say what constraint is added by the generic gate:
sl*l + sr*r + so*o + sm*l*r + sc === 0
- We should make the function signature as unconfusing as possible. Currently, it's 8 inputs only documented by 1- or 2-letter vars, which is just unreadable (see the code where you used this gate).
Suggestion: You could make the input consist of two separate objects: the constant coefficients/selectors, and the input variables. Then in each of those objects the keys provide a degree of self-explanation
function generic(
coefficients: { left: bigint, right: bigint, out: bigint, mul: bigint, const: bigint },
inputs: { left: Field, right: Field, out: Field }
)
This is just a suggestion, feel free to modify to your preference - the main point is that the function signature should be easy to use
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 this should be ready to merge! Once again, I'm sorry for sporadic work/communication.
Let me know if there is anything else I should change and I will do so right after I wake up tomorrow! :)
generic( | ||
sl: FieldConst, | ||
l: FieldVar, | ||
sr: FieldConst, | ||
r: FieldVar, | ||
so: FieldConst, | ||
o: FieldVar, | ||
sm: FieldConst, | ||
sc: FieldConst | ||
): void; |
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 made the generic interface more friendly in TS and left the OCaml side as is (so that everything matches basic
).
Let me know if you would like me to change anything more (e.g., have OCaml use the same interface, or just rename these variables).
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's perfect!
); | ||
|
||
// compute values for gate | ||
// explanation: https://o1-labs.github.io/proof-systems/specs/kimchi.html?highlight=gates#and |
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.
Should this line be removed?
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's valuable!
* [Generic gate](https://o1-labs.github.io/proof-systems/specs/kimchi.html?highlight=foreignfield#double-generic-gate) | ||
* The vanilla PLONK gate that allows us to do operations like: | ||
* * addition of two registers (into an output register) | ||
* * multiplication of two registers | ||
* * equality of a register with a constant | ||
* | ||
* More generally, the generic gate controls the coefficients (denoted `c_`) in the equation: |
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.
nice description 💯
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 ✅
Summary
This PR adds the implementation for the AND gadget to the existing Gadgets namespace, which behaves similarly to the
&
operator in JavaScript.Context
The AND gadget does not require a new custom gate. Instead, it uses the XOR gate and one generic gate. See OCaml implementation for context.
Tested
This PR adds e2e tests, unit tests, and verification key tests for the AND gate.
Basic gate PR in o1js (required for this PR): o1-labs/o1js-bindings#190
closes: #1140