Remove unnecessary statedb copies and move the prepare inside Finalise

[oneeman]

Background

Logs emitted by the EVM are recorded using StateDB's AddLog() method, which records on the log the transaction hash, transaction index, and block hash, based on the fields thash, txindex, and bhash. These fields are set using StateDB's Prepare() method, which must be called before processing EVM calls. For example, it is called before processing a transaction, both during block construction and during block verification.

In Celo, we have some contract calls done outside of any transaction, as for example the gas price minimum update, epoch rewards, and validator elections. Any logs from these calls are added to a special "block receipt" (see #1628). The way we look up these logs is using state.GetLogs(common.Hash{}) (i.e. looking up logs with zero as their transaction hash). Therefore, before making these EVM calls we need to ensure that state.thash is set to zero using a call to Prepare().

We do this during block verification:

celo-blockchain/core/state_processor.go

Lines 95 to 96 in 765e371

	statedb.Prepare(common.Hash{}, block.Hash(), len(block.Transactions()))
	p.engine.Finalize(p.bc, header, statedb, block.Transactions())

But we don't do it during block construction:

celo-blockchain/consensus/istanbul/backend/engine.go

Lines 502 to 504 in 765e371

	func (sb *Backend) FinalizeAndAssemble(chain consensus.ChainHeaderReader, header *types.Header, state *state.StateDB, txs []*types.Transaction, receipts []*types.Receipt, randomness *types.Randomness) (*types.Block, error) {
	sb.Finalize(chain, header, state, txs)

This has not led to problems because was a StateDB.Copy() call in between transaction processing and calling FinalizeAndAssemble(), and the thash (and related) fields are not copied in .Copy(), so instead the copy has thash set to the zero value, which is what the Prepare() should be setting it to. This call to .Copy() was originally there from upstream for unrelated reasons, and kept during the miner refactor (#1545) as it seemed to be necessary. Without it, thash would be the hash of the block's last transaction (if there were any), and as a result the system call logs would get assigned this incorrect hash is their index, which would lead to their not being found by state.GetLogs(common.Hash{}) and to no block receipt being added. In contrast, during block verification it would be added, leading to a mismatch so that the block is judged invalid.

Description

This PR moves the pre-Finalize() Prepare() calls to inside Finalize(). This is desirable for two reasons:

1. It ensures it's executed during both construction and verification, without having to duplicate it.
2. It's conceptually correct, because (as the name suggests) Prepare() prepares statedb for the EVM calls which follow, and so it belongs together with the code responsible for those calls.

Other changes

• Remove the state.Copy() call which was previously making up for the missing Prepare() call. It is now superfluous.
• Remove another superfluous state.Copy() call (can be seen to be superfluous by the fact that it's called right below where the statedb instance is first initialized, and so we knows it's a fresh instance that no one else has a reference to. This call is at

  celo-blockchain/consensus/istanbul/backend/backend.go

  Lines 600 to 601 in 0390b45

  	// Make a copy of the state
  	state = state.Copy()

• Factor out the logic for adding a block receipt, to reduce duplication. Further, it standardizes the logic to updating the transaction hash, block hash, and transaction index on the system receipt's logs.

Tested

• Confirmed that the stall which would previously happen when removing that .Copy() no longer happens
• Automated tests
• Ran full mode sync on an Alfajores, successfully synced up to block 135,000 with no BAD BLOCK errors.

[codecov]

Codecov Report

Merging #1673 (2305926) into master (14fb27a) will increase coverage by 0.01%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##           master    #1673      +/-   ##
==========================================
+ Coverage   56.06%   56.07%   +0.01%     
==========================================
  Files         605      606       +1     
  Lines       80269    80261       -8     
==========================================
+ Hits        45001    45010       +9     
+ Misses      31744    31726      -18     
- Partials     3524     3525       +1     

Impacted Files	Coverage Δ
consensus/istanbul/backend/backend.go	50.60% <ø> (-0.11%)	⬇️
core/block_receipt.go	18.18% <18.18%> (ø)
consensus/istanbul/backend/engine.go	64.21% <100.00%> (+0.66%)	⬆️
core/state_processor.go	77.41% <100.00%> (+8.84%)	⬆️
miner/block.go	48.79% <100.00%> (-0.34%)	⬇️
consensus/istanbul/core/roundchange.go	83.68% <0.00%> (-3.69%)	⬇️
trie/proof.go	76.77% <0.00%> (-2.25%)	⬇️
miner/worker.go	56.56% <0.00%> (-1.52%)	⬇️
consensus/istanbul/core/core.go	69.29% <0.00%> (-0.53%)	⬇️
... and 8 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14fb27a...2305926. Read the comment docs.

[gastonponti]

LGTM