-
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
Foreign fields 2: Test DSL to detect broken gate chains #1241
Changes from all commits
d98e3a2
1e5919b
4605ad2
4f68250
c6c4f97
d17d47f
ff73b23
657d28f
1e4a1a0
dfffeda
2604f54
fb5ad69
ad137ed
f4a0bfe
9759938
82e5d28
3e0fabb
015d29f
5883f5e
bd35877
2a95ca8
4b400e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,13 +78,22 @@ const FieldVar = { | |
isConstant(x: FieldVar): x is ConstantFieldVar { | ||
return x[0] === FieldType.Constant; | ||
}, | ||
// TODO: handle (special) constants | ||
add(x: FieldVar, y: FieldVar): FieldVar { | ||
if (FieldVar.isConstant(x) && x[1][1] === 0n) return y; | ||
if (FieldVar.isConstant(y) && y[1][1] === 0n) return x; | ||
if (FieldVar.isConstant(x) && FieldVar.isConstant(y)) { | ||
return FieldVar.constant(Fp.add(x[1][1], y[1][1])); | ||
} | ||
return [FieldType.Add, x, y]; | ||
}, | ||
// TODO: handle (special) constants | ||
scale(c: FieldConst, x: FieldVar): FieldVar { | ||
return [FieldType.Scale, c, x]; | ||
scale(c: bigint | FieldConst, x: FieldVar): FieldVar { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a note from our discussion of this code. Remember to do the "collapsing" of |
||
let c0 = typeof c === 'bigint' ? FieldConst.fromBigint(c) : c; | ||
if (c0[1] === 0n) return FieldVar.constant(0n); | ||
if (c0[1] === 1n) return x; | ||
if (FieldVar.isConstant(x)) { | ||
return FieldVar.constant(Fp.mul(c0[1], x[1][1])); | ||
} | ||
return [FieldType.Scale, c0, x]; | ||
}, | ||
[0]: [FieldType.Constant, FieldConst[0]] satisfies ConstantFieldVar, | ||
[1]: [FieldType.Constant, FieldConst[1]] satisfies ConstantFieldVar, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ import { | |
witnessSlice, | ||
witnessNextValue, | ||
divideWithRemainder, | ||
toVar, | ||
} from './common.js'; | ||
import { rangeCheck64 } from './range-check.js'; | ||
|
||
|
@@ -37,18 +38,12 @@ function not(a: Field, length: number, checked: boolean = false) { | |
} | ||
|
||
// create a bitmask with all ones | ||
let allOnesF = new Field(2n ** BigInt(length) - 1n); | ||
|
||
let allOnes = Provable.witness(Field, () => { | ||
return allOnesF; | ||
}); | ||
|
||
allOnesF.assertEquals(allOnes); | ||
let allOnes = new Field(2n ** BigInt(length) - 1n); | ||
Comment on lines
-40
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. defensive logic like this is no longer necessary - we can easily check that the resulting gate layout is correct |
||
|
||
if (checked) { | ||
return xor(a, allOnes, length); | ||
} else { | ||
return allOnes.sub(a); | ||
return allOnes.sub(a).seal(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes sure that the constraint created by the subtraction is added at this point. In my tests I found that the constraint system created by unchecked It was empty because additions and subtractions are treated as a form of lazy constraint / AST, which are only turned into an actual constraint once the result is included in some multiplication or assertion or custom gate.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The snarky version of |
||
} | ||
} | ||
|
||
|
@@ -252,6 +247,10 @@ function rot( | |
} | ||
); | ||
|
||
// flush zero var to prevent broken gate chain (zero is used in rangeCheck64) | ||
// TODO this is an abstraction leak, but not clear to me how to improve | ||
toVar(0n); | ||
Comment on lines
+250
to
+252
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the kind of thing that is impossible to catch without tests There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In OCaml we had a PR where whenever you add a gate it adds a "shadow row" that specifies what the next row should be with Some or None (for anything), per cell in the next row. I guess this PR is about something similar. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We also had some tests which added an even/odd number of generic operations before calling the actual gadget so that in case the gadget was creating some "orphan" generic which was coincidentally correctly pushed to the end of the chain, we could catch it and fix the gadget accordingly. Do you think this could make sense in your tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @querolita I think my tests already exhibit that case, because I am creating random linear combinations of variables as inputs, something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That sounds interesting! Do you know where that PR is? |
||
|
||
// Compute current row | ||
Gates.rotate( | ||
field, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,16 @@ import { Fp, mod } from '../../bindings/crypto/finite_field.js'; | |
import { Field } from '../core.js'; | ||
import { Gadgets } from './gadgets.js'; | ||
import { Random } from '../testing/property.js'; | ||
import { | ||
constraintSystem, | ||
contains, | ||
equals, | ||
ifNotAllConstant, | ||
repeat, | ||
and, | ||
withoutGenerics, | ||
} from '../testing/constraint-system.js'; | ||
import { GateType } from '../../snarky.js'; | ||
|
||
const maybeField = { | ||
...field, | ||
|
@@ -183,3 +193,63 @@ await equivalentAsync({ from: [field], to: field }, { runs: 3 })( | |
return proof.publicOutput; | ||
} | ||
); | ||
|
||
// check that gate chains stay intact | ||
|
||
function xorChain(bits: number) { | ||
return repeat(Math.ceil(bits / 16), 'Xor16').concat('Zero'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice |
||
} | ||
|
||
constraintSystem( | ||
'xor', | ||
{ from: [Field, Field] }, | ||
Bitwise.rawMethods.xor, | ||
ifNotAllConstant(contains(xorChain(254))) | ||
); | ||
|
||
constraintSystem( | ||
'not checked', | ||
{ from: [Field] }, | ||
Bitwise.rawMethods.notChecked, | ||
ifNotAllConstant(contains(xorChain(254))) | ||
); | ||
|
||
constraintSystem( | ||
'not unchecked', | ||
{ from: [Field] }, | ||
Bitwise.rawMethods.notUnchecked, | ||
ifNotAllConstant(contains('Generic')) | ||
); | ||
|
||
constraintSystem( | ||
'and', | ||
{ from: [Field, Field] }, | ||
Bitwise.rawMethods.and, | ||
ifNotAllConstant(contains(xorChain(64))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you only supporting And64 then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, this tests a specific example circuit -- the |
||
); | ||
|
||
let rotChain: GateType[] = ['Rot64', 'RangeCheck0', 'RangeCheck0']; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, but remember to have copy constraints to the RangeCheck0's to constant zero for the two most significant sublimbs to make it 64-bit instead of 88-bit. |
||
let isJustRotate = ifNotAllConstant( | ||
and(contains(rotChain), withoutGenerics(equals(rotChain))) | ||
); | ||
|
||
constraintSystem( | ||
'rotate', | ||
{ from: [Field] }, | ||
Bitwise.rawMethods.rot, | ||
isJustRotate | ||
); | ||
|
||
constraintSystem( | ||
'left shift', | ||
{ from: [Field] }, | ||
Bitwise.rawMethods.leftShift, | ||
isJustRotate | ||
); | ||
|
||
constraintSystem( | ||
'right shift', | ||
{ from: [Field] }, | ||
Bitwise.rawMethods.rightShift, | ||
isJustRotate | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
import { Provable } from '../provable.js'; | ||
import { Field, FieldConst } from '../field.js'; | ||
import { TupleN } from '../util/types.js'; | ||
import { Field, FieldConst, FieldVar, FieldType } from '../field.js'; | ||
import { Tuple, TupleN } from '../util/types.js'; | ||
import { Snarky } from '../../snarky.js'; | ||
import { MlArray } from '../ml/base.js'; | ||
|
||
|
@@ -9,13 +9,21 @@ const MAX_BITS = 64 as const; | |
export { | ||
MAX_BITS, | ||
exists, | ||
existsOne, | ||
toVars, | ||
toVar, | ||
assert, | ||
bitSlice, | ||
witnessSlice, | ||
witnessNextValue, | ||
divideWithRemainder, | ||
}; | ||
|
||
function existsOne(compute: () => bigint) { | ||
let varMl = Snarky.existsVar(() => FieldConst.fromBigint(compute())); | ||
return new Field(varMl); | ||
} | ||
|
||
function exists<N extends number, C extends () => TupleN<bigint, N>>( | ||
n: N, | ||
compute: C | ||
|
@@ -27,6 +35,31 @@ function exists<N extends number, C extends () => TupleN<bigint, N>>( | |
return TupleN.fromArray(n, vars); | ||
} | ||
|
||
/** | ||
* Given a Field, collapse its AST to a pure Var. See {@link FieldVar}. | ||
* | ||
* This is useful to prevent rogue Generic gates added in the middle of gate chains, | ||
* which are caused by snarky auto-resolving constants, adds and scales to vars. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is really awesome |
||
* | ||
* Same as `Field.seal()` with the difference that `seal()` leaves constants as is. | ||
*/ | ||
function toVar(x: Field | bigint) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this is better called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's not just for constants though! |
||
// don't change existing vars | ||
if (x instanceof Field && x.value[1] === FieldType.Var) return x; | ||
let xVar = existsOne(() => Field.from(x).toBigInt()); | ||
xVar.assertEquals(x); | ||
return xVar; | ||
} | ||
|
||
/** | ||
* Apply {@link toVar} to each element of a tuple. | ||
*/ | ||
function toVars<T extends Tuple<Field | bigint>>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
fields: T | ||
): { [k in keyof T]: Field } { | ||
return Tuple.map(fields, toVar); | ||
} | ||
|
||
function assert(stmt: boolean, message?: string) { | ||
if (!stmt) { | ||
throw Error(message ?? 'Assertion failed'); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
import { Field } from '../field.js'; | ||
import * as Gates from '../gates.js'; | ||
import { bitSlice, exists } from './common.js'; | ||
import { bitSlice, exists, toVar, toVars } from './common.js'; | ||
|
||
export { rangeCheck64, multiRangeCheck, compactMultiRangeCheck, L }; | ||
|
||
|
@@ -64,10 +64,13 @@ function multiRangeCheck([x, y, z]: [Field, Field, Field]) { | |
} | ||
return; | ||
} | ||
// ensure we are using pure variables | ||
[x, y, z] = toVars([x, y, z]); | ||
let zero = toVar(0n); | ||
Comment on lines
+67
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is what fixed the multi-range check gadget |
||
|
||
let [x64, x76] = rangeCheck0Helper(x); | ||
let [y64, y76] = rangeCheck0Helper(y); | ||
rangeCheck1Helper({ x64, x76, y64, y76, z, yz: new Field(0) }); | ||
rangeCheck1Helper({ x64, x76, y64, y76, z, yz: zero }); | ||
} | ||
|
||
/** | ||
|
@@ -88,6 +91,8 @@ function compactMultiRangeCheck(xy: Field, z: Field): [Field, Field, Field] { | |
let [x, y] = splitCompactLimb(xy.toBigInt()); | ||
return [new Field(x), new Field(y), z]; | ||
} | ||
// ensure we are using pure variables | ||
[xy, z] = toVars([xy, z]); | ||
Comment on lines
+94
to
+95
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fix for the compact range check gadget |
||
|
||
let [x, y] = exists(2, () => splitCompactLimb(xy.toBigInt())); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,23 @@ | ||
import type { Gate } from '../../snarky.js'; | ||
import { mod } from '../../bindings/crypto/finite_field.js'; | ||
import { Field } from '../../lib/core.js'; | ||
import { ZkProgram } from '../proof_system.js'; | ||
import { Provable } from '../provable.js'; | ||
import { | ||
Spec, | ||
boolean, | ||
equivalentAsync, | ||
fieldWithRng, | ||
} from '../testing/equivalent.js'; | ||
import { Random } from '../testing/property.js'; | ||
import { assert, exists } from './common.js'; | ||
import { assert } from './common.js'; | ||
import { Gadgets } from './gadgets.js'; | ||
import { L } from './range-check.js'; | ||
import { expect } from 'expect'; | ||
import { | ||
constraintSystem, | ||
contains, | ||
equals, | ||
ifNotAllConstant, | ||
withoutGenerics, | ||
} from '../testing/constraint-system.js'; | ||
|
||
let uint = (n: number | bigint): Spec<bigint, Field> => { | ||
let uint = Random.bignat((1n << BigInt(n)) - 1n); | ||
|
@@ -29,29 +33,30 @@ let maybeUint = (n: number | bigint): Spec<bigint, Field> => { | |
|
||
// constraint system sanity check | ||
|
||
function csWithoutGenerics(gates: Gate[]) { | ||
return gates.map((g) => g.type).filter((type) => type !== 'Generic'); | ||
} | ||
|
||
let check64 = Provable.constraintSystem(() => { | ||
let [x] = exists(1, () => [0n]); | ||
Gadgets.rangeCheck64(x); | ||
}); | ||
let multi = Provable.constraintSystem(() => { | ||
let x = exists(3, () => [0n, 0n, 0n]); | ||
Gadgets.multiRangeCheck(x); | ||
}); | ||
let compact = Provable.constraintSystem(() => { | ||
let [xy, z] = exists(2, () => [0n, 0n]); | ||
Gadgets.compactMultiRangeCheck(xy, z); | ||
}); | ||
constraintSystem( | ||
'range check 64', | ||
{ from: [Field] }, | ||
Gadgets.rangeCheck64, | ||
ifNotAllConstant(withoutGenerics(equals(['RangeCheck0']))) | ||
); | ||
|
||
let expectedLayout64 = ['RangeCheck0']; | ||
let expectedLayoutMulti = ['RangeCheck0', 'RangeCheck0', 'RangeCheck1', 'Zero']; | ||
constraintSystem( | ||
'multi-range check', | ||
{ from: [Field, Field, Field] }, | ||
(x, y, z) => Gadgets.multiRangeCheck([x, y, z]), | ||
ifNotAllConstant( | ||
contains(['RangeCheck0', 'RangeCheck0', 'RangeCheck1', 'Zero']) | ||
) | ||
); | ||
|
||
expect(csWithoutGenerics(check64.gates)).toEqual(expectedLayout64); | ||
expect(csWithoutGenerics(multi.gates)).toEqual(expectedLayoutMulti); | ||
expect(csWithoutGenerics(compact.gates)).toEqual(expectedLayoutMulti); | ||
constraintSystem( | ||
'compact multi-range check', | ||
{ from: [Field, Field] }, | ||
Gadgets.compactMultiRangeCheck, | ||
ifNotAllConstant( | ||
contains(['RangeCheck0', 'RangeCheck0', 'RangeCheck1', 'Zero']) | ||
) | ||
); | ||
|
||
Comment on lines
+52
to
+59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removing ad-hoc tests which didn't catch the bugs in favor of comprehensive tests using the DSL |
||
// TODO: make a ZkFunction or something that doesn't go through Pickles | ||
// -------------------------- | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -271,6 +271,13 @@ function ZkProgram< | |
analyzeMethods: () => ReturnType<typeof analyzeMethod>[]; | ||
publicInputType: ProvableOrUndefined<Get<StatementType, 'publicInput'>>; | ||
publicOutputType: ProvableOrVoid<Get<StatementType, 'publicOutput'>>; | ||
rawMethods: { | ||
[I in keyof Types]: Method< | ||
InferProvableOrUndefined<Get<StatementType, 'publicInput'>>, | ||
InferProvableOrVoid<Get<StatementType, 'publicOutput'>>, | ||
Types[I] | ||
>['method']; | ||
}; | ||
} & { | ||
[I in keyof Types]: Prover< | ||
InferProvableOrUndefined<Get<StatementType, 'publicInput'>>, | ||
|
@@ -427,6 +434,9 @@ function ZkProgram< | |
Get<StatementType, 'publicOutput'> | ||
>, | ||
analyzeMethods, | ||
rawMethods: Object.fromEntries( | ||
Object.entries(methods).map(([k, v]) => [k, v.method]) | ||
) as any, | ||
Comment on lines
+437
to
+439
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can now access the original zkprogram methods (not wrapped to return proofs) on |
||
}, | ||
provers | ||
); | ||
|
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.
If you do these types of checks often, perhaps you want to create a
FieldVar.isZeroConst(x)
helper for brevity.