feat!: connect app version with consensus params in end block

CHANGELOG.md

@@ -154,6 +154,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (x/staking) [#14590](https://github.com/cosmos/cosmos-sdk/pull/14590) `MsgUndelegateResponse` now includes undelegated amount. `x/staking` module's `keeper.Undelegate` now returns 3 values (completionTime,undelegateAmount,error) instead of 2.
 * (x/staking) (#15731) (https://github.com/cosmos/cosmos-sdk/pull/15731) Introducing a new index to retrieve the delegations by validator efficiently.
 * (baseapp) [#15930](https://github.com/cosmos/cosmos-sdk/pull/15930) change vote info provided by prepare and process proposal to the one in the block 
+* (x/upgrade) [#16244](https://github.com/cosmos/cosmos-sdk/pull/16244) upgrade module no longer stores the app version but gets and sets the app version stored in the `ParamStore` of baseapp.
 
 ### API Breaking Changes
 
@@ -247,6 +248,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
     * `simulation.NewOperationMsg` is now 2-arity instead of 3-arity with the obsolete argument `codec.ProtoCodec` removed.
     * The field `OperationMsg.Msg` is now of type `[]byte` instead of `json.RawMessage`.
 * (cli) [#16209](https://github.com/cosmos/cosmos-sdk/pull/16209) Add API `StartCmdWithOptions` to create customized start command.
+* (baseapp) [#16244](https://github.com/cosmos/cosmos-sdk/pull/16244) `SetProtocolVersion` has been renamed to `SetAppVersion`. It now updates the consensus params in baseapp's `ParamStore`.
 * (x/auth) [#16016](https://github.com/cosmos/cosmos-sdk/pull/16016) Use collections for accounts state management:
     * removed: keeper `HasAccountByID`, `AccountAddressByID`, `SetParams
 * (x/distribution) [#16211](https://github.com/cosmos/cosmos-sdk/pull/16211) Use collections for params state management.

baseapp/abci.go

@@ -150,11 +150,22 @@ func (app *BaseApp) InitChain(req *abci.RequestInitChain) (*abci.ResponseInitCha
 
 func (app *BaseApp) Info(req *abci.RequestInfo) (*abci.ResponseInfo, error) {
 	lastCommitID := app.cms.LastCommitID()
+	appVersion := InitialAppVersion
+	if lastCommitID.Version > 0 {
+		ctx, err := app.CreateQueryContext(lastCommitID.Version, false)
+		if err != nil {
+			return nil, fmt.Errorf("creating query context: %w", err)
+		}
+		appVersion, err = app.AppVersion(ctx)
+		if err != nil {
+			return nil, fmt.Errorf("getting app version: %w", err)
+		}
+	}
 
 	return &abci.ResponseInfo{
 		Data:             app.name,
 		Version:          app.version,
-		AppVersion:       app.appVersion,
+		AppVersion:       appVersion,
 		LastBlockHeight:  lastCommitID.Version,
 		LastBlockAppHash: lastCommitID.Hash,
 	}, nil

baseapp/abci_test.go

@@ -21,6 +21,12 @@ import (
 	"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"
+	cmttypes "github.com/cometbft/cometbft/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"
@@ -34,6 +40,8 @@ import (
 
 func TestABCI_Info(t *testing.T) {
 	suite := NewBaseAppSuite(t)
+	ctx := suite.baseApp.NewContext(true)
+	suite.baseApp.StoreConsensusParams(ctx, cmttypes.DefaultConsensusParams().ToProto())
 
 	reqInfo := abci.RequestInfo{}
 	res, err := suite.baseApp.Info(&reqInfo)
@@ -43,7 +51,16 @@ func TestABCI_Info(t *testing.T) {
 	require.Equal(t, t.Name(), res.GetData())
 	require.Equal(t, int64(0), res.LastBlockHeight)
 	require.Equal(t, []uint8(nil), res.LastBlockAppHash)
-	require.Equal(t, suite.baseApp.AppVersion(), res.AppVersion)
+	appVersion, err := suite.baseApp.AppVersion(ctx)
+	require.NoError(t, err)
+	require.Equal(t, appVersion, res.AppVersion)
+
+	suite.baseApp.FinalizeBlock(&abci.RequestFinalizeBlock{Height: 1})
+	suite.baseApp.Commit()
+	require.NoError(t, suite.baseApp.SetAppVersion(ctx, 1))
+	res, err = suite.baseApp.Info(&reqInfo)
+	require.NoError(t, err)
+	require.Equal(t, uint64(1), res.AppVersion)
 }
 
 func TestABCI_First_block_Height(t *testing.T) {

baseapp/baseapp.go

@@ -159,10 +159,6 @@ type BaseApp struct {
 	// application's version string
 	version string
 
-	// application's protocol version that increments on every upgrade
-	// if BaseApp is passed to the upgrade keeper's NewKeeper method.
-	appVersion uint64
-
 	// recovery handler for app.runTx method
 	runTxRecoveryMiddleware recoveryMiddleware
 
@@ -240,8 +236,19 @@ func (app *BaseApp) Name() string {
 }
 
 // AppVersion returns the application's protocol version.
-func (app *BaseApp) AppVersion() uint64 {
-	return app.appVersion
+func (app *BaseApp) AppVersion(ctx context.Context) (uint64, error) {
+	if app.paramStore == nil {
+		return 0, errors.New("app.paramStore is nil")
+	}
+
+	cp, err := app.paramStore.Get(ctx)
+	if err != nil {
+		return 0, fmt.Errorf("getting consensus params: %w", err)
+	}
+	if cp.Version == nil {
+		return 0, nil
+	}
+	return cp.Version.App, nil
 }
 
 // Version returns the application's version string.

baseapp/options.go

@@ -1,6 +1,8 @@
 package baseapp
 
 import (
+	"context"
+	"errors"
 	"fmt"
 	"io"
 
@@ -119,9 +121,25 @@ func (app *BaseApp) SetVersion(v string) {
 	app.version = v
 }
 
-// SetProtocolVersion sets the application's protocol version
-func (app *BaseApp) SetProtocolVersion(v uint64) {
-	app.appVersion = v
+// SetAppVersion sets the application's version this is used as part of the
+// header in blocks and is returned to the consensus engine in EndBlock.
+func (app *BaseApp) SetAppVersion(ctx context.Context, v uint64) error {
+	if app.paramStore == nil {
+		return errors.New("param store must be set to set app version")
+	}
+
+	cp, err := app.paramStore.Get(ctx)
+	if err != nil {
+		return err
+	}
+	if cp.Version == nil {
+		return errors.New("version is not set in param store")
+	}
+	cp.Version.App = v
+	if err := app.paramStore.Set(ctx, cp); err != nil {
+		return err
+	}
+	return nil
 }
 
 func (app *BaseApp) SetDB(db dbm.DB) {

baseapp/params.go

@@ -6,10 +6,21 @@ import (
 	cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
 )
 
+const InitialAppVersion uint64 = 0
+
 // ParamStore defines the interface the parameter store used by the BaseApp must
 // fulfill.
 type ParamStore interface {
 	Get(ctx context.Context) (cmtproto.ConsensusParams, error)
 	Has(ctx context.Context) (bool, error)
 	Set(ctx context.Context, cp cmtproto.ConsensusParams) error
 }
+
+// AppVersionModifier defines the interface fulfilled by BaseApp
+// which allows getting and setting it's appVersion field. This
+// in turn updates the consensus params that are sent to the
+// consensus engine in EndBlock
+type AppVersionModifier interface {
+	SetAppVersion(context.Context, uint64) error
+	AppVersion(context.Context) (uint64, error)
+}

runtime/module.go

@@ -73,6 +73,7 @@ func init() {
 			ProvideHeaderInfoService,
 			ProvideCometInfoService,
 			ProvideBasicManager,
+			ProvideAppVersionModifier,
 		),
 		appmodule.Invoke(SetupAppBuilder),
 	)
@@ -259,3 +260,7 @@ func ProvideHeaderInfoService(app *AppBuilder) header.Service {
 func ProvideBasicManager(app *AppBuilder) module.BasicManager {
 	return app.app.basicManager
 }
+
+func ProvideAppVersionModifier(app *AppBuilder) baseapp.AppVersionModifier {
+	return app.app
+}

simapp/app_test.go

@@ -278,8 +278,6 @@ func TestUpgradeStateOnGenesis(t *testing.T) {
 			require.Equal(t, vm[v], i.ConsensusVersion())
 		}
 	}
-
-	require.NotNil(t, app.UpgradeKeeper.GetVersionSetter())
 }
 
 // TestMergedRegistry tests that fetching the gogo/protov2 merged registry

tests/e2e/upgrade/cli_test.go

@@ -19,7 +19,6 @@ func TestE2ETestSuite(t *testing.T) {
 	app := simapp.Setup(t, false)
 	ctx := app.BaseApp.NewContext(false)
 
-	app.UpgradeKeeper.SetVersionSetter(app.BaseApp)
 	app.UpgradeKeeper.SetModuleVersionMap(ctx, app.ModuleManager.GetVersionMap())
 
 	suite.Run(t, NewE2ETestSuite(cfg, app.UpgradeKeeper, ctx))

x/upgrade/abci_test.go

@@ -14,6 +14,10 @@ import (
 	"cosmossdk.io/core/header"
 	"cosmossdk.io/log"
 	storetypes "cosmossdk.io/store/types"
+	cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
+	cmttypes "github.com/cometbft/cometbft/types"
+	"github.com/stretchr/testify/require"
+	"github.com/stretchr/testify/suite"
 	"cosmossdk.io/x/upgrade"
 	"cosmossdk.io/x/upgrade/keeper"
 	"cosmossdk.io/x/upgrade/types"
@@ -56,9 +60,9 @@ func setupTest(t *testing.T, height int64, skip map[int64]bool) *TestSuite {
 		testCtx.DB,
 		s.encCfg.TxConfig.TxDecoder(),
 	)
+	s.baseApp.SetParamStore(&paramStore{params: cmttypes.DefaultConsensusParams().ToProto()})
 
-	s.keeper = keeper.NewKeeper(skip, storeService, s.encCfg.Codec, t.TempDir(), nil, authtypes.NewModuleAddress(govtypes.ModuleName).String())
-	s.keeper.SetVersionSetter(s.baseApp)
+	s.keeper = keeper.NewKeeper(skip, storeService, s.encCfg.Codec, t.TempDir(), s.baseApp, authtypes.NewModuleAddress(govtypes.ModuleName).String())
 
 	s.ctx = testCtx.Ctx.WithHeaderInfo(header.Info{Time: time.Now(), Height: height})
 
@@ -541,3 +545,22 @@ func TestDowngradeVerification(t *testing.T) {
 		}
 	}
 }
+
+type paramStore struct {
+	params cmtproto.ConsensusParams
+}
+
+var _ baseapp.ParamStore = (*paramStore)(nil)
+
+func (ps *paramStore) Set(_ context.Context, value cmtproto.ConsensusParams) error {
+	ps.params = value
+	return nil
+}
+
+func (ps paramStore) Has(_ context.Context) (bool, error) {
+	return true, nil
+}
+
+func (ps paramStore) Get(_ context.Context) (cmtproto.ConsensusParams, error) {
+	return ps.params, nil
+}

x/upgrade/exported/exported.go

@@ -1,7 +1,11 @@
 package exported
 
-// ProtocolVersionSetter defines the interface fulfilled by BaseApp
-// which allows setting it's appVersion field.
-type ProtocolVersionSetter interface {
-	SetProtocolVersion(uint64)
-}
+import (
+	"github.com/cosmos/cosmos-sdk/baseapp"
+)
+
+// AppVersionModifier defines the interface fulfilled by BaseApp
+// which allows getting and setting it's appVersion field. This
+// in turn updates the consensus params that are sent to the
+// consensus engine in EndBlock
+type AppVersionModifier baseapp.AppVersionModifier

x/upgrade/keeper/keeper.go

@@ -41,7 +41,7 @@ type Keeper struct {
 	storeService       corestore.KVStoreService        // key to access x/upgrade store
 	cdc                codec.BinaryCodec               // App-wide binary codec
 	upgradeHandlers    map[string]types.UpgradeHandler // map of plan name to upgrade handler
-	versionSetter      xp.ProtocolVersionSetter        // implements setting the protocol version field on BaseApp
+	versionModifier    xp.AppVersionModifier           // implements setting the protocol version field on BaseApp
 	downgradeVerified  bool                            // tells if we've already sanity checked that this binary version isn't being used against an old state.
 	authority          string                          // the address capable of executing and canceling an upgrade. Usually the gov module account
 	initVersionMap     module.VersionMap               // the module version map at init genesis
@@ -53,14 +53,14 @@ type Keeper struct {
 // cdc - the app-wide binary codec
 // homePath - root directory of the application's config
 // vs - the interface implemented by baseapp which allows setting baseapp's protocol version field
-func NewKeeper(skipUpgradeHeights map[int64]bool, storeService corestore.KVStoreService, cdc codec.BinaryCodec, homePath string, vs xp.ProtocolVersionSetter, authority string) *Keeper {
+func NewKeeper(skipUpgradeHeights map[int64]bool, storeService corestore.KVStoreService, cdc codec.BinaryCodec, homePath string, vs xp.AppVersionModifier, authority string) *Keeper {
 	k := &Keeper{
 		homePath:           homePath,
 		skipUpgradeHeights: skipUpgradeHeights,
 		storeService:       storeService,
 		cdc:                cdc,
 		upgradeHandlers:    map[string]types.UpgradeHandler{},
-		versionSetter:      vs,
+		versionModifier:    vs,
 		authority:          authority,
 	}
 
@@ -71,16 +71,6 @@ func NewKeeper(skipUpgradeHeights map[int64]bool, storeService corestore.KVStore
 	return k
 }
 
-// SetVersionSetter sets the interface implemented by baseapp which allows setting baseapp's protocol version field
-func (k *Keeper) SetVersionSetter(vs xp.ProtocolVersionSetter) {
-	k.versionSetter = vs
-}
-
-// GetVersionSetter gets the protocol version field of baseapp
-func (k *Keeper) GetVersionSetter() xp.ProtocolVersionSetter {
-	return k.versionSetter
-}
-
 // SetInitVersionMap sets the initial version map.
 // This is only used in app wiring and should not be used in any other context.
 func (k *Keeper) SetInitVersionMap(vm module.VersionMap) {
@@ -100,35 +90,6 @@ func (k Keeper) SetUpgradeHandler(name string, upgradeHandler types.UpgradeHandl
 	k.upgradeHandlers[name] = upgradeHandler
 }
 
-// setProtocolVersion sets the protocol version to state
-func (k Keeper) setProtocolVersion(ctx context.Context, v uint64) error {
-	store := k.storeService.OpenKVStore(ctx)
-	versionBytes := make([]byte, 8)
-	binary.BigEndian.PutUint64(versionBytes, v)
-	return store.Set([]byte{types.ProtocolVersionByte}, versionBytes)
-}
-
-// getProtocolVersion gets the protocol version from state
-func (k Keeper) getProtocolVersion(ctx context.Context) (uint64, error) {
-	store := k.storeService.OpenKVStore(ctx)
-	ok, err := store.Has([]byte{types.ProtocolVersionByte})
-	if err != nil {
-		return 0, err
-	}
-
-	if ok {
-		pvBytes, err := store.Get([]byte{types.ProtocolVersionByte})
-		if err != nil {
-			return 0, err
-		}
-
-		protocolVersion := binary.BigEndian.Uint64(pvBytes)
-		return protocolVersion, nil
-	}
-	// default value
-	return 0, nil
-}
-
 // SetModuleVersionMap saves a given version map to state
 func (k Keeper) SetModuleVersionMap(ctx context.Context, vm module.VersionMap) error {
 	if len(vm) > 0 {
@@ -445,6 +406,7 @@ func (k Keeper) GetUpgradePlan(ctx context.Context) (plan types.Plan, err error)
 func (k Keeper) setDone(ctx context.Context, name string) error {
 	store := k.storeService.OpenKVStore(ctx)
 	sdkCtx := sdk.UnwrapSDKContext(ctx)
+	fmt.Println("setting done", "height", sdkCtx.HeaderInfo().Height, "name", name)
 	return store.Set(encodeDoneKey(name, sdkCtx.HeaderInfo().Height), []byte{1})
 }
 
@@ -455,6 +417,7 @@ func (k Keeper) HasHandler(name string) bool {
 }
 
 // ApplyUpgrade will execute the handler associated with the Plan and mark the plan as done.
+// If successful, it will increment the app version and clear the IBC state
 func (k Keeper) ApplyUpgrade(ctx context.Context, plan types.Plan) error {
 	handler := k.upgradeHandlers[plan.Name]
 	if handler == nil {
@@ -476,20 +439,16 @@ func (k Keeper) ApplyUpgrade(ctx context.Context, plan types.Plan) error {
 		return err
 	}
 
-	// incremement the protocol version and set it in state and baseapp
-	nextProtocolVersion, err := k.getProtocolVersion(ctx)
-	if err != nil {
-		return err
-	}
-	nextProtocolVersion++
-	err = k.setProtocolVersion(ctx, nextProtocolVersion)
-	if err != nil {
-		return err
-	}
+	// incremement the app version and set it in state and baseapp
+	if k.versionModifier != nil {
+		currentAppVersion, err := k.versionModifier.AppVersion(ctx)
+		if err != nil {
+			return err
+		}
 
-	if k.versionSetter != nil {
-		// set protocol version on BaseApp
-		k.versionSetter.SetProtocolVersion(nextProtocolVersion)
+		if err := k.versionModifier.SetAppVersion(ctx, currentAppVersion+1); err != nil {
+			return err
+		}
 	}
 
 	// Must clear IBC state after upgrade is applied as it is stored separately from the upgrade plan.