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

Jump to bottom

fix: populate ctx.ConsensusParams for begin blockers #10725

Merged
merged 7 commits into from
Dec 15, 2021
Merged

fix: populate ctx.ConsensusParams for begin blockers #10725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7cc6894
populate ctx.ConsensusParams for begin blockers
yihuang Dec 10, 2021
83322ab
Merge branch 'master' into consensus-params
alexanderbez Dec 13, 2021
b781705
chain the WithConsensusParams call
yihuang Dec 14, 2021
f951e03
Merge branch 'master' into consensus-params
yihuang Dec 14, 2021
d1815df
Merge branch 'master' into consensus-params
amaury1093 Dec 14, 2021
4f20fcd
Merge branch 'master' into consensus-params
yihuang Dec 15, 2021
fdacf83
Merge branch 'master' into consensus-params
amaury1093 Dec 15, 2021
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 @@ -189,6 +189,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/bank) [\#9890] (https://github.com/cosmos/cosmos-sdk/pull/9890) Remove duplicate denom from denom metadata key.
* (x/upgrade) [\#10189](https://github.com/cosmos/cosmos-sdk/issues/10189) Removed potential sources of non-determinism in upgrades
* [\#10393](https://github.com/cosmos/cosmos-sdk/pull/10422) Add `MinCommissionRate` param to `x/staking` module.
* [#10725](https://github.com/cosmos/cosmos-sdk/pull/10725) populate `ctx.ConsensusParams` for begin/end blockers.

### Deprecated

Expand Down
3 changes: 2 additions & 1 deletion baseapp/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,8 @@ func (app *BaseApp) BeginBlock(req abci.RequestBeginBlock) (res abci.ResponseBeg

app.deliverState.ctx = app.deliverState.ctx.
WithBlockGasMeter(gasMeter).
WithHeaderHash(req.Hash)
WithHeaderHash(req.Hash).
WithConsensusParams(app.GetConsensusParams(app.deliverState.ctx))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need

cosmos-sdk/baseapp/baseapp.go

Line 491 in b3e922d

ctx = ctx.WithConsensusParams(app.GetConsensusParams(ctx))

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we need it for checkTx?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added a condition to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just add WithConsensusParams(app.GetConsensusParams(app.deliverState.ctx)) with the above app.deliverState.ctx. No need for a separate call.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was worried that the GetConsensusParams might get a uninitialized ctx when I write in that way. The gas meter and header hash not initialized.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In that way, the ctx passed to GetConsensusParams is not initialized with gas meter and block header hash, is that a problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's not a problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it's not a problem.

👍 changed.


// we also set block gas meter to checkState in case the application needs to
// verify gas consumption during (Re)CheckTx
Expand Down
46 changes: 46 additions & 0 deletions baseapp/baseapp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package baseapp_test

import (
"bytes"
"context"
"encoding/binary"
"encoding/json"
"fmt"
Expand Down Expand Up @@ -239,6 +240,51 @@ func TestMountStores(t *testing.T) {
require.NotNil(t, store2)
}

type MockTxHandler struct {
T *testing.T
}

func (th MockTxHandler) CheckTx(goCtx context.Context, req tx.Request, checkReq tx.RequestCheckTx) (tx.Response, tx.ResponseCheckTx, error) {
ctx := sdk.UnwrapSDKContext(goCtx)
require.NotNil(th.T, ctx.ConsensusParams())
return tx.Response{}, tx.ResponseCheckTx{}, nil
}

func (th MockTxHandler) DeliverTx(goCtx context.Context, req tx.Request) (tx.Response, error) {
ctx := sdk.UnwrapSDKContext(goCtx)
require.NotNil(th.T, ctx.ConsensusParams())
return tx.Response{}, nil
}

func (th MockTxHandler) SimulateTx(goCtx context.Context, req tx.Request) (tx.Response, error) {
ctx := sdk.UnwrapSDKContext(goCtx)
require.NotNil(th.T, ctx.ConsensusParams())
return tx.Response{}, nil
}

func TestConsensusParamsNotNil(t *testing.T) {
app := setupBaseApp(t, func(app *baseapp.BaseApp) {
app.SetBeginBlocker(func(ctx sdk.Context, req abci.RequestBeginBlock) abci.ResponseBeginBlock {
require.NotNil(t, ctx.ConsensusParams())
return abci.ResponseBeginBlock{}
})
}, func(app *baseapp.BaseApp) {
app.SetEndBlocker(func(ctx sdk.Context, req abci.RequestEndBlock) abci.ResponseEndBlock {
require.NotNil(t, ctx.ConsensusParams())
return abci.ResponseEndBlock{}
})
}, func(app *baseapp.BaseApp) {
app.SetTxHandler(MockTxHandler{T: t})
})

header := tmproto.Header{Height: 1}
app.BeginBlock(abci.RequestBeginBlock{Header: header})
app.EndBlock(abci.RequestEndBlock{Height: header.Height})
app.CheckTx(abci.RequestCheckTx{})
app.DeliverTx(abci.RequestDeliverTx{})
app.Simulate([]byte{})
}

// Test that we can make commits and then reload old versions.
// Test that LoadLatestVersion actually does.
func TestLoadVersion(t *testing.T) {
Expand Down