fix: export app-version #3650

najeal · 2024-07-02T15:07:45Z

Related to #3472

Overview

ParamStoreKeyVersionParams key was never set in paramStore.
The changes forces storage of app-version and fix test for exporting version 2.

coderabbitai · 2024-07-02T15:07:52Z

Walkthrough

The application's version initialization process has been updated. The InitializeAppVersion method was removed from app/app.go. In ExportAppStateAndValidators function within app/export.go, the version initialization is now handled differently depending on block height and upgrade version. Additionally, relevant test assertions in export_test.go have been uncommented.

Changes

Files	Change Summary
app/app.go	Removed the `InitializeAppVersion` method that handled setting the application version.
app/export.go	Updated logic in `ExportAppStateAndValidators` for initializing the app version based on block height and upgrade version.
app/test/export_test.go	Uncommented an assertion related to `ConsensusParams.Version.AppVersion` in the `TestExportAppStateAndValidators` function.

Sequence Diagrams

sequenceDiagram
    participant User
    participant App
    participant ParameterStore

    User->>App: Call ExportAppStateAndValidators()
    App->>ParameterStore: Check block height & upgrade version
    ParameterStore-->>App: Return block height & upgrade version
    alt Block height and upgrade version check
        App->>App: SetInitialAppVersionInConsensusParams()
        App->>App: SetAppVersion()
    end
    App-->>User: Return exported app state and validators

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

rootulp

This looks good to me, thanks! I plan on manually testing this PR by stop + starting a node before and after the upgrade height b/c persisting the app version has a lot of edge cases.

rootulp

Not sure how to explain this b/c the unit test seems to pass but when I manually tried this:

./scrips/single-node.sh

# wait until after block height 3 then stop the node

celestia-appd export

the resulting JSON does not populate the app version in consensus params:

		"version": {

		}

najeal · 2024-07-02T15:29:26Z

@rootulp yes I need to open a PR an cosmos-sdk side to close the issue.
This PR partially resolves the issue as we are now providing an expected v1 or v2 version value. Previously only v1 was working regarding the tests.
But cosmos-sdk still does not populate this value.

najeal · 2024-07-02T15:32:30Z

In addition to my previous anwer. This is where Version is not populated:
https://github.com/celestiaorg/cosmos-sdk/blob/83c372bc533d09c332c9dc0ee422ee8d087443b1/server/export.go#L83-L97

coderabbitai

Actionable comments posted: 1

coderabbitai · 2024-07-02T15:33:31Z

app/export.go

+	versionToInit := v1
+	if app.LastBlockHeight() >= app.upgradeHeightV2 {
+		versionToInit = v2
+	}
+	app.SetInitialAppVersionInConsensusParams(ctx, versionToInit)
+	app.SetAppVersion(ctx, versionToInit)


Tip
Codebase Verification

The new logic for app version initialization is not handled in cmd/celestia-appd/cmd/root.go.

Ensure that the logic for initializing the app version based on block height and upgrade version is correctly integrated where ExportAppStateAndValidators is called.

Specifically, update the cmd/celestia-appd/cmd/root.go file to include the necessary logic before calling ExportAppStateAndValidators.

Analysis chain

LGTM! But verify the function usage in the codebase.

The code changes are approved.

However, ensure that all function calls to ExportAppStateAndValidators correctly handle the new logic.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash # Description: Verify all function calls to `ExportAppStateAndValidators` correctly handle the new logic. # Test: Search for the function usage. Expect: Correct handling of the new logic. rg --type go $'ExportAppStateAndValidators'

Length of output: 679

Script:

#!/bin/bash # Extract the relevant portion of cmd/celestia-appd/cmd/root.go to verify the correct handling of the new logic. rg -A 10 -B 10 'ExportAppStateAndValidators' cmd/celestia-appd/cmd/root.go

Length of output: 779

rootulp · 2024-07-02T17:44:32Z

Thanks! I think we'll have to make a PR to https://github.com/celestiaorg/cosmos-sdk first, get it merged, cut a release, bump to that version in celestia-app and then proceed with this PR so will mark as draft for now.

cmwaters · 2024-07-19T10:32:18Z

app/export.go

+	if app.LastBlockHeight() >= app.upgradeHeightV2 {
+		versionToInit = v2
+	}


This won't work if the user hasn't set the appropriate flag. I would prefer trying to read the consensus params to know what the version is

rootulp · 2024-07-24T14:11:56Z

@najeal celestia-app bumped to the version of cosmos-sdk with your fix on all 3 supported branches (main, v1.x, and v2.x) so your PR should be unblocked.

najeal · 2024-07-25T11:03:38Z

@rootulp normally, the PR just needs to uncomment the test right now, so my first commit can be deleted.
One problem is, when uncommenting, the test fails whereas testing from using the script single-node.sh works.
It looks like the problem comes from the test state itself, not the feature.
I will investigate a little bit more

najeal · 2024-07-26T00:04:34Z

There is some weird stuff. Checking version from param store after the upgrade: version := testApp.GetAppVersionFromParamStore(ctx) fails. It falls in the ParamStoreKeyVersionParams key not stored logic.
But looking with the debugger, this key is stored during the upgrade with v2 value.

Either I don't initialise the context the way I should or the store is reset during the commit of the upgrade (see: cosmos-sdk/store/rootmulti.store.go loadVersion(ver int64, upgrades *types.StoreUpgrades) function). What do you think?
I suspect new param store to be created without old values moved in.

najeal requested a review from a team as a code owner July 2, 2024 15:07

najeal requested review from rootulp and cmwaters and removed request for a team July 2, 2024 15:07

najeal changed the title ~~fix export app-version~~ fix: export app-version Jul 2, 2024

celestia-bot requested a review from a team July 2, 2024 15:08

github-actions bot added the external label Jul 2, 2024

najeal force-pushed the fix-export-appversion branch from d3b3c42 to e353ac7 Compare July 2, 2024 15:09

rootulp assigned najeal Jul 2, 2024

rootulp previously approved these changes Jul 2, 2024

View reviewed changes

rootulp reviewed Jul 2, 2024

View reviewed changes

rootulp self-requested a review July 2, 2024 15:19

coderabbitai bot reviewed Jul 2, 2024

View reviewed changes

rootulp marked this pull request as draft July 2, 2024 17:44

cmwaters reviewed Jul 19, 2024

View reviewed changes

HEGred approved these changes Jul 21, 2024

View reviewed changes

najeal added 2 commits July 25, 2024 11:17

delete old comment

66c82ec

Merge remote-tracking branch 'upstream/main' into fix-export-appversion

0ffc469

check app version from param store

7dfbbb1

najeal dismissed rootulp’s stale review via 7dfbbb1 July 25, 2024 23:53

najeal force-pushed the fix-export-appversion branch from e353ac7 to 7dfbbb1 Compare July 25, 2024 23:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: export app-version #3650

fix: export app-version #3650

najeal commented Jul 2, 2024

coderabbitai bot commented Jul 2, 2024 •

edited

Loading

Walkthrough

Changes

Sequence Diagrams

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

rootulp left a comment

rootulp left a comment •

edited

Loading

najeal commented Jul 2, 2024 •

edited

Loading

najeal commented Jul 2, 2024

coderabbitai bot left a comment

coderabbitai bot Jul 2, 2024

rootulp commented Jul 2, 2024

cmwaters Jul 19, 2024

rootulp commented Jul 24, 2024

najeal commented Jul 25, 2024

najeal commented Jul 26, 2024

fix: export app-version #3650

Are you sure you want to change the base?

fix: export app-version #3650

Conversation

najeal commented Jul 2, 2024

Overview

coderabbitai bot commented Jul 2, 2024 • edited Loading

Walkthrough

Changes

Sequence Diagrams

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

rootulp left a comment

Choose a reason for hiding this comment

rootulp left a comment • edited Loading

Choose a reason for hiding this comment

najeal commented Jul 2, 2024 • edited Loading

najeal commented Jul 2, 2024

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Jul 2, 2024

Choose a reason for hiding this comment

rootulp commented Jul 2, 2024

cmwaters Jul 19, 2024

Choose a reason for hiding this comment

rootulp commented Jul 24, 2024

najeal commented Jul 25, 2024

najeal commented Jul 26, 2024

coderabbitai bot commented Jul 2, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

rootulp left a comment •

edited

Loading

najeal commented Jul 2, 2024 •

edited

Loading