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

Use Hal's optimized secp256k1 verifier #2061

Closed
wants to merge 9 commits into from
Closed

Use Hal's optimized secp256k1 verifier #2061

wants to merge 9 commits into from

Conversation

@sipa
Copy link
Member

sipa commented Dec 2, 2012

The first commit is a rebased version of Hal's feb 2011 patch. The second commit improves the code a bit (precalculate constants, and use BN_CTX_get for temporary values).

This reduces reindexing time for the first 210k blocks (script checks enabled everywhere, 4 verification threads, -dbcache=900) from 1h14m to 1h1m on my system.

@gavinandresen

This comment has been minimized.

Copy link
Contributor

gavinandresen commented Dec 3, 2012

I don't think the 20% speedup is worth the extra code complexity. I could be convinced if there are some EC crypto experts hanging around who will chime in and say "oh, yeah, that's an obvious optimization and implementation looks correct...."

It seems to me that this type of low-level speedup would be better implemented in OpenSSL. I don't know if they would accept a patch to speed up one curve or not, but ideally I think that is where this code belongs.

@gmaxwell

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Dec 3, 2012

Hal claims that it can be increased to 40% with some other changes, but they weren't immediately clear to me. I think Pieter's plan was to get this in (as it has the structural changes) and then talk to an EC expert he may have access to about doing the rest. That might satisfy both your concerns.

I would note that script checking all txn with Hal is similar in performance to script skipping before the checkpoint without Hal (if not faster). In terms of the uncertainty wrt security implications I'd prefer the former.

@gmaxwell

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Dec 3, 2012

I was a little over eager in my last claim there: syncing from start to 210000 the current parallel checking branch without hal is 23:58 while without checkpoints but with hal it's 37:10 for me (47:21 hal-less).

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 6, 2012

If we decide to include low-level crypto code like this, we could just as well include all the ECDSA code (for the particular curve that we use) so that we can build with OpenSSL built without ECDSA.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Dec 8, 2012

@laanwj I believe there is quite some non-ECDSA-specific EC code left in OpenSSL that would need to be included in that case too...

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Dec 9, 2012

Refactored the optimized algorithm into an almost exact copy of OpenSSL's own ecdsa_do_verify() function, but using an optimized version of EC_POINT_mul().

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Dec 10, 2012

New commit: if compiled with -DVERIFY_OPTIMIZED_SECP256K1, checks will be compiled in that compare the generic OpenSSL code with the specialized one. It's not enabled by default, but I verified it for the entire current block chain & unit tests.

@Diapolo

This comment has been minimized.

Copy link

Diapolo commented Dec 12, 2012

@sipa This pull can be tested independently from your other one with parallel verification or do they depend on eachother?

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Dec 12, 2012

They're independent.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Dec 15, 2012

Added verification code for checking k == k1 + lambda_k2 and for checking p2 == lambda_p. Verified against unit tests and testnet.

EDIT: and mainnet now as well.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Dec 25, 2012

New commit: implemented a small improvement suggested by Hal.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Jan 24, 2013

Added a commit to build the core .o files for tests separately, and add - DVERIFY_OPTIMIZED_SECP256K1 to them, so the unit tests now compare the internal values in ECDSA verification between optimized and non-optimized code.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Jan 30, 2013

Added a fuzzer that compares intermediate values during validation in optimized and non-optimized code, for message hashes with a random 1-bit difference for every real check.

@BitcoinPullTester

This comment has been minimized.

Copy link

BitcoinPullTester commented Jan 30, 2013

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/1f369d512b5b5c66e9623d2eaf9692eee9b11d36 for binaries and test log.

@SergioDemianLerner

This comment has been minimized.

Copy link
Contributor

SergioDemianLerner commented Feb 1, 2013

It would be good if someone checks this new implementation against timing attacks. Systems that automatically sign transactions (like exchanges) may be vulnerable to key recovery using timing attacks.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Feb 1, 2013

This code isn't used for signing.

@SergioDemianLerner

This comment has been minimized.

Copy link
Contributor

SergioDemianLerner commented Feb 2, 2013

Ok, I will check against specially crafted pubkeys/signatures in a few weeks. I've found bugs in other implementations.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Feb 2, 2013

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Feb 24, 2013

Rebased.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Mar 4, 2013

Added precomputation of G (doable as a separate pull if necessary), which improves verify performance by 2-3% (consistently), and turn off Hal's optimizations by default; -turbo turns them on.

@Diapolo

This comment has been minimized.

Copy link

Diapolo commented May 2, 2013

How do these pulls get tagged updated, when I see no changes here? Rebase?

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented May 2, 2013

I don't intend to keep this updated, as I'm working on a separately library that implements ECDSA directly, with much more optimizations than this pullreq does. See http://github.com/sipa/secp256k1

@Diapolo

This comment has been minimized.

Copy link

Diapolo commented May 2, 2013

You missunderstood my comment, this pull or issue was listed updated for me and I asked what made Github think it was updated. I think your work on this is great, but my intention was just to understand Github here.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented May 2, 2013

@Diapolo Github hiccup, I guess.

@sipa sipa closed this May 2, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.