Skip to content
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

Merged
merged 22 commits into from
Nov 16, 2023
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,17 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

## [Unreleased](https://github.com/o1-labs/o1js/compare/26363465d...HEAD)

> No unreleased changes yet
### Added

- Comprehensive internal testing of constraint system layouts generated by new gadgets https://github.com/o1-labs/o1js/pull/1241

## [0.14.1](https://github.com/o1-labs/o1js/compare/e8e7510e1...26363465d)

### Added

- `Gadgets.not()`, new provable method to support bitwise shifting for native field elements. https://github.com/o1-labs/o1js/pull/1198
- `Gadgets.leftShift() / Gadgets.rightShift()`, new provable method to support bitwise shifting for native field elements. https://github.com/o1-labs/o1js/pull/1194
- `Gadgets.and()`, new provable method to support bitwise and for native field elements. https://github.com/o1-labs/o1js/pull/1193
- `Gadgets.not()`, new provable method to support bitwise not. https://github.com/o1-labs/o1js/pull/1198
- `Gadgets.leftShift() / Gadgets.rightShift()`, new provable methods to support bitwise shifting. https://github.com/o1-labs/o1js/pull/1194
- `Gadgets.and()`, new provable method to support bitwise and. https://github.com/o1-labs/o1js/pull/1193
- `Gadgets.multiRangeCheck()` and `Gadgets.compactMultiRangeCheck()`, two building blocks for non-native arithmetic with bigints of size up to 264 bits. https://github.com/o1-labs/o1js/pull/1216

## [0.14.0](https://github.com/o1-labs/o1js/compare/045faa7...e8e7510e1)
Expand Down
2 changes: 1 addition & 1 deletion src/bindings
17 changes: 13 additions & 4 deletions src/lib/field.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link

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.

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 {
Copy link

Choose a reason for hiding this comment

The 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 scale that the OCaml code does, i.e. Scale a (Scale b x) becomes Scale a*b x

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,
Expand Down
15 changes: 7 additions & 8 deletions src/lib/gadgets/bitwise.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
witnessSlice,
witnessNextValue,
divideWithRemainder,
toVar,
} from './common.js';
import { rangeCheck64 } from './range-check.js';

Expand Down Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does .seal() do here?

Copy link
Collaborator Author

@mitschabaude mitschabaude Nov 14, 2023

Choose a reason for hiding this comment

The 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 not() is empty (I would have expected a generic gate, for the subtraction).

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.

seal() triggers that concretizing of the AST, by creating a new variable that is asserted equal with the old variable, so with seal() this gadget immediately leads to a generic gate in the CS. I thought this was the better behavior for a gadget, as it's normalized and not dependent on the remaining circuit where this subtraction will end up in the CS.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So seal() is for making sure snarky doesn't add generic gates where they are not desired (e.g. in the middle of a chained gate)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The snarky version of seal() could be used for that! The o1js version not because it doesn't "seal" constants. The o1js version was intended more to prevent unnecessary large numbers of constraints when the same long AST is flushed to generic gates again and again. The function added in this PR that does what you descirbe @jspada is called toVar()

}
}

Expand Down Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the kind of thing that is impossible to catch without tests

Copy link

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 x + 20y + 3z + 17, and this will generate an even or odd number of generic gates when passed to a gate, depending on the length of the linear combination. Since we test many random inputs, I'm pretty sure all cases are covered

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

That sounds interesting! Do you know where that PR is?


// Compute current row
Gates.rotate(
field,
Expand Down
70 changes: 70 additions & 0 deletions src/lib/gadgets/bitwise.unit-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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');
Copy link

Choose a reason for hiding this comment

The 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)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you only supporting And64 then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, this tests a specific example circuit -- the and() method on the Bitwise zkprogram, which happens to use 64-bit and. but we support all bit lengths

);

let rotChain: GateType[] = ['Rot64', 'RangeCheck0', 'RangeCheck0'];
Copy link
Member

Choose a reason for hiding this comment

The 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
);
37 changes: 35 additions & 2 deletions src/lib/gadgets/common.ts
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';

Expand All @@ -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
Expand All @@ -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.
Copy link

Choose a reason for hiding this comment

The 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) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is better called ConstrainConstantHere or something like that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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>>(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConstrainConstantsHere?

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');
Expand Down
9 changes: 7 additions & 2 deletions src/lib/gadgets/range-check.ts
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 };

Expand Down Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 });
}

/**
Expand All @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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()));

Expand Down
55 changes: 30 additions & 25 deletions src/lib/gadgets/range-check.unit-test.ts
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);
Expand All @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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
// --------------------------
Expand Down
10 changes: 10 additions & 0 deletions src/lib/proof_system.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'>>,
Expand Down Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 program.rawMethods. this was asked for by community members as well and it helped me reuse the methods for tests in this PR

},
provers
);
Expand Down
Loading
Loading