GameOfZones Phase 1: Fix to solve capability initialization

[jackzampolin]

Port @AdityaSripal fix to working branch of SDK

Superceeds: #6126

[codecov]

Codecov Report

Merging #6127 into master will decrease coverage by 0.00%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #6127      +/-   ##
==========================================
- Coverage   54.64%   54.64%   -0.01%     
==========================================
  Files         443      443              
  Lines       26655    26657       +2     
==========================================
  Hits        14566    14566              
- Misses      11092    11093       +1     
- Partials      997      998       +1     

[alexanderbez]

Can you explain why and when this happens?

[AdityaSripal]

This is a hack, it never happens during actual chain execution. But we just pass the height in as -1 in the app initialization so that the NewContext function goes into this condition instead of the checkTx or deliverTx conditions.

This allows us to return a context that has CommitMultiStore directly inside of it, which allows us to write the capabilities directly into the memstore during InitializeAndSeal

The problem before was that we were using the BaseApp.NewContext with checkTx's multistore. But both checkTx and deliverTx will wrap the multistore in a CacheMultiStore. So all the writes we made to stores in InitializeAndSeal were being written to CacheStore. Since we didn't (and couldn't) call Commit before the chain started, these cached writes were simply discarded when the chain started.

We didn't have a way to Write the cacheStore from inside NewApp. So we had to create a way to return a context that was un-cached, so that we could write directly into memstores and not have to commit afterwards. That's what these changes accomplish

[alexanderbez]

I see; that makes sense. It's a bit scary that we didn't actually catch this in testing or even some sort of minimal manual testing of running a network.

In any case, can we at the very least add a simple test case and update the godoc for NewContext to explain what happens when a negative height is passed?

[cwgoes]

Agreed on documentation. I also wonder if we should avoid using this negative-height trick entirely, it just seems confusing - maybe we should make a function that allows app.cms to be accessed directly, but can no longer be called after app.Seal() or something like that?

[alexanderbez]

Agreed. We should either take that route or create an entirely new separate method that solely uses the cms (e.g. NewContextFromMultiStore).

[alexanderbez]

https://github.com/cosmos/cosmos-sdk/pull/6127/files#r419175073

[cwgoes]

I understand the issue; I think a separate function might be less likely to be misused.

[cwgoes]

Agreed on documentation. I also wonder if we should avoid using this negative-height trick entirely, it just seems confusing - maybe we should make a function that allows app.cms to be accessed directly, but can no longer be called after app.Seal() or something like that?