fix: add missing rollback command #23618
Conversation
The sentence "Recently we have identified the following key target user groups:" was accidentally repeated. This commit removes the duplicate to improve readability.
It should be readonly instead of "radonly"
📝 WalkthroughWalkthroughThis pull request rephrases and restructures an RFC document to clarify the identification of key target user groups for the Cosmos SDK. It also introduces a new rollback command ( Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as Command Line Interface
participant CBS as CometBFT Server
U->>CLI: Execute "comet rollback" command
CLI->>CBS: Retrieve CLICommands (includes RollbackStateCmd)
CBS-->>CLI: Provide rollback command execution
Assessment against linked issues
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package sqlite3: could not load export data: no export data for "github.com/bvinc/go-sqlite-lite/sqlite3"" Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
C:server/v2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/rfc/rfc-003-crosslang.md (1)
12-15: Add blank lines around the list for better readability.According to markdown best practices, lists should be surrounded by blank lines.
Apply this diff to improve formatting:
have been underway since early 2023. Recently, we have identified the following key target user groups: 1. projects that want to primarily target a single programming language and virtual machine environment besides Golang but who still want to use Cosmos SDK internals for consensus and storage 2. projects that want to integrate multiple programming languages and virtual machine environments into an integrated application While these two user groups may have substantially different needs,🧰 Tools
🪛 LanguageTool
[style] ~14-~14: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... consensus and storage 2. projects that want to integrate multiple programming language...(REP_WANT_TO_VB)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/rfc/rfc-003-crosslang.md(2 hunks)server/v2/cometbft/server.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.go`: Review the Golang code for conformity with the Ub...
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/server.go
`**/*.md`: "Assess the documentation for misspellings, gramm...
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
docs/rfc/rfc-003-crosslang.md
🪛 markdownlint-cli2 (0.17.2)
docs/rfc/rfc-003-crosslang.md
173-173: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-simapp-v2
- GitHub Check: test-integration
- GitHub Check: test-system-v2
- GitHub Check: build (arm64)
- GitHub Check: build (amd64)
- GitHub Check: Analyze
- GitHub Check: golangci-lint
- GitHub Check: markdown-link-check
- GitHub Check: Summary
🔇 Additional comments (1)
server/v2/cometbft/server.go (1)
356-356: LGTM! Addition of rollback command.The addition of
cmtcmd.RollbackStateCmdto the CLI commands is appropriate and aligns with the PR objectives to add the missing rollback command.Let's verify the command's integration:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the rollback command is properly integrated # Test: Check if the command is properly imported and accessible rg -A 2 "RollbackStateCmd"Length of output: 175
Validation: Rollback Command Integration Verified
The shell script output confirms thatcmtcmd.RollbackStateCmdis present in the codebase in the expected location, indicating that the rollback command has been properly integrated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
CHANGELOG.md (4)
4-11: Ensure changelog principles are followed consistentlyThe guiding principles section provides good guidelines for changelog entries. However, some entries in the changelog could be improved to better follow these principles, particularly around being human-readable and having clear version linkages.
Consider adding examples for each principle to make them more concrete and easier to follow.
12-23: Usage instructions are clear but could be enhancedThe usage instructions are clear but could benefit from:
- Examples of properly formatted entries
- Common pitfalls to avoid
- Links to example PRs that follow the format well
2780-2784: Important release notice needs formatting improvementsThe Stargate release notice should be formatted more prominently, perhaps using:
- Headers
- Bullet points for key changes
- Links to migration guides
- v0.40.0, known as the Stargate release of the Cosmos SDK, is one of the largest releases - of the Cosmos SDK since launch. Please read through this changelog and [release notes](https://github.com/cosmos/cosmos-sdk/blob/v0.40.0/RELEASE_NOTES.md) to make - sure you are aware of any relevant breaking changes. + # v0.40.0 - Stargate Release + + This is one of the largest releases of the Cosmos SDK since launch, introducing several major features and improvements: + + ## Key Changes + - Feature A + - Feature B + + ## Migration Guide + Please carefully review the [release notes](https://github.com/cosmos/cosmos-sdk/blob/v0.40.0/RELEASE_NOTES.md) for breaking changes and migration steps.
3155-3157: Previous releases section needs improvementThe previous releases section should:
- Include a brief summary of major changes in each previous version
- Maintain consistent formatting with the rest of the changelog
- Consider archiving very old changes to a separate file
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.md`: "Assess the documentation for misspellings, gramm...
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
CHANGELOG.md
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-x-accounts-multisig
- GitHub Check: test-x-accounts
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: runtime-v2
- GitHub Check: test-sim-nondeterminism
- GitHub Check: cometbft
- GitHub Check: test-integration
- GitHub Check: Check go mod tidy
- GitHub Check: build (arm64)
- GitHub Check: Analyze
- GitHub Check: build (amd64)
- GitHub Check: golangci-lint
- GitHub Check: markdown-link-check
- GitHub Check: Summary
🔇 Additional comments (3)
CHANGELOG.md (3)
1-3: LGTM: Clear and concise introductionThe introduction provides a clear overview of the changelog's purpose and scope.
24-34: Well-defined stanza typesThe stanza types are well-defined and cover the major categories of changes. The descriptions are clear and help maintainers properly categorize their changes.
1558-1559: Security advisory notice needs more detailsThe security advisory notice should include:
- Severity level
- Affected versions
- Mitigation steps
- Timeline for fixes
Consider enhancing this section to help users better assess and respond to the security issue.
|
Thanks! That looks good to me however I think there's a bug in CometBFT: https://github.com/cometbft/cometbft/blob/v1.0.1/cmd/cometbft/commands/reset.go#L40-L43 Instead of using the global: https://github.com/cometbft/cometbft/blob/main/cmd/cometbft/commands/rollback.go#L35-L38 EDIT: While I still believe they should parse the config instead of reading the global, their rollback command isn't complete, as it rollsback only on their side. The app needs to be rolled back as well: #23618 (review) |
There was a problem hiding this comment.
You actually need to rollback the app as well, check this: https://github.com/cosmos/cosmos-sdk/blob/release/v0.52.x/server/rollback.go#L42-L44.
My bad about the previous message, we shouldn't just use the cometbft command directly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/v2/CHANGELOG.md (1)
25-26: Changelog Entry DetailThe entry for the rollback command is added in the correct "Unreleased" section and follows the expected issue reference format. Consider providing a tad more detail (if applicable) to clarify that this fix addresses the missing rollback command issue (#23618) and improves rollback functionality. This extra context can help users quickly grasp the purpose of the change.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/v2/CHANGELOG.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.md`: "Assess the documentation for misspellings, gramm...
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
server/v2/CHANGELOG.md
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-simapp-v2
- GitHub Check: test-integration
- GitHub Check: test-system-v2
- GitHub Check: cometbft
- GitHub Check: build (arm64)
- GitHub Check: build (amd64)
- GitHub Check: Analyze
- GitHub Check: markdown-link-check
- GitHub Check: golangci-lint
- GitHub Check: Summary
Thanks for your information and let me update it |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/v2/cometbft/commands.go(2 hunks)server/v2/cometbft/server.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/v2/cometbft/server.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Golang code for conformity with the Ub...
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/commands.go
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: tests (00)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: markdown-link-check
- GitHub Check: Summary
🔇 Additional comments (1)
server/v2/cometbft/commands.go (1)
10-10: Import usage is valid and clear.
No issues found here. The import alias “cmtcmd” is used in the rollback logic below, so this addition is appropriate.
|
|
||
| func (s *CometBFTServer[T]) RollbackStateCmd() *cobra.Command { | ||
| var removeBlock bool | ||
|
|
||
| cmd := &cobra.Command{ | ||
| Use: "rollback", | ||
| Short: "rollback Cosmos SDK and CometBFT state by one height", | ||
| Long: ` | ||
| A state rollback is performed to recover from an incorrect application state transition, | ||
| when CometBFT has persisted an incorrect app hash and is thus unable to make | ||
| progress. Rollback overwrites a state at height n with the state at height n - 1. | ||
| The application also rolls back to height n - 1. No blocks are removed, so upon | ||
| restarting CometBFT the transactions in block n will be re-executed against the | ||
| application. | ||
| `, | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| cfg := client.GetConfigFromCmd(cmd) | ||
| // rollback CometBFT state | ||
| height, hash, err := cmtcmd.RollbackState(cfg, removeBlock) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to rollback CometBFT state: %w", err) | ||
| } | ||
| // rollback the store | ||
| // if err := s.store.RollbackToVersion(height); err != nil { | ||
| // return fmt.Errorf("failed to rollback to version: %w", err) | ||
| // } | ||
|
|
||
| fmt.Printf("Rolled back state to height %d and hash %X\n", height, hash) | ||
| return nil | ||
| }, | ||
| } | ||
|
|
||
| cmd.Flags().BoolVar(&removeBlock, "hard", false, "remove last block as well as state") | ||
| return cmd | ||
| } |
There was a problem hiding this comment.
Rollback logic is incomplete.
The code’s description (“rollback Cosmos SDK and CometBFT state”) contradicts the fact that the application store rollback is commented out. This can lead to inconsistencies where CometBFT state is rolled back, but the Cosmos SDK application state remains at the newer height. Please either remove the lines referencing application rollback from the description or fully implement the store rollback to avoid leaving the system in a partially rolled-back state.
Below is a possible fix to re-enable the store rollback. Adjust as needed to ensure full coordination between CometBFT and Cosmos SDK state:
- // if err := s.store.RollbackToVersion(height); err != nil {
- // return fmt.Errorf("failed to rollback to version: %w", err)
- // }
+ if err := s.store.RollbackToVersion(height); err != nil {
+ return fmt.Errorf("failed to rollback to version: %w", err)
+ }📝 Committable 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *CometBFTServer[T]) RollbackStateCmd() *cobra.Command { | |
| var removeBlock bool | |
| cmd := &cobra.Command{ | |
| Use: "rollback", | |
| Short: "rollback Cosmos SDK and CometBFT state by one height", | |
| Long: ` | |
| A state rollback is performed to recover from an incorrect application state transition, | |
| when CometBFT has persisted an incorrect app hash and is thus unable to make | |
| progress. Rollback overwrites a state at height n with the state at height n - 1. | |
| The application also rolls back to height n - 1. No blocks are removed, so upon | |
| restarting CometBFT the transactions in block n will be re-executed against the | |
| application. | |
| `, | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| cfg := client.GetConfigFromCmd(cmd) | |
| // rollback CometBFT state | |
| height, hash, err := cmtcmd.RollbackState(cfg, removeBlock) | |
| if err != nil { | |
| return fmt.Errorf("failed to rollback CometBFT state: %w", err) | |
| } | |
| // rollback the store | |
| // if err := s.store.RollbackToVersion(height); err != nil { | |
| // return fmt.Errorf("failed to rollback to version: %w", err) | |
| // } | |
| fmt.Printf("Rolled back state to height %d and hash %X\n", height, hash) | |
| return nil | |
| }, | |
| } | |
| cmd.Flags().BoolVar(&removeBlock, "hard", false, "remove last block as well as state") | |
| return cmd | |
| } | |
| func (s *CometBFTServer[T]) RollbackStateCmd() *cobra.Command { | |
| var removeBlock bool | |
| cmd := &cobra.Command{ | |
| Use: "rollback", | |
| Short: "rollback Cosmos SDK and CometBFT state by one height", | |
| Long: ` | |
| A state rollback is performed to recover from an incorrect application state transition, | |
| when CometBFT has persisted an incorrect app hash and is thus unable to make | |
| progress. Rollback overwrites a state at height n with the state at height n - 1. | |
| The application also rolls back to height n - 1. No blocks are removed, so upon | |
| restarting CometBFT the transactions in block n will be re-executed against the | |
| application. | |
| `, | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| cfg := client.GetConfigFromCmd(cmd) | |
| // rollback CometBFT state | |
| height, hash, err := cmtcmd.RollbackState(cfg, removeBlock) | |
| if err != nil { | |
| return fmt.Errorf("failed to rollback CometBFT state: %w", err) | |
| } | |
| // rollback the store | |
| if err := s.store.RollbackToVersion(height); err != nil { | |
| return fmt.Errorf("failed to rollback to version: %w", err) | |
| } | |
| fmt.Printf("Rolled back state to height %d and hash %X\n", height, hash) | |
| return nil | |
| }, | |
| } | |
| cmd.Flags().BoolVar(&removeBlock, "hard", false, "remove last block as well as state") | |
| return cmd | |
| } |
@julienrbrt The rollback command is added as subcommand of comet since it is more easy to get dependencies from there. The current store V2 does not implement RollbackToVersion method. Can anyone help implement it at the store layer? |
update codes to fix #23613
Description
Closes: #XXXX
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit