Test VM exceptionError bug

[holgerd77]

This is testing behavior along a potential bug fix for #1157, do not merge.

[codecov]

Codecov Report

Merging #1168 (d40475f) into master (88823f3) will increase coverage by 1.87%.
The diff coverage is 100.00%.

Flag	Coverage Δ
block	79.77% <ø> (-0.40%)	⬇️
blockchain	84.16% <ø> (-0.07%)	⬇️
client	84.00% <ø> (ø)
common	87.39% <ø> (ø)
devp2p	84.20% <ø> (-0.08%)	⬇️
ethash	82.08% <ø> (ø)
tx	82.26% <ø> (-0.22%)	⬇️
vm	86.57% <100.00%> (+5.71%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

[holgerd77]

@alcuadrado ok, this doesn't affect VM test run behavior at all. I would cautiously say we could eventually merge this. From my understanding of the whole coherences I would assume that the effect of this would essentially be that if you guys or we directly in the VM have a bug in the code (e.g. in a custom state manager), this would directly break and trickle up and not be disguised any more and so the effect from this would be that it will get easier to have these kind of failures fixed? Is this correct?

[ryanio]

What's the status of this PR? I'm thinking we can add a test to ensure that non-VmErrors propagate through as expected, then it can be ready to merge in?

Codecov didn't give any coverage warning on the newly added lines in this PR so I'm wondering if the code path is already being covered, I'd be interested to see.

[holgerd77]

@ryanio I think this can be merged.

The suggestion on setting up a test is great 😄 , what would be good setup here?

I was thinking about subclassing Interpreter and add some fitting function which just throws, but it is impressively complex (or rather: laborious) to get this setup going respectively do an instantiation with all this message setup, I think we should really simplify here at some point as far as possible and generally make it easier to instantiate an independent EVM separate from all this outer environmental processing logic stuff brought in in runTx(), runBlock() and the like.

The other way for adding a test here might be the usage of the testdouble library which we already use in the client.

Any preference here respectively suggestion?

[holgerd77]

@ryanio short ping (see last question) 😄 (also feel free to add a test yourself if you have an easy idea here)

[ryanio]

rebased this, ok let me know what you think of the added tests. tried to keep it as simple as possible and used @alcuadrado's original suggestion in the issue to throw an error from stateManager.putContractStorage()

[holgerd77]

Yeah, nice, this looks really good, will merge. 😄 👍

Also a somewhat really core-centered change (like the recent and discussed StateManager changes) with some potential to cause some reactions once released, but I guess very much net-positive since either a) people will discover own bugs in their customizations which were previously hidden or b) people might report bugs we still have in the VM.

So really excited about this change, long overdue! 🎉

(will merge by admin-merge since technically reviewed by @ryanio by continuing the work)

[holgerd77]

Ah, yeah, really nice test cases! 😄