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.

Sign up for GitHub

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

fix: empty app hash on first block's FinalizeBlock #18524

Closed
wants to merge 1 commit into from
Closed

fix: empty app hash on first block's FinalizeBlock #18524

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions baseapp/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,13 +726,25 @@ func (app *BaseApp) internalFinalizeBlock(ctx context.Context, req *abci.Request
))
}

// 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.
Comment on lines +729 to +730
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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.

var appHash []byte
if !app.LastCommitID().IsZero() {
appHash = app.LastCommitID().Hash
} else {
// $ echo -n '' | sha256sum
// e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
emptyHash := sha256.Sum256([]byte{})
appHash = emptyHash[:]
}
Comment on lines 726 to +739
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@vitsalis vitsalis Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


header := cmtproto.Header{
ChainID: app.chainID,
Height: req.Height,
Time: req.Time,
ProposerAddress: req.ProposerAddress,
NextValidatorsHash: req.NextValidatorsHash,
AppHash: app.LastCommitID().Hash,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whynot set app.LastCommitID() to an empty value on start? this would help in avoiding the above code and potentially make it cleaner

AppHash: appHash,
}

// finalizeBlockState should be set on InitChain or ProcessProposal. If it is
Expand All @@ -751,7 +763,7 @@ func (app *BaseApp) internalFinalizeBlock(ctx context.Context, req *abci.Request
Height: req.Height,
Time: req.Time,
Hash: req.Hash,
AppHash: app.LastCommitID().Hash,
AppHash: appHash,
}).
WithConsensusParams(app.GetConsensusParams(app.finalizeBlockState.ctx)).
WithVoteInfos(req.DecidedLastCommit.Votes).
Expand Down
Loading