new(tests): EIP-7069: Different RETURNDATACOPY oob

[pdobacz]

🗒️ Description

Simple test case covering the updated out-of-bounds behavior of RETURNDATACOPY accross legacy and EOF contracts.

I'm not sure whether to place this new test in the existing 7069_extcall subdir or in its own, please advise.

🔗 Related Issues

Related update to the spec ethereum/EIPs#8617. Update to evmone ethereum/evmone#909

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

[pdobacz]

I admit #595 went completely under my radar. But this PR here adds a test which I intended to test a specific edge case - that the RETURNDATACOPY OOB behavior is correct regardless of the caller frame being legacy or eof, that is:

• eof calls legacy, which does OOB RETURNDATACOPY
• eof calls eof, which does OOB RETURNDATACOPY
• legacy calls legacy, which does OOB RETURNDATACOPY
• legacy calls eof, which does OOB RETURNDATACOPY

In other words, that the correctness of OOB RETURNDATACOPY execution doesn't depend on the outside frame being eof/legacy.

@marioevz @shemnon let me know how you see incorporating this edge case (maybe I'm missing that #595 does cover all 4 of these? I think it doesn't b/c entry point is always legacy)

[marioevz]

I think it doesn't b/c entry point is always legacy

It's true but then the legacy code makes a call to a contract that can be either EOF or legacy, and that is the one that contains the RETURNDATACOPY part under test, then it does the oob testing when parameter size is greater than len(return_data).

[pdobacz]

I think it doesn't b/c entry point is always legacy

It's true but then the legacy code makes a call to a contract that can be either EOF or legacy, and that is the one that contains the RETURNDATACOPY part under test, then it does the oob testing when parameter size is greater than len(return_data).

Correct, but I specifically want to check if part under test is independent of the "outside", which is the caller context, i.e. RETURNDATACOPY logic correctly changes on the eof-legacy boundary, both ways. This is so far our only opcode where EOF and legacy behavior differs, so I want to give it a little extra attention. WDYT?

[marioevz]

I think it doesn't b/c entry point is always legacy

It's true but then the legacy code makes a call to a contract that can be either EOF or legacy, and that is the one that contains the RETURNDATACOPY part under test, then it does the oob testing when parameter size is greater than len(return_data).

Correct, but I specifically want to check if part under test is independent of the "outside", which is the caller context, i.e. RETURNDATACOPY logic correctly changes on the eof-legacy boundary, both ways. This is so far our only opcode where EOF and legacy behavior differs, so I want to give it a little extra attention. WDYT?

Makes sense, seems to me though that we could add another pytest.mark.parametrize to

execution-spec-tests/tests/prague/eip7692_eof_v1/eip7069_extcall/test_returndataload.py

Line 74 in eee1625

def test_returndatacopy_handling(

, where we modify the entry-point address calling opcode here:

execution-spec-tests/tests/prague/eip7692_eof_v1/eip7069_extcall/test_returndataload.py

Line 103 in eee1625

address_entry_point: Account(

to make it either EXT*CALL or *CALL, which would make it EOF or legacy.

Although, it would create a big amount of tests, but it might be necessary.

[shemnon]

In other words, that the correctness of OOB RETURNDATACOPY execution doesn't depend on the outside frame being eof/legacy.

I'm not understanding how any rational implementation would have different handling of returndatacopy if the called contract was EOF or legacy. It's a byte array in the frame context, not caring nor knowing how it was generated. This brings us into the realm of testing hypothetical implementation strategies.

[pdobacz]

Makes sense, seems to me though that we could add another pytest.mark.parametrize to

yea, I was wondering that this is better, once we have test_returndataload.py now, but the large number of tests was why I considered a separate one. What would be your preference (hypothetically, assuming we do want to have it)?

I'm not understanding how any rational implementation would have different handling of returndatacopy

I'm not sure about rational, but buggy or misunderstood - yes: one where eof/legacy-ness-flag of the RETURNDATACOPY is set once per transaction and/or then incorrectly modified when creating a new frame.

[marioevz]

yea, I was wondering that this is better, once we have test_returndataload.py now, but the large number of tests was why I considered a separate one. What would be your preference (hypothetically, assuming we do want to have it)?

Yes I think having a separate function, even if it has much of the code in test_returndatacopy_handling, would be better since that function already produces 448 tests.

[shemnon]

I can look into trimming the returndata test to representative values, the cartesian join gets big quick.

[pdobacz]

Now I'm also worried about the complexity of the resulting test function, if we combine them, not only # of generated tests. Let me make the first step of moving the new test into the existing file and aligning conventions, and then we can see if we like it or not.

[pdobacz]

I reworked the test, adding it under the existing test file. I think it is better not to overload the existing function, it takes a long time to fill already, and that would double it.