fix: empty app hash on first block's FinalizeBlock #18524
Conversation
WalkthroughThe update to the Changes
TipsChat with CodeRabbit Bot (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| )) | ||
| } | ||
|
|
||
| // In the case of a new chain, AppHash will be the hash of an empty string. | ||
| // During an upgrade, it'll be the hash of the last committed block. | ||
| var appHash []byte | ||
| if !app.LastCommitID().IsZero() { | ||
| appHash = app.LastCommitID().Hash | ||
| } else { | ||
| // $ echo -n '' | sha256sum | ||
| // e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 | ||
| emptyHash := sha256.Sum256([]byte{}) | ||
| appHash = emptyHash[:] | ||
| } |
There was a problem hiding this comment.
Change potentially affects state.
Call sequence:
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).internalFinalizeBlock (baseapp/abci.go:712)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).FinalizeBlock (baseapp/abci.go:869)
There was a problem hiding this comment.
Is this really correct? In the case of the first block of a new chain, the app hash, IIRC, is the genesis hash which is supplied during BeginBlock.
There was a problem hiding this comment.
This is what I observed, i.e. a nil app hash. Also set up a BeginBlock under x/wasm of wasmd and observed the same behavior. Regarding the contents of the hash, I followed suit with what InitChain was doing, but not sure if this is the intended behavior.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- baseapp/abci.go (2 hunks)
Additional comments: 2
baseapp/abci.go (2)
726-749: The code correctly handles the generation of
appHashbased on whether the last commit ID is zero or not. This is a crucial change that addresses the issue of an emptyAppHashfor the first block after genesis. The logic to use the hash of an empty string when there is no last commit is consistent with the behavior inInitChain. This change should ensure that theAppHashis not nil even when there is no last commit, which is the expected behavior for the first block.763-766: The
coreheader.Infostruct is being populated with theappHashwhich is correctly computed in the previous lines. This ensures that theAppHashis consistent and reflects the correct state of the application after theInitChainor the last commit. This is a critical fix for the issue described in the pull request summary.
|
My understanding on why this happened: Until v0.47.x the BeginBlockRequest included the appHash (which was returned before by InitChain), so that would get put into the context. On v0.50.x we don't have the app hash in the FinalizeBlock request so we take it directly from the store, which can be nil and we don't do the "special treatment" we have in InitChain. |
| // In the case of a new chain, AppHash will be the hash of an empty string. | ||
| // During an upgrade, it'll be the hash of the last committed block. |
There was a problem hiding this comment.
| // In the case of a new chain, AppHash will be the hash of an empty string. | |
| // During an upgrade, it'll be the hash of the last committed block. | |
| // In the case of a new chain, AppHash will be the hash of an empty string | |
| // for the first block. During an upgrade, it'll be the hash of the last committed block. |
| header := cmtproto.Header{ | ||
| ChainID: app.chainID, | ||
| Height: req.Height, | ||
| Time: req.Time, | ||
| ProposerAddress: req.ProposerAddress, | ||
| NextValidatorsHash: req.NextValidatorsHash, | ||
| AppHash: app.LastCommitID().Hash, |
There was a problem hiding this comment.
whynot set app.LastCommitID() to an empty value on start? this would help in avoiding the above code and potentially make it cleaner
|
Nil app hash is not a thing, we return the hash of an empty byte slice, which is different. I believe the problem is it's not present on the 1st block during |
|
That makes sense. Maybe we should disallow nil apphashes in general? would be a simple fix in store that would avoid having to be defensive every where we call .Hash() |
|
did this pr to illustrate what i meant in my comment. #18563 Personally i like to root cause being fixed instead of at the source causing use to have to patch in many areas |
Description
The
FinalizeBlockcall for the first block after genesis contains a nilAppHashasapp.LastCommitId().Hashisnil. This PR uses the same functionality asInitChainref to handle an empty app hash for the first height.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
appHashis determined and utilized.