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: use btcec/v2 for no-cgo #24533

Merged
merged 1 commit into from
Mar 16, 2022
Merged

crypto: use btcec/v2 for no-cgo #24533

merged 1 commit into from
Mar 16, 2022

Conversation

chappjc
Copy link
Contributor

@chappjc chappjc commented Mar 13, 2022

This updates the no-cgo code in the crypto package to use the new github.com/btcsuite/btcd/btcec/v2 module instead of the older btcec package that was part of the main github.com/btcsuite/btcd module.

This is nice as it removes the dependency on the legacy btcd mega-module that does not yet follow semantic import versioning (has breaking Go API changes between versions like v0.21 and v0.22) with the new btcec/v2 module that does follow SIV, plus it brings performance improvements:

name                   old time/op  new time/op  delta
EcrecoverSignature-32   198µs ± 0%   144µs ± 0%  -27.11%
VerifySignature-32      177µs ± 0%   128µs ± 0%  -27.44%
DecompressPubkey-32    20.9µs ± 0%  10.1µs ± 0%  -51.51%

The tests that passed previously with CGO_ENABLED=0 still pass, and the secp256k1 fuzzing results show no discrepancies with the cgo version.

tests/fuzzers/secp256k1:

$ CGO_ENABLED=0 go-fuzz
2022/03/12 17:58:32 workers: 16, corpus: 60 (3s ago), crashers: 0, restarts: 1/0, execs: 0 (0/sec), cover: 0, uptime: 3s
2022/03/12 17:58:35 workers: 16, corpus: 60 (6s ago), crashers: 0, restarts: 1/0, execs: 0 (0/sec), cover: 949, uptime: 6s
2022/03/12 17:58:38 workers: 16, corpus: 60 (9s ago), crashers: 0, restarts: 1/4357, execs: 209144 (23236/sec), cover: 949, uptime: 9s
...
2022/03/12 17:59:44 workers: 16, corpus: 61 (17s ago), crashers: 0, restarts: 1/9514, execs: 4938271 (65843/sec), cover: 949, uptime: 1m15s
2022/03/12 17:59:47 workers: 16, corpus: 61 (20s ago), crashers: 0, restarts: 1/9592, execs: 5151037 (66038/sec), cover: 949, uptime: 1m18s

The most notable recent breaking change in the main btcd module that was previously required is that with the introduction of the btcec/v2 module, there is no longer a btcec package in the main btcd module, which creates build errors when the latest revisions of btcd are used by either go-ethereum or transitive dependencies. Moving to the btcec/v2 module resolves this issue.

CC @fjl

crypto/signature_nocgo.go Outdated Show resolved Hide resolved
@fjl
Copy link
Contributor

fjl commented Mar 13, 2022

Thanks! I will review tomorrow!

@chappjc chappjc force-pushed the btcec/v2 branch 2 times, most recently from 8e1026c to 2f96c2b Compare March 14, 2022 19:23
@chappjc
Copy link
Contributor Author

chappjc commented Mar 14, 2022

Pushed some minor changes to make it a bit more defensive: checking expected lengths, using package-level consts, and zeroing the temporary btcec.PrivateKey constructed in Sign. diff: https://github.com/ethereum/go-ethereum/compare/8e1026cab147b84a43ab69d03abadf1df7c9e41a..2f96c2bf0a175ca9b893b1073aa81961b4dd5014

This updates the no-cgo implementations in the crypto package to use
the github.com/btcsuite/btcd/btcec/v2 module instead of the older btcec
package that was part of the main github.com/btcsuite/btcd module.

name                   old time/op  new time/op  delta
EcrecoverSignature-32   198µs ± 0%   144µs ± 0%  -27.11%
VerifySignature-32      177µs ± 0%   128µs ± 0%  -27.44%
DecompressPubkey-32    20.9µs ± 0%  10.1µs ± 0%  -51.51%

Use (*ModNScalar).IsOverHalfOrder instead of math/big.Int when checking
for malleable signatures.
Copy link
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

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

I've checked this some more and it works as expected! While performance of btcec still lags behind C libsecp256k1 quite a bit, I can confirm 20% speedup of the non-cgo path with this update.

Thanks again for this work!

@fjl fjl merged commit 830231c into ethereum:master Mar 16, 2022
@fjl fjl added this to the 1.10.17 milestone Mar 16, 2022
@fjl fjl removed the status:triage label Mar 16, 2022
@chappjc chappjc deleted the btcec/v2 branch March 16, 2022 14:27
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Mar 16, 2022
JacekGlen pushed a commit to JacekGlen/go-ethereum that referenced this pull request May 26, 2022
This updates the no-cgo implementations in the crypto package to use
the github.com/btcsuite/btcd/btcec/v2 module instead of the older btcec
package that was part of the main github.com/btcsuite/btcd module.

name                   old time/op  new time/op  delta
EcrecoverSignature-32   198µs ± 0%   144µs ± 0%  -27.11%
VerifySignature-32      177µs ± 0%   128µs ± 0%  -27.44%
DecompressPubkey-32    20.9µs ± 0%  10.1µs ± 0%  -51.51%

Use (*ModNScalar).IsOverHalfOrder instead of math/big.Int when checking
for malleable signatures.
cp-wjhan pushed a commit to cp-yoonjin/go-wemix that referenced this pull request Nov 16, 2022
This updates the no-cgo implementations in the crypto package to use
the github.com/btcsuite/btcd/btcec/v2 module instead of the older btcec
package that was part of the main github.com/btcsuite/btcd module.

name                   old time/op  new time/op  delta
EcrecoverSignature-32   198µs ± 0%   144µs ± 0%  -27.11%
VerifySignature-32      177µs ± 0%   128µs ± 0%  -27.44%
DecompressPubkey-32    20.9µs ± 0%  10.1µs ± 0%  -51.51%

Use (*ModNScalar).IsOverHalfOrder instead of math/big.Int when checking
for malleable signatures.
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Nov 29, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 2, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 2, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 4, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 9, 2024
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.

3 participants