Tendermint: store/store.BlockStore.SaveBlock is not atomic

[dshulyak]

Tendermint version (use tendermint version or git rev-parse --verify HEAD if installed from source):

Tip of the master branch https://github.com/tendermint/tendermint/tree/1d16e39c0ea72a1e337662c35e4d50641a3df5fa

In this method:
https://github.com/tendermint/tendermint/blob/1d16e39c0ea72a1e337662c35e4d50641a3df5fa/store/store.go#L419-L472

Parts of the block, block metadata, commited hash and precommited hash are written to the database, and the last write executed with Sync=true. It seems to me that the intent is to fully flush all these items on to the disk, but Sync=true doesn't guarantee that previous writes will be flushed.

This is definitely not the case with goleveldb, because it rotates journals that are used for write, not sure about other used data stores.

As a result here:
https://github.com/tendermint/tendermint/blob/1d16e39c0ea72a1e337662c35e4d50641a3df5fa/consensus/state.go#L1528-L1533

Height might be updated, because metadata is flushed but actual block parts will be missing.

It does seem that this is an easy fix with batch writer, but if it is already handled in some other way feel free to close.

[alexanderbez]

Your reasoning seems to line up and make sense. I also agree here using a database batch object to write all the data is the correct approach. Does this seem reasonable @erikgrinaker?

[erikgrinaker]

Batches don't necessarily map onto database transactions, so it depends on the underlying database backend whether they're atomic or not. Ideally tm-db should expose a transaction interface that guarantees atomicity (and preferably the other ACID properties), but barring that batches would be the next-best thing.

[githubsands]

Went ahead and jump started this issue - currently is a draft/WIP. Let me know if you guys have any input.

6018 && https://github.com/tendermint/tm-db/pull/147