✨ feat(tests): PUSH*

[raxhvl]

🗒️ Description

Introduces tests for PUSH* opcodes ported from ethereum/tests.

🔗 Related Issues

closes #974

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

[raxhvl]

This snippet appears throughout the codebase. I would like to refactor it into a helper function to avoid repetition, but I fear this to be a pedantic optimization. Developers unaware of the helper function might end up repeating this code.

This snippet also reveals a fact: a transaction is a function of the fork in which its executed, since forks can influence how a transaction behaves. I believe pytest Transaction class should have a fork field, which would allow the class to abstract fork-specific transaction mutations, like this one. I’m noting this for what it’s worth, not suggesting we should take action on it right now.

[danceratopz]

Good observation, a possible solution could to make tx a pytest fixture (as pre is right now) which would allow automatic setting of the protected field based on the value of fork (which is also a pytest fixture). tx would have to be specified in the function arguments, as is pre right now.

I would like to add this functionality anyway, I think it's a possible solution that would allow generalization of transactions for other chains, unlocking EEST for L2 EVM testing.

[raxhvl]

Thats a nice solution. I'm noting things that feels against the grain as I port more tests. Should we create an issue for this?

[raxhvl]

I don't understand how the contents of converted-ethereum-tests.txt is ordered. Do I just add this at the end of the file?

[danceratopz]

I don't understand how the contents of converted-ethereum-tests.txt is ordered. Do I just add this at the end of the file?

Yup, just tack it on at the end 🙃 This is the current limitation that I mentioned in the call. We could make this file structured instead of using plain text, but I think an even better solution would be to add a ported_from mark, e.g.,

@pytest.mark.ported_from("src/GeneralStateTestsFiller/VMTests/vmTests/pushFiller.yml", "ref=ff138a31ffd109b5ef0d5dd735a8914a60d95fe7")

This would even allow us to create a dedicated section in the docs, for example, that lists tests ported from ethereum/tests.

Open to suggestions on this. I think the only source of ported tests would be ethereum/tests? But we could think about adding other metadata tags such as "fuzz", "gentest", "bug", etc.

[raxhvl]

This would even allow us to create a dedicated section in the docs, for example, that lists tests ported from ethereum/tests.

Yep! This would be really nice.

Open to suggestions on this. I think the only source of ported tests would be ethereum/tests? But we could think about adding other metadata tags such as "fuzz", "gentest", "bug", etc.

Perhaps we can add a generic "meta" marker that lets you add arbitrary metadata?

[danceratopz]

@raxhvl: Sometimes it can be tricky to get coverage parity, please reach out to @winsvega if you can't see anything obvious and need some pointers.

An example, although not applicable here, is if you port a YAML test with Yul Code to a Python test using Opcodes. In this case solc can optimize stack items increasing the "coverage" of the YAML-based test:

execution-spec-tests/tests/homestead/coverage/test_coverage.py

Lines 16 to 29 in 5cf1f24

	def test_coverage(
	state_test: StateTestFiller,
	pre: Alloc,
	fork: Fork,
	):
	"""
	This test covers gaps that result from transforming Yul code into
	`ethereum_test_tools.vm.opcode.Opcodes` bytecode.
	E.g. Yul tends to optimize stack items by using `SWAP1` and `DUP1` opcodes, which are not
	regularly used in python code.
	Modify this test to cover more Yul code if required in the future.
	"""

[raxhvl]

Thanks @danceratopz

Let me give it a shot. I'm sure this will take some time, but I hope to pick up some learnings for future reference. If I get to a standstill, I will reach out to @winsvega.

[raxhvl]

I spent last week trying to understand how coverage works, and why its failing here.

TL;DR: The current test uses fewer opcodes to achieve the same result.

Previous strategy

The base test is parameterized using calldata. Here is a simplified pseudocode of how it works:

for calldata in [0..1f]:
    call(
        gas=gas(), address=add(0x1000,calldata(4)), value=0, argOffset=0, argSize=0,
        returnOffset=0, returnSize=0
    )

Full bytecode:

[00]	PUSH1	00
[02]	PUSH1	00
[04]	PUSH1	00
[06]	PUSH1	00
[08]	PUSH1	00
[0a]	PUSH1	04
[0c]	CALLDATALOAD	
[0d]	PUSH2	1000
[10]	ADD	
[11]	GAS	
[12]	CALL	
[13]	STOP	

In this setup, the each calldata is mapped to a contract address, which focuses on a single PUSH opcode. The to address then routes each test case to the appropriate contract.

Call data containing a (redundant) function selector + destination contract address:

0x693c61390000000000000000000000000000000000000000000000000000000000000000

Current strategy

In the current approach, a unique contract is deployed for each PUSH opcode at the to address, eliminating the need for routing. Parameterization is handled directly by pytest.

Why coverage fails

The absence of a routing mechanism in the current test causes coverage failures. The simplified structure of the test means that opcodes like CALL, ADD, and CALLDATALOAD are no longer required. For example, the following coverage failure occurs due to the missing calldataload call:

This is inherently because of the difference in test strategies: parameterization using EVM vs parameterization using pytest.

Larger picture

The base test employs a relatively complex strategy. Two areas of complexity include:

1. The calldata includes a function selector that is never used.
2. The calldata computes the destination contract address dynamically using an offset, which could be simplified by hardcoding the address.

Moreover, we should aim to test with a minimal set of opcodes—in this case, PUSH* and SSTORE—which would simplify the tests, make them faster, and avoid any potential unintended side effects.

With pytest now handling parameterization, the complex routing logic is no longer necessary.

I'm still very new to this, so its quite likely that I'm missing an important piece here. But this is my understanding of the problem.

cc: @winsvega @danceratopz @marioevz

[winsvega]

Yes, we have already some basic coverage that is applied by default. In test_all_opcodes.py.

But the goal of coverage here is to check that at least no lines lost after converting the test.

Reading the coverage report already helped me to find out a few bugs where I relied that code works, but it didn't