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): Implement Bytecode class #631

Merged
merged 8 commits into from
Jun 20, 2024
Merged

feat(fw): Implement Bytecode class #631

merged 8 commits into from
Jun 20, 2024

Conversation

marioevz
Copy link
Member

🗒️ Description

Changes included:

  • Implement Bytecode class, which is the new parent of Opcode and Macro.
  • The output of all opcode operations is now the new Bytecode class (instead of bytes output), and the operation also performs the correct calculation of popped_stack_items, pushed_stack_items, max_stack_height and min_stack_height for the compounded output.
    • E.g. on a Op.PUSH1[1] + Op.PUSH1[1] operation, the output is a Bytecode instance with the following properties: popped_stack_items = 0, pushed_stack_items = 2, max_stack_height = 2, min_stack_height = 2, and the new max_stack_height can be imported by the EOF container automatically. More examples:
  • b"".join(Op.PUSH1[x] for x in range(123)) patterns are no longer valid (because the sum of two opcodes is no longer bytes, but Bytecode) and should simply be rewritten as sum(Op.PUSH1[x] for x in range(123)). This PR refactors most of the tests.
  • Removes max_stack_height from most Section.Code usages, but there are some instances where it could not be removed since Bytecode calculated the incorrect maximum height (particularly when CALLF is involved).

As is, the branch of this PR generates a 1-to-1 fixture output of the following commands when compared against main:

fill -n auto --until=Prague
fill --fork=CancunEIP7692 --evm-bin=evmone-t8n -n auto tests/prague/eip7692_eof_v1/

🔗 Related Issues

None

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

@marioevz marioevz added scope:tests Scope: Test cases type:refactor Type: Refactor type:feat type: Feature scope:fw Scope: Framework (evm|tools|forks|pytest) labels Jun 17, 2024
@marioevz marioevz requested a review from spencer-tb June 17, 2024 22:07
@marioevz
Copy link
Member Author

@shemnon @chfast @gumb0 please take a look.

The idea is that max_stack_height should no longer be a blocker to auto-convert any test to EOF, but we still need to figure out the deprecated opcodes.

@marioevz
Copy link
Member Author

Will be used to improve #610

@marioevz marioevz force-pushed the implement-bytecode branch from 5e89735 to 26d4729 Compare June 20, 2024 13:42
@marioevz
Copy link
Member Author

Currently the generated fixtures are different from the main fixtures, and this is the diff to make them exactly equal, but I think the changes in this PR are indeed correct, and tests were incorrectly setting the max stack height of the invalid containers (but uncaught because the issue under test is hit first).

diff --git a/tests/prague/eip7692_eof_v1/eip3540_eof_v1/container.py b/tests/prague/eip7692_eof_v1/eip3540_eof_v1/container.py
index e6da71462..d86070520 100644
--- a/tests/prague/eip7692_eof_v1/eip3540_eof_v1/container.py
+++ b/tests/prague/eip7692_eof_v1/eip3540_eof_v1/container.py
@@ -265,7 +265,7 @@ INVALID: List[Container] = [
     Container(
         name="no_section_terminator_3",
         header_terminator=bytes(),
-        sections=[Section.Code(code=Op.PUSH1(0) + Op.STOP)],
+        sections=[Section.Code(code=Op.PUSH1(0) + Op.STOP, max_stack_height=0)],
         validity_error=EOFException.INVALID_SECTION_BODIES_SIZE,
     ),
     # The following cases just remove the terminator
@@ -306,13 +306,13 @@ INVALID: List[Container] = [
     Container(
         name="no_section_terminator_nonzero_3",
         header_terminator=b"04",
-        sections=[Section.Code(code=Op.PUSH1(0) + Op.STOP)],
+        sections=[Section.Code(code=Op.PUSH1(0) + Op.STOP, max_stack_height=0)],
         validity_error=EOFException.MISSING_TERMINATOR,
     ),
     Container(
         name="no_section_terminator_nonzero_4",
         header_terminator=b"fe",
-        sections=[Section.Code(code=Op.PUSH1(0) + Op.STOP)],
+        sections=[Section.Code(code=Op.PUSH1(0) + Op.STOP, max_stack_height=0)],
         validity_error=EOFException.MISSING_TERMINATOR,
     ),
     Container(
@@ -329,7 +329,7 @@ INVALID: List[Container] = [
     ),
     Container(
         name="trailing_bytes_after_code_section",
-        sections=[Section.Code(code=Op.PUSH1(0) + Op.STOP)],
+        sections=[Section.Code(code=Op.PUSH1(0) + Op.STOP, max_stack_height=0)],
         extra=bytes([0xDE, 0xAD, 0xBE, 0xEF]),
         validity_error=EOFException.INVALID_SECTION_BODIES_SIZE,
     ),
@@ -402,7 +402,7 @@ INVALID: List[Container] = [
         name="trailing_bytes_after_data_section",
         extra=bytes([0xEE]),
         sections=[
-            Section.Code(code=Op.PUSH1(0) + Op.STOP),
+            Section.Code(code=Op.PUSH1(0) + Op.STOP, max_stack_height=0),
             Section.Data(data="0xAABBCCDD"),
         ],
         # TODO should be more specific exception about trailing bytes
@@ -411,7 +411,7 @@ INVALID: List[Container] = [
     Container(
         name="multiple_data_sections",
         sections=[
-            Section.Code(code=Op.PUSH1(0) + Op.STOP),
+            Section.Code(code=Op.PUSH1(0) + Op.STOP, max_stack_height=0),
             Section.Data(data="0xAABBCC"),
             Section.Data(data="0xAABBCC"),
         ],
@@ -581,20 +581,20 @@ VALID += [
 INVALID += [
     Container(
         name="single_code_section_non_zero_inputs",
-        sections=[Section.Code(code=Op.POP + Op.RETF, code_inputs=1)],
+        sections=[Section.Code(code=Op.POP + Op.RETF, code_inputs=1, max_stack_height=0)],
         # TODO the exception must be about code or non, cause it looks legit
         validity_error=EOFException.INVALID_FIRST_SECTION_TYPE,
     ),
     Container(
         name="single_code_section_non_zero_outputs",
-        sections=[Section.Code(code=Op.PUSH0 + Op.RETF, code_outputs=1)],
+        sections=[Section.Code(code=Op.PUSH0 + Op.RETF, code_outputs=1, max_stack_height=0)],
         # TODO the exception must be about code or non, cause it looks legit
         validity_error=EOFException.INVALID_FIRST_SECTION_TYPE,
     ),
     Container(
         name="multiple_code_section_non_zero_inputs",
         sections=[
-            Section.Code(code=Op.POP + Op.RETF, code_inputs=1),
+            Section.Code(code=Op.POP + Op.RETF, code_inputs=1, max_stack_height=0),
             Section.Code(Op.STOP),
         ],
         # TODO the actual exception should be EOFException.INVALID_TYPE_BODY,
@@ -603,7 +603,7 @@ INVALID += [
     Container(
         name="multiple_code_section_non_zero_outputs",
         sections=[
-            Section.Code(code=Op.PUSH0, code_outputs=1),
+            Section.Code(code=Op.PUSH0, code_outputs=1, max_stack_height=0),
             Section.Code(Op.STOP),
         ],
         # TODO the actual exception should be EOFException.INVALID_TYPE_BODY,
diff --git a/tests/prague/eip7692_eof_v1/eip4200_relative_jumps/test_rjump.py b/tests/prague/eip7692_eof_v1/eip4200_relative_jumps/test_rjump.py
index 406f3f985..39ab58546 100644
--- a/tests/prague/eip7692_eof_v1/eip4200_relative_jumps/test_rjump.py
+++ b/tests/prague/eip7692_eof_v1/eip4200_relative_jumps/test_rjump.py
@@ -436,6 +436,7 @@ def test_rjump_into_dupn(
                     + Op.DUPN[1]
                     + Op.SSTORE
                     + Op.STOP,
+                    max_stack_height=2,  # FIXME: Might be 3
                 ),
             ],
         ),
diff --git a/tests/prague/eip7692_eof_v1/eip4200_relative_jumps/test_rjumpv.py b/tests/prague/eip7692_eof_v1/eip4200_relative_jumps/test_rjumpv.py
index 4a013ac1f..cbcdcda0c 100644
--- a/tests/prague/eip7692_eof_v1/eip4200_relative_jumps/test_rjumpv.py
+++ b/tests/prague/eip7692_eof_v1/eip4200_relative_jumps/test_rjumpv.py
@@ -659,7 +659,7 @@ def test_rjumpv_into_push_1(
         code = Op.PUSH1(1) + Op.RJUMPV[jump_table] + Op.STOP
     eof_test(
         data=Container(
-            sections=[Section.Code(code=code)],
+            sections=[Section.Code(code=code, max_stack_height=1)],
         ),
         expect_exception=EOFException.INVALID_RJUMP_DESTINATION,
     )
@@ -749,7 +749,9 @@ def test_rjumpv_into_push_n(
         code = opcode[1] + Op.RJUMPV[jump_table] + Op.STOP
     eof_test(
         data=Container(
-            sections=[Section.Code(code=code)],
+            sections=[
+                Section.Code(code=code, max_stack_height=1),
+            ],
         ),
         expect_exception=EOFException.INVALID_RJUMP_DESTINATION,
     )

Copy link
Collaborator

@spencer-tb spencer-tb left a comment

Choose a reason for hiding this comment

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

LGTM!

@marioevz marioevz merged commit a3743c8 into main Jun 20, 2024
9 checks passed
spencer-tb pushed a commit that referenced this pull request Jun 26, 2024
* feat(fw): Create `Bytecode` class

* refactor(tests): pre-merge: use Bytecode

* refactor(tests): cancun tests - use Bytecode

* refactor(tests): shanghai tests - use Bytecode

* refactor(tests): prague tests - use Bytecode

* refactor(tests): EOF - use Bytecode

* refactor(tests): EOF - remove max_stack_height from most tests

* fix(tests): remove incorrect stack height fixme
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) scope:tests Scope: Test cases type:feat type: Feature type:refactor Type: Refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants