fix: gov and group amino and snapshot hashes #49

ryanchristo · 2022-12-12T19:27:38Z

Description

This pull request adds the gov and group module amino codec fixes that were added in the following pull requests but were not included in cosmos sdk v0.46.7:

This pull request also includes RegisterLegacyAminoCodec in the mint module added in the following pull request:

feat: deprecate x/params usage in x/mint cosmos/cosmos-sdk#12363

Update

Another issue was discovered as tests were failing for me locally (using go 1.19) and intermittently on github (using go 1.18) and so I discovered cosmos#13373, which was resolved with two pull requests:

Also noticed the latest commit added to the release branch that fixes another failing test:

fix(store): Fix the TestFileStreamingService unit test. cosmos/cosmos-sdk#14305

Update

To resolve the snapshot mismatch between this branch and main, one more pull request was added:

feat: Make extension snapshotter interface safer to use cosmos/cosmos-sdk#11825

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

ryanchristo · 2022-12-12T19:28:55Z

Pending cosmos#14103 (and therefore cosmos#14214)

clevinson · 2022-12-12T23:59:48Z

Can we cherry pick the PRs individually from upstream so they show up in the commit log?

* fix!(group): Register types with Amino * Chagenlog

* fix!: Fix gov amino codec * changelog

* fix!: Fix group amino codec * changelog

…ork/cosmos-sdk into ryan/add-amino-fixes

ryanchristo · 2022-12-14T21:46:47Z

The tests that are failing appear to be failing on release/v0.46.x. Should this be a concern?

…mos#14305) (cosmos#14309) Co-authored-by: Daniel Wedul <github@wedul.com> Co-authored-by: Julien Robert <julien@rbrt.fr>

ryanchristo · 2022-12-15T00:31:22Z

We may want to clean this up and merge in separate pull requests but changes we need to make should all be included here to fix amino and to fix snapshots (and therefore require go 1.19 in regen ledger).

ryanchristo · 2022-12-15T00:54:39Z

One item I'm not totally sure on is the changes included from 2ee3503.

I excluded some changes and had to update some of the hashes separately, because some things do not exist in the release branch that were included in the original pull request.

clevinson

Went through all the diff files (comparing against the source PRs in the description line-by-line), and I think is looking good to me!

A few comments, pointing out the hashes @ryanchristo alluded to in his last comment. Would appreciate an SDK perspective on this if @AmauryM you can have a look?

Otherwise, giving this an ACK. I'm in general support of us adding in the last commits so we ensure building with go 1.19 works as expected for regen ledger v5.0.

clevinson · 2022-12-15T00:38:54Z

snapshots/manager_test.go

@@ -91,7 +91,7 @@ func TestManager_Take(t *testing.T) {
 		Height: 5,
 		Format: snapshotter.SnapshotFormat(),
 		Chunks: 1,
-		Hash:   []uint8{0xcd, 0x17, 0x9e, 0x7f, 0x28, 0xb6, 0x82, 0x90, 0xc7, 0x25, 0xf3, 0x42, 0xac, 0x65, 0x73, 0x50, 0xaa, 0xa0, 0x10, 0x5c, 0x40, 0x8c, 0xd5, 0x1, 0xed, 0x82, 0xb5, 0xca, 0x8b, 0xe0, 0x83, 0xa2},
+		Hash:   []uint8{0xcf, 0xd8, 0x16, 0xd2, 0xf8, 0x11, 0xe8, 0x90, 0x92, 0xf1, 0xfe, 0x3b, 0xea, 0xd2, 0x94, 0xfc, 0xfa, 0x4f, 0x9e, 0x2a, 0x91, 0xbe, 0xb0, 0x50, 0x83, 0xe9, 0x28, 0x62, 0x48, 0x6a, 0x4b, 0x4},


These hashes are different from the hashes in the tests on cosmos#13400 (see snapshots/manager_test.go).

@AmauryM any thoughts as to whether this might be a problem?

I think we should understand why there's a hash mismatch before merging this PR. Currently I'm not sure, and I also don't know if this will break state sync on regen-ledger or not.

Looking at git blames on the snapshot-related files, I noticed this addition wasn't included in this PR: https://github.com/cosmos/cosmos-sdk/blame/main/store/snapshots/store.go#L263. I also see we do a proto.Marshal() on the snaphot object. Maybe it's worth giving a try at backporting cosmos#13070 here?

Resolved with 8985cf8

clevinson · 2022-12-15T00:53:21Z

store/v2alpha1/multi/snapshot_test.go

@@ -151,7 +151,7 @@ func TestMultistoreSnapshot_Checksum(t *testing.T) {
 			"05dfef0e32c34ef3900300f9de51f228d7fb204fa8f4e4d0d1529f083d122029",
 			"77d30aeeb427b0bdcedf3639adde1e822c15233d652782e171125280875aa492",
 			"c00c3801da889ea4370f0e647ffe1e291bd47f500e2a7269611eb4cc198b993f",
-			"6d565eb28776631f3e3e764decd53436c3be073a8a01fa5434afd539f9ae6eda",
+			"df5b8eeea83ca7f1e10824d0161bff46200ca12c02f50c1f42c8ed156368493e",


This hash update also seems to not be in a source PR (e.g. cosmos#13400)... Would be good to confirm that there's nothing to worry about here.

Store v2alpha1 is not used, so I'm not too worried about this. Though I suspect the reason of this hash mismatch is the same as the comment above.

clevinson · 2022-12-15T01:07:03Z

x/mint/types/codec.go

+
+// RegisterLegacyAminoCodec registers concrete types on the LegacyAmino codec
+func RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {
+	cdc.RegisterConcrete(Params{}, "cosmos-sdk/x/mint/Params", nil)


I think it's OK to keep this added, but I'm not sure if its actually necessary since we don't have MsgUpdateParams in v0.46.x. Either way, I don't think its harmful to add the registration here.

amaury1093 · 2022-12-15T10:47:44Z

x/gov/types/v1/codec.go

+	RegisterLegacyAminoCodec(govcodec.Amino)
+	RegisterLegacyAminoCodec(groupcodec.Amino)


One thing we shouldn't forget to do in the PR that integrates this into regen-ledger, is to add these 2 lines to regen-ledger's modules too

Included in regen-network/regen-ledger#1694

amaury1093 · 2022-12-15T11:25:38Z

store/v2alpha1/multi/snapshot_test.go

@@ -151,7 +151,7 @@ func TestMultistoreSnapshot_Checksum(t *testing.T) {
 			"05dfef0e32c34ef3900300f9de51f228d7fb204fa8f4e4d0d1529f083d122029",
 			"77d30aeeb427b0bdcedf3639adde1e822c15233d652782e171125280875aa492",
 			"c00c3801da889ea4370f0e647ffe1e291bd47f500e2a7269611eb4cc198b993f",
-			"6d565eb28776631f3e3e764decd53436c3be073a8a01fa5434afd539f9ae6eda",
+			"df5b8eeea83ca7f1e10824d0161bff46200ca12c02f50c1f42c8ed156368493e",


Store v2alpha1 is not used, so I'm not too worried about this. Though I suspect the reason of this hash mismatch is the same as the comment above.

amaury1093 · 2022-12-15T11:27:57Z

snapshots/manager_test.go

@@ -91,7 +91,7 @@ func TestManager_Take(t *testing.T) {
 		Height: 5,
 		Format: snapshotter.SnapshotFormat(),
 		Chunks: 1,
-		Hash:   []uint8{0xcd, 0x17, 0x9e, 0x7f, 0x28, 0xb6, 0x82, 0x90, 0xc7, 0x25, 0xf3, 0x42, 0xac, 0x65, 0x73, 0x50, 0xaa, 0xa0, 0x10, 0x5c, 0x40, 0x8c, 0xd5, 0x1, 0xed, 0x82, 0xb5, 0xca, 0x8b, 0xe0, 0x83, 0xa2},
+		Hash:   []uint8{0xcf, 0xd8, 0x16, 0xd2, 0xf8, 0x11, 0xe8, 0x90, 0x92, 0xf1, 0xfe, 0x3b, 0xea, 0xd2, 0x94, 0xfc, 0xfa, 0x4f, 0x9e, 0x2a, 0x91, 0xbe, 0xb0, 0x50, 0x83, 0xe9, 0x28, 0x62, 0x48, 0x6a, 0x4b, 0x4},


I think we should understand why there's a hash mismatch before merging this PR. Currently I'm not sure, and I also don't know if this will break state sync on regen-ledger or not.

Looking at git blames on the snapshot-related files, I noticed this addition wasn't included in this PR: https://github.com/cosmos/cosmos-sdk/blame/main/store/snapshots/store.go#L263. I also see we do a proto.Marshal() on the snaphot object. Maybe it's worth giving a try at backporting cosmos#13070 here?

* Make extension snapshotter interface safer to use Closes: cosmos#11824 Solution: - Use new methods `SnapshotExtension`/`RestoreExtension` to handle payload stream specifically. - Improve unit tests. * update changelog * Update snapshots/types/util.go * changelog * go linter * Update CHANGELOG.md Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

ryanchristo · 2022-12-16T00:05:45Z

snapshots/manager_test.go

@@ -91,7 +95,7 @@ func TestManager_Take(t *testing.T) {
 		Height: 5,
 		Format: snapshotter.SnapshotFormat(),
 		Chunks: 1,
-		Hash:   []uint8{0xcf, 0xd8, 0x16, 0xd2, 0xf8, 0x11, 0xe8, 0x90, 0x92, 0xf1, 0xfe, 0x3b, 0xea, 0xd2, 0x94, 0xfc, 0xfa, 0x4f, 0x9e, 0x2a, 0x91, 0xbe, 0xb0, 0x50, 0x83, 0xe9, 0x28, 0x62, 0x48, 0x6a, 0x4b, 0x4},
+		Hash:   []uint8{0xc5, 0xf7, 0xfe, 0xea, 0xd3, 0x4d, 0x3e, 0x87, 0xff, 0x41, 0xa2, 0x27, 0xfa, 0xcb, 0x38, 0x17, 0xa, 0x5, 0xeb, 0x27, 0x4e, 0x16, 0x5e, 0xf3, 0xb2, 0x8b, 0x47, 0xd1, 0xe6, 0x94, 0x7e, 0x8b},


This was updated manually within the last commit to align with what was originally expected.

i.e. what it was updated to in cosmos#13400

ryanchristo · 2022-12-16T00:19:24Z

This did not fail for me locally but will try to rerun tests to see if intermittent:

https://github.com/regen-network/cosmos-sdk/actions/runs/3708969320/jobs/6287086423#step:6:183

ryanchristo · 2022-12-16T00:44:15Z

This did not fail for me locally but will try to rerun tests to see if intermittent:

Seems to be intermittent and unrelated.

clevinson · 2022-12-16T02:14:27Z

Just took a final pass over the latest commit comparing it to the source PR (cosmos#11825). I also verified that hashes now match what we expected from backporting cosmos#13400.

Looks like we didn't need to back port cosmos#13070 after all? Makes sense to me since the line in question that @AmauryM pointed out (defer chunkBody.Close()) only actually had an update of the //nolint comment, so there isn't any actual changes to store.go from this PR from what I can tell.

ACK from me 🎉

fix: register gov and group amino codec

f0db6bc

github-actions bot added C:Simulations C:x/auth C:x/authz C:x/bank C:x/crisis C:x/distribution C:x/evidence C:x/feegrant C:x/gov C:x/mint C:x/slashing C:x/staking C:x/upgrade C:x/group labels Dec 12, 2022

ryanchristo requested review from aaronc, aleem1314, amaury1093, blushi and clevinson December 12, 2022 19:29

ryanchristo changed the base branch from release/v0.46.x to release/v0.46.7-regen December 14, 2022 04:55

amaury1093 and others added 4 commits December 13, 2022 21:42

fix(group)!: Register types with Amino (cosmos#13592)

cb7b2b2

* fix!(group): Register types with Amino * Chagenlog

fix!: Fix gov amino codec (cosmos#13196)

c5a1f2a

* fix!: Fix gov amino codec * changelog

fix!: Fix group amino codec (cosmos#13307)

638d47e

* fix!: Fix group amino codec * changelog

Merge branch 'release/v0.46.7-regen' of https://github.com/regen-netw…

aec8723

…ork/cosmos-sdk into ryan/add-amino-fixes

ryanchristo force-pushed the ryan/add-amino-fixes branch from 4b46d09 to aec8723 Compare December 14, 2022 05:58

build(deps): bump go minimum version to go 1.19 (cosmos#13397)

eb0ee54

github-actions bot added C:Cosmovisor C:Rosetta Type: Build Type: CI C:Store C:orm labels Dec 15, 2022

fix: double close (cosmos#13400)

2ee3503

ryanchristo force-pushed the ryan/add-amino-fixes branch from 09a3d24 to 2ee3503 Compare December 15, 2022 00:25

fix(store): Fix the TestFileStreamingService unit test. (backport cos…

6a9ec03

…mos#14305) (cosmos#14309) Co-authored-by: Daniel Wedul <github@wedul.com> Co-authored-by: Julien Robert <julien@rbrt.fr>

ryanchristo changed the title ~~fix: register gov and group amino codec~~ fix: gov and group amino and bump go version Dec 15, 2022

ryanchristo marked this pull request as ready for review December 15, 2022 01:08

clevinson approved these changes Dec 15, 2022

View reviewed changes

ryanchristo mentioned this pull request Dec 15, 2022

build: update go version to 1.19 regen-network/regen-ledger#1689

Merged

19 tasks

ryanchristo changed the title ~~fix: gov and group amino and bump go version~~ fix: gov and group amino and snapshot hashes Dec 15, 2022

amaury1093 reviewed Dec 15, 2022

View reviewed changes

blushi mentioned this pull request Dec 15, 2022

Show detailed message info in Keplr regen-network/regen-web#1982

Closed

github-actions bot added the Type: ADR label Dec 16, 2022

ryanchristo commented Dec 16, 2022

View reviewed changes

ryanchristo merged commit 3c9dcb9 into release/v0.46.7-regen Dec 16, 2022

ryanchristo deleted the ryan/add-amino-fixes branch December 16, 2022 02:20

This was referenced Dec 16, 2022

chore: cherry-pick gov and group amino support #50

Closed

build: use sdk v0.46.7-regen and register amino regen-network/regen-ledger#1694

Merged

ryanchristo mentioned this pull request Feb 22, 2023

chore: merge v0.46.7-regen-2 into release/v0.46.10-regen #52

Merged

19 tasks

ryanchristo mentioned this pull request May 31, 2023

helpers for nested create Any objects cosmology-tech/telescope#281

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: gov and group amino and snapshot hashes #49

fix: gov and group amino and snapshot hashes #49

ryanchristo commented Dec 12, 2022 •

edited

Loading

ryanchristo commented Dec 12, 2022

clevinson commented Dec 12, 2022 •

edited

Loading

ryanchristo commented Dec 14, 2022 •

edited

Loading

ryanchristo commented Dec 15, 2022

ryanchristo commented Dec 15, 2022

clevinson left a comment

clevinson Dec 15, 2022

amaury1093 Dec 15, 2022

ryanchristo Dec 16, 2022

clevinson Dec 15, 2022

amaury1093 Dec 15, 2022

clevinson Dec 15, 2022

amaury1093 Dec 15, 2022

ryanchristo Dec 16, 2022

amaury1093 Dec 15, 2022

amaury1093 Dec 15, 2022

ryanchristo Dec 16, 2022 •

edited

Loading

ryanchristo Dec 16, 2022 •

edited

Loading

ryanchristo commented Dec 16, 2022

ryanchristo commented Dec 16, 2022

clevinson commented Dec 16, 2022 •

edited

Loading

		RegisterLegacyAminoCodec(govcodec.Amino)
		RegisterLegacyAminoCodec(groupcodec.Amino)

fix: gov and group amino and snapshot hashes #49

fix: gov and group amino and snapshot hashes #49

Conversation

ryanchristo commented Dec 12, 2022 • edited Loading

Description

Update

Update

Author Checklist

Reviewers Checklist

ryanchristo commented Dec 12, 2022

clevinson commented Dec 12, 2022 • edited Loading

ryanchristo commented Dec 14, 2022 • edited Loading

ryanchristo commented Dec 15, 2022

ryanchristo commented Dec 15, 2022

clevinson 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanchristo Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

ryanchristo Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

ryanchristo commented Dec 16, 2022

ryanchristo commented Dec 16, 2022

clevinson commented Dec 16, 2022 • edited Loading

ryanchristo commented Dec 12, 2022 •

edited

Loading

clevinson commented Dec 12, 2022 •

edited

Loading

ryanchristo commented Dec 14, 2022 •

edited

Loading

ryanchristo Dec 16, 2022 •

edited

Loading

ryanchristo Dec 16, 2022 •

edited

Loading

clevinson commented Dec 16, 2022 •

edited

Loading