-
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 curve and ECDSA #1007
Foreign curve and ECDSA #1007
Conversation
package.json
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.
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
src/examples/crypto/ecdsa/ecdsa.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.
this example was moved here from examples/zkprogram/
and now uses the new class APIs
* let isXZero = x.equals(0); | ||
* ``` | ||
*/ | ||
equals(y: bigint | number) { |
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 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
src/lib/foreign-field.ts
Outdated
@@ -186,7 +191,7 @@ class ForeignField { | |||
* | |||
* Returns the field element as a {@link CanonicalForeignField}. | |||
*/ | |||
assertCanonicalFieldElement() { | |||
assertCanonical() { |
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.
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> { |
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.
making this support toJSON
etc. makes a nicer inferred Provable
for ForeignCurve
and EcdsaSignature
as well
summary() { | ||
let gateTypes: Partial<Record<GateType | 'Total rows', number>> = {}; | ||
gateTypes['Total rows'] = rows; |
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 had this code manually copied in a lot of files, find it useful to expose
// prevent blow-up of constant size | ||
if (x[0] === FieldType.Constant && y[0] === FieldType.Constant) return x; |
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.
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)
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.` | ||
); | ||
} |
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.
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'); |
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 prevents the accident with ecdsa, where we weren't testing it in provable code, in the future
"rows": 38846, | ||
"digest": "892b0a1fad0f13d92ba6099cd54e6780" | ||
"rows": 38888, | ||
"digest": "70f148db469f028f1d8cb99e8e8e278a" |
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 example circuit changed: it previously did no validity checks on the input public key, now it does (automatically, when the input is witnessed)
* `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 |
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 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); |
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.
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?
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.
no, map to curve is faster. EC scale is a fairly heavy operation - much heavier than square root
This reverts commit 7ec8090.
Reminder to remove "coming soon" from the docs when this is merged |
stacked behind both #985 and #1240
closes #966, #1156 , #1158
Main changes:
gadgets/elliptic-curve.ts
:assertOnCurve()
,assertInSubgroup()
,negate()
,scale()
Curve
and possibly other configForeignCurve
class andcreateForeignCurve()
factory, seeforeign-curve.ts
EcdsaSignature
class andcreateEcdsa()
factory, seeforeign-ecdsa.ts
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 thingsForeignField
bindings: o1-labs/o1js-bindings#215