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.

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

chore: bump sdk to v1.20.0 #2860

Merged
merged 13 commits into from
Nov 22, 2023
5 changes: 1 addition & 4 deletions app/app.go
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.

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
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
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
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
10 changes: 3 additions & 7 deletions app/test/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/celestiaorg/celestia-app/app"
"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/blob"
"github.com/celestiaorg/celestia-app/pkg/da"
appns "github.com/celestiaorg/celestia-app/pkg/namespace"
Expand Down Expand Up @@ -191,10 +192,7 @@ func (s *IntegrationTestSuite) TestMaxBlockSize() {
require.LessOrEqual(t, size, uint64(appconsts.DefaultGovMaxSquareSize))
require.GreaterOrEqual(t, size, uint64(appconsts.MinSquareSize))

// assert that the app version is correctly set
// FIXME: This should return the latest version but tendermint v0.34.x doesn't copy
// over the version when converting from proto so it disappears
require.EqualValues(t, 0, blockRes.Block.Header.Version.App)
require.EqualValues(t, v2.Version, blockRes.Block.Header.Version.App)

sizes = append(sizes, size)
ExtendBlobTest(t, blockRes.Block)
Expand Down Expand Up @@ -329,9 +327,7 @@ func (s *IntegrationTestSuite) TestShareInclusionProof() {
blockRes, err := node.Block(context.Background(), &txResp.Height)
require.NoError(t, err)

// FIXME: This should return the latest version but tendermint v0.34.x doesn't copy
// over the version when converting from proto so it disappears
require.EqualValues(t, 0, blockRes.Block.Header.Version.App)
require.EqualValues(t, v2.Version, blockRes.Block.Header.Version.App)

_, isBlobTx := coretypes.UnmarshalBlobTx(blockRes.Block.Txs[txResp.Index])
require.True(t, isBlobTx)
Expand Down
12 changes: 1 addition & 11 deletions cmd/celestia-appd/cmd/root.go
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
4 changes: 2 additions & 2 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ require (
github.com/bits-and-blooms/bitset v1.7.0 // indirect
github.com/consensys/bavard v0.1.13 // indirect
github.com/consensys/gnark-crypto v0.12.1 // indirect
github.com/cosmos/ledger-cosmos-go v0.13.2 // indirect
github.com/cosmos/ledger-cosmos-go v0.12.4 // indirect
rootulp marked this conversation as resolved.
Show resolved Hide resolved
github.com/crate-crypto/go-kzg-4844 v0.7.0 // indirect
github.com/docker/distribution v2.8.2+incompatible // indirect
github.com/docker/go-connections v0.4.0 // indirect
Expand Down 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
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
8 changes: 4 additions & 4 deletions go.sum
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 h1:L2j5luXXoEMQLzJkxBixeeEWkgcxnGFm6HnwN3X3L9g=
github.com/celestiaorg/cosmos-sdk v1.20.0-sdk-v0.46.16/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 Expand Up @@ -420,8 +420,8 @@ github.com/cosmos/iavl v0.19.6 h1:XY78yEeNPrEYyNCKlqr9chrwoeSDJ0bV2VjocTk//OU=
github.com/cosmos/iavl v0.19.6/go.mod h1:X9PKD3J0iFxdmgNLa7b2LYWdsGd90ToV5cAONApkEPw=
github.com/cosmos/ibc-go/v6 v6.2.1 h1:NiaDXTRhKwf3n9kELD4VRIe5zby1yk1jBvaz9tXTQ6k=
github.com/cosmos/ibc-go/v6 v6.2.1/go.mod h1:XLsARy4Y7+GtAqzMcxNdlQf6lx+ti1e8KcMGv5NIK7A=
github.com/cosmos/ledger-cosmos-go v0.13.2 h1:aY0KZSmUwNKbBm9OvbIjvf7Ozz2YzzpAbgvN2C8x2T0=
github.com/cosmos/ledger-cosmos-go v0.13.2/go.mod h1:HENcEP+VtahZFw38HZ3+LS3Iv5XV6svsnkk9vdJtLr8=
github.com/cosmos/ledger-cosmos-go v0.12.4 h1:drvWt+GJP7Aiw550yeb3ON/zsrgW0jgh5saFCr7pDnw=
github.com/cosmos/ledger-cosmos-go v0.12.4/go.mod h1:fjfVWRf++Xkygt9wzCsjEBdjcf7wiiY35fv3ctT+k4M=
github.com/cpuguy83/go-md2man v1.0.10 h1:BSKMNlYxDvnunlTymqtgONjNnaRV1sTpcovwwjF22jk=
github.com/cpuguy83/go-md2man v1.0.10/go.mod h1:SmD6nW6nTyfqj6ABTjUi3V3JVMnlJmwcJI5acqYI6dE=
github.com/cpuguy83/go-md2man/v2 v2.0.0-20190314233015-f79a8a8ca69d/go.mod h1:maD7wRr/U5Z6m/iR4s+kqSMx2CaBsrgA7czyZG/E6dU=
Expand Down
1 change: 1 addition & 0 deletions test/e2e/simple_test.go
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
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
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
2 changes: 1 addition & 1 deletion x/blob/ante/max_total_blob_size_ante.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func (d MaxTotalBlobSizeDecorator) getMaxSquareSize(ctx sdk.Context) int {
return int(appconsts.DefaultGovMaxSquareSize)
}

upperBound := appconsts.SquareSizeUpperBound(ctx.ConsensusParams().Version.AppVersion)
upperBound := appconsts.SquareSizeUpperBound(ctx.BlockHeader().Version.App)
govParam := d.k.GovMaxSquareSize(ctx)
return min(upperBound, int(govParam))
}
Expand Down
12 changes: 8 additions & 4 deletions x/upgrade/upgrade_test.go
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