-
Notifications
You must be signed in to change notification settings - Fork 146
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
Foreign fields 3: Addition #1220
Conversation
function array<T, S>( | ||
spec: ProvableSpec<T, S>, | ||
n: number | ||
): ProvableSpec<T[], S[]>; | ||
function array<T, S>( | ||
spec: Spec<T, S>, | ||
n: Random<number> | number | ||
): Spec<T[], S[]>; |
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.
Are these function declarations?
Why is the second declaration needed (it's the same type as the implementation right below)?
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 how you declare if there are two different function signatures. The third one is not exposed, it's the "internal" signature used in writing the function. So if I remove the second one, consumers only see the first one
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.
some notes for reviewers!
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.
the code for the new gadgets! please review carefully
/** | ||
* Helper methods to interact with 3-limb vectors of Fields. | ||
* | ||
* **Note:** This interface does not contain any provable methods. | ||
*/ | ||
Field3, |
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'm a bit conflicted about this. I sometimes got the feedback that it's confusing how o1js APIs are a mix of provable and non-provable methods. I'm not sure if putting them on a separate sub-namespace of Gadgets
is enough separation. In any case, these helper methods are essential to reduce boiler-plate of interacting with all foreign field and foreign curve gadgets, and I also didn't feel good about another top-level export for them.
export namespace Gadgets { | ||
/** | ||
* A 3-tuple of Fields, representing a 3-limb bigint. | ||
*/ | ||
export type Field3 = [Field, Field, Field]; | ||
} |
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.
note: this is how to get a type namespace alongside a value namespace. It's possible to use the same name if the type namespace is only types. We could've also put everything in a single namespace
, but I prefer the syntax for writing JS objects over the clunky namespace syntax which isn't even real JS (just a TS addition to the language).
/** | ||
* Convenience function to run {@link constraintSystem} on the method of a {@link ZkProgram}. | ||
* | ||
* @example | ||
* ```ts | ||
* const program = ZkProgram({ methods: { myMethod: ... }, ... }); | ||
* | ||
* constraintSystem.fromZkProgram(program, 'myMethod', contains('Rot64')); | ||
* ``` | ||
*/ | ||
constraintSystem.fromZkProgram = function fromZkProgram< |
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 a continuation of #1241. I figured I could make writing constraint system tests for zkprogram methods even easier by reading the input types off the zkprogram.
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.
changes in this file are in support of the new constraintSystem.fromZkProgram()
publicInputType: publicInputType as ProvableOrUndefined< | ||
Get<StatementType, 'publicInput'> | ||
>, | ||
publicOutputType: publicOutputType as ProvableOrVoid< | ||
Get<StatementType, 'publicOutput'> | ||
>, | ||
analyzeMethods, | ||
privateInputTypes: Object.fromEntries( |
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.
exposing the private input types on zkprogram seems consistent, since we already expose the public input types and the methods
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.
What is this exactly for?
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 in support of a testing utility I added: #1220 (comment)
You can ignore it, it's not related to the circuit gadgets added here :D
@@ -925,6 +945,7 @@ ZkProgram.Proof = function < | |||
static tag = () => program; | |||
}; | |||
}; | |||
ExperimentalZkProgram.Proof = ZkProgram.Proof; |
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 was a bug which accidentally broke Experimental.ZkProgram
, fixed it while I was here
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.
additions here are in support of the foreign field unit tests
@@ -307,6 +308,7 @@ const Random = Object.assign(Random_, { | |||
reject, | |||
dice: Object.assign(dice, { ofSize: diceOfSize() }), | |||
field, | |||
otherField: fieldWithInvalid, |
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.
random generator for foreign fields, for tests
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.
Approving with minor comments
export { ForeignField, Field3, Sign }; | ||
|
||
/** | ||
* A 3-tuple of Fields, representing a 3-limb bigint. |
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.
Maybe clarify here what's the highest and lowest limbs?
* | ||
* assumes that inputs are range checked, does range check on the result. | ||
*/ | ||
function sum(x: Field3[], sign: Sign[], f: bigint) { |
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 sum being exposed or just add, sub and sumchain?
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.
sum()
is exposed! it is actually the same as sumchain
in OCaml, I renamed it because I think the "chain" aspect is more about the internal implementation and less important for users
assert(x.length === sign.length + 1, 'inputs and operators match'); | ||
|
||
// constant case | ||
if (x.every((x) => x.every((x) => x.isConstant()))) { |
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.
what's the meaning of this notation =>
?
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 an inline function, called arrow function in JS. in Rust you'd call it a closure
({ result } = singleAdd(result, x[i + 1], sign[i], f)); | ||
} | ||
// final zero row to hold result | ||
Gates.zero(...result); |
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.
But maybe the last gate could be a FFMul0 for example, in case someone wanted to use that output for the left input of a multiplication, no?
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 guess we had to provide some kind of tradeoff between ease-of-use and functionality 💭
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 completely agree, this isn't yet the best version of the sumchain gadget. Wait for the ECDSA PR, there I needed chaining into ffmul and I came up with an API which is both easy to use and flexible
* **warning**: this just adds the `foreignFieldAdd` row; | ||
* it _must_ be chained with a second row that holds the result in its first 3 cells. | ||
* | ||
* the second row could, for example, be `zero`, `foreignFieldMul`, or another `foreignFieldAdd`. |
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.
Oh I see, so you still provide "fine grain" access to this in case the user wants to create a more efficient circuit.
let overflow = 0n; | ||
if (sign === 1n && r >= f) overflow = 1n; | ||
if (sign === -1n && r < 0n) overflow = -1n; | ||
if (f === 0n) overflow = 0n; // special case where overflow doesn't change anything |
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 would we like to support foreign field zero? Is that even a thing?
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.
yes I think it could be useful if you want to do plain bigint addition (not modulo a field)
// note: this "just works" with negative r01 | ||
let r01 = collapse2(x_) + sign * collapse2(y_) - overflow * collapse2(f_); | ||
let carry = r01 >> twoL; | ||
r01 &= twoLMask; |
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.
what's twoLMask
?
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.
Oh this is 2^176 - 1 perhaps?
@@ -53,6 +61,7 @@ function rangeCheck64(x: Field) { | |||
const L = 88n; | |||
const twoL = 2n * L; | |||
const lMask = (1n << L) - 1n; | |||
const twoLMask = (1n << twoL) - 1n; |
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.
Oh it is indeed 😄
publicInputType: publicInputType as ProvableOrUndefined< | ||
Get<StatementType, 'publicInput'> | ||
>, | ||
publicOutputType: publicOutputType as ProvableOrVoid< | ||
Get<StatementType, 'publicOutput'> | ||
>, | ||
analyzeMethods, | ||
privateInputTypes: Object.fromEntries( |
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.
What is this exactly for?
let specialField = oneOf(0n, 1n, F(-1)); | ||
let field = oneOf<bigint[]>(randomField, randomField, uint64, specialField); | ||
let specialField = oneOf(0n, 1n, F.negate(1n)); | ||
let roughLogSize = 1 << Math.ceil(Math.log2(F.sizeInBits) - 1); |
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 are you using this size?
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.
because the biguint
random generator I'm passing it in needs the bit size to be a power of 2 (for efficiency)
src/lib/gadgets/foreign-field.ts
Outdated
function toField3(x: bigint3): Field3 { | ||
return Tuple.map(x, (x) => new Field(x)); | ||
} | ||
function bigint3(x: Field3): bigint3 { |
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 would prefer if we could name this toBigint3
, similar to toField3
for consistency
ForeignField
gadget namespace with methods for addition, subtraction and general sums using non-native modular arithmetic.constraintSystem()
test runner introduced in Foreign fields 2: Test DSL to detect broken gate chains #1241 so that it becomes very easy to check methods of a zkprogramcloses #1152
closes #1180
depends on bindings PRs o1-labs/o1js-bindings#203 and o1-labs/o1js-bindings#205
Proof that we can save range checks in
sum()
An optimization used here is to not range-check intermediate results in addition chains of length > 2, i.e. what is exposed as
sum()
. The reasoning why this is sound is as follows:Assume we are doing an addition chain over summands$a_0,\ldots,a_n$ . Let's denote the lower input limbs by $a_i^{(01)}$ and the high limb by $a_i^{(2)}$ . Each FFadd gate individually proves that
where$o_i$ and $c_i$ are overflow and carry, and $n$ is the native modulus. Since addition modulo $n$ is associative, a full chain of ffadd gates proves the following equations about the final output $r$ :
Since we assume the inputs$a_i$ to be range-checked, and both $o_i$ and $c_i$ are 0 or 1 or -1, and the final output $r$ is range-checked as well, the difference of left and right hand side is too small to overflow the native field:
This is enough to conclude that$LHS = RHS$ over the integers. Which in this case just means that we are doing correct addition or subtraction modulo the foreign field.