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

refactor: migrate consensus to collections #15553

Merged
merged 16 commits into from
Mar 28, 2023
Merged
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (baseapp) [#15023](https://github.com/cosmos/cosmos-sdk/pull/15023) & [#15213](https://github.com/cosmos/cosmos-sdk/pull/15213) Add `MessageRouter` interface to baseapp and pass it to authz, gov and groups instead of concrete type.
* (simtestutil) [#15305](https://github.com/cosmos/cosmos-sdk/pull/15305) Add `AppStateFnWithExtendedCb` with callback function to extend rawState.
* (x/distribution) [#15462](https://github.com/cosmos/cosmos-sdk/pull/15462) Add delegator address to the event for withdrawing delegation rewards
* (x/consensus) [#15553](https://github.com/cosmos/cosmos-sdk/pull/15553) Migrate consensus module to use collections

### State Machine Breaking

Expand Down
50 changes: 25 additions & 25 deletions api/cosmos/consensus/v1/query.pulsar.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 13 additions & 13 deletions api/cosmos/consensus/v1/query_grpc.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 4 additions & 5 deletions baseapp/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func (app *BaseApp) InitChain(req abci.RequestInitChain) (res abci.ResponseInitC
// done after the deliver state and context have been set as it's persisted
// to state.
if req.ConsensusParams != nil {
err := app.StoreConsensusParams(app.deliverState.ctx, req.ConsensusParams)
err := app.StoreConsensusParams(app.deliverState.ctx, *req.ConsensusParams)
if err != nil {
panic(err)
}
Expand Down Expand Up @@ -239,9 +239,8 @@ func (app *BaseApp) EndBlock(req abci.RequestEndBlock) (res abci.ResponseEndBloc
res.Events = sdk.MarkEventsToIndex(res.Events, app.indexEvents)
}

if cp := app.GetConsensusParams(app.deliverState.ctx); cp != nil {
res.ConsensusParamUpdates = cp
}
cp := app.GetConsensusParams(app.deliverState.ctx)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call
res.ConsensusParamUpdates = &cp

// call the streaming service hook with the EndBlock messages
for _, abciListener := range app.streamingManager.ABCIListeners {
Expand Down Expand Up @@ -880,7 +879,7 @@ func (app *BaseApp) GetBlockRetentionHeight(commitHeight int64) int64 {
// on the unbonding period and block commitment time as the two should be
// equivalent.
cp := app.GetConsensusParams(app.deliverState.ctx)
if cp != nil && cp.Evidence != nil && cp.Evidence.MaxAgeNumBlocks > 0 {
if cp.Evidence != nil && cp.Evidence.MaxAgeNumBlocks > 0 {
retentionHeight = commitHeight - cp.Evidence.MaxAgeNumBlocks
}

Expand Down
14 changes: 5 additions & 9 deletions baseapp/baseapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -434,29 +434,25 @@ func (app *BaseApp) setState(mode runTxMode, header cmtproto.Header) {

// GetConsensusParams returns the current consensus parameters from the BaseApp's
// ParamStore. If the BaseApp has no ParamStore defined, nil is returned.
func (app *BaseApp) GetConsensusParams(ctx sdk.Context) *cmtproto.ConsensusParams {
func (app *BaseApp) GetConsensusParams(ctx sdk.Context) cmtproto.ConsensusParams {
if app.paramStore == nil {
return nil
return cmtproto.ConsensusParams{}
}

cp, err := app.paramStore.Get(ctx)
if err != nil {
panic(err)
panic(fmt.Errorf("consensus key is nil: %w", err))

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The key isn't nil, the value is empty/nil.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I'll amend in another pr.

}

return cp
}

// StoreConsensusParams sets the consensus parameters to the baseapp's param store.
func (app *BaseApp) StoreConsensusParams(ctx sdk.Context, cp *cmtproto.ConsensusParams) error {
func (app *BaseApp) StoreConsensusParams(ctx sdk.Context, cp cmtproto.ConsensusParams) error {
if app.paramStore == nil {
panic("cannot store consensus params with no params store set")
}

if cp == nil {
return nil
}

return app.paramStore.Set(ctx, cp)
// We're explicitly not storing the CometBFT app_version in the param store. It's
// stored instead in the x/upgrade store, with its own bump logic.
Expand All @@ -474,7 +470,7 @@ func (app *BaseApp) AddRunTxRecoveryHandler(handlers ...RecoveryHandler) {
// one.
func (app *BaseApp) GetMaximumBlockGas(ctx sdk.Context) uint64 {
cp := app.GetConsensusParams(ctx)
if cp == nil || cp.Block == nil {
if cp.Block == nil {
return 0
}

Expand Down
10 changes: 5 additions & 5 deletions baseapp/baseapp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func NewBaseAppSuite(t *testing.T, opts ...func(*baseapp.BaseApp)) *BaseAppSuite
app.SetInterfaceRegistry(cdc.InterfaceRegistry())
app.MsgServiceRouter().SetInterfaceRegistry(cdc.InterfaceRegistry())
app.MountStores(capKey1, capKey2)
app.SetParamStore(&paramStore{db: dbm.NewMemDB()})
app.SetParamStore(paramStore{db: dbm.NewMemDB()})
app.SetTxDecoder(txConfig.TxDecoder())
app.SetTxEncoder(txConfig.TxEncoder())

Expand Down Expand Up @@ -587,16 +587,16 @@ func TestGetMaximumBlockGas(t *testing.T) {
suite.baseApp.InitChain(abci.RequestInitChain{})
ctx := suite.baseApp.NewContext(true, cmtproto.Header{})

suite.baseApp.StoreConsensusParams(ctx, &cmtproto.ConsensusParams{Block: &cmtproto.BlockParams{MaxGas: 0}})
suite.baseApp.StoreConsensusParams(ctx, cmtproto.ConsensusParams{Block: &cmtproto.BlockParams{MaxGas: 0}})
require.Equal(t, uint64(0), suite.baseApp.GetMaximumBlockGas(ctx))

suite.baseApp.StoreConsensusParams(ctx, &cmtproto.ConsensusParams{Block: &cmtproto.BlockParams{MaxGas: -1}})
suite.baseApp.StoreConsensusParams(ctx, cmtproto.ConsensusParams{Block: &cmtproto.BlockParams{MaxGas: -1}})
require.Equal(t, uint64(0), suite.baseApp.GetMaximumBlockGas(ctx))

suite.baseApp.StoreConsensusParams(ctx, &cmtproto.ConsensusParams{Block: &cmtproto.BlockParams{MaxGas: 5000000}})
suite.baseApp.StoreConsensusParams(ctx, cmtproto.ConsensusParams{Block: &cmtproto.BlockParams{MaxGas: 5000000}})
require.Equal(t, uint64(5000000), suite.baseApp.GetMaximumBlockGas(ctx))

suite.baseApp.StoreConsensusParams(ctx, &cmtproto.ConsensusParams{Block: &cmtproto.BlockParams{MaxGas: -5000000}})
suite.baseApp.StoreConsensusParams(ctx, cmtproto.ConsensusParams{Block: &cmtproto.BlockParams{MaxGas: -5000000}})
require.Panics(t, func() { suite.baseApp.GetMaximumBlockGas(ctx) })
}

Expand Down
4 changes: 2 additions & 2 deletions baseapp/params.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
// ParamStore defines the interface the parameter store used by the BaseApp must
// fulfill.
type ParamStore interface {
Get(ctx context.Context) (*cmtproto.ConsensusParams, error)
Get(ctx context.Context) (cmtproto.ConsensusParams, error)
Has(ctx context.Context) (bool, error)
Set(ctx context.Context, cp *cmtproto.ConsensusParams) error
Set(ctx context.Context, cp cmtproto.ConsensusParams) error
}
7 changes: 5 additions & 2 deletions baseapp/params_legacy.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,13 @@ func GetConsensusParams(ctx sdk.Context, paramStore LegacyParamStore) *cmtproto.
return cp
}

func MigrateParams(ctx sdk.Context, lps LegacyParamStore, ps ParamStore) {
func MigrateParams(ctx sdk.Context, lps LegacyParamStore, ps ParamStore) error {
if cp := GetConsensusParams(ctx, lps); cp != nil {
ps.Set(ctx, cp)
if err := ps.Set(ctx, *cp); err != nil {
return err
}
} else {
ctx.Logger().Info("warning: consensus parameters are undefined; skipping migration")
}
return nil
}
14 changes: 7 additions & 7 deletions baseapp/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ type paramStore struct {

var _ baseapp.ParamStore = (*paramStore)(nil)

func (ps *paramStore) Set(_ context.Context, value *cmtproto.ConsensusParams) error {
func (ps paramStore) Set(_ context.Context, value cmtproto.ConsensusParams) error {
bz, err := json.Marshal(value)
if err != nil {
return err
Expand All @@ -250,26 +250,26 @@ func (ps *paramStore) Set(_ context.Context, value *cmtproto.ConsensusParams) er
return ps.db.Set(ParamStoreKey, bz)
}

func (ps *paramStore) Has(_ context.Context) (bool, error) {
func (ps paramStore) Has(_ context.Context) (bool, error) {
return ps.db.Has(ParamStoreKey)
}

func (ps paramStore) Get(ctx context.Context) (*cmtproto.ConsensusParams, error) {
func (ps paramStore) Get(_ context.Context) (cmtproto.ConsensusParams, error) {
bz, err := ps.db.Get(ParamStoreKey)
if err != nil {
return nil, err
return cmtproto.ConsensusParams{}, err
}

if len(bz) == 0 {
return nil, errors.New("params not found")
return cmtproto.ConsensusParams{}, errors.New("params not found")
}

var params cmtproto.ConsensusParams
if err := json.Unmarshal(bz, &params); err != nil {
return nil, err
return cmtproto.ConsensusParams{}, err
}

return &params, nil
return params, nil
}

func setTxSignature(t *testing.T, builder client.TxBuilder, nonce uint64) {
Expand Down
6 changes: 3 additions & 3 deletions proto/cosmos/consensus/v1/query.proto
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,16 @@ option go_package = "github.com/cosmos/cosmos-sdk/x/consensus/types";
// Query defines the gRPC querier service.
service Query {
// Params queries the parameters of x/consensus_param module.
rpc Params(QueryParamsRequest) returns (QueryParamsResponse) {
rpc GetParams(QueryGetParamsRequest) returns (QueryGetParamsResponse) {
option (google.api.http).get = "/cosmos/consensus/v1/params";
}
}

// QueryParamsRequest defines the request type for querying x/consensus parameters.
message QueryParamsRequest {}
message QueryGetParamsRequest {}

// QueryParamsResponse defines the response type for querying x/consensus parameters.
message QueryParamsResponse {
message QueryGetParamsResponse {
Copy link
Member

Choose a reason for hiding this comment

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

Is that proto breaking warning ok to be ignored? cc @AmauryM

Copy link
Member

Choose a reason for hiding this comment

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

Reverted in #15627

// params are the tendermint consensus params stored in the consensus module.
// Please note that `params.version` is not populated in this response, it is
// tracked separately in the x/upgrade module.
Expand Down
4 changes: 2 additions & 2 deletions server/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func isZeroExportedApp(a types.ExportedApp) bool {
return a.AppState == nil &&
len(a.Validators) == 0 &&
a.Height == 0 &&
a.ConsensusParams == nil
a.ConsensusParams == cmtproto.ConsensusParams{}
}

// mockExporter provides an Export method matching server/types.AppExporter,
Expand All @@ -127,7 +127,7 @@ type mockExporter struct {
// when e.Export is called.
func (e *mockExporter) SetDefaultExportApp() {
e.ExportApp = types.ExportedApp{
ConsensusParams: &cmtproto.ConsensusParams{
ConsensusParams: cmtproto.ConsensusParams{
Block: &cmtproto.BlockParams{
MaxBytes: 5 * 1024 * 1024,
MaxGas: -1,
Expand Down
3 changes: 1 addition & 2 deletions server/mock/app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,11 @@ import (
"testing"
"time"

"cosmossdk.io/log"
abci "github.com/cometbft/cometbft/abci/types"
cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
"github.com/stretchr/testify/require"

"cosmossdk.io/log"

simtypes "github.com/cosmos/cosmos-sdk/types/simulation"
genutiltypes "github.com/cosmos/cosmos-sdk/x/genutil/types"
)
Expand Down
2 changes: 1 addition & 1 deletion server/types/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ type (
// Height is the app's latest block height.
Height int64
// ConsensusParams are the exported consensus params for ABCI.
ConsensusParams *cmtproto.ConsensusParams
ConsensusParams cmtproto.ConsensusParams
}

// AppExporter is a function that dumps all app state to
Expand Down
2 changes: 1 addition & 1 deletion simapp/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ func NewSimApp(

// set the BaseApp's parameter store
app.ConsensusParamsKeeper = consensusparamkeeper.NewKeeper(appCodec, runtime.NewKVStoreService(keys[consensusparamtypes.StoreKey]), authtypes.NewModuleAddress(govtypes.ModuleName).String())
bApp.SetParamStore(&app.ConsensusParamsKeeper)
bApp.SetParamStore(app.ConsensusParamsKeeper.Params)

// add keepers
app.AccountKeeper = authkeeper.NewAccountKeeper(appCodec, runtime.NewKVStoreService(keys[authtypes.StoreKey]), authtypes.ProtoBaseAccount, maccPerms, sdk.Bech32MainPrefix, authtypes.NewModuleAddress(govtypes.ModuleName).String())
Expand Down
Loading