-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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/secp256k1: Remove btcsuite intermediary. #1689
crypto/secp256k1: Remove btcsuite intermediary. #1689
Conversation
This updates the core/crypto/secp256k1 code to make use of the dcrec/secp256k1/v4 module directly instead of using btcec/v2 which itself is just a shim around dcrec/secp256k1/v4 anyway. This has the benefit of removing the additional github.com/btcsuite/btcd/chaincfg/chainhash dependency since dcrec/secp256k1/v4 is its own module and does rely on it. It also updates to the latest v4.1.0 release which implements direct key generation and has some other nice optimizations that speed up signature verification as compared to the v4.0.1 release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thx (even tho it's a bit late)
I was actually self debating doing this while making #1686 but didn't because I liked that I didn't actually touched any of |
@Jorropo Sure thing. I agree that it makes more sense to have it as a separate PR like this versus doing it as a part of #1686. I typically am very much in favor of keeping things more isolated and methodical versus commingling concerns in a single PR. Having it separate means 1686 is able to focus solely on supporting |
All I had to do is bump |
I noticed the CI checks seem to have timed out. I just restored the branch if you're planning on triggering them to run again. |
This updates the
core/crypto/secp256k1
code to make use of thedcrec/secp256k1/v4
module directly instead of usingbtcec/v2
which itself is now just a shim arounddcrec/secp256k1/v4
anyway.This has the benefit of removing the additional transitive
github.com/btcsuite/btcd/chaincfg/chainhash
dependency sincedcrec/secp256k1/v4
is its own module and does not rely on it.It also updates to the latest
v4.1.0
release which implements direct key generation and has some other nice optimizations that speed up signature verification as compared to thev4.0.1
release.This was originally opened as libp2p/go-libp2p-core#280 and is being reopened here as requested.
Also, per discussion on the referenced PR, I confirmed that this is actually not a breaking change to the API despite changing the type aliases because the types in
btcec/v2
are themselves just type aliases to the same underlying type this changes them to. For example, see the btcec/v2.PrivateKey type declaration which is an alias fordcrec/secp256k1/v4.PrivateKey
. The same is true for btcec/v2.PublicKey.In order to check that is indeed the case, I intentionally kept the type assertion in
KeyPairFromStdKey
as*btcec.PrivateKey
and ran theTestKeyPairFromKey
test which generates the key withsecp256k1.GeneratePrivateKey
(which is actually a*secp256k1.PrivateKey
) and then callsKeyPairFromStdKey
with it to ensure they are considered identical by the compiler.Here is the diff relative to this PR to be more concrete about what I just explained:
$ go test -run=TestKeyPairFromKey PASS ok github.com/libp2p/go-libp2p/core/crypto 0.474s
Closes libp2p/go-libp2p-core#11.