EVM execution error should not result in DispatchError

[sorpaas]

fixes #178

Even if the EVM execution fails, it should not result in DispatchError, because the state is still changed. The error should be communicated via a Substrate event.

[JoshOrndorff]

it should not result in DispatchError, because the state is still changed

I also struggled with this. I know it is generally a Substrate best practice to "verify first, write last". But I thought that's because it is almost always the right thing to do. Like we wouldn't want someone to cast a vote twice or have a partial token transfer or something.

I was thinking in this case it might actually make sense to mutate anyway.

I'm fine with this fix, but I would like to hear your thoughts whether "verify first, write last" is an absolutely always rule or if there might be exceptions (like using unsafe Rust is sometimes okay).

[sorpaas]

@JoshOrndorff I think this is an absolutely-always rule. Otherwise many assumptions within runtime will break. I remember @shawntabrizi once corrected me on this point.

[JoshOrndorff]

I was thinking about this more, and I think the way pallet-ethereum uses runtime storage is pretty unique. We don't actually persist any state between blocks. All the state is transient, and used only to store data in a particular block. Then it is reset at the beginning of the next block (this is similar to how events are stored in frame system).

Because we use storage in this transient way, I think it would be okay to use it even when the transaction fails.

(Again, I'm fine with how this PR ended. Just having more of an architectural discussion.)