Fix block detail updating

[AtkinsChang]

In ethcore client, update of block detail occurs in two locations: BlockChain::mark_finalized and BlockChain::insert_block
https://github.com/paritytech/parity-ethereum/blob/a9cb5722383cb89ebf38ba4ea2fa2073f15d5f8b/ethcore/src/client/client.rs#L527
https://github.com/paritytech/parity-ethereum/blob/a9cb5722383cb89ebf38ba4ea2fa2073f15d5f8b/ethcore/src/client/client.rs#L536-L539
But in BlockChain::prepare_block_details_update (called by insert_block), parent block detail is read from cache/db, this may cause previous modification of BlockChain::mark_finalized be overwritten.
https://github.com/paritytech/parity-ethereum/blob/a9cb5722383cb89ebf38ba4ea2fa2073f15d5f8b/ethcore/blockchain/src/blockchain.rs#L1345-L1348

testcase is added to reproduce this situation

[parity-cla-bot]

It looks like @AtkinsChang signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[tomusdrw]

I don't think I fully understand the issue.
mark_finalized does update the cache anyway, so prepare_block_details_update should actually read the updated version already.

I'm also not super in favor of the solution, where we special-case the parent block, if that's really an issue isn't there a way to make sure we don't need to do that and rather have the cache in consistent state no matter what order the methods are called in?

[tomusdrw]

please fix the indentation

[AtkinsChang]

fixed in force-pushed commit

[AtkinsChang]

mark_finalized updates the pending_block_details
prepare_block_details_update reads from block_details so the previous is_finalized update is overwritten
try the testcase in second commit 784e159

Handling parent is the special case that it reads data updated previously in the same transaction so I use this.

I can change to another solution like add a 2-layered cache read and replace
https://github.com/paritytech/parity-ethereum/blob/a9cb5722383cb89ebf38ba4ea2fa2073f15d5f8b/ethcore/blockchain/src/blockchain.rs#L1347
with

let mut parent_details = self.db.key_value().read_with_two_layer_cache(db::COL_EXTRA, &self.pending_block_details, &self.block_details, hash)

if you prefer this

[tomusdrw]

mark_finalized updates the pending_block_details
prepare_block_details_update reads from block_details so the previous is_finalized update is overwritten

I see now, thanks for explanations.

The Blockchain API indeed seems very fragile, I feel we can run into more issues if we commit multiple block at once in a single batch (see insert_block) - all the updates to block_details will only end up in pending_block_details cache, not block_details cache, so we will just keep overwriting the same stuff.

Imagine we are importing a A -> B and A -> B' in single batch, we should end up with two children entries in A, but we will only have one.

I think I'm more inclined to the double cache approach, and go through all methods that work on a batch to make sure we are always taking pending_blocks_details cache into account whenever we are preparing the ExtrasUpdate.
In prinicple there is another issue, where you have two concurrent batches present - the blockchain API doesn't really prevent that, but I think we never do that in practice (so I don't think we need to address it here).

Do you also feel the issues I've raised above are valid?

[AtkinsChang]

Yup, it may cause data inconsistency for two concurrent batch. We should make it more like transaction in RMDB to provide hardened Blockchain API

And aside from this issue, there are also issues for warp sync that cause wrong block details.

[tomusdrw]

Looks good, but my gut feeling tells me it would be good to update all other cases where we are in a batch context and try to fetch block_details to use this newly added method:
https://github.com/paritytech/parity-ethereum/blob/d1827ea563fd2ef3708473475d05ef603d30a765/ethcore/blockchain/src/blockchain.rs#L809
https://github.com/paritytech/parity-ethereum/blob/d1827ea563fd2ef3708473475d05ef603d30a765/ethcore/blockchain/src/blockchain.rs#L1061
https://github.com/paritytech/parity-ethereum/blob/d1827ea563fd2ef3708473475d05ef603d30a765/ethcore/blockchain/src/blockchain.rs#L1168
https://github.com/paritytech/parity-ethereum/blob/d1827ea563fd2ef3708473475d05ef603d30a765/ethcore/blockchain/src/blockchain.rs#L1667

Even though some places are probably safe (we read total_difficulty that is not really altered anywhere), I'd use this method for consistency and to make the code more future-proof.

[AtkinsChang]

@tomusdrw Updated as your suggestion

[tomusdrw]

👌

[dvdplm]

@AtkinsChang good stuff, thanks! Before merging we're going to run a few tests from this branch just to make sure we don't mess up.
/cc @adilimroz

[adilimroz]

@dvdplm Ran syncs with the mentioned commits. It all looks good to me.

[dvdplm]

Minor nitpicks and some documentation questions.

[AtkinsChang]

Thanks for review
fixed in amended commit