new(tests): EOF validation of opcodes #932
Conversation
| """ | ||
| Test that an invalid opcode placed after STOP (terminating instruction) invalidates EOF. | ||
| """ | ||
| eof_code = Container(sections=[Section.Code(code=Op.STOP + opcode), Section.Data("00" * 32)]) |
There was a problem hiding this comment.
this could be parametrized with all terminating instructions and an RJUMP[-3] too
| sorted(push_opcodes), | ||
| ) | ||
| @pytest.mark.parametrize( | ||
| "any_imm", |
There was a problem hiding this comment.
any_imm name threw me off the scent. Maybe "truncate_whole" ("truncate_one_byte") verbosely
There was a problem hiding this comment.
At the risk of test bloat, should we test all lengths within a push? It's over 500 tests but super easy to code.
There was a problem hiding this comment.
At the risk of test bloat, should we test all lengths within a push? It's over 500 tests but super easy to code.
I'd say pass to this.
There was a problem hiding this comment.
this particular combo of 2 tests begs to add 3rd test combining them: a truncated push after a terminating instr? Or is it too much?
| eof_code = Container(sections=[Section.Code(code=Op.STOP + opcode), Section.Data("00" * 32)]) | ||
| eof_test( | ||
| data=eof_code, | ||
| expect_exception=EOFException.UNDEFINED_INSTRUCTION, |
There was a problem hiding this comment.
not sure if we want to add this preemptively, but some impls could fail with UNREACHABLE_CODE and still make sense. But maybe better to keep simple, it is quite likely that instruction definedness is checked first.
There was a problem hiding this comment.
It should be possible to define a list of possible exceptions
There was a problem hiding this comment.
For a combined loop it makes sense to check if the code has been forward referenced before decoding the opcode. So it could easily be unreachable.
Which begs the question, should we test for all opcodes after a STOP, not just invalid ones? Is this a code reachability issue or a opcode validity issue?
There was a problem hiding this comment.
I originally read it as an opcode validity issue, unreachability is added as noise to confuse the eof. But it could be ofc generalized to a reachability issue, with opcode validity as a parameter. It depends on whether and where we have unreachable valid opcodes tested
tests/osaka/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py
Outdated
Show resolved
Hide resolved
|
Created ipsilon#4 with a suggestion, let me know if the changes look good to you. |
Add missing opcode tests: - invalid opcode placed after a STOP instruction, - PUSH opcodes with truncated immediate bytes. Mark rest of the "opcode tests" as done by providing links to tests.
chfast
force-pushed
the
eof/migrate_opcode_validation
branch
from
3d305f0
to
98d3b74
Compare
| opcode_with_data_portion: bytes = bytes(opcode[1]) | ||
|
|
||
| if len(opcode_with_data_portion) == 2 and trunc_all: | ||
| pytest.skip("can only be truncated one-way") |
There was a problem hiding this comment.
I don't think the use of pytest.skip is good here. We want to exclude some combination of parameters but don't want to print skip logs for these tests.
tests/osaka/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py::test_truncated_data_portion_opcodes[fork_Osaka-eof_test-compute_max_stack_height_True-trunc_all_True-opcode_CALLF] SKIPPED (max_stack_height is 0) [916/922]
tests/osaka/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py::test_truncated_data_portion_opcodes[fork_Osaka-eof_test-compute_max_stack_height_True-trunc_all_True-opcode_JUMPF] SKIPPED (max_stack_height is 0) [917/922]
tests/osaka/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py::test_truncated_data_portion_opcodes[fork_Osaka-eof_test-compute_max_stack_height_True-trunc_all_True-opcode_DUPN] SKIPPED (can only be truncated one-way) [918/922]
tests/osaka/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py::test_truncated_data_portion_opcodes[fork_Osaka-eof_test-compute_max_stack_height_True-trunc_all_True-opcode_SWAPN] SKIPPED (can only be truncated one-way) [919/922]
tests/osaka/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py::test_truncated_data_portion_opcodes[fork_Osaka-eof_test-compute_max_stack_height_True-trunc_all_True-opcode_EXCHANGE] SKIPPED (can only be truncated one-way) [920/922]
tests/osaka/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py::test_truncated_data_portion_opcodes[fork_Osaka-eof_test-compute_max_stack_height_True-trunc_all_True-opcode_EOFCREATE] SKIPPED (can only be truncated one-way) [921/922]
tests/osaka/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py::test_truncated_data_portion_opcodes[fork_Osaka-eof_test-compute_max_stack_height_True-trunc_all_True-opcode_RETURNCONTRACT] SKIPPED (can only be truncated one-way) [922/922]
🗒️ Description
Add missing opcode tests:
Mark rest of the "opcode tests" as done by providing links to tests.
✅ Checklist
mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.