Remove subroutine tests #793
Conversation
Has EIP-2315 been removed from Berlin? Do you have a cite to that effect?
Considering the EVM behaves the same in both cases (reverts the transaction), I don't think there is a way to check this. |
|
There is a suggestion to remove ethereum/pm#263. Let's please wait how this plays out until we proceed here. |
|
This formulation only tests a EIP-2315 conformant usage of the opcodes, only triggering an invalid opcode at JUMPSUB (0x5c), and not checking JUMPDEST (0x5e) or RETSUB (0x5c). The other two opcodes are illegal to encounter without first encountering the JUMPSUB instruction. One by rule (you cannot increment into a JUMPDEST operation) and the next because the return stack is empty (RETSUB). Should we keep the fillers for |
|
lightclient
force-pushed
the
remove-eip2315
branch
from
f656184
to
d85fdc7
Compare
|
Still need to refill https://github.com/ethereum/tests/tree/develop/BlockchainTests/GeneralStateTests/stSubroutine. One more file change and 10 more file deletes. Other than that Besu passes these tests when using this version - shemnon/besu@a577fe8 |
|
Thanks for the feedback @shemnon. |
|
I believe I've made all the changes @shemnon recommended - please let me know if any other changes should be made. Thanks! edit: given the CI is passing with what I assume to be a client with 2315 still enabled - something must be wrong somewhere with my changes. Any ideas? At minimum edit 2: I confirmed that geth t8n tool fails this test with 2315 enabled, so not sure what's going on in CI. The |
|
the test repo check is a little semantic check and that all tests are regenerated up to date with the fillers. |
BlockchainTests/GeneralStateTests/stSubroutine/simpleSubroutine.json
Outdated
Show resolved
Hide resolved
src/BlockchainTestsFiller/ValidBlocks/bcStateTests/testOpcodesFiller.yml
Outdated
Show resolved
Hide resolved
| "//code" : "This should jump into a subroutine, back out and stop.", | ||
| "code" : "(asm 0x04 JUMPSUB STOP BEGINSUB RETURNSUB)", | ||
| "//code" : "This should jump into a subroutine, store 0x01 at 0x00, back out and stop.", | ||
| "code" : "(asm 0x04 JUMPSUB STOP BEGINSUB 0x01 0x00 SSTORE RETURNSUB)", |
There was a problem hiding this comment.
if there wouls be no subroutine opcodes I think those codes will be used for something else.
we can't really use beginsub returnsub anymore ?
There was a problem hiding this comment.
I can replace with :raw code?
There was a problem hiding this comment.
I went with this approach in b38f7bd. Easy to remove if you prefer a different approach.
|
I would make one PR removeingn the existing subroutine tests. |
lightclient
force-pushed
the
remove-eip2315
branch
from
a71d0a0
to
3e9ce4f
Compare
Will be moved to stBadOpcode, as these opcodes are now bad. Also renamed to a more descriptive name.
|
I'm sorry, but my git skills aren't up to grafting into this tree what I think it should have. I'll open another pull request. |
This PR will remove tests for EIP-2315: Simple Subroutines. It also inverts the simple, valid case to ensure that client hasn't accidentally forgotten to disable the EIP.
This is my first contribution to this repository, so please forgive me for not knowing the process. I tested this change against a version of geth with 2315 included in Berlin (ethereum/go-ethereum@19d7a37) and one with it omitted (ethereum/go-ethereum#22414). This test failed with 2315 included and passed with it omitted.
The
badOpcodestests passes for both clients, but for different failure reasons (invalid semantics vs. unknown ops). I'm not sure if there is better way to capture that.