-
Notifications
You must be signed in to change notification settings - Fork 118
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 scalar mul and other Scalar improvements #1530
Conversation
This reverts commit 04f8648.
@@ -319,24 +320,7 @@ class Field { | |||
* See {@link Field.isEven} for examples. | |||
*/ | |||
isOdd() { | |||
if (this.isConstant()) return new Bool((this.toBigInt() & 1n) === 1n); |
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 moved this gadget into gadgets/comparison.ts
to reuse it for scaleField()
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 is where they new scaling gadgets live! needs careful crypto review
@@ -101,67 +106,49 @@ class Group { | |||
return fromProjective(g_proj); | |||
} | |||
} else { | |||
const { x: x1, y: y1 } = 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.
this code was moved to native-group.ts
to reuse there. it was also rewritten a bit and a bug fixed
* **Warning**: If one of the inputs is zero, the result will be garbage and the proof useless. | ||
* This case has to be prevented or handled separately by the caller of this method. | ||
*/ | ||
addNonZero(g2: Group, allowZeroOutput = false): Group { |
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 added this, I think it's actually a bit more common to use this than fully complete addition
@@ -108,83 +102,3 @@ equivalent({ from: [f], to: f })( | |||
return ForeignScalar.fromBits(bits); | |||
} | |||
); | |||
|
|||
// scalar shift in foreign field arithmetic vs in the exponent |
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 test was no longer valid, so I removed it
@@ -62,6 +66,26 @@ function bytes(length: number) { | |||
}); | |||
} | |||
|
|||
function pointSpec<T>(field: ProvableSpec<bigint, T>, Curve: CurveAffine) { |
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.
just a testing utility, ends up unused (I used it during development to monitor the size of some circuits)
* `numBits <= n - 2` where `n` is the bit length of the scalar field. | ||
* In our case, n=255 so numBits <= 253. | ||
*/ | ||
scaleFastUnpack( |
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 is the scaling gadget that Pickles uses as well
@@ -173,6 +174,16 @@ const BasicCS = constraintSystem('Basic', { | |||
}, | |||
}); | |||
|
|||
const CryptoCS = constraintSystem('Crypto', { |
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 vk regression test
src/lib/provable/gadgets/scalar.ts
Outdated
let tHi0 = existsOne(() => t0.toBigInt() >> 5n); | ||
|
||
// prove split | ||
// since we know that t0 < 2^88, this proves that t0High < 2^83 |
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.
tHi0 you mean?
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'm afraid you're reviewing outdated code :O
yeah I meant tHi0
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.
yeah, yesterday I was commit-reviewing to gain more context. Today I will focus on the updated native-curve.ts
file
src/lib/provable/gadgets/scalar.ts
Outdated
@@ -61,7 +61,7 @@ function scale(P: { x: Field; y: Field }, s: Field): Group { | |||
|
|||
/** | |||
* Internal helper to compute `(t + 2^254)*P`. | |||
* `t` is expected to be split into 250 high bits (t >> 5) and 5 low bits (t & 0xf1). | |||
* `t` is expected to be split into 250 high bits (t >> 5) and 5 low bits (t & 0x1f). |
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.
ah okay, that's now 5 bits
src/lib/provable/gadgets/scalar.ts
Outdated
let tHi0 = existsOne(() => t0.toBigInt() >> 5n); | ||
|
||
// prove split | ||
// since we know that t0 < 2^88, this proves that t0High < 2^83 |
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.
yeah, yesterday I was commit-reviewing to gain more context. Today I will focus on the updated native-curve.ts
file
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.
Very clear comments to understand the technique for an outsider. Couldn't find anything fishy after review.
This revises the scalar multiplication gadgets and internal representation of
Scalar
.BEFORE:
Scalar
used to be represented as 255 bits, which imposed an unavoidable cost of 255 rows on any gadget that wanted to scale a group element by aField
. This is a very typical use case for scaling, seeSignature.verify()
andNullifier.verify()
scaleShifted()
helper which unshifts "in the exponent" at the cost of doing 3 full scalar muls instead of 1.Group.scale()
didn't support -1, 0 and 1 as scalarsAFTER:
Scalar
uses the representation that Pickles uses as well and which our scalar mul gadgets were designed for: 1 low bit and 254 high bits. The high bits are a single field element. The representation is still shifted, but in a slightly different way than before.Field
, and for converting aField
to aScalar
Scalar.fromBits()
performs the shift behind the scenes in provable code now, as doGroup.scale()
andScalar.fromField()
Group.scale()
supports all scalars, and there's a new test to check thatSignature.verify()
now uses 350 instead of 800 rowsCloses #1440
Closes #984
Closes #980