Should we implement the rewind feature in tendermint?

[bradyjoestar]

I was reading the code of go-ethereum and found that it had supported the rewind feature.

// SetHead rewinds the local chain to a new head. In the case of headers, everything
// above the new head will be deleted and the new one set. In the case of blocks
// though, the head may be further rewound if block bodies are missing (non-archive
// nodes after a fast sync).
func (bc *BlockChain) SetHead(head uint64) error {
	log.Warn("Rewinding blockchain", "target", head)

	bc.mu.Lock()
	defer bc.mu.Unlock()

	// Rewind the header chain, deleting all block bodies until then
	delFn := func(hash common.Hash, num uint64) {
		DeleteBody(bc.db, hash, num)
	}
	bc.hc.SetHead(head, delFn)
	currentHeader := bc.hc.CurrentHeader()

	// Clear out any stale content from the caches
	bc.bodyCache.Purge()
	bc.bodyRLPCache.Purge()
	bc.blockCache.Purge()
	bc.futureBlocks.Purge()

	// Rewind the block chain, ensuring we don't end up with a stateless head block
	if currentBlock := bc.CurrentBlock(); currentBlock != nil && currentHeader.Number.Uint64() < currentBlock.NumberU64() {
		bc.currentBlock.Store(bc.GetBlock(currentHeader.Hash(), currentHeader.Number.Uint64()))
	}
	if currentBlock := bc.CurrentBlock(); currentBlock != nil {
		if _, err := state.New(currentBlock.Root(), bc.stateCache); err != nil {
			// Rewound state missing, rolled back to before pivot, reset to genesis
			bc.currentBlock.Store(bc.genesisBlock)
		}
	}
	// Rewind the fast block in a simpleton way to the target head
	if currentFastBlock := bc.CurrentFastBlock(); currentFastBlock != nil && currentHeader.Number.Uint64() < currentFastBlock.NumberU64() {
		bc.currentFastBlock.Store(bc.GetBlock(currentHeader.Hash(), currentHeader.Number.Uint64()))
	}
	// If either blocks reached nil, reset to the genesis state
	if currentBlock := bc.CurrentBlock(); currentBlock == nil {
		bc.currentBlock.Store(bc.genesisBlock)
	}
	if currentFastBlock := bc.CurrentFastBlock(); currentFastBlock == nil {
		bc.currentFastBlock.Store(bc.genesisBlock)
	}
	currentBlock := bc.CurrentBlock()
	currentFastBlock := bc.CurrentFastBlock()
	if err := WriteHeadBlockHash(bc.db, currentBlock.Hash()); err != nil {
		log.Crit("Failed to reset head full block", "err", err)
	}
	if err := WriteHeadFastBlockHash(bc.db, currentFastBlock.Hash()); err != nil {
		log.Crit("Failed to reset head fast block", "err", err)
	}
	return bc.loadLastState()
}

Should we implement a similar feature in the tendermint?

If needed, I will write an ADR Documents based my prior work on it. Maybe it will take some time. :)

[melekes]

Why we'd want that?

[bradyjoestar]

Why we'd want that?

Once a block is committed in Tendermint, it's final. Unlike PoW chains where you have many branches and you pick the one with the most "work" (which sometimes requires you to rewind and apply another branch), in Tendermint there's a single chain of blocks.

If we are running in a production enviroment, at a moment there is a thorny problems that happened and restart the node didnt work.

Maybe its better to rewind to previous blocks to keep system up and running. Then we have enough time to fix this bug.

[mattkanwisher]

Biggest reason, is we have seen the chain halt for consensus problems with App Hash. Once a node has a bad app hash, there is no simple way for it to rejoin the chain without a complete resync. This can create large downtimes. Also since tendermint isn't maintaining backwards compatibility, checkpoints of the chain have to periodically stored off chain, to even restore a node from scratch, cause you can't possible replay the chain after a tendermint upgrade.

[ebuchman]

Interesting. I could see us supporting this from a debuggability/maintenance perspective. How would it work? tendermint unsafe_revert_block <height> ? Might be tricky to ensure it works with the app too

[bradyjoestar]

I add two flag into tendermint node

RollbackFlag bool `mapstructure:"rollback_data"` //false

RollbackHeight int `mapstructure:"rollback_height"`

And if RollabackFlag = true it will revert.

It is a good idea to create a new command unsafe_revert_block and run tendermint unsafe_revert_block <height>. We should access the blockstore.db and state.db in it.

I use etheremint(0.5.3) as ABCI app and it passed. As @melekes mentioned, it is easy for those PoW chains to rewind like ethereum, but it need some work on data processing for another APP.

[bradyjoestar]

In testcase, I will use kvstore as ABCIAPP to judge whether revert works.

If we revert a node from height 1200 to height 1000, it will replay to 1000 when it restart and then use blockpool to get blocks from other nodes to catch up them.

[ebuchman]

What about the priv_validator_state.json? it could cause double signing.

[bradyjoestar]

reset it to the following state:

filePv := privval.LoadOrGenFilePV(config.PrivValidatorFile())
filePv.LastHeight = rollbackHeight
filePv.LastRound = 0
filePv.LastStep = 0
filePv.LastSignature = nil
filePv.LastSignBytes = nil
filePv.Save()

[ebuchman]

This could still cause double signing though. I think we might want two cases.

Assuming rollbackHeight is the height we roll back to, we may want options to set filePv.LastHeight = rollbackHeight, so we can double sign if we're cool with that, and filePv.LastHeight = rollbackHeight + 1 so we can't.

Or maybe we can make the rollback command ensure that we don't process evidence from the next height? It's a bit weird. If your app handles evidence, this could get kind of tricky.

[bradyjoestar]

I'm reading the consensus code to try to solve this problem.
There may be three situations in rollback, take a cluster of 7 nodes as a example and they are all in the height 1200:

• rollback a node to 1000
• rollback 3 nodes to 1000
• rollback 7 nodes to 1000

I only considered the situation 1&3 previously. But if we rollback 3 nodes it may casue double signing.

[bradyjoestar]

It's a good idea to support two different case.

Take the example above:

• if we rollback 7 nodes from 1200 to 1000
  we should reset the filePv.LastHeight = 1000 and all votes and datas above height 1000 will be discarded include walMessages in the walFile. All nodes are at the same line.
• If we rollback a node from 1200 to 1000
  It may doesnt mather whether we change priv_validator_state.json, when the rollbacking node started, the cluster may have gone to height 1300 , so double signing doesnt exist.
• rollback 3 nodes to from 1200 to 1000:
  we'd better not change priv_validator_state.json, I thought. If we rollbacked node, the cluster will stay　height 1200 unless we restart the rollbacking node. This is similar as that three node crashed at height 1200. If the rollbacking node catch up with the current height, the priv_validator_state.json will used to prevent double signing?

[mattkanwisher]

Any sample code to try out? Trying to handle this situation right now

[bradyjoestar]

@mattkanwisher I had wrote some code for test and it worked well with tendermint0.27.4

Usage: tendermint node --proxy_app=kvstore --rollback_data=true --rollback_height=$rollbackHeight

master...bradyjoestar:revert

Catuion: it wil directly modify data so be careful!!

Wal file error could be skipped for sample.

I think it will take some time to create a pull request because it is really a dangerous operation.

[bradyjoestar]

@mattkanwisher
I created a pull request and there are three scripts in it as test case.
#3239

[ebuchman]

What if we do this as an independent tool for now rather than making it available in the tendermint command?

[ebuchman]

We shouldn't clutter the tendermint node command line options with things like this. It should be uncommon enough, and certainly dangerous enough, to warrant installing a separate binary.

[bradyjoestar]

you're right. Considering of security, its better to create a independent tool.
I would change it.