crypto/keyring: fix offline keys migration #8639

sahith-narahari · 2021-02-19T09:39:05Z

Description

closes: #8633
I tested this branch against gaia, and works for migrating multisig keys. I'm not sure about the ledger though, having issues connecting mine. Would be nice if someone can test this.

Steps for testing

1) cosmos-sdk: git checkout sahith/fix-key-multisig

2) gaia: git checkout v2.0.15 && make install

3) create local keys, multisig key and a ledger key

4) gaia: git checkout v4.0.3

5) add the following replace directive to go.mod in gaia - 
       replace github.com/cosmos/cosmos-sdk => <path to cosmos-sdk with specified branch>

6) make install

7) gaiad keys migrate --home <path to gaiacli(default: $HOME/.gaiacli)>

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

lgtm-com · 2021-02-19T10:04:03Z

This pull request introduces 2 alerts when merging 5610039 into ef8dabc - view on LGTM.com

new alerts:

2 for Useless assignment to local variable

lgtm-com · 2021-02-19T10:56:05Z

This pull request introduces 2 alerts when merging f599220 into ef8dabc - view on LGTM.com

new alerts:

2 for Useless assignment to local variable

lgtm-com · 2021-02-19T18:37:27Z

This pull request introduces 2 alerts when merging 48286a3 into 7e481cc - view on LGTM.com

new alerts:

2 for Useless assignment to local variable

alessio

oh hold on - tests are failing

sahith-narahari · 2021-02-23T09:16:21Z

oh hold on - tests are failing

yes, working on them. I wanted others to test using a ledger if possible, updated the description for the same.

alessio · 2021-02-23T12:10:56Z

testing is failing with ledger:

alessio@phoenix:~$ gaiad --log_level=info keys --keyring-backend=file migrate --home=/home/alessio/.gaiacli
Migrating key: 'bar (local)' ...
Skip key migration? [y/N]: n
Enter passphrase to decrypt key:
Enter keyring passphrase:
Re-enter keyring passphrase:
Migrating key: 'ledger (ledger)' ...
Skip key migration? [y/N]: n
Error: unmarshal to types.PubKey failed after 4 bytes (unrecognized prefix bytes 03497EC0): 03497EC092EF901027049E4F39200C60040D3562CD7F104A39F62E6E5A39A818F4

alessio · 2021-02-23T12:11:43Z

It fails with offline keys too (pubkeys info only):

Migrating key: 'pub1 (offline)' ...
Skip key migration? [y/N]: n
Error: unmarshal to types.PubKey failed after 4 bytes (unrecognized prefix bytes 0392A330): 0392A33059328676A9F6CD8A795F91A86E2B0DD4AF51035178C40D19EF30502689
Usage:
  gaiad keys migrate [flags]

alessio · 2021-02-23T12:22:36Z

Failure seems to happen in crypto/keyring/keyring.go, line 320:

	pubKey, err := legacy.PubKeyFromBytes(pubBytes)
	if err != nil {
		return err
	}

…nto sahith/fix-key-multi

robert-zaremba · 2021-02-23T13:22:54Z

Is this PR ready for review or it's still in draft?

client/keys/migrate_test.go

robert-zaremba · 2021-02-26T20:59:28Z

client/keys/migrate.go

 	if err != nil {
 		return err
 	}

-	defer legacyKb.Close()
+	defer func() { _ = legacyKb.Close() }()


why we need to do this?

I want to ignore that error, as it does not really matter whether the keybase can be closed or not (we are basically doing only read-only ops)

InfoImporter is implemented by those Keyring implementations that support import of Info objects.

.github/workflows/test.yml

client/keys/migrate.go

amaury1093

ACK. I tested (last Friday) on local and ledger keys.

client/keys/migrate.go

alessio · 2021-03-01T18:27:43Z

@mergifyhq backport release/v0.41.x

mergify · 2021-03-01T18:28:14Z

Command backport release/v0.41.x: failure

No backport have been created
GitHub App like Mergify are not allowed to create pull request where .github/workflows is changed.

Hey, I reacted but my real name is @Mergifyio

Fix `keys migrate` command (#8703) crypto/keyring: reinstate the InfoImporter interface InfoImporter is implemented by those Keyring implementations that support import of Info objects. Co-authored-by: Alessio Treglia <alessio@tendermint.com> Co-authored-by: Jonathan Gimeno <jgimeno@gmail.com> Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

* crypto/keyring: fix offline keys migration (#8639) Fix `keys migrate` command (#8703) crypto/keyring: reinstate the InfoImporter interface InfoImporter is implemented by those Keyring implementations that support import of Info objects. Co-authored-by: Alessio Treglia <alessio@tendermint.com> Co-authored-by: Jonathan Gimeno <jgimeno@gmail.com> Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com> * refresh golangci-lint * Rename InfoImporter -> LegacyInfoImporter (#8739) Avoid namespace clash with the InfoImporter interface that already exists in the v0.41 release series. * Revert "refresh golangci-lint" This reverts commit 38e1349. Co-authored-by: SaReN <sahithnarahari@gmail.com> Co-authored-by: Jonathan Gimeno <jgimeno@gmail.com> Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

add fix for multisig keys

5610039

update tests

f599220

Merge branch 'master' into sahith/fix-key-multi

48286a3

sahith-narahari and others added 5 commits February 20, 2021 00:09

fix lint

ca120e8

Merge branch 'master' into sahith/fix-key-multi

266c5f5

Merge branch 'master' into sahith/fix-key-multi

03e84ec

rename function legacy

88e1eed

Merge branch 'master' into sahith/fix-key-multi

48068ae

sahith-narahari requested review from alessio, jgimeno, robert-zaremba, amaury1093 and blushi February 23, 2021 09:07

alessio approved these changes Feb 23, 2021

View reviewed changes

alessio suggested changes Feb 23, 2021

View reviewed changes

jgimeno and others added 2 commits February 23, 2021 11:43

fix linter

3288631

update keys

da71b04

jgimeno and others added 5 commits February 23, 2021 13:23

test offline

4b9f9d9

temp commit

42682ce

Merge branch 'sahith/fix-key-multi' of github.com:cosmos/cosmos-sdk i…

9208860

…nto sahith/fix-key-multi

update ledger keys

1afcbe3

Merge branch 'master' into sahith/fix-key-multi

8944c17

fixing the test case

a7babac

alessio reviewed Feb 26, 2021

View reviewed changes

client/keys/migrate_test.go Show resolved Hide resolved

robert-zaremba reviewed Feb 26, 2021

View reviewed changes

Alessio Treglia added 2 commits February 27, 2021 17:16

new testdata

455ccce

Merge branch 'master' into sahith/fix-key-multi

a49440e

alessio requested review from tac0turtle and fedekunze February 27, 2021 17:19

crypto/keyring: reinstate the InfoImporter interface

d6fca2b

InfoImporter is implemented by those Keyring implementations that support import of Info objects.

alessio reviewed Feb 27, 2021

View reviewed changes

.github/workflows/test.yml Show resolved Hide resolved

sahith-narahari commented Feb 27, 2021

View reviewed changes

client/keys/migrate.go Outdated Show resolved Hide resolved

Update error message

8b85b5c

alessio changed the title ~~Fix multisig keys migrate~~ crypto/keyring: fix offline keys migration Feb 27, 2021

amaury1093 approved these changes Mar 1, 2021

View reviewed changes

Merge branch 'master' into sahith/fix-key-multi

72229d2

alessio added A:automerge Automatically merge PR once all prerequisites pass. and removed A:automerge Automatically merge PR once all prerequisites pass. labels Mar 1, 2021

fdymylja approved these changes Mar 1, 2021

View reviewed changes

alessio reviewed Mar 1, 2021

View reviewed changes

client/keys/migrate.go Outdated Show resolved Hide resolved

Update client/keys/migrate.go

f5df8f1

alessio merged commit b1c72fd into master Mar 1, 2021

alessio deleted the sahith/fix-key-multi branch March 1, 2021 12:41

alessio mentioned this pull request Mar 1, 2021

crypto/keyring: fix offline keys migration #8738

Merged

9 tasks

clevinson mentioned this pull request Mar 3, 2021

Update keyring migrate documentation for v0.42 and version specific migrations #8769

Closed

4 tasks

amaury1093 mentioned this pull request Mar 3, 2021

Keys migrate failed #8319

Closed

4 tasks

dependabot bot mentioned this pull request Mar 11, 2021

Bump github.com/cosmos/cosmos-sdk from 0.40.0-rc5 to 0.42.1 celestiaorg/celestia-app#45

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto/keyring: fix offline keys migration #8639

crypto/keyring: fix offline keys migration #8639

sahith-narahari commented Feb 19, 2021 •

edited

Loading

lgtm-com bot commented Feb 19, 2021

lgtm-com bot commented Feb 19, 2021

lgtm-com bot commented Feb 19, 2021

alessio left a comment

sahith-narahari commented Feb 23, 2021

alessio commented Feb 23, 2021

alessio commented Feb 23, 2021

alessio commented Feb 23, 2021

robert-zaremba commented Feb 23, 2021

robert-zaremba Feb 26, 2021

alessio Feb 26, 2021

amaury1093 left a comment

alessio commented Mar 1, 2021 •

edited

Loading

mergify bot commented Mar 1, 2021

crypto/keyring: fix offline keys migration #8639

crypto/keyring: fix offline keys migration #8639

Conversation

sahith-narahari commented Feb 19, 2021 • edited Loading

Description

Steps for testing

lgtm-com bot commented Feb 19, 2021

lgtm-com bot commented Feb 19, 2021

lgtm-com bot commented Feb 19, 2021

alessio left a comment

Choose a reason for hiding this comment

sahith-narahari commented Feb 23, 2021

alessio commented Feb 23, 2021

alessio commented Feb 23, 2021

alessio commented Feb 23, 2021

robert-zaremba commented Feb 23, 2021

robert-zaremba Feb 26, 2021

Choose a reason for hiding this comment

alessio Feb 26, 2021

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

alessio commented Mar 1, 2021 • edited Loading

mergify bot commented Mar 1, 2021

sahith-narahari commented Feb 19, 2021 •

edited

Loading

alessio commented Mar 1, 2021 •

edited

Loading