EIP-2046 independent activation by gumb0 · Pull Request #5741 · ethereum/aleth

gumb0 · 2019-09-10T12:37:09Z

Addresses #5789

It introduces support for individual EIP activation without fork, then moves EIP-2046 implementation from Berlin fork definition to this new individual model of activation.

How this is supposed to be used

Implementing new EIP

Add new member to AdditionalEIPs struct https://github.com/ethereum/aleth/pull/5741/files#diff-c93cb246fbbc22300c737cb7ce2a2741R57-R60
If EIP affects anything outside EVM, the helper ChainOperationParams::isEIPXXXXEnabled would be helpful https://github.com/ethereum/aleth/pull/5741/files#diff-c93cb246fbbc22300c737cb7ce2a2741R72-R75
If EIP affects EVM, affected prices/flags should be set to EVMSchedule in the function addEIPsToSchedule https://github.com/ethereum/aleth/pull/5741/files#diff-363b4e07ecbf6ed5a7eb650e5cb69bcbR12-R19

Including EIP into the new fork definition, after it's accepted

Remove EIP member from AdditionalEIPs struct
If EIP affects anything outside EVM, modify ChainOperationParams::isEIPXXXXEnabled helper https://github.com/ethereum/aleth/pull/5741/files#diff-c93cb246fbbc22300c737cb7ce2a2741R76-R80
If EIP affects EVM remove it from addEIPsToSchedule and add to the fork's EVMSchedule instance

Writing a unit test

Instantiate ChainParams with additional EIPs activated using ChainParams(string, AdditionalEIPs) constructor https://github.com/ethereum/aleth/pull/5741/files#diff-bca8e2c5f9ec26d6b11f9fd86abd6338R815-R817

Writing consensus state test

Use Forkname+EIPXXXX instead of regular fork name.
See example test for EIP2046 that I created ethereum/tests@8bbbd48
(based on existing CallIdentitiy_1 test)

Limitations / future improvements (if needed)

Not supported in blockchain tests, transaction tests
Not supported in generating blockchain tests from state tests
Not supported in chain config json, only in testeth. So it's not possible to create a chain with individual EIPs activated, that also means it won't work for retesteth.

These should be fairly easy to support though.

@winsvega please review the changes in testeth. Basically it operates now with a network name string (which can possibly contain additional EIP numbers) in the places where it used to use eth::Network. Then creates ChainParams based on this string, when it needs to execute the transaction.

codecov-io · 2019-10-24T16:29:53Z

Codecov Report

❗ No coverage uploaded for pull request base (master@57b8156). Click here to learn what that means.
The diff coverage is 79.34%.

@@            Coverage Diff            @@
##             master    #5741   +/-   ##
=========================================
  Coverage          ?   64.08%           
=========================================
  Files             ?      360           
  Lines             ?    30830           
  Branches          ?     3425           
=========================================
  Hits              ?    19756           
  Misses            ?     9844           
  Partials          ?     1230

gumb0 · 2019-10-30T12:18:30Z

Rebased.

libethcore/ChainOperationParams.h

libethcore/ChainOperationParams.cpp

libethcore/EVMSchedule.cpp

halfalicious · 2019-11-02T22:13:32Z

You mentioned in the issue that the syntax for specifying at runtime which EIPs to enable is a delimited list of EIPs - is there a chance that different EIPs can change the same settings so the ordering of the EIPs in the list matters? Or is this generally not a concern?

halfalicious · 2019-11-02T22:16:10Z

What happens if someone specifies an eip not in the "AdditionalEIPs" struct? Presumably we get some sort of error?

halfalicious · 2019-11-02T22:17:21Z

libethereum/ChainParams.cpp


+ChainParams ChainParams::addEIPs(AdditionalEIPs const& _eips) const
+{
+    ChainParams cp(*this);


halfalicious · 2019-11-02T22:19:05Z

libethereum/ChainParams.h

    ChainParams loadConfig(std::string const& _json, h256 const& _stateRoot = {},
        const boost::filesystem::path& _configPath = {}) const;

+    /// Activatie additional EIPs on top of the last fork block


Typo (Activatie -> Activate)

libethereum/ChainParams.cpp

halfalicious · 2019-11-02T22:26:59Z

test/tools/libtesteth/ImportTest.cpp

+{
+    // network name may be network+EIPs
+    vector<string> networkAndEIPs;
+    boost::algorithm::split(networkAndEIPs, _networkName, boost::is_any_of("+"));


There's no appropriate STL function that we can use?

Not really, string functions are pretty low-level and/or verbose (like you could of course string::find in a loop)

I like boost string functions because they create higher-level code, e.g. similar to split/join functions of other languages.

(We're probably going to get split via ranges in C++20 https://stackoverflow.com/questions/48402558/how-to-split-a-stdstring-into-a-range-v3-of-stdstring-views)

halfalicious · 2019-11-02T22:29:37Z

test/tools/libtesteth/ImportTest.cpp

    for(auto const& tr : m_transactions)
    {
-        if (network == netIdToString(tr.netId) || network == "ALL")
+        if (network == tr.netId || network == "ALL")


(Nit) Perhaps we should make this comparison case-insensitive?

It's case-sensitive everywhere in testeth, I'd leave it like this.

(these strings are actually field names test JSON files, so proper behaviour depends on whether field names are case-sensitive in JSON, are they?)

Makes sense, if it's already case sensitive everywhere else let's leave it alone.

halfalicious · 2019-11-02T22:38:00Z

libethereum/ChainParams.cpp

    *this = loadConfig(_json, _stateRoot);
 }

+ChainParams::ChainParams(std::string const& _s, AdditionalEIPs const& _additionalEIPs)


(Nit) Can we choose a name for the _s arg which reflects the semantics of the data it contains?

halfalicious · 2019-11-02T22:41:21Z

libethereum/ChainParams.cpp

+ChainParams::ChainParams(std::string const& _s, AdditionalEIPs const& _additionalEIPs)
+  : ChainParams(_s)
+{
+    *this = addEIPs(_additionalEIPs);


I think this approach is a little confusing (calling another ctor which sets some state and then overwriting the current instance) - I'd prefer if the addEIPs function either updates the current instances state (i.e. it's non-const) or we turn this ctor into a static function which calls addEIPs and returns the result.

Agree, this is complicated. I originally added only addEIPs and made it similar to loadConfig and loadGenesis (which create new instance out of the current one)

But now I think I should remove addEIPs and leave only this constructor.

test/tools/libtesteth/ImportTest.cpp

gumb0 · 2019-11-04T13:45:49Z

You mentioned in the issue that the syntax for specifying at runtime which EIPs to enable is a delimited list of EIPs - is there a chance that different EIPs can change the same settings so the ordering of the EIPs in the list matters? Or is this generally not a concern?

@halfalicious In theory it's possible, even if not about the same settings, some rules might be interrelated so that the order of applying matters... But I think the complexity of handling such flexibility is not worth the trouble for now (that is, EIPs in close areas will be applied in hard-coded order)

What happens if someone specifies an eip not in the "AdditionalEIPs" struct? Presumably we get some sort of error?

Not sure if I understand the question, but if someone implements an EIP, there will be some kind of check against chainParams to activate it - like without this individual activation the check looked like if (blockNumber >= chainParams.EIP158ForkBlock) - and this way EIP is included in some fork definition.

halfalicious · 2019-11-05T04:36:43Z

libethereum/ChainParams.cpp

+ChainParams::ChainParams(std::string const& _configJson, AdditionalEIPs const& _additionalEIPs)
+  : ChainParams(_configJson)
+{
+    lastForkAdditionalEIPs = _additionalEIPs;


(Nit) Can we initialize this as a part of the initializers list?

halfalicious · 2019-11-05T04:45:27Z

libethcore/EVMSchedule.h

 {
    EVMSchedule(): tierStepGas(std::array<unsigned, 8>{{0, 2, 3, 5, 8, 10, 20, 0}}) {}
    EVMSchedule(bool _efcd, bool _hdc, unsigned const& _txCreateGas): exceptionalFailedCodeDeposit(_efcd), haveDelegateCall(_hdc), tierStepGas(std::array<unsigned, 8>{{0, 2, 3, 5, 8, 10, 20, 0}}), txCreateGas(_txCreateGas) {}
+    // consturct schedule with additional EIPs on top


(Nit) Spelling typo

halfalicious

A couple of minor suggestions, otherwise looks good!

gumb0 · 2019-11-05T11:37:33Z

@winsvega @chfast I'm going to merge this today, if no reviews from you

gumb0 added the in progress label Sep 10, 2019

gumb0 changed the title ~~EIP-2046 separate activation~~ EIP-2046 independent activation Sep 25, 2019

gumb0 force-pushed the eip-2046-separate branch from 93a7e67 to 8e5ef55 Compare October 2, 2019 17:15

gumb0 force-pushed the eip-2046-separate branch 2 times, most recently from 9945c5d to 0413a31 Compare October 24, 2019 13:39

gumb0 force-pushed the eip-2046-separate branch from 980b658 to a0b0cb9 Compare October 29, 2019 11:37

gumb0 removed the in progress label Oct 29, 2019

gumb0 force-pushed the eip-2046-separate branch from 2b96438 to 7add7e2 Compare October 29, 2019 15:41

gumb0 requested review from chfast, halfalicious and winsvega October 29, 2019 16:11

gumb0 force-pushed the eip-2046-separate branch 2 times, most recently from 30bf23d to e019753 Compare October 30, 2019 11:39