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: ensure height is correct on first block #16259

Merged
merged 5 commits into from
May 24, 2023
Merged

fix: ensure height is correct on first block #16259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3c0cafa
updates
alexanderbez May 23, 2023
50135af
updates
alexanderbez May 23, 2023
5cb7c72
Merge branch 'main' into bez/fix-block-one-ctx
alexanderbez May 23, 2023
637c31d
Merge branch 'main' into bez/fix-block-one-ctx
alexanderbez May 23, 2023
8ffc8b2
Merge branch 'main' into bez/fix-block-one-ctx
alexanderbez May 23, 2023
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Bug Fixes

* (baseapp) [#16259](https://github.com/cosmos/cosmos-sdk/pull/16259) Ensure the `Context` block height is correct after `InitChain` and prior to the second block.
* (x/staking) [#16043](https://github.com/cosmos/cosmos-sdk/pull/16043) Call `AfterUnbondingInitiated` hook for new unbonding entries only and fix `UnbondingDelegation` entries handling
* (types) [#16010](https://github.com/cosmos/cosmos-sdk/pull/16010) Let `module.CoreAppModuleBasicAdaptor` fallback to legacy genesis handling.
* (x/group) [#16017](https://github.com/cosmos/cosmos-sdk/pull/16017) Correctly apply account number in group v2 migration.
Expand Down
14 changes: 12 additions & 2 deletions baseapp/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,16 @@ func (app *BaseApp) InitChain(req abci.RequestInitChain) (res abci.ResponseInitC
}
}

defer func() {
Copy link
Collaborator

@yihuang yihuang May 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

defer is usually for resource cleanup, I think it's clearer to simply put the code after the commit call, for example if panic happens before it, there's no point to still run this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although the tutorial on defer states that, there's no idiomatic usage of it.

defer is often used where e.g. ensure and finally would be used in other languages.

Finally is exactly what we're trying to do here. I think if we do it in Commit, we'll have to explicitly check if height == 0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean just after the initChainer call, I think we don't want to run this if there's any errors or panics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh yes, the reason why I added in the defer is if initChainer == nil, you'll need to add it before the return. That's why I added in the defer instead of having it twice.

// InitChain represents the state of the application BEFORE the first block,
// i.e. the genesis block. This means that when processing the app's InitChain
// handler, the block height is zero by default. However, after Commit is called
// the height needs to reflect the true block height.
initHeader.Height = req.InitialHeight
app.checkState.ctx = app.checkState.ctx.WithBlockHeader(initHeader)
Copy link
Member

Choose a reason for hiding this comment

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

can we set this in header info as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Header info? What do you mean by that?

app.deliverState.ctx = app.deliverState.ctx.WithBlockHeader(initHeader)
}()

if app.initChainer == nil {
return
}
Expand Down Expand Up @@ -119,8 +129,8 @@ func (app *BaseApp) InitChain(req abci.RequestInitChain) (res abci.ResponseInitC
appHash = emptyHash[:]
}

// NOTE: We don't commit, but BeginBlock for block `initial_height` starts from this
// deliverState.
// NOTE: We don't commit, but BeginBlock for block InitialHeight starts from
// this deliverState.
return abci.ResponseInitChain{
ConsensusParams: res.ConsensusParams,
Validators: res.Validators,
Expand Down
27 changes: 20 additions & 7 deletions baseapp/abci_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,17 @@ import (
"strings"
"testing"

dbm "github.com/cosmos/cosmos-db"

abci "github.com/cometbft/cometbft/abci/types"
cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
"github.com/cosmos/gogoproto/jsonpb"
"github.com/stretchr/testify/require"

errorsmod "cosmossdk.io/errors"
"cosmossdk.io/log"
pruningtypes "cosmossdk.io/store/pruning/types"
"cosmossdk.io/store/snapshots"
snapshottypes "cosmossdk.io/store/snapshots/types"
storetypes "cosmossdk.io/store/types"
abci "github.com/cometbft/cometbft/abci/types"
cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
dbm "github.com/cosmos/cosmos-db"
"github.com/cosmos/gogoproto/jsonpb"
"github.com/stretchr/testify/require"

"github.com/cosmos/cosmos-sdk/baseapp"
baseapptestutil "github.com/cosmos/cosmos-sdk/baseapp/testutil"
Expand All @@ -44,6 +42,21 @@ func TestABCI_Info(t *testing.T) {
require.Equal(t, suite.baseApp.AppVersion(), res.AppVersion)
}

func TestABCI_First_block_Height(t *testing.T) {
suite := NewBaseAppSuite(t, baseapp.SetChainID("test-chain-id"))
app := suite.baseApp

app.InitChain(abci.RequestInitChain{
ChainId: "test-chain-id",
ConsensusParams: &cmtproto.ConsensusParams{Block: &cmtproto.BlockParams{MaxGas: 5000000}},
InitialHeight: 1,
})
_ = app.Commit()

ctx := app.GetContextForCheckTx(nil)
require.Equal(t, int64(1), ctx.BlockHeight())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

w/o this fix, this value is actually 0 which would cause CheckTx to fail for txs prior to the 1st block.

}

func TestABCI_InitChain(t *testing.T) {
name := t.Name()
db := dbm.NewMemDB()
Expand Down
4 changes: 4 additions & 0 deletions baseapp/test_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,7 @@ func (app *BaseApp) NewUncachedContext(isCheckTx bool, header cmtproto.Header) s
func (app *BaseApp) GetContextForDeliverTx(txBytes []byte) sdk.Context {
return app.getContextForTx(runTxModeDeliver, txBytes)
}

func (app *BaseApp) GetContextForCheckTx(txBytes []byte) sdk.Context {
return app.getContextForTx(runTxModeCheck, txBytes)
}
1 change: 1 addition & 0 deletions x/auth/ante/sigverify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
errorsmod "cosmossdk.io/errors"
storetypes "cosmossdk.io/store/types"
txsigning "cosmossdk.io/x/tx/signing"

codectypes "github.com/cosmos/cosmos-sdk/codec/types"

"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
Expand Down