ed25519: use golang/x/crypto fork #2558
Conversation
crypto/ed25519/ed25519.go
Outdated
| return PubKeyEd25519(pubkeyBytes) | ||
|
|
||
| if !initialized { | ||
| panic("pubkey bytes should always be initialized!") |
There was a problem hiding this comment.
This may not be true with multi-language support for generating private keys. The pubkey in the private key is an optimization, but isn't "canonical" as far as what is described in the paper. I know dalek requires the pubkey to be inputted along with the private key when signing, so it wouldn't be included in marshalling.
I'm in favor of keeping the old code here though.
There was a problem hiding this comment.
So the only place we read in a priv key is the priv_validator.json and node_key.json. I think we can just make it a requirement that these come in the format that includes the pubkey? Then we don't have to worry here.
Think that's legit?
There was a problem hiding this comment.
Having the pubkey inside of the encoded private key for ed25519 is a design quirk they had to do due to the limited interface of cryptosystems within golang. I think we should support using the normal format as well. We will be reading ed25519 privkeys from gaiacli as well eventually, and I think its reasonable to expect more than just the sdk may be writing to gaiacli's privkeys?
Its not that big of a deal, so I guess we could just remove that support. (But then the error message should probably give a lot more detail, and will make it up to the user to debug this quirk)
Codecov Report
@@ Coverage Diff @@
## develop #2558 +/- ##
===========================================
+ Coverage 61.29% 61.44% +0.15%
===========================================
Files 203 203
Lines 16815 16759 -56
===========================================
- Hits 10307 10298 -9
+ Misses 5637 5587 -50
- Partials 871 874 +3
|
| @@ -72,6 +72,11 @@ | |||
| ## Some repos dont have releases. | |||
| ## Pin to revision | |||
|
|
|||
| [[constraint]] | |||
| name = "golang.org/x/crypto" | |||
There was a problem hiding this comment.
not a fan of aliasing dependencies. if I am a developer and I see an import "github.com/.../...", I usually go to github.com to check out the source code for that import. But here you get what you see concept is broken, so one had to be aware of the alias ??somehow??
There was a problem hiding this comment.
Ok - would a comment everywhere we import this that it's actually an alias be sufficient?
There was a problem hiding this comment.
Yep. Or we could indicate that it's a fork by appending fork or tendermint to name:tendermint.com/golang.org/x/crypto ||golang.tendermint.com/x/crypto
There was a problem hiding this comment.
Hmm - I like this idea but dep doesn't want to let me do it so long as these URLs don't exist. Any ideas? Or do we need to create the valid endpoint first?
There was a problem hiding this comment.
I like this idea but dep doesn't want to let me do it so long as these URLs don't exist
did not know that! probably not worth the efforts short-term
There was a problem hiding this comment.
I don't really get why we don't just import tendermint/crypto everywhere
There was a problem hiding this comment.
Golang forking voodoo. It breaks things like internal packages
There was a problem hiding this comment.
ohh, ok this lgtm then
There was a problem hiding this comment.
This looks good to me, though we may still want more clarification surrounding the name aliasing. (i.e. rational in the dep file perhaps, and changing the comment from // forked to github.com/tendermint/crypto to something perhaps like // code lies in the fork github.com/tendermint/crypto
|
Ok, opened an issue for follow up: #2608 |
Closes #1959.