Skip to content

Conversation

@Gustav-Simonsson
Copy link

Depends on #1853 - please review & merge it first.

NOTE: package crypto/ecies needs some refactoring love - this patch is not ideal as package ecies is a bit messy. Discussed with @fjl and we have several ideas around how to refactor it. Would prefer to make that refactoring in a separate PR so it's easier to review separated from this change.

  • Add libsecp256k1 Scalar Multiplication API based on it's ECDH API and use this in the curve interface function ScalarMult for this curve.
  • Add benchmark of Scalar Multiplication (lib + Go wrapping code)
  • Add shared secret test with static values to catch errors when integrating.
  • Move Go secp256k1 curve to crypto/secp256k1 so it can be used in crypto/ecies package (needed to run tests with the correct curve - currently tests are run with another curve - P256)
  • Fix RLPx test setup to avoid hanging in case of errors returned from crypto libs
  • Some boilerplate added to squeeze this curve into current ECIES impl.

Speedup:

benchmark                     old ns/op     new ns/op     delta
BenchmarkGenSharedKeyS256     6523809       165052        -97.47%

@robotally
Copy link

Vote Count Reviewers
👍 1 @fjl
👎 0

Updated: Wed Dec 2 11:48:57 UTC 2015

@Gustav-Simonsson
Copy link
Author

Looks like P2P RLPx tests fails sometimes with this patch - in progress until we figure out why.

@obscuren
Copy link
Contributor

Nice speedup :)

@obscuren
Copy link
Contributor

@Gustav-Simonsson any update on this?

@codecov-io
Copy link

Current coverage is 46.72%

Merging #1862 into develop will decrease coverage by -0.53% as of 352acec

Powered by Codecov. Updated on successful CI builds.

@Gustav-Simonsson Gustav-Simonsson force-pushed the libsecp256k1_ecdh branch 2 times, most recently from b2159b3 to 95b2317 Compare November 26, 2015 01:47
crypto/crypto.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is no longer required since you added an entry for secp256k1 in the default params registry.

@Gustav-Simonsson
Copy link
Author

@obscuren updated, seems to work as expected. the windows build failure is unrelated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this code into the curve method.

@Gustav-Simonsson Gustav-Simonsson force-pushed the libsecp256k1_ecdh branch 2 times, most recently from 51df9ee to 5ff85a8 Compare November 26, 2015 18:12
@fjl
Copy link
Contributor

fjl commented Nov 26, 2015

👍

@fjl fjl changed the title Libsecp256k1 ecdh crypto, crypto/secp256k1: use libsecp256k1 for scalar multiplication Nov 26, 2015
@obscuren
Copy link
Contributor

I think needs some rebasing

@Gustav-Simonsson Gustav-Simonsson force-pushed the libsecp256k1_ecdh branch 3 times, most recently from e6b5f93 to c8ad64f Compare November 30, 2015 14:14
@fjl
Copy link
Contributor

fjl commented Dec 2, 2015

@obscuren this is rebased now

obscuren added a commit that referenced this pull request Dec 2, 2015
crypto, crypto/secp256k1: use libsecp256k1 for scalar multiplication
@obscuren obscuren merged commit 888e7bc into ethereum:develop Dec 2, 2015
@obscuren obscuren removed the review label Dec 2, 2015
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.

5 participants