Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return app hash from InitChain #7020

Closed
4 tasks
erikgrinaker opened this issue Aug 12, 2020 · 11 comments · Fixed by #7208
Closed
4 tasks

Return app hash from InitChain #7020

erikgrinaker opened this issue Aug 12, 2020 · 11 comments · Fixed by #7208
Assignees
Labels

Comments

@erikgrinaker
Copy link
Contributor

erikgrinaker commented Aug 12, 2020

Summary

In tendermint/tendermint#5227 (will be released in 0.34.0-rc3), Tendermint added ResponseInitChain.app_hash which replaces the value in the genesis file and is recorded in the genesis block. The SDK should probably return the app hash here instead.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez
Copy link
Contributor

alexanderbez commented Aug 12, 2020

The genesis docs that I've thus far seen typically don't have an AppHash. How is this typically constructed or evaluated? I hope this won't be difficult to implement because, in order to get an app hash during InitChain, we have to commit the root multi-store which bumps the version.

@erikgrinaker
Copy link
Contributor Author

In that case I guess you would just return an empty hash from InitChain. This is application-specific, the motivation was to record the initial app hash in the genesis block and validate it against what's specified in the genesis file. Note that after cosmos/iavl#304 IAVL will return a hash for an empty tree with no versions.

@alexanderbez
Copy link
Contributor

Ok I guess, we can just return an empty hash for now?

@erikgrinaker
Copy link
Contributor Author

erikgrinaker commented Aug 12, 2020

That will work, although the hash of an empty input would probably be more consistent with what IAVL and Tendermint does.

Not sure how this affects upgrades - I believe the genesis file then contains some app state and probably an app hash as well, so InitChain would have to return the same app hash as specified in the genesis file.

@alexanderbez
Copy link
Contributor

alexanderbez commented Aug 12, 2020

I believe there might be some overlap or common bits to shared with #7018.

@erikgrinaker
Copy link
Contributor Author

Following user feedback I've changed the behavior such that ResponseInitChain.app_hash replaces the value from the genesis file instead of having to match it. We may want to e.g. remove the genesis file app hash completely before the final 0.34 release, added https://github.com/tendermint/tendermint/issues/5238 to discuss this and would appreciate the SDK team's input on this.

@alexanderbez
Copy link
Contributor

Just thinking out loud, I think the app_hash only has any semantic meaning when starting from a fork/ugprade, otherwise, there is no pre-existing state and we don't commit on InitChain. With that in mind, maybe we can just return LastCommitID.Hash? In the case of a new chain, it'll be empty. During an upgrade, it'll be the hash of the last committed block.

Thoughts @zmanian?

@erikgrinaker
Copy link
Contributor Author

erikgrinaker commented Aug 13, 2020

I don't know the details of the SDK's state and hashing, but in general, identical states should give identical hashes. If height 2 is empty (contains no user-submitted transaction data), presumably height 1 should have the same hash as height 2?

@alexanderbez
Copy link
Contributor

Not quite, even if there is no new state between 1 and 2, the height/version is increased which gives you a new hash.

@erikgrinaker
Copy link
Contributor Author

erikgrinaker commented Aug 13, 2020

That must be because the SDK changes its internal state then. IAVL returns the same hash for version 1 and 2 if there were no changes.

Either way, this is application-specific, so the SDK is free to choose whatever hashing scheme it finds appropriate. Not having to special-case version 1 for e.g. proofs and stuff might be helpful to users though, but probably not a big deal.

@alexanderbez
Copy link
Contributor

Perhaps yeah, but in InitChain, I think using the last commit ID should work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants