store: remove Version from CommitID

[ebuchman]

Unclear what benefit we get from storing the version in the CommitID, especially since it inhibits no-empty-blocks

Thanks @ethanfrey for the investigation in iov-one/weave#72 (comment)

Here's a diff that just hashes the tree hash without the version, without changing any other code:

diff --git a/store/rootmultistore.go b/store/rootmultistore.go
index 11cebc22..11e8b3cd 100644
--- a/store/rootmultistore.go
+++ b/store/rootmultistore.go
@@ -341,7 +341,7 @@ type storeCore struct {
 func (si storeInfo) Hash() []byte {
        // Doesn't write Name, since merkle.SimpleHashFromMap() will
        // include them via the keys.
-       bz, _ := cdc.MarshalBinary(si.Core) // Does not error
+       bz := si.Core.CommitID.Hash
        hasher := ripemd160.New()
        hasher.Write(bz)
        return hasher.Sum(nil)

If we remove staking/slashing from baseapp (which we should, otherwise its not a basecoin!), then no-empty-blocks works with this change

[jaekwon]

We shouldn't assume that the version won't change in a KVStore with no empty blocks.
Many apps will still have per-block updates (e.g. background updates at EndBlocker) that will modify the state. Lets implement something for ResponseEndBlock that tells Tendermint to stop. We discussed this in Berkeley, not sure if you commented here before or after that discussion.

[ebuchman]

not sure if you commented here before or after that discussion.

After.

For some reason I thought it was an issue in the IAVL, but Frey pointed out it was in the MultiStore. Still don't understand what benefit we get from hashing the version in the multistore.

Many apps will still have per-block updates

Sure, but why not make it easier for those that don't ? Many apps will only have changes based around txs.

Lets implement something for ResponseEndBlock that tells Tendermint to stop.

Sure. It adds some complexity, and hard to prioritize given everything else going on. There also seem to be some existing issues with no-empty-blocks that we're working through, eg. see

• tendermint/tendermint@a519825#diff-80343e80650fe43a4394fc26afd5cb3bR463
• tendermint/tendermint@1b400b9#diff-80343e80650fe43a4394fc26afd5cb3bR1385

So we need to make sure that's all fixed up first

[ebuchman]

Opened tendermint/tendermint#1909

I still think we should remove the version from the CommitID if we can to simplify this for users where the state doesn't necessarily transition every block. Eg. we should be able to provide a basecoin that doesn't have to use EndBlock to do this.

[ValarDragon]

I agree with the proposal. Unless there is a concrete reason why version should be in the commit ID hash, we shouldn't include it imo. I can't think of any safety concern here.

[jackzampolin]

Can we move this to proposal accepted @jaekwon @ebuchman ?

[jdkanani]

@ebuchman Any timeline on when this will be merged?

[haasted]

How did this issue end up? Will the suggested change be merged, or are there any plans to support not creating empty blocks in some other way?

[tac0turtle]

@jackzampolin @alexanderbez @jaekwon @ValarDragon can we put this as "proposal accepted"?

[ethanfrey]

I just made a PR with the above fix, along with a simple test. I don't actually use cosmos-sdk, but it bothers me seeing this question from so many projects.

Anyone who wants this functionality for your project, please review / comment on #4613

@haasted This is for you in particular

[alexdupre]

@ebuchman why the app hash changes when there are no transactions but there is the staking module, even if the stake and validators don't changes?

[b00f]

in cosmos-sdk some modules change the app-hash in each height: like slashing and distribution. In case you want to keep app-hash same you need to modify these modules. Go ahead!