Fix upgrate store loader

[dgsbl]

Description

Add the deleted store, or the delete operation will not be committed, and the data in old store will not be properly deleted.

[dgsbl]

if the deleted store is not added in the method loadVersion(), the tc.origStoreKey will still have data ,and v will never be nil

[alexanderbez]

looks reasonable to me, but I do not have too much context here. I do recall @ethanfrey implementing this logic so it'd be good to get a review/ACK from him too.

[codecov]

Codecov Report

Merging #7991 (3bdd955) into master (513daba) will increase coverage by 1.32%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7991      +/-   ##
==========================================
+ Coverage   61.61%   62.93%   +1.32%     
==========================================
  Files         606      687      +81     
  Lines       34791    45152   +10361     
==========================================
+ Hits        21435    28416    +6981     
- Misses      11129    13892    +2763     
- Partials     2227     2844     +617     

Impacted Files	Coverage Δ
store/rootmulti/store.go	66.12% <100.00%> (+0.07%)	⬆️
x/bank/keeper/grpc_query.go	50.00% <0.00%> (-20.22%)	⬇️
types/module/configurator.go	17.24% <0.00%> (-16.10%)	⬇️
x/auth/module.go	45.65% <0.00%> (-8.64%)	⬇️
client/broadcast.go	57.84% <0.00%> (-8.26%)	⬇️
client/keys/import.go	66.66% <0.00%> (-6.67%)	⬇️
x/evidence/module.go	40.00% <0.00%> (-5.24%)	⬇️
x/slashing/keeper/keeper.go	40.00% <0.00%> (-5.00%)	⬇️
client/keys/list.go	84.00% <0.00%> (-4.89%)	⬇️
x/params/module.go	6.97% <0.00%> (-4.57%)	⬇️
... and 268 more

[amaury1093]

gentle ping for @ethanfrey, I tried to review this PR, but also don't feel super familiar with the code.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[ethanfrey]

I missed this til now. It seems reasonable. I assumed this was committed but too far from the sdk to say for sure now.

It would be good to have a better insight. This is only for store renames, do we need to do something different for deleteStore? Also, if we keep the name after a rename, then we need to explicitly delete it on the next migration to remove it from CommitInfo?

The test that would convince me the most would be to attach a TraceStore to the database, perform this rename (in tests) and check the output of the trace store. Same with deleting a store. To show what operations are actually committed in these simple cases. Also to print out the multistore CommitInfo before and after the upgradeStore operation.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[aaronc]

I think this shouldn't have been closed. I will try to take a look soon.

[technicallyty]

It would be good to have a better insight. This is only for store renames, do we need to do something different for deleteStore? Also, if we keep the name after a rename, then we need to explicitly delete it on the next migration to remove it from CommitInfo?

delete store actually deletes, as its key is put into the newStores map which ensures it gets committed. It is correct that we need to remove these stores from CommitInfo explicitly.

The test that would convince me the most would be to attach a TraceStore to the database, perform this rename (in tests) and check the output of the trace store. Same with deleting a store. To show what operations are actually committed in these simple cases. Also to print out the multistore CommitInfo before and after the upgradeStore operation.

I'm not sure this is possible without editing the loadStores function directly. When LoadLatestVersionAndUpgrade is ran, it doesn't care if you had a traced store or not, since it'll just loop over the keys in memory and cast them to a normal KV store. Only way around this is to edit the loadVersion function directly to utilize tracers, which can be done if needed, but that would be more of a manual test i think.

I was able to get rid of the removed stores from CommitInfo by adding a new field to the root store, removalMap, which will keep a map of keys of stores that needs to be removed. When a commit is run, it adds all known keys except for those in removalMap to commitinfo. once the function is done we remove the entry from the root store's memory and reset the removalMap.

I've created a draft PR implementing this fix

[robert-zaremba]

shall we close this PR? It seams #9409 superseeds this PR