fix!: split out upgrade into a separate signal module (#3290)

Closes: #3284 We've been experienceing an app hash mismatch when running `main` on mainnet. I believe this is caused because in v1 we had an upgrade module (albeit unreachable) which was still part of state (kept around for compatibility with IBC). In `main` we coped the upgrade modules state and extended it with the signalling mechanism however this module only applied from v2 to v2 meaning there wasn't the same state in v1. This attempts to fix this. It separates out the signal protocol into it's own module (which correctly should only be v2) and reimport the upgrade keeper from the sdk (as was in v1.x) --------- Co-authored-by: Rootul P <[email protected]>
celestiaorg · Apr 11, 2024 · 1d73e33 · 1d73e33
1 parent c9a7e8f
commit 1d73e33
Show file tree

Hide file tree

Showing 26 changed files with 240 additions and 376 deletions.
diff --git a/app/app.go b/app/app.go
@@ -9,8 +9,6 @@ import (
 	"github.com/celestiaorg/celestia-app/v2/x/mint"
 	mintkeeper "github.com/celestiaorg/celestia-app/v2/x/mint/keeper"
 	minttypes "github.com/celestiaorg/celestia-app/v2/x/mint/types"
-	"github.com/celestiaorg/celestia-app/v2/x/upgrade"
-	upgradetypes "github.com/celestiaorg/celestia-app/v2/x/upgrade/types"
 	"github.com/cosmos/cosmos-sdk/baseapp"
 	"github.com/cosmos/cosmos-sdk/client"
 	nodeservice "github.com/cosmos/cosmos-sdk/client/grpc/node"
@@ -68,6 +66,8 @@ import (
 	"github.com/cosmos/cosmos-sdk/x/staking"
 	stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
+	upgradekeeper "github.com/cosmos/cosmos-sdk/x/upgrade/keeper"
+	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
 	"github.com/cosmos/ibc-go/v6/modules/apps/transfer"
 	ibctransferkeeper "github.com/cosmos/ibc-go/v6/modules/apps/transfer/keeper"
 	ibctransfertypes "github.com/cosmos/ibc-go/v6/modules/apps/transfer/types"
@@ -97,6 +97,8 @@ import (
 	"github.com/celestiaorg/celestia-app/v2/x/blobstream"
 	blobstreamkeeper "github.com/celestiaorg/celestia-app/v2/x/blobstream/keeper"
 	blobstreamtypes "github.com/celestiaorg/celestia-app/v2/x/blobstream/types"
+	"github.com/celestiaorg/celestia-app/v2/x/signal"
+	signaltypes "github.com/celestiaorg/celestia-app/v2/x/signal/types"
 	ibctestingtypes "github.com/cosmos/ibc-go/v6/testing/types"
 
 	packetforward "github.com/cosmos/ibc-apps/middleware/packet-forward-middleware/v6/router"
@@ -134,7 +136,7 @@ var (
 		vesting.AppModuleBasic{},
 		blob.AppModuleBasic{},
 		blobstream.AppModuleBasic{},
-		upgrade.AppModuleBasic{},
+		signal.AppModuleBasic{},
 		minfee.AppModuleBasic{},
 		packetforward.AppModuleBasic{},
 		icaModule{},
@@ -194,7 +196,8 @@ type App struct {
 	DistrKeeper      distrkeeper.Keeper
 	GovKeeper        govkeeper.Keeper
 	CrisisKeeper     crisiskeeper.Keeper
-	UpgradeKeeper    upgrade.Keeper
+	UpgradeKeeper    upgradekeeper.Keeper // This is included purely for the IBC Keeper. It is not used for upgrading
+	SignalKeeper     signal.Keeper
 	ParamsKeeper     paramskeeper.Keeper
 	IBCKeeper        *ibckeeper.Keeper // IBCKeeper must be a pointer in the app, so we can SetRouter on it correctly
 	EvidenceKeeper   evidencekeeper.Keeper
@@ -254,6 +257,7 @@ func New(
 		ibchost.StoreKey,
 		packetforwardtypes.StoreKey,
 		icahosttypes.StoreKey,
+		signaltypes.StoreKey,
 	)
 	tkeys := sdk.NewTransientStoreKeys(paramstypes.TStoreKey)
 	memKeys := sdk.NewMemoryStoreKeys(capabilitytypes.MemStoreKey)
@@ -315,7 +319,10 @@ func New(
 	)
 
 	app.FeeGrantKeeper = feegrantkeeper.NewKeeper(appCodec, keys[feegrant.StoreKey], app.AccountKeeper)
-	app.UpgradeKeeper = upgrade.NewKeeper(keys[upgradetypes.StoreKey], stakingKeeper)
+	// The ugrade keeper is intialised solely for the ibc keeper which depends on it to know what the next validator hash is for after the
+	// upgrade. This keeper is not used for the actual upgrades but merely for compatibility reasons. Ideally IBC has their own upgrade module
+	// for performing IBC based upgrades. Note, as we use rolling upgrades, IBC technically never needs this functionality.
+	app.UpgradeKeeper = upgradekeeper.NewKeeper(nil, keys[upgradetypes.StoreKey], appCodec, "", app.BaseApp, authtypes.NewModuleAddress(govtypes.ModuleName).String())
 	app.upgradeHeight = upgradeHeight
 
 	app.BlobstreamKeeper = *blobstreamkeeper.NewKeeper(
@@ -334,7 +341,7 @@ func New(
 		),
 	)
 
-	// ... other modules keepers
+	app.SignalKeeper = signal.NewKeeper(keys[signaltypes.StoreKey], app.StakingKeeper)
 
 	app.IBCKeeper = ibckeeper.NewKeeper(
 		appCodec,
@@ -510,7 +517,7 @@ func New(
 			FromVersion: v1, ToVersion: v2,
 		},
 		{
-			Module:      upgrade.NewAppModule(app.UpgradeKeeper),
+			Module:      signal.NewAppModule(app.SignalKeeper),
 			FromVersion: v2, ToVersion: v2,
 		},
 		{
@@ -554,7 +561,7 @@ func New(
 		paramstypes.ModuleName,
 		authz.ModuleName,
 		vestingtypes.ModuleName,
-		upgradetypes.ModuleName,
+		signaltypes.ModuleName,
 		minfee.ModuleName,
 		icatypes.ModuleName,
 		packetforwardtypes.ModuleName,
@@ -580,7 +587,7 @@ func New(
 		paramstypes.ModuleName,
 		authz.ModuleName,
 		vestingtypes.ModuleName,
-		upgradetypes.ModuleName,
+		signaltypes.ModuleName,
 		minfee.ModuleName,
 		packetforwardtypes.ModuleName,
 		icatypes.ModuleName,
@@ -614,7 +621,7 @@ func New(
 		feegrant.ModuleName,
 		paramstypes.ModuleName,
 		authz.ModuleName,
-		upgradetypes.ModuleName,
+		signaltypes.ModuleName,
 		packetforwardtypes.ModuleName,
 		icatypes.ModuleName,
 	)
@@ -684,7 +691,7 @@ func (app *App) EndBlocker(ctx sdk.Context, req abci.RequestEndBlock) abci.Respo
 			}
 		}
 		// from v2 to v3 and onwards we use a signalling mechanism
-	} else if shouldUpgrade, newVersion := app.UpgradeKeeper.ShouldUpgrade(); shouldUpgrade {
+	} else if shouldUpgrade, newVersion := app.SignalKeeper.ShouldUpgrade(); shouldUpgrade {
 		// Version changes must be increasing. Downgrades are not permitted
 		if newVersion > currentVersion {
 			if err := app.Upgrade(ctx, currentVersion, newVersion); err != nil {

diff --git a/app/module/migrations_test.go b/app/module/migrations_test.go
@@ -6,8 +6,8 @@ import (
 	"github.com/celestiaorg/celestia-app/v2/app"
 	"github.com/celestiaorg/celestia-app/v2/app/encoding"
 	"github.com/celestiaorg/celestia-app/v2/app/module"
-	"github.com/celestiaorg/celestia-app/v2/x/upgrade"
-	upgradetypes "github.com/celestiaorg/celestia-app/v2/x/upgrade/types"
+	"github.com/celestiaorg/celestia-app/v2/x/signal"
+	signaltypes "github.com/celestiaorg/celestia-app/v2/x/signal/types"
 	"github.com/cosmos/cosmos-sdk/store"
 	storetypes "github.com/cosmos/cosmos-sdk/store/types"
 	"github.com/cosmos/cosmos-sdk/tests/mocks"
@@ -76,15 +76,15 @@ func TestConfiguratorRegistersAllMessageTypes(t *testing.T) {
 	cdc := encoding.MakeConfig(app.ModuleEncodingRegisters...)
 	configurator := module.NewConfigurator(cdc.Codec, mockServer, mockServer)
 
-	storeKey := sdk.NewKVStoreKey(upgradetypes.StoreKey)
+	storeKey := sdk.NewKVStoreKey(signaltypes.StoreKey)
 
 	db := dbm.NewMemDB()
 	stateStore := store.NewCommitMultiStore(db)
 	stateStore.MountStoreWithDB(storeKey, storetypes.StoreTypeIAVL, db)
 	require.NoError(t, stateStore.LoadLatestVersion())
 
-	keeper := upgrade.NewKeeper(storeKey, nil)
-	upgradeModule := upgrade.NewAppModule(keeper)
+	keeper := signal.NewKeeper(storeKey, nil)
+	upgradeModule := signal.NewAppModule(keeper)
 	mm, err := module.NewManager([]module.VersionedModule{
 		{Module: upgradeModule, FromVersion: 2, ToVersion: 2},
 	})
@@ -94,7 +94,7 @@ func TestConfiguratorRegistersAllMessageTypes(t *testing.T) {
 	mm.RegisterServices(configurator)
 	acceptedMessages := configurator.GetAcceptedMessages()
 	require.Equal(t, map[uint64]map[string]struct{}{
-		2: {"/celestia.upgrade.v1.MsgSignalVersion": {}, "/celestia.upgrade.v1.MsgTryUpgrade": {}},
+		2: {"/celestia.signal.v1.MsgSignalVersion": {}, "/celestia.signal.v1.MsgTryUpgrade": {}},
 	}, acceptedMessages)
 
 	require.NotNil(t, keeper)

diff --git a/app/test/std_sdk_test.go b/app/test/std_sdk_test.go
@@ -11,7 +11,7 @@ import (
 	"github.com/celestiaorg/celestia-app/v2/test/util/blobfactory"
 	"github.com/celestiaorg/celestia-app/v2/test/util/testfactory"
 	"github.com/celestiaorg/celestia-app/v2/test/util/testnode"
-	upgrade "github.com/celestiaorg/celestia-app/v2/x/upgrade/types"
+	signal "github.com/celestiaorg/celestia-app/v2/x/signal/types"
 	"github.com/cosmos/cosmos-sdk/crypto/hd"
 	"github.com/cosmos/cosmos-sdk/crypto/keyring"
 	"github.com/cosmos/cosmos-sdk/testutil/mock"
@@ -292,7 +292,7 @@ func (s *StandardSDKIntegrationTestSuite) TestStandardSDK() {
 			msgFunc: func() (msgs []sdk.Msg, signer string) {
 				account := s.unusedAccount()
 				addr := testfactory.GetAddress(s.cctx.Keyring, account)
-				msg := upgrade.NewMsgTryUpgrade(addr)
+				msg := signal.NewMsgTryUpgrade(addr)
 				return []sdk.Msg{msg}, account
 			},
 			expectedCode: abci.CodeTypeOK,
@@ -301,7 +301,7 @@ func (s *StandardSDKIntegrationTestSuite) TestStandardSDK() {
 			name: "signal a version change",
 			msgFunc: func() (msgs []sdk.Msg, signer string) {
 				valAccount := s.getValidatorAccount()
-				msg := upgrade.NewMsgSignalVersion(valAccount, 2)
+				msg := signal.NewMsgSignalVersion(valAccount, 2)
 				return []sdk.Msg{msg}, s.getValidatorName()
 			},
 			expectedCode: abci.CodeTypeOK,

diff --git a/app/test/upgrade_test.go b/app/test/upgrade_test.go
@@ -0,0 +1,32 @@
+package app_test
+
+import (
+	"testing"
+
+	v1 "github.com/celestiaorg/celestia-app/v2/pkg/appconsts/v1"
+	v2 "github.com/celestiaorg/celestia-app/v2/pkg/appconsts/v2"
+	"github.com/stretchr/testify/require"
+	abci "github.com/tendermint/tendermint/abci/types"
+	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
+	tmversion "github.com/tendermint/tendermint/proto/tendermint/version"
+)
+
+func TestUpgradeAppVersion(t *testing.T) {
+	testApp, _ := setupTestApp(t, 3)
+
+	supportedVersions := []uint64{v1.Version, v2.Version}
+
+	require.Equal(t, supportedVersions, testApp.SupportedVersions())
+
+	testApp.BeginBlock(abci.RequestBeginBlock{Header: tmproto.Header{
+		Height:  2,
+		Version: tmversion.Consensus{App: 1},
+	}})
+	// app version should not have changed yet
+	require.EqualValues(t, 1, testApp.AppVersion())
+	respEndBlock := testApp.EndBlock(abci.RequestEndBlock{Height: 2})
+	// now the app version changes
+	require.NotNil(t, respEndBlock.ConsensusParamUpdates.Version)
+	require.EqualValues(t, 2, respEndBlock.ConsensusParamUpdates.Version.AppVersion)
+	require.EqualValues(t, 2, testApp.AppVersion())
+}
diff --git a/proto/celestia/upgrade/v1/query.proto → proto/celestia/signal/v1/query.proto b/proto/celestia/upgrade/v1/query.proto → proto/celestia/signal/v1/query.proto
@@ -1,9 +1,9 @@
 syntax = "proto3";
-package celestia.upgrade.v1;
+package celestia.signal.v1;
 
 import "google/api/annotations.proto";
 
-option go_package = "github.com/celestiaorg/celestia-app/x/upgrade/types";
+option go_package = "github.com/celestiaorg/celestia-app/x/signal/types";
 
 // Query defines the upgrade Query service.
 service Query {

diff --git a/proto/celestia/upgrade/v1/tx.proto → proto/celestia/signal/v1/tx.proto b/proto/celestia/upgrade/v1/tx.proto → proto/celestia/signal/v1/tx.proto
@@ -1,9 +1,9 @@
 syntax = "proto3";
-package celestia.upgrade.v1;
+package celestia.signal.v1;
 
 import "google/api/annotations.proto";
 
-option go_package = "github.com/celestiaorg/celestia-app/x/upgrade/types";
+option go_package = "github.com/celestiaorg/celestia-app/x/signal/types";
 
 // Msg defines the upgrade Msg service.
 service Msg {

diff --git a/x/upgrade/README.md → x/signal/README.md b/x/upgrade/README.md → x/signal/README.md
@@ -1,6 +1,6 @@
-# `x/upgrade`
+# `x/signal`
 
-This upgrade module is a fork of the cosmos-sdk's [x/upgrade](https://github.com/cosmos/cosmos-sdk/tree/main/x/upgrade) module. The primary purpose of this module is to allow for rolling network upgrades as proposed in [ADR-018](../../docs/architecture/adr-018-network-upgrades.md) and [CIP-10](https://github.com/celestiaorg/CIPs/blob/main/cips/cip-10.md).
+The signal module acts as a coordinating mechanism for the application on when it should transition from one app version to another. The application itself handles multiple versioned modules as well as migrations, however it requires some protocol for knowing which height to perform this upgrade as it is critical that all nodes upgrade at the same point.
 
 Note: this module won't be used for upgrading from app version v1 to v2 but will be used for upgrading from v2 to v3 and onwards.
 
@@ -26,19 +26,19 @@ See [types/msgs.go](./types/msgs.go) for the message types.
 ### CLI
 
 ```shell
-celestia-appd query upgrade tally
-celestia-appd tx upgrade signal
-celestia-appd tx upgrade try-upgrade
+celestia-appd query signal tally
+celestia-appd tx signal signal
+celestia-appd tx signal try-upgrade
 ```
 
 ### gRPC
 
 ```api
-celestia.upgrade.v1.Query/VersionTally
+celestia.signal.v1.Query/VersionTally
 ```
 
 ```shell
-grpcurl -plaintext localhost:9090 celestia.upgrade.v1.Query/VersionTally
+grpcurl -plaintext localhost:9090 celestia.signal.v1.Query/VersionTally
 ```
 
 ## Appendix

diff --git a/x/upgrade/cli/cli_test.go → x/signal/cli/cli_test.go b/x/upgrade/cli/cli_test.go → x/signal/cli/cli_test.go
@@ -4,7 +4,7 @@ import (
 	"testing"
 
 	"github.com/celestiaorg/celestia-app/v2/test/util/testnode"
-	"github.com/celestiaorg/celestia-app/v2/x/upgrade/cli"
+	"github.com/celestiaorg/celestia-app/v2/x/signal/cli"
 	testutil "github.com/cosmos/cosmos-sdk/testutil/cli"
 	"github.com/stretchr/testify/suite"
 )

diff --git a/x/upgrade/cli/query.go → x/signal/cli/query.go b/x/upgrade/cli/query.go → x/signal/cli/query.go
@@ -4,7 +4,7 @@ import (
 	"fmt"
 	"strconv"
 
-	"github.com/celestiaorg/celestia-app/v2/x/upgrade/types"
+	"github.com/celestiaorg/celestia-app/v2/x/signal/types"
 	"github.com/cosmos/cosmos-sdk/client"
 	"github.com/cosmos/cosmos-sdk/client/flags"
 	"github.com/spf13/cobra"

diff --git a/x/upgrade/cli/tx.go → x/signal/cli/tx.go b/x/upgrade/cli/tx.go → x/signal/cli/tx.go
@@ -4,7 +4,7 @@ import (
 	"fmt"
 	"strconv"
 
-	"github.com/celestiaorg/celestia-app/v2/x/upgrade/types"
+	"github.com/celestiaorg/celestia-app/v2/x/signal/types"
 	"github.com/cosmos/cosmos-sdk/client"
 	"github.com/cosmos/cosmos-sdk/client/flags"
 	"github.com/cosmos/cosmos-sdk/client/tx"

diff --git a/x/upgrade/integration_test.go → x/signal/integration_test.go b/x/upgrade/integration_test.go → x/signal/integration_test.go
@@ -1,11 +1,11 @@
-package upgrade_test
+package signal_test
 
 import (
 	"testing"
 
 	"github.com/celestiaorg/celestia-app/v2/app"
 	testutil "github.com/celestiaorg/celestia-app/v2/test/util"
-	"github.com/celestiaorg/celestia-app/v2/x/upgrade/types"
+	"github.com/celestiaorg/celestia-app/v2/x/signal/types"
 	"github.com/stretchr/testify/require"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -27,7 +27,7 @@ func TestUpgradeIntegration(t *testing.T) {
 	goCtx := sdk.WrapSDKContext(ctx)
 	ctx = sdk.UnwrapSDKContext(goCtx)
 
-	res, err := app.UpgradeKeeper.VersionTally(goCtx, &types.QueryVersionTallyRequest{
+	res, err := app.SignalKeeper.VersionTally(goCtx, &types.QueryVersionTallyRequest{
 		Version: 2,
 	})
 	require.NoError(t, err)
@@ -37,24 +37,24 @@ func TestUpgradeIntegration(t *testing.T) {
 	valAddr, err := sdk.ValAddressFromBech32(validators[0].OperatorAddress)
 	require.NoError(t, err)
 
-	_, err = app.UpgradeKeeper.SignalVersion(ctx, &types.MsgSignalVersion{
+	_, err = app.SignalKeeper.SignalVersion(ctx, &types.MsgSignalVersion{
 		ValidatorAddress: valAddr.String(),
 		Version:          2,
 	})
 	require.NoError(t, err)
 
-	res, err = app.UpgradeKeeper.VersionTally(goCtx, &types.QueryVersionTallyRequest{
+	res, err = app.SignalKeeper.VersionTally(goCtx, &types.QueryVersionTallyRequest{
 		Version: 2,
 	})
 	require.NoError(t, err)
 	require.EqualValues(t, 1, res.VotingPower)
 	require.EqualValues(t, 1, res.ThresholdPower)
 	require.EqualValues(t, 1, res.TotalVotingPower)
 
-	_, err = app.UpgradeKeeper.TryUpgrade(ctx, nil)
+	_, err = app.SignalKeeper.TryUpgrade(ctx, nil)
 	require.NoError(t, err)
 
-	shouldUpgrade, version := app.UpgradeKeeper.ShouldUpgrade()
+	shouldUpgrade, version := app.SignalKeeper.ShouldUpgrade()
 	require.True(t, shouldUpgrade)
 	require.EqualValues(t, 2, version)
 }
diff --git a/x/upgrade/interfaces.go → x/signal/interfaces.go b/x/upgrade/interfaces.go → x/signal/interfaces.go
@@ -1,4 +1,4 @@
-package upgrade
+package signal
 
 import (
 	"cosmossdk.io/math"

diff --git a/x/upgrade/keeper.go → x/signal/keeper.go b/x/upgrade/keeper.go → x/signal/keeper.go
@@ -1,11 +1,11 @@
-package upgrade
+package signal
 
 import (
 	"context"
 	"encoding/binary"
 
 	sdkmath "cosmossdk.io/math"
-	"github.com/celestiaorg/celestia-app/v2/x/upgrade/types"
+	"github.com/celestiaorg/celestia-app/v2/x/signal/types"
 	storetypes "github.com/cosmos/cosmos-sdk/store/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
@@ -20,12 +20,12 @@ var (
 	defaultSignalThreshold = sdk.NewDec(5).Quo(sdk.NewDec(6))
 )
 
-// SignalThreshold is the fraction of voting power that is required
+// Threshold is the fraction of voting power that is required
 // to signal for a version change. It is set to 5/6 as the middle point
 // between 2/3 and 3/3 providing 1/6 fault tolerance to halting the
 // network during an upgrade period. It can be modified through a
 // hard fork change that modified the app version
-func SignalThreshold(_ uint64) sdk.Dec {
+func Threshold(_ uint64) sdk.Dec {
 	return defaultSignalThreshold
 }
 
@@ -169,7 +169,7 @@ func (k Keeper) TallyVotingPower(ctx sdk.Context, threshold int64) (bool, uint64
 // upgrade to a new version.
 func (k Keeper) GetVotingPowerThreshold(ctx sdk.Context) sdkmath.Int {
 	totalVotingPower := k.stakingKeeper.GetLastTotalPower(ctx)
-	thresholdFraction := SignalThreshold(ctx.BlockHeader().Version.App)
+	thresholdFraction := Threshold(ctx.BlockHeader().Version.App)
 	return thresholdFraction.MulInt(totalVotingPower).Ceil().TruncateInt()
 }