SstoreCallToSelfSubRefundBelowZero failing in Istanbul

[s1na]

The SstoreCallToSelfSubRefundBelowZero test case is failing for Istanbul (see #607). Here's why:

The test case calls a contract which has an existing storage slot. It clears the slot, thereby refunding some gas according to EIP-2200. It then calls itself and in the call it sets the slot again to some value, which means the prior refund has to be cleared:

https://github.com/ethereumjs/ethereumjs-vm/blob/00d8fe3c7941fe571581fff1cd4e327980928127/lib/evm/opFns.ts#L1018-L1023

But here it panics because the refund counter of the EEI is 0 in the current message, i.e. in the nested call EEI is not aware that there has been some previous refund, because we create a new EEI instance for every message. We should move the refund counter to outer layers (two of the possibilities are in EVM or StateManager).

Similar to geth which store their refund counter in StateDB I tried to store it in our StateManager and could make the test pass. I also had to clear the counter in runTx for next txes. This raises the question whether we should also clear the counter in runCall and runCode? not sure...

I'll also try with storing it in EVM, because it somehow feels more logically related.

[alcuadrado]

Nice catch @s1na !

I also think that the EVM class looks more related to this than the StateManager. Only one EVM is created per runTx/Code/Call, so it should work.

This raises the question whether we should also clear the counter in runCall and runCode? not sure...

I guess they should also clear it. Wouldn't it break runTx if they set the counter and don't clear it? The next call to runTx would start with a greater than 0 counter.

[evertonfraga]

PR #611 didn't close this one automatically, so I am doing it now.