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.

Sign up for GitHub

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

Jump to bottom

fix!: v3 upgrade delay #4008

Merged
merged 11 commits into from
Oct 29, 2024
Merged

fix!: v3 upgrade delay #4008

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
15086e8
fix!: use the correct version when upgrading
evan-forbes Oct 24, 2024
fd4b108
fix: lingering api breaks
evan-forbes Oct 24, 2024
c9b257e
test: add unit test for versioned upgrade
evan-forbes Oct 24, 2024
07a1271
chore: deprecate WithTimeoutCommit
evan-forbes Oct 24, 2024
ba4ef7b
refactor: use arabica-11 instead of all arabica networks
evan-forbes Oct 28, 2024
bce247e
Update x/signal/keeper.go
evan-forbes Oct 28, 2024
02b2a78
add upgradeHeightdelay to the specs
evan-forbes Oct 28, 2024
93ac7b6
Merge branch 'evan/fix-v3-upgrade-delay' of github.com:celestiaorg/la…
evan-forbes Oct 28, 2024
04c7c34
chore: linter
evan-forbes Oct 28, 2024
ec29c4b
Merge branch 'main' into evan/fix-v3-upgrade-delay
evan-forbes Oct 28, 2024
c53ee35
refactor: timeout option
evan-forbes Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/test/priority_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func (s *PriorityTestSuite) SetupSuite() {
cfg := testnode.DefaultConfig().
WithFundedAccounts(s.accountNames...).
// use a long block time to guarantee that some transactions are included in the same block
WithTimeoutCommit(time.Second)
WithBlockTime(time.Second)
evan-forbes marked this conversation as resolved.
Show resolved Hide resolved

cctx, _, _ := testnode.NewNetwork(t, cfg)
s.cctx = cctx
Expand Down
4 changes: 3 additions & 1 deletion app/test/square_size_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)).
WithBlockTime(time.Second)

cctx, rpcAddr, grpcAddr := testnode.NewNetwork(t, cfg)

Expand Down
8 changes: 4 additions & 4 deletions app/test/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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),
evan-forbes marked this conversation as resolved.
Show resolved Hide resolved
})

require.Equal(t, appconsts.GetTimeoutCommit(appVersion), endBlockResp.Timeouts.TimeoutCommit)
Expand All @@ -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},
},
})
Expand All @@ -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)
}
Expand Down
5 changes: 5 additions & 0 deletions pkg/appconsts/chain_ids.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package appconsts

const (
ArabicaChainID = "arabica-11"
)
8 changes: 7 additions & 1 deletion pkg/appconsts/versioned_consts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand Down
53 changes: 53 additions & 0 deletions pkg/appconsts/versioned_consts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,56 @@ 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 an arabica network other than arabica-11",
chainID: "arabica-11",
version: v2.Version,
expectedUpgradeHeightDelay: v3.UpgradeHeightDelay, // falls back to v3 because of arabica bug
},
{
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)
})
}
}
9 changes: 5 additions & 4 deletions specs/src/parameters_v2.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
evan-forbes marked this conversation as resolved.
Show resolved Hide resolved

## Module parameters

Expand Down
9 changes: 5 additions & 4 deletions specs/src/parameters_v3.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Comment on lines -11 to +15
Copy link
Member Author

Choose a reason for hiding this comment

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

added the version const to the spec, but there are a lot of versioned consts unrelated to this PR missing here

Copy link
Member Author

Choose a reason for hiding this comment

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

#4010


## Module parameters

Expand Down
4 changes: 2 additions & 2 deletions test/txsim/run_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ func TestTxSimulator(t *testing.T) {
func Setup(t testing.TB) (keyring.Keyring, string, string) {
t.Helper()

cfg := testnode.DefaultConfig().WithTimeoutCommit(300 * time.Millisecond).WithFundedAccounts("txsim-master")
cfg := testnode.DefaultConfig().WithBlockTime(300 * time.Millisecond).WithFundedAccounts("txsim-master")

cctx, rpcAddr, grpcAddr := testnode.NewNetwork(t, cfg)

Expand All @@ -165,7 +165,7 @@ func TestTxSimUpgrade(t *testing.T) {
cp := app.DefaultConsensusParams()
cp.Version.AppVersion = v2.Version
cfg := testnode.DefaultConfig().
WithTimeoutCommit(300 * time.Millisecond).
WithBlockTime(300 * time.Millisecond).
WithConsensusParams(cp).
WithFundedAccounts("txsim-master")
cctx, _, grpcAddr := testnode.NewNetwork(t, cfg)
Expand Down
18 changes: 14 additions & 4 deletions test/util/testnode/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,17 @@ func (c *Config) WithSuppressLogs(sl bool) *Config {
return c
}

// WithTimeoutCommit sets the TimeoutCommit and returns the Config.
// WithBlockTime sets the block time and returns the Config.
func (c *Config) WithBlockTime(d time.Duration) *Config {
c.TmConfig.Consensus.TimeoutCommit = d
creator := DefaultAppCreator(d)
c.WithAppCreator(creator)
return c
}
Copy link
Member Author

Choose a reason for hiding this comment

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

added this to help fix the insanely flakey square size test

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not still call this TimeoutCommit because it changes the TimeoutCommit not the BlockTime

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry, I'm not understanding this comment completely. the reasoning for this change was because we were overriding the timeout

Copy link
Contributor

Choose a reason for hiding this comment

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

You deprecated the method WithTimeoutComimt and created a new method called WithBlockTime. My comment was why not just modify the WithTimeoutCommit? WithBlockTime is misleading because it doesn't change the block time to that duration, just the timeout commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

ahh I get it now thanks, good point

c53ee35


// Deprecated: use WithBlockTime instead. WithTimeoutCommit sets the timeout
// commit in the cometBFT config and returns the Config. Warning, this won't
// actually change the block time and is being deprecated.
func (c *Config) WithTimeoutCommit(d time.Duration) *Config {
c.TmConfig.Consensus.TimeoutCommit = d
return c
Expand Down Expand Up @@ -132,7 +142,7 @@ func DefaultConfig() *Config {
WithTendermintConfig(DefaultTendermintConfig()).
WithAppConfig(DefaultAppConfig()).
WithAppOptions(DefaultAppOptions()).
WithAppCreator(DefaultAppCreator()).
WithAppCreator(DefaultAppCreator(time.Millisecond * 30)).
evan-forbes marked this conversation as resolved.
Show resolved Hide resolved
WithSuppressLogs(true)
}

Expand Down Expand Up @@ -171,7 +181,7 @@ func DefaultTendermintConfig() *tmconfig.Config {
return tmCfg
}

func DefaultAppCreator() srvtypes.AppCreator {
func DefaultAppCreator(blockTime time.Duration) srvtypes.AppCreator {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the options paradigm to modify this. To me, this isn't really a default anymore if you need to specify the blockTime.

We can just have something like:

type AppCreationOptions func (app *App)

func WithTimeoutCommit(d time.Duration) {
    return func (app *App) {
        app.SetEndBlocker(wrapEndBlocker, app, d)
    }
}

func DefaultAppCreator(AppCreationOptions...) srvtypes.AppCreator {
...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea c53ee35

return func(_ log.Logger, _ tmdb.DB, _ io.Writer, _ srvtypes.AppOptions) srvtypes.Application {
encodingConfig := encoding.MakeConfig(app.ModuleEncodingRegisters...)
app := app.New(
Expand All @@ -184,7 +194,7 @@ func DefaultAppCreator() srvtypes.AppCreator {
simapp.EmptyAppOptions{},
baseapp.SetMinGasPrices(fmt.Sprintf("%v%v", appconsts.DefaultMinGasPrice, app.BondDenom)),
)
app.SetEndBlocker(wrapEndBlocker(app, time.Millisecond*30))
app.SetEndBlocker(wrapEndBlocker(app, blockTime))
return app
}
}
Expand Down
4 changes: 3 additions & 1 deletion x/signal/integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
3 changes: 2 additions & 1 deletion x/signal/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
5 changes: 3 additions & 2 deletions x/signal/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
})
}

Expand All @@ -441,6 +441,7 @@ func setup(t *testing.T) (signal.Keeper, sdk.Context, *mockStakingKeeper) {
Block: 1,
App: 1,
},
ChainID: "test",
evan-forbes marked this conversation as resolved.
Show resolved Hide resolved
}, false, log.NewNopLogger())
mockStakingKeeper := newMockStakingKeeper(
map[string]int64{
Expand Down
Loading