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

go: Use a faster ed25519 implementation #1533

Merged
merged 1 commit into from
Oct 22, 2019

Conversation

Yawning
Copy link
Contributor

@Yawning Yawning commented Mar 18, 2019

This should force everything to use our ed25519 and x25519.

@Yawning Yawning added p:1 Priority: core feature c:common Category: common libraries golang labels Mar 18, 2019
@Yawning Yawning self-assigned this Mar 18, 2019
@codecov
Copy link

codecov bot commented Mar 18, 2019

Codecov Report

Merging #1533 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1533      +/-   ##
==========================================
+ Coverage   52.94%   52.95%   +0.01%     
==========================================
  Files         272      272              
  Lines       27185    27185              
==========================================
+ Hits        14393    14397       +4     
+ Misses      11314    11311       -3     
+ Partials     1478     1477       -1
Impacted Files Coverage Δ
go/common/crypto/mrae/api/api.go 22.91% <ø> (ø) ⬆️
...n/crypto/signature/signers/memory/memory_signer.go 55.26% <ø> (ø) ⬆️
go/common/crypto/signature/signature.go 37.07% <ø> (ø) ⬆️
...ommon/crypto/signature/signers/file/file_signer.go 54.21% <ø> (ø) ⬆️
go/scheduler/tendermint/tendermint.go 66.32% <0%> (+4.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f478ec...23e684a. Read the comment docs.

@Yawning Yawning force-pushed the yawning/feature/faster-ed25519 branch 2 times, most recently from 87ea723 to edc8ed7 Compare April 16, 2019 09:29
@Yawning Yawning force-pushed the yawning/feature/faster-ed25519 branch from edc8ed7 to 21565b5 Compare May 28, 2019 11:55
@Yawning Yawning force-pushed the yawning/feature/faster-ed25519 branch from 21565b5 to eccb752 Compare July 1, 2019 13:48
@Yawning Yawning force-pushed the yawning/feature/faster-ed25519 branch from eccb752 to 625fa4c Compare September 18, 2019 11:31
@Yawning
Copy link
Contributor Author

Yawning commented Sep 18, 2019

Bring this up to date again.

While this branch has been sitting waiting for the courage to merge it, Go 1.13 has promoted the slow x/crypto/ed25519 code to crypto/ed25519. While this isn't a problem for us, if our dependencies switch to using the standard library import path, it will limit our ability to override the implementation.

Because this only really matters for tendermint, we can always just define our own Ed25519 implementation that tendermint will use, especially if we want to switch to ph or ctx there as well (and we should).

Edit: Can't define custom key types in tendermint, because crypto/encoding/amino is poorly implemented.

@Yawning Yawning force-pushed the yawning/feature/faster-ed25519 branch 3 times, most recently from e10e504 to 486f250 Compare September 19, 2019 12:01
@Yawning Yawning added the s:blocked Status: blocked on other work label Sep 19, 2019
@Yawning Yawning force-pushed the yawning/feature/faster-ed25519 branch 2 times, most recently from 069b706 to a0b1deb Compare October 4, 2019 13:06
@Yawning Yawning force-pushed the yawning/feature/faster-ed25519 branch 5 times, most recently from f3dcddc to fb9f690 Compare October 17, 2019 13:37
@Yawning
Copy link
Contributor Author

Yawning commented Oct 17, 2019

For what it's worth, I'm of the opinion that this is good to go, the moment we get the ed25519 code cleared.

@Yawning Yawning marked this pull request as ready for review October 17, 2019 13:51
@Yawning Yawning force-pushed the yawning/feature/faster-ed25519 branch from fb9f690 to 3182e32 Compare October 22, 2019 07:32
Copy link
Contributor

@peterjgilbert peterjgilbert left a comment

Choose a reason for hiding this comment

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

🎉

@Yawning Yawning force-pushed the yawning/feature/faster-ed25519 branch from 3182e32 to 23e684a Compare October 22, 2019 15:57
@Yawning Yawning removed the s:blocked Status: blocked on other work label Oct 22, 2019
@Yawning Yawning merged commit 8c56b73 into master Oct 22, 2019
@Yawning Yawning deleted the yawning/feature/faster-ed25519 branch October 22, 2019 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:common Category: common libraries golang p:1 Priority: core feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants