convert calldatacopy test

[pacrob]

🗒️ Description

Converts calldatacopy tests from here

PR to delete from ethereum/tests is here

Tests pass run against evm statetest and evm blocktest geth version 1.14.12-stable-293a300d

Questions:

• I'm not sure how to address the coverage CI failure. I tried running it locally but didn't get any more clarifying info from it.

🔗 Related Issues

✅ 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]

Thanks for this! I've left some comments to make the code more readable, I can fully review once these are applied 👍

[marioevz]

Questions:

• I'm not sure why tests before Byzantium are failing.

This could potentially be due to the protected field in the transaction, since I think the transactions can only be protected on Byzantium and afterwards. You can try protected=fork>=Byzantium in the transaction building call (requires adding fork: Fork as parameter to the test function, and importing Fork from ethereum_test_forks).

• If the file is to be deleted from ethereum/tests, how should I reference the source in the docstring?

We could use the permalink to the file: https://github.com/ethereum/tests/blob/ae4791077e8fcf716136e70fe8392f1a1f1495fb/src/GeneralStateTestsFiller/VMTests/vmTests/calldatacopyFiller.yml

• I'm not sure how to address the coverage CI failure. I tried running it locally but didn't get any more clarifying info from it.

This might be because of the way the code is compiled for the ethereum/tests version of the test, so there's a difference between the opcodes used, but we could analyze the result and if deemed unimportant we can merge as is, no problem.

[winsvega]

or you can reference legacy test file. because ethereum/tests:: develop tests are essentially a copy of tests in legacytests/cancun.
legacytests are frozen from modifications
ethereum/tests are to be converted to .py and deleted. so no development or new modifications

[pacrob]

OK! I've addressed all the comments. Last questions from the checklist:

• I can't set tags. What should they be?
• How do you want the tests to be run? I've successfully fed them to evm statetest and evm blocktest (geth version 1.14.12-stable-293a300d), but not sure how to use a filled test file with the geth t8n tool.
• mkdocs serve runs fine, but there doesn't seem to be any documentation generated in this case. Is that correct?

Also, I got the last failing tests to run by raising the gas limit in the tx from 100_000 to 100_000_000, but didn't try anything in between. Is it important here to try to be specific in the gas amount, or is just adding a bunch OK?

[marioevz]

Hey! thanks, giving it another look just now :)

• I can't set tags. What should they be?
  Do you mean the ones in the PR? I think it should be ok to set the labels to scope: tests.

• How do you want the tests to be run? I've successfully fed them to evm statetest and evm blocktest (geth version 1.14.12-stable-293a300d), but not sure how to use a filled test file with the geth t8n tool.

This is perfect, evm statetest and evm blocktest should be more than enough.

• mkdocs serve runs fine, but there doesn't seem to be any documentation generated in this case. Is that correct?

I tried it out and it generated ok, checkout "Test Case Reference":

Then in the sidebar you should see the test case you wrote here:

Also, I got the last failing tests to run by raising the gas limit in the tx from 100_000 to 100_000_000, but didn't try anything in between. Is it important here to try to be specific in the gas amount, or is just adding a bunch OK?

If there's no way around it then it's ok to add a bunch of gas, but if we can keep the gas limit to less than 30,000,000 it means that the test can run in live networks.
Otherwise we can just add the

@pytest.mark.execute(pytest.mark.skip("too much gas"))

marker (the string is not standardized) and it should be gracefully skipped when running in live networks. 👍

[marioevz]

Hey looks good so far, I have a couple more comments.

The gas issue I haven't been able to figure out but one thing that we could to is just limit the test to be filled only on Byzantium and after.

Let me know what you think.

[marioevz]

Figured out the gas issue: It seems like passing something above the current available gas results in the CALL opcode charging a really high amount of gas in Frontier and Homestead, which I think was fixed in Byzantium with EIP-150.

The solution seems to be something along the lines of:

Op.CALL(
                gas=Op.SUB(Op.GAS(), 0x100),
                address=code_address,
                value=0x0,
                args_offset=0xF,
                args_size=0x10,
                ret_offset=0x20,
                ret_size=0x40,
            )

so the call does not try to send a gas amount higher than it currently gas.

With this change, sending the transaction with a gas limit of 100_000 works for all tests 🎉

[pacrob]

OK! All requested updates pushed. I found them in docs, just didn't know where to look 😄

[winsvega]

the only missing coverage is calldataload and add opcodes. can be ignored.

[marioevz]

LGTM, thanks!

[pacrob]

Thanks for updating the gas, @marioevz - guess I got it in the one place and missed the other.

[marioevz]

I've verified the coverage loss and it's down to a couple of things:

• The ADD and CALLDATALOAD opcodes are no longer used, which is ok
• There are no empty accounts in the test's pre-state, also ok
• MPT state differences (due to different accounts used), also ok

We should be able to merge without issues 👍