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 curve and ECDSA #1007

Merged
merged 113 commits into from
Dec 12, 2023
Merged

Foreign curve and ECDSA #1007

merged 113 commits into from
Dec 12, 2023

Conversation

mitschabaude
Copy link
Member

@mitschabaude mitschabaude commented Jun 28, 2023

stacked behind both #985 and #1240

closes #966, #1156 , #1158

Main changes:

  • Adds several missing EC gadgets in gadgets/elliptic-curve.ts: assertOnCurve(), assertInSubgroup(), negate(), scale()
    • unifies the API of all EC gadgets, to take inputs first and then a Curve and possibly other config
  • Adds unit tests for EC gadgets
  • New export: ForeignCurve class and createForeignCurve() factory, see foreign-curve.ts
  • New export: EcdsaSignature class and createEcdsa() factory, see foreign-ecdsa.ts
  • Removes the lower-level Gadgets.Ecdsa namespace. I concluded that it wasn't very usable without a sibling low-level EC gadgets namespace, and exposing the full EC + ECDSA API in two different forms seemed complete overkill to me. Better to have one way to do things
  • Some additions and changes to ForeignField

bindings: o1-labs/o1js-bindings#215

package.json Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

I simplified the scripts as I was finding myself running npm run build:test everytime -- now both npm run dev and npm run build include building test dependencies, and only the production build doesn't

Copy link
Member Author

Choose a reason for hiding this comment

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

this example was moved here from examples/zkprogram/ and now uses the new class APIs

* let isXZero = x.equals(0);
* ```
*/
equals(y: bigint | number) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed (when I wanted to use it) that the original ForeignField.equals() is very error-prone, since it checked for exact equality instead of equality mod p. I replaced it with this constant version (which assumes an almost reduced input) and the full version on CanonicalForeignField

@@ -186,7 +191,7 @@ class ForeignField {
*
* Returns the field element as a {@link CanonicalForeignField}.
*/
assertCanonicalFieldElement() {
assertCanonical() {
Copy link
Member Author

Choose a reason for hiding this comment

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

old name was too long

@@ -661,7 +696,7 @@ type Constructor<T> = new (...args: any[]) => T;

function provable<F extends ForeignField>(
Class: Constructor<F> & { check(x: ForeignField): void }
): ProvablePure<F> {
): ProvablePureExtended<F, string> {
Copy link
Member Author

Choose a reason for hiding this comment

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

making this support toJSON etc. makes a nicer inferred Provable for ForeignCurve and EcdsaSignature as well

Comment on lines +108 to +110
summary() {
let gateTypes: Partial<Record<GateType | 'Total rows', number>> = {};
gateTypes['Total rows'] = rows;
Copy link
Member Author

Choose a reason for hiding this comment

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

I had this code manually copied in a lot of files, find it useful to expose

Comment on lines +400 to +401
// prevent blow-up of constant size
if (x[0] === FieldType.Constant && y[0] === FieldType.Constant) return x;
Copy link
Member Author

Choose a reason for hiding this comment

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

changes here prevent rare cases where constraintSystem tests would be flaky on a very large constant input (for example, if the circuit asserts that the input is less than 256)

Comment on lines +245 to +251
if (verbose) {
let ms = (performance.now() - start).toFixed(1);
let runs = nRuns.toString().padStart(2, ' ');
console.log(
`${label.padEnd(20, ' ')} success on ${runs} runs in ${ms}ms.`
);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

new argument verbose prints a nice summary of the test run. I used this to debug why the new EC unit tests were taking so long (turns out that scale() just takes a pretty long time, like a few seconds, I'm planning to look into how it can be made faster, as a follow-up)

let fromUnions = fromRaw.map(toUnion);
assert(fromUnions.some(isProvable), 'equivalentProvable: no provable input');
Copy link
Member Author

Choose a reason for hiding this comment

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

this prevents the accident with ecdsa, where we weren't testing it in provable code, in the future

Comment on lines 209 to 210
"rows": 38846,
"digest": "892b0a1fad0f13d92ba6099cd54e6780"
"rows": 38888,
"digest": "70f148db469f028f1d8cb99e8e8e278a"
Copy link
Member Author

Choose a reason for hiding this comment

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

the example circuit changed: it previously did no validity checks on the input public key, now it does (automatically, when the input is witnessed)

@mitschabaude mitschabaude marked this pull request as ready for review December 5, 2023 13:52
@mitschabaude mitschabaude requested a review from a team as a code owner December 5, 2023 13:52
* `createForeignCurve(params)` takes curve parameters {@link CurveParams} as input.
* We support `modulus` and `order` to be prime numbers up to 259 bits.
*
* The returned {@link ForeignCurve} class represents a _non-zero curve point_ and supports standard
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there are any important algorithms where we actually have to deal with the point at infinity 🤔


function random(Curve: CurveAffine) {
let x = Curve.Field.random();
return simpleMapToCurve(x, Curve);
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't it be more efficient to scale the generator by a random element x to gain a new random point instead of going through a map to curve algorithm?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, map to curve is faster. EC scale is a fairly heavy operation - much heavier than square root

Base automatically changed from feature/ecdsa-new to main December 6, 2023 21:06
@nicc
Copy link
Collaborator

nicc commented Dec 7, 2023

Reminder to remove "coming soon" from the docs when this is merged

@mitschabaude mitschabaude merged commit bf00ea6 into main Dec 12, 2023
13 checks passed
@mitschabaude mitschabaude deleted the feature/ecdsa branch December 12, 2023 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port EcCheckSubgroup gadget to TypeScript Port ECIsOnCurve gadget to TypeScript Expose ECDSA
4 participants