Tests for EIP-3855: PUSH0 instruction #1033
Conversation
src/GeneralStateTestsFiller/VMTests/vmTests/push0Gas2Filler.yml
Outdated
Show resolved
Hide resolved
src/GeneralStateTestsFiller/VMTests/vmTests/push0Gas2Filler.yml
Outdated
Show resolved
Hide resolved
|
Tests updated based on comments. |
|
@winsvega Do you think it's ok to add these new tests to |
|
Yes, its not a legacy anymore. Ori updated it to GStateTests format |
| 0000000000000000000000000000000000000100: | ||
| balance: 0 | ||
| code: ':raw 0x5f' | ||
| nonce: '0' |
There was a problem hiding this comment.
Test for gasPrice
(sstore 0 (gas))
Push0
(Sstore 0 (sub (gas) (sload 0)))
Make it irrelevant from other gas that is affected by other evm changes
There was a problem hiding this comment.
Changed to record gas usage in storage.
There was a problem hiding this comment.
this is a version using MSTORE/MLOAD: https://gist.github.com/hugo-dc/4740c46803771c701b12d417a348f3b8#file-push0gasfiller-yml-L21
Looks very similar to the SSTORE/SLOAD version, although the expect section looks cleaner.
There was a problem hiding this comment.
memory is also ok as long as the result is checed in sstore. and memory does not intersect with some other region that is in use
| - 'London' | ||
| result: | ||
| a94f5374fce5edbc8e2a8697c15331677e6ebf0b: | ||
| balance: 9099500 |
There was a problem hiding this comment.
Same here. Use difference.
There was a problem hiding this comment.
Changed, now gas is not tested by checking final balance but using values recorded to storage.
| 0x00: 0x01 | ||
| b94f5374fce5edbc8e2a8697c15331677e6ebf0b: | ||
| storage: | ||
| 0x00: 0x01 |
There was a problem hiding this comment.
Can you put more comments what we expect here. (In all expect sections of this test) what does sstore equals 1 mean.
| 0000000000000000000000000000000000000100: | ||
| balance: 0 | ||
| # sstore(0, gas) push0 sstore(0, (sub(sload(0), gas))) stop | ||
| code: ':raw 0x5a6000555f5a6000540360015500' |
There was a problem hiding this comment.
I don't think PUSH0 is available in Solidity, that's why.
There was a problem hiding this comment.
can use custom lllc
mstore(0, gas) push0 sstore(0, (sub(mload(0), gas))) stop
the first gas will be affected by intrinsic transaction cost which might be changed by other eip
| code: ':raw 0x60ff5f5360016000f3' | ||
| nonce: 0 | ||
| storage: {} | ||
|
|
There was a problem hiding this comment.
Could we add a test where we have a JUMPDEST right in front of a PUSH0 and then jump to it ?
This could help us make sure that jumpdest-analysis is not affected somehow by this opcode.
There was a problem hiding this comment.
Added a new test case, with a contract having a JUMPDEST after PUSH0 and jumping into it.
| storage: {} # Nothing is stored, PUSH0 (0x5f) is undefined/invalid in London | ||
| - indexes: | ||
| data: ':label use_push0' | ||
| gas: !!int -1 |
There was a problem hiding this comment.
btw -1 is set by default can now be omited from the filler
| result: | ||
| 0000000000000000000000000000000000000100: | ||
| balance: 0 | ||
| storage: {} # Nothing is stored, PUSH0 (0x5f) is undefined/invalid in London |
There was a problem hiding this comment.
will require update invalidOpcodes test once the eip is merged
There was a problem hiding this comment.
It is merged in geth as "+3855".
Expectations in the tests could be changed to Merge / Merge+3855 though.
…t using staticcall, improve gas usage calculation to avoid strong dependency on SSTORE
remove default indexes in expect section fix comments from main PR
This PR contains the test cases in PR #1000 and some changes according to the comments in such PR.