fix: remove stores from renamed/deleted store upgrades #9409

technicallyty · 2021-05-27T20:35:46Z

Description

stores that were renamed are now properly deleted
deleted/renamed and renamed stores are no longer added to CommitInfo
deleted/renamed stores are now properly removed from rootmulti store's memory

ref: #7991
closes: N/A

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 · 2021-05-27T20:44:53Z

Codecov Report

Merging #9409 (21b451b) into master (0027111) will increase coverage by 24.96%.
The diff coverage is 62.97%.

❗ Current head 21b451b differs from pull request most recent head 8f44ec5. Consider uploading reports for the commit 8f44ec5 to get more accurate results

@@             Coverage Diff             @@
##           master    #9409       +/-   ##
===========================================
+ Coverage   35.48%   60.44%   +24.96%     
===========================================
  Files         332      590      +258     
  Lines       32620    37253     +4633     
===========================================
+ Hits        11575    22518    +10943     
+ Misses      19819    12754     -7065     
- Partials     1226     1981      +755

Impacted Files	Coverage Δ
client/keys/types.go	`100.00% <ø> (+100.00%)`	⬆️
client/keys/utils.go	`42.85% <ø> (+2.50%)`	⬆️
client/query.go	`16.98% <ø> (ø)`
client/rpc/block.go	`10.00% <ø> (ø)`
client/rpc/routes.go	`100.00% <ø> (ø)`
client/rpc/status.go	`47.72% <ø> (ø)`
client/rpc/validators.go	`0.00% <ø> (ø)`
client/test_helpers.go	`0.00% <ø> (ø)`
client/tx/factory.go	`28.20% <ø> (ø)`
client/tx/legacy.go	`68.42% <ø> (ø)`
... and 701 more

clevinson · 2021-06-15T22:32:22Z

@robert-zaremba @aaronc can you of you have a look at this and give an ack if all looks good?

tac0turtle

utACK.

For upgrades, would we want to keep renamed stores around and route to the old module if state is still present on the node for it?

robert-zaremba

I'm missing a context here.

What problem are we solving? Fix upgrate store loader #7991 doesn't provide a clear context either.
Why do we need to remember deleted stores?
What should happen if someone wants to save a data to a deleted store? I suggest we should fail (return error). We are missing a test for that.
What should happen when we restart an app?
Documentation is missing.

robert-zaremba · 2021-06-17T11:10:55Z

store/rootmulti/store.go


-	rs.lastCommitInfo = commitStores(version, rs.stores)
+	// remove remnants of removed stores
+	for sk := range rs.removalMap {


Suggested change

for sk := range rs.removalMap {

for key := range rs.removalMap {

technicallyty · 2021-06-17T15:50:17Z

re: @robert-zaremba

What problem are we solving? Fix upgrate store loader #7991 doesn't provide a clear context either.

Renamed stores were not being removed from storage. Since the original store from a rename was never added to the newStore map, once a commit happened - the commit store would just assume no changes were made to that store, so it would leave it as is.

Why do we need to remember deleted stores?

The stores that need to be deleted are only held temporarily in the removalMap until the next commit. Once a commit happens, the map is used to clear the root store of the stores in the map (ones that need to be deleted). Without that, renamed and deleted stores persist to memory, and renamed stores persist to storage, rather than being cleared.

What should happen if someone wants to save a data to a deleted store? I suggest we should fail (return error). We are missing a test for that.

Wouldn't you first have to acquire the key? I believe we already have errors to handle that situation, but i can look into fleshing out a more direct test for this situation though

What should happen when we restart an app?

not sure - do you see any issues that could arise from this?

Documentation is missing.

i can add some lines to cosmos-sdk/store readme

technicallyty · 2021-06-17T16:18:44Z

re: @marbar3778

For upgrades, would we want to keep renamed stores around and route to the old module if state is still present on the node for it?

hmm could you explain this situation more? i'd assumed a rename with new module would be more of a hard fork situation

tac0turtle · 2021-06-17T16:57:43Z

hmm could you explain this situation more? i'd assumed a rename with new module would be more of a hard fork situation

oh, I was thinking in the case of rolling upgrades. In the case of a hard fork I don't think this would matter cause migration scripts would handle it.

technicallyty · 2021-06-17T17:32:53Z

oh, I was thinking in the case of rolling upgrades. In the case of a hard fork I don't think this would matter cause migration scripts would handle it.

ahh okay gotcha. are there situations where devs would inject StoreUpgrades outside of a migration script? maybe i'm missing the other use cases. my first thought here was to just add docs for devs to not run store upgrades outside a migration script.

anilcse · 2021-06-17T17:48:31Z

hmm could you explain this situation more? i'd assumed a rename with new module would be more of a hard fork situation

oh, I was thinking in the case of rolling upgrades. In the case of a hard fork I don't think this would matter cause migration scripts would handle it.

iirc on renaming a store, all the data is moved/copied over to the new store.

robert-zaremba

utACK

technicallyty added 10 commits May 24, 2021 08:36

daily

d61f55b

Merge branch 'master' into ty/na-store_loader

2dba501

daily

b864464

Merge branch 'master' into ty/na-store_loader

736c3f3

daily

fa5656f

fix commit info

bccfbf9

Merge branch 'master' into ty/na-store_loader

65b72f9

fix store upgrades

400b366

cleanup

e8e4cd4

remove all

b5efa1f

github-actions bot added the C:x/upgrade label May 27, 2021

technicallyty mentioned this pull request May 27, 2021

Fix upgrate store loader #7991

Closed

linter

69b29bd

technicallyty requested review from amaury1093, alexanderbez, aaronc and ethanfrey May 27, 2021 21:55

technicallyty added 6 commits May 28, 2021 13:04

Merge branch 'master' into ty/na-store_loader

5047266

Merge branch 'master' into ty/na-store_loader

1483f68

Merge branch 'master' into ty/na-store_loader

1952529

Merge branch 'master' into ty/na-store_loader

bd364fe

Merge branch 'master' into ty/na-store_loader

e2b7122

Merge branch 'master' into ty/na-store_loader

21b451b

technicallyty marked this pull request as ready for review June 15, 2021 22:32

technicallyty requested a review from robert-zaremba June 15, 2021 22:51

tac0turtle approved these changes Jun 17, 2021

View reviewed changes

robert-zaremba reviewed Jun 17, 2021

View reviewed changes

amaury1093 self-assigned this Jun 21, 2021

technicallyty requested a review from robert-zaremba July 9, 2021 16:16

technicallyty added the review label Jul 12, 2021

robert-zaremba approved these changes Jul 12, 2021

View reviewed changes

technicallyty added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 12, 2021

Merge branch 'master' into ty/na-store_loader

8f44ec5

mergify bot merged commit d4d25f5 into master Jul 12, 2021

mergify bot deleted the ty/na-store_loader branch July 12, 2021 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove stores from renamed/deleted store upgrades #9409

fix: remove stores from renamed/deleted store upgrades #9409

technicallyty commented May 27, 2021 •

edited

Loading

codecov bot commented May 27, 2021 •

edited

Loading

clevinson commented Jun 15, 2021 •

edited

Loading

tac0turtle left a comment

robert-zaremba left a comment •

edited

Loading

robert-zaremba Jun 17, 2021

technicallyty commented Jun 17, 2021

technicallyty commented Jun 17, 2021

tac0turtle commented Jun 17, 2021

technicallyty commented Jun 17, 2021

anilcse commented Jun 17, 2021

robert-zaremba left a comment

	for sk := range rs.removalMap {
	for key := range rs.removalMap {

fix: remove stores from renamed/deleted store upgrades #9409

fix: remove stores from renamed/deleted store upgrades #9409

Conversation

technicallyty commented May 27, 2021 • edited Loading

Description

codecov bot commented May 27, 2021 • edited Loading

Codecov Report

clevinson commented Jun 15, 2021 • edited Loading

tac0turtle left a comment

Choose a reason for hiding this comment

robert-zaremba left a comment • edited Loading

Choose a reason for hiding this comment

robert-zaremba Jun 17, 2021

Choose a reason for hiding this comment

technicallyty commented Jun 17, 2021

technicallyty commented Jun 17, 2021

tac0turtle commented Jun 17, 2021

technicallyty commented Jun 17, 2021

anilcse commented Jun 17, 2021

robert-zaremba left a comment

Choose a reason for hiding this comment

technicallyty commented May 27, 2021 •

edited

Loading

codecov bot commented May 27, 2021 •

edited

Loading

clevinson commented Jun 15, 2021 •

edited

Loading

robert-zaremba left a comment •

edited

Loading