Skip to content
This repository was archived by the owner on Feb 8, 2018. It is now read-only.

Use uECC instead of Crypto++ for ECDH#11

Closed
arkpar wants to merge 2 commits intoethereum:developfrom
arkpar:crypto
Closed

Use uECC instead of Crypto++ for ECDH#11
arkpar wants to merge 2 commits intoethereum:developfrom
arkpar:crypto

Conversation

@arkpar
Copy link
Copy Markdown
Contributor

@arkpar arkpar commented Sep 22, 2015

No description provided.

@arkpar
Copy link
Copy Markdown
Contributor Author

arkpar commented Sep 22, 2015

One of the steps to get rid of libcrypto++
library source: https://github.com/kmackay/micro-ecc

@subtly
Copy link
Copy Markdown
Member

subtly commented Sep 23, 2015

Nice! Will test on the various OSs (hopefully jenkins can do this too!)

Comment thread libdevcrypto/Common.cpp
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we should remove this check and/or may need to add a flag (default to verify) for toPublic.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The check is done in toPublic anyway, it will return zero in case of invalid key. As far as I understand crypto++ validate() checks for curve validity, which is not needed for libsecp256k1 anyway and also checks that the point is valid on the curve, which secp256k1_ec_pubkey_create also does.
The previous behavior of this function for invalid secret was to set m_secret to invalid key, but leave m_public and m_address to the previous value. With this change m_public and m_address will be set to zero, which is better since these don't throw anyway.

@chfast
Copy link
Copy Markdown
Member

chfast commented Sep 30, 2015

Maybe a newbe question, but why not ECDH from secp256k1?

@arkpar
Copy link
Copy Markdown
Contributor Author

arkpar commented Sep 30, 2015

Last time I checked It was not supported. At least the version in webthree-helpers does not have any API for ECDH

@chfast
Copy link
Copy Markdown
Member

chfast commented Sep 30, 2015

It is implemented as a module in upstream repo:
https://github.com/bitcoin/secp256k1/blob/master/src/modules/ecdh/main_impl.h

Can you verify that this short implementation is good for us?

@arkpar
Copy link
Copy Markdown
Contributor Author

arkpar commented Sep 30, 2015

This header is not enough, we need to update the library. I will try and do that

@chfast
Copy link
Copy Markdown
Member

chfast commented Sep 30, 2015

@subtly had opinions about upgrading secp256k1.

@Gustav-Simonsson
Copy link
Copy Markdown

Go integration of libsecp256k1 ECDH: ethereum/go-ethereum#1862
It modifies the source to return the point x value before the SHA256 hashing, since RLPx uses it's own hashing of the raw secret.

@arkpar
Copy link
Copy Markdown
Contributor Author

arkpar commented Oct 1, 2015

dropped in favor of libsecp256k1

@arkpar arkpar closed this Oct 1, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants