Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Implement EIP-1884: Repricing for trie-size-dependent opcodes in LegacyVM #5700

Merged
merged 4 commits into from
Sep 2, 2019

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented Jul 25, 2019

https://eips.ethereum.org/EIPS/eip-1884

Note that it puts repricing and SELFBALANCE behind account versioning, i.e. reduced costs and the new opcode will be active only in contracts deployed with external transaction after Istanbul fork.

If it's decided that it should be in effect for version 0, we'll need to introduce another EVMSchedule ("between" current ConstantinopleFixSchedule and IstanbulSchedule)
Account versioning is disabled in Istanbul.

@gumb0 gumb0 added this to the Istanbul milestone Jul 25, 2019
@gumb0 gumb0 removed the in progress label Jul 25, 2019
@codecov-io
Copy link

codecov-io commented Jul 25, 2019

Codecov Report

Merging #5700 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5700      +/-   ##
==========================================
+ Coverage   63.22%   63.28%   +0.06%     
==========================================
  Files         353      353              
  Lines       30276    30322      +46     
  Branches     3390     3392       +2     
==========================================
+ Hits        19142    19190      +48     
  Misses       9898     9898              
+ Partials     1236     1234       -2

@gumb0 gumb0 requested review from chfast and halfalicious July 25, 2019 15:12
@gumb0
Copy link
Member Author

gumb0 commented Jul 25, 2019

cc @holiman @winsvega

@gumb0
Copy link
Member Author

gumb0 commented Aug 6, 2019

This is rebased and ready for review.
The current state of the EIP is "tentatively accepted".

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gumb0 gumb0 mentioned this pull request Aug 7, 2019
18 tasks
@gumb0
Copy link
Member Author

gumb0 commented Aug 26, 2019

Rebased.

EIP is Accepted now, please review @chfast @halfalicious

@halfalicious
Copy link
Contributor

@gumb0 For reference, why was the account versioning EIP bumped from Istanbul?

void testSelfBalanceisInvalidBeforeIstanbul()
{
se.reset(ChainParams(genesisInfo(Network::ConstantinopleFixTest)).createSealEngine());
version = ConstantinopleFixSchedule.accountVersion;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come the account version is available in the Constantinople Fix schedule..do we need to reference it in this test since from what I understand it's been punted to post-Istanbul?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Versioning still exists in the code for all forks, with version being 0 in all forks before we activate version 1 (currently it's Experimental fork)

This line is actually redundant.

@halfalicious
Copy link
Contributor

Should we also add the following tests from the EIP:

  • balance(this) and selfbalance return the same value (first test case)
  • balance(this) costs 700 gas (second test case)
  • selfbalance doesn't pop from stack (third test case)

Copy link
Contributor

@halfalicious halfalicious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with some minor questions not worth blocking merging for

@gumb0 gumb0 merged commit 02ca548 into master Sep 2, 2019
@gumb0 gumb0 deleted the eip-1884 branch September 2, 2019 09:03
@gumb0
Copy link
Member Author

gumb0 commented Sep 2, 2019

@halfalicious Re account versioning: for now it's decided that no changes accepted for Istanbul are radical enough to put them under versioning. Actually I'm not sure anymore it will be ever needed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants