refactor(tests): confirm migration of EOF rjump tests #1031

chfast · 2024-12-18T20:48:38Z

🗒️ Description

The EOF validation tests related to rjumps (EIP-4200) are already here. This change confirms the migration is completed and updates the EOF test tracker.

There is some additional refactoring done in the tests to trigger the test coverage CI job.

✅ 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 migrated EOF RJUMP validation tests tests#1421
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.

Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.

Files not reviewed (1)

converted-ethereum-tests.txt: Language not supported

shemnon · 2024-12-19T16:51:52Z

tests/osaka/eip7692_eof_v1/eip4200_relative_jumps/test_rjump.py

-        data=Container(
-            sections=[Section.Code(code=Op.RJUMP + Op.STOP)],
-        ),
+        data=Container.Code(Op.RJUMP + Op.STOP),


Not wild about this notation for a truncated instruction tests as it implies an opcode, but truncation tests are about broken conventions anyway, so I'll not complain too loundly.

What is good replacement? Op.RJUMP + "00"?

It did cause confusion for me at first glance. What about allowing arbitrary data portions with when argument in the square brackets is already a bytes-like object. E.g.

Op.RJUMP[0] -> 0xef0000

Op.RJUMP[b"\0"] -> 0xef00

Op.RJUMP[b""] -> 0xef

Not for this PR anyway because I'm not sure how many tests such a change would break.

Fine to me.

Allowing verbatim hex in the code is also good to me. E.g. we sometimes need to use undefined opcodes.

verbatim hex is great. bytes-like as literal args is also good.

This works currently: Op.RJUMP + Bytecode(b"\x00"). Should I change it?

Op.RJUMP + Bytecode(b"\x00")

Yes I think this is our best choice at the moment.

Separately, I'm going to prepare the PR to allow verbatim bytes, but it's going to take me a bit to make sure it does not break existing tests.

Ok. I will update these tests with Bytecode(b"\x00").

I can also try the separate PR mentioned. Is this about overloading + operator?

Ah, actually the Bytecode(b"\x00") is broken because Bytecode tries to read some metadata of the argument.

What works is: bytes(Op.RJUMP) + b"\x00"
?

marioevz

LGTM, thanks for the changes!

marioevz · 2024-12-19T17:27:54Z

tests/osaka/eip7692_eof_v1/eip4200_relative_jumps/test_rjump.py

-        data=Container(
-            sections=[Section.Code(code=Op.RJUMP + Op.STOP)],
-        ),
+        data=Container.Code(Op.RJUMP + Op.STOP),


It did cause confusion for me at first glance. What about allowing arbitrary data portions with when argument in the square brackets is already a bytes-like object. E.g.

Op.RJUMP[0] -> 0xef0000

Op.RJUMP[b"\0"] -> 0xef00

Op.RJUMP[b""] -> 0xef

Not for this PR anyway because I'm not sure how many tests such a change would break.

marioevz · 2024-12-19T17:30:35Z

tests/osaka/eip7692_eof_v1/eip4200_relative_jumps/test_rjump.py

@@ -609,7 +503,7 @@ def test_rjump_into_eofcreate(
        data=Container(
            sections=[
                Section.Code(
-                    code=Op.RJUMP[1] + Op.EOFCREATE[0](0, 0, 0, 0) + Op.STOP,
+                    code=Op.PUSH0 * 4 + Op.RJUMP[1] + Op.EOFCREATE[0] + Op.STOP,


Nice, I think this makes more sense.

It was actually a bug. Previously it jumped to the first PUSH data before EOFCREATE. Also jumping over PUSHes changes stack height. But the main issue is that it is hard to tell that (0, 0, 0, 0) are actually 4 PUSH1 instructions. And maybe they will be changed to PUSH0 in some future update?

There are similar tests for RJUMPI and RJUMPV but without a bug. Should I got and change them too?

Yes I think it's a good opportunity, this is simply a much better approach, and it's much more resilient too.

The EOF validation tests related to rjumps (EIP-4200) are already here. This change confirms the migration is completed and updates the EOF test tracker. There is some additional refactoring done in the tests to trigger the test coverage CI job.

marioevz · 2024-12-20T16:08:32Z

tests/osaka/eip7692_eof_v1/eip4200_relative_jumps/test_rjump.py

-        data=Container(
-            sections=[Section.Code(code=Op.RJUMP + Op.STOP)],
-        ),
+        data=Container.Code(Op.RJUMP + Op.STOP),


Op.RJUMP + Bytecode(b"\x00")

Yes I think this is our best choice at the moment.

Separately, I'm going to prepare the PR to allow verbatim bytes, but it's going to take me a bit to make sure it does not break existing tests.

chfast added scope:tests Scope: Test cases type:test Type: Test labels Dec 18, 2024

chfast force-pushed the eof/migrate_rjumps branch 2 times, most recently from 5f149f9 to 5b11203 Compare December 18, 2024 22:25

chfast requested a review from Copilot December 18, 2024 22:26

Copilot AI reviewed Dec 18, 2024

View reviewed changes

chfast marked this pull request as ready for review December 18, 2024 22:29

chfast force-pushed the eof/migrate_rjumps branch from 5b11203 to 6c18a1f Compare December 19, 2024 07:59

pdobacz approved these changes Dec 19, 2024

View reviewed changes

chfast force-pushed the eof/migrate_rjumps branch from 6c18a1f to 1728142 Compare December 19, 2024 15:05

chfast requested review from shemnon and marioevz December 19, 2024 15:19

shemnon approved these changes Dec 19, 2024

View reviewed changes

marioevz approved these changes Dec 19, 2024

View reviewed changes

chfast force-pushed the eof/migrate_rjumps branch from 1728142 to 081bacb Compare December 19, 2024 18:16

marioevz approved these changes Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(tests): confirm migration of EOF rjump tests #1031

refactor(tests): confirm migration of EOF rjump tests #1031

chfast commented Dec 18, 2024 •

edited

Loading

shemnon Dec 19, 2024

chfast Dec 19, 2024

marioevz Dec 19, 2024 •

edited

Loading

chfast Dec 19, 2024

shemnon Dec 19, 2024 •

edited

Loading

chfast Dec 20, 2024

marioevz Dec 20, 2024

chfast Dec 20, 2024

chfast Dec 20, 2024

marioevz left a comment

marioevz Dec 19, 2024 •

edited

Loading

marioevz Dec 19, 2024

chfast Dec 19, 2024

marioevz Dec 19, 2024

chfast Dec 19, 2024

marioevz Dec 20, 2024

refactor(tests): confirm migration of EOF rjump tests #1031

Are you sure you want to change the base?

refactor(tests): confirm migration of EOF rjump tests #1031

Conversation

chfast commented Dec 18, 2024 • edited Loading

🗒️ Description

✅ Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marioevz Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shemnon Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marioevz left a comment

Choose a reason for hiding this comment

marioevz Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented Dec 18, 2024 •

edited

Loading

marioevz Dec 19, 2024 •

edited

Loading

shemnon Dec 19, 2024 •

edited

Loading

marioevz Dec 19, 2024 •

edited

Loading