EIP96 - blockhash refactoring #4066

gumb0 · 2017-05-03T14:42:16Z

Contract creation
A call to update stored block hashes before processing transactions in each block
BLOCKHASH implementation using the call to contract
Change BLOCKHASH cost after Metropolis

axic · 2017-05-05T08:27:30Z

libdevcrypto/Common.cpp

@@ -67,6 +67,8 @@ const Address dev::ZeroAddress = Address();

 const Address dev::MaxAddress = Address("0xffffffffffffffffffffffffffffffffffffffff");

+const Address dev::SuperuserAddress = Address("0xfffffffffffffffffffffffffffffffffffffffe");


This has been renamed to system.

Thanks, I'll rename it

axic · 2017-05-11T09:47:36Z

libethereum/ExtVM.cpp

+
+h256 ExtVM::blockHash(u256 _number)
+{
+	if (envInfo().number() < m_sealEngine.chainParams().u256Param("metropolisForkBlock") + 256)


I think it was agreed last time that the opcode remains limited to the last 256 blocks.

Also the comment in ExtVM.h should be kept up to date with the final decision:

/// Hash of a block if within the last 256 blocks, or h256() otherwise.

So where the code or comment disagrees with this?

Right now the comment disagrees with the code. If you update the code to match the current EIP (limited to 256) then the comment is fine again :)

I don't see the disagreement.
This line in particular just checks whether the current block (not the block requested) is before or after Metro+256 (according to this spec https://github.com/ethereum/EIPs/pull/210/files#diff-e02a92c2fb96c1a1bfb05e4c6e2ef5daR34 )

If we are before Metro+256, we return, as comment says, "hash of a block if within the last 256 blocks, or h256() otherwise."
Otherwise we do the same but through the call.

I do not see that it is limited to the last 256 blocks through the call below after Metropolis?

Oh, I see, I was assuming that the contract itself (the new version of it) will return 0 if requested block is out of bounds.
Yeah, anyway it will makes sense to avoid making a call in this case, I'll change it.
Thanks for noticing this!

The contract has no easy way knowing where the call came from and its code may be different too in the future. Additionally the spec will/should say that the opcode itself filters for <=256.

gumb0 · 2017-05-17T15:21:43Z

Current status:

Waiting for Blockchain tests update to be merged Update Blockchain Transition tests for BLOCKHASH refactoring EIP tests#167
Waiting for failing state tests to be updated by @winsvega (the plan is to convert state tests that use BLOCKHASH into Blockchain tests)
don't pass last hashes to Executive after Metropolis, saving performance

This should better go into separate PR, because looks like some non-trivial refactoring is needed there

Otherwise I think it's ready for review.

pirapira · 2017-05-18T10:20:21Z

libevmcore/EVMSchedule.h

@@ -108,6 +109,7 @@ static const EVMSchedule EIP158Schedule = []
 static const EVMSchedule MetropolisSchedule = []
 {
 	EVMSchedule schedule = EIP158Schedule;
+	schedule.blockhashGas = 800;


It's uncertain to me if 800 should be applied from the beginning of Metropolis from 256 blocks into Metropolis. I asked a question https://github.com/ethereum/EIPs/pull/210/files#r117211219

And I will ask in the allcoredev meeting ethereum/pm#14 (comment)

Yes, this seem to be strange. Shouldn't we just charge for the contract execution?

This can stay as it is. In this previous allcoredev meeting, it was agreed that the gas schedule changes at the Metropolis block (inclusive), not later.

pirapira · 2017-05-18T13:59:00Z

libethereum/Block.cpp

+		Transaction tx(0, 0, gas, c_blockhashContractAddress, m_previousBlock.hash().asBytes(), nonce);
+		tx.forceSender(SystemAddress);
+		// passing empty lastHashes - assuming the contract doesn't use BLOCKHASH
+		m_state.execute(EnvInfo(info(), {}, 0), *m_sealEngine, tx, Permanence::Committed);


I guess this doesn't increment the nonce.

Hm, I think it does. Shouldn't it?

I don't know if it should or not. So I need to ask.

I asked https://github.com/ethereum/EIPs/pull/210/files#r117260179 ethereum/pm#14 (comment)

Performing the call to update the blockhash contract should be similar to CALL from EVM, not to external transaction and should not increment SYSTEM_ADDRESS's nonce.

So the fix is needed here.

pirapira · 2017-05-18T14:06:33Z

libethereum/ExtVM.cpp

+
+	if (currentNumber < m_sealEngine.chainParams().u256Param("metropolisForkBlock") + 256)
+	{
+		assert(envInfo().lastHashes().size() > (unsigned)(currentNumber - 1 - _number));


The assertion seems to fail if currentNumber == 0 and _number == 0xff..ff (which is 0 - 1). I think lastHashes().size() should be zero in that case, and the right hand side evaluates to zero.

We won't get here because of the check at line 142, requested _number is always less than currentNumber here

Also currentNumber is never 0 here, because you can't execute anything in the genesis block

OK, seems valid.

…asses

gumb0 · 2017-05-19T12:28:51Z

Rebased and comments addressed.

chriseth · 2017-05-19T15:11:43Z

libethashseal/genesis/metropolisTransitionTest.cpp

+		"difficultyBoundDivisor": "0x0800",
+		"durationLimit": "0x0d",
+		"blockReward": "0x4563918244F40000",
+		"registrar" : "0xc6d9d2cd449a754c494264e1809c50e34d64562b",


Can we remove this from the codebase?

I'll remove it in a separate PR

chriseth · 2017-05-19T15:12:36Z

libethcore/Common.cpp

@@ -53,6 +53,9 @@ const unsigned c_databaseVersionModifier = 0;

 const unsigned c_databaseVersion = c_databaseBaseVersion + (c_databaseVersionModifier << 8) + (23 << 9);

+const Address c_blockhashContractAddress(0xf0);
+const bytes c_blockhashContractCode(fromHex("0x73fffffffffffffffffffffffffffffffffffffffe33141561006a5760014303600035610100820755610100810715156100455760003561010061010083050761010001555b6201000081071515610064576000356101006201000083050761020001555b5061013e565b4360003512151561008457600060405260206040f361013d565b61010060003543031315156100a857610100600035075460605260206060f361013c565b6101006000350715156100c55762010000600035430313156100c8565b60005b156100ea576101006101006000350507610100015460805260206080f361013b565b620100006000350715156101095763010000006000354303131561010c565b60005b1561012f57610100620100006000350507610200015460a052602060a0f361013a565b600060c052602060c0f35b5b5b5b5b"));


Can we use an assert to check the hash of this code against other implementations? Or will the code hash be part of the block tests anyway?

Might there be any global variable initialization race condition problems here?

Yeah, blockchain tests check that the contract with correct code exists in the state

There shouldn't be as long as we don't use these constants for other static variable initialization in another module. Which we shouldn't do.

…f executing fake Transaction. Thus SystemAddresss's nonce won't be incremented as EIP96 requires.

# Conflicts: # test/tools/libtesteth/TestHelper.h

pirapira · 2017-05-30T11:14:10Z

The Travis test failure looks related to the blockhash https://travis-ci.org/ethereum/cpp-ethereum/jobs/237208475#L1908

…ary stack object.

gumb0 · 2017-06-02T10:25:12Z

test/tools/jsontests/StateTests.cpp

@@ -115,25 +116,49 @@ class generaltestfixture

 	void fillAllFilesInFolder(string _folder)
 	{
-		std::string fillersPath = dev::test::getTestPath() + "/src/GeneralStateTestsFiller/" + _folder;
+		std::string fillersPath = getTestPath() + "/src/GeneralStateTestsFiller/" + _folder;
+		using dirIt = boost::filesystem::directory_iterator;


@winsvega Begin type alias with uppercase latter

gumb0 · 2017-06-02T10:26:21Z

test/tools/jsontests/StateTests.cpp


-		boost::filesystem::directory_iterator iterator_tmp(fillersPath);
+		dirIt iterator_tmp(fillersPath);


@winsvega use camelCase instead of snake_case

gumb0 · 2017-06-02T10:29:40Z

test/tools/jsontests/StateTests.cpp

+					//Skip filling this tests as state tests
+					if (Options::get().filltests && !Options::get().fillchain)
+					{
+						TestOutputHelper::incTestCount(2);


@winsvega What does this do?

as state test was not designed to run as a blockchain test. I have to overrun the test counter with this to get 100% tests executed output.

gumb0 · 2017-06-02T11:08:12Z

test/tools/jsontests/StateTests.cpp

 			if (boost::filesystem::is_regular_file(iterator->path()) && iterator->path().extension() == ".json")
 			{
 				string fileboost = iterator->path().filename().string();
-				dev::test::executeTests(fileboost, "/GeneralStateTests/"+_folder, "/GeneralStateTestsFiller/"+_folder, dev::test::doStateTests);
+				if (fileboost.rfind("BCFiller.json") != std::string::npos)


@winsvega could you explain what happens here and why do you need to hardcode json file name?

so not to convert 100 of tests into blockchain we decided to add a marker to such tests to be run as blockchain but being state tests.
I had an option in mind - to read file contents and then look for an option to run this test as blockchain.
then I realized that to do that I will have to parse whole json file and that would add to execution time.
there for we have this hardcoded thing.

the mere fact that state tests are being run as blockchain is a workaround. it was not designed to be run as blockchain tests.

@winsvega can you keep a list of these names in a file in the tests repository?

gumb0 · 2017-06-02T11:11:29Z

test/tools/jsontests/BlockChainTests.cpp

-	if (!Options::get().fillchain) //fill blockchain through state tests
+	bool isRunFromStateTests = true;
+	string suitename = boost::unit_test::framework::get<boost::unit_test::test_suite>(boost::unit_test::framework::current_test_case().p_parent_id).p_name;
+	if (suitename.find("StateTests") == string::npos)


@winsvega Is there any way to pass the flag/enum to this function telling that we're in a special case of StateTests instead of querying suite name from boost? Why is doBlockchainTests called from StateTests suite anyway?

gumb0 · 2017-06-02T12:12:32Z

After discussion with @winsvega he agreed to try a different approach with fixing tests for this - rolling back current workaround changes in testeth code and just convert affected json files

chfast

I need more time to review test changes.

gumb0 · 2017-06-02T15:05:31Z

@chfast Don't spend your time on that yet, @winsvega plans now a different approach

codecov-io · 2017-06-02T15:45:22Z

Codecov Report

Merging #4066 into develop will increase coverage by 0.3%.
The diff coverage is 99.18%.

@@            Coverage Diff             @@
##           develop    #4066     +/-   ##
==========================================
+ Coverage    65.41%   65.71%   +0.3%     
==========================================
  Files          301      303      +2     
  Lines        22882    22998    +116     
==========================================
+ Hits         14968    15114    +146     
+ Misses        7914     7884     -30

Impacted Files	Coverage Δ
test/tools/jsontests/vm.h	`100% <ø> (ø)`	⬆️
libethcore/Common.h	`90% <ø> (ø)`	⬆️
libethereum/Block.h	`60.86% <ø> (ø)`	⬆️
test/tools/libtesteth/TestHelper.cpp	`50.54% <ø> (-0.71%)`	⬇️
libevm/ExtVMFace.h	`78.57% <ø> (-0.31%)`	⬇️
test/tools/libtesteth/TestHelper.h	`50% <ø> (ø)`	⬆️
libethereum/ExtVM.h	`100% <ø> (ø)`	⬆️
libevmcore/Instruction.cpp	`17.5% <ø> (ø)`	⬆️
libdevcrypto/Common.h	`90% <ø> (ø)`	⬆️
libevm/VM.cpp	`91.78% <100%> (+1.49%)`	⬆️
... and 29 more

chfast · 2017-06-02T19:16:30Z

test/tools/jsontests/vm.cpp

+{
+	if (_number < envInfo().number() && _number >= (std::max<u256>(256, envInfo().number()) - 256))
+		return sha3(toString(_number));
+	else


There should be no else.

gumb0 added the in progress label May 3, 2017

gumb0 added this to the Metropolis milestone May 3, 2017

gumb0 force-pushed the eip96 branch 3 times, most recently from e44f355 to f5f802a Compare May 4, 2017 10:01

chfast mentioned this pull request May 4, 2017

[META] Byzantium implementation progress #4050

Closed

17 tasks

axic reviewed May 5, 2017

View reviewed changes

gumb0 force-pushed the eip96 branch from f5f802a to cd5fae2 Compare May 9, 2017 20:15

axic reviewed May 11, 2017

View reviewed changes

gumb0 mentioned this pull request May 11, 2017

Update Blockchain Transition tests for BLOCKHASH refactoring EIP ethereum/tests#167

Closed

gumb0 force-pushed the eip96 branch 9 times, most recently from aeec7b8 to c4dd751 Compare May 17, 2017 13:36

gumb0 requested review from chfast, winsvega, chriseth and pirapira May 17, 2017 15:27

gumb0 added needs review and removed in progress labels May 17, 2017

pirapira reviewed May 18, 2017

View reviewed changes

pirapira mentioned this pull request May 18, 2017

Create blockhash_refactoring.md ethereum/EIPs#210

Merged

gumb0 added 3 commits May 19, 2017 14:00

EIP96 - BLOCKHASH costs 20 gas before Metropolis and 800 in Metropolis

35d9284

Unit tests for BLOCKHASH-refactoring-related code in Block & ExtVM cl…

bad7222

…asses

Minor improvement after review comments

36b3a21

gumb0 force-pushed the eip96 branch from c4dd751 to 36b3a21 Compare May 19, 2017 12:27

chfast approved these changes May 19, 2017

View reviewed changes

chriseth reviewed May 19, 2017

View reviewed changes

chriseth approved these changes May 19, 2017

View reviewed changes

gumb0 mentioned this pull request May 19, 2017

Don't ask for lastHashes from Blockchain after Metropolis #4084

Closed

gumb0 and others added 2 commits May 19, 2017 19:32

Update blockhash contract through a direct Executive::call, instead o…

2e3d9c4

…f executing fake Transaction. Thus SystemAddresss's nonce won't be incremented as EIP96 requires.

Merge remote-tracking branch 'origin/develop' into eip96

a11c37f

# Conflicts: # test/tools/libtesteth/TestHelper.h

Fix incorrect data access because of bytesConstRef pointing to tempor…

6cd51fd

…ary stack object.

gumb0 mentioned this pull request Jun 1, 2017

Update tests for BLOCKHASH refactoring EIP ethereum/tests#176

Merged

winsvega force-pushed the eip96 branch from f69de9a to 988c145 Compare June 1, 2017 22:27

gumb0 commented Jun 2, 2017

View reviewed changes

chfast suggested changes Jun 2, 2017

View reviewed changes

update tests

f90ffd2

winsvega force-pushed the eip96 branch from ce37e4d to f90ffd2 Compare June 2, 2017 15:45

chfast approved these changes Jun 2, 2017

View reviewed changes

style fix

0b2e08e

winsvega merged commit 8bd9a8b into develop Jun 3, 2017

winsvega removed the needs review label Jun 3, 2017

gumb0 mentioned this pull request Jun 9, 2017

LastHashes refactoring #4126

Merged

axic deleted the eip96 branch July 10, 2017 09:53

ekpyron mentioned this pull request Aug 13, 2018

blockhash always returns zero in solidity tests for constantinople #5184

Closed

		@@ -67,6 +67,8 @@ const Address dev::ZeroAddress = Address();

		const Address dev::MaxAddress = Address("0xffffffffffffffffffffffffffffffffffffffff");

		const Address dev::SuperuserAddress = Address("0xfffffffffffffffffffffffffffffffffffffffe");


		boost::filesystem::directory_iterator iterator_tmp(fillersPath);
		dirIt iterator_tmp(fillersPath);

EIP96 - blockhash refactoring #4066

EIP96 - blockhash refactoring #4066

Conversation

gumb0 commented May 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic May 11, 2017 • 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

gumb0 commented May 17, 2017 • 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

gumb0 commented May 19, 2017 • 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

pirapira commented May 30, 2017

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

gumb0 Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Jun 2, 2017

chfast left a comment

Choose a reason for hiding this comment

gumb0 commented Jun 2, 2017

codecov-io commented Jun 2, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

gumb0 commented May 3, 2017 •

edited

Loading

axic May 11, 2017 •

edited

Loading

gumb0 commented May 17, 2017 •

edited

Loading

gumb0 commented May 19, 2017 •

edited

Loading

gumb0 Jun 2, 2017 •

edited

Loading

codecov-io commented Jun 2, 2017 •

edited

Loading