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

[Crypto] Switch to libsecp256k1 signature verification and update the lib #549

Merged
merged 7 commits into from Jun 28, 2018

Conversation

Projects
None yet
5 participants
@Warrows
Copy link
Collaborator

Warrows commented Feb 25, 2018

Here is the long overdue update for PIVX to let go of OpenSSL in its consensus code.
The rationale behind it is to avoid depending on an external and changing library where our consensus code is affected. This is security and consensus critical. It also brings some performances improvements.
EDIT: Actually, we won't be absolutely free of OpenSSL dependencies because of the zerocoin library. But this is an important first step in this direction and zerocoin lib will follow.

The update is made of 6 commits:

  1. The first 3 are libsecp256k1 update. It's now on the latest version available on https://github.com/bitcoin-core/secp256k1. None of the changes are really mine. By the way, the lib is now handled as a git subtree.
  2. 6839f3b Code changes to allow compatibility and actually use this library. That's the bulk of what you'll need to review.
  3. 65e009a New auto generated script tests.
  4. 21234db is an update after rebasing on top of zPOS.

Here are some highlights for commit 6839f3b and the changes I introduce:
-Removed src/arith_uint256.cpp since its a duplicate implementation of src/uint256.cpp.
-Changed src/bip38.cpp to properly call secp256k1.
-Removed src/eccryptoverify.cpp and src/ecwrapper.cpp since they were used o call OpenSSL crypto.
-Change unsigned char[32] to a struct ChainCode.
-Numerous changes to src/key.cpp and src/pubkey.cpp to comply with libsecp256k1, including:
-Bringing in some code about keys import/export
-Change calls from OpenSSL crypto to libsecp256k1
-Introduce some named values instead of magic numbers
-Move keystore functions implementations from .h to .cpp

Possible testing includes pretty much everything requiring signing or verifying signatures.

This PR is unrelated to #447 and contrary to what I first stated there, PIVX is currently not independent from OpenSSL in its consensus code.
After this PR, the libzerocoin code should be reviewed and if possible uncorrelated from OpenSSL as well.

Performances:
I tried to do an IBD on testnet comparison between this PR and master. But the result ended up being surprising since both were at around 1h10. Actually a bit more for this PR.
However, full IBD also depends on other factors such as download speed. We also have almost empty blocks for which the verification time is not determinant in the perfs.
The point here is to check if signature validation is faster. And it is.
For example, on master, verifying testnet block 349533 took me 457 ms while with this PR it took 171 ms.
Since mainnet probably holds more transactions (and hence signatures to check) I expect the IBD to be quicker with this pr on it. Using bootstrap might be much quicker too.

This PR should not generate too much conflicts with #413 or #416 as they stand. Should it arise, I am prepared to rebase this PR or help rebase the others if this one was to be merged first. EDIT: rebase is done.

@Warrows Warrows self-assigned this Feb 25, 2018

@wafflebot wafflebot bot added the review label Feb 25, 2018

@Warrows Warrows force-pushed the Warrows:libsecp256k1_update branch Mar 1, 2018

@Warrows Warrows force-pushed the Warrows:libsecp256k1_update branch 2 times, most recently Mar 4, 2018

@Fuzzbawls Fuzzbawls added this to the Future milestone Mar 15, 2018

Warrows added some commits May 9, 2018

Squashed 'src/secp256k1/' content from commit 452d8e4d2
git-subtree-dir: src/secp256k1
git-subtree-split: 452d8e4d2a2f9f1b5be6b02e18f1ba102e5ca0b4
[Crypto] Switch from openssl to secp256k1 for consensus
[Refactoring] Moved and removed some stuff
-Removed duplicated arith uint files
-Removed unused variables
-Move keystore impls to .cpp instead of .h
-Removed useless function in key.cpp
[Crypto] fix bip38 compilation for latest libsecp256k1
[Compilation] Change compilation and some code to use libsec instead of sslcrypto
[Crypto] Update keys to comply with latest secp256k1 lib

@Warrows Warrows force-pushed the Warrows:libsecp256k1_update branch to 802c354 May 9, 2018

@Warrows

This comment has been minimized.

Copy link
Collaborator Author

Warrows commented May 9, 2018

UPDATE: Rebased this PR on top of current master. Merge conflicts with zPoS have been resolved.

@Warrows Warrows modified the milestones: Future, 3.2.0 May 17, 2018

@Warrows Warrows force-pushed the Warrows:libsecp256k1_update branch from 802c354 to 21234db May 28, 2018

@Fuzzbawls

This comment has been minimized.

Copy link
Collaborator

Fuzzbawls commented Jun 1, 2018

been running with this for the past few days and haven't seen any noticeable issues

@Warrows Warrows requested a review from rejectedpromise Jun 6, 2018

@rejectedpromise
Copy link
Collaborator

rejectedpromise left a comment

Looks like some good changes with both clarity and efficiency!
utACK

@jonspock

This comment has been minimized.

Copy link

jonspock commented Jun 9, 2018

Apart from libzerocoin, is there a need to retain dependency for OpenSSL in crypter.cpp?

//passpoint is the ec_mult of passfactor on secp256k1
int clen = 65;
return secp256k1_ec_pubkey_create(UBEGIN(passpoint), &clen, passfactor.begin(), true) != 0;
if (!secp256k1_ec_pubkey_create(ctx, &pubkey, passfactor.begin()))

This comment has been minimized.

Copy link
@jonspock

jonspock Jun 10, 2018

Looks like a potential issue here. You are passing a NULL pointer ctx to this function

This comment has been minimized.

Copy link
@Warrows

Warrows Jun 10, 2018

Author Collaborator

Great catch, I'll fix it.

@Warrows

This comment has been minimized.

Copy link
Collaborator Author

Warrows commented Jun 10, 2018

@jonspock crypter.cpp is a different question. It's not directly consensus related. But yes, removing openSSL there would be nice too and it should simply be a backport of bitcoin@976f9ec. I don't think libzerocoin has any influence there.

@Warrows Warrows force-pushed the Warrows:libsecp256k1_update branch to f10439c Jun 11, 2018

@jonspock

This comment has been minimized.

Copy link

jonspock commented Jun 11, 2018

Thanks. Unfortunately the update is not as simple as the diff, but thanks for the hint. Yes, I realized it wasn't consensus related but brought it up because you said " Actually, we won't be absolutely free of OpenSSL dependencies because of the zerocoin library. "

@rejectedpromise
Copy link
Collaborator

rejectedpromise left a comment

Haven't run into any problems
ACK

@Warrows

This comment has been minimized.

Copy link
Collaborator Author

Warrows commented Jun 19, 2018

Are we waiting on any more reviews on this?

@Mrs-X

Mrs-X approved these changes Jun 28, 2018

Copy link
Collaborator

Mrs-X left a comment

utACK (haven't reviewed ALL 120 files, though) and merging...

@Mrs-X Mrs-X merged commit f10439c into PIVX-Project:master Jun 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Mrs-X added a commit that referenced this pull request Jun 28, 2018

Merge #549: [Crypto] Switch to libsecp256k1 signature verification an…
…d update the lib

f10439c [Crypto] Add ctx initialisation for bip38 (warrows)
21234db [Crypto] Bring back function CKey.SetPrivKey for zPIV (warrows)
65e009a [Tests] Add new auto generated script tests (warrows)
6839f3b [Crypto] Switch from openssl to secp256k1 for consensus (warrows)
8a901f9 Squashed 'src/secp256k1/' content from commit 452d8e4d2 (warrows)
d98a584 [Refactor] Delete secp256k1 folder for subtreefication (warrows)

Tree-SHA512: f0f6777be57777ba86f83af1b891a6c0f384e6b059afc9249599269c71e5d3bf46a6498325488878af71b6685c6dac6cb672d0147c2ebf43b36f6d786fc38a10

@wafflebot wafflebot bot removed the review label Jun 28, 2018

@Warrows Warrows deleted the Warrows:libsecp256k1_update branch Jun 29, 2018

@Fuzzbawls Fuzzbawls modified the milestones: 3.2.0, 3.1.1 Jul 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.