new(tests): EOF - EIP-7620 EOFCREATE gas testing

[pdobacz]

🗒️ Description

EOFCREATE gas testing using the "Op.GAS harness" used in #771.

I'm pushing early to signal the fix(fw): max stack height calculation in __add__. The new bytecodes used in the new tests didn't have the correct autocalculated max stack height. After some digging, I've noticed that the algorithm might not handle all cases well, and I am proposing an alternative. I've left all my overly verbose comments in the code to illustrate the train of thought, as this has been quite hard to grasp the original code intentions (maybe there has been an easier fix hidden from my sight there). EDIT: the max stack height fix split out to #810, merge 810 first

Apart from that the EOFCREATE test reveals some difficulties related to the harness, and how the harness needs to be generalized to test such instructions. However, since the difficulties are related to satisfying the requirements of subject code (the EOFCREATE in question here has many), I suppose these difficulties would be present in all gas testing approaches.

🔗 Related Issues

Addresses the last remaining item off of the list ipsilon/eof#138.

✅ Checklist

• All: Set appropriate labels for the changes.
• All: Considered squashing commits to improve commit history.
• All: Added an entry to CHANGELOG.md.
• All: Considered updating the online docs in the ./docs/ directory.
• Tests: All converted JSON/YML tests from ethereum/tests have been added to converted-ethereum-tests.txt.
• Tests: A PR with removal of converted JSON/YML tests from ethereum/tests have been opened.
• Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
• Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

[marioevz]

It looks good, but I think we should merge the changes to Bytecode first as a separate PR.

[marioevz]

Looks really good, just a few comments.

One concern I have is that it might be too many tests, we have an utility to parametrize in a more conservative manner by using defaults in some of the parameters and only create a limited amount of vectors, instead of creating the product of all values of all parameters: https://ethereum.github.io/execution-spec-tests/main/writing_tests/writing_a_new_test/#the-extend_with_defaults-utility

It might be worth exploring using this utility here.

[pdobacz]

One concern I have is that it might be too many tests, we have an utility to parametrize in a more conservative manner by using

Ah, missed this comment, let me take a look before I ask for re-review, sorry

[pdobacz]

One concern I have is that it might be too many tests, we have an utility to parametrize in a more conservative manner by using

Ah, missed this comment, let me take a look before I ask for re-review, sorry

@marioevz ok, I used extend_with_defaults, thought I'm not 100% sure I like it that much. It requires me to assume knowledge about which parameter value might interact and which not, and spell out a lot of cases manually. I've used the tool to make most tests use the default memory expansion value, and added extra memory expansion cases where I guessed it might matter "more".

But this is a bit impacting the test with implementation details. Is the fill time, test run time or JSON files size the limiting factor here?

[marioevz]

But this is a bit impacting the test with implementation details. Is the fill time, test run time or JSON files size the limiting factor here?

Yes I guess it's a bit limiting, the main issues is the run time, but we are planning to mitigate this by composing a large test fixture with all state tests in it, so the amount of tests will be a bit less important.

I think we can roll this back, just wanted for us to give it some consideration.

[pdobacz]

I think we can roll this back, just wanted for us to give it some consideration.

Done. I'll keep this change on the shelf in case run-time needs some shaving off. I'll also keep this tool in mind (I'm working on a huge eof_test RN, so I'm trying to figure out how to reduce the number of cases. However, I think with that other test I'd face similar difficulties as here, if I used extend_with_defaults)

[marioevz]

LGTM!

[shemnon]

LGTM