cli: allow node operator to rollback last state #7033
Conversation
cmwaters
requested review from
creachadair,
ebuchman,
tychoish and
williambanfield
as code owners
cmwaters
added
the
S:backport-to-v0.34.x
|
Thank you for this PR. And I think only rolling back 1 block is fine for 90% of use cases, so let's just do that. |
This is a very good point. I will look at SDK rollback, but since the SDK embeds tendermint, it will need to expose a command to revert it's own app state, then call into Tendermint to revert it's last block. |
cmd/tendermint/commands/rollback.go
Outdated
|
|
||
| // rollbackState takes a state at height n and overwrites it with the state | ||
| // at height n - 1. Note state here refers to tendermint state not application state. | ||
| func rollbackState(bs state.BlockStore, ss state.Store) error { |
There was a problem hiding this comment.
Could you make this function public?
And maybe place it in a different package?
In any case, an sdk command should be able to call this and then do it's own rollback
There was a problem hiding this comment.
+1 to making this be somewhere else and maybe testing writing some more unittests.
| "github.com/tendermint/tendermint/version" | ||
| ) | ||
|
|
||
| func TestRollback(t *testing.T) { |
There was a problem hiding this comment.
is there a non-happy path test that we could write?
There was a problem hiding this comment.
Sure I'll try think of a few. I think the main non-happy paths are a matter of failure to get data i.e. blocks, state or validator sets. I don't think there's too much of a problem with the atomicity of operations (the write only happens once at the end).
cmd/tendermint/commands/rollback.go
Outdated
|
|
||
| // rollbackState takes a state at height n and overwrites it with the state | ||
| // at height n - 1. Note state here refers to tendermint state not application state. | ||
| func rollbackState(bs state.BlockStore, ss state.Store) error { |
There was a problem hiding this comment.
+1 to making this be somewhere else and maybe testing writing some more unittests.
Makes sense. I will make a public |
cmwaters
force-pushed
the
callum/rollback
branch
from
f4f7f08
to
49ee521
Compare
Thank you! Otherwise, it looks great 👍 |
libs/cli/rollback.go
Outdated
| @@ -0,0 +1,34 @@ | |||
| package cli | |||
There was a problem hiding this comment.
the libs/cli package is really just helpers for managing/using cobra, and so this is sort of a weird case.
I think this should end up in either:
nodecmd/tendermint/commands
There was a problem hiding this comment.
I don't think putting it in node makes sense because that should be node-only stuff. I initially had it in cmd/tendermint/commands but this feels also strange because it's not a command, rather some tooling function. Maybe we just have to face that there's probably no idea home for it and leave it in cmd/tendermint/commands.
(cherry picked from commit 4ca130d) # Conflicts: # CHANGELOG_PENDING.md # cmd/tendermint/commands/reindex_event.go # cmd/tendermint/main.go # state/state.go # types/time/rollback.go # types/time/rollback_test.go
(cherry picked from commit 4ca130d) # Conflicts: # CHANGELOG_PENDING.md
Closes: #3845
Adds a simple
tendermint rollbackcommand which takes a tendermint state at height n and rebuilds the tendermint state at height n-1, overriding the latter state. In order to recover from diverging app hashes, applications need to also be able to rollback their last state as well. This does not remove the block at height n. Upon restart Tendermint will reexecute the transactions at height n against the application.I thought about adding a
--heightflag which would allow operators to rollback tendermint state to a selected height with which to replay all the transactions again but I think hard coding it to the previous height is sufficientcc @ethanfrey