fix!: v3 upgrade delay (#4008)

## Overview Currently, there's a bug in v3 that will use the next version's upgrade delay instead of the current version's. For non-arabica networks, this PR introduces a consensus breaking change that fixes this issue. For arabica networks, they will continue to use the next version's delay. --------- Co-authored-by: Callum Waters <[email protected]>
celestiaorg · Oct 29, 2024 · b6a3ed2 · b6a3ed2
1 parent c410d88
commit b6a3ed2
Show file tree

Hide file tree

Showing 11 changed files with 102 additions and 24 deletions.
diff --git a/app/test/square_size_test.go b/app/test/square_size_test.go
@@ -46,8 +46,10 @@ func (s *SquareSizeIntegrationTest) SetupSuite() {
 	t := s.T()
 	t.Log("setting up square size integration test")
 	s.ecfg = encoding.MakeConfig(app.ModuleEncodingRegisters...)
+
 	cfg := testnode.DefaultConfig().
-		WithModifiers(genesis.ImmediateProposals(s.ecfg.Codec))
+		WithModifiers(genesis.ImmediateProposals(s.ecfg.Codec)).
+		WithTimeoutCommit(time.Second)
 
 	cctx, rpcAddr, grpcAddr := testnode.NewNetwork(t, cfg)
 

diff --git a/app/test/upgrade_test.go b/app/test/upgrade_test.go
@@ -106,7 +106,7 @@ func TestAppUpgradeV3(t *testing.T) {
 
 	// brace yourselfs, this part may take a while
 	initialHeight := int64(4)
-	for height := initialHeight; height < initialHeight+appconsts.UpgradeHeightDelay(v2.Version); height++ {
+	for height := initialHeight; height < initialHeight+appconsts.UpgradeHeightDelay(testApp.GetChainID(), v2.Version); height++ {
 		appVersion := v2.Version
 		_ = testApp.BeginBlock(abci.RequestBeginBlock{
 			Header: tmproto.Header{
@@ -116,7 +116,7 @@ func TestAppUpgradeV3(t *testing.T) {
 		})
 
 		endBlockResp = testApp.EndBlock(abci.RequestEndBlock{
-			Height: 3 + appconsts.UpgradeHeightDelay(v2.Version),
+			Height: 3 + appconsts.UpgradeHeightDelay(testApp.GetChainID(), v2.Version),
 		})
 
 		require.Equal(t, appconsts.GetTimeoutCommit(appVersion), endBlockResp.Timeouts.TimeoutCommit)
@@ -141,7 +141,7 @@ func TestAppUpgradeV3(t *testing.T) {
 	_ = testApp.BeginBlock(abci.RequestBeginBlock{
 		Header: tmproto.Header{
 			ChainID: genesis.ChainID,
-			Height:  initialHeight + appconsts.UpgradeHeightDelay(v3.Version),
+			Height:  initialHeight + appconsts.UpgradeHeightDelay(testApp.GetChainID(), v3.Version),
 			Version: tmversion.Consensus{App: 3},
 		},
 	})
@@ -152,7 +152,7 @@ func TestAppUpgradeV3(t *testing.T) {
 	require.Equal(t, abci.CodeTypeOK, deliverTxResp.Code, deliverTxResp.Log)
 
 	respEndBlock := testApp.EndBlock(abci.
-		RequestEndBlock{Height: initialHeight + appconsts.UpgradeHeightDelay(v3.Version)})
+		RequestEndBlock{Height: initialHeight + appconsts.UpgradeHeightDelay(testApp.GetChainID(), v3.Version)})
 	require.Equal(t, appconsts.GetTimeoutCommit(v3.Version), respEndBlock.Timeouts.TimeoutCommit)
 	require.Equal(t, appconsts.GetTimeoutPropose(v3.Version), respEndBlock.Timeouts.TimeoutPropose)
 }

diff --git a/pkg/appconsts/chain_ids.go b/pkg/appconsts/chain_ids.go
@@ -0,0 +1,5 @@
+package appconsts
+
+const (
+	ArabicaChainID = "arabica-11"
+)
diff --git a/pkg/appconsts/versioned_consts.go b/pkg/appconsts/versioned_consts.go
@@ -79,7 +79,7 @@ func GetTimeoutCommit(v uint64) time.Duration {
 }
 
 // UpgradeHeightDelay returns the delay in blocks after a quorum has been reached that the chain should upgrade to the new version.
-func UpgradeHeightDelay(v uint64) int64 {
+func UpgradeHeightDelay(chainID string, v uint64) int64 {
 	if OverrideUpgradeHeightDelayStr != "" {
 		parsedValue, err := strconv.ParseInt(OverrideUpgradeHeightDelayStr, 10, 64)
 		if err != nil {
@@ -91,6 +91,12 @@ func UpgradeHeightDelay(v uint64) int64 {
 	case v1.Version:
 		return v1.UpgradeHeightDelay
 	case v2.Version:
+		// ONLY ON ARABICA: don't return the v2 value even when the app version is
+		// v2 on arabica. This is due to a bug that was shipped on arabica, where
+		// the next version was used.
+		if chainID == ArabicaChainID {
+			return v3.UpgradeHeightDelay
+		}
 		return v2.UpgradeHeightDelay
 	default:
 		return v3.UpgradeHeightDelay

diff --git a/pkg/appconsts/versioned_consts_test.go b/pkg/appconsts/versioned_consts_test.go
@@ -80,3 +80,50 @@ func TestVersionedConsts(t *testing.T) {
 		})
 	}
 }
+
+func TestUpgradeHeightDelay(t *testing.T) {
+	tests := []struct {
+		name                       string
+		chainID                    string
+		version                    uint64
+		expectedUpgradeHeightDelay int64
+	}{
+		{
+			name:                       "v1 upgrade delay",
+			chainID:                    "test-chain",
+			version:                    v1.Version,
+			expectedUpgradeHeightDelay: v1.UpgradeHeightDelay,
+		},
+		{
+			name:                       "v1 arabica upgrade delay",
+			chainID:                    "arabica-11",
+			version:                    v1.Version,
+			expectedUpgradeHeightDelay: v1.UpgradeHeightDelay,
+		},
+		{
+			name:                       "v2 upgrade delay on non-arabica chain",
+			chainID:                    "celestia",
+			version:                    v2.Version,
+			expectedUpgradeHeightDelay: v2.UpgradeHeightDelay,
+		},
+		{
+			name:                       "v2 upgrade delay on arabica",
+			chainID:                    "arabica-11",
+			version:                    v2.Version,
+			expectedUpgradeHeightDelay: v3.UpgradeHeightDelay, // falls back to v3 because of arabica bug
+		},
+		{
+			name:                       "v3 upgrade delay",
+			chainID:                    "mocha-4",
+			version:                    3,
+			expectedUpgradeHeightDelay: v3.UpgradeHeightDelay,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			actual := appconsts.UpgradeHeightDelay(tc.chainID, tc.version)
+			require.Equal(t, tc.expectedUpgradeHeightDelay, actual)
+		})
+	}
+}
diff --git a/specs/src/parameters_v2.md b/specs/src/parameters_v2.md
@@ -8,10 +8,11 @@ hardcoded in the application or they are blocked by the `x/paramfilter` module.
 
 ## Global parameters
 
-| Parameter         | Default | Summary                                                                                                                | Changeable via Governance |
-|-------------------|---------|------------------------------------------------------------------------------------------------------------------------|---------------------------|
-| MaxBlockSizeBytes | 100MiB  | Hardcoded value in CometBFT for the protobuf encoded block.                                                            | False                     |
-| MaxSquareSize     | 128     | Hardcoded maximum square size determined per shares per row or column for the original data square (not yet extended). | False                     |
+| Parameter          | Default | Summary                                                                                                                | Changeable via Governance |
+|--------------------|---------|------------------------------------------------------------------------------------------------------------------------|---------------------------|
+| MaxBlockSizeBytes  | 100MiB  | Hardcoded value in CometBFT for the protobuf encoded block.                                                            | False                     |
+| MaxSquareSize      | 128     | Hardcoded maximum square size determined per shares per row or column for the original data square (not yet extended). | False                     |
+| UpgradeHeightDelay | 50400   | Height based delay after a successful `MsgTryUpgrade` has been submitted.                                              | False                     |
 
 ## Module parameters
 

diff --git a/specs/src/parameters_v3.md b/specs/src/parameters_v3.md
@@ -8,10 +8,11 @@ hardcoded in the application or they are blocked by the `x/paramfilter` module.
 
 ## Global parameters
 
-| Parameter         | Default | Summary                                                                                                                | Changeable via Governance |
-|-------------------|---------|------------------------------------------------------------------------------------------------------------------------|---------------------------|
-| MaxBlockSizeBytes | 100MiB  | Hardcoded value in CometBFT for the protobuf encoded block.                                                            | False                     |
-| MaxSquareSize     | 128     | Hardcoded maximum square size determined per shares per row or column for the original data square (not yet extended). | False                     |
+| Parameter          | Default | Summary                                                                                                                | Changeable via Governance |
+|--------------------|---------|------------------------------------------------------------------------------------------------------------------------|---------------------------|
+| MaxBlockSizeBytes  | 100MiB  | Hardcoded value in CometBFT for the protobuf encoded block.                                                            | False                     |
+| MaxSquareSize      | 128     | Hardcoded maximum square size determined per shares per row or column for the original data square (not yet extended). | False                     |
+| UpgradeHeightDelay | 100800  | Height based delay after a successful `MsgTryUpgrade` has been submitted.                                              | False                     |
 
 ## Module parameters
 

diff --git a/test/util/testnode/config.go b/test/util/testnode/config.go
@@ -82,10 +82,10 @@ func (c *Config) WithSuppressLogs(sl bool) *Config {
 	return c
 }
 
-// WithTimeoutCommit sets the TimeoutCommit and returns the Config.
+// WithTimeoutCommit sets the timeout commit in the cometBFT config and returns
+// the Config.
 func (c *Config) WithTimeoutCommit(d time.Duration) *Config {
-	c.TmConfig.Consensus.TimeoutCommit = d
-	return c
+	return c.WithAppCreator(DefaultAppCreator(WithTimeoutCommit(d)))
 }
 
 // WithFundedAccounts sets the genesis accounts and returns the Config.
@@ -132,7 +132,7 @@ func DefaultConfig() *Config {
 		WithTendermintConfig(DefaultTendermintConfig()).
 		WithAppConfig(DefaultAppConfig()).
 		WithAppOptions(DefaultAppOptions()).
-		WithAppCreator(DefaultAppCreator()).
+		WithTimeoutCommit(time.Millisecond * 30).
 		WithSuppressLogs(true)
 }
 
@@ -171,7 +171,15 @@ func DefaultTendermintConfig() *tmconfig.Config {
 	return tmCfg
 }
 
-func DefaultAppCreator() srvtypes.AppCreator {
+type AppCreationOptions func(app *app.App)
+
+func WithTimeoutCommit(d time.Duration) AppCreationOptions {
+	return func(app *app.App) {
+		app.SetEndBlocker(wrapEndBlocker(app, d))
+	}
+}
+
+func DefaultAppCreator(opts ...AppCreationOptions) srvtypes.AppCreator {
 	return func(_ log.Logger, _ tmdb.DB, _ io.Writer, _ srvtypes.AppOptions) srvtypes.Application {
 		encodingConfig := encoding.MakeConfig(app.ModuleEncodingRegisters...)
 		app := app.New(
@@ -184,7 +192,11 @@ func DefaultAppCreator() srvtypes.AppCreator {
 			simapp.EmptyAppOptions{},
 			baseapp.SetMinGasPrices(fmt.Sprintf("%v%v", appconsts.DefaultMinGasPrice, app.BondDenom)),
 		)
-		app.SetEndBlocker(wrapEndBlocker(app, time.Millisecond*30))
+
+		for _, opt := range opts {
+			opt(app)
+		}
+
 		return app
 	}
 }

diff --git a/x/signal/integration_test.go b/x/signal/integration_test.go
@@ -23,10 +23,12 @@ func TestUpgradeIntegration(t *testing.T) {
 	cp := app.DefaultConsensusParams()
 	cp.Version.AppVersion = v2.Version
 	app, _ := testutil.SetupTestAppWithGenesisValSet(cp)
+	chainID := "test"
 	ctx := sdk.NewContext(app.CommitMultiStore(), tmtypes.Header{
 		Version: tmversion.Consensus{
 			App: v2.Version,
 		},
+		ChainID: chainID,
 	}, false, tmlog.NewNopLogger())
 	goCtx := sdk.WrapSDKContext(ctx)
 	ctx = sdk.UnwrapSDKContext(goCtx)
@@ -77,7 +79,7 @@ func TestUpgradeIntegration(t *testing.T) {
 	require.False(t, shouldUpgrade)
 	require.EqualValues(t, 0, version)
 
-	ctx = ctx.WithBlockHeight(ctx.BlockHeight() + appconsts.UpgradeHeightDelay(version))
+	ctx = ctx.WithBlockHeight(ctx.BlockHeight() + appconsts.UpgradeHeightDelay(chainID, version))
 
 	shouldUpgrade, version = app.SignalKeeper.ShouldUpgrade(ctx)
 	require.True(t, shouldUpgrade)

diff --git a/x/signal/keeper.go b/x/signal/keeper.go
@@ -102,9 +102,10 @@ func (k *Keeper) TryUpgrade(ctx context.Context, _ *types.MsgTryUpgrade) (*types
 		if version <= sdkCtx.BlockHeader().Version.App {
 			return &types.MsgTryUpgradeResponse{}, types.ErrInvalidUpgradeVersion.Wrapf("can not upgrade to version %v because it is less than or equal to current version %v", version, sdkCtx.BlockHeader().Version.App)
 		}
+		header := sdkCtx.BlockHeader()
 		upgrade := types.Upgrade{
 			AppVersion:    version,
-			UpgradeHeight: sdkCtx.BlockHeader().Height + appconsts.UpgradeHeightDelay(version),
+			UpgradeHeight: header.Height + appconsts.UpgradeHeightDelay(header.ChainID, header.Version.App),
 		}
 		k.setUpgrade(sdkCtx, upgrade)
 	}

diff --git a/x/signal/keeper_test.go b/x/signal/keeper_test.go
@@ -183,7 +183,7 @@ func TestTallyingLogic(t *testing.T) {
 	require.False(t, shouldUpgrade) // should be false because upgrade height hasn't been reached.
 	require.Equal(t, uint64(0), version)
 
-	ctx = ctx.WithBlockHeight(ctx.BlockHeight() + appconsts.UpgradeHeightDelay(version))
+	ctx = ctx.WithBlockHeight(ctx.BlockHeight() + appconsts.UpgradeHeightDelay("test", version))
 
 	shouldUpgrade, version = upgradeKeeper.ShouldUpgrade(ctx)
 	require.True(t, shouldUpgrade) // should be true because upgrade height has been reached.
@@ -426,7 +426,7 @@ func TestGetUpgrade(t *testing.T) {
 		got, err := upgradeKeeper.GetUpgrade(ctx, &types.QueryGetUpgradeRequest{})
 		require.NoError(t, err)
 		assert.Equal(t, v2.Version, got.Upgrade.AppVersion)
-		assert.Equal(t, appconsts.UpgradeHeightDelay(v2.Version), got.Upgrade.UpgradeHeight)
+		assert.Equal(t, appconsts.UpgradeHeightDelay("test", v2.Version), got.Upgrade.UpgradeHeight)
 	})
 }
 
@@ -441,6 +441,7 @@ func setup(t *testing.T) (signal.Keeper, sdk.Context, *mockStakingKeeper) {
 			Block: 1,
 			App:   1,
 		},
+		ChainID: "test",
 	}, false, log.NewNopLogger())
 	mockStakingKeeper := newMockStakingKeeper(
 		map[string]int64{