EIP98 - Remove state root hash from transaction receipt #4035

gumb0 · 2017-04-11T12:51:54Z

Implementing Option 1 of EIP - removing state root hash from Receipt RLP

Save zero hash instead of root state hash
Remove root state hash instead of saving zero hash
Implement debug RPC methods (trace, storageRangeAt) without relying on state root in receipt
Fix tests relying on state root in receipts

codecov-io · 2017-04-11T15:22:12Z

Codecov Report

Merging #4035 into develop will decrease coverage by 0.02%.
The diff coverage is 51.72%.

@@             Coverage Diff             @@
##           develop    #4035      +/-   ##
===========================================
- Coverage    65.89%   65.86%   -0.03%     
===========================================
  Files          308      308              
  Lines        22876    22897      +21     
===========================================
+ Hits         15075    15082       +7     
- Misses        7801     7815      +14

Impacted Files	Coverage Δ
libethereum/State.h	`94.28% <ø> (ø)`	⬆️
libethereum/Block.h	`60.86% <ø> (ø)`	⬆️
libethereum/Executive.h	`66.66% <ø> (ø)`	⬆️
libethereum/Client.cpp	`24.55% <ø> (+0.47%)`	⬆️
libethereum/Client.h	`68.42% <ø> (ø)`	⬆️
libethereum/Block.cpp	`83.76% <0%> (+0.34%)`	⬆️
test/unittests/libethereum/Block.cpp	`69.71% <0%> (ø)`	⬆️
libethereum/Executive.cpp	`57.43% <0%> (ø)`	⬆️
test/tools/libtesteth/BlockChainHelper.cpp	`76.6% <100%> (-0.46%)`	⬇️
libethereum/TransactionReceipt.h	`27.27% <100%> (+7.27%)`	⬆️
... and 11 more

…eAt, admin_eth_vmTrace RPC methods work even when intermediate state root hash is not available in receipts (this going to be the case after Metropolis). In this case execute all previous transactions in the block to get the intermediate state.

…it with zero hash

gumb0 · 2017-04-28T15:24:34Z

libethereum/Client.cpp

@@ -775,28 +775,6 @@ Block Client::block(h256 const& _blockHash, PopulationStatistics* o_stats) const
 	}
 }

-State Client::state(unsigned _txi, h256 const& _blockHash) const


These were not used

pirapira · 2017-04-30T20:56:05Z

libethereum/Block.h

@@ -202,10 +202,11 @@ class Block
 	/// Get the bloom filter of a particular transaction that happened in the block.
 	LogBloom const& logBloom(unsigned _i) const { return receipt(_i).bloom(); }

-	/// Get the State immediately after the given number of pending transactions have been applied.
+	/// Get the State root hash immediately before after all previous transactions before transaction @a _i have been applied.


"before after" reads strange.

Thanks, fixed

chriseth · 2017-05-02T13:19:24Z

libethereum/Executive.cpp

+namespace
+{
+
+State& initIntermediateState(State& _s, Block const& _block, unsigned _txIndex, BlockChain const& _bc)


Can you name this o_s to signal that it will be modified?

chriseth · 2017-05-02T13:20:58Z

libethereum/Executive.cpp

@@ -158,7 +177,7 @@ Executive::Executive(Block& _s, LastHashes const& _lh, unsigned _level):
 }

 Executive::Executive(State& _s, Block const& _block, unsigned _txIndex, BlockChain const& _bc, unsigned _level):


Also here, this should be named o_s - but I see this is not part of the PR.

chriseth · 2017-05-02T13:22:23Z

libethereum/State.cpp

+{
+	u256 gasUsed = 0;
+	for (unsigned i = 0; i < _txCount; ++i)
+	{


The whole for-loop body should be a helper function also used elsewhere - or are the cases there more complicated?

Yeah, the other case - State::execute needs to get gas, logs, and return value from Executive...
But I've moved the common part (initialize-execute-go-finalize) to separate helper method

I wanted to move it to a separate function for a long time.

chriseth · 2017-05-02T13:23:53Z

libethereum/TransactionReceipt.cpp

@@ -44,7 +54,13 @@ TransactionReceipt::TransactionReceipt(h256 _root, u256 _gasUsed, LogEntries con

 void TransactionReceipt::streamRLP(RLPStream& _s) const
 {
-	_s.appendList(4) << m_stateRoot << m_gasUsed << m_bloom;
+	if (m_stateRoot)


Can you document this is the header? Perhaps best to put that into the class description.

Added a comment

chriseth · 2017-05-02T13:25:35Z

libweb3jsonrpc/Debug.cpp

-		State state = block.fromPending(i);
+		State state(block.state());
+		u256 const rootHash = block.stateRootBeforeTx(_txIndex);
+		if (rootHash)


I have seen this block of code before :-)

Extracted this to common function dev::eth::createIntermediateState in State.cpp

chriseth · 2017-05-04T09:40:22Z

libethereum/State.cpp

+	else
+	{
+		o_s.setRoot(_block.stateRootBeforeTx(0));
+		o_s.executeBlockTransactions(_block, _txIndex, _bc.lastHashes(_block.info().parentHash()), *_bc.sealEngine());


Do we need a check for dereferencing sealEngine here?

I don't think so. I've spot it before, it should not be a pointer type.

Yeah, it's always non-null in practice

chfast · 2017-05-04T09:43:34Z

libethereum/Executive.cpp

@@ -157,8 +157,8 @@ Executive::Executive(Block& _s, LastHashes const& _lh, unsigned _level):
 {
 }

-Executive::Executive(State& _s, Block const& _block, unsigned _txIndex, BlockChain const& _bc, unsigned _level):
-	m_s(_s = _block.fromPending(_txIndex)),
+Executive::Executive(State& o_s, Block const& _block, unsigned _txIndex, BlockChain const& _bc, unsigned _level):


I guess the convention is io_s, however I'm not big fun of it.

In some sense it's not really used as input, it is assigned immediately another State in createIntermediateState. On the other hand it initializes m_state member... Ok, I think I'll rename it

chfast · 2017-05-04T09:46:32Z

libethereum/State.cpp

+{
+	u256 gasUsed = 0;
+	for (unsigned i = 0; i < _txCount; ++i)
+	{


I wanted to move it to a separate function for a long time.

chfast · 2017-05-04T09:47:12Z

libethereum/State.cpp

+	return make_pair(res, receipt);
+}
+
+void State::executeTransaction(Executive& _e, Transaction const& _t, OnOpFunc const& _onOp)


It does not depend on State, can we make it a free function somewhere in Executive.h?

Maybe better internal in Executive.cpp then?

If possible, yes.

I meant here in State.cpp, sorry, moved it now.

chfast · 2017-05-04T09:48:46Z

libethereum/State.cpp

+	else
+	{
+		o_s.setRoot(_block.stateRootBeforeTx(0));
+		o_s.executeBlockTransactions(_block, _txIndex, _bc.lastHashes(_block.info().parentHash()), *_bc.sealEngine());


I don't think so. I've spot it before, it should not be a pointer type.

chfast · 2017-05-04T09:51:26Z

test/unittests/libethereum/Block.cpp

@@ -59,7 +59,7 @@ BOOST_AUTO_TEST_CASE(bStates)
 		OverlayDB const& genesisDB = genesisBlock.state().db();
 		BlockChain const& blockchain = testBlockchain.interface();

-		State stateBofore = testBlockchain.topBlock().state();
+		h256 stateBefore = testBlockchain.topBlock().state().rootHash();


Rename to stateRootBefore?

chfast · 2017-05-04T09:52:38Z

test/unittests/libethereum/Block.cpp

-		State stateAfterInsert = block2.fromPending(0); //get the state of blockchain on previous block
-		BOOST_REQUIRE(ImportTest::compareStates(stateBofore, stateAfterInsert) == 0);
+		h256 stateAfterInsert = block2.stateRootBeforeTx(0); //get the state of blockchain on previous block
+		BOOST_REQUIRE_EQUAL(stateBefore, stateAfterInsert);


Just a notice, BOOST_REQUIRE is overused in tests. I think BOOST_CHECK is usually better.

I'd say finer-grained tests with a single BOOST_REQUIRE in each one is better than large tests with a series of BOOST_CHECKs

Always use CHECK if it is possible to continue the test in case it fails, and only REQUIRE if it would not make sense to continue the test.

I think @gumb0 understand the difference. His point is that in case of failure in CHECK boost will output a long list of errors instead of single one per test.

Yeah, here it doesn't make sense to proceed with other checks when the first check fails, they are dependent

chfast · 2017-05-04T17:54:13Z

Great job, as usual!

pirapira · 2017-05-04T19:51:16Z

I was asked to review this, and I intend to read the Metropolis changes in all major clients. I added this to my pending list with priority H and deadline June 15.

gumb0 added the in progress label Apr 11, 2017

gumb0 changed the title ~~EIP98 =~~ EIP98 - Remove state root hash from transaction receipt Apr 11, 2017

gumb0 added this to the Metropolis milestone Apr 11, 2017

gumb0 force-pushed the eip98 branch 2 times, most recently from 414a057 to a693d63 Compare April 11, 2017 14:29

gumb0 force-pushed the eip98 branch 3 times, most recently from 87c7661 to 29df14f Compare April 20, 2017 08:59

chfast mentioned this pull request Apr 21, 2017

[META] Byzantium implementation progress #4050

Closed

17 tasks

gumb0 force-pushed the eip98 branch 2 times, most recently from 4f9d148 to 83c5f61 Compare April 26, 2017 10:50

gumb0 added 4 commits April 26, 2017 19:10

EIP98: remove state root hash from transaction receipt

f0da2f3

Remove unused methods

0dd3e54

EIP98 - remove medstate root hash from receipts instead of replacing …

1ce0eb6

…it with zero hash

gumb0 force-pushed the eip98 branch from 83c5f61 to 91e8287 Compare April 27, 2017 09:54

Fix tests after that relied on the medstate in receipts

d01152a

gumb0 force-pushed the eip98 branch from 91e8287 to d01152a Compare April 27, 2017 15:21

gumb0 commented Apr 28, 2017

View reviewed changes

gumb0 added needs review and removed in progress labels Apr 28, 2017

pirapira reviewed Apr 30, 2017

View reviewed changes

gumb0 force-pushed the eip98 branch from 1d7ed3b to bc024fb Compare May 2, 2017 09:59

gumb0 requested review from chfast, winsvega and chriseth May 2, 2017 10:00

chriseth suggested changes May 2, 2017

View reviewed changes

gumb0 force-pushed the eip98 branch 2 times, most recently from aa1dce9 to ea91ddd Compare May 3, 2017 09:24

chriseth reviewed May 4, 2017

View reviewed changes

chriseth approved these changes May 4, 2017

View reviewed changes

chfast reviewed May 4, 2017

View reviewed changes

gumb0 added 2 commits May 4, 2017 15:26

Style polishing & comments improvement

23a9cf5

Enable BlockSuite/bStates test to be run on Travis/AppVeyor

ce0e0a2

gumb0 force-pushed the eip98 branch from ea91ddd to ce0e0a2 Compare May 4, 2017 13:55

gumb0 merged commit 9198667 into develop May 4, 2017

gumb0 removed the needs review label May 4, 2017

pirapira deleted the eip98 branch May 4, 2017 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP98 - Remove state root hash from transaction receipt #4035

EIP98 - Remove state root hash from transaction receipt #4035

gumb0 commented Apr 11, 2017 •

edited

Loading

codecov-io commented Apr 11, 2017 •

edited

Loading

gumb0 Apr 28, 2017

pirapira Apr 30, 2017

gumb0 May 2, 2017

chriseth May 2, 2017

chriseth May 2, 2017

chriseth May 2, 2017

gumb0 May 3, 2017

chfast May 4, 2017

chriseth May 2, 2017

gumb0 May 3, 2017

chriseth May 2, 2017

gumb0 May 3, 2017

chriseth May 4, 2017

chfast May 4, 2017

gumb0 May 4, 2017

chfast May 4, 2017

gumb0 May 4, 2017

chfast May 4, 2017

chfast May 4, 2017

gumb0 May 4, 2017

chfast May 4, 2017

gumb0 May 4, 2017

chfast May 4, 2017

chfast May 4, 2017

chfast May 4, 2017

gumb0 May 4, 2017

chriseth May 4, 2017

chfast May 4, 2017

gumb0 May 4, 2017

chfast commented May 4, 2017

pirapira commented May 4, 2017

		@@ -158,7 +177,7 @@ Executive::Executive(Block& _s, LastHashes const& _lh, unsigned _level):
		}

		Executive::Executive(State& _s, Block const& _block, unsigned _txIndex, BlockChain const& _bc, unsigned _level):

EIP98 - Remove state root hash from transaction receipt #4035

EIP98 - Remove state root hash from transaction receipt #4035

Conversation

gumb0 commented Apr 11, 2017 • edited Loading

codecov-io commented Apr 11, 2017 • 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

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

chfast commented May 4, 2017

pirapira commented May 4, 2017

gumb0 commented Apr 11, 2017 •

edited

Loading

codecov-io commented Apr 11, 2017 •

edited

Loading