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

chore(crypto): update btcec to v2 #13513

Merged
merged 3 commits into from
Oct 17, 2022
Merged

chore(crypto): update btcec to v2 #13513

merged 3 commits into from
Oct 17, 2022

Conversation

elias-orijtech
Copy link
Contributor

@elias-orijtech elias-orijtech commented Oct 11, 2022

Similar to Tendermint's PR,

tendermint/tendermint#9250

Note that crypto/ledger is not updated in this PR, because if its dependency on the R and S values being exposed by ParseDERSignature.

Closes #13423

CC @odeke-em

@codecov
Copy link

codecov bot commented Oct 11, 2022

Codecov Report

Merging #13513 (97330c8) into main (18d8b64) will decrease coverage by 0.00%.
The diff coverage is 85.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #13513      +/-   ##
==========================================
- Coverage   53.93%   53.93%   -0.01%     
==========================================
  Files         645      645              
  Lines       55681    55679       -2     
==========================================
- Hits        30033    30031       -2     
  Misses      23223    23223              
  Partials     2425     2425              
Impacted Files Coverage Δ
crypto/keys/secp256k1/secp256k1_nocgo.go 64.70% <82.35%> (-18.63%) ⬇️
crypto/hd/hdpath.go 98.40% <100.00%> (ø)
crypto/keys/secp256k1/secp256k1.go 88.63% <100.00%> (ø)
server/rosetta/converter.go 56.75% <100.00%> (ø)
x/staking/simulation/operations.go 75.91% <0.00%> (+1.37%) ⬆️

Copy link
Collaborator

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @elias-orijtech! Kindly pinging @julienrbrt @tac0turtle and others for co-review.

@tac0turtle
Copy link
Member

Can you clarify if this is backwards compatible.

@elias-orijtech
Copy link
Contributor Author

It is backwards compatible. See tendermint/tendermint#9250 (comment)

@odeke-em
Copy link
Collaborator

@elias-orijtech this change brought some good performance improvements for the cryptography benchmarks per https://dashboard.bencher.orijtech.com/benchmark/d892b345308f4a7a82222ee9e6ec1bff
image
image

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

utACK

@@ -122,8 +122,10 @@ github.com/bgentry/speakeasy v0.1.0 h1:ByYyxL9InA1OWqxJqqp2A5pYHUrCiAL6K3J+LKSsQ
github.com/bgentry/speakeasy v0.1.0/go.mod h1:+zsyZBPWlz7T6j88CTgSN5bM796AkVf0kBD4zp0CCIs=
github.com/btcsuite/btcd v0.0.0-20190115013929-ed77733ec07d/go.mod h1:d3C0AkH6BRcvO8T0UEPu53cnw4IbV63x1bEjildYhO0=
github.com/btcsuite/btcd v0.20.1-beta/go.mod h1:wVuoA8VJLEcwgqHBwHmzLRazpKxTv13Px/pDuV7OomQ=
github.com/btcsuite/btcd v0.22.2 h1:vBZ+lGGd1XubpOWO67ITJpAEsICWhA0YzqkcpkgNBfo=
Copy link
Member

Choose a reason for hiding this comment

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

In the meantime we've bumped this to v0.22.2. Shouldn't we keep this version instead of reverting to v0.22.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Done.

@odeke-em
Copy link
Collaborator

@tac0turtle would you like us to merge this change? Thank you.

Similar to Tendermint's PR,

tendermint/tendermint#9250

Note that crypto/ledger is not updated in this PR, because if its
dependency on the R and S values being exposed by ParseDERSignature.

Updates cosmos#13423

Signed-off-by: Elias Naur <elias@orijtech.com>
@tac0turtle tac0turtle merged commit ed9cd41 into cosmos:main Oct 17, 2022
@julienrbrt
Copy link
Member

Seems like we need ledger to be somehow ported too: https://github.com/cosmos/cosmos-sdk/actions/runs/3594121426/jobs/6051975194

yihuang pushed a commit that referenced this pull request Feb 7, 2023
Similar to Tendermint's PR,

tendermint/tendermint#9250

Note that crypto/ledger is not updated in this PR, because if its
dependency on the R and S values being exposed by ParseDERSignature.

Updates #13423

Signed-off-by: Elias Naur <elias@orijtech.com>

Signed-off-by: Elias Naur <elias@orijtech.com>
Co-authored-by: Marko <marbar3778@yahoo.com>
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
Similar to Tendermint's PR,

tendermint/tendermint#9250

Note that crypto/ledger is not updated in this PR, because if its
dependency on the R and S values being exposed by ParseDERSignature.

Updates cosmos#13423

Signed-off-by: Elias Naur <elias@orijtech.com>

Signed-off-by: Elias Naur <elias@orijtech.com>
Co-authored-by: Marko <marbar3778@yahoo.com>
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.

Bump github.com/btcsuite/btcd/btcec to github.com/btcsuite/btcd/btcec/v2
5 participants