EIP150 version1-1c gasPrice change proposal #3345

winsvega · 2016-10-11T12:45:43Z

Implements http://github.com/ethereum/EIPs/issues/150
connects to #3343

squash commits and rebase on top of develop
fix scripts/test.sh and test.bat back to develop branch
changes in gas costs
define transition block for the main network
the 63/64 rule for limiting the call stack
test for the gas cost changes
test for 63/64 call gas rules
If SUICIDE ~~or CALLCODE~~ hits a newly created account, it triggers an additional gas cost of 25000 (similar to CALLs)

moved from #3344

chriseth · 2016-10-12T12:09:37Z

libethashseal/Ethash.cpp

-	if (_envInfo.number() >= chainParams().u256Param("frontierCompatibilityModeLimit"))
-		return HomesteadSchedule;
+	if (_envInfo.number() >= chainParams().u256Param("EIP150forkBlock"))
+		return DevconSchedule;


Please make a consistent change for EIP150, that is its proper name.

why git does not update this comment?
code has been updated to use EIP150Schedule

This is an issue with the new GitHub review interface. On this page it shows old comments even the code has been already changed. It is better to see all changes in https://github.com/ethereum/cpp-ethereum/pull/3345/files.

chriseth · 2016-10-12T12:10:16Z

libethashseal/Ethash.cpp

 	else
-		return FrontierSchedule;
+		if (_envInfo.number() >= chainParams().u256Param("frontierCompatibilityModeLimit"))


Do we break anything if we change this to homsteadForkBlock? It would be a bit more consistent.

in eth when parsing json config this field is called frontierCompatibilityModeLimit
other then that its fine

chriseth · 2016-10-12T12:10:40Z

libethashseal/GenesisInfo.cpp

@@ -119,6 +119,7 @@ R"E(
 		"accountStartNonce": "0x00",
 		"frontierCompatibilityModeLimit": "0xffffffffffffffff",
 		"daoHardforkBlock": "0xfffffffffffffff",
+		"EIP150forkBlock": "0xfffffffffffffff",


CamelCase would be EIP150ForkBlock

chriseth · 2016-10-12T12:25:31Z

libevm/VMCalls.cpp

+	{
+		auto createGas = *m_io_gas;
+		if (m_schedule->isEIP150())
+			createGas -= createGas / 64;


There is no agreement on this yet, right?

From 1b: "CREATE only provides 63/64 of the parent gas, rounded down, to the child call." It must be like that if we want to replace static call depth limit.

Specification has changed to this in the meantime.

chriseth · 2016-10-12T12:26:35Z

libevm/VMCalls.cpp

+	bool depthOk = m_schedule->isEIP150() ? true : m_ext->depth < 1024;
+	if (m_ext->balance(m_ext->myAddress) >= endowment && depthOk)
+	{
+		auto createGas = *m_io_gas;


Please don't use auto here.

I mostly agree. However, in case of u256 createGas = *m_io_gas if we change m_io_gas to int64_t later on we will not auto notice the implicit conversion here. auto guarantees not conversion.

I will switch to explicit types.

chriseth · 2016-10-12T12:27:47Z

libevm/VMCalls.cpp

+		auto createGas = *m_io_gas;
+		if (m_schedule->isEIP150())
+			createGas -= createGas / 64;
+		auto gas = createGas;


Please don't use auto here.

chriseth · 2016-10-12T12:29:41Z

libevm/VMCalls.cpp

@@ -142,10 +150,18 @@ bool VM::caseCallSetup(CallParameters *callParams)
 		memNeed(m_stack[(1 + m_sp - m_stack) - sizesOffset], m_stack[(1 + m_sp - m_stack) - sizesOffset - 1])
 	);
 	updateMem();
+	updateIOGas();
+
+	// "Static" const already applied. Calculate call gas.


const -> cost

chriseth · 2016-10-12T12:40:11Z

libevm/VMCalls.cpp

+
+	// "Static" const already applied. Calculate call gas.
+	auto requestedCallGas = *m_sp;
+	auto maxAllowedCallGas = *m_io_gas - *m_io_gas / 64;


Again: Please don't use auto here. Note that m_runGas is a 64 bit variable and m_io_gas is a 256 bit variable. auto completely hides this fact and might introduce performance and correctness issues.

Also, perhaps less confusing:

auto maxAllowedCallGas = *m_io_gas - (m_schedule->isEIP150() ? *m_io_gas / 64 : 0); auto callGas = std::min(requestedCallGas, maxAllowedCallGas);

chriseth · 2016-10-12T12:48:36Z

libevmcore/EVMSchedule.h

+	unsigned balanceGas = 20;
+	unsigned suicideGas = 0;
+
+	bool isEIP150() const { return callGas == 700; }


Previously, we had booleans to trigger certain behaviour, so I think it would be better to replace this by
bool staticCallDepthLimit = true, set the value to false for the EVMScheduleEIP150 and replace all current calls to isEIP150() by !staticCallDepthLimit.

I would avoid having redundant data in the schedule struct. But you suggestion looks fine. I would be even better to have unsinged callDepthLimit equal 1024 or 0. Now we have this magic number in many places. But this should not go with this PR.

yep, good suggestion

changed to

bool staticCallDepthLimit() const { return !eip150Mode; } bool suicideChargesNewAccountGas() const { return eip150Mode; }

chriseth · 2016-10-12T12:51:28Z

test/libethereum/BlockchainTestsFiller/TestNetwork/bcEIP150TestFiller.json

+	    }
+        },
+        "pre" : {
+            "d4fe7bc31cedb7bfb8a345f31e668033056b2728" : { "balance" : "1000000", "nonce" : "0", "code" : "", "storage": {} },


Please explain why we need so many accounts. Furthermore, the name of the test suggests that it tests all aspects of EIP150 although I would say it only tests one particular thing (testing all aspects in one test is also not good testing practice).

this is transition test. it includes daohardfork.
I think to implement more scenarious in StateTests
things are in rush now. we started with simple blockchain test that is now too complicated .

chriseth · 2016-10-12T17:16:25Z

libevm/VM.cpp

+
+			// After EIP150 hard fork charge additional cost of sending
+			// ethers to non-existing account.
+			if (!m_schedule->staticCallDepthLimit && !m_ext->exists(dest))


This requires a new bool, charging callNewAccountGas here is different from staticCallDepthLimit.
To get both readability and non-redundancy, I would suggest the following:

Add two inline functions to the schedule: staticCallDepthLimit and suicideChargesNewAccountGas and replace the staticCallDepthLimit by a bool member that is called eip150Mode and let the two inline function return the respective values depending on that member.

chriseth · 2016-10-12T17:16:55Z

libevm/VMCalls.cpp

+	u256 requestedCallGas = *m_sp;
+	u256 maxAllowedCallGas = *m_io_gas - *m_io_gas / 64;
+	u256 callGas = m_schedule->staticCallDepthLimit
+	               ? requestedCallGas


Coding style: separators should go to the end of the line.

chriseth · 2016-10-12T17:17:07Z

libevmcore/EVMSchedule.h

@@ -35,6 +35,7 @@ struct EVMSchedule
 	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) {}
 	bool exceptionalFailedCodeDeposit = true;
 	bool haveDelegateCall = true;
+    bool staticCallDepthLimit = true;


Indentation

chriseth · 2016-10-12T17:17:18Z

libevmcore/EVMSchedule.h

+{
+	//EIP150
+	EVMSchedule schedule = HomesteadSchedule;
+    schedule.staticCallDepthLimit = false;


Indentation.

axic · 2016-10-12T18:02:00Z

(Why is it called EIP150 and not ERC150?)

RE: because of the ethereum/EIPs#150

chriseth · 2016-10-13T07:57:49Z

libevm/VMCalls.cpp

@@ -155,9 +155,8 @@ bool VM::caseCallSetup(CallParameters *callParams)
 	// "Static" costs already applied. Calculate call gas.
 	u256 requestedCallGas = *m_sp;
 	u256 maxAllowedCallGas = *m_io_gas - *m_io_gas / 64;


As commented on an earlier version: I think it confusing to call this maxAllowedCallGas even if it is not an upper bound in all code paths.

chriseth · 2016-10-13T08:01:24Z

@winsvega we do have tests for "63/64 call gas rules", right? If yes, please update description.

winsvega · 2016-10-13T19:53:08Z

"63/64 call gas rules"
not yet. we need more tests for that

axic · 2016-10-13T22:30:02Z

@winsvega yep, the repo is called EIP, but mostly those not accepted (= part of the repo and not the issues), were referred to as ERC (Ethereum Request for Comments)

RE: ethereum/EIPs#150

chriseth · 2016-10-14T09:29:32Z

@winsvega please set the hard fork block to 2457000 and then I think this is ready to go.
Please focus on adding tests to the general test suite (not the cpp-specific one).

winsvega · 2016-10-17T17:20:39Z

chriseth, rebase on top of the current develop branch is needed

chriseth · 2016-10-18T09:36:00Z

@winsvega some tests are failing, please investigate.

chriseth

Please explain why we need such gigantic test files. Aren't the cases covered by tests in the tests repository?

chriseth · 2016-10-18T09:38:10Z

libethashseal/Ethash.cpp

 	else
-		return FrontierSchedule;
+		if (_envInfo.number() >= chainParams().u256Param("homsteadForkBlock"))


Please do not indent.

why? last time you said the opposite

I mean join that into else if

chriseth · 2016-10-18T09:39:28Z

libethashseal/Ethash.cpp

@@ -163,7 +166,7 @@ void Ethash::verify(Strictness _s, BlockHeader const& _bi, BlockHeader const& _p

 void Ethash::verifyTransaction(ImportRequirements::value _ir, TransactionBase const& _t, BlockHeader const& _bi) const
 {
-	if (_ir & ImportRequirements::TransactionSignatures && _bi.number() >= chainParams().u256Param("frontierCompatibilityModeLimit"))


Didn't we agree that changing this would be too bad for other users?

should be fine.

chriseth · 2016-10-18T09:41:23Z

libethereum/Block.cpp

@@ -371,6 +371,7 @@ pair<TransactionReceipts, bool> Block::sync(BlockChain const& _bc, TransactionQu
 					else
 					{
 						clog(StateTrace) << t.sha3() << "Temporarily no gas left in current block (txs gas > block's gas limit)";
+						_tq.drop(t.sha3());


I don't think we should drop the transaction here, especially not as part of the EIP150 change.

other clients are in consensus with that test.
eth cpp will hang otherwise

chriseth · 2016-10-18T09:44:59Z

libevmcore/EVMSchedule.h

@@ -63,11 +64,34 @@ struct EVMSchedule
 	unsigned txDataZeroGas = 4;
 	unsigned txDataNonZeroGas = 68;
 	unsigned copyGas = 3;
+
+	//EIP150


Please remove this comment, those are the pre-eip150 gas values.

chriseth · 2016-10-18T09:45:23Z

test/libethereum/BlockChainTests.cpp

+
+BOOST_AUTO_TEST_CASE(bcSuicideIssueEIP150)
+ {
+	//dev::test::TestBlockChain::s_sealEngineNetwork = eth::Network::EIP150Test;


Please either uncomment, explain why commented or remove.

chriseth · 2016-10-18T09:45:41Z

test/libethereum/BlockChainTests.cpp

@@ -780,6 +886,12 @@ BOOST_AUTO_TEST_CASE(bcForkStressTestHomestead)
 	dev::test::executeTests("bcForkStressTest", "/BlockchainTests/Homestead",dev::test::getFolder(__FILE__) + "/BlockchainTestsFiller/Homestead", dev::test::doBlockchainTests);
 }

+BOOST_AUTO_TEST_CASE(bcSuicideIssueHomestead)
+ {
+	//dev::test::TestBlockChain::s_sealEngineNetwork = eth::Network::HomesteadTest;


Please either uncomment, explain why commented or remove.

winsvega added the in progress label Oct 11, 2016

chfast force-pushed the eip150 branch 2 times, most recently from 5e4d2af to 652cbd4 Compare October 12, 2016 11:24

chriseth suggested changes Oct 12, 2016

View reviewed changes

chriseth reviewed Oct 12, 2016

View reviewed changes

chfast force-pushed the eip150 branch from 1fbf716 to 97770d5 Compare October 12, 2016 14:24

chriseth suggested changes Oct 12, 2016

View reviewed changes

chriseth suggested changes Oct 13, 2016

View reviewed changes

chriseth approved these changes Oct 13, 2016

View reviewed changes

winsvega changed the title ~~EIP150 version1 gasPrice change proposal~~ EIP150 version1-1c gasPrice change proposal Oct 17, 2016

marcusrbrown mentioned this pull request Oct 17, 2016

EIP-150 for Ethereum Classic cpp-ethereum. ethereumproject/cpp-ethereum#10

Open

chriseth suggested changes Oct 18, 2016

View reviewed changes

EIP150 fork changes

e4c6f97

winsvega force-pushed the eip150 branch from df4777c to 3bfecc5 Compare October 19, 2016 09:34

winsvega added needs review and removed in progress labels Oct 19, 2016

EIP150 fork tests

6c772b2

winsvega force-pushed the eip150 branch 2 times, most recently from f049ff1 to 6c772b2 Compare October 19, 2016 11:30

chriseth approved these changes Oct 19, 2016

View reviewed changes

chriseth merged commit a654380 into develop Oct 19, 2016

chriseth removed the needs review label Oct 19, 2016

axic deleted the eip150 branch November 14, 2016 13:12

EIP150 version1-1c gasPrice change proposal #3345

EIP150 version1-1c gasPrice change proposal #3345

Conversation

winsvega commented Oct 11, 2016 • 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

winsvega Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth Oct 12, 2016 • 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

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

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

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

axic commented Oct 12, 2016 • edited by winsvega Loading

Choose a reason for hiding this comment

chriseth commented Oct 13, 2016

winsvega commented Oct 13, 2016

axic commented Oct 13, 2016 • edited by winsvega Loading

chriseth commented Oct 14, 2016

winsvega commented Oct 17, 2016

chriseth commented Oct 18, 2016

chriseth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth Oct 18, 2016 • 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

winsvega Oct 18, 2016 • 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

winsvega commented Oct 11, 2016 •

edited

Loading

winsvega Oct 12, 2016 •

edited

Loading

chriseth Oct 12, 2016 •

edited

Loading

axic commented Oct 12, 2016 •

edited by winsvega

Loading

axic commented Oct 13, 2016 •

edited by winsvega

Loading

chriseth Oct 18, 2016 •

edited

Loading

winsvega Oct 18, 2016 •

edited

Loading