-
Notifications
You must be signed in to change notification settings - Fork 26
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
feat(app): rollback many blocks #349
base: main
Are you sure you want to change the base?
Conversation
client/cmd/flags.go
Outdated
cmd.Flags().BoolVar(&cfg.RemoveBlock, "hard", false, "remove last block as well as state") | ||
func bindRollbackFlags(cmd *cobra.Command, cfg *rollbackConfig) { | ||
cmd.Flags().BoolVar(&cfg.RemoveBlock, "hard", false, "remove Cosmos & CometBFT blocks as well as states") | ||
cmd.Flags().BoolVar(&cfg.RemoveBlock, "rollback-evm", false, "remove EVM blocks as well") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the evm rollback is using the same config param as the hard, is it intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we don't support rollback-evm, I think we can remove this flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reflected in e78cb90
client/cmd/rollback.go
Outdated
|
||
// newRollbackCmd returns a new cobra command that rolls back one block of the story consensus client. | ||
func newRollbackCmd(appCreateFunc func(context.Context, app.Config) *app.App) *cobra.Command { | ||
rollbackCfg := rollbackConfig{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use the similar pattern that use rollbackConfig.DefaultConfig()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 5cc6dd4
client/app/rollback.go
Outdated
return err | ||
} else if latestBlock.BeaconRoot() == nil { | ||
return errors.New("cannot rollback EVM with nil beacon root", "height", latestHeigth) | ||
if !rollbackEVM || rollbackHeights <= 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need || rollbackHeights <= 1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reflected in e78cb90
client/app/rollback.go
Outdated
} | ||
|
||
db, err := dbm.NewDB("application", cfg.BackendType(), cfg.DataDir()) | ||
engineCl, err := newEngineClient(ctx, appCfg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think based on what we discussed with Omni and this PR, we cannot support EVM rollback and hence this part of the code was removed by Omni
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reflected in e78cb90
a246b15
to
e78cb90
Compare
func bindRollbackFlags(cmd *cobra.Command, cfg *config.Config) { | ||
cmd.Flags().BoolVar(&cfg.RemoveBlock, "hard", false, "remove last block as well as state") | ||
func bindRollbackFlags(cmd *cobra.Command, cfg *config.RollbackConfig) { | ||
cmd.Flags().BoolVar(&cfg.RemoveBlock, "hard", false, "remove Cosmos & CometBFT blocks as well as states") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't need to support RemoveBlock flag since as we discussed, removeBlock currently won't work in our case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a parameter accepted by Rollback
exposed in CometBFT. We could set it to false
and not take the parameter externally if that's what you mean.
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 - X. | ||
The application also rolls back to height n - X. If --hard=true, the block |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to update the description here. --hard is not supported any more.
rollback multiple Cosmos App blocks and corresponding CometBFT states
issue: none