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

feat: Add ECDSA #310

Merged
merged 19 commits into from
Jan 25, 2023
Merged

feat: Add ECDSA #310

merged 19 commits into from
Jan 25, 2023

Conversation

yelhousni
Copy link
Collaborator

@yelhousni yelhousni commented Jan 18, 2023

Adds ECDSA scheme using gnark-crypto primitives on all curves. This follows "crypto/ecdsa", FIPS 186-4 and SEC1 v2.

todos:

  • use fr.Inverse() instead of bigInt.ModInverse() (experimented speedup is just ~1-2% so need to dig more why)
  • Implement Strauss-Shamir trick [a]P + [b]Q for fast verification (might also look into JSF encoding of scalars if perf is critical)

@yelhousni
Copy link
Collaborator Author

  • Strauss-Shamir trick (without GLV)

Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

Looks good. I think you should copy input scalars to JointScalarMultiplication. In usual ScalarMul, you get the scalars from GLV decomposition and they are safe to modify locally. For ECDSA it is safe as you create big.Int inputs freshly, but if anyone would use JointScalarMul separately, then it would mutate its inputs.

internal/generator/ecc/template/point.go.tmpl Outdated Show resolved Hide resolved
internal/generator/ecc/template/point.go.tmpl Outdated Show resolved Hide resolved
Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

To record conversation - maybe it is better to use fr.Element in Signature and PrivateKey types.

Also, it seems params type isn't strictly necessary - we have all the information in the curve template anyway. And add add signature/ecdsa package which calls the actual implementation depending on the curve.

internal/generator/ecdsa/template/ecdsa.go.tmpl Outdated Show resolved Hide resolved
internal/generator/ecdsa/template/ecdsa.go.tmpl Outdated Show resolved Hide resolved
@yelhousni yelhousni requested a review from ivokub January 24, 2023 11:46
Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

LGTM

@yelhousni yelhousni merged commit 9b7bfdb into develop Jan 25, 2023
@yelhousni yelhousni deleted the feat/ecdsa branch January 25, 2023 16:37
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.

3 participants