Migrate Tendermint PubKey types to the SDK #7047

sahith-narahari · 2020-08-13T17:32:32Z

Description

This PR imports secp256k1, ed25519, sr25519 packages from tendermint/crypto. I'll do a followup PR to update the references to contain this PR as is.
ref: #7008

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2020-08-13T17:36:42Z

Codecov Report

Merging #7047 into master will decrease coverage by 0.20%.
The diff coverage is 18.69%.

@@            Coverage Diff             @@
##           master    #7047      +/-   ##
==========================================
- Coverage   54.81%   54.60%   -0.21%     
==========================================
  Files         542      547       +5     
  Lines       36907    37121     +214     
==========================================
+ Hits        20230    20270      +40     
- Misses      15021    15192     +171     
- Partials     1656     1659       +3

tac0turtle

How come ed25519 was brought over? If you will be using the same interface as Tendermint then might as well just import ed25519?

aaronc · 2020-08-13T19:28:02Z

We have a design for how addresses get computed that's different Extensible address format #5694
We probably want to make these native proto types

aaronc · 2020-08-13T19:29:44Z

Also you suggested us having our own types here @marbar3778 #7008 (comment)

tac0turtle · 2020-08-13T19:31:49Z

Ah the address thing sorry. Yea I said that but thought it would be a different interface. Ignore my comment.

Super happy to see this moving forward. I will transfer the secp issues we have in Tendermint to the sek in the morning.

amaury1093

lgtm 👍

amaury1093 · 2020-08-14T09:18:02Z

crypto/benchmarking/bench.go

@@ -0,0 +1,88 @@
+package benchmarking


crypto/ already has some folders (hd, keyring, ledger). Would it make more sense to put all these files into crypto/pubkey/?

crypto/(key, keys) because it houses private keys as well

I didn't have a preference here, either way seems fine to me. Will move to crypto/keys if that seems better

I also prefer keys than key.

tac0turtle · 2020-08-15T00:56:41Z

crypto/ed25519/ed25519.go

+}
+
+// PrivKeyEd25519 implements crypto.PrivKey.
+type PrivKeyEd25519 [64]byte


you will not want sized byte arrays in the sdk, this was discussed in an issue, cant recall which. the current tendermint impl uses unsized to make it easier but this does bring an extra need for carfulness.

tac0turtle · 2020-08-15T01:01:47Z

crypto/sr25519/pubkey.go

+const PubKeySr25519Size = 32
+
+// PubKeySr25519 implements crypto.PubKey for the Sr25519 signature scheme.
+type PubKeySr25519 [PubKeySr25519Size]byte


Should be unsized.

I understand this is a copy of what is in Tendermint but maybe the sdk should first start with migrating what it uses (secp256k1). I understand I said we should migrate the other keys but if the other keys are not used then it is better to start with the minimum (secp256k1) and build up from there? I am up for a discussion on this.

I am proposing this because when I switched sims to ed25519 keys they failed. This makes me think there is a reliance on key type currently?

thoughts @aaronc @alexanderbez

Yeah we can start with just SECP256K1 for now. Also, they should be unsized if we want to be able to use oneof I think?

We do want unsized byte arrays, is it okay if we do that in a separate PR? The intention here was mainly to do an exact copy so we don't have to get both the copying and the changes at once

Should the copy be of master? Much of these changes are already made.

Okay, we'll need to update this against master then.

crypto/secp256k1/secp256k1_nocgo.go

crypto/secp256k1/secp256k1.go

aaronc · 2020-08-16T13:36:48Z

@marbar3778 our intention here is to just copy and then in separate PRs:

make changes, and
update references

What do you think ?

tac0turtle · 2020-08-16T14:39:01Z

our intention here is to just copy and then in separate PRs:
* make changes, and

* update references
What do you think ?

sounds good. will this be the base PR or will it be merged into master

aaronc · 2020-08-16T15:19:11Z

sounds good. will this be the base PR or will it be merged into master

The idea is to merge this into master and then proceed with separate PRs against master.

…rate-tm-keys

aaronc

ACK

My desire here is to just do a clean copy of code from tendermint into the SDK and for modifications to go in separate PRs so that the diffs is clean. With that in mind, all I want to know is - did the code get copied correctly from tendermint. For efficiency, that would be my request of other reviewers.

To verify, I checked out tendermint master (42e4e8b58ebc58ebd663c114d2bcd7ab045b1c55) and ran diffs on the code:

~/tendermint/crypto/secp256k1 master ❯ diff . ~/cosmos-sdk/crypto/keys/secp256k1/                                                                             
diff ./bench_test.go /Users/arc/cosmos-sdk/crypto/keys/secp256k1/bench_test.go
8c8,9
<       "github.com/tendermint/tendermint/crypto/internal/benchmarking"
---
> 
>       "github.com/cosmos/cosmos-sdk/crypto/keys/benchmarking"
Only in .: encoding.go
Common subdirectories: ./internal and /Users/arc/cosmos-sdk/crypto/keys/secp256k1/internal

Only tmjson changes (encoding.go deleted).

~/tendermint/crypto/ed25519 master ❯ diff . ~/cosmos-sdk/crypto/keys/ed25519                                                                                   
diff ./bench_test.go /Users/arc/cosmos-sdk/crypto/keys/ed25519/bench_test.go
8c8,9
<       "github.com/tendermint/tendermint/crypto/internal/benchmarking"
---
> 
>       "github.com/cosmos/cosmos-sdk/crypto/keys/benchmarking"
diff ./ed25519.go /Users/arc/cosmos-sdk/crypto/keys/ed25519/ed25519.go
13d12
<       tmjson "github.com/tendermint/tendermint/libs/json"
21,22d19
<       PrivKeyName = "tendermint/PrivKeyEd25519"
<       PubKeyName  = "tendermint/PubKeyEd25519"
37,41d33
< func init() {
<       tmjson.RegisterType(PubKey{}, PubKeyName)
<       tmjson.RegisterType(PrivKey{}, PrivKeyName)
< }
<

Again, only tmjson changes.

~/tendermint/crypto/sr25519 master ❯ diff . ~/cosmos-sdk/crypto/keys/sr25519                                                                                    
diff ./bench_test.go /Users/arc/cosmos-sdk/crypto/keys/sr25519/bench_test.go
8c8,9
<       "github.com/tendermint/tendermint/crypto/internal/benchmarking"
---
> 
>       "github.com/cosmos/cosmos-sdk/crypto/keys/benchmarking"
diff ./encoding.go /Users/arc/cosmos-sdk/crypto/keys/sr25519/encoding.go
5d4
<       tmjson "github.com/tendermint/tendermint/libs/json"
11,13d9
<       PrivKeyName = "tendermint/PrivKeySr25519"
<       PubKeyName  = "tendermint/PubKeySr25519"
< 
18,23d13
< 
< func init() {
< 
<       tmjson.RegisterType(PubKey{}, PubKeyName)
<       tmjson.RegisterType(PrivKey{}, PrivKeyName)
< }

Again, only tmjson changes.

~/tendermint/crypto/internal/benchmarking master ❯ diff . ~/cosmos-sdk/crypto/keys/benchmarking                                                                 19:09:05
diff ./bench.go /Users/arc/cosmos-sdk/crypto/keys/benchmarking/bench.go
40,44c40
<               _, err := priv.Sign(message)
< 
<               if err != nil {
<                       b.FailNow()
<               }
---
>               priv.Sign(message)

Why is the error handled differently here @sahith-narahari? This seems to be the only substantive difference but doesn't seem critical as this is in bench code.

I also checked go.mod and the only major difference is that tendermint uses:

        golang.org/x/crypto v0.0.0-20200406173513-056763e48d71

which appears to be an older version. I'm assuming that's okay because it's more recent? There is one newer commit here btw: https://github.com/golang/crypto/commits/master

sahith-narahari · 2020-08-18T11:36:45Z

@aaronc the two changes wrt tendermint are intended. As cosmos-sdk still uses tendermint, registering with tmjson again panics, as it already happens in tendermint/crypto.
Also benchmarking should be an internal package here too, will make the changes accordingly.

… into sahith/migrate-tm-keys

amaury1093

LGTM overall 👍 .

I understand removing tmjson.Register()..., but can you explain why this part is removed?

<               _, err := priv.Sign(message)
< 
<               if err != nil {
<                       b.FailNow()
<               }

anilcse

Looks good. I would prefer to have a TODO and comment out the code that panics instead of removing it. Also for packages, I would suggest to address that via a separate PR. We can add a todo item to the tracking issue

sahith-narahari · 2020-08-18T13:28:58Z

LGTM overall +1 .

I understand removing tmjson.Register()..., but can you explain why this part is removed?
<               _, err := priv.Sign(message)
< 
<               if err != nil {
<                       b.FailNow()
<               }

This was updated later in tm master, whereas this code was from 0.34-rc3. I updated the code to fail with error now, as in master

sahith-narahari · 2020-08-18T13:31:43Z

Looks good. I would prefer to have a TODO and comment out the code that panics instead of removing it. Also for packages, I would suggest to address that via a separate PR. We can add a todo item to the tracking issue

I personally don't prefer commenting out code, and tmjson as I understand is tendermint's way of dealing with amino json. I'm not sure if we'll be doing the same after migration. I can add a todo if we'll be using the same approach in sdk too.
cc @aaronc

amaury1093

lgtm!

aaronc · 2020-08-18T13:54:03Z

Looks good. I would prefer to have a TODO and comment out the code that panics instead of removing it. Also for packages, I would suggest to address that via a separate PR. We can add a todo item to the tracking issue

I personally don't prefer commenting out code, and tmjson as I understand is tendermint's way of dealing with amino json. I'm not sure if we'll be doing the same after migration. I can add a todo if we'll be using the same approach in sdk too.

cc @aaronc

tmjson is not needed. Removing it is correct

sahith-narahari added 2 commits August 13, 2020 22:54

Move crypto packages from tm

73f7486

Revert refactor changes

1d3714f

Fix lint

aa40ce0

sahith-narahari marked this pull request as ready for review August 13, 2020 18:56

sahith-narahari requested review from aaronc, alessio, alexanderbez, clevinson, fedekunze and jgimeno as code owners August 13, 2020 18:56

tac0turtle reviewed Aug 13, 2020

View reviewed changes

amaury1093 reviewed Aug 14, 2020

View reviewed changes

tac0turtle reviewed Aug 15, 2020

View reviewed changes

crypto/secp256k1/secp256k1_nocgo.go Outdated Show resolved Hide resolved

tac0turtle reviewed Aug 15, 2020

View reviewed changes

crypto/secp256k1/secp256k1.go Outdated Show resolved Hide resolved

sahith-narahari mentioned this pull request Aug 17, 2020

Migrate Tendermint PubKey types to the SDK #7008

Closed

3 tasks

sahith-narahari added 6 commits August 17, 2020 09:21

Move to crypto/keys

eddbd66

Merge branch 'master' of github.com:cosmos/cosmos-sdk into sahith/mig…

7b6339e

…rate-tm-keys

Update to tm 0.34-rc3

70796ee

Update benchmark tests

61e50a3

Fix tests

0ed7dcb

Merge branch 'master' into sahith/migrate-tm-keys

0036cb2

sahith-narahari added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 17, 2020

aaronc approved these changes Aug 17, 2020

View reviewed changes

Merge branch 'master' into sahith/migrate-tm-keys

9412451

aaronc added T: Security C:Crypto labels Aug 17, 2020

Update to tm master

bee04f6

sahith-narahari added 2 commits August 18, 2020 17:11

Merge branch 'sahith/migrate-tm-keys' of github.com:cosmos/cosmos-sdk…

2c00861

… into sahith/migrate-tm-keys

Merge branch 'master' into sahith/migrate-tm-keys

7a089a3

amaury1093 reviewed Aug 18, 2020

View reviewed changes

anilcse reviewed Aug 18, 2020

View reviewed changes

amaury1093 approved these changes Aug 18, 2020

View reviewed changes

mergify bot added 2 commits August 18, 2020 13:38

Merge branch 'master' into sahith/migrate-tm-keys

18dac82

Merge branch 'master' into sahith/migrate-tm-keys

f9de5ed

mergify bot merged commit 443e0c1 into master Aug 18, 2020

mergify bot deleted the sahith/migrate-tm-keys branch August 18, 2020 13:53

sahith-narahari mentioned this pull request Aug 18, 2020

Update tm pubkey references #7102

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Tendermint PubKey types to the SDK #7047

Migrate Tendermint PubKey types to the SDK #7047

sahith-narahari commented Aug 13, 2020 •

edited

Loading

codecov bot commented Aug 13, 2020 •

edited

Loading

tac0turtle left a comment

aaronc commented Aug 13, 2020 •

edited

Loading

aaronc commented Aug 13, 2020

tac0turtle commented Aug 13, 2020

amaury1093 left a comment

amaury1093 Aug 14, 2020

tac0turtle Aug 14, 2020

sahith-narahari Aug 14, 2020

amaury1093 Aug 14, 2020

tac0turtle Aug 15, 2020

tac0turtle Aug 15, 2020

sahith-narahari Aug 16, 2020

alexanderbez Aug 16, 2020

aaronc Aug 17, 2020 •

edited

Loading

tac0turtle Aug 17, 2020

aaronc Aug 17, 2020

aaronc commented Aug 16, 2020

tac0turtle commented Aug 16, 2020

aaronc commented Aug 16, 2020

aaronc left a comment •

edited

Loading

sahith-narahari commented Aug 18, 2020

amaury1093 left a comment

anilcse left a comment

sahith-narahari commented Aug 18, 2020

sahith-narahari commented Aug 18, 2020

amaury1093 left a comment

aaronc commented Aug 18, 2020

Migrate Tendermint PubKey types to the SDK #7047

Migrate Tendermint PubKey types to the SDK #7047

Conversation

sahith-narahari commented Aug 13, 2020 • edited Loading

Description

codecov bot commented Aug 13, 2020 • edited Loading

Codecov Report

tac0turtle left a comment

Choose a reason for hiding this comment

aaronc commented Aug 13, 2020 • edited Loading

aaronc commented Aug 13, 2020

tac0turtle commented Aug 13, 2020

amaury1093 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc commented Aug 16, 2020

tac0turtle commented Aug 16, 2020

aaronc commented Aug 16, 2020

aaronc left a comment • edited Loading

Choose a reason for hiding this comment

sahith-narahari commented Aug 18, 2020

amaury1093 left a comment

Choose a reason for hiding this comment

anilcse left a comment

Choose a reason for hiding this comment

sahith-narahari commented Aug 18, 2020

sahith-narahari commented Aug 18, 2020

amaury1093 left a comment

Choose a reason for hiding this comment

aaronc commented Aug 18, 2020

sahith-narahari commented Aug 13, 2020 •

edited

Loading

codecov bot commented Aug 13, 2020 •

edited

Loading

aaronc commented Aug 13, 2020 •

edited

Loading

aaronc Aug 17, 2020 •

edited

Loading

aaronc left a comment •

edited

Loading