-
Notifications
You must be signed in to change notification settings - Fork 59
[feat] Add support for Twisted Edwards Curves into the elliptic curve VM extension #1255
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
base: develop
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
39db1c0
to
87757aa
Compare
This comment has been minimized.
This comment has been minimized.
extensions/ecc/circuit/Cargo.toml
Outdated
@@ -21,6 +21,7 @@ openvm-rv32-adapters = { workspace = true } | |||
openvm-ecc-transpiler = { workspace = true } | |||
|
|||
num-bigint = { workspace = true } | |||
num-bigint-dig = { workspace = true } |
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 got rid of num-bigint-dig, let's only use num-bigint from now on
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.
num-bigint-dig has jacobi symbol computation while num-bigint doesn't. I'll try to replicate it with num-bigint
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 need jacobi symbol for checking if a is QR and d is not QR for completeness
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.
hm... that is really unfortunate (I don't want two bigint crates, and num-bigint-dig
is less maintained than num-bigint
) let me investigate
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 forgot to update you, but I have removed the dependency on num-bigint-dig
. I copied over the jacobi symbol code into extensions/ecc/circuit/src/edwards_chip/utils.rs
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3672c6b
to
0d99466
Compare
4bb302d
to
963e4f7
Compare
b6aaa08
to
e46b959
Compare
ad295b3
to
2fbe8e7
Compare
FYI: I added some more changes that fix a bug that I found (weierstrass and edwards opcodes would overlap in some cases) I also added decompression hints (just like for weierstrass) to the edwards curves since I will use it in eddsa |
ff27e26
to
c689ccc
Compare
Update: rebased onto develop |
…weierstrass and edwards curves)
Co-authored-by: Jonathan Wang <[email protected]>
Co-authored-by: Jonathan Wang <[email protected]>
A bug involving opcode collisions between short Weierstrass and twisted Edwards curves was found. To fix this, CurveConfig was rewritten and separate opcodes were given to the two types of curves.
840135f
to
9a1c396
Compare
Commit: 9a1c396 |
// ANCHOR_END: imports | ||
openvm_algebra_guest::moduli_macros::moduli_declare! { | ||
// The Secp256k1 modulus and scalar field modulus are already declared in the k256 module |
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.
oudated comment
}); | ||
|
||
#[derive(Clone, Debug, derive_new::new, Serialize, Deserialize)] | ||
pub struct WeierstrassExtension { | ||
pub supported_curves: Vec<CurveConfig>, | ||
pub struct EccExtension { |
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 can see it's easier to say "ECC Extension" but here I feel like we could have done WeierstrassExtension
and TwistedEdwardsExtension
as two separate structs. I don't have strong feelings either way
pub struct WeierstrassExtension { | ||
pub supported_curves: Vec<CurveConfig>, | ||
pub struct EccExtension { | ||
pub supported_sw_curves: Vec<CurveConfig<SwCurveCoeffs>>, |
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.
another option is to keep a single vector but use enum to specify different curve types within CurveConfig
. That seems like a might be a little nicer ux wise?
Primary change:
mod-builder
frameworka
andd
from the equation of a Twisted Edwards curve are correcta
is a quadratic residue and thatd
is not a quadratic residue. This property of a Twisted Edwards curve ensures that the addition operation is the same for all input pointsRelated changes:
mod-builder
framework to handle setup rows that verify more than one constantmod-builder
framework to the new method of padding rows. That is, by using a temporary range checker and constructing a dummy row (see fix: EcDoubleChip dummy row #1239)mod-builder
frameworkCurveConfig
struct to accommodate for curves in Twisted Edwards curve formCloses INT-2999