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

fix: use initialHeight correctly #15789

Merged
merged 6 commits into from
Apr 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Bug Fixes

* (baseapp) [#15789](https://github.com/cosmos/cosmos-sdk/pull/15789) Ensure `PrepareProposal` and `ProcessProposal` respect `InitialHeight` set by CometBFT when set to a value greater than 1.
* (types) [#15691](https://github.com/cosmos/cosmos-sdk/pull/15691) Make Coin.Validate() check that .Amount is not nil
* (types) [#15433](https://github.com/cosmos/cosmos-sdk/pull/15433) Allow disabling of account address caches (for printing bech32 account addresses).
* (x/auth) [#15059](https://github.com/cosmos/cosmos-sdk/pull/15059) `ante.CountSubKeys` returns 0 when passing a nil `Pubkey`.
Expand Down
17 changes: 10 additions & 7 deletions baseapp/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,14 @@ func (app *BaseApp) InitChain(req abci.RequestInitChain) (res abci.ResponseInitC

app.logger.Info("InitChain", "initialHeight", req.InitialHeight, "chainID", req.ChainId)

// If req.InitialHeight is > 1, then we set the initial version in the
// stores.
// Set the initial height, which will be used to determine if we are proposing
// or processing the first block or not.
app.initialHeight = req.InitialHeight

// if req.InitialHeight is > 1, then we set the initial version on all stores
if req.InitialHeight > 1 {
app.initialHeight = req.InitialHeight
initHeader = cmtproto.Header{ChainID: req.ChainId, Height: req.InitialHeight, Time: req.Time}
err := app.cms.SetInitialVersion(req.InitialHeight)
if err != nil {
initHeader.Height = req.InitialHeight
Copy link
Member

Choose a reason for hiding this comment

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

any clues why we are doing this here and not above?

if err := app.cms.SetInitialVersion(req.InitialHeight); err != nil {
panic(err)
}
}
Expand Down Expand Up @@ -1017,11 +1018,13 @@ func SplitABCIQueryPath(requestPath string) (path []string) {
// ProcessProposal. We use deliverState on the first block to be able to access
// any state changes made in InitChain.
func (app *BaseApp) getContextForProposal(ctx sdk.Context, height int64) sdk.Context {
if height == 1 {
if height == app.initialHeight {
ctx, _ = app.deliverState.ctx.CacheContext()

// clear all context data set during InitChain to avoid inconsistent behavior
ctx = ctx.WithBlockHeader(cmtproto.Header{})
return ctx
}

return ctx
}
1 change: 1 addition & 0 deletions baseapp/abci_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1407,6 +1407,7 @@ func TestABCI_Proposal_Read_State_PrepareProposal(t *testing.T) {
suite := NewBaseAppSuite(t, setInitChainerOpt, prepareOpt)

suite.baseApp.InitChain(abci.RequestInitChain{
InitialHeight: 1,
ConsensusParams: &cmtproto.ConsensusParams{},
})

Expand Down
20 changes: 11 additions & 9 deletions baseapp/baseapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -493,19 +493,21 @@ func (app *BaseApp) validateHeight(req abci.RequestBeginBlock) error {
return fmt.Errorf("invalid height: %d", req.Header.Height)
}

// expectedHeight holds the expected height to validate.
lastBlockHeight := app.LastBlockHeight()

// expectedHeight holds the expected height to validate
var expectedHeight int64
if app.LastBlockHeight() == 0 && app.initialHeight > 1 {
// In this case, we're validating the first block of the chain (no
// previous commit). The height we're expecting is the initial height.
if lastBlockHeight == 0 && app.initialHeight > 1 {
// In this case, we're validating the first block of the chain, i.e no
// previous commit. The height we're expecting is the initial height.
expectedHeight = app.initialHeight
} else {
// This case can mean two things:
// - either there was already a previous commit in the store, in which
// case we increment the version from there,
// - or there was no previous commit, and initial version was not set,
// in which case we start at version 1.
expectedHeight = app.LastBlockHeight() + 1
//
// - Either there was already a previous commit in the store, in which
// case we increment the version from there.
// - Or there was no previous commit, in which case we start at version 1.
expectedHeight = lastBlockHeight + 1
}

Comment on lines 493 to 512
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).validateHeight (baseapp/baseapp.go:491)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).BeginBlock (baseapp/baseapp.go:162)

if req.Header.Height != expectedHeight {
Expand Down