EIP649 reward reduction and difficulty bomb delay #4338

pirapira · 2017-08-15T17:18:10Z

The reward-reduction and difficulty bomb delay have been both implemented.

Tests are updated so that testeth with no options passes.

pirapira · 2017-08-15T17:19:21Z

test/tools/jsontests/BlockChainTests.cpp

@@ -531,8 +552,9 @@ void testBCTest(json_spirit::mObject& _o)
 		//check the balance before and after the block according to mining rules
 		if (blockFromFields.blockHeader().parentHash() == preHash)
 		{
+			bool const isByzantiumOrLater = ChainBranch::networkIsByzantiumOrLater(blockChainName);


@winsvega what's the best way to know if it's already Byzantium here?

winsvega · 2017-08-15T20:10:10Z

test/tools/jsontests/BlockChainTests.cpp

 {
 	unique_ptr<SealEngineFace> se(ChainParams(genesisInfo(test::TestBlockChain::s_sealEngineNetwork)).createSealEngine());
-	bigint baseReward = se->chainParams().blockReward;
+	bigint baseReward = se->chainParams().blockReward(isByzantium);


this should be done inside chain params. the thing is that blockReward in this case will depend on blockNumber and evm schedule like other fork dependent params.

isByzantium = number >= chainParams().u256Param("ByzantiumBlockNumber")

I mean it feels like isByzantium should be calculated inside chainParams.
Cause it knows the schedule from s_sealEngineNetwork. if should be able to return the reward if you give it the blocknumber

No, ChainParams does not know the schedule. It does not have access to a sealEngineNetwork.

ChainParams constructed from genesisInfo so it does know all the transition blocks.
Schedule variables are: EVMSchedule.h

Ah, and here there is a function
EVMSchedule const& SealEngineBase::evmSchedule(u256 const& _blockNumber) const

OK. I'll file another PR first to fix this first.

@winsvega so, the SealEngine knows when Byzantium starts. ChainParams should not know that.

why? chainParams initialized from genesisInfo. it should know that. cause seal engine knows that from chainParams.

chainParams().u256Param("byzantiumForkBlock")
indicates when Byzanium starts.

do you think we should look at this issue more carefully and perhaps do a refactoring PR for libethereum instead?

No, I came up with one solution.

chriseth · 2017-08-16T09:36:55Z

libethcore/ChainOperationParams.cpp

+u256 ChainOperationParams::blockReward(bool isByzantiumOrLater) const
+{
+	if (isByzantiumOrLater)
+		return u256(3 * (bigint(10) ^ 18)); // 3 ETH


Is ^ implemented as exp or as xor?

I think there should also be a global constant called ether somewhere with the right value.

I wondered these too.

winsvega

ok. we need a blockchain test that covers up this reward change and difficulty reduction.
a test in Transition tests (transition from EIP158TOByzantine) for difficulty bomb delay.

and change an expect section for Byzantine in already existing reward test

pirapira · 2017-08-17T15:50:15Z

@winsvega shall these cases appear in the test case spreadsheet?

winsvega · 2017-08-17T15:57:26Z

I mean we could add the tests with this PR. that was the idea of a submodule.
to cover the PR changes with relevant tests

pirapira · 2017-08-17T15:59:28Z

@winsvega I prefer a separate PR for adding new tests. We should deliver the fixed tests earlier, even without the new tests.

winsvega · 2017-08-17T16:06:06Z

if we have the tests at least we know the change on this PR is covered. that what is the point

pirapira · 2017-08-17T16:16:03Z

Let me see if I need to fix any fillers. If yes, we have the changes covered somehow.

pirapira · 2017-08-23T07:52:18Z

I had an ethereum/tests commit on my local machine, pushed that and used that as the submodule.

chfast · 2017-08-23T09:21:58Z

At first look, I don't like how we manage chain configuration. Do we need to use JSON-like structure and constructs like "get u256 param" all the way to the bottom?

pirapira · 2017-08-23T09:29:28Z

@chfast I believe we should do

_bc.chainParams().EIP158ForkBlock()

instead of

_bc.chainParams().u256Param("EIP158ForkBlock")

And, remove u256Param from the public interface of ChainParams.

I want to do it in a separate PR. Do you want it before this one?

chfast · 2017-08-23T09:33:41Z

After.

I'd like to review the chain config design later on...
For EVM-C single enum is enough. Also we can consider configs for chains + separated config for chain switches (hard forks).

chfast

Only 2 small nits. The rest is good.

chfast · 2017-08-23T09:36:18Z

libethcore/EVMSchedule.h

@@ -71,6 +72,8 @@ struct EVMSchedule
 	unsigned blockhashGas = 20;
 	unsigned maxCodeSize = unsigned(-1);

+	boost::optional<u256> blockRewardOverwrite{};


Explicit initialization not needed.

chfast · 2017-08-23T09:37:07Z

libethcore/ChainOperationParams.h

@@ -73,7 +73,11 @@ struct ChainOperationParams
 	std::string sealEngineName = "NoProof";

 	/// General chain params.
-	u256 blockReward = 0;
+private:
+	u256 m_blockReward = 0;


Explicit initialization not needed.

codecov-io · 2017-08-23T09:38:34Z

Codecov Report

Merging #4338 into develop will decrease coverage by <.01%.
The diff coverage is 96.55%.

@@             Coverage Diff             @@
##           develop    #4338      +/-   ##
===========================================
- Coverage    67.69%   67.69%   -0.01%     
===========================================
  Files          307      307              
  Lines        23368    23386      +18     
===========================================
+ Hits         15819    15831      +12     
- Misses        7549     7555       +6

Impacted Files	Coverage Δ
libethcore/SealEngine.h	`75% <ø> (ø)`	⬆️
libethcore/ChainOperationParams.h	`100% <ø> (ø)`	⬆️
libethcore/ChainOperationParams.cpp	`100% <100%> (ø)`	⬆️
libethereum/Block.cpp	`87.79% <100%> (+0.06%)`	⬆️
libethcore/EVMSchedule.h	`100% <100%> (ø)`	⬆️
test/tools/jsontests/BlockChainTests.cpp	`30.6% <100%> (ø)`	⬆️
libethcore/SealEngine.cpp	`86.84% <100%> (+1.12%)`	⬆️
libethereum/ChainParams.cpp	`95.76% <100%> (ø)`	⬆️
libethashseal/Ethash.cpp	`85.16% <80%> (-0.27%)`	⬇️
libp2p/NodeTable.cpp	`39.52% <0%> (-0.6%)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e21ee1...b6617fe. Read the comment docs.

pirapira · 2017-08-23T09:45:38Z

@chfast removed the unnecessary initializations.

chriseth added the in progress label Aug 15, 2017

pirapira commented Aug 15, 2017

View reviewed changes

winsvega reviewed Aug 15, 2017

View reviewed changes

chriseth reviewed Aug 16, 2017

View reviewed changes

pirapira force-pushed the eip649 branch from 8940c50 to 938bbcd Compare August 16, 2017 15:52

winsvega reviewed Aug 17, 2017

View reviewed changes

pirapira force-pushed the eip649 branch from 938bbcd to 6cfb783 Compare August 17, 2017 13:45

pirapira changed the title ~~[WIP] EIP649 difficulty bomb delay and reward reduction~~ [WIP] EIP649 reward reduction part. Aug 17, 2017

pirapira changed the title ~~[WIP] EIP649 reward reduction part.~~ [WIP] EIP649 reward reduction and difficulty bomb delay Aug 17, 2017

pirapira force-pushed the eip649 branch from 19bd3aa to 134e9dc Compare August 17, 2017 14:42

pirapira force-pushed the eip649 branch 9 times, most recently from 86e1441 to 6d07823 Compare August 22, 2017 13:58

pirapira changed the title ~~[WIP] EIP649 reward reduction and difficulty bomb delay~~ EIP649 reward reduction and difficulty bomb delay Aug 22, 2017

pirapira mentioned this pull request Aug 23, 2017

EIP649: difficulty and reward changes ethereum/tests#269

Merged

pirapira requested review from chfast and winsvega August 23, 2017 07:51

pirapira added the needs review label Aug 23, 2017

chfast suggested changes Aug 23, 2017

View reviewed changes

pirapira added 8 commits August 23, 2017 11:44

Implementing EIP649, the reward reduction part

c145575

baseReward should be const

2e00236

_ in front of an argument

b98ea73

blockRewardOverwrite in EVMSchedule

9c21726

Refactor away common logic to SealEngine::blockReward

d7c8b35

Implement the EIP649 difficulty bomb delay

60461b4

Update tests

acf05c5

Update tests

b6617fe

pirapira force-pushed the eip649 branch from 1c01089 to b6617fe Compare August 23, 2017 09:44

chfast approved these changes Aug 23, 2017

View reviewed changes

winsvega approved these changes Aug 23, 2017

View reviewed changes

pirapira merged commit 55865c8 into develop Aug 23, 2017

chriseth removed in progress needs review labels Aug 23, 2017

pirapira deleted the eip649 branch August 23, 2017 12:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP649 reward reduction and difficulty bomb delay #4338

EIP649 reward reduction and difficulty bomb delay #4338

pirapira commented Aug 15, 2017 •

edited

pirapira Aug 15, 2017

winsvega Aug 15, 2017

pirapira Aug 16, 2017

winsvega Aug 17, 2017

pirapira Aug 17, 2017

pirapira Aug 17, 2017

winsvega Aug 17, 2017

pirapira Aug 17, 2017

pirapira Aug 17, 2017

winsvega Aug 17, 2017

pirapira Aug 17, 2017

chriseth Aug 16, 2017

chriseth Aug 16, 2017

pirapira Aug 16, 2017

winsvega left a comment

pirapira commented Aug 17, 2017

winsvega commented Aug 17, 2017

pirapira commented Aug 17, 2017

winsvega commented Aug 17, 2017

pirapira commented Aug 17, 2017

pirapira commented Aug 23, 2017 •

edited

chfast commented Aug 23, 2017

pirapira commented Aug 23, 2017

chfast commented Aug 23, 2017

chfast left a comment

chfast Aug 23, 2017

chfast Aug 23, 2017

codecov-io commented Aug 23, 2017 •

edited

pirapira commented Aug 23, 2017

EIP649 reward reduction and difficulty bomb delay #4338

EIP649 reward reduction and difficulty bomb delay #4338

Conversation

pirapira commented Aug 15, 2017 • edited

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

winsvega left a comment

Choose a reason for hiding this comment

pirapira commented Aug 17, 2017

winsvega commented Aug 17, 2017

pirapira commented Aug 17, 2017

winsvega commented Aug 17, 2017

pirapira commented Aug 17, 2017

pirapira commented Aug 23, 2017 • edited

chfast commented Aug 23, 2017

pirapira commented Aug 23, 2017

chfast commented Aug 23, 2017

chfast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 23, 2017 • edited

Codecov Report

pirapira commented Aug 23, 2017

pirapira commented Aug 15, 2017 •

edited

pirapira commented Aug 23, 2017 •

edited

codecov-io commented Aug 23, 2017 •

edited