Use embedded lib key type in sdk pub keys instead of bytes. #7672

robert-zaremba · 2020-10-26T14:21:41Z

Description

When doing the conversion of validators public key from string to Any I noticed that we are doing unnecessary memory allocation for ed25519 keys and we can simplify by using the ed25519.PrivateKey directly in protobuf ( ed25519.PrivateKey = []byte).

This changeset:

adds cross library signature tests for ed25519
adds cross library signature tests for secp256k1 (note: signature validation was not simple - requires assumption about signature)
uses ed25519 from stdlib:
In Go 1.13 the new crypto/ed25519 package implements the Ed25519 signature scheme.
This functionality was previously provided by the golang.org/x/crypto/ed25519 package,
which becomes a wrapper for crypto/ed25519 when used with Go 1.13+.
uses ed25519.PrivateKey direactly as "our" private key reducing the code (by delegating directly to the stdlib code!) an unnecessary memory allocations.

closes: #7643

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-10-26T14:28:13Z

Codecov Report

Merging #7672 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #7672   +/-   ##
=======================================
  Coverage   54.10%   54.11%           
=======================================
  Files         611      611           
  Lines       38571    38571           
=======================================
+ Hits        20869    20871    +2     
+ Misses      15574    15573    -1     
+ Partials     2128     2127    -1

alessio · 2020-10-26T15:06:33Z

crypto/keys/ed25519/ed25519_test.go

@@ -1,6 +1,7 @@
-package ed25519_test
+package ed25519


to avoid weird package renames. It simplifies reading the code.
Also, we only have unit tests here. Using ed25519_test is good for integration tests.

This is a weird package rename. It actually helps the reader to have _test packages marked consistently.

is good for integration tests.

Why? How? In which cases?

Please revert this unnecesary change.

BTW there are plenty of good reasons to hide internals when testing Go packages, that's why _test packages are idiomatic in Go. We've made efforts in the past to make tests live within their respective _test packages. This is a step backwards.

Testing in same package is the Go idiomatic way. The go test roughly mentions about _test package. The testing godoc doesn't mention it at all, same go wiki - learn testing. Go code review guidelines mention _test packages only in one place - to use it to avoid circular dependencies - they suggest to use dot import for that.

In all projects I was working (many) there was a preference to test in the package (not _test). Personally I'm advocating to use use _test packages for integration tests to have better feeling of a black box testing. This also gives a better separation between unit tests and other tests.

It's not about which projects use and don't use _test. We should strive to use when possible for better isolation. If the change isn't necessary, then we shouldn't change it.

I wanted to point out that it's a usual Go way for unit tests.
So, how about this:

@alessio , @aaronc - If you prefer to keep the _test name, how about using the dot import (ref: same comment above) to avoid this name clashes ?

I wanted to point out that it's a usual Go way for unit tests.

And I would like to point out that:

Test files that declare a package with the suffix "_test" will be compiled as a separate package, and then linked and run with the main test binary.

Per this piece of documentation here, that incidentally is hosted at tip.golang.org.

how about using the dot import

No, it adds no value, plus it would make the imported symbols unpredictable.

How about reverting this change?

Per this piece of documentation here, that incidentally is hosted at tip.golang.org.

I already linked go test docs (using the source file) in one of my comments above. I don't see any relevant change in tip. If there is any, would you mind to pointing it out?
NOTE: I compared, there is no change in tip

How about reverting this change?

OK, my motivation was to use the more adopted way for Go unit tests. If this is not an SDK way, no problem, reverting.

alessio · 2020-10-26T15:27:21Z

crypto/keys/ed25519/ed25519.go

@@ -1,14 +1,14 @@
 package ed25519

 import (
+	"crypto/ed25519"


This is good BTW

yes, I made a note in the commit. Here are the details: https://golang.org/doc/go1.13#crypto/ed25519
We shouldn't use golang.org/x/crypto/ed25519 package.

alessio

Few changes are requested.

robert-zaremba · 2020-10-27T23:05:42Z

I've rolled back the package rename from ed25519 to ed25519_test.

blushi

The changes look ok to me but could you resolve merge conflicts?

amaury1093

the code lgtm, though I would prefer to approve once the tests pass.

crypto/keys/secp256k1/secp256k1_test.go

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

In Go 1.13 the new crypto/ed25519 package implements the Ed25519 signature scheme. This functionality was previously provided by the golang.org/x/crypto/ed25519 package, which becomes a wrapper for crypto/ed25519 when used with Go 1.13+.

robert-zaremba · 2020-10-28T09:54:34Z

Tests were passing, but there were conflicts after the nights merge. Now solved.

amaury1093

utACK, thanks!

robert-zaremba requested review from aaronc, alessio and alexanderbez as code owners October 26, 2020 14:21

robert-zaremba requested review from blushi and amaury1093 October 26, 2020 14:22

alessio reviewed Oct 26, 2020

View reviewed changes

alessio suggested changes Oct 26, 2020

View reviewed changes

robert-zaremba force-pushed the robert/pk-type branch from 50d4f7a to c7ccf0f Compare October 27, 2020 23:04

blushi reviewed Oct 28, 2020

View reviewed changes

amaury1093 reviewed Oct 28, 2020

View reviewed changes

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

fadeev and others added 8 commits October 28, 2020 10:46

Remove duplicate print message on keys add command (#7654)

2445437

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

ed25519: use stdlib/crypto types

1646604

use standard package testing

b317e22

use standard package testing for secp256k1 tests

0df1db5

secp256k1: add cross packages signature checks

5260eb6

ed25519: rollback the _test package name

41c9cfb

rename underlyingSecp256k1 to btcSecp256k1

86b8250

robert-zaremba force-pushed the robert/pk-type branch from c7ccf0f to 86b8250 Compare October 28, 2020 09:48

package update

389e531

robert-zaremba requested review from amaury1093 and blushi October 28, 2020 09:53

amaury1093 approved these changes Oct 28, 2020

View reviewed changes

alessio approved these changes Oct 28, 2020

View reviewed changes

robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 28, 2020

mergify bot merged commit bd9af94 into master Oct 28, 2020

mergify bot deleted the robert/pk-type branch October 28, 2020 10:24

robert-zaremba mentioned this pull request Oct 30, 2020

Use type aliasing or embedded types for handling protobuf and ed25519 operations #7382

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use embedded lib key type in sdk pub keys instead of bytes. #7672

Use embedded lib key type in sdk pub keys instead of bytes. #7672

robert-zaremba commented Oct 26, 2020 •

edited

Loading

codecov bot commented Oct 26, 2020 •

edited

Loading

alessio Oct 26, 2020

robert-zaremba Oct 26, 2020

alessio Oct 26, 2020

alessio Oct 26, 2020

robert-zaremba Oct 26, 2020 •

edited

Loading

alexanderbez Oct 27, 2020 •

edited

Loading

robert-zaremba Oct 27, 2020 •

edited

Loading

alessio Oct 27, 2020

robert-zaremba Oct 27, 2020 •

edited

Loading

robert-zaremba Oct 27, 2020

alessio Oct 26, 2020

robert-zaremba Oct 26, 2020 •

edited

Loading

alessio left a comment

robert-zaremba commented Oct 27, 2020 •

edited

Loading

blushi left a comment

amaury1093 left a comment

robert-zaremba commented Oct 28, 2020

amaury1093 left a comment

Use embedded lib key type in sdk pub keys instead of bytes. #7672

Use embedded lib key type in sdk pub keys instead of bytes. #7672

Conversation

robert-zaremba commented Oct 26, 2020 • edited Loading

Description

codecov bot commented Oct 26, 2020 • edited Loading

Codecov Report

alessio Oct 26, 2020

Choose a reason for hiding this comment

robert-zaremba Oct 26, 2020

Choose a reason for hiding this comment

alessio Oct 26, 2020

Choose a reason for hiding this comment

alessio Oct 26, 2020

Choose a reason for hiding this comment

robert-zaremba Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

alexanderbez Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

robert-zaremba Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

alessio Oct 27, 2020

Choose a reason for hiding this comment

robert-zaremba Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

robert-zaremba Oct 27, 2020

Choose a reason for hiding this comment

alessio Oct 26, 2020

Choose a reason for hiding this comment

robert-zaremba Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

robert-zaremba commented Oct 27, 2020 • edited Loading

blushi left a comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

robert-zaremba commented Oct 28, 2020

amaury1093 left a comment

Choose a reason for hiding this comment

robert-zaremba commented Oct 26, 2020 •

edited

Loading

codecov bot commented Oct 26, 2020 •

edited

Loading

robert-zaremba Oct 26, 2020 •

edited

Loading

alexanderbez Oct 27, 2020 •

edited

Loading

robert-zaremba Oct 27, 2020 •

edited

Loading

robert-zaremba Oct 27, 2020 •

edited

Loading

robert-zaremba Oct 26, 2020 •

edited

Loading

robert-zaremba commented Oct 27, 2020 •

edited

Loading