search

ethereum / evmone

Fast Ethereum Virtual Machine implementation
Apache License 2.0
864 stars 286 forks source link

eof: Add trait since when an instruction is in EOF #989

Closed chfast closed 2 months ago

chfast commented 2 months ago

Add std::optional<evmc_revision> eof_since to the instr::traits with the information in which EVM revision an instruction has become valid in EOF.

Using std::optional has some inconvenience: the std::nullopt is less-than any concrete value (think "Frontier-1") while for undefined instructions we rather want to assign +infinity.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.08%. Comparing base (fdc48da) to head (cf6b23c). Report is 1 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #989 +/- ## ======================================= Coverage 94.08% 94.08% ======================================= Files 144 144 Lines 16203 16203 ======================================= Hits 15245 15245 Misses 958 958 ``` | [Flag](https://app.codecov.io/gh/ethereum/evmone/pull/989/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | Coverage Δ | | |---|---|---| | [eof_execution_spec_tests](https://app.codecov.io/gh/ethereum/evmone/pull/989/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `16.65% <100.00%> (ø)` | | | [ethereum_tests](https://app.codecov.io/gh/ethereum/evmone/pull/989/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `26.56% <100.00%> (ø)` | | | [ethereum_tests_silkpre](https://app.codecov.io/gh/ethereum/evmone/pull/989/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `18.62% <100.00%> (ø)` | | | [execution_spec_tests](https://app.codecov.io/gh/ethereum/evmone/pull/989/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `17.72% <100.00%> (ø)` | | | [unittests](https://app.codecov.io/gh/ethereum/evmone/pull/989/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `89.54% <100.00%> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files with missing lines](https://app.codecov.io/gh/ethereum/evmone/pull/989?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | Coverage Δ | | |---|---|---| | [lib/evmone/baseline\_instruction\_table.cpp](https://app.codecov.io/gh/ethereum/evmone/pull/989?src=pr&el=tree&filepath=lib%2Fevmone%2Fbaseline_instruction_table.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#diff-bGliL2V2bW9uZS9iYXNlbGluZV9pbnN0cnVjdGlvbl90YWJsZS5jcHA=) | `100.00% <100.00%> (ø)` | | | [test/unittests/instructions\_test.cpp](https://app.codecov.io/gh/ethereum/evmone/pull/989?src=pr&el=tree&filepath=test%2Funittests%2Finstructions_test.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#diff-dGVzdC91bml0dGVzdHMvaW5zdHJ1Y3Rpb25zX3Rlc3QuY3Bw) | `88.88% <ø> (ø)` | |
gumb0 commented 2 months ago

Another idea is having one since revision + 2 flags legacy and eof, assuming when instruction is activated in both, it's always activated in the same revision.

Argument against can be that it can be seen as some old opcodes are activated before Prague, but also have eof flag, so might be interpreted as "activated in eof before Prague", but I think it shouldn't influnce execution, it's equivalent to the table before this change.

chfast commented 2 months ago

Another idea is having one since revision + 2 flags legacy and eof, assuming when instruction is activated in both, it's always activated in the same revision.

Argument against can be that it can be seen as some old opcodes are activated before Prague, but also have eof flag, so might be interpreted as "activated in eof before Prague", but I think it shouldn't influnce execution, it's equivalent to the table before this change.

I considered this but this is less generic, requires one more field, and the main usage (build cost table) would need to use both revision+flag anyway.