Skip to content
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

[Bug]: halt-height is not deterministic #16638

Closed
yihuang opened this issue Jun 21, 2023 · 0 comments · Fixed by #16639
Closed

[Bug]: halt-height is not deterministic #16638

yihuang opened this issue Jun 21, 2023 · 0 comments · Fixed by #16639
Labels

Comments

@yihuang
Copy link
Collaborator

yihuang commented Jun 21, 2023

Summary of Bug

Currently halt-height will send a signal to current process, the signal is handled asynchronously, which means there could be more blocks being committed before the signal handler is trigger and stop the consensus state machine, especially in catching up phase where blocks are executed very fast.

Sometimes I use halt-height feature to restore node state to a specific height, I first restore state with a snapshot before that height, then start it with --halt-height <target height>, but with current behavior, it's not guaranteed to stop at the exact height.

Version

Steps to Reproduce

Suggestion

  • Add a check in BaseApp.BeginBlocker, do a panic if the height is to be halted, this will ensure the future blocks won't be executed succesfully.
 func (app *BaseApp) BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock) abci.ResponseBeginBlock {
+       var halt bool
+       switch {
+       case app.haltHeight > 0 && uint64(req.Header.Height) > app.haltHeight:
+               halt = true
+
+       case app.haltTime > 0 && req.Header.Time.Unix() > int64(app.haltTime):
+               halt = true
+       }
+
+       if halt {
+               app.Logger().Info("halting node per configuration", "height", app.haltHeight, "time", app.haltTime)
+               if err := app.Close(); err != nil {
+                       app.Logger().Info("close application failed", "error", err)
+               }
+               panic("halt application")
+       }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant