Implement EIP-1702 Generalized Account Versioning Scheme #5640

gumb0 · 2019-06-26T11:25:37Z

Closes #5638
https://eips.ethereum.org/EIPS/eip-1702

codecov-io · 2019-06-26T12:00:03Z

Codecov Report

Merging #5640 into master will increase coverage by 0.21%.
The diff coverage is 95.43%.

@@            Coverage Diff            @@
##           master   #5640      +/-   ##
=========================================
+ Coverage   62.79%     63%   +0.21%     
=========================================
  Files         349     350       +1     
  Lines       29721   29906     +185     
  Branches     3346    3350       +4     
=========================================
+ Hits        18663   18843     +180     
- Misses       9845    9846       +1     
- Partials     1213    1217       +4

gumb0 · 2019-06-26T14:01:23Z

libethereum/State.h

@@ -136,8 +136,8 @@ struct Change
    {}

    /// Helper constructor especially for new code change log.
-    Change(Address const& _addr, bytes const& _oldCode):
-            kind(Code), address(_addr), oldCode(_oldCode)
+    Change(Address const& _addr, bytes const& _oldCode, u256 const& _oldVersion)


I forgot what's the exact reason to save the old code, I'll try to remember, but if it's needed, probably we need to save the version, too, and restore during rollback.

Apparently this is not needed anymore, I think I'll remove saving the old code in #5641, then will rebase this.

gumb0 · 2019-06-26T14:02:59Z

libethereum/Account.cpp

@@ -155,7 +156,7 @@ AccountMap dev::eth::jsonToAccountMap(std::string const& _json, u256 const& _def
                        cerr << "Error importing code of account " << a
                             << "! Code needs to be hex bytecode prefixed by \"0x\".";
                    else
-                        ret[a].setCode(fromHex(codeStr));
+                        ret[a].setCode(fromHex(codeStr), 0);


I'll create an issue to handle this properly. We should have the ability to set contract's version in config file.

gumb0 · 2019-07-02T14:14:07Z

libethereum/Account.cpp

-    m_hasNewCode = true;
-    m_codeHash = sha3(m_codeCache);
+    auto const newHash = sha3(_code);
+    if (newHash != m_codeHash)


I think we should have done this check before, too, because without it in case init code returns empty code, setCode({}) raises m_hasNewCode flag, and this makes commit function to save into database the pair EmptySHA3 => "". This should better not happen.

Here it also helps with the same problem in case of empty init code. In this case setCode is still called to set the version.

Separate PR?

Yeah can be separate

gumb0 · 2019-07-02T14:16:44Z

libethcore/EVMSchedule.h

@@ -146,10 +147,26 @@ static const EVMSchedule ConstantinopleFixSchedule = [] {
    return schedule;
 }();

+static const EVMSchedule IstanbulSchedule = [] {
+    EVMSchedule schedule = ConstantinopleFixSchedule;
+    schedule.version = 1;


This value is not finalized yet, see ethereum/EIPs#2156

gumb0 · 2019-07-02T14:20:36Z

I think this is ready for review. I've also added some relevant unit tests for Executive, which we never had before.

halfalicious

Can you add a changelog entry?

halfalicious · 2019-07-04T03:17:55Z

libethashseal/genesis/test/istanbulTransitionTest.cpp

@@ -0,0 +1,66 @@
+/*


Shouldn't we use the new license message?

halfalicious · 2019-07-04T03:18:32Z

libethcore/EVMSchedule.h

@@ -29,6 +29,7 @@ struct EVMSchedule
 {
    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) {}
+    int version = 0;


Should this be unsigned since values are only >= 0?

Well in general it's not a good idea to try to enforce non-negative values with unsigned (because it doesn't really enforce it)
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#es106-dont-try-to-avoid-negative-values-by-using-unsigned
But now I think that I'll change it since other values in this struct are unsigned (gas prices), and in other parts of the code I made it u256

Maybe makes sense to make it u256 here as well, but for now unsigned definitely will work, too.

I was thinking less of enforcing non-negative values and more of code clarity, i.e. someone reading the code will be able to tell that version should be non-negative.

halfalicious · 2019-07-04T03:23:15Z

libethereum/Account.cpp

@@ -34,18 +34,24 @@ using namespace dev::eth::validation;

 namespace fs = boost::filesystem;

-void Account::setCode(bytes&& _code)
+void Account::setCode(bytes&& _code, u256 const& _version)


Not related to your changes, but why do we have a setCode function rather than only allowing code to be set in a ctor since from what I understand smart contracts can't be redeployed at the same address?

I think because contract creation has init code execution phase. First init code (provided in the contract deployment transaction or in CREATE/CREATE2 params) is executed, and it returns in the end another code, which becomes the code of new contract, saved into the state (with setCode).

So init code execution is performed in the context of the newly created contract, and so this new contract should already exist, but its code is empty until init code is finished.

@gumb0 Ah okay, thanks for the clarification! Still not clear on this:

and it returns in the end another code, which becomes the code of new contract, saved into the state (with setCode).

What is this other code that is returned by init code execution, how does it differ from the code supplied in the contract creation tx?

The code supplied in the contract creation tx (or in CREATE/CREATE2 instructions) can return any data (via RETURN instruction). The rules are that the data returned from this code becomes the code of the newly created account.

In other words the code supplied in the contract creation tx is only constructor code (in solidity terms) and it returns the code that becomes the actual contract code.

halfalicious · 2019-07-04T03:30:33Z

Should we also reset the version in Account::kill:

aleth/libethereum/Account.h

Lines 92 to 102 in 23b85b5

    
           void kill() 
        
           { 
        
               m_isAlive = false; 
        
               m_storageOverlay.clear(); 
        
               m_storageOriginal.clear(); 
        
               m_codeHash = EmptySHA3; 
        
               m_storageRoot = EmptyTrie; 
        
               m_balance = 0; 
        
               m_nonce = 0; 
        
               changed(); 
        
           }

halfalicious · 2019-07-04T03:45:34Z

Where do we retrieve the code version during syncing pass it to the VM?

[Edit] Never mind, I see it in Executive::call

gumb0 · 2019-07-04T14:20:31Z

@halfalicious Good catch about Account::kill. Doesn't look like it would affect consensus, because killed accounts are not executed / are removed from DB anyway, but it's better to set version to 0 there, too, I think.

aleth-vm/main.cpp

chfast · 2019-07-05T14:06:08Z

libethashseal/genesis/test/istanbulTransitionTest.cpp

+
+#include "../../GenesisInfo.h"
+
+static std::string const c_genesisInfoIstanbulTransitionTest = std::string() +


Can you add Istanbul configs being a copy of Petersburg in separate PR?

chfast · 2019-07-05T14:11:59Z

libethcore/EVMSchedule.h

@@ -29,6 +29,7 @@ struct EVMSchedule
 {
    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) {}
+    unsigned version = 0;


Maybe it should be named accountVersion for precision?

libethcore/EVMSchedule.h

chfast · 2019-07-05T14:15:11Z

libethereum/Account.cpp

-    m_hasNewCode = true;
-    m_codeHash = sha3(m_codeCache);
+    auto const newHash = sha3(_code);
+    if (newHash != m_codeHash)


Separate PR?

libethereum/Account.cpp

chfast · 2019-07-05T14:23:21Z

libethereum/ExtVM.h

+        if (_blockNumber < m_sealEngine.chainParams().istanbulForkBlock)
+            return m_sealEngine.evmSchedule(_blockNumber);
+        else
+            return evmScheduleForAccountVersion(_version);


Wouldn't this work all the time because we have version 0 before Istanbul?

Not really, EvmSchedule that we need changes each fork before Istanbul, so this function to work correctly would need to return different schedules for version 0 based on block number.
If I understood correctly your question.

Yes, you are right.

But I think the more future proof strategy is to select schedule group by version number and then select the schedule within the group by the block number. As I understand the EIP-1702 and as I want to be, we will not bump the version on each HF but only if some breaking changes are introduced.

@chfast I see, it makes sense, but also makes the logic of choosing schedule more complicated, as there's no one-to-one relation between schedules and versions.

I changed this just slightly now. It's not exactly as you suggest, but I think more in line with this strategy.
So basically we care about choosing by block number only within the latest version, while all past versions choose the fixed schedule "just before the version bump".
What do you think (see code)?

halfalicious · 2019-07-05T21:25:20Z

libethashseal/GenesisInfo.h

@@ -67,6 +67,8 @@ enum class Network
    ConstantinopleNoProofTest = 82,
    /// Byzantium + Constantinople + ConstantinopleFix active from block 0
    ConstantinopleFixTest = 83,
+    /// ConstantinopleFixTest + Istanbul active from block 2


Why is this active from block 2 rather than block 0?

This is a special kind of config for the tests to be able to check the behavior before Istanbul (in block 1) as well as after Istanbul. We have similar configs for other forks, too.

halfalicious · 2019-07-05T21:31:34Z

libethashseal/GenesisInfo.cpp

@@ -77,6 +78,8 @@ std::string const& dev::eth::genesisInfo(Network _n)
        return c_genesisInfoExperimentalTransitionTest;
    case Network::ConstantinopleFixTest:
        return c_genesisInfoConstantinopleFixTest;
+    case Network::IstanbulTransitionTest:


Do we need to add genesis info for Instabul tests as a part of this PR?

I'll split it into separate PR. (note that this is a config only for tests, though)

halfalicious · 2019-07-05T21:40:18Z

libethcore/EVMSchedule.h

@@ -29,6 +29,7 @@ struct EVMSchedule
 {
    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) {}
+    unsigned version = 0;


Is this the best place to store the version? EVMSchedule appears to only contain opcode / gas cost info, is there another place where we store other VM configuration information?

There's also at least block reward value, and some other things. I think it's an ok place, I don't think there is any other currently.

gumb0 · 2019-07-08T15:26:04Z

Rebased and issues addressed.

halfalicious · 2019-07-09T04:10:27Z

@gumb0 Feel free to check in once Pawel signs off, I don't know enough about the related areas to be able to sign off on this PR with a sufficient level of confidence

halfalicious · 2019-07-09T04:33:26Z

libethereum/Executive.cpp

+            false);
+    else
+        // code stays empty, but we set the version
+        m_s.setCode(m_newAddress, {}, _version);


What's the point of setting the version if there's no code?

This question might be delegated to the EIP.

I discussed this with EIP author here sorpaas/EIPs#2 (comment)

halfalicious · 2019-07-09T04:35:48Z

libethereum/State.cpp

+    // version is 0 if absent from RLP
+    auto const version = state[4] ? state[4].toInt<u256>() : 0;
+
+    auto i = m_cache.emplace(std::piecewise_construct, std::forward_as_tuple(_addr),


Don't need std:: prefix since there's using namespace std

halfalicious · 2019-07-09T04:36:02Z

libethereum/State.cpp

 {
    // rollback assumes that overwriting of the code never happens
    // (not allowed in contract creation logic in Executive)
    assert(!addressHasCode(_address));
    m_changeLog.emplace_back(Change::Code, _address);
-    m_cache[_address].setCode(std::move(_code));
+    m_cache[_address].setCode(std::move(_code), _version);


Don't need std:: prefix

chfast

Ok to me. Check @halfalicious comments.

chfast · 2019-07-09T10:21:29Z

libethereum/Executive.cpp

+            false);
+    else
+        // code stays empty, but we set the version
+        m_s.setCode(m_newAddress, {}, _version);


This question might be delegated to the EIP.

gumb0 · 2019-07-09T11:11:31Z

@halfalicious Please approve or dismiss your review

Not familiar enough with the area

halfalicious · 2019-07-09T16:51:27Z

@halfalicious Please approve or dismiss your review

@gumb0 : Done!

and some other minor issues raised in review.

…ules corresponding to a single version in the future.

gumb0 · 2019-07-09T18:13:41Z

Rebased.

gumb0 added the in progress label Jun 26, 2019

gumb0 added this to the Istanbul milestone Jun 26, 2019

gumb0 changed the title ~~Implement EIP-1702 Generalized Account Versioning Scheme in LegacyVM~~ Implement EIP-1702 Generalized Account Versioning Scheme Jun 26, 2019

gumb0 commented Jun 26, 2019

View reviewed changes

gumb0 mentioned this pull request Jun 27, 2019

EIP-1702: Generalized Account Versioning Scheme sorpaas/EIPs#2

Open

gumb0 force-pushed the account-versioning branch 5 times, most recently from aeae419 to 23b85b5 Compare July 2, 2019 14:02

gumb0 commented Jul 2, 2019

View reviewed changes

gumb0 removed the in progress label Jul 2, 2019

gumb0 requested review from chfast and halfalicious July 2, 2019 14:41

halfalicious previously requested changes Jul 4, 2019

View reviewed changes

halfalicious reviewed Jul 4, 2019

View reviewed changes

chfast reviewed Jul 5, 2019

View reviewed changes

halfalicious reviewed Jul 5, 2019

View reviewed changes

gumb0 added the in progress label Jul 8, 2019

gumb0 removed the in progress label Jul 8, 2019

halfalicious reviewed Jul 9, 2019

View reviewed changes

chfast approved these changes Jul 9, 2019

View reviewed changes

halfalicious self-requested a review July 9, 2019 16:50

gumb0 added 14 commits July 9, 2019 20:13

Implement EIP-1702 Generalized Account Versioning Scheme in LegacyVM

a116550

Use Istanbul revision of EVMC when account has this version

4988876

Some unit tests for Account Versioning implementation

4d68732

Run version 0 with ConstantinopleFixSchedule

3eaebc4

Fix test compilation after rebase

854b157

Version is set even if init code is empty

aa72ac3

More unit tests

fc4fd42

Add changelog item

e668993

Address minor review issues

2e74886

Rename EVMSchedule::version to account Version

e95dff3

and some other minor issues raised in review.

Change the logic of choosing EVMSchedule to account for several sched…

4d438f8

…ules corresponding to a single version in the future.

Fix build after rebase

5c12f9f

Fix tests

50c7156

Remove some redundant std::

0554648

gumb0 force-pushed the account-versioning branch from 43e2dfb to 0554648 Compare July 9, 2019 18:13

gumb0 merged commit d169557 into master Jul 10, 2019

gumb0 deleted the account-versioning branch July 10, 2019 09:16

gumb0 mentioned this pull request Aug 7, 2019

[META] Istanbul implementation #5716

Closed

18 tasks


		#include "../../GenesisInfo.h"

		static std::string const c_genesisInfoIstanbulTransitionTest = std::string() +

Implement EIP-1702 Generalized Account Versioning Scheme #5640

Implement EIP-1702 Generalized Account Versioning Scheme #5640

Conversation

gumb0 commented Jun 26, 2019 • edited Loading

codecov-io commented Jun 26, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Jul 2, 2019

halfalicious left a comment

Choose a reason for hiding this comment

halfalicious Jul 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halfalicious Jul 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halfalicious Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halfalicious commented Jul 4, 2019 • edited Loading

halfalicious commented Jul 4, 2019 • edited Loading

gumb0 commented Jul 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 Jul 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halfalicious Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Jul 8, 2019

halfalicious commented Jul 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Jul 9, 2019

halfalicious commented Jul 9, 2019

gumb0 commented Jul 9, 2019

gumb0 commented Jun 26, 2019 •

edited

Loading

codecov-io commented Jun 26, 2019 •

edited

Loading

halfalicious Jul 4, 2019 •

edited

Loading

halfalicious Jul 4, 2019 •

edited

Loading

halfalicious Jul 5, 2019 •

edited

Loading

halfalicious commented Jul 4, 2019 •

edited

Loading

halfalicious commented Jul 4, 2019 •

edited

Loading

gumb0 Jul 5, 2019 •

edited

Loading

gumb0 Jul 8, 2019 •

edited

Loading

halfalicious Jul 5, 2019 •

edited

Loading