[Crypto] Switch to libsecp256k1 signature verification and update the lib #549
Conversation
Warrows
added
enhancement
Wallet
Backport
Needs Release Notes
ghost
added
the
Warrows
removed
Backport
Needs Release Notes
Warrows
force-pushed
the
libsecp256k1_update
branch
from
9e9b604 to
ff0c4ad
Compare
Warrows
force-pushed
the
libsecp256k1_update
branch
2 times, most recently
from
90345b4 to
71179d4
Compare
git-subtree-dir: src/secp256k1 git-subtree-split: 452d8e4
[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
force-pushed
the
libsecp256k1_update
branch
from
71179d4 to
802c354
Compare
|
UPDATE: Rebased this PR on top of current master. Merge conflicts with zPoS have been resolved. |
Warrows
force-pushed
the
libsecp256k1_update
branch
from
802c354 to
21234db
Compare
|
been running with this for the past few days and haven't seen any noticeable issues |
rejectedpromise
left a comment
rejectedpromise
left a comment
There was a problem hiding this comment.
Looks like some good changes with both clarity and efficiency!
utACK
|
Apart from libzerocoin, is there a need to retain dependency for OpenSSL in crypter.cpp? |
src/bip38.cpp
Outdated
| //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())) |
There was a problem hiding this comment.
Looks like a potential issue here. You are passing a NULL pointer ctx to this function
|
@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
force-pushed
the
libsecp256k1_update
branch
from
49b95da to
f10439c
Compare
|
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
left a comment
rejectedpromise
left a comment
There was a problem hiding this comment.
Haven't run into any problems
ACK
Fuzzbawls
added
the
Needs Release Notes
|
Are we waiting on any more reviews on this? |
Mrs-X
left a comment
Mrs-X
left a comment
There was a problem hiding this comment.
utACK (haven't reviewed ALL 120 files, though) and merging...
…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 452d8e4 (warrows) d98a584 [Refactor] Delete secp256k1 folder for subtreefication (warrows) Tree-SHA512: f0f6777be57777ba86f83af1b891a6c0f384e6b059afc9249599269c71e5d3bf46a6498325488878af71b6685c6dac6cb672d0147c2ebf43b36f6d786fc38a10
Fuzzbawls
removed
Needs Release Notes
5 participants
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:
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.