Merge pull request #998 from o1-labs/feature/better-readvar-error

Improve error when reading variable in provable code

CHANGELOG.md

@@ -21,6 +21,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 - `Group` operations now generate a different set of constraints. This breaks deployed contracts, because the circuit changed. https://github.com/o1-labs/snarkyjs/pull/967
 
+### Changed
+
+- Improve error message `Can't evaluate prover code outside an as_prover block` https://github.com/o1-labs/snarkyjs/pull/998
+
 ## [0.11.0](https://github.com/o1-labs/snarkyjs/compare/a632313a...3fbd9678e)
 
 ### Breaking changes

src/lib/account_update.ts

@@ -3,11 +3,10 @@ import {
   FlexibleProvable,
   provable,
   provablePure,
-  Struct,
 } from './circuit_value.js';
 import { memoizationContext, memoizeWitness, Provable } from './provable.js';
 import { Field, Bool } from './core.js';
-import { Ledger, Pickles, Test } from '../snarky.js';
+import { Pickles, Test } from '../snarky.js';
 import { jsLayout } from '../bindings/mina-transaction/gen/js-layout.js';
 import {
   Types,
@@ -30,7 +29,6 @@ import { hashWithPrefix, packToFields } from './hash.js';
 import { prefixes } from '../bindings/crypto/constants.js';
 import { Context } from './global-context.js';
 import { assert } from './errors.js';
-import { Ml } from './ml/conversion.js';
 import { MlArray } from './ml/base.js';
 import { Signature, signFieldElement } from '../mina-signer/src/signature.js';
 import { MlFieldConstArray } from './ml/fields.js';

src/lib/account_update.unit-test.ts

@@ -1,5 +1,4 @@
 import {
-  Ledger,
   AccountUpdate,
   PrivateKey,
   Field,

src/lib/bool.ts

@@ -1,5 +1,11 @@
 import { Snarky } from '../snarky.js';
-import { Field, FieldConst, FieldType, FieldVar } from './field.js';
+import {
+  Field,
+  FieldConst,
+  FieldType,
+  FieldVar,
+  readVarMessage,
+} from './field.js';
 import { Bool as B } from '../provable/field-bigint.js';
 import { defineBinable } from '../bindings/lib/binable.js';
 import { NonNegativeInteger } from 'src/bindings/crypto/non-negative.js';
@@ -179,8 +185,10 @@ class Bool {
     let value: FieldConst;
     if (this.isConstant()) {
       value = this.value[1];
-    } else {
+    } else if (Snarky.run.inProverBlock()) {
       value = Snarky.field.readVar(this.value);
+    } else {
+      throw Error(readVarMessage('toBoolean', 'b', 'Bool'));
     }
     return FieldConst.equal(value, FieldConst[1]);
   }
@@ -368,7 +376,7 @@ function toBoolean(x: boolean | Bool): boolean {
   if (typeof x === 'boolean') {
     return x;
   }
-  return (x as Bool).toBoolean();
+  return x.toBoolean();
 }
 
 // TODO: This is duplicated

src/lib/circuit_value.ts

@@ -1,6 +1,6 @@
 import 'reflect-metadata';
 import { ProvablePure } from '../snarky.js';
-import { Field, Bool } from './core.js';
+import { Field, Bool, Scalar, Group } from './core.js';
 import {
   provable,
   provablePure,
@@ -451,21 +451,26 @@ function Struct<
   return Struct_ as any;
 }
 
-let primitives = new Set(['Field', 'Bool', 'Scalar', 'Group']);
+let primitives = new Set([Field, Bool, Scalar, Group]);
+function isPrimitive(obj: any) {
+  for (let P of primitives) {
+    if (obj instanceof P) return true;
+  }
+  return false;
+}
 
-// FIXME: the logic in here to check for obj.constructor.name actually doesn't work
-// something that works is Field(1).constructor === obj.constructor etc
 function cloneCircuitValue<T>(obj: T): T {
   // primitive JS types and functions aren't cloned
   if (typeof obj !== 'object' || obj === null) return obj;
 
   // HACK: callbacks, account udpates
   if (
-    ['GenericArgument', 'Callback'].includes((obj as any).constructor?.name)
+    obj.constructor?.name.includes('GenericArgument') ||
+    obj.constructor?.name.includes('Callback')
   ) {
     return obj;
   }
-  if (['AccountUpdate'].includes((obj as any).constructor?.name)) {
+  if (obj.constructor?.name.includes('AccountUpdate')) {
     return (obj as any).constructor.clone(obj);
   }
 
@@ -480,7 +485,9 @@ function cloneCircuitValue<T>(obj: T): T {
   if (ArrayBuffer.isView(obj)) return new (obj.constructor as any)(obj);
 
   // snarkyjs primitives aren't cloned
-  if (primitives.has((obj as any).constructor.name)) return obj;
+  if (isPrimitive(obj)) {
+    return obj;
+  }
 
   // cloning strategy that works for plain objects AND classes whose constructor only assigns properties
   let propertyDescriptors: Record<string, PropertyDescriptor> = {};

src/lib/field.ts

@@ -2,14 +2,24 @@ import { Snarky, Provable } from '../snarky.js';
 import { Field as Fp } from '../provable/field-bigint.js';
 import { defineBinable } from '../bindings/lib/binable.js';
 import type { NonNegativeInteger } from '../bindings/crypto/non-negative.js';
-import { asProver } from './provable-context.js';
+import { asProver, inCheckedComputation } from './provable-context.js';
 import { Bool } from './bool.js';
+import { assert } from './errors.js';
 
 // external API
 export { Field };
 
 // internal API
-export { ConstantField, FieldType, FieldVar, FieldConst, isField, withMessage };
+export {
+  ConstantField,
+  FieldType,
+  FieldVar,
+  FieldConst,
+  isField,
+  withMessage,
+  readVarMessage,
+  toConstantField,
+};
 
 type FieldConst = Uint8Array;
 
@@ -190,6 +200,10 @@ class Field {
     return this.value[0] === FieldType.Constant;
   }
 
+  #toConstant(name: string): ConstantField {
+    return toConstantField(this, name, 'x', 'field element');
+  }
+
   /**
    * Create a {@link Field} element equivalent to this {@link Field} element's value,
    * but is a constant.
@@ -204,10 +218,7 @@ class Field {
    * @return A constant {@link Field} element equivalent to this {@link Field} element.
    */
   toConstant(): ConstantField {
-    if (this.isConstant()) return this;
-    // TODO: fix OCaml error message, `Can't evaluate prover code outside an as_prover block`
-    let value = Snarky.field.readVar(this.value);
-    return new Field(value) as ConstantField;
+    return this.#toConstant('toConstant');
   }
 
   /**
@@ -224,7 +235,7 @@ class Field {
    * @return A bigint equivalent to the bigint representation of the Field.
    */
   toBigInt() {
-    let x = this.toConstant();
+    let x = this.#toConstant('toBigInt');
     return FieldConst.toBigint(x.value[1]);
   }
 
@@ -242,7 +253,7 @@ class Field {
    * @return A string equivalent to the string representation of the Field.
    */
   toString() {
-    return this.toBigInt().toString();
+    return this.#toConstant('toString').toBigInt().toString();
   }
 
   /**
@@ -1111,7 +1122,7 @@ class Field {
    * @return A string equivalent to the JSON representation of the {@link Field}.
    */
   toJSON() {
-    return this.toString();
+    return this.#toConstant('toJSON').toString();
   }
 
   /**
@@ -1215,17 +1226,12 @@ class Field {
 
 const FieldBinable = defineBinable({
   toBytes(t: Field) {
-    return [...t.toConstant().value[1]];
+    return [...toConstantField(t, 'toBytes').value[1]];
   },
   readBytes(bytes, offset) {
     let uint8array = new Uint8Array(32);
     uint8array.set(bytes.slice(offset, offset + 32));
-    return [
-      Object.assign(Object.create(new Field(1).constructor.prototype), {
-        value: [0, uint8array],
-      }) as Field,
-      offset + 32,
-    ];
+    return [new Field(uint8array), offset + 32];
   },
 });
 
@@ -1256,3 +1262,48 @@ function withMessage(error: unknown, message?: string) {
   error.message = `${message}\n${error.message}`;
   return error;
 }
+
+function toConstantField(
+  x: Field,
+  methodName: string,
+  varName = 'x',
+  varDescription = 'field element'
+): ConstantField {
+  // if this is a constant, return it
+  if (x.isConstant()) return x;
+
+  // a non-constant can only appear inside a checked computation. everything else is a bug.
+  assert(
+    inCheckedComputation(),
+    'variables only exist inside checked computations'
+  );
+
+  // if we are inside an asProver or witness block, read the variable's value and return it as constant
+  if (Snarky.run.inProverBlock()) {
+    let value = Snarky.field.readVar(x.value);
+    return new Field(value) as ConstantField;
+  }
+
+  // otherwise, calling `toConstant()` is likely a mistake. throw a helpful error message.
+  throw Error(readVarMessage(methodName, varName, varDescription));
+}
+
+function readVarMessage(
+  methodName: string,
+  varName: string,
+  varDescription: string
+) {
+  return `${varName}.${methodName}() was called on a variable ${varDescription} \`${varName}\` in provable code.
+This is not supported, because variables represent an abstract computation, 
+which only carries actual values during proving, but not during compiling.
+
+Also, reading out JS values means that whatever you're doing with those values will no longer be
+linked to the original variable in the proof, which makes this pattern prone to security holes.
+
+You can check whether your ${varDescription} is a variable or a constant by using ${varName}.isConstant().
+
+To inspect values for debugging, use Provable.log(${varName}). For more advanced use cases,
+there is \`Provable.asProver(() => { ... })\` which allows you to use ${varName}.${methodName}() inside the callback.
+Warning: whatever happens inside asProver() will not be part of the zk proof.
+`;
+}

src/lib/hash-input.unit-test.ts

@@ -107,11 +107,13 @@ function testInput<T, TJson>(
   // console.log('json', json);
   let input1 = MlHashInput.from(toInputOcaml(JSON.stringify(json)));
   let input2 = Module.toInput(value);
-  // console.log('snarkyjs', JSON.stringify(input2));
+  let input1Json = JSON.stringify(input1);
+  let input2Json = JSON.stringify(input2);
+  // console.log('snarkyjs', input2Json);
   // console.log();
-  // console.log('protocol', JSON.stringify(input1));
-  let ok1 = JSON.stringify(input2) === JSON.stringify(input1);
-  expect(JSON.stringify(input2)).toEqual(JSON.stringify(input1));
+  // console.log('protocol', input1Json);
+  let ok1 = input1Json === input2Json;
+  expect(input2Json).toEqual(input1Json);
   // console.log('ok?', ok1);
   let fields1 = MlFieldConstArray.from(
     hashInputFromJson.packInput(MlHashInput.to(input1))

src/lib/provable-context.ts

@@ -58,7 +58,7 @@ function inCompileMode() {
 
 function asProver(f: () => void) {
   if (inCheckedComputation()) {
-    Snarky.asProver(f);
+    Snarky.run.asProver(f);
   } else {
     f();
   }
@@ -67,7 +67,7 @@ function asProver(f: () => void) {
 function runAndCheck(f: () => void) {
   let id = snarkContext.enter({ inCheckedComputation: true });
   try {
-    Snarky.runAndCheck(f);
+    Snarky.run.runAndCheck(f);
   } catch (error) {
     throw prettifyStacktrace(error);
   } finally {
@@ -78,7 +78,7 @@ function runAndCheck(f: () => void) {
 function runUnchecked(f: () => void) {
   let id = snarkContext.enter({ inCheckedComputation: true });
   try {
-    Snarky.runUnchecked(f);
+    Snarky.run.runUnchecked(f);
   } catch (error) {
     throw prettifyStacktrace(error);
   } finally {
@@ -90,7 +90,7 @@ function constraintSystem<T>(f: () => T) {
   let id = snarkContext.enter({ inAnalyze: true, inCheckedComputation: true });
   try {
     let result: T;
-    let { rows, digest, json } = Snarky.constraintSystem(() => {
+    let { rows, digest, json } = Snarky.run.constraintSystem(() => {
       result = f();
     });
     let { gates, publicInputSize } = gatesFromJson(json);