Optimise CALL by throwing stack underflow earlier

[Marchhill]

Changes

• Pop from stack at start of instruction to fail faster without having to access state etc. Also better matches spec

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

No regressions in hive consensus tests.

If yes, did you write tests?

• Yes
• No

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

[LukaszRozmej]

Please run hive tests as sometimes order matters

[flcl42]

Order seems to be important, we need to request a hive test. And cover by internal own ones

[tanishqjasoria]

i think this would result in different access lists and then cause gas mismatch in nested calls.

[Marchhill]

i think this would result in different access lists and then cause gas mismatch in nested calls.

Are you talking block access lists here? Because yes that's the main point of the change, the BAL would be incorrect without it. In the execution specs popping from the stack always happens at the start of the CALL: https://github.com/ethereum/execution-specs/blob/forks/osaka/src/ethereum/forks/osaka/vm/instructions/system.py#L365

Could you expand on why it could cause a gas mismatch?

[tanishqjasoria]

i think this would result in different access lists and then cause gas mismatch in nested calls.

Are you talking block access lists here? Because yes that's the main point of the change, the BAL would be incorrect without it. In the execution specs popping from the stack always happens at the start of the CALL: https://github.com/ethereum/execution-specs/blob/forks/osaka/src/ethereum/forks/osaka/vm/instructions/system.py#L365

Could you expand on why it could cause a gas mismatch?

i was taking about mismatch because of this - https://github.com/ethereum/execution-specs/blob/forks/osaka/src/ethereum/forks/osaka/vm/instructions/system.py#L376
if we fail before adding the to address to accessed_addresses, then if in the same transaction we try to charge for the same address again, then we would not charge the warm gas but the cold gas. But if it is like this in the spec maybe this case never arises?

[LukaszRozmej]

please observe if no regressions in hive tests

[tanishqjasoria]

we revert access tracker when the call fails.

[Marchhill]

please observe if no regressions in hive tests

setting up a machine to check this before merging, will take quite a few hours to run

[flcl42]

Unit tests that confirm outcome is not changed when running this code part with not enough items in stack/underfunded account

[Marchhill]

Unit tests that confirm outcome is not changed when running this code part with not enough items in stack/underfunded account

@flcl42 you think it needs changes?

[flcl42]

Unit tests that confirm outcome is not changed when running this code part with not enough items in stack/underfunded account

@flcl42 you think it needs changes?

Yes, the pr lacks unit tests?

[Marchhill]

Unit tests that confirm outcome is not changed when running this code part with not enough items in stack/underfunded account

@flcl42 you think it needs changes?

Yes, the pr lacks unit tests?

Added some unit tests and confirmed no regressions in hive consensus tests