-
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
Efficient comparisons #1523
Efficient comparisons #1523
Conversation
cc39156
to
ffae661
Compare
src/lib/provable/crypto/signature.ts
Outdated
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.
here we use the new efficient isOdd()
, plus some boyscouting
return [x & 1n, x >> 1n]; | ||
}); | ||
let isOdd = b.assertBool(); | ||
z.assertLessThan((Field.ORDER + 1n) / 2n); |
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.
using the new full field comparison!
* Field(2).lessThan(3).assertEquals(Bool(true)); | ||
* let isTrue = Field(2).lessThan(3); |
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.
some comments boyscouting in this file. I didn't like that the example for lessThan()
was one that should actually have used assertLessThan()
* Calling this function is equivalent to `Field(...).lessThan(...).assertEquals(Bool(true))`. | ||
* | ||
* Note: This uses fewer constraints than `x.lessThan(y).assertTrue()`. |
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.
didn't like that this said it "was equivalent", people should use the assertion version if possible because they are cheaper
src/lib/provable/gadgets/basic.ts
Outdated
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.
removing the import cycles was mainly about importing Field
only as a type, and using what we need from field-constructor.ts
instead
/** | ||
* Version of `multiRangeCheck` which does the check on a truncated version of the input, | ||
* so that it always succeeds, and then checks equality of the truncated and full input. | ||
* | ||
* This is a hack to get an error when the constraint fails, around the fact that multiRangeCheck | ||
* is not checked by snarky. | ||
*/ | ||
function indirectMultiRangeChange( |
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.
added this to not make the (negative) unit tests break. it's also better DX
let xMinusYFits = RangeCheck.isDefinitelyInRangeN( | ||
UInt64.NUM_BITS, | ||
xMinusY | ||
); | ||
|
||
let yMinusXFits = RangeCheck.isDefinitelyInRangeN( | ||
UInt64.NUM_BITS, | ||
yMinusX | ||
); | ||
|
||
xMinusYFits.or(yMinusXFits).assertEquals(true); | ||
// x <= y if y - x fits in 64 bits | ||
return yMinusXFits; | ||
} | ||
return lessThanOrEqualGeneric(this.value, y.value, 1n << 64n, (v) => | ||
RangeCheck.rangeCheckN(UInt64.NUM_BITS, v) | ||
); |
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 used to be less efficient because it called rangeCheckHelper()
twice. the new version only calls it once
asProver, | ||
constraintSystem, | ||
generateWitness, | ||
} from './core/provable-context.js'; | ||
import { exists, existsAsync } from './core/exists.js'; | ||
import { witness, witnessAsync } from './types/witness.js'; |
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.
moved witness()
to its own file to make it not depend on Field
assertBoolean(this); | ||
return Bool.Unsafe.fromField(this); |
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 made assertBool()
return the Bool because this is a common use case: assert that some witnessed field element is boolean and then use the bool
equivalentProvable({ from: [big264, big264], to: big264 })( | ||
// addition can fail on two unreduced inputs because we can get x + y - f > 2^264 | ||
equivalentProvable({ from: [big264, f], to: big264 })( |
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.
we didn't catch this before because of the missing errors on multi range check failures 😬
@@ -30,6 +30,12 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm | |||
- Remove `CircuitValue`, `prop`, `arrayProp` and `matrixProp` https://github.com/o1-labs/o1js/pull/1507 | |||
- Remove `Mina.accountCreationFee()`, `Mina.BerkeleyQANet`, all APIs which accept private keys for feepayers, `Token`, `AccountUpdate.tokenSymbol`, `SmartContract.{token, setValue, setPermissions}`, "assert" methods for preconditions, `MerkleTee.calculateRootSlow()`, `Scalar.fromBigInt()`, `UInt64.lt()` and friends, deprecated static methods on `Group`, utility methods on `Circuit` like `Circuit.if()`, `Field.isZero()`, `isReady` and `shutdown()` https://github.com/o1-labs/o1js/pull/1515 | |||
- Remove `privateKey` from the accepted arguments of `SmartContract.deploy()` https://github.com/o1-labs/o1js/pull/1515 | |||
- **Efficient comparisons**. Support arbitrary bit lengths for `Field` comparisons and massively reduce their constraints https://github.com/o1-labs/o1js/pull/1523 | |||
- `Field.assertLessThan()` goes from 510 to 24 constraints, `Field.lessThan()` from 509 to 38 |
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!
CHANGELOG.md
Outdated
@@ -30,6 +30,12 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm | |||
- Remove `CircuitValue`, `prop`, `arrayProp` and `matrixProp` https://github.com/o1-labs/o1js/pull/1507 | |||
- Remove `Mina.accountCreationFee()`, `Mina.BerkeleyQANet`, all APIs which accept private keys for feepayers, `Token`, `AccountUpdate.tokenSymbol`, `SmartContract.{token, setValue, setPermissions}`, "assert" methods for preconditions, `MerkleTee.calculateRootSlow()`, `Scalar.fromBigInt()`, `UInt64.lt()` and friends, deprecated static methods on `Group`, utility methods on `Circuit` like `Circuit.if()`, `Field.isZero()`, `isReady` and `shutdown()` https://github.com/o1-labs/o1js/pull/1515 | |||
- Remove `privateKey` from the accepted arguments of `SmartContract.deploy()` https://github.com/o1-labs/o1js/pull/1515 | |||
- **Efficient comparisons**. Support arbitrary bit lengths for `Field` comparisons and massively reduce their constraints https://github.com/o1-labs/o1js/pull/1523 | |||
- `Field.assertLessThan()` goes from 510 to 24 constraints, `Field.lessThan()` from 509 to 38 | |||
- Moderately improve other comparisons: `UInt64.assertLessThan()` from 27 to 14,, `UInt64.lessThan()` from 27 to 15, `UInt32` similar. |
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.
double ,,
closes #1434
isEven()
andisOdd()
using full field comparison, which makes them much cheaper and also handle 255-bit field elementsisEven()
for converting between compressed and uncompressed public keysField
class, and prevent against import cycle problems in the future, I got rid of the dependency onField
in many of the gadgets and a few other places