feat(src): EIP-8024 tests added#2021
feat(src): EIP-8024 tests added#2021marioevz merged 22 commits intoethereum:eips/amsterdam/eip-8024from
Conversation
|
|
||
| """ | ||
| # STACK | ||
| pass |
There was a problem hiding this comment.
You also need to check stack height for potential stack overflow/underflow. Similarly, this was guaranteed to be safe by EOF, now it's wild west again. You should also quickly realize that you are not able to follow regular scheme: stack, gas, operation. You have to decode the immediate data before checking the stack.
| charge_gas(evm, GAS_VERY_LOW) | ||
|
|
||
| # OPERATION | ||
| immediate_data = evm.code[evm.pc + Uint(1)] |
There was a problem hiding this comment.
i appreciate that u take the time to review this but the feedback would be more helpful if you explained why something is wrong + what the fix is. ty for your attention to this matter
There was a problem hiding this comment.
I was commuting... here you need to check if the immediate_data is even there (without EOF you don't have such guarantees).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## eips/amsterdam/eip-8024 #2021 +/- ##
===========================================================
+ Coverage 86.16% 86.33% +0.16%
===========================================================
Files 599 599
Lines 39527 39578 +51
Branches 3780 3789 +9
===========================================================
+ Hits 34059 34170 +111
+ Misses 4847 4817 -30
+ Partials 621 591 -30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
spencer-tb
left a comment
There was a problem hiding this comment.
Feels good to me so far. Do we need to add the decode functions to packages/testings or import them from EELS:
def decode_single(x: int) -> int:
"""EIP-8024 decode for DUPN/SWAPN."""
if not (0 <= x <= 90 or 128 <= x <= 255):
return 0 # Invalid range
if x <= 90:
return x + 17
else:
return x - 20
def decode_pair(x: int) -> tuple[int, int]:
"""EIP-8024 decode for EXCHANGE."""
if not (0 <= x <= 79 or 128 <= x <= 255):
return 1, 2 # Invalid range fallback
k = x if x <= 79 else x - 48
q, r = divmod(k, 16)
if q < r:
return q + 1, r + 1
else:
return r + 1, 29 - qThen use them within opcodes.py, updating the modifier functions:
def _dupn_stack_properties_modifier(data: bytes) -> tuple[int, int, int, int]:
imm = int.from_bytes(data, "big")
n = _decode_single(imm) # Was: imm + 1
min_stack_height = n
return 0, 1, min_stack_height, min_stack_height + 1
...
def _exchange_stack_properties_modifier(data: bytes) -> tuple[int, int, int, int]:
imm = int.from_bytes(data, "big")
n, m = _decode_pair(imm) # Was: nibble extraction
min_stack_height = max(n, m) + 1
return 0, 0, min_stack_height, min_stack_height
chfast
left a comment
There was a problem hiding this comment.
Similar comments apply to all new instructions.
| charge_gas(evm, GAS_VERY_LOW) | ||
|
|
||
| # OPERATION | ||
| immediate_data = evm.code[evm.pc + Uint(1)] |
There was a problem hiding this comment.
I was commuting... here you need to check if the immediate_data is even there (without EOF you don't have such guarantees).
|
|
||
| """ | ||
| # STACK | ||
| pass |
There was a problem hiding this comment.
You also need to check stack height for potential stack overflow/underflow. Similarly, this was guaranteed to be safe by EOF, now it's wild west again. You should also quickly realize that you are not able to follow regular scheme: stack, gas, operation. You have to decode the immediate data before checking the stack.
IMO we should try to avoid duplication of logic. So keep it as is (implement once in EELS and then just import from there) |
|
Previous commit is for testing this eip modifying pr |
marioevz
left a comment
There was a problem hiding this comment.
Left some comments on the tests, thanks!
There was a problem hiding this comment.
I think a small catch, added below. Can we confirm that these tests here are all implemented or take note?
https://github.com/ethereum/EIPs/blob/master/EIPS/eip-8024.md#test-cases
Assembly/Disassembly test vectors:
e6005b[DUPN 17, JUMPDEST]e6605b[INVALID_DUPN, PUSH1 0x5b]e7610000[INVALID_SWAPN, PUSH2 0x0000]e65f[INVALID_DUPN, PUSH0]e812[EXCHANGE 2 3]e8d0[EXCHANGE 1 19]e850[INVALID_EXCHANGE, POP]
Execution test vectors:
60016000808080808080808080808080808080e618 stack items (DUPN at end of code)600160008080808080808080808080808080806002e718 stack items (SWAPN at end of code)600060006000600060006000600060006000600060006000600060006000600060006000600060006000600060006000600060 006000600060016002e830 stack items, top=2, bottom=1
fselmo
left a comment
There was a problem hiding this comment.
Did a first pass here. Other comments have addressed the immediate test cases that came to mind. I left some additional minor comments.
thanks! adding them to |
gurukamath
left a comment
There was a problem hiding this comment.
Just left some typing and readability observations from an EELS perspective
marioevz
left a comment
There was a problem hiding this comment.
I've reviewed SWAPN + DUPN portion of the tests and signing off for that part, leaving follow up tests in the appropriate tracking issue here: #1877 (comment)
Amazing work so far @felix314159! 🎉
tests/amsterdam/eip8024_dupn_swapn_exchange/test_eip_vectors.py
Outdated
Show resolved
Hide resolved
tests/amsterdam/eip8024_dupn_swapn_exchange/test_eip_vectors.py
Outdated
Show resolved
Hide resolved
tests/amsterdam/eip8024_dupn_swapn_exchange/test_eip_vectors.py
Outdated
Show resolved
Hide resolved
reviewed separately and cherry-picked from: #1
1a231a6
into
ethereum:eips/amsterdam/eip-8024
🗒️ Description
uv run fill --clean -v -s tests/amsterdam/eip8024_dupn_swapn_exchange/ --fork=AmsterdamWe need to carefully ensure that the test logic is still correct after the port from EOF to EVM.
🔗 Related Issues or PRs
Solves #1877 and means #1873 can proceed to remove the EOF versions of this.
✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx tox -e statictype(scope):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.@ported_frommarker.Cute Animal Picture