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

Replace crypto library (secp256k1->elliptic) #184

Closed
gurrpi opened this issue Mar 3, 2019 · 3 comments
Closed

Replace crypto library (secp256k1->elliptic) #184

gurrpi opened this issue Mar 3, 2019 · 3 comments

Comments

@gurrpi
Copy link

@gurrpi gurrpi commented Mar 3, 2019

Currently ethereumjs uses secp256k1 for crypto functionality(ECDSA sign, verify...and so on). But this library is incomplete as it could only verify lower-s signature (See also cryptocoinjs/secp256k1-node#90).
Therefore a function like isValidSignature would return false when input signature is high-s although it is a valid signature but unsupported by secp256k1. So I suggest using elliptic that support full compatibility.

@gurrpi gurrpi changed the title Replace crypto library(secp256k1->elliptic) Replace crypto library (secp256k1->elliptic) Mar 3, 2019
@s1na

This comment has been minimized.

Copy link
Contributor

@s1na s1na commented Mar 3, 2019

Do you know by any chance how the performance compares? Btw, do you have a valid input signature which would fail, so we can add a test case?

@fanatid

This comment has been minimized.

Copy link
Member

@fanatid fanatid commented Mar 4, 2019

@gurrpi why not use signatureNormalize (convert signature to lower-S) before validation?

@s1na elliptic very slow compare to bindings, you can check results for bindings/wasm/elliptic here: https://blog.bitjson.com/just-released-webassembly-version-of-secp256k1-10x-faster-than-javascript-eb3cebe4d411

@gurrpi

This comment has been minimized.

Copy link
Author

@gurrpi gurrpi commented Mar 9, 2019

@fanatid Thanks for informing benchmark.

Closing this issue since Ethereum also only accept low-s signature. (ref. EIP-2)

@gurrpi gurrpi closed this Mar 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.