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

Testing setup #36

Merged
merged 12 commits into from
Dec 4, 2024
Merged

Testing setup #36

merged 12 commits into from
Dec 4, 2024

Conversation

debjit-bw
Copy link
Member

Adds necessary handlers and drivers for testing with ethereum/tests and the new eels tests.

  • We are skipping tests targeted at the following forks:
    • Frontier
    • Homestead
    • Byzantium
    • ByzantiumToConstantinopleAt5
    • Istanbul
    • Berlin
    • London
    • Constantinople
    • ConstantinopleFix
    • MergeEOF
    • MergeMeterInitCode
    • MergePush0

This DOES NOT mean that the features introduced at these forks are untested. Any feature introduced have tests targeted at all the forks after that, and we ARE testing for the fork "Merge" and later, skipping the ones before it.

  • We are directly using the tests generated for Ethereum, since we don't currently have the tests generated for Gnosis.
    • Therefore, several tests are failing, particularly 1559, withdrawals etc.
    • For most tests however, Gnosis and Mainnet work similarly.
    • 53 out of the 71 tests (combining eest and ethereum/tests) are PASSING. For now I have feature-gated the failing tests.
    • 18 tests are inside failing-tests feature (25%).

This PR is targeted mostly at setting up the code needed to run the EELS or ethereum tests. The tests themselves will be kept updated as and when Gnosis tests are available. The tests currently passing is just to prove that we aren't doing anything wildly wrong.

@debjit-bw debjit-bw marked this pull request as ready for review December 3, 2024 18:46
| "eip1559.json"
| "mergeTest.json"

// These tests are passing, but they take a lot of time to execute so we are going to skip them.
Copy link
Member

Choose a reason for hiding this comment

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

How long for curiosity?

Copy link
Member Author

Choose a reason for hiding this comment

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

as long as it's running now (10 mins)
we don't exclude any tests now (not the false &&)

general_state_test!(st_stack, stStackTests);
general_state_test!(st_static_call, stStaticCall); // passing, but conflicts with rewards contract
general_state_test!(st_transaction, stTransactionTest);
general_state_test!(vm_tests, VMTests);
Copy link
Member

Choose a reason for hiding this comment

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

All of these should fail? Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

apart from eip 1559, withdrawals, tests rn fail for 2 main reasons:

  • wrong impl on our end
  • the test "should fail" and it is faililng, but the exact error string in reth (ExecutionStageError.MaxGasReached) is different than in exec specs (MAX_GAS_REACHED, for eg)

Copy link
Member

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Awesome!! Love this addition, good progress

@dapplion
Copy link
Member

dapplion commented Dec 4, 2024

I'll merge as is, we can work on passing more tests in another branch

@dapplion dapplion merged commit 7265c54 into consistent-client-behaviour Dec 4, 2024
7 checks passed
@dapplion dapplion deleted the testing-setup branch December 4, 2024 11:38
- name: Downloading ethereum/tests
run: git clone https://github.com/ethereum/tests ethereum-tests
- name: Downloading EELS fixtures released at Cancun
run: curl -LO https://github.com/ethereum/execution-spec-tests/releases/download/v2.1.1/fixtures.tar.gz && tar -xzf fixtures.tar.gz
Copy link
Member

Choose a reason for hiding this comment

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

Was going to suggest caching, but this is very fast already

Copy link
Member Author

Choose a reason for hiding this comment

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

the curl -LO is done within a sec, but git clone does take some time tbh

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.

2 participants