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: Add MigrationModuleManager to handle migration of upgrade module before other modules #16583

Merged
merged 35 commits into from
Aug 11, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
1e55c09
add WithConsensusParamsGetter for manager
mmsqe Jun 15, 2023
5d513a3
test WithConsensusParamsGetter
mmsqe Jun 16, 2023
3fcd4d6
test continuously call BeginBlock
mmsqe Jun 16, 2023
34d47d1
add doc
mmsqe Jun 16, 2023
c21cff7
rm module name check
mmsqe Jun 16, 2023
ea9e96a
add change log
mmsqe Jun 16, 2023
721fb8b
Update CHANGELOG.md
mmsqe Jun 16, 2023
3158f6a
fix interface
mmsqe Jun 16, 2023
e54fc1a
fix lint
mmsqe Jun 16, 2023
81038ec
Apply suggestions from code review
mmsqe Jun 20, 2023
36a3944
Merge remote-tracking branch 'origin/main' into fix_cp
mmsqe Jun 20, 2023
5e43e65
cleanup doc
mmsqe Jun 16, 2023
9c3a25b
fix resolve
mmsqe Jun 20, 2023
5bca160
Merge remote-tracking branch 'origin/main' into fix_cp
mmsqe Jun 26, 2023
9f6994d
fix lint
mmsqe Jun 26, 2023
591d7cf
add ck
mmsqe Jun 27, 2023
e3f80d6
Revert "add ck"
mmsqe Jun 27, 2023
26e9624
add ConsensusParamsGetter interface
mmsqe Jun 27, 2023
6e50d86
Merge remote-tracking branch 'origin/main' into fix_cp
mmsqe Jul 17, 2023
42fc450
fix doc
mmsqe Jul 18, 2023
59f4bef
Merge remote-tracking branch 'origin/main' into fix_cp
mmsqe Aug 9, 2023
120cdae
handle upgrade 1st
mmsqe Aug 9, 2023
74746c9
Update x/upgrade/module.go
mmsqe Aug 10, 2023
7c812be
Merge remote-tracking branch 'origin/main' into fix_cp
mmsqe Aug 10, 2023
bbf4eef
fix doc
mmsqe Aug 10, 2023
ca99e59
replace WithConsensusParamsGetter with MigrationModuleManager
mmsqe Aug 10, 2023
86bc9c0
fix test
mmsqe Aug 10, 2023
1a1b3ce
Apply suggestions from code review
mmsqe Aug 10, 2023
5400774
check nil
mmsqe Aug 11, 2023
2b56da6
update doc
mmsqe Aug 11, 2023
95c6ed9
add test
mmsqe Aug 11, 2023
08a9279
Merge remote-tracking branch 'origin/main' into fix_cp
mmsqe Aug 11, 2023
6ceebbf
update doc
mmsqe Aug 11, 2023
71ea378
Apply suggestions from code review
mmsqe Aug 11, 2023
ff7f08d
Merge branch 'main' into fix_cp
julienrbrt Aug 11, 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,11 @@ Ref: https://keepachangelog.com/en/1.0.0/
* remove `Keeper`: `IterateValidatorAccumulatedCommission`, `GetValidatorAccumulatedCommission`, `SetValidatorAccumulatedCommission`, `DeleteValidatorAccumulatedCommission`
* (x/distribution) [#16590](https://github.com/cosmos/cosmos-sdk/pull/16590) use collections for `ValidatorOutstandingRewards` state management:
* remove `Keeper`: `IterateValidatorOutstandingRewards`, `GetValidatorOutstandingRewards`, `SetValidatorOutstandingRewards`, `DeleteValidatorOutstandingRewards`

### Bug Fixes

* (x/auth/types) [#16554](https://github.com/cosmos/cosmos-sdk/pull/16554) `ModuleAccount.Validate` now reports a nil `.BaseAccount` instead of panicking.
* (migrations) [#16583](https://github.com/cosmos/cosmos-sdk/pull/16583) Add `WithConsensusParamsGetter` to allow other modules to access updated context with consensus parameters within the same block that executes migration.
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
* (baseapp) [#16613](https://github.com/cosmos/cosmos-sdk/pull/16613) Ensure each message in a transaction has a registered handler, otherwise `CheckTx` will fail.

## [v0.50.0-alpha.0](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.0-alpha.0) - 2023-06-07
Expand Down
2 changes: 1 addition & 1 deletion runtime/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func SetupAppBuilder(inputs AppInputs) {
app.config = inputs.Config
app.appConfig = inputs.AppConfig
app.logger = inputs.Logger
app.ModuleManager = module.NewManagerFromMap(inputs.Modules)
app.ModuleManager = module.NewManagerFromMap(inputs.Modules).WithConsensusParamsGetter(app)

for name, mod := range inputs.Modules {
if customBasicMod, ok := inputs.CustomModuleBasics[name]; ok {
Expand Down
28 changes: 25 additions & 3 deletions types/module/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,9 @@ import (
"github.com/spf13/cobra"
"golang.org/x/exp/maps"

storetypes "cosmossdk.io/store/types"

errorsmod "cosmossdk.io/errors"

storetypes "cosmossdk.io/store/types"
tmproto "github.com/cometbft/cometbft/proto/tendermint/types"
Copy link
Contributor

Choose a reason for hiding this comment

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

@tac0turtle is there any harm on depending on cometbft types in types/module?

Copy link
Member

Choose a reason for hiding this comment

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

ideally we dont do this, this seems like a bandaid that we will have to clean up later on. i think we should identify how to fix this problem without allowing modules to override consensus params.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We must be doing this within BeginBlock of Manager since the consensus params is accessed by other modules right after migration within the same block execution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I guess there's no way around it it seems :/

Copy link
Contributor

Choose a reason for hiding this comment

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

We could perhaps return an any instead and then type cast where needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if get params by consensusKeeper is better, but we need add module one by one (not to mention module outside sdk)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tac0turtle May I ask if you have any better idea beside these two approaches?

Copy link
Collaborator

@yihuang yihuang Aug 9, 2023

Choose a reason for hiding this comment

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

I think the core issue here is the migration is "system" level functionalities, probably shouldn't happen in a module in the first place, it should be executed before all the module logics. is it possible to move the migration logic into baseapp somehow?

Copy link
Member

Choose a reason for hiding this comment

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

sorry for delay, the dependency is fine. agree with @yihuang. Maybe for this version of baseapp this is fine but for later on we can change it. Im not sure we have a clean alternative

Copy link
Contributor

Choose a reason for hiding this comment

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

100% agree 👍

"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/codec/types"
Expand Down Expand Up @@ -262,6 +261,11 @@ func (GenesisOnlyAppModule) EndBlock(sdk.Context) ([]abci.ValidatorUpdate, error
return []abci.ValidatorUpdate{}, nil
}

// ConsensusParamsGetter is an interface to retrieve consensus parameters for a given context.
type ConsensusParamsGetter interface {
GetConsensusParams(ctx sdk.Context) tmproto.ConsensusParams
Copy link
Contributor

Choose a reason for hiding this comment

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

what if we just return an any here and then type cast? Then we can avoid the CometBFT dependency.

Copy link
Contributor Author

@mmsqe mmsqe Jun 27, 2023

Choose a reason for hiding this comment

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

since need use as cp := getter.GetConsensusParams(ctx).(tmproto.ConsensusParams), we can't use any for WithConsensusParams, or can we add a separate package to avoid import cometbft type directly

}

// Manager defines a module manager that provides the high level utility for managing and executing
// operations for a group of modules
type Manager struct {
Expand All @@ -273,6 +277,7 @@ type Manager struct {
OrderPrepareCheckStaters []string
OrderPrecommiters []string
OrderMigrations []string
consensusParamsGetter ConsensusParamsGetter
}

// NewManager creates a new Manager object.
Expand Down Expand Up @@ -319,6 +324,12 @@ func NewManagerFromMap(moduleMap map[string]appmodule.AppModule) *Manager {
}
}

// WithConsensusParamsGetter sets ConsensusParamsGetter for Manager.
func (m *Manager) WithConsensusParamsGetter(g ConsensusParamsGetter) *Manager {
m.consensusParamsGetter = g
return m
}

// SetOrderInitGenesis sets the order of init genesis calls
func (m *Manager) SetOrderInitGenesis(moduleNames ...string) {
m.assertNoForgottenModules("SetOrderInitGenesis", moduleNames, func(moduleName string) bool {
Expand Down Expand Up @@ -701,6 +712,17 @@ func (m *Manager) BeginBlock(ctx sdk.Context) (sdk.BeginBlock, error) {
if err != nil {
return sdk.BeginBlock{}, err
}
if m.consensusParamsGetter != nil {
cp := ctx.ConsensusParams()
// Manager skips this step if Block is non-nil since upgrade module is expected to set this params
// and consensus parameters should not be overwritten.
if cp.Block == nil {
cp = m.consensusParamsGetter.GetConsensusParams(ctx)
Fixed Show fixed Hide fixed
if cp.Block != nil {
ctx = ctx.WithConsensusParams(cp)
}
}
}
}
}

Expand Down
65 changes: 65 additions & 0 deletions types/module/module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/json"
"errors"
"io"
"reflect"
"testing"

"cosmossdk.io/core/appmodule"
Expand Down Expand Up @@ -481,6 +482,70 @@ func TestCoreAPIManager_BeginBlock(t *testing.T) {
require.EqualError(t, err, "some error")
}

type MockConsensusParamGetter struct {
ctrl *gomock.Controller
recorder *MockConsensusParamGetterRecorder
}

type MockConsensusParamGetterRecorder struct {
mock *MockConsensusParamGetter
}

func NewMockConsensusParamGetter(ctrl *gomock.Controller) *MockConsensusParamGetter {
mock := &MockConsensusParamGetter{ctrl: ctrl}
mock.recorder = &MockConsensusParamGetterRecorder{mock}
return mock
}

func (m *MockConsensusParamGetter) EXPECT() *MockConsensusParamGetterRecorder {
return m.recorder
}

func (m *MockConsensusParamGetter) GetConsensusParams(arg0 sdk.Context) cmtproto.ConsensusParams {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "GetConsensusParams", arg0)
ret0, _ := ret[0].(cmtproto.ConsensusParams)
return ret0
}

func (mr *MockConsensusParamGetterRecorder) GetConsensusParams(arg0 interface{}) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetConsensusParams", reflect.TypeOf((*MockConsensusParamGetter)(nil).GetConsensusParams), arg0)
}

func TestManager_BeginBlock_WithConsensusParamsGetter(t *testing.T) {
mockCtrl := gomock.NewController(t)
t.Cleanup(mockCtrl.Finish)

mockUpgradeModule := mock.NewMockCoreAppModule(mockCtrl)
mockParamsGetter := NewMockConsensusParamGetter(mockCtrl)

// Create a Manager with the mock consensusParamsGetter and an upgrade module
m := module.NewManagerFromMap(map[string]appmodule.AppModule{
"upgrade": mockUpgradeModule,
}).WithConsensusParamsGetter(mockParamsGetter)

// Create a context with a nil consensus params object and an empty event manager
ctx := sdk.Context{}

mockUpgradeModule.EXPECT().BeginBlock(gomock.Any()).Times(1).Return(nil)
mockParamsGetter.EXPECT().GetConsensusParams(gomock.Any()).Times(1).Return(cmtproto.ConsensusParams{
Block: &cmtproto.BlockParams{MaxBytes: 1000},
})
_, err := m.BeginBlock(ctx)
require.NoError(t, err)

// Create a context with a consensus params object and an empty event manager
ctx = sdk.Context{}.WithConsensusParams(cmtproto.ConsensusParams{
Block: &cmtproto.BlockParams{MaxBytes: 1000},
})

mockUpgradeModule.EXPECT().BeginBlock(gomock.Any()).Times(1).Return(nil)
mockParamsGetter.EXPECT().GetConsensusParams(gomock.Any()).Times(0)
_, err = m.BeginBlock(ctx)
require.NoError(t, err)
}

func TestCoreAPIManager_EndBlock(t *testing.T) {
mockCtrl := gomock.NewController(t)
t.Cleanup(mockCtrl.Finish)
Expand Down