Only change rootmultistore hash when substore hashes change

[ethanfrey]

Closes #1351

The fix was proposed one year ago, and was recently moved to "proposal accepted". This enables no-empty-block to be enabled with cosmos-sdk code without any other breaking changes.

• 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.

• Wrote tests

• Updated relevant documentation (docs/)

• Added a relevant changelog entry: clog add [section] [stanza] [message]

• rereviewed Files changed in the github PR explorer

Note: I don't know of any relevant documentation as this is a bugfix. Please point me to any points I need to update if any.

---

For Admin Use:

• Added appropriate labels to PR (ex. wip, ready-for-review, docs)
• Reviewers Assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[codecov]

Codecov Report

Merging #4613 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4613      +/-   ##
==========================================
+ Coverage   53.62%   53.63%   +0.01%     
==========================================
  Files         260      260              
  Lines       16177    16177              
==========================================
+ Hits         8675     8677       +2     
+ Misses       6856     6854       -2     
  Partials      646      646

[ethanfrey]

Please note that this issue was brought up as a problem by some groups during the Hackatom Berlin.

Also, this not being resolved has prompted other quick fixes, which would have much more breaking impacts, such as cosmos/iavl#143

[alexanderbez]

Concept ACK. I'm pretty sure this is state machine breaking, this I've added the label. Non-contentious update regardless. Thanks @ethanfrey 👍

[alessio]

Nihil obstat from me

[ethanfrey]

Concept ACK. I'm pretty sure this is state machine breaking, this I've added the label. Non-contentious update regardless. Thanks @ethanfrey +1

Yes, this will break all app hashes and require a chain restart for an upgrade to new version. I am fine if this merge waits until there are other state-machine breaking changes lined up, to all go in one release.

[alexanderbez]

@ethanfrey we have a significant amount of breaking changes already in master and ready for the next major upgrade. I see no reason we can't get this in now :)

[cwgoes]

Concept ACK, code utACK

We should note somewhere that applications which utilize no_empty_blocks will not have regular block timing and should use e.g. BFT timestamps for any periodic EndBlock / BeginBlock logic.

[alexanderbez]

We should note somewhere that applications which utilize no_empty_blocks will not have regular block timing and should use e.g. BFT timestamps for any periodic EndBlock / BeginBlock logic.

Suggestions on a location @cwgoes?

[cwgoes]

Suggestions on a location @cwgoes?

In the default configuration file and in comments above EndBlock in the module abstraction perhaps.

[mdyring]

Thanks for the work on this @ethanfrey - much appreciated!

[ethanfrey]

Concept ACK, code utACK

What does utACK mean?

I will add comments about no_empty_blocks to the two suggested locations later on today.

[cwgoes]

What does utACK mean?

Acknowledged (read the code, looks correct) but untested.

I will add comments about no_empty_blocks to the two suggested locations later on today.

Thanks.

[jackzampolin]

Is this something we want to get in for gaia 1.0?

[alexanderbez]

@jackzampolin yes. I'll be merging this once @ethanfrey updates some docs.

[ethanfrey]

I rebased on master and added some comments to BeginBlocker/EndBlocker interfaces, which will be used be developers.

baseapp.BaseApp.{Begin,End}Block seemed like large code blocks that only core devs look at, so I didn't add docs there (but could if you want).

I could not find the default configuration besides client/config.go with no mention of no_empty_blocks in any toml file in the database. I assume this comes from the tendermint default toml file and the comment would have to go there:

https://github.com/tendermint/tendermint/blob/4253e67c07c69be6d7f7263ab03944ce30a9fc90/config/toml.go#L294-L297

(I also see that the name is now create_empty_blocks and will update the docs accordingly)

[ethanfrey]

I just found this PR: https://github.com/cosmos/cosmos-sdk/pull/2284/files

I guess I should revert that as well. Does that belong in this PR or another one?

[alexanderbez]

I just found this PR: https://github.com/cosmos/cosmos-sdk/pull/2284/files

I guess I should revert that as well. Does that belong in this PR or another one?

Yes, let's create another PR for that too.

I think we can safely merge this.

[mostcute]

@alexanderbez @ethanfrey
Hey friends,I want to support create-empty-block=false but i don't want to update my sdk version. how many changes i have to merge to my code? my sdk version is 0.34.4
I have merge the diff in #4613 && #4624.
and set the config.toml as

timeout_commit = "1s"

# Make progress as soon as we have all the precommits (as if TimeoutCommit = 0)
skip_timeout_commit = false

# EmptyBlocks mode and possible interval between empty blocks
create_empty_blocks = false
create_empty_blocks_interval = "10s"

But my program still product block per second

I have read more discuss about this function

#1351 (comment)

It seems the slashing and distribution prevent the "no_empty_block" function.
Is that ture i can get the func after i remove the two modules?
Does any other module affect this function?