Stop adding account to cache when checking if it is empty.

[danjm]

This PR fixes three currently failing blockchain tests:

• "OOGStateCopyContainingDeletedContract",
• "suicideCoinbase",
• "RecallSuicidedContractInOneBlock"

These started failing with #147

The PR implements changes that are required for EIP-158. EIP-158 requires that accounts evaluated as empty be deleted.

With #147, we started checking if accounts were empty, in runTx.js by calling stateManager.accountIsEmpty. This in turn calls stateManager.getAccount which calls cache.getOrLoad.

The call to cache.getOrLoad is problematic because if it cannot find an account in the cache, it will add one to the cache. (Either the one it finds in the merkle tree / cache.trie or a new one.) However, this runs contrary to the original intention of the stateManager.accountIsEmpty call in runTx.js which deletes the account from the cache if it is found to be empty (thereby meeting some of the requirements of EIP158).

The result is that the expected state root hash from the state manager does not match what is expected given the header and is caught as an error after flushing the cache in parseBlockResults() of runBlock.js. (In between, the stateManager.cache.flush causes the account unintentionally added to the cache to be added to the trie, thereby changing its state root.)

The solution in this PR is to create a new method in stateManager which is to be called if you want to get an account without the side effect of adding it to the cache if it does not exist.

This also exposes another improvement that can be made (for which I will create an issue): refactoring existing calls of stateManager.getAccount so that we only call the cache.getOrLoad method when the side effect of loading a new account into the cache is desired.

[holgerd77]

Hi @danjm, great, thanks for this extensive PR and deep dive into that, great work! Currently @mattdean-digicatapult is in parallel working on a StateManager overhaul - this PR #309 is the summing-up meta PR and this PR (some extract from the first) on caching #366 is currently in the works.

Can you guys please cross-coordinate? There might be some overlap and eventually some double work already being done, have not enough oversight atm to judge. There nevertheless should also being so much distinct work being done on both side that it should be possible to bring this together in a value-adding way from boths sides.

Anyhow, please get in contact! 😄

[mattdean-digicatapult]

Great find @danjm, I hadn't even thought that this would be an issue! I don't think this conflicts badly with my open PR (#366) so I'm happy for this to be included now. My only recommendation would be to mark the new method getAccountPure private by prepending it with an _ as with other "private" methods.

For the ultimate goals of #309 I might refactor this but it's probably better to merge this now and fix this sooner 👍

[holgerd77]

Hi @danjm, I've now merged the StateManager cache refactoring PR from @mattdean-digicatapult, can you do a (cautious) rebase of this? Matthew estimated that it wouldn't be too conflicting though.

[danjm]

@holgerd77 I have rebased onto the latest master. There were no conflicts.

@mattdean-digicatapult I marked the _getAccountPure method as private as you suggested.

[holgerd77]

Hi, sorry, have just merged a larger documentation refactoring PR #377, could you once rebase again on top of this and see if things go well? Then I can directly merge your PR, thanks!

[danjm]

@holgerd77 rebased again.

Also. I just had a thought. Before you merge, I am going to run the blockchain tests locally with the tap-spec option and compare results between my branch and master. I want to make sure that this PR didn't inadvertently cause another blockchain test to fail.

[holgerd77]

Ah, you can directly have a look at the end of the CircleCI run results, or am I missing something here?

[holgerd77]

Or do you mean that the sum of failing tests might be smaller, but eventually with a large decrease + some increase? Never thought about that case! 😄

[holgerd77]

Ah, and just a note: can you do the next PR directly on the main repository and not your own fork? That makes it easier for third-party rebase, allows for easier collaboration work + currently coveralls is not working for PRs from forks.

[danjm]

Yeah, I wanted to compare the list of failing test names to cover that second case you mentioned.
It is taking too long locally, so I just sifted through the circleCI results. I think we are good to go.

There are 50 failing tests on this branch, but 60 on master. And the 10 tests in the difference are all on master.

Also I discovered that this fixes a couple more files than I previously thought. The five files fixed are:
OOGStateCopyContainingDeletedContract_Byzantium
suicideCoinbase_Byzantium
suicideStorageCheck_Byzantium
suicideStorageCheckVCreate_Byzantium
RecallSuicidedContractInOneBlock_Byzantium

[danjm]

can you do the next PR directly on the main repository and not your own fork?

Yes, I will do that.

[holgerd77]

Thanks so much for the work, having blockchain tests fixed really feels great! 😄

Will approve and merge.