fix multisig account pubkeys migration #8788

akhilkumarpilli · 2021-03-04T16:37:08Z

Description

When migrating genesis to v0.40, set pub_key to null if account is multisig as a quick fix

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

* v0.40.0 final changelog & release notes * Trigger Build

* fix reproducible builds (#8300) * fix library file path (#8301) Co-authored-by: SaReN <[email protected]> Co-authored-by: Alessio Treglia <[email protected]>

From: #8282 Co-authored-by: MD Aleem <[email protected]> Co-authored-by: SaReN <[email protected]>

) (#8373) From: #8273 Co-authored-by: Federico Kunze <[email protected]>

From #8282 Co-authored-by: Robert Zaremba <[email protected]> Co-authored-by: Alessio Treglia <[email protected]>

#8376) From #8291 Bumps [github.com/gogo/protobuf](https://github.com/gogo/protobuf) from 1.3.1 to 1.3.2. - [Release notes](https://github.com/gogo/protobuf/releases) - [Commits](gogo/protobuf@v1.3.1...v1.3.2) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

From: #8287 Co-authored-by: Alessio Treglia <[email protected]> Co-authored-by: Federico Kunze <[email protected]> Co-authored-by: SaReN <[email protected]>

From: #8385 Co-authored-by: Alessio Treglia <[email protected]>

* Update gogo proto deps with v1.3.2 security fixes * Regenerate proto files Co-authored-by: Aaron Craelius <[email protected]>

* fix-ibc-client * add changelog Co-authored-by: Segue <[email protected]>

* fix proto generation * merge grpc_gateway into gocosmos_out * change env variable names Co-authored-by: Marko <[email protected]>

@fedekunze

* add missing UnpackInterfaces functions * fix build * add tests cc @fedekunze Co-authored-by: Federico Kunze <[email protected]> Co-authored-by: colin axnér <[email protected]> Co-authored-by: Federico Kunze <[email protected]>

From: #8317 Co-authored-by: Amaury <[email protected]>

* Update changelog for 0.40.1 * add tendermint * Add release notes * add known issues * update * update release notes * fix a few things in CHANGELOG * Update RELEASE_NOTES.md Co-authored-by: Alessio Treglia <[email protected]>

* update to tendermint v0.34.3 * go.mod replace with grpc v1.33.2 Co-authored-by: Amaury Martiny <[email protected]> Co-authored-by: Alessio Treglia <[email protected]>

From: #8417

Co-authored-by: Alessio Treglia <[email protected]>

@fedekunze

From: #8404 Thanks: @fedekunze @colin-axner

@fedekunze

From: #8437 Closes: #8266 Thanks: @fedekunze

Co-authored-by: Alessio Treglia <[email protected]>

Release v0.41.0

From: #8475

From: #8436 Co-authored-by: Jonathan Gimeno <[email protected]>

From #8458 Co-authored-by: Ethan Frey <[email protected]>

@michaelfig

From: #8481 Thanks: @michaelfig for the patch.

@odeke-em

Closes: #8530 From: #8531 Thanks: @odeke-em for pointing this out.

Still a problem with `cat` - a tmp_genesis file needs to be created and then moved to the old location Co-authored-by: Amaury <[email protected]> Co-authored-by: Cory <[email protected]>

alessio · 2021-03-04T16:46:54Z

Thanks @akhilkumarpilli for taking the time and making this PR! Did you test this, and if so how?

codecov · 2021-03-04T16:47:12Z

Codecov Report

Merging #8788 (1ebd023) into release/v0.41.x (3aaf1bc) will increase coverage by 0.00%.
The diff coverage is 33.33%.

@@               Coverage Diff                @@
##           release/v0.41.x    #8788   +/-   ##
================================================
  Coverage            61.95%   61.95%           
================================================
  Files                  612      612           
  Lines                35417    35432   +15     
================================================
+ Hits                 21942    21953   +11     
- Misses               11155    11156    +1     
- Partials              2320     2323    +3

Impacted Files	Coverage Δ
x/auth/legacy/v040/migrate.go	`87.30% <33.33%> (-2.87%)`	⬇️
x/ibc/core/02-client/keeper/client.go	`98.27% <0.00%> (+0.06%)`	⬆️
x/ibc/core/02-client/types/encoding.go	`60.00% <0.00%> (+6.15%)`	⬆️

akhilkumarpilli · 2021-03-04T16:58:02Z

Thanks @akhilkumarpilli for taking the time and making this PR! Did you test this, and if so how?

Yes, tested locally. I just ran akash local devnet with couple of multisig accounts and migrated it from v0.39 to v0.40. I tried to do multisig transaction but got some panic error from tx. I found issue is with empty public_keys array.

Later, I tried with this fix by setting multisig account pub_key to null and it worked fine.

aaronc · 2021-03-04T17:01:54Z

Would be good to understand if this is an active issue on the hub

alessio · 2021-03-04T17:18:19Z

Would be good to understand if this is an active issue on the hub

It looks like an issue for which a known workaround is available. It still is an issue though I think.

zmanian · 2021-03-04T17:30:42Z

I have used hub multisigs after the migration and it worked fine.

zmanian · 2021-03-04T23:27:11Z

This seems like a good direction.

the ways addresses work right now, it is always safe to set a pubkey to nil on export.

If we implement key rotation and addresses become stateful, then export will be required for upgrades.

alessio · 2021-03-04T23:54:06Z

@zmanian so you reckon this can be safely merged, yeah?

boz · 2021-03-05T01:34:54Z

cosmoshub-4 was not affected by this - the genesis had "pub_key": null for all accounts.

anilcse

Thanks @akhilkumarpilli

anilcse · 2021-03-05T04:01:11Z

I have used hub multisigs after the migration and it worked fine.

v0.38 migration sets nil for all the account pubkeys. That's the reason Hub upgrade worked just fine. This issue will be only for chains who migrate form 039 to 040 as we are setting pubkey to the account if exists.

I suggest to merge this asap and tag a release. Otherwise other chains might run into similar issues and non-recoverable cases

cc @aaronc @clevinson @alessio

anilcse · 2021-03-05T08:23:41Z

@akhilkumarpilli this should go into master and then backported to release-0.41.x

Cc @clevinson

amaury1093 · 2021-03-05T09:18:44Z

@akhilkumarpilli Could you rebase?

akhilkumarpilli · 2021-03-05T09:27:43Z

Actually, I raised PR with base from v0.41.4. But there are lot of conflicts when I changed base to master. Do I need to close this PR and raise a new one with adding fix in master as there are more conflicts?

amaury1093 · 2021-03-05T09:52:08Z

@akhilkumarpilli this PR has already 2 approvals, so I would suggest to push --force. But if that's too messy, okay to create a new PR too.

akhilkumarpilli · 2021-03-05T10:34:58Z

I tried to resolve conflicts but there are lot of conflicts and too messy. So, I will raise a new PR.

akhilkumarpilli · 2021-03-05T11:25:35Z

Closing this and opened new one #8794. Thank you.

clevinson and others added 30 commits January 8, 2021 17:43

Stargate Release PR -- v0.40.0 Final (#8284)

a334a59

* v0.40.0 final changelog & release notes * Trigger Build

Backport/reproducible builds (#8381)

b6b79e3

* fix reproducible builds (#8300) * fix library file path (#8301) Co-authored-by: SaReN <[email protected]> Co-authored-by: Alessio Treglia <[email protected]>

fix GET /upgrade/current query (#8280) (#8372)

c657222

From: #8282 Co-authored-by: MD Aleem <[email protected]> Co-authored-by: SaReN <[email protected]>

build(deps): bump github.com/armon/go-metrics from 0.3.5 to 0.3.6 (#8273

0e9a6b2

) (#8373) From: #8273 Co-authored-by: Federico Kunze <[email protected]>

fix zero time checks (#8282) (#8374)

c18ba39

From #8282 Co-authored-by: Robert Zaremba <[email protected]> Co-authored-by: Alessio Treglia <[email protected]>

Fix sequence value in auth sign signature only (#8287) (#8383)

f2de540

From: #8287 Co-authored-by: Alessio Treglia <[email protected]> Co-authored-by: Federico Kunze <[email protected]> Co-authored-by: SaReN <[email protected]>

fix: wrappedError.Is (#8355) (#8377)

ca2ff62

From: #8385 Co-authored-by: Alessio Treglia <[email protected]>

fix: wrappedError.Is (#8355) (#8377)

07a68b5

From: #8385 Co-authored-by: Alessio Treglia <[email protected]>

Update gogo proto deps with v1.3.2 security fixes (#8350) (#8386)

52f0baa

* Update gogo proto deps with v1.3.2 security fixes * Regenerate proto files Co-authored-by: Aaron Craelius <[email protected]>

Fix ibc client (#8341) (#8375)

5acc12a

* fix-ibc-client * add changelog Co-authored-by: Segue <[email protected]>

fix proto generation (#8361) (#8389)

1c76416

* fix proto generation * merge grpc_gateway into gocosmos_out * change env variable names Co-authored-by: Marko <[email protected]>

backport: client denom metadata queries (#8364)

e474138

From: #8317 Co-authored-by: Amaury <[email protected]>

Backport/changelog (#8390)

df5bf85

* Update changelog for 0.40.1 * add tendermint * Add release notes * add known issues * update * update release notes * fix a few things in CHANGELOG * Update RELEASE_NOTES.md Co-authored-by: Alessio Treglia <[email protected]>

update to tendermint v0.34.3 (#8388) (#8391)

7cb7a3a

* update to tendermint v0.34.3 * go.mod replace with grpc v1.33.2 Co-authored-by: Amaury Martiny <[email protected]> Co-authored-by: Alessio Treglia <[email protected]>

stargate: balance and metadata validation (#8421)

5b45deb

From: #8417

backport #8418 (#8424)

5179e7e

Co-authored-by: Alessio Treglia <[email protected]>

Reorder IBC channel callbacks (#8441)

f67c4f9

From: #8404 Thanks: @fedekunze @colin-axner

ibc: MsgTransfer amino JSON, commits from @fedekunze (#8440)

e6dac1d

From: #8437 Closes: #8266 Thanks: @fedekunze

chore: update RELEASE_NOTES.md (#8443)

c54025d

Co-authored-by: Alessio Treglia <[email protected]>

Merge pull request #8444 from cosmos/release/v0.40.x

f8a6987

Release v0.41.0

backport #8479 (#8486)

a72c975

snapshots: fix flaky tests [o.41-cherry-pick] (#8484)

2437b66

From: #8475

Fix keyring import from older versions. (#8489)

ad46b2f

From: #8436 Co-authored-by: Jonathan Gimeno <[email protected]>

Add packet_connection attribute to packet lifecycle methods (#8483)

37e1d93

From #8458 Co-authored-by: Ethan Frey <[email protected]>

fix: tendermint subcommands should not create missing files (#8493)

8a79ec1

From: #8481 Thanks: @michaelfig for the patch.

bank: don't ignore errors returned by Balance.GetAddress() (#8534)

5bd006c

Closes: #8530 From: #8531 Thanks: @odeke-em for pointing this out.

add cosmovisot to running a node (#8320)

e009135

Update chain-upgrade-guide-040.md (#8326)

41d50c8

Still a problem with `cat` - a tmp_genesis file needs to be created and then moved to the old location Co-authored-by: Amaury <[email protected]> Co-authored-by: Cory <[email protected]>

akhilkumarpilli requested review from aaronc, alessio and alexanderbez as code owners March 4, 2021 16:37

alessio requested a review from zmanian March 4, 2021 16:45

alessio requested review from fdymylja, jgimeno, robert-zaremba, sahith-narahari and tac0turtle March 4, 2021 16:47

alessio approved these changes Mar 4, 2021

View reviewed changes

anilcse approved these changes Mar 5, 2021

View reviewed changes

akhilkumarpilli changed the base branch from release/v0.41.x to master March 5, 2021 09:10

akhilkumarpilli closed this Mar 5, 2021

alessio deleted the akhil/multisig-migrate-fix branch March 14, 2021 01:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix multisig account pubkeys migration #8788

fix multisig account pubkeys migration #8788

akhilkumarpilli commented Mar 4, 2021

alessio commented Mar 4, 2021

codecov bot commented Mar 4, 2021

akhilkumarpilli commented Mar 4, 2021

aaronc commented Mar 4, 2021

alessio commented Mar 4, 2021

zmanian commented Mar 4, 2021

zmanian commented Mar 4, 2021

alessio commented Mar 4, 2021

boz commented Mar 5, 2021

anilcse left a comment

anilcse commented Mar 5, 2021 •

edited

Loading

anilcse commented Mar 5, 2021

amaury1093 commented Mar 5, 2021

akhilkumarpilli commented Mar 5, 2021 •

edited

Loading

amaury1093 commented Mar 5, 2021

akhilkumarpilli commented Mar 5, 2021

akhilkumarpilli commented Mar 5, 2021

fix multisig account pubkeys migration #8788

fix multisig account pubkeys migration #8788

Conversation

akhilkumarpilli commented Mar 4, 2021

Description

alessio commented Mar 4, 2021

codecov bot commented Mar 4, 2021

Codecov Report

akhilkumarpilli commented Mar 4, 2021

aaronc commented Mar 4, 2021

alessio commented Mar 4, 2021

zmanian commented Mar 4, 2021

zmanian commented Mar 4, 2021

alessio commented Mar 4, 2021

boz commented Mar 5, 2021

anilcse left a comment

Choose a reason for hiding this comment

anilcse commented Mar 5, 2021 • edited Loading

anilcse commented Mar 5, 2021

amaury1093 commented Mar 5, 2021

akhilkumarpilli commented Mar 5, 2021 • edited Loading

amaury1093 commented Mar 5, 2021

akhilkumarpilli commented Mar 5, 2021

akhilkumarpilli commented Mar 5, 2021

anilcse commented Mar 5, 2021 •

edited

Loading

akhilkumarpilli commented Mar 5, 2021 •

edited

Loading