Add bounds checks to GPC #1823
Add bounds checks to GPC #1823
Conversation
There was a problem hiding this comment.
As usual, I reviewed the functional code but not the tests yet, which I'll save for a later round.
I made some structural and interface-level comments which I'd like to hear your thoughts on, since they could motivate some changes to the spec or config types.
packages/lib/gpc/src/gpcTypes.ts
Outdated
| * 64-bit integer value and will be revealed by virtue of its inclusion in the | ||
| * proof configuration. | ||
| */ | ||
| minValue?: bigint; |
There was a problem hiding this comment.
I'm not sure, but I think these should maybe be PODValue rather than bigint.
Advantage of bare bigint value is terseness. It also directly represents what happens in the circuit.
Advantage of PODValue is that supported range is implicitly specified by the type, and it'll work in future for differently-encoded numeric values, such as a date.
PODValue also gives a slightly simpler path toward JSON serialization (assuming PODValue is a type for which serialization will be solved). And if an eventual JSONPODValue type includes number as an option for encoding small ints, then we get back to terseness.
I'm not sure if this is worth dealing with now. It can wait to be dealt with along with one of the features which make it more valuable (date type, PODValue serialization).
| @@ -130,6 +130,14 @@ export type GPCProofEntryConfigCommon = { | |||
| * and constraints should be enabled for that entry. | |||
| */ | |||
| export type GPCProofEntryConfig = GPCProofEntryConfigCommon & { | |||
| /** | |||
There was a problem hiding this comment.
In future, we could consider shortcuts for this if we think it's too verbose. E.g. inRange: "int" might be a shortcut for checking a value against POD_INT_MIN and POD_INT_MAX. Or inRange: {min: 100, max: "int"} could be a shortcut for a "one-sided" bounds-check, given that we can't really support truly one-sided bounds.
This is most likely to come up when we add further modules which require a bounds-check as a pre-requisite (e.g. ordered comparison, summing values). I'm not sure yet the best way to represent that requirement, and whether it should be implicit (if you compare two fields, the bounds checks happen silently) or explicit (you're required to specify the bounds check on the fields you compare).
Co-authored-by: Andrew Twyman <[email protected]>
| const hasBoundsCheck = entryConfig.inRange !== undefined; | ||
|
|
||
| if (hasBoundsCheck) { | ||
| const inRange = entryConfig.inRange as BoundsConfig; |
There was a problem hiding this comment.
I think this could be satisfies rather than as.
As I understand it, the former will compile only if the input really does match the fields for right type, while the latter will "take your word for it" and not trigger a compile error if you did something wrong.
There was a problem hiding this comment.
Agreed, but weirdly enough the compiler still thinks it could be undefined so that satisfies does not work, which is why I introduced this variable to do the type coercion for the following lines. It is a different story with if(hasBoundsCheck) replaced with if(entryConfig.inRange !== undefined).
There was a problem hiding this comment.
Oh, yeah I've seen this particular bit of dumb typechecking before.
I think if you saved the bounds in a variable and then checked that variable directly against undefined in your if statement the compiler would see that it's defined, but then you return statement would get more complicated.
I wish there were a more limited form for a compile-time assertion that something is defined, without fully overriding its type.
Resolves https://linear.app/0xparc-pcd/issue/0XP-951/gpc-lowerboundupperbound-proofs
This PR adds bounds checks to the GPC config. Summary of changes:
minValueandmaxValuefields to the proof entry config