Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ feat(tests): PUSH opcode tests added #1018

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

felix314159
Copy link

@felix314159 felix314159 commented Dec 12, 2024

πŸ—’οΈ Description

hi, this is my first PR for this repo. please let me know if the tests are faulty or not useful.

i still have a few questions:

  1. i ran fill --fork=Cancun ./tests/frontier/opcodes/test_push.py to get 99 passed. However, it also says 'No tests executed - the test fixtures in "fixtures/" may now be executed against a client'. How can I do this?
  2. i wanted to include 'negative tests' (e.g. PUSH4 value that requires more than 4 bytes) to see that it correctly fails, but I didn't know how to signal this in this Python script. I would also then have to somehow programmatically specify which failure reason is the 'correct' one. LMK if this is possible

πŸ”— Related Issues

Fixes #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.

@marioevz
Copy link
Member

1. i ran `fill --fork=Cancun ./tests/frontier/opcodes/test_push.py` to get 99 passed. However, it also says 'No tests executed - the test fixtures in "fixtures/" may now be executed against a client'. How can I do this?

The easiest way to do this would be to compile geth's evm tool, which can be done by cloning their repo, installing golang, then doing go build -o ./build/bin/evm ./cmd/evm/ to build the command, and then add the generated binary (./build/bin/evm) to the PATH.

Then you can use the consume command to run the fixtures you generated:

uv run consume direct --input ./fixtures/
2. i wanted to include 'negative tests' (e.g. PUSH4 value that requires more than 4 bytes) to see that it correctly fails, but I didn't know how to signal this in this Python script. I would also then have to somehow programmatically specify which failure reason is the 'correct' one. LMK if this is possible

Yes I think it's always interesting to add negative tests cases, but let me explain a bit on how these data-portion opcodes such as PUSH4 work in the EVM:

The way these opcodes work is that the EVM interpreter already knows how many bytes it needs to read after PUSH4 to interpret its data, so when it encounters the PUSH4 opcode it figures "the next four bytes are not other opcodes but data instead". So if you add a fifth byte after PUSH4 it would be interpreted as the next opcode instead of an oversized data.

However, the interesting thing would be to crop the bytecode of the smart contract so there's not enough bytes to complete the 4 bytes required by PUSH4 πŸ˜‰

@felix314159
Copy link
Author

Thanks for your reply! I tried running uv run consume direct --input ./fixtures/ but I get this error:
consume direct issue
I was able to work around this by running hive with hive --dev --client go-ethereum , setting the HIVE_SIMULATOR variable and then running uv run consume engine --input ./fixtures/, it now shows that 49 passed. My question here is why only 49 passed when it previously (when generating the fixtures) said that 99 passed. Are some of these tests not consumable by geth?

@marioevz
Copy link
Member

Thanks for your reply! I tried running uv run consume direct --input ./fixtures/ but I get this error

My bad! Reason is that the --evm-bin flag is missing, could you try adding --evm-bin /path/to/the/binary/evm with the correct path and see if that works.

I was able to work around this by running hive with hive --dev --client go-ethereum

Amazing! hive is one step further πŸ˜‰

it now shows that 49 passed. My question here is why only 49 passed when it previously (when generating the fixtures) said that 99 passed. Are some of these tests not consumable by geth?

Reason being that fill generates multiple kinds of tests to be run in different environments.

Hive consumes two types of tests:

  • Blockchain RLP, which is the blocks being fed to the client as serialized blocks, which is a similar format to what the client receives from other clients via P2P messages (when syncing for example).
  • Blockchain Engine, which is a JSON format encoded block that the client receives from the consensus client, and this typically happens when the client is already in sync with its peers and a new block is proposed by a validator as the new head of the chain. More info on this JSON format here https://github.com/ethereum/execution-apis/tree/main/src/engine

So when doing uv run consume engine ... only half of the produced fixtures are processed, while the others should be processed with uv run consume rlp ....

@felix314159
Copy link
Author

Sadly, I still can't get the --evm-bin flag to work. Seems to be an issue with a plugin:
consume evm bin issue

The uv run consume rlp ... worked well, I now ran the other tests successfully. πŸ‘

@danceratopz
Copy link
Member

Sadly, I still can't get the --evm-bin flag to work. Seems to be an issue with a plugin:

Hi @felix314159, could you try the following (note the addition of = between the flag and the arguments) and let me know if it works?

uv run consume direct --input ./fixtures/ --evm-bin=$HOME/Documents/go-ethereum/build/bin/evm

The uv run consume rlp ... worked well, I now ran the other tests successfully. πŸ‘

Nice!

@felix314159
Copy link
Author

Hi @felix314159, could you try the following (note the addition of = between the flag and the arguments) and let me know if it works?

uv run consume direct --input ./fixtures/ --evm-bin=$HOME/Documents/go-ethereum/build/bin/evm

Thanks, this works! What also works is uv run consume direct --input=./fixtures/ --evm-bin=$HOME/Documents/go-ethereum/build/bin/evm, so it's probably best for me to make a habit of consistently using '=' as separator 😊

@raxhvl
Copy link
Contributor

raxhvl commented Dec 13, 2024

FYI - #975 adds the same test.

@felix314159
Copy link
Author

FYI - #975 add the same test.

Good to know, but I would not call it the same test as you do not seem to test PUSH0.

@raxhvl
Copy link
Contributor

raxhvl commented Dec 13, 2024

Good to know, but I would not call it the same test as you do not seem to test PUSH0.

Thats correct. The test for PUSH0 has already been added.

Comment on lines 122 to 124
# in global scope so that it doesn't get re-created every iteration
# generate list of values that require x bytes, x+1 bytes, etc. to store
values = [0] + [get_largest_value_that_requires_at_least_x_bytes(i) for i in range(1, 33)]
Copy link
Member

@marioevz marioevz Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback, I pushed an improved version. Let me know if this still does not do what I originally intended it to do

@felix314159 felix314159 changed the title added test for PUSH opcodes ✨ feat(tests): PUSH opcode tests added Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(tests): port ethereum/tests test cases (tracking issue)
4 participants