chore: bump sdk to v1.20.0 #2860
Conversation
WalkthroughThe changes across the codebase reflect a shift in how the application version is managed and retrieved, moving from a protocol version setting to an application version context. The Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- go.mod
- go.sum
Files selected for processing (6)
- app/app.go (2 hunks)
- app/prepare_proposal.go (1 hunks)
- app/process_proposal.go (1 hunks)
- app/square_size.go (1 hunks)
- test/util/malicious/out_of_order_prepare.go (1 hunks)
- x/upgrade/upgrade_test.go (4 hunks)
Additional comments: 4
app/process_proposal.go (1)
- 113-116: The
ProcessProposalfunction has been correctly updated to pass thesdkCtxparameter to theAppVersionmethod, aligning with the changes described in the pull request summary. This ensures that the application version is retrieved in a context-aware manner, which is a key part of the versioning system update. The use ofsdkCtxinGovSquareSizeUpperBoundis also consistent with the new requirement for context-awareness in version retrieval.app/prepare_proposal.go (1)
- 62-68: The changes to the
PrepareProposalmethod reflect the updated call toapp.GetBaseApp().AppVersion(sdkCtx)andapp.GovSquareSizeUpperBound(sdkCtx), which now require thesdkCtxparameter. This aligns with the pull request's goal of making the application version context-aware. It is important to ensure that all other parts of the code that rely on theAppVersionandGovSquareSizeUpperBoundmethods are updated to pass thesdkCtxparameter as well. Additionally, the error handling here uses apanic, which is appropriate for developer errors that should halt the node, but it's crucial to ensure that this behavior is consistent with the rest of the application's error handling strategy.app/app.go (2)
576-580: The code correctly updates the application version using
SetAppVersionduring the upgrade process. However, it is important to ensure that theSetAppVersionmethod is implemented correctly and that it handles the version update in a thread-safe manner, especially if the application is running in a concurrent environment. Additionally, it would be beneficial to verify that theShouldUpgrademethod ofUpgradeKeeperis correctly determining when an upgrade should occur based on the provided height.587-591: The
InitChainerfunction no longer sets the protocol version, which aligns with the summary stating a move away from protocol versioning to application versioning. This change should be carefully reviewed to ensure that it does not have unintended side effects on the initialization process of the blockchain, especially if there are any dependencies on the protocol version elsewhere in the codebase.
| if req.ConsensusParams != nil && req.ConsensusParams.Version != nil { | ||
| app.SetProtocolVersion(req.ConsensusParams.Version.AppVersion) | ||
| } |
There was a problem hiding this comment.
why is this no longer necessary?
There was a problem hiding this comment.
It's done in the SDK instead
There was a problem hiding this comment.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Because this behaviour is logically the responsibility of baseapp not app
There was a problem hiding this comment.
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?
go.mod
Outdated
| @@ -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-rc0 | |||
There was a problem hiding this comment.
[not blocking] why not use an official release of celestiaorg/cosmos-sdk? If rc-0 is just for testing, can we test pre merge instead of post-merge?
There was a problem hiding this comment.
I plan to cut v1.20.0 once I've tested that the app version works as expected. I opened this PR so docker could build the image I need to run the e2e tests (apparently opening them in draft mode doesn't work)
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (6)
- Makefile (1 hunks)
- app/app.go (2 hunks)
- cmd/celestia-appd/cmd/root.go (2 hunks)
- test/e2e/simple_test.go (1 hunks)
- test/e2e/upgrade_test.go (4 hunks)
- x/upgrade/keeper.go (2 hunks)
Files skipped from review due to trivial changes (1)
- cmd/celestia-appd/cmd/root.go
Additional comments: 3
app/app.go (1)
- 588-592: The
InitChainerfunction no longer sets the protocol version based onreq.ConsensusParams.Version.AppVersion. Ensure that this change is intentional and that the protocol version is being set correctly elsewhere in the codebase to avoid issues with version compatibility and consensus.test/e2e/upgrade_test.go (2)
12-17: The addition of the new import statement for
v1 "github.com/celestiaorg/celestia-app/pkg/appconsts/v1"is consistent with the changes described in the pull request summary, which mentions that the application version handling has been updated. This import is necessary for the updated assertion in theTestMajorUpgradeToV2function.136-142: The added case for handling
latestVersionwith a length of 7 characters is likely related to the handling of git commit hashes, which are often abbreviated to 7 characters. This change should be verified to ensure that it aligns with the versioning strategy and that the corresponding docker image tags are indeed using the 7-character commit hash.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- go.mod
- go.sum
Files selected for processing (2)
- app/app.go (3 hunks)
- x/upgrade/upgrade_test.go (4 hunks)
Additional comments: 4
app/app.go (1)
- 3-7: The import of the "fmt" package is added, but it is not clear if it is used elsewhere in the file. If "fmt" is not used, it should be removed to avoid importing unnecessary packages.
x/upgrade/upgrade_test.go (3)
9-15: The import statements are well-organized and follow the Go convention of grouping standard library imports separately from third-party packages. However, ensure that all imported packages are used within the file to avoid unnecessary dependencies.
22-30: The test
TestUpgradeAppVersionhas been updated to check the app version before and after a block is processed. The use ofsdk.Context{}to create a new context for theAppVersionmethod is acceptable in a test environment, but it's important to ensure that this context is correctly initialized with all necessary values in a real-world scenario. The assertions are correctly checking the expected app version before and after the block, which is good for ensuring the upgrade logic is working as intended.42-49: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [42-71]
The setup function
setupTestAppis correctly initializing the application and asserting the initial app version. The use ofrequire.EqualValuesensures that the test will fail immediately if the app version does not match the expected value, which is a good practice for critical checks in tests. However, ensure that thegenesisStateis correctly configured to match the expected initial app version, as this is assumed in the test but not shown in the provided code.
app/app.go
Outdated
| // 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) | ||
| fmt.Println("upgrading to v2", "height", req.Height) | ||
| app.SetAppVersion(ctx, v2.Version) | ||
| } |
There was a problem hiding this comment.
The code introduces a hard-coded check for an upgrade to version 2. While this is acknowledged in the comments as a one-off event, it is important to consider the implications of hard-coding such logic. Future upgrades will require changes to this code, which could be error-prone and not scalable. It's worth considering a more dynamic approach to version management, as suggested in previous comments.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (4)
- Makefile (1 hunks)
- cmd/celestia-appd/cmd/root.go (2 hunks)
- test/e2e/simple_test.go (2 hunks)
- test/e2e/upgrade_test.go (4 hunks)
Files skipped from review due to trivial changes (1)
- Makefile
Additional comments: 3
cmd/celestia-appd/cmd/root.go (2)
4-9: The removal of the "strconv" package and associated parsing code is not shown in the provided hunk. If the import and code for "strconv" were indeed removed, ensure that there are no remaining references to "strconv" in the codebase that would lead to compilation errors.
236-246: The use of
cast.ToInt64(appOpts.Get(UpgradeHeightFlag))directly retrieves the upgrade height value without manual parsing, which simplifies the code. However, ensure that theappOpts.Getmethod is guaranteed to return a value that can be cast to anint64without error. If there's a possibility of a casting error, it should be handled appropriately to prevent runtime panics.test/e2e/upgrade_test.go (1)
- 177-181: The assertion on line 180 checks the app version against
v1.Versionbefore the upgrade. This is a good practice to ensure that the network is initially running on the expected version. However, ensure thatv1.Versionis correctly set to the expected version number before the upgrade.
|
This can now be reviewed. I still need to cut the final v1.20.0 release and update it here |
| 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] |
There was a problem hiding this comment.
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.
| 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] |
|
It looks as though the ledger test is failing. @rootulp, do you think actually downgrading the dependency here: celestiaorg/cosmos-sdk#357 (comment) is what is causing it to fail? |
|
I manually verified Ledger works when testing celestiaorg/cosmos-sdk#357 so I guess it could be a false negative with the test? Let's see if Ledger test passes on CI in #2865 Update: ledger test failed on that PR. I'll investigate |
| 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] |
Overview
This includes the change to persist the app version in state for v2 onwards
I opened this PR so docker could build the image I need to run the e2e tests (apparently opening them in draft mode doesn't work)
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests
Chores