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(fw): add missing retesteth exceptions #572

Merged
merged 1 commit into from
Jun 3, 2024
Merged

feat(fw): add missing retesteth exceptions #572

merged 1 commit into from
Jun 3, 2024

Conversation

winsvega
Copy link
Collaborator

@winsvega winsvega commented May 27, 2024

πŸ—’οΈ Description

Convert retesteth exceptions into pyspec format
Adjust the .json fillers accrodingly for consume command

πŸ”— 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: 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.

@winsvega winsvega requested a review from danceratopz May 27, 2024 10:55
@winsvega winsvega added the type:test Type: Test label May 27, 2024
@winsvega winsvega marked this pull request as draft May 27, 2024 10:56
@danceratopz
Copy link
Member

Hey @winsvega, do you have a corresponding branch in ethereum/tests that uses these exceptions?

I think the end goal for this PR should be able to load every fixture from https://github.com/ethereum/tests/tree/2e37a9f41167534b07e0e8f247ea934a5fe3cac9/BlockchainTests into an execution-spec-tests blockchain pydantic model (Fixture from ethereum_test_tools.spec.blockchain.types). In particular, the tests from BlockchainTests/InvalidBlocks.

This can be tested by generating a test index file for ethereum/tests, i.e.,

genindex -i ethereum/tests/BlockchainTests/

Once we can load all these fixtures and generate an index file, we can then execute ethereum/tests fixtures against clients via consume direct and consume rlp πŸš€

Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

Looks good to me. As mentioned in the comment above I think we should test this against ethereum/tests before merge.

This would mean preparing a branch of ethereum/tests with the new EEST exceptions changed, for example:

diff --git a/BlockchainTests/InvalidBlocks/bcInvalidHeaderTest/DifficultyIsZero.json b/BlockchainTests/InvalidBlocks/bcInvalidHeaderTest/DifficultyIsZero.json
index b90de9907d..f0f208e8c9 100644
--- a/BlockchainTests/InvalidBlocks/bcInvalidHeaderTest/DifficultyIsZero.json
+++ b/BlockchainTests/InvalidBlocks/bcInvalidHeaderTest/DifficultyIsZero.json
@@ -14,7 +14,7 @@
             {
                 "blocknumber" : "1",
                 "chainname" : "default",
-                "expectException" : "InvalidDifficulty",
+                "expectException" : "BlockException.INVALID_DIFFICULTY",
                 "rlp" : "0xf90261f901f6a0aa6c733b954fb03814267ff4e698d18b5dd210aa85e7457d856718c43fa4d5f9a01dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347948888f1f195afa192cfee860698584c030f4c9db1a0edf42f78d05a438f6408aef357002a7380f4aca2587645d674498efc64705bd4a0e7e93016861d480e9fd60fc3dfd935eff097aa94c7380ed8df2ca7ec5b578214a021f4ebc5b0fb1ad80a00f78d04e67d1b30af68cecb3a8a2bf55247f11df1e3edb90100000000000000000010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000200000000000000000008000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000200000000000400000000000000000000000000000000000000000000000000000008001832fefd882560b8454c9906942a00000000000000000000000000000000000000000000000000000000000000000880000000000000000f865f863808203e882c35094095e7baea6a6c7c4c2dfeb977efac326af552d87821388801ca0742e5cbd6ff6fedbc62821d019c5809bf620cba582f2dd33c23d3ca8956bd117a039c7a217fe45c3a6aa2e2138311af6eef76b571dc7523dd1745112bcc44851abc0",
                 "rlp_decoded" : {
                     "blockHeader" : {
@@ -251,7 +251,7 @@
             {
                 "blocknumber" : "1",
                 "chainname" : "default",
-                "expectException" : "InvalidDifficulty",
+                "expectException" : "BlockException.INVALID_DIFFICULTY",
                 "rlp" : "0xf90261f901f6a0aa6c733b954fb03814267ff4e698d18b5dd210aa85e7457d856718c43fa4d5f9a01dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347948888f1f195afa192cfee860698584c030f4c9db1a0edf42f78d05a438f6408aef357002a7380f4aca2587645d674498efc64705bd4a0e7e93016861d480e9fd60fc3dfd935eff097aa94c7380ed8df2ca7ec5b578214a021f4ebc5b0fb1ad80a00f78d04e67d1b30af68cecb3a8a2bf55247f11df1e3edb90100000000000000000010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000200000000000000000008000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000200000000000400000000000000000000000000000000000000000000000000000008001832fefd882560b8454c9906942a00000000000000000000000000000000000000000000000000000000000000000880000000000000000f865f863808203e882c35094095e7baea6a6c7c4c2dfeb977efac326af552d87821388801ca0742e5cbd6ff6fedbc62821d019c5809bf620cba582f2dd33c23d3ca8956bd117a039c7a217fe45c3a6aa2e2138311af6eef76b571dc7523dd1745112bcc44851abc0",
                 "rlp_decoded" : {
                     "blockHeader" : {

I also wondered if you wanted to preserve the exception <-> string mappings that retesteth has? Similar to what we added for EOF exceptions in #519 (src/ethereum_test_tools/exceptions/evmone_exceptions.py)?

src/ethereum_test_tools/exceptions/exceptions.py Outdated Show resolved Hide resolved
src/ethereum_test_tools/exceptions/exceptions.py Outdated Show resolved Hide resolved
@danceratopz danceratopz changed the title move retesteth exceptions into pyspecs feat(fw): add missing retesteth exceptions May 29, 2024
@danceratopz danceratopz added type:feat type: Feature scope:fw Scope: Framework (evm|tools|forks|pytest) and removed type:test Type: Test labels May 29, 2024
@winsvega winsvega force-pushed the re_exceptions branch 2 times, most recently from eb995b0 to 813cce3 Compare May 30, 2024 08:12
Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

LGTM!

Ideally we should put some warning somewhere to remind us that we don't have any test that triggers some of these exceptions.

@winsvega
Copy link
Collaborator Author

We have with consume rlp existing tests .jsons

@winsvega winsvega marked this pull request as ready for review June 3, 2024 13:13
@marioevz marioevz merged commit ddafeac into main Jun 3, 2024
9 checks passed
@danceratopz
Copy link
Member

Thanks a lot for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:fw Scope: Framework (evm|tools|forks|pytest) type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants