-
Notifications
You must be signed in to change notification settings - Fork 54
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(rollback): enable rollback support #656
base: main
Are you sure you want to change the base?
Conversation
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.
Seems to do the opposite of what the issue says
I don't think rollback is going to work properly anyway, in general, because UpdateStateAfterCommit only changes ConsensusParams or RollappParams if there is some response from the block.
So say you have heights 1,2,3
On height 2 the consensus params and rollapp params are changed
Then rollback from 3 -> 1 will not have the right values in state afterwards
server/commands/rollback.go
Outdated
fmt.Printf("Pruning store from height %d to %d\n", heightInt+1, blockManager.State.Height()+1) | ||
|
||
pruned, err := blockManager.Store.PruneHeights(uint64(heightInt+1), blockManager.State.Height()+1, ctx.Logger) | ||
if err != nil { | ||
return fmt.Errorf("pruning: %w", err) | ||
} | ||
|
||
_, err = blockManager.Store.LoadBlock(blockManager.State.Height() + 2) | ||
if err == nil { | ||
extraPruned, err := blockManager.Store.PruneHeights(blockManager.State.Height()+1, blockManager.State.Height()+2, ctx.Logger) | ||
if err != nil { | ||
return fmt.Errorf("pruning: %w", err) | ||
} | ||
pruned += extraPruned | ||
} | ||
|
||
fmt.Println("Pruned blocks:", pruned) |
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.
can dry it a lot if you check the state.height+2 first, and then do PruneHeights once (with possible+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.
done
server/commands/rollback.go
Outdated
@@ -74,19 +74,49 @@ func RollbackCmd(appCreator types.AppCreator) *cobra.Command { | |||
return fmt.Errorf("app rollback to specific height: %w", err) | |||
} | |||
|
|||
fmt.Printf("RollApp state moved back to height %d successfully.\n", heightInt) | |||
|
|||
skipStorePruning := ctx.Viper.GetBool(flags.FlagLogLevel) |
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.
wrong 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.
fixed
server/commands/rollback.go
Outdated
|
||
skipStorePruning := ctx.Viper.GetBool(flags.FlagLogLevel) | ||
|
||
if !skipStorePruning { |
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.
if !skipStorePruning { | |
if skipStorePruning { | |
return | |
} |
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.
fixed
server/commands/rollback.go
Outdated
return fmt.Errorf("pruning: %w", err) | ||
} | ||
|
||
_, err = blockManager.Store.LoadBlock(blockManager.State.Height() + 2) |
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.
not sure why it's needed
dymint store shouldn't be accessed while rollback in progress
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.
to check if an extra block its been produced but not applied yet to remove it
dymint stores all block responses and every response includes consensus and rollapp params for height. UpdateStateFromApp replaces dymint state cons and rollapp params from the responses of rollback height. |
Description
Closes #623
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.
PR review checkboxes:
I have...
Unreleased
section inCHANGELOG.md
godoc
commentsSDK Checklist
map
time.Now()
sendCoin
and notSendCoins
Full security checklist here
For Reviewer:
After reviewer approval: