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

Implement EIP-2200: Structured Definitions for Net Gas Metering in LegacyVM #5709

Merged
merged 7 commits into from
Sep 24, 2019

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented Jul 31, 2019

ethereum/EIPs#2200

I refactored SSTORE pricing unit tests a bit, to make them more table-driven.

@gumb0 gumb0 added this to the Istanbul milestone Jul 31, 2019
@codecov-io
Copy link

codecov-io commented Jul 31, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5709   +/-   ##
=========================================
  Coverage          ?   63.76%           
=========================================
  Files             ?      358           
  Lines             ?    30556           
  Branches          ?     3405           
=========================================
  Hits              ?    19483           
  Misses            ?     9844           
  Partials          ?     1229

@gumb0
Copy link
Member Author

gumb0 commented Sep 3, 2019

Rebased.

@halfalicious
Copy link
Contributor

You're getting an internal compiler error on the gcc config for some reason:

[195/251] Building CXX object aleth/CMakeFiles/aleth.dir/main.cpp.o
FAILED: aleth/CMakeFiles/aleth.dir/main.cpp.o 
/usr/bin/ccache /usr/bin/g++-6  -DETH_FATDB -I/home/builder/project -I/home/builder/project/aleth/../utils -isystem /home/builder/.hunter/_Base/c022f0c/0fe0970/6d1bd54/Install/include -I. -isystem deps/include -I/home/builder/project/evmc/include -O2 -g -fPIE   -fstack-protector-strong -Wall -Wextra -Werror -Wno-unknown-pragmas -fvisibility=hidden -g --coverage -std=c++11 -MD -MT aleth/CMakeFiles/aleth.dir/main.cpp.o -MF aleth/CMakeFiles/aleth.dir/main.cpp.o.d -o aleth/CMakeFiles/aleth.dir/main.cpp.o -c /home/builder/project/aleth/main.cpp
g++-6: internal compiler error: Killed (program cc1plus)
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-6/README.Bugs> for instructions.
[196/251] Building CXX object test/CMakeFiles/testeth.dir/tools/libtesteth/ImportTest.cpp.o
[197/251] Building CXX object test/CMakeFiles/testeth.dir/tools/libtesteth/boostTest.cpp.o
ninja: build stopped: subcommand failed.
Exited with code 1

@chfast
Copy link
Member

chfast commented Sep 5, 2019

You're getting an internal compiler error on the gcc config for some reason:

Not enough memory. Try to lower the number of parallel builds.

@gumb0
Copy link
Member Author

gumb0 commented Sep 5, 2019

@chfast @halfalicious lowered BUILD_PARALLEL_JOBS to 2, now it passes

@@ -156,6 +158,9 @@ static const EVMSchedule IstanbulSchedule = [] {
schedule.extcodehashGas = 700;
schedule.haveChainID = true;
schedule.haveSelfbalance = true;
schedule.eip1283Mode = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this var? eip1283 was rejected from Istanbul in favor of eip2200: https://eips.ethereum.org/EIPS/eip-1679

Can we control both the sstore and sload net gas metering via eip2200Mode?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well it's basically the same rules...
(These two EIPs differ only in some price constants and in additional "throw below 2300 gas" rule)

But yeah, I think it would make sense to have eip1283Mode = true only in Constantinople, and then I'll change sstoreNetGasMetering() to return true for both eip1283Mode and eip2200Mode

Copy link
Member Author

Choose a reason for hiding this comment

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

changed

@@ -97,10 +97,13 @@ void LegacyVM::adjustStack(unsigned _removed, unsigned _added)

void LegacyVM::updateSSGas()
{
if (m_schedule->sstoreThrowsIfGasBelowCallStipend() && m_io_gas <= m_schedule->callStipend)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we only throw OOG if EIP2200 is enabled, I'd expect us to always throw OOG if we don't have enough gas to cover the call stipend?

Copy link
Member Author

Choose a reason for hiding this comment

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

This rule is introduced only in EIP-2200.
(without it we normally throw in case of OOG from the updateIOGas call here

updateIOGas();
)

@gumb0 gumb0 force-pushed the eip-2200 branch 2 times, most recently from 988c8cf to 0c4aaaf Compare September 23, 2019 10:54
@gumb0 gumb0 changed the title Implement EIP-2200: Rebalance net-metered SSTORE gas cost with consideration of SLOAD gas cost change in LegacyVM Implement EIP-2200: Structured Definitions for Net Gas Metering in LegacyVM Sep 23, 2019
@gumb0
Copy link
Member Author

gumb0 commented Sep 23, 2019

Rebased.

@gumb0 gumb0 merged commit 0f41bb1 into master Sep 24, 2019
@gumb0 gumb0 deleted the eip-2200 branch September 24, 2019 16:47
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