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

chore: bump sdk to v1.20.0 #2860

Merged
merged 13 commits into from
Nov 22, 2023
Merged

chore: bump sdk to v1.20.0 #2860

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
20009fd
chore: bump sdk to v1.20.0
cmwaters Nov 20, 2023
fc378bd
fix compile errors
cmwaters Nov 20, 2023
9536657
try to get the v2 upgrade to work
cmwaters Nov 21, 2023
a6b82fc
bump to rc1
cmwaters Nov 21, 2023
856c42f
take 3
cmwaters Nov 21, 2023
69f9d6a
fix the test to make it work
cmwaters Nov 21, 2023
9dec0bb
gofumpt
cmwaters Nov 21, 2023
16e071a
Merge branch 'main' into cal/bump-sdk
cmwaters Nov 21, 2023
fa90c36
use block header version
cmwaters Nov 21, 2023
e60aed5
fix integration tests
cmwaters Nov 21, 2023
71dd65a
use v0.12.4 instead
cmwaters Nov 22, 2023
6cb16da
Merge branch 'main' into cal/bump-sdk
cmwaters Nov 22, 2023
1dae88c
use the actual v1.20.0 version
cmwaters Nov 22, 2023
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
5 changes: 1 addition & 4 deletions app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,7 @@ func (app *App) EndBlocker(ctx sdk.Context, req abci.RequestEndBlock) abci.Respo
// 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)
app.SetAppVersion(ctx, v2.Version)
staheri14 marked this conversation as resolved.
Show resolved Hide resolved
}
evan-forbes marked this conversation as resolved.
Show resolved Hide resolved
return res
}
Expand All @@ -587,9 +587,6 @@ func (app *App) InitChainer(ctx sdk.Context, req abci.RequestInitChain) abci.Res
if err := tmjson.Unmarshal(req.AppStateBytes, &genesisState); err != nil {
panic(err)
}
if req.ConsensusParams != nil && req.ConsensusParams.Version != nil {
app.SetProtocolVersion(req.ConsensusParams.Version.AppVersion)
}
Comment on lines -590 to -592
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this no longer necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's done in the SDK instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See https://github.com/celestiaorg/cosmos-sdk/blob/18e768c968e05660e4ad0a7f4cc3ea8a33d5c993/baseapp/abci.go#L64

Copy link
Collaborator

Choose a reason for hiding this comment

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

Assuming it can be done in either location (app or SDK), why do it in the SDK where it has the risk of getting overwritten when we pull upstream changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this behaviour is logically the responsibility of baseapp not app

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then can it be upstreamed to cosmos/cosmos-sdk so it doesn't accidentally get reverted in celestiaorg/cosmos-sdk when we pull upstream changes?

return app.mm.InitGenesis(ctx, app.appCodec, genesisState)
evan-forbes marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down
2 changes: 1 addition & 1 deletion app/prepare_proposal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func (app *App) PrepareProposal(req abci.RequestPrepareProposal) abci.ResponsePr

// build the square from the set of valid and prioritised transactions.
// The txs returned are the ones used in the square and block
dataSquare, txs, err := square.Build(txs, app.GetBaseApp().AppVersion(), app.GovSquareSizeUpperBound(sdkCtx))
dataSquare, txs, err := square.Build(txs, app.GetBaseApp().AppVersion(sdkCtx), app.GovSquareSizeUpperBound(sdkCtx))
if err != nil {
panic(err)
}
Expand Down
2 changes: 1 addition & 1 deletion app/process_proposal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) (resp abci.Resp
}

// Construct the data square from the block's transactions
dataSquare, err := square.Construct(req.BlockData.Txs, app.GetBaseApp().AppVersion(), app.GovSquareSizeUpperBound(sdkCtx))
dataSquare, err := square.Construct(req.BlockData.Txs, app.GetBaseApp().AppVersion(sdkCtx), app.GovSquareSizeUpperBound(sdkCtx))
if err != nil {
logInvalidPropBlockError(app.Logger(), req.Header, "failure to compute data square from transactions:", err)
return reject()
Expand Down
8 changes: 2 additions & 6 deletions app/square_size.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,6 @@ func (app *App) GovSquareSizeUpperBound(ctx sdk.Context) int {
}

gmax := int(app.BlobKeeper.GovMaxSquareSize(ctx))
// perform a secondary check on the max square size.
if gmax > appconsts.SquareSizeUpperBound(app.AppVersion()) {
gmax = appconsts.SquareSizeUpperBound(app.AppVersion())
}

return gmax
hardMax := appconsts.SquareSizeUpperBound(ctx.BlockHeader().Version.App)
return min(gmax, hardMax)
}
evan-forbes marked this conversation as resolved.
Show resolved Hide resolved
12 changes: 1 addition & 11 deletions cmd/celestia-appd/cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"io"
"os"
"path/filepath"
"strconv"

bscmd "github.com/celestiaorg/celestia-app/x/blobstream/client"

Expand Down Expand Up @@ -237,20 +236,11 @@ 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.
upgradeHeight,
cast.ToInt64(appOpts.Get(UpgradeHeightFlag)),
appOpts,
baseapp.SetPruning(pruningOpts),
baseapp.SetMinGasPrices(cast.ToString(appOpts.Get(server.FlagMinGasPrices))),
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ require (
)

replace (
github.com/cosmos/cosmos-sdk => github.com/celestiaorg/cosmos-sdk v1.18.3-sdk-v0.46.14
github.com/cosmos/cosmos-sdk => github.com/celestiaorg/cosmos-sdk v1.20.0-sdk-v0.46.16-rc1
github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1
github.com/syndtr/goleveldb => github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7
github.com/tendermint/tendermint => github.com/celestiaorg/celestia-core v1.29.0-tm-v0.34.29
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,8 +322,8 @@ github.com/celestiaorg/blobstream-contracts/v3 v3.1.0 h1:h1Y4V3EMQ2mFmNtWt2sIhZI
github.com/celestiaorg/blobstream-contracts/v3 v3.1.0/go.mod h1:x4DKyfKOSv1ZJM9NwV+Pw01kH2CD7N5zTFclXIVJ6GQ=
github.com/celestiaorg/celestia-core v1.29.0-tm-v0.34.29 h1:Fd7ymPUzExPGNl2gZw4i5S74arMw+iDHLE78M/cCxl4=
github.com/celestiaorg/celestia-core v1.29.0-tm-v0.34.29/go.mod h1:xrICN0PBhp3AdTaZ8q4wS5Jvi32V02HNjaC2EsWiEKk=
github.com/celestiaorg/cosmos-sdk v1.18.3-sdk-v0.46.14 h1:+Te28r5Zp4Vp69f82kcON9/BIF8f1BNXb0go2+achuc=
github.com/celestiaorg/cosmos-sdk v1.18.3-sdk-v0.46.14/go.mod h1:Og5KKgoBEiQlI6u56lDLG191pfknIdXctFn3COWLQP8=
github.com/celestiaorg/cosmos-sdk v1.20.0-sdk-v0.46.16-rc1 h1:GtiF/vbSeJVd89W5b3jCFpik9JwcDmYsvZ1o9Tqg8Ds=
github.com/celestiaorg/cosmos-sdk v1.20.0-sdk-v0.46.16-rc1/go.mod h1:Tvsc3YnqvflXTYC8xIy/Q07Es95xZ1pZC/imoKogtbg=
github.com/celestiaorg/knuu v0.10.0 h1:uYO6hVsoCAJ/Q4eV0Pn8CLbbupGAjD56xQc4y2t4lf0=
github.com/celestiaorg/knuu v0.10.0/go.mod h1:2NjDX29x09hRpaaWaKvi7pw9VjI/SHo+/4HldyEW50g=
github.com/celestiaorg/merkletree v0.0.0-20210714075610-a84dc3ddbbe4 h1:CJdIpo8n5MFP2MwK0gSRcOVlDlFdQJO1p+FqdxYzmvc=
Expand Down
1 change: 1 addition & 0 deletions test/e2e/simple_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ func TestE2ESimple(t *testing.T) {
switch {
case isSemVer:
case latestVersion == "latest":
case len(latestVersion) == 7:
case len(latestVersion) == 8:
// assume this is a git commit hash (we need to trim the last digit to match the docker image tag)
latestVersion = latestVersion[:7]
Comment on lines 33 to 39
Copy link
Contributor

Choose a reason for hiding this comment

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

The switch statement has cases that fall through without any action or comment, which can be confusing. Specifically, the cases for isSemVer, latestVersion == "latest", and len(latestVersion) == 7 do not have any code or comments. It's important to either implement the necessary logic or add comments explaining why no action is taken. This will improve the maintainability and readability of the code.

		switch {
		case isSemVer:
+			// Handle semantic versioning specific logic here, if any.
		case latestVersion == "latest":
+			// Handle the case when the latest version is denoted by the string "latest".
		case len(latestVersion) == 7:
+			// If the length of latestVersion is exactly 7 characters, no action is needed.
+			// This case assumes that the version is already in the correct format for further processing.
		case len(latestVersion) == 8:
			// assume this is a git commit hash (we need to trim the last digit to match the docker image tag)
			latestVersion = latestVersion[:7]
Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
switch {
case isSemVer:
case latestVersion == "latest":
case len(latestVersion) == 7:
case len(latestVersion) == 8:
// assume this is a git commit hash (we need to trim the last digit to match the docker image tag)
latestVersion = latestVersion[:7]
switch {
case isSemVer:
// Handle semantic versioning specific logic here, if any.
case latestVersion == "latest":
// Handle the case when the latest version is denoted by the string "latest".
case len(latestVersion) == 7:
// If the length of latestVersion is exactly 7 characters, no action is needed.
// This case assumes that the version is already in the correct format for further processing.
case len(latestVersion) == 8:
// assume this is a git commit hash (we need to trim the last digit to match the docker image tag)
latestVersion = latestVersion[:7]

Copy link
Collaborator

Choose a reason for hiding this comment

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

good idea!

Expand Down
25 changes: 10 additions & 15 deletions test/e2e/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

"github.com/celestiaorg/celestia-app/app"
"github.com/celestiaorg/celestia-app/app/encoding"
v1 "github.com/celestiaorg/celestia-app/pkg/appconsts/v1"
v2 "github.com/celestiaorg/celestia-app/pkg/appconsts/v2"
"github.com/celestiaorg/celestia-app/test/txsim"
"github.com/celestiaorg/knuu/pkg/knuu"
Expand Down Expand Up @@ -134,6 +135,7 @@ func TestMajorUpgradeToV2(t *testing.T) {
switch {
case isSemVer:
case latestVersion == "latest":
case len(latestVersion) == 7:
case len(latestVersion) == 8:
// assume this is a git commit hash (we need to trim the last digit to match the docker image tag)
latestVersion = latestVersion[:7]
cmwaters marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -143,7 +145,7 @@ func TestMajorUpgradeToV2(t *testing.T) {
}

numNodes := 4
upgradeHeight := int64(10)
upgradeHeight := int64(12)
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

Expand All @@ -158,7 +160,6 @@ func TestMajorUpgradeToV2(t *testing.T) {
require.NoError(t, err)

for i := 0; i < numNodes; i++ {
t.Log("Starting node", "node", i, "version", latestVersion)
require.NoError(t, testnet.CreateGenesisNode(latestVersion, 10000000, upgradeHeight))
}

Expand All @@ -168,16 +169,6 @@ func TestMajorUpgradeToV2(t *testing.T) {
require.NoError(t, testnet.Setup())
require.NoError(t, testnet.Start())

// assert that the network is initially running on v1
for i := 0; i < numNodes; i++ {
client, err := testnet.Node(i).Client()
require.NoError(t, err)
resp, err := client.Header(ctx, nil)
require.NoError(t, err)
// FIXME: we are not correctly setting the app version at genesis
require.Equal(t, uint64(0), resp.Header.Version.App, "version mismatch before upgrade")
}

errCh := make(chan error)
encCfg := encoding.MakeConfig(app.ModuleEncodingRegisters...)
opts := txsim.DefaultOptions().WithSeed(seed).SuppressLogs()
Expand All @@ -187,12 +178,16 @@ func TestMajorUpgradeToV2(t *testing.T) {
errCh <- txsim.Run(ctx, testnet.GRPCEndpoints()[0], kr, encCfg, opts, sequences...)
}()

// wait for all nodes to move past the upgrade height
// assert that the network is initially running on v1
heightBefore := upgradeHeight - 1
for i := 0; i < numNodes; i++ {
client, err := testnet.Node(i).Client()
require.NoError(t, err)
require.NoError(t, waitForHeight(ctx, client, upgradeHeight+2, time.Minute))
resp, err := client.Header(ctx, nil)
require.NoError(t, waitForHeight(ctx, client, upgradeHeight, time.Minute))
resp, err := client.Header(ctx, &heightBefore)
require.NoError(t, err)
require.Equal(t, v1.Version, resp.Header.Version.App, "version mismatch before upgrade")
resp, err = client.Header(ctx, &upgradeHeight)
require.NoError(t, err)
require.Equal(t, v2.Version, resp.Header.Version.App, "version mismatch after upgrade")
}
Expand Down
2 changes: 1 addition & 1 deletion test/util/malicious/out_of_order_prepare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func (a *App) OutOfOrderPrepareProposal(req abci.RequestPrepareProposal) abci.Re

// build the square from the set of valid and prioritised transactions.
// The txs returned are the ones used in the square and block
dataSquare, txs, err := Build(txs, a.GetBaseApp().AppVersion(), a.GovSquareSizeUpperBound(sdkCtx), OutOfOrderExport)
dataSquare, txs, err := Build(txs, a.GetBaseApp().AppVersion(sdkCtx), a.GovSquareSizeUpperBound(sdkCtx), OutOfOrderExport)
if err != nil {
panic(err)
}
cmwaters marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
12 changes: 8 additions & 4 deletions x/upgrade/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/celestiaorg/celestia-app/app/encoding"
"github.com/celestiaorg/celestia-app/test/util"
"github.com/cosmos/cosmos-sdk/crypto/keyring"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/stretchr/testify/require"
abci "github.com/tendermint/tendermint/abci/types"
"github.com/tendermint/tendermint/libs/log"
Expand All @@ -21,11 +22,12 @@ func TestUpgradeAppVersion(t *testing.T) {

testApp.BeginBlock(abci.RequestBeginBlock{Header: tmproto.Header{Height: 2}})
// app version should not have changed yet
require.EqualValues(t, 1, testApp.AppVersion())
require.EqualValues(t, 1, testApp.AppVersion(sdk.Context{}))
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())
require.EqualValues(t, 2, testApp.AppVersion(sdk.Context{}))
}

func setupTestApp(t *testing.T, upgradeHeight int64) (*app.App, keyring.Keyring) {
Expand All @@ -40,7 +42,8 @@ func setupTestApp(t *testing.T, upgradeHeight int64) (*app.App, keyring.Keyring)

stateBytes, err := json.MarshalIndent(genesisState, "", " ")
require.NoError(t, err)
require.EqualValues(t, 0, testApp.GetBaseApp().AppVersion())
infoResp := testApp.Info(abci.RequestInfo{})
require.EqualValues(t, 0, infoResp.AppVersion)

cp := app.DefaultConsensusParams()
abciParams := &abci.ConsensusParams{
Expand All @@ -64,7 +67,8 @@ func setupTestApp(t *testing.T, upgradeHeight int64) (*app.App, keyring.Keyring)
)

// assert that the chain starts with version provided in genesis
require.EqualValues(t, app.DefaultConsensusParams().Version.AppVersion, testApp.GetBaseApp().AppVersion())
infoResp = testApp.Info(abci.RequestInfo{})
require.EqualValues(t, app.DefaultConsensusParams().Version.AppVersion, infoResp.AppVersion)

_ = testApp.Commit()
return testApp, kr
Expand Down
Loading