-
Notifications
You must be signed in to change notification settings - Fork 322
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
[EIP2315] Add tests for subroutines #685
[EIP2315] Add tests for subroutines #685
Conversation
Awesome, well done!
This case is a bit problematic to test -- because since it exits with error, it will consume all gas regardless of whether it exits at step |
I can verify that the state-tests pass seems to be correct, at least according to the geth-implementation:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but let's wait a bit more and see if the EIP will change
Worth noting, though, is that two of the tests suffer from the same problem that I described regarding that one that exits with error.
I don't have any splendid ideas immediately on how to improve the accuracy of those tests |
I tried your bytecode which allows you to reach the limit of the rstack. But when we add a SSTORE to save a value, the storage in the postState remains empty in case of error. A simple test without counter
|
Ah right (doh!) |
Here's an example that does it five times: The code recursively calls into a subroutine, N times. It should succeeed if N is below the threshold for rstack size
|
This is the underlying instrutions:
|
Oh, and the code below only "stores back" the counter after the zero-check, so it should end up with |
I had not seen your message and I added my own implementation which is very close to yours. As you have already documented yours I will update my tests |
I just put your implementation. tests work well on Besu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some tests with 1023/24/25 subcalls already. You can also peek if there are some ways to organize stack depth calls for tests
src/GeneralStateTestsFiller/stSubroutine/twoLevelsSubroutinesFiller.json
Show resolved
Hide resolved
src/GeneralStateTestsFiller/stSubroutine/subroutineShallowReturnStackFiller.json
Show resolved
Hide resolved
src/GeneralStateTestsFiller/stSubroutine/subroutineInvalidJumpFiller.json
Show resolved
Hide resolved
src/GeneralStateTestsFiller/stSubroutine/subroutineAtEndOfCodeFiller.json
Show resolved
Hide resolved
src/GeneralStateTestsFiller/stSubroutine/shouldSucceedWhenReturnStackGrowsUntil1023Filler.json
Outdated
Show resolved
Hide resolved
src/GeneralStateTestsFiller/stSubroutine/shouldErrorWhenReturnStackGrowsAbove1023Filler.json
Outdated
Show resolved
Hide resolved
I just updated the tests to manage the last PR (enter only via jumpsub) ethereum/EIPs#2646
shouldErrorWhenExecuteBeginSub
Error: at pc=3, op=BEGINSUB: evm: out of gas shouldErrorWhenSubroutineEnteredViaBeginSub
Error: at pc=5, op=BEGINSUB: evm: out of gas beginSubAtEndOfCode
|
Hm, I'm actually getting an error now
|
are you testing with the latest updates? |
Sorry, my bad. We haven't enabled 'Berlin', so I added a hack to redefine "Berlin" as "Byzantium+2315", but the correct thing was |
I guess besu doesn't use the |
Yes, for the moment it is not managed. we will try to manage this feature as soon as possible |
@holiman thank you for your review on this PR. As the tests are pushed in develop does that mean that it confirms a JUMPSUB which costs 8? |
I can't really make that call, but it's definitely my opinion. I will make a PR to update the eip and reach out to other implementors
|
Addition of tests for the https://eips.ethereum.org/EIPS/eip-2315. Tests generated with the current Besu implementation of the EIP2315.
The tests are those present in the description of the EIP.
Need update of retesteth ethereum/retesteth#90