OOGX tests using BytecodeRunner.runOnlyForGasCost + MSIZE

[lorenzogentile404]

Closes #1685

• BytecodeRunner.runOnlyForGasCost Method to calculate cost dynamically - based on LondonGasCalculator (Besu)
• OOGX for opcodes with no mem expansion : list from opCodeToOpCodeDataMap.values() without opcodes with mem expansion cost
• OOGX for opcodes with mem expansion cost : MSTORE, MSTORE8, MLOAD, CALLDATACOPY, CODECOPY, EXTCODECOPY, RETURN, RETURNDATACOPY, REVERT, CREATE, LOGX
• MSIZE test: with non zero value and post memory expansion

Local benchmark done on 366 tests with

• calculate gas cost manually 1min40s
• calculate gas cost dynamically 1min30s
• calculate gas cost by running BytecodeRunner.run twice 2min50s

Test coverage improvement
+7 lines covered
+4 conditions covered
Results
+0.3% condition coverage on Unit tests
+0.1% overall coverage score (with blockchain ref tests)

[OlivierBBB]

Obvious remark: both MSTORE's expand memory, the first to size 0x20, the second to size 0x40.

[amkCha]

You're right, it felt better to go from a non-zero value to a non-zero value, but it's strictly the same behavior. Let me simplify it 👍

[OlivierBBB]

This paragraph could be extracted into a method I imagine ? It's duplicated quite a number of times.

[amkCha]

Good idea, I created a Utils method 👌

[OlivierBBB]

It would make sense to rename this test to

void outOfGasExceptionWarmExtCodeCopy(int cornerCase) { ... }

(the address in question will be warm after the successful deployment) and to add a second test called e.g. outOfGasExceptionColdExtCodeCopy where you

• define a ToyAccount with address c0deadd7e55, provide it with byte code that is e.g. simply "ff".repeat(foreignCodeSize) where you set final int foreignCodeSize = 70;, say, and add it to the list of accounts in the test
• then run the following code in the root

program
        .push(foreignCodeSize + 3) // size
        .push(11) // offset
        .push(33) // destoffset
        .push("c0deadd7e55") // address
        .op(OpCode.EXTCODECOPY);

This test targets a cold account and will read out of bounds of its code.

[OlivierBBB]

This is more of a side note than anything.

The above could be achieved more directly. Currently you

• pre-populate the state with an account at address c0de with some code
• EXTCODECOPY this code in full
• use this code as the init code of a CREATE which deploys a new SMC with code I'll call returnDataProviderCode
• STATICCALL into this account, thus producing some nonempty return data

You could shortcircuit the first steps and instead

• pre-populate the state with an account at address c0de with returnDataProviderCode
• STATICCALL into the c0de account, thus producing some nonempty return data

The bytecode would be

program
        .push(0) // byte size of return data
        .push(0) // retOffset
        .push(0) // byte size calldata
        .push(0) // argsOffset
        .op("c0de") // address of 'return data provider' account
        .op(OpCode.GAS) // gas
        .op(OpCode.STATICCALL)
        // 3. Clean the stack
        .op(OpCode.POP)
        // 4. Return data copy
        .push(32) // size
        .push(0) // offset
        .push(65) // destoffset, trigger mem expansion
        .op(OpCode.RETURNDATACOPY);

[OlivierBBB]

I would also suggest to add a variant of the above where we do the following

program
        .push(0) // byte size of return data
        .push(0) // retOffset
        .push(0) // byte size calldata
        .push(0) // argsOffset
        .op("c0de") // address of 'return data provider' account
        .op(OpCode.GAS) // gas
        .op(OpCode.STATICCALL)
        // 3. Clean the stack
        .op(OpCode.POP)
        // 4. Doubly exceptional return data copy
        .op(OpCode.RETURNDATASIZE)
        .push(1)
        .op(OpCode.ADD); // size = RDS + 1, which will trigger the `returnDataCopyException`
        .push(0) // offset
        .push(65) // destoffset, trigger mem expansion
        .op(OpCode.RETURNDATACOPY);

We could name this test to doubleExceptionReturnDataCopyTestOogxAndRdcx. We have so far not written tests where we deliberately trigger several exceptions at once.

[OlivierBBB]

Are we sure this is the correct gasCost ? I'm asking because a naïve implementation of runOnlyForGasCost would compute the total gas consumed by the transaction, say. But this would include the PUSH's and MSTORE's and RETURN's of the deployment inside the CREATE. Are we sure those costs aren't taken into account when running runOnlyForGasCost ?

What we need here is the cost just up to an including the upfrontGasCost of the CREATE.

[OlivierBBB]

Oh, I guess this is what the 6420 etc is about ...

[OlivierBBB]

Is this OUT_OF_GAS_EXCEPTION happening inside of the deployment frame ?

[amkCha]

Yes exactly, can it be checked in a better way ? It's the case where the deployment frame has enough gas to run the code but not enough to cover the depositFee, so it triggers an OOGX

[OlivierBBB]

Here, too, we have the opportunity to test simultaneous exceptions, in this case:

jumpException
outOfGasException

And it would be trivial to implement, just switch the second push to

    final Bytes bytecode =
        BytecodeCompiler.newProgram()
            .push(1) // pc = 0, 1
            .push(6) // pc = 2, 3
            // ...

[OlivierBBB]

It would make sense to create a new class for these MultiExceptionTests.

[OlivierBBB]

Which would include the RETURNDATACOPY double exception test mentioned earlier.

[OlivierBBB]

In terms of MultiExceptionsTest one could have a simple test where one triggers simultaneously

staticException
outOfGasException

or

staticException
stackUnderflowException

One needs two smart contracts with byte code

BytecodeCompiler simpleLog0 = BytecodeCompiler.newProgram();
simpleLog0.push(0).push(0).op(LOG0);

BytecodeCompiler stackUnderflowLog0 = BytecodeCompiler.newProgram();
stackUnderflowLog0.push(0).op(LOG0);

Which are called upon through a STATICCALL with gas = 3 + 3 + 375 + cornerCase / gas = 3 + 375 + cornerCase.

[OlivierBBB]

Left several comments