LFVM: Test integer overflow from stack values

[LuisPH3]

part of #751

This test checks operations that fetch values (256 Bits) from the stack for 64 bits operators.
Both CALL and CREATE family of operators are too complicated for this batch testing, and require to be tested on its own.

Included refactor:

• Reorder some operations code to prioritize check of operators overflow before more specific operator checks.
• Remove hard-coded maxuint64 uses.
• Remove ignored err values, return them instead.

[HerbertJordan]

I like the code cleanup, bit I am not sure the property that is tested by the new test is an actual requirement or just an implementation artifact.

In all cases I reviewed in this PR, it seems like numeric overflows are related to offsets in the memory, in code, or in various data buffers (call return data or some code). If there is such an overflow, setting the 64-bit value derived from the 256-bit value on the stack to the maximum 64-bit value (saturation semantic) should automatically lead to an actual error (e.g. out-of-gas when attempting to access memory) or to a correct handling (returning zero values when reading from a buffer, since no buffer is long enough).

Thus, the dedicated handling of overflows and reporting those as errors seems to be redundant. It actually introduces the risk of identifying an overflow as an issue that should actually be ignored if, for instance, the offset of a memory access overflows but the size of the memory offset is zero. Such a case should not be marked as an error.

If you agree, I would suggest to update the tests to check for any kind of errors and also use 2^64 as a test input, since this value would be reduced to 0 if the overflow check is incorrectly implemented, although it should be considered equivalent to 2^64-1 = MaxUint64.

[HerbertJordan]

If you define the inputs field to be of type [][]uint256.Int the initialization could look like this:

Suggested change

	inputs: []inputData{
	inputStack(maxUint256, zero, zero),
	inputStack(zero, maxUint256, zero),
	inputStack(one, zero, maxUint256),
	},
	inputs: [][]uint256.Int{
	{maxUint256, zero, zero},
	{zero, maxUint256, zero},
	{one, zero, maxUint256},
	},

This eliminates the need for the inputData type and the inputStack factory and places everything that is happening in the initialization code.

Wdyt?

[LuisPH3]

I find it quite cumbersome to write arrays of arrays and the code begins to look like Perl, I will remove the alias as it seems to be that it is not idiomatic.

[HerbertJordan]

For JUMPI a test should be added that makes sure that an overflow is not causing an error if the condition is false.

[LuisPH3]

there is no tests for this, right.

[HerbertJordan]

nit: The parameter order here is reversed ... the top most element is last. Took me some time to figure this out. Maybe it can be commented somewhere or the order could be inverted?

[LuisPH3]

after rebase, i can start using some tool Facu wrote.

[HerbertJordan]

The first or second (actually second or third) parameter should be tested for overflows. These are the source and destination offsets respectively.

[HerbertJordan]

This only checks the size for an overflow, but not the two other parameters, which are offsets.

In the code, size overflows seem to be treated differently than offset overflows. Size overflows lead to errOverflow while other overflows seem to result in out-of-gas errors. I am wondering whether not all overflows related to offsets in memory accessed would lead to out-of-gas errors, and thus should not be explicitly checked.

tl;dr: are some of those extra checks for overflow errors that would actually also be called by out-of-gas errors?

[HerbertJordan]

If I understand correctly, this tests make sure that overflows in offsets are ignored if the size is zero. If so, this should be reflected in the name. I do not think it has anything to do with rounding ...

[HerbertJordan]

Another way to think about it: if we would update the member functions of the memory implementation to deal with uint256.Int instead of uint64 many of the checks in the op-codes dealing with the memory would move into the memory implementation. And there, "overflows" would be naturally caught by out-of-gas errors.

[LuisPH3]

This PR is closed because revision exposed that the code has structural flaws that should be refactored insted of tested.