Add EVMC_LONDON revision #595
Conversation
Codecov Report
@@ Coverage Diff @@
## master #595 +/- ##
==========================================
- Coverage 93.78% 93.64% -0.14%
==========================================
Files 25 25
Lines 3747 3763 +16
==========================================
+ Hits 3514 3524 +10
- Misses 233 239 +6 |
tools/evmc/main.cpp
Outdated
| @@ -48,7 +48,7 @@ int main(int argc, const char** argv) | |||
| std::string vm_config; | |||
| std::string code_arg; | |||
| int64_t gas = 1000000; | |||
| auto rev = EVMC_ISTANBUL; | |||
| auto rev = EVMC_MAX_REVISION; | |||
There was a problem hiding this comment.
Here we rather want to go with stable Berlin for now. If we want to minimize number of changes here, we may want to introduce EVMC_LATEST_STABLE_REVISON (something we considered before). Comments @axic ?
There was a problem hiding this comment.
Took a bit of digging, but this was the issue: #175. And this was discussion sparking it: #174 (comment)
I think what we discussed there still stands.
| */ | ||
| EVMC_ISTANBUL = 7, | ||
|
|
||
| /** | ||
| * The Berlin revision. | ||
| * | ||
| * The spec draft: https://eips.ethereum.org/EIPS/eip-2070. | ||
| * https://github.com/ethereum/eth1.0-specs/blob/master/network-upgrades/mainnet-upgrades/berlin.md |
There was a problem hiding this comment.
Still don't get why the EIPs are deprecated and now there's a volatile place for the spec, i.e. this link has changed twice already since it has been added to the EIP 😬
There was a problem hiding this comment.
This looks bad to me too. I think we should ask for shorter permanent links.
| const auto ln = evmc_get_instruction_names_table(EVMC_LONDON); | ||
| const auto bn = evmc_get_instruction_names_table(EVMC_BERLIN); | ||
|
|
||
| for (int op{OP_STOP}; op <= OP_SELFDESTRUCT; ++op) |
There was a problem hiding this comment.
I know this isn't introduced by this PR as a new thing, but why is this going from STOP->SELFDESTRUCT and not 0 to 255 or some min/max based on the enum type? I know it is identical but still feels weird basing it on the instruction name.
There was a problem hiding this comment.
Make sense, I will change that later.
| @@ -147,6 +147,8 @@ const ( | |||
| Constantinople Revision = C.EVMC_CONSTANTINOPLE | |||
| Petersburg Revision = C.EVMC_PETERSBURG | |||
| Istanbul Revision = C.EVMC_ISTANBUL | |||
| Berlin Revision = C.EVMC_BERLIN | |||
There was a problem hiding this comment.
Hmm, isn't this a bugfix which should be released as 8.0.1?
There was a problem hiding this comment.
It is, but we don't need to fix it proactively.
Add
evmc_revisionvalue for the upcoming London fork.