feat(tests): converting `calldataload` and `calldatasize` tests #1236

pacrob · 2025-02-19T23:13:04Z

🗒️ Description

🔗 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.
remove calldataload and calldatasize tests tests#1477
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.
used geth version 1.15.2-stable-c8c62daf
Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

winsvega · 2025-03-20T09:53:54Z

tests/frontier/opcodes/test_calldatasize.py

+            Account(storage={0x00: 0x11}),
+        ),
+        (
+            b"\x1a\x84Q\xe6\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00!\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01",


is there better way to declare bytes ? more readable

shortened and labeled!

winsvega · 2025-03-20T09:54:32Z

evmone build script failing

pacrob · 2025-03-21T20:31:49Z

OK, I think it's gtg. Still getting coverage loss though. How can I see what's lost , and is there a solution? I think my previous PR had the same issue.

===============
COVERAGE REPORT
/tests/DIFF/index.html
New covered Lines: 180
New covered function Paths: 77

ERROR: coverage is lost!
Lost coverage Lines: 26
Lost coverage function Paths: 5

marioevz

Hey, some comments which I think should make the test a bit more EEST-like, but we might lose irrelevant coverage (Like the SHA3 function which is IMO is out-of-scope for these tests) but that's ok.

tests/frontier/opcodes/test_calldataload.py

marioevz · 2025-03-27T18:23:16Z

tests/frontier/opcodes/test_calldatasize.py

+@pytest.mark.parametrize(
+    "args_size,sha3_len,storage",
+    [
+        ("0002", "01", Account(storage={0x00: 0x02})),


Suggested change

("0002", "01", Account(storage={0x00: 0x02})),

("0002", "01", 0x02),

Similar comment to previous file, Account(storage=... can be defined inside the function to make the parametrization more readable.

tests/frontier/opcodes/test_calldataload.py

marioevz · 2025-03-27T18:38:04Z

tests/frontier/opcodes/test_calldatasize.py

+            )
+        )
+    )
+    tx_data = bytes.fromhex("1a8451e6" + "00" * 30 + args_size + "00" * 31 + sha3_len)


No need to encode this as call data to be decoded by the contract, args_size and sha3_len could be integers and they could be placed inside of the contract directly:

to = pre.deploy_contract( code=( Op.MSTORE(offset=0x0, value=Op.SHA3(offset=0x0, size=sha3_len)) + Op.CALL( gas=Op.SUB(Op.GAS(), 0x100), address=address, value=0x0, args_offset=0x0, args_size=args_size, ret_offset=0x0, ret_size=0x0, ) ) )

Although I'm having trouble understanding why the original test is using SHA3 in the first place since the memory contents are irrelevant, and my best guess is to just have a non-zero filled memory.

I think we should replace all of this with calldata just like the comment in test_calldataload.py, and also parametrize this function to test calldatasize at the top-level call (returns len(tx.data)) or a sub-level call (returns arg_size).

@marioevz I simplified it a bit, but I'm pretty sure it's still checking correctly. Anything I'm missing?

winsvega · 2025-04-07T10:44:38Z

lets add push32, mstore8, add opcodes to default opcodes exempt from coverage check requirements.
it is in the file tests/homestead/coverage/test_coverage.py

pacrob · 2025-04-08T00:13:10Z

lets add push32, mstore8, add opcodes to default opcodes exempt from coverage check requirements. it is in the file tests/homestead/coverage/test_coverage.py

Not all fixed, but better. Thanks!

before:

ERROR: coverage is lost!
Lost coverage Lines: 34
Lost coverage function Paths: 11

after:

ERROR: coverage is lost!
Lost coverage Lines: 33
Lost coverage function Paths: 3

pacrob · 2025-04-09T19:51:28Z

Latest coverage fail:

Lost coverage Lines: 56
Lost coverage function Paths: 9

Looking into it:

expected:

Lost 22 lines and 6 functions from lib/evmone due to no longer using keccak or calldataload in the calldatasize test
Lost 7 lines test/state/host.cpp, 3 lines and one function in test/state/host.cpp due to not including a value in the transaction in the calldatasize test.

unknown:

Lost 2 lines in evmc/include/evmc, showing that the test is no longer checking Prague. Why might that be happening?
Leaving 22 lines and 2 functions across mpt.cpp, state.cpp, and system_contracts.cpp. Not sure of source.

I'll keep poking, but if you know whether the last 2 can be fixed or should be ignored, let me know!

winsvega · 2025-04-10T13:40:56Z

About sha3

I don't get what are the inputs for call in Oris test.
I suggest we debug original test, derrive the inputs and reproduce in python.

marioevz

Hey, some comments, sorry about the back and forth but hopefully this is the last one :)

converted-ethereum-tests.txt

docs/CHANGELOG.md

tests/frontier/opcodes/test_calldataload.py

marioevz · 2025-05-01T15:11:30Z

tests/frontier/opcodes/test_calldataload.py

+    address_b = pre.deploy_contract(
+        Om.MSTORE(calldata, 0x0)
+        + Op.CALL(
+            gas=Op.SUB(Op.GAS(), 0x100),
+            address=address_a,
+            value=0x0,
+            args_offset=0x0,
+            args_size=len(calldata),
+            ret_offset=0x0,
+            ret_size=0x0,
+        )
+    )
+
+    to = pre.deploy_contract(
+        code=(
+            Op.ADD(0x1000, Op.CALLDATALOAD(offset=0x4))
+            + Op.CALL(
+                gas=Op.SUB(Op.GAS(), 0x100),
+                address=address_b,
+                value=0x0,
+                args_offset=0x0,
+                args_size=0x0,
+                ret_offset=0x0,
+                ret_size=0x0,
+            )
+            + Op.STOP
+        ),
+    )


Suggested change

address_b = pre.deploy_contract(

Om.MSTORE(calldata, 0x0)

+ Op.CALL(

gas=Op.SUB(Op.GAS(), 0x100),

address=address_a,

value=0x0,

args_offset=0x0,

args_size=len(calldata),

ret_offset=0x0,

ret_size=0x0,

)

)

to = pre.deploy_contract(

code=(

Op.ADD(0x1000, Op.CALLDATALOAD(offset=0x4))

+ Op.CALL(

gas=Op.SUB(Op.GAS(), 0x100),

address=address_b,

value=0x0,

args_offset=0x0,

args_size=0x0,

ret_offset=0x0,

ret_size=0x0,

)

+ Op.STOP

),

)

to = pre.deploy_contract(

Om.MSTORE(calldata, 0x0)

+ Op.CALL(

gas=Op.SUB(Op.GAS(), 0x100),

address=address_a,

value=0x0,

args_offset=0x0,

args_size=len(calldata),

ret_offset=0x0,

ret_size=0x0,

)

)

Can be simplified like this.

It might result in a drop in coverage because we eliminate the use of the ADD opcode, but it's not the focus of the test so it's ok.

Reason behind these extra contracts in the original test is because the original yml hardcoded many contracts in the same test, but it's no longer necessary.

We could also do a variant of the test where the entry point is address_a to verify that Op.CALLDATALOAD behaves correctly when it's fetching data from the transaction's calldata.

@pytest.mark.parametrize("calldata_source", ["tx", "contract"])

And if calldata_source == "tx":

tx = Transaction( data=calldata, gas_limit=100_000, protected=fork >= Byzantium, sender=pre.fund_eoa(), to=address_a, )

And the to contract is omitted entirely.

marioevz · 2025-05-01T15:16:46Z

tests/frontier/opcodes/test_calldatasize.py

+    to = pre.deploy_contract(
+        code=(
+            Om.MSTORE(b"\x01" * args_size, 0x0)
+            + Op.CALL(
+                gas=Op.SUB(Op.GAS(), 0x100),
+                address=address,
+                value=0x0,
+                args_offset=0x0,
+                args_size=args_size,
+                ret_offset=0x0,
+                ret_size=0x0,
+            )
+        )
+    )


Same comment as the other test where we can parametrize by calldata_source.

pacrob · 2025-05-05T19:33:36Z

Hey, @marioevz, I've addressed your recent feedback. Let me know if you have more!

marioevz

LGTM, thanks!

marioevz · 2025-05-08T23:16:49Z

Checking the lost coverage and it's due of the following reasons:

Removal of SHA3 opcode.
Account deletion code branches.
Value transferring code branches.
The lost coverage is not relevant to the test, so we can safely proceed to merge.

…#1236)

pacrob force-pushed the calldata-tests branch from 89f3e94 to 5344526 Compare March 19, 2025 17:41

pacrob mentioned this pull request Mar 19, 2025

remove calldataload and calldatasize tests ethereum/tests#1477

Merged

pacrob force-pushed the calldata-tests branch 4 times, most recently from 91c493a to f6a91be Compare March 19, 2025 21:44

pacrob marked this pull request as ready for review March 19, 2025 21:45

winsvega reviewed Mar 20, 2025

View reviewed changes

pacrob force-pushed the calldata-tests branch from f6a91be to bfe80f7 Compare March 21, 2025 19:48

marioevz reviewed Mar 27, 2025

View reviewed changes

pacrob force-pushed the calldata-tests branch 3 times, most recently from eb7658b to 982f52f Compare April 4, 2025 20:34

pacrob force-pushed the calldata-tests branch 2 times, most recently from 13ce646 to b07e05c Compare April 7, 2025 23:55

spencer-tb changed the title ~~converting calldataload and calldatasize tests~~ feat(tests): converting calldataload and calldatasize tests Apr 14, 2025

spencer-tb added scope:tests Scope: Changes EL client test cases in `./tests` port Related to porting ethereum/tests to EEST type:feat type: Feature and removed port Related to porting ethereum/tests to EEST labels Apr 14, 2025

marioevz reviewed May 1, 2025

View reviewed changes

pacrob force-pushed the calldata-tests branch 2 times, most recently from 6e2c9fa to e67cdd5 Compare May 5, 2025 18:56

pacrob force-pushed the calldata-tests branch from e67cdd5 to 47abd75 Compare May 5, 2025 19:00

convert calldataload and calldatasize tests

96a761a

pacrob force-pushed the calldata-tests branch from 47abd75 to 96a761a Compare May 5, 2025 19:02

marioevz approved these changes May 8, 2025

View reviewed changes

marioevz merged commit 244852f into ethereum:main May 8, 2025
11 of 12 checks passed

pacrob deleted the calldata-tests branch May 9, 2025 12:09

felix314159 pushed a commit to felix314159/execution-spec-tests that referenced this pull request May 12, 2025

feat(tests): converting calldataload and calldatasize tests (ethereum…

a56ab7a

…#1236)

felix314159 pushed a commit to felix314159/execution-spec-tests that referenced this pull request May 16, 2025

feat(tests): converting calldataload and calldatasize tests (ethereum…

59d79a1

…#1236)

danceratopz mentioned this pull request May 22, 2025

feat(tests): port ethereum/tests CALLDATALOAD tests #1248

Closed

8 tasks

codeofcarson pushed a commit to codeofcarson/execution-spec-tests that referenced this pull request Jul 1, 2025

feat(tests): converting calldataload and calldatasize tests (ethereum…

66e7931

…#1236)

	("0002", "01", Account(storage={0x00: 0x02})),
	("0002", "01", 0x02),

feat(tests): converting calldataload and calldatasize tests #1236

feat(tests): converting calldataload and calldatasize tests #1236

Uh oh!

Conversation

pacrob commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🗒️ Description

🔗 Related Issues

✅ Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winsvega commented Mar 20, 2025

Uh oh!

pacrob commented Mar 21, 2025

Uh oh!

marioevz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winsvega commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pacrob commented Apr 8, 2025

Uh oh!

pacrob commented Apr 9, 2025

Uh oh!

winsvega commented Apr 10, 2025

Uh oh!

marioevz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pacrob commented May 5, 2025

Uh oh!

marioevz left a comment

Choose a reason for hiding this comment

Uh oh!

marioevz commented May 8, 2025

Uh oh!

Uh oh!

Uh oh!

feat(tests): converting `calldataload` and `calldatasize` tests #1236

feat(tests): converting `calldataload` and `calldatasize` tests #1236

pacrob commented Feb 19, 2025 •

edited

Loading

winsvega commented Apr 7, 2025 •

edited

Loading