new(tests): port ethereum/tests calldataload opcode tests

[LouisTsai-Csie]

🗒️ Description

Migrate calldataload test from ethereum/tests

🔗 Related Issues

https://github.com/ethereum/execution-spec-tests/issues/972

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

[winsvega]

Op.GAS. and if doesn't work on Frontier
Op.Sub(Op.Gas, 20_000) where 20_000 is the gas we reserve for all instructions after the call.

[LouisTsai-Csie]

Could you please provide more details? I am curios about this, I could not pass the tests until the changes

Is it because EIP-150 was not yet been implemented in the Frontier upgrade, causing the CALL instruction to forward all available gas rather than reserving some for following operations?

[winsvega]

untill Homestead if CALL have gas=100000 it will consume all of it.
after if CALL have gas = 100_000 it will only consume as much as it requires and will not OOG if you have lets say 50_000 and it consumed less then that. but will fail OOG if it consumes > than you have or > 100_000

[LouisTsai-Csie]

I noticed a TODO in the README that says, "Consider squashing commits to improve commit history." When addressing issues, should I squash my commits into a single commit right away, or should I keep pushing commits until all changes are complete and then squash them into one? Which approach would be more helpful for the reviewer?

[LouisTsai-Csie]

I analyze other PRs and found there is a specific setting for the protected variable. I am not sure what it is for until I analyze related function:

execution-spec-tests/src/ethereum_test_types/types.py

Line 830 in 1115574

if self.protected:

v, r, s = (
            signature_bytes[64],
            int.from_bytes(signature_bytes[0:32], byteorder="big"),
            int.from_bytes(signature_bytes[32:64], byteorder="big"),
        )
        if self.ty == 0:
            if self.protected:
                v += 35 + (self.chain_id * 2)
            else:  # not protected
                v += 27

I want to double check, is it for EIP-155 that prevents replay attack?

[winsvega]

Yes, protected is eip155

[winsvega]

I noticed a TODO in the README that says, "Consider squashing commits to improve commit history." When addressing issues, should I squash my commits into a single commit right away, or should I keep pushing commits until all changes are complete and then squash them into one? Which approach would be more helpful for the reviewer?

Squash commits when they are too many or affect many different files. So, not to deal with rebase conflicts.
Generally 3-5 commits are ok.

[marioevz]

Hi Louis! Thanks for the PR, it looks pretty good!

Just a couple of comments though.

[marioevz]

It's a bit more readable to do, e.g.:

        pytest.param(
            0x00,
            Bytecode(Op.MSTORE8(offset=0x0, value=0x25) + Op.MSTORE8(offset=0x1, value=0x60)),
            0x02,
            0x2560000000000000000000000000000000000000000000000000000000000000,
            id="two_bytes",
        )

So basically use pytest.param to have the parameters and the ID in the same place.

[LouisTsai-Csie]

I analyze similar test case before doing this PR, and I notice there is several instance that can also be refactored.

For parametrization

execution-spec-tests/tests/frontier/opcodes/test_calldatacopy.py

Line 10 in 8513a8e

@pytest.mark.parametrize(

For zero value in creating transaction

execution-spec-tests/tests/frontier/opcodes/test_dup.py

Line 64 in 8513a8e

tx = Transaction(

There are more instances, just to name few here

[marioevz]

This data is not actually used right now, so we can remove it.

But... we could write a second test where test Op.CALLDATALOAD on the first call stack level:

Right now we are testing on the second call stack level, simply because we are using the opcode on a second contract via Op.CALL.

Another test could use this transaction field to test Op.CALLDATALOAD using a single contract.

[LouisTsai-Csie]

That would be great, the calldata is loaded from the data provided in the transaction, I could add a few test cases for it.

Should I add the scenario in the current PR or create a new one? Which one do you prefer?

[danceratopz]

Test looks good. However fill doesn't work. Need to see whats wrong around hash 0x20000

Hey @LouisTsai-Csie, thanks for the PR! Just a heads up, the failing Github Action Evmone Coverage Report is a separate issue unrelated to your test. It should be fixed with #1268.

[LouisTsai-Csie]

@marioevz Thanks for your review, I updated the file and left several comments.

I wonder if I should use git reset to clean up my commit history and ensure that each commit represents an independent task. Based on my previous experience, some teams prefer not to reset commit history. I’d love to hear your thoughts on this.

[marioevz]

@marioevz Thanks for your review, I updated the file and left several comments.

I wonder if I should use git reset to clean up my commit history and ensure that each commit represents an independent task. Based on my previous experience, some teams prefer not to reset commit history. I’d love to hear your thoughts on this.

We don't have strict guidelines regarding how to merge commits in this repository, but usually we simply squash and merge a PR, which means the commit history is not normally preserved.

For PRs containing only test changes like this one, squashing commits would be the approach, so don't worry too much about preserving every commit 👍

An example of a PR we would like to preserve the history is one where not only tests are affected but it also contains framework changes.

[marioevz]

One last minor comment and we should be able to merge. Thanks!

[LouisTsai-Csie]

I've updated the comment. Please let me know if it is clear. Thanks!

[winsvega]

need to check why this line coverage is lost

 evmone::state::MPTNode::ext(evmone::state::(anonymous namespace)::Path const&, std::unique_ptr<evmone::state::MPTNode, std::default_delete<evmone::state::MPTNode> >)
evmone::state::State::journal_balance_change(evmc::address const&, intx::uint<256u> const&)

the other seem to be related to add opcode not being called anymore

lost coverage line:

for (const auto& addr : diff.deleted_accounts)
      49 [LBC](file:///home/wins/Downloads/coverage-diff-native-13585044713-2/DIFF/evmone/test/state/test_state.cpp.gcov.html#top)           0 :         erase(addr);

there used to be an account removed from state in original test. thats why coverage lost.