feat!: support coordinated v2 upgrade with flag (#2803)

As discussed, given no pre-existing coordinating mechanism (i.e. signalling), v2 will have to work differently to future upgrades. Namely, nodes will run `start` with an `upgrade-height` flag. At the height before that in `EndBlock`, the protocol version will be updated and cometbft will receive the new v2 app version. There will be no state migrations. The block at the following height will be v2 and validators will begin validating and executing accordingly. Co-authored-by: Evan Forbes <[email protected]> Co-authored-by: Rootul P <[email protected]>
celestiaorg · Nov 9, 2023 · 9270f4b · 9270f4b
1 parent 75f9393
commit 9270f4b
Show file tree

Hide file tree

Showing 16 changed files with 47 additions and 459 deletions.
diff --git a/app/app.go b/app/app.go
@@ -85,6 +85,7 @@ import (
 	"github.com/celestiaorg/celestia-app/app/ante"
 	"github.com/celestiaorg/celestia-app/app/encoding"
 	"github.com/celestiaorg/celestia-app/pkg/appconsts"
+	v2 "github.com/celestiaorg/celestia-app/pkg/appconsts/v2"
 	"github.com/celestiaorg/celestia-app/pkg/proof"
 	blobmodule "github.com/celestiaorg/celestia-app/x/blob"
 	blobmodulekeeper "github.com/celestiaorg/celestia-app/x/blob/keeper"
@@ -239,23 +240,21 @@ type App struct {
 }
 
 // New returns a reference to an initialized celestia app.
+//
+// NOTE: upgradeHeight refers specifically to the height that
+// a node will upgrade from v1 to v2. It will be deprecated in v3
+// in place for a dynamically signalling scheme
 func New(
 	logger log.Logger,
 	db dbm.DB,
 	traceStore io.Writer,
 	loadLatest bool,
 	invCheckPeriod uint,
 	encodingConfig encoding.Config,
-	upgradeSchedule map[string]upgrade.Schedule,
+	upgradeHeight int64,
 	appOpts servertypes.AppOptions,
 	baseAppOptions ...func(*baseapp.BaseApp),
 ) *App {
-	for _, schedule := range upgradeSchedule {
-		if err := schedule.ValidateVersions(supportedVersions); err != nil {
-			panic(err)
-		}
-	}
-
 	appCodec := encodingConfig.Codec
 	cdc := encodingConfig.Amino
 	interfaceRegistry := encodingConfig.InterfaceRegistry
@@ -334,7 +333,7 @@ func New(
 	)
 
 	app.FeeGrantKeeper = feegrantkeeper.NewKeeper(appCodec, keys[feegrant.StoreKey], app.AccountKeeper)
-	app.UpgradeKeeper = upgrade.NewKeeper(keys[upgrade.StoreKey], upgradeSchedule)
+	app.UpgradeKeeper = upgrade.NewKeeper(keys[upgrade.StoreKey], upgradeHeight)
 
 	app.BlobstreamKeeper = *bsmodulekeeper.NewKeeper(
 		appCodec,
@@ -574,14 +573,10 @@ func (app *App) BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock) abci.R
 // EndBlocker application updates every end block
 func (app *App) EndBlocker(ctx sdk.Context, req abci.RequestEndBlock) abci.ResponseEndBlock {
 	res := app.mm.EndBlock(ctx, req)
-	if app.UpgradeKeeper.ShouldUpgrade() {
-		newAppVersion := app.UpgradeKeeper.GetNextAppVersion()
-		app.SetProtocolVersion(newAppVersion)
-		_, err := app.mm.RunMigrations(ctx, app.configurator, GetModuleVersion(newAppVersion))
-		if err != nil {
-			panic(err)
-		}
-		app.UpgradeKeeper.MarkUpgradeComplete()
+	// NOTE: this is a specific feature for upgrading to v2 as v3 and onward is expected
+	// to be coordinated through the signalling protocol
+	if app.UpgradeKeeper.ShouldUpgrade(req.Height) {
+		app.SetProtocolVersion(v2.Version)
 	}
 	return res
 }

diff --git a/app/deliver_tx.go b/app/deliver_tx.go
diff --git a/app/prepare_proposal.go b/app/prepare_proposal.go
@@ -7,7 +7,6 @@ import (
 	"github.com/celestiaorg/celestia-app/pkg/da"
 	"github.com/celestiaorg/celestia-app/pkg/shares"
 	"github.com/celestiaorg/celestia-app/pkg/square"
-	"github.com/celestiaorg/celestia-app/x/upgrade"
 	"github.com/cosmos/cosmos-sdk/telemetry"
 	abci "github.com/tendermint/tendermint/abci/types"
 	core "github.com/tendermint/tendermint/proto/tendermint/types"
@@ -59,27 +58,6 @@ func (app *App) PrepareProposal(req abci.RequestPrepareProposal) abci.ResponsePr
 		txs = make([][]byte, 0)
 	} else {
 		txs = FilterTxs(app.Logger(), sdkCtx, handler, app.txConfig, req.BlockData.Txs)
-
-		// TODO: this would be improved if we only attempted the upgrade in the first round of the
-		// height to still allow transactions to pass through without being delayed from trying
-		// to coordinate the upgrade height
-		if newVersion, ok := app.UpgradeKeeper.ShouldProposeUpgrade(req.ChainId, req.Height); ok && newVersion > app.GetBaseApp().AppVersion() {
-			upgradeTx, err := upgrade.NewMsgVersionChange(app.txConfig, newVersion)
-			if err != nil {
-				panic(err)
-			}
-			// the upgrade transaction must be the first transaction in the block
-			txs = append([][]byte{upgradeTx}, txs...)
-
-			// because we are adding bytes, we need to check that we are not going over the limit
-			// if we are, we continually prune the last tx (the lowest paying blobTx).
-			size := sizeOf(txs)
-			for size > int(req.BlockDataSize) {
-				lastTx := txs[len(txs)-1]
-				txs = txs[:len(txs)-1]
-				size -= len(lastTx)
-			}
-		}
 	}
 
 	// build the square from the set of valid and prioritised transactions.
@@ -124,11 +102,3 @@ func (app *App) PrepareProposal(req abci.RequestPrepareProposal) abci.ResponsePr
 		},
 	}
 }
-
-func sizeOf(txs [][]byte) int {
-	size := 0
-	for _, tx := range txs {
-		size += len(tx)
-	}
-	return size
-}
diff --git a/app/process_proposal.go b/app/process_proposal.go
@@ -11,7 +11,6 @@ import (
 	"github.com/celestiaorg/celestia-app/pkg/shares"
 	"github.com/celestiaorg/celestia-app/pkg/square"
 	blobtypes "github.com/celestiaorg/celestia-app/x/blob/types"
-	"github.com/celestiaorg/celestia-app/x/upgrade"
 	"github.com/cosmos/cosmos-sdk/telemetry"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	abci "github.com/tendermint/tendermint/abci/types"
@@ -76,27 +75,6 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) (resp abci.Resp
 				return reject()
 			}
 
-			if appVersion, ok := upgrade.IsUpgradeMsg(msgs); ok {
-				if idx != 0 {
-					logInvalidPropBlock(app.Logger(), req.Header, fmt.Sprintf("upgrade message %d is not the first transaction", idx))
-					return reject()
-				}
-
-				if !IsSupported(appVersion) {
-					logInvalidPropBlock(app.Logger(), req.Header, fmt.Sprintf("block proposes an unsupported app version %d", appVersion))
-					return reject()
-				}
-
-				// app version must always increase
-				if appVersion <= app.GetBaseApp().AppVersion() {
-					logInvalidPropBlock(app.Logger(), req.Header, fmt.Sprintf("block proposes an app version %d that is not greater than the current app version %d", appVersion, app.GetBaseApp().AppVersion()))
-					return reject()
-				}
-
-				// we don't need to pass this message through the ante handler
-				continue
-			}
-
 			// we need to increment the sequence for every transaction so that
 			// the signature check below is accurate. this error only gets hit
 			// if the account in question doens't exist.

diff --git a/cmd/celestia-appd/cmd/root.go b/cmd/celestia-appd/cmd/root.go
@@ -4,6 +4,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"strconv"
 
 	bscmd "github.com/celestiaorg/celestia-app/x/blobstream/client"
 
@@ -45,6 +46,8 @@ const (
 
 	// FlagLogToFile specifies whether to log to file or not.
 	FlagLogToFile = "log-to-file"
+
+	UpgradeHeightFlag = "v2-upgrade-height"
 )
 
 // NewRootCmd creates a new root command for celestia-appd. It is called once in the
@@ -157,6 +160,7 @@ func initRootCmd(rootCmd *cobra.Command, encodingConfig encoding.Config) {
 
 func addModuleInitFlags(startCmd *cobra.Command) {
 	crisis.AddModuleInitFlags(startCmd)
+	startCmd.Flags().Int64(UpgradeHeightFlag, 0, "Upgrade height to switch from v1 to v2. Must be coordinated amongst all validators")
 }
 
 func queryCommand() *cobra.Command {
@@ -233,11 +237,20 @@ func NewAppServer(logger log.Logger, db dbm.DB, traceStore io.Writer, appOpts se
 		panic(err)
 	}
 
+	var upgradeHeight int64
+	upgradeHeightStr, ok := appOpts.Get(UpgradeHeightFlag).(string)
+	if ok {
+		upgradeHeight, err = strconv.ParseInt(upgradeHeightStr, 10, 64)
+		if err != nil {
+			panic(err)
+		}
+	}
+
 	return app.New(
 		logger, db, traceStore, true,
 		cast.ToUint(appOpts.Get(server.FlagInvCheckPeriod)),
 		encoding.MakeConfig(app.ModuleEncodingRegisters...), // Ideally, we would reuse the one created by NewRootCmd.
-		nil,
+		upgradeHeight,
 		appOpts,
 		baseapp.SetPruning(pruningOpts),
 		baseapp.SetMinGasPrices(cast.ToString(appOpts.Get(server.FlagMinGasPrices))),
@@ -260,13 +273,13 @@ func createAppAndExport(
 	encCfg.Codec = codec.NewProtoCodec(encCfg.InterfaceRegistry)
 	var capp *app.App
 	if height != -1 {
-		capp = app.New(logger, db, traceStore, false, uint(1), encCfg, nil, appOpts)
+		capp = app.New(logger, db, traceStore, false, uint(1), encCfg, 0, appOpts)
 
 		if err := capp.LoadHeight(height); err != nil {
 			return servertypes.ExportedApp{}, err
 		}
 	} else {
-		capp = app.New(logger, db, traceStore, true, uint(1), encCfg, nil, appOpts)
+		capp = app.New(logger, db, traceStore, true, uint(1), encCfg, 0, appOpts)
 	}
 
 	return capp.ExportAppStateAndValidators(forZeroHeight, jailWhiteList)

diff --git a/test/e2e/simple_test.go b/test/e2e/simple_test.go
@@ -52,7 +52,7 @@ func TestE2ESimple(t *testing.T) {
 	encCfg := encoding.MakeConfig(app.ModuleEncodingRegisters...)
 	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
 	defer cancel()
-	opts := txsim.DefaultOptions().WithSeed(seed)
+	opts := txsim.DefaultOptions().WithSeed(seed).SuppressLogs()
 	err = txsim.Run(ctx, testnet.GRPCEndpoints()[0], kr, encCfg, opts, sequences...)
 	require.True(t, errors.Is(err, context.DeadlineExceeded), err.Error())
 

diff --git a/test/e2e/upgrade_test.go b/test/e2e/upgrade_test.go
@@ -69,7 +69,7 @@ func TestMinorVersionCompatibility(t *testing.T) {
 
 	errCh := make(chan error)
 	encCfg := encoding.MakeConfig(app.ModuleEncodingRegisters...)
-	opts := txsim.DefaultOptions().WithSeed(seed)
+	opts := txsim.DefaultOptions().WithSeed(seed).SuppressLogs()
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	go func() {

diff --git a/test/tokenfilter/setup.go b/test/tokenfilter/setup.go
@@ -140,7 +140,7 @@ func SetupWithGenesisValSet(t testing.TB, valSet *tmtypes.ValidatorSet, genAccs
 	encCdc := encoding.MakeConfig(app.ModuleEncodingRegisters...)
 	genesisState := app.NewDefaultGenesisState(encCdc.Codec)
 	app := app.New(
-		log.NewNopLogger(), db, nil, true, 5, encCdc, nil, simapp.EmptyAppOptions{},
+		log.NewNopLogger(), db, nil, true, 5, encCdc, 0, simapp.EmptyAppOptions{},
 	)
 
 	// set genesis accounts

diff --git a/test/util/malicious/app.go b/test/util/malicious/app.go
@@ -57,7 +57,7 @@ func New(
 	appOpts servertypes.AppOptions,
 	baseAppOptions ...func(*baseapp.BaseApp),
 ) *App {
-	goodApp := app.New(logger, db, traceStore, loadLatest, invCheckPeriod, encodingConfig, nil, appOpts, baseAppOptions...)
+	goodApp := app.New(logger, db, traceStore, loadLatest, invCheckPeriod, encodingConfig, 0, appOpts, baseAppOptions...)
 	badApp := &App{App: goodApp}
 
 	// set the malicious prepare proposal handler if it is set in the app options

diff --git a/test/util/network/network.go b/test/util/network/network.go
@@ -77,7 +77,7 @@ func DefaultConfig() network.Config {
 		AppConstructor: func(val network.Validator) servertypes.Application {
 			return app.New(
 				val.Ctx.Logger, tmdb.NewMemDB(), nil, true, 0,
-				encCfg, nil,
+				encCfg, 0,
 				simapp.EmptyAppOptions{},
 				baseapp.SetPruning(pruningtypes.NewPruningOptionsFromString(val.AppConfig.Pruning)),
 				baseapp.SetMinGasPrices(val.AppConfig.MinGasPrices),

diff --git a/test/util/test_app.go b/test/util/test_app.go
@@ -62,7 +62,7 @@ func SetupTestAppWithGenesisValSet(cparams *tmproto.ConsensusParams, genAccounts
 		log.NewNopLogger(), db, nil, true,
 		cast.ToUint(emptyOpts.Get(server.FlagInvCheckPeriod)),
 		encCfg,
-		nil,
+		0,
 		emptyOpts,
 	)
 

diff --git a/x/upgrade/keeper.go b/x/upgrade/keeper.go
@@ -1,8 +1,6 @@
 package upgrade
 
 import (
-	fmt "fmt"
-
 	storetypes "github.com/cosmos/cosmos-sdk/store/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/upgrade/types"
@@ -16,26 +14,18 @@ type Keeper struct {
 	// safely be ported over without any migration
 	storeKey storetypes.StoreKey
 
-	// in memory copy of the upgrade schedule if any. This is local per node
+	// in memory copy of the upgrade height if any. This is local per node
 	// and configured from the config.
-	upgradeSchedule map[string]Schedule
-
-	// the app version that should be set in end blocker
-	pendingAppVersion uint64
+	upgradeHeight int64
 }
 
 type VersionSetter func(version uint64)
 
 // NewKeeper constructs an upgrade keeper
-func NewKeeper(storeKey storetypes.StoreKey, upgradeSchedule map[string]Schedule) Keeper {
-	for chainID, schedule := range upgradeSchedule {
-		if err := schedule.ValidateBasic(); err != nil {
-			panic(fmt.Sprintf("invalid schedule %s: %v", chainID, err))
-		}
-	}
+func NewKeeper(storeKey storetypes.StoreKey, upgradeHeight int64) Keeper {
 	return Keeper{
-		storeKey:        storeKey,
-		upgradeSchedule: upgradeSchedule,
+		storeKey:      storeKey,
+		upgradeHeight: upgradeHeight,
 	}
 }
 
@@ -99,3 +89,9 @@ func (k Keeper) ClearIBCState(ctx sdk.Context, lastHeight int64) {
 	store.Delete(types.UpgradedClientKey(lastHeight))
 	store.Delete(types.UpgradedConsStateKey(lastHeight))
 }
+
+// ShouldUpgrade returns true if the current height is one before
+// the locally provided upgrade height that is passed as a flag
+func (k Keeper) ShouldUpgrade(height int64) bool {
+	return k.upgradeHeight == height+1
+}