-
Notifications
You must be signed in to change notification settings - Fork 133
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 1: Multi-range-check gadgets #1216
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.
Looks great to me! I just had one question for myself :D
return [new Field(x), new Field(y), z]; | ||
} | ||
|
||
let [x, y] = exists(2, () => splitCompactLimb(xy.toBigInt())); |
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.
Just a question for me here :D
My understanding is that exists
brings in a witness, just like Provable.witness
does. When should you use exists
vs Provable.witness
?
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.
Provable.witness
handles any provable data types. I created exists
to have something that's quicker to use if you just want an array of field elements. It's better than witness
for that in three ways:
- no boilerplate for specifying the type
Provable.Array(Field, 5)
- you can return bigints from the callback instead of
Fields
- the array length is typed! i.e. it's a type error to return or unpack too many outputs, like
[x,y,z]
in this example
My thinking was that for low-level gadgets, everything will be arrays of field elements so it makes sense to have something which optimizes for that
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! Thank you for the explanation. I must admit, I used Provable.witness
for the rotation gadget, not a big deal but I wish I used exists
instead!
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!
src/lib/gadgets/gadgets.ts
Outdated
* In particular, the 3x88-bit range check supports bigints up to 264 bits, which in turn is enough | ||
* to support foreign field multiplication with moduli up to 2^259. | ||
*/ | ||
multiRangeCheck(x: Field, y: Field, z: 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.
It would be great to match the template similar to the other gates, such as having an @example
block, describing inputs, if it can throw, etc, just for consistency stake.
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.
Minor suggestions and a question about endianness. Otherwise, lgtm!
/** | ||
* Compact multi-range check | ||
* | ||
* This is a variant of {@link multiRangeCheck} where the first two variables are passed in |
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 you intending that the compact range-check is usable externally as part of the API or is it only for the internal optimization?
Maybe a good idea to note any limitations of this gadget and when this is safe 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.
Yeah - so for context, the way we expose this is on a Gadgets
namespace which is presented as a set of low-level provable methods for advanced users (including ourselves, in higher-level APIs). So I think the compact range check fits well to include. (It gives you something you can't do in any other way, so in a sense it can't be left out)
What do you think are limitations/security concerns that should be highlighted?
@@ -2,10 +2,10 @@ import { Field } from '../field.js'; | |||
import * as Gates from '../gates.js'; | |||
import { bitSlice, exists } from './common.js'; | |||
|
|||
export { rangeCheck64 }; | |||
export { rangeCheck64, multiRangeCheck, compactMultiRangeCheck, L }; |
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.
LIMB_BITS
would be a better name than L
return [new Field(x), new Field(y), z]; | ||
} | ||
|
||
let [x, y] = exists(2, () => splitCompactLimb(xy.toBigInt())); |
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!
exists(13, () => { | ||
let zz = z.toBigInt(); | ||
return [ | ||
bitSlice(zz, 22, 2), |
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.
Which endianness is this in? The curr
and next
row seem in opposite order of the rust implementation. Probably just endianness. Otherwise, lgtm!
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 little endian - bitSlice(zz, 22, 2)
means that I take 2 bits starting at bit 22, where bit 0 is the LSB. so the current
row gets the more significant bits, the next
row gets the less significant bits. This matches the Rust docs
]; | ||
}); | ||
|
||
// 12-bit limbs |
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 seems different than the rust implementation. In the rust implementation the crumbs are in columns 7-14. Maybe it has to do with the endianness... in the rust implementation of the gate limbs are mapped to columns in big-endian order (the lowest columns contain the highest bits).
The reason for this is so that the hiehgest bits are covered by copy constraints and lookups, so that we can copy the highest two 12-bit limbs to zero in order to use the RangeCheck0
gate for 64-bit range-checks.
You probably have this right here and it's just because of a different byte-order, but I just wanted to call it out explicitly to make sure.
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.
so, I guess I create the witnesses in little endian order here (which seemed more natural to me), but note that I pass them to the gate in big endian order, see here
}, | ||
async (xy, z) => { | ||
let proof = await RangeCheck.checkCompact(xy, z); | ||
return await RangeCheck.verify(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.
I don't see any out of range tests. might be a good idea.
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 test for both in range and out of range! the function signature spec is
{ from: [maybeUint(2n * L), uint(L)], to: boolean },
in our test framework, this means that the first argument xy
will sometimes be an invalid and sometimes a valid uint of 2L
bits. the test is run on a number of random samples. since it does a proof everytime, we only do 3 samples per test run ({ runs: 3 }
) but locally I do 20 runs or so when first creating a test like this. the distributions are pretty clever so this always tends to uncover the bugs
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 equivalent()
test checks that the two functions either both throw an error or return the same result. The first function just checks the range and throws an error if it's out of range, so we want to be equivalent to that
Add multi-range check gadgets and tests