Skip to content
This repository was archived by the owner on Feb 2, 2019. It is now read-only.

Conversation

@odeke-em
Copy link

@odeke-em odeke-em commented Sep 7, 2017

MakePublicKey takes in a privateKey and mutates it setting
the first 32 bytes to those original 32 bytes used to
generate the publicKey.
This wasn't made clear in the comments and required digging
into the actual implementation when I was advocating for
tendermint/go-crypto#31.
I noticed that in crypto
https://github.com/tendermint/go-crypto/blob/1bc8de4caa844f8b64c120e65b047898f22b7f3e/priv_key.go#L99-L114

there was an unexplained call to ed25519.MakePublicKey nor were
there any comments to explain that.

MakePublicKey takes in a privateKey and mutates it setting
the first 32 bytes to those original 32 bytes used to
generate the publicKey.
This wasn't made clear in the comments and required digging
into the actual implementation when I was advocating for
tendermint/go-crypto#31.
I noticed that in crypto
https://github.com/tendermint/go-crypto/blob/1bc8de4caa844f8b64c120e65b047898f22b7f3e/priv_key.go#L99-L114

there was an unexplained call to ed25519.MakePublicKey nor were
there any comments to explain that.
@odeke-em
Copy link
Author

odeke-em commented Sep 7, 2017

/cc @ebuchman

Copy link

@ebuchman ebuchman left a comment

Choose a reason for hiding this comment

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

I don't understand why we actually need this function in the first place and this fork of the lib.

Looks like we're missing some recent preformance improvements from upstream. I'll ask Jae why we needed to do this, but looks to me like we should just use the upstream GenerateKey ...

// MakePublicKey makes a publicKey from the first half of privateKey.
// MakePublicKey makes a publicKey from the first 32 bytes
// of privateKey, and at the end mutates privateKey setting
// its first 32 bytes to those of the generated publicKey.

Choose a reason for hiding this comment

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

its second 32 bytes

@odeke-em
Copy link
Author

Thanks @ebuchman for taking a look. Ooh in that case perhaps we should think about moving all our code over to use the upstream package.

@odeke-em
Copy link
Author

In regards to code depending on this library, we've got:

Github search

https://github.com/search?l=Go&q=org%3Atendermint+%22tendermint%2Fed25519%22&type=Code&utf8=%E2%9C%93
screen shot 2017-09-12 at 11 44 27 am

Local filesystem grep

$ grep -Rn 'tendermint/ed25519' * | awk '{ if ($0 !~ /vendor/) print $0 }'
abci/glide.lock:50:- name: github.com/tendermint/ed25519
Binary file basecoin/cmd/baseserver/bin/baseserver_darwin matches
Binary file basecoin/cmd/baseserver/bin/baseserver_linux matches
Binary file basecoin/cmd/baseserver/bin/baseserver_windows matches
basecoin/glide.lock:114:- name: github.com/tendermint/ed25519
ed25519/.git/config:9:	url = git@github.com:tendermint/ed25519.git
ed25519/.git/logs/HEAD:1:0000000000000000000000000000000000000000 1f52c6f8b8a5c7908aff4497c186af344b428925 Emmanuel Odeke <emm.odeke@gmail.com> 1500799397 -0600	clone: from https://github.com/tendermint/ed25519
ed25519/.git/logs/refs/heads/master:1:0000000000000000000000000000000000000000 1f52c6f8b8a5c7908aff4497c186af344b428925 Emmanuel Odeke <emm.odeke@gmail.com> 1500799397 -0600	clone: from https://github.com/tendermint/ed25519
ed25519/.git/logs/refs/remotes/origin/HEAD:1:0000000000000000000000000000000000000000 1f52c6f8b8a5c7908aff4497c186af344b428925 Emmanuel Odeke <emm.odeke@gmail.com> 1500799397 -0600	clone: from https://github.com/tendermint/ed25519
ed25519/ed25519.go:17:	"github.com/tendermint/ed25519/edwards25519"
ed25519/extra25519/extra25519.go:10:	"github.com/tendermint/ed25519/edwards25519"
ed25519/extra25519/extra25519_test.go:12:	"github.com/tendermint/ed25519"
ethermint/glide.lock:162:- name: github.com/tendermint/ed25519
go-crypto/glide.lock:87:- name: github.com/tendermint/ed25519
go-crypto/glide.yaml:9:- package: github.com/tendermint/ed25519
go-crypto/priv_key.go:7:	"github.com/tendermint/ed25519"
go-crypto/priv_key.go:8:	"github.com/tendermint/ed25519/extra25519"
go-crypto/pub_key.go:8:	"github.com/tendermint/ed25519"
go-crypto/pub_key.go:9:	"github.com/tendermint/ed25519/extra25519"
go-crypto/signature_test.go:9:	"github.com/tendermint/ed25519"
light-client/glide.lock:114:- name: github.com/tendermint/ed25519
merkleeyes/glide.lock:89:- name: github.com/tendermint/ed25519
tendermint/blockchain/reactor_test.go:9:	"github.com/tendermint/ed25519"
Binary file tendermint/build/tendermint matches
tendermint/glide.lock:103:- name: github.com/tendermint/ed25519

Seems like we could resonably get rid of it, but then we'll have to update all the respective vendored dependencies in all our repos.

@zramsay
Copy link

zramsay commented Jan 24, 2018

should we try to get rid of this fork & our use of it?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants