fix: export app version #408

najeal · 2024-07-05T17:34:44Z

Description

This changes the export command, embedding appVersion value in the exported struct.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

najeal · 2024-07-05T17:40:39Z

@rootulp I tried to replace path of cosmos-sdk dependency on celestia-app to test locally but it looks like scripts/single-node.sh fails to start a node. I don't know if there is some requirements in addition to make build && make install ?

rootulp · 2024-07-08T14:58:19Z

server/export.go

+				Version: tmproto.VersionParams{
+					AppVersion: exported.ConsensusParams.GetVersion().GetAppVersion(),
+				},


[question] I'm curious why upstream Cosmos SDK didn't already have this diff. Without this diff, how do other Cosmos SDK chains stop + export + start a chain?

I have no idea

rootulp · 2024-07-12T08:31:18Z

@rootulp I tried to replace path of cosmos-sdk dependency on celestia-app to test locally but it looks like scripts/single-node.sh fails to start a node. I don't know if there is some requirements in addition to make build && make install ?

@najeal sorry I totally missed this. I usually test cosmos-sdk changes via this diff in celestia-app go.mod:

-	github.com/cosmos/cosmos-sdk => github.com/celestiaorg/cosmos-sdk v1.23.0-sdk-v0.46.16
+	github.com/cosmos/cosmos-sdk => ../cosmos-sdk

and I can start single-node.sh after trying that with your PR. Can you elaborate on what error you're hitting?

najeal · 2024-07-12T16:48:33Z

@rootulp no problem !

My issue was the same than celestiaorg/celestia-app#3362 but syncing celestia-app with main branch resolved it 👍

cmwaters · 2024-07-19T10:35:04Z

server/export_test.go

@@ -152,6 +153,7 @@ func setupApp(t *testing.T, tempDir string) (*simapp.SimApp, context.Context, *t
 			AppStateBytes:   genDoc.AppState,
 		},
 	)
+	app.SetInitialAppVersionInConsensusParams(app.NewContext(false, tmproto.Header{}), simapp.DefaultConsensusParams.Version.AppVersion)


For backwards compatibility, app version should only be part of the consensus params from v2 onwards. Because of this we should only set the initial app version if the version is v2 (which the default is not)

I pushed new commit to only init app version when value is equal or higher than 2 and only export app version when equal or higher than 2.

rootulp

Overall LGTM! Were you able to test celestia-app with this change (using the replace in go.mod) and try these test cases:

Start a celestia-appd v1.13.0 binary. Wait a few blocks then export.
Start a celestia-appd v2.0.0-rc4 binary with a --v2-upgrade-height 5 and export before block height 5
Start a celestia-appd v2.0.0-rc4 binary with a --v2-upgrade-height 5 and export after block height 5

rootulp · 2024-07-23T14:02:57Z

server/export_test.go

 	}
+	for _, utest := range tests {


Suggested change

for _, utest := range tests {

for _, test := range tests {

rootulp · 2024-07-23T14:06:44Z

server/export.go

@@ -95,6 +95,10 @@ func ExportCmd(appExporter types.AppExporter, defaultNodeHome string) *cobra.Com
 					PubKeyTypes: exported.ConsensusParams.Validator.PubKeyTypes,
 				},
 			}
+			const minAppVersionExport = 2


[nit] can this be extracted to the top of the file in the const block? Also a comment would help.

const ( FlagHeight = "height" FlagForZeroHeight = "for-zero-height" FlagJailAllowedAddrs = "jail-allowed-addrs" // minAppVersionExport is the lowest app version where the app version is stored in consensus params and exported via this command. minAppVersionExport = 2 )

najeal · 2024-07-23T16:42:10Z

Overall LGTM! Were you able to test celestia-app with this change (using the replace in go.mod) and try these test cases:

Start a celestia-appd v1.13.0 binary. Wait a few blocks then export.

Start a celestia-appd v2.0.0-rc4 binary with a --v2-upgrade-height 5 and export before block height 5

Start a celestia-appd v2.0.0-rc4 binary with a --v2-upgrade-height 5 and export after block height 5

Result looks good to me:

"version":{}
"version":{}
"version":{"app_version":"2"}

najeal · 2024-07-23T17:01:42Z

The tests you asked for are still valid after the two commits (see the results above)

But I may have spotted one issue. It looks like appVersion is upgraded just before the height provided by --v2-upgrade-height
For example:

--v2-upgrade-height 3
I stop the node right after these logs (block 2 indexation):

6:53PM INF finalizing commit of block hash={} height=2 module=consensus num_txs=0 root=13CD6CA14FF49F70C487F454E72C6C1F1AB32C6356F791E09249D2DD3426DF92
6:53PM INF minted coins from module account amount=33698055utia from=mint module=x/bank
6:53PM INF upgrading from app version 1 to 2
6:53PM INF executed block height=2 module=state num_invalid_txs=0 num_valid_txs=0
6:53PM INF adding a new module: interchainaccounts
6:53PM INF created new capability module=ibc name=ports/icahost
6:53PM INF port binded module=x/ibc/port port=icahost
6:53PM INF claimed capability capability=2 module=icahost name=ports/icahost
6:53PM INF adding a new module: minfee
6:53PM INF adding a new module: packetfowardmiddleware
6:53PM INF adding a new module: signal
6:53PM INF commit synced commit=436F6D6D697449447B5B31383820343420313133203233392031322031383520373720363720323033203136382037362034352031393620363220323920313134203632203739203135372032352031323520313620323134203231322032303620313230203137342034342031313220383720313033203136385D3A327D
6:53PM INF committed state app_hash=BC2C71EF0CB94D43CBA84C2DC43E1D723E4F9D197D10D6D4CE78AE2C705767A8 height=2 module=state num_txs=0
6:53PM INF indexed block exents height=2 module=txindex

If I export, I will already see the "version":{"app_version":"2"}

It is the same for --v2-upgrade-height 5
I will get "version":{"app_version":"2"} right after the log of block 4 indexation

rootulp · 2024-07-23T18:10:10Z

Thanks for your thorough testing. The upgrade from app version 1 -> 2 takes place in EndBlock of the block height immediately before the upgrade height (see these lines). So what you observed is the expected behavior even though it is confusing.

rootulp · 2024-07-23T18:10:42Z

After this PR gets another approval, we can merge, I'll cut a release of cosmos-sdk and then we can bump to that release in celestia-app.

server: export app version

a662b80

najeal requested a review from a team as a code owner July 5, 2024 17:34

najeal requested review from rootulp and staheri14 and removed request for a team July 5, 2024 17:34

rootulp assigned najeal Jul 8, 2024

rootulp reviewed Jul 8, 2024

View reviewed changes

cmwaters reviewed Jul 19, 2024

View reviewed changes

najeal added 2 commits July 23, 2024 10:40

server: export app version only when version is equal or higher than 2

5e02d0e

server: test - only init app version when equal or higher than 2

cc1aebf

rootulp approved these changes Jul 23, 2024

View reviewed changes

rootulp requested a review from cmwaters July 23, 2024 14:09

najeal added 2 commits July 23, 2024 18:45

server: rename variable from utest to test

a90f84a

server: move const on top of file

813533c

rootulp removed the request for review from staheri14 July 23, 2024 18:10

evan-forbes approved these changes Jul 23, 2024

View reviewed changes

rootulp merged commit 0dc27d4 into celestiaorg:release/v0.46.x-celestia Jul 23, 2024
33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: export app version #408

fix: export app version #408

najeal commented Jul 5, 2024

najeal commented Jul 5, 2024

rootulp Jul 8, 2024

najeal Jul 11, 2024

rootulp commented Jul 12, 2024 •

edited

Loading

najeal commented Jul 12, 2024

cmwaters Jul 19, 2024

najeal Jul 23, 2024

rootulp left a comment

rootulp Jul 23, 2024

rootulp Jul 23, 2024

najeal commented Jul 23, 2024

najeal commented Jul 23, 2024

rootulp commented Jul 23, 2024

rootulp commented Jul 23, 2024

fix: export app version #408

fix: export app version #408

Conversation

najeal commented Jul 5, 2024

Description

Author Checklist

Reviewers Checklist

najeal commented Jul 5, 2024

rootulp Jul 8, 2024

Choose a reason for hiding this comment

najeal Jul 11, 2024

Choose a reason for hiding this comment

rootulp commented Jul 12, 2024 • edited Loading

najeal commented Jul 12, 2024

cmwaters Jul 19, 2024

Choose a reason for hiding this comment

najeal Jul 23, 2024

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

rootulp Jul 23, 2024

Choose a reason for hiding this comment

rootulp Jul 23, 2024

Choose a reason for hiding this comment

najeal commented Jul 23, 2024

najeal commented Jul 23, 2024

rootulp commented Jul 23, 2024

rootulp commented Jul 23, 2024

rootulp commented Jul 12, 2024 •

edited

Loading