search

ethereum / execution-spec-tests

A Python framework and collection of test cases to generate test vectors for Ethereum execution clients
https://ethereum.github.io/execution-spec-tests
MIT License
88 stars 62 forks source link

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

Closed pdobacz closed 1 week ago

pdobacz commented 3 weeks ago

🗒️ 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 https://github.com/ethereum/EIPs/pull/8617. Update to evmone https://github.com/ethereum/evmone/pull/909

✅ Checklist

pdobacz commented 2 weeks ago

I admit https://github.com/ethereum/execution-spec-tests/pull/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:

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 https://github.com/ethereum/execution-spec-tests/blob/eee1625f21ec7f2bf78850e5e49192fc118ef2cf/tests/prague/eip7692_eof_v1/eip7069_extcall/test_returndataload.py#L74, where we modify the entry-point address calling opcode here: https://github.com/ethereum/execution-spec-tests/blob/eee1625f21ec7f2bf78850e5e49192fc118ef2cf/tests/prague/eip7692_eof_v1/eip7069_extcall/test_returndataload.py#L103 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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

pdobacz commented 2 weeks ago

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 commented 2 weeks ago

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.