-
Notifications
You must be signed in to change notification settings - Fork 2.2k
EIP96 - blockhash refactoring #4066
Changes from 7 commits
8fe97ec
6bb7464
39100c0
bceca2c
35d9284
bad7222
36b3a21
2e3d9c4
a11c37f
6cd51fd
f90ffd2
0b2e08e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
/* | ||
This file is part of cpp-ethereum. | ||
|
||
cpp-ethereum is free software: you can redistribute it and/or modify | ||
it under the terms of the GNU General Public License as published by | ||
the Free Software Foundation, either version 3 of the License, or | ||
(at your option) any later version. | ||
|
||
cpp-ethereum is distributed in the hope that it will be useful, | ||
but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
GNU General Public License for more details. | ||
|
||
You should have received a copy of the GNU General Public License | ||
along with cpp-ethereum. If not, see <http://www.gnu.org/licenses/>. | ||
*/ | ||
#include "../GenesisInfo.h" | ||
|
||
static dev::h256 const c_genesisStateRootMetropolisTransitionTest; | ||
static std::string const c_genesisInfoMetropolisTransitionTest = std::string() + | ||
R"E( | ||
{ | ||
"sealEngine": "NoProof", | ||
"params": { | ||
"accountStartNonce": "0x00", | ||
"maximumExtraDataSize": "0x20", | ||
"homsteadForkBlock": "0x00", | ||
"daoHardforkBlock": "0xfffffffffffffff", | ||
"EIP150ForkBlock": "0x00", | ||
"EIP158ForkBlock": "0x00", | ||
"metropolisForkBlock": "0x02", | ||
"minGasLimit": "0x1388", | ||
"maxGasLimit": "7fffffffffffffff", | ||
"tieBreakingGas": false, | ||
"gasLimitBoundDivisor": "0x0400", | ||
"minimumDifficulty": "0x020000", | ||
"difficultyBoundDivisor": "0x0800", | ||
"durationLimit": "0x0d", | ||
"blockReward": "0x4563918244F40000", | ||
"registrar" : "0xc6d9d2cd449a754c494264e1809c50e34d64562b", | ||
"networkID" : "0x1", | ||
"chainID": "0x01", | ||
"allowFutureBlocks" : true | ||
}, | ||
"genesis": { | ||
"nonce": "0x0000000000000042", | ||
"difficulty": "0x400000000", | ||
"mixHash": "0x0000000000000000000000000000000000000000000000000000000000000000", | ||
"author": "0x0000000000000000000000000000000000000000", | ||
"timestamp": "0x00", | ||
"parentHash": "0x0000000000000000000000000000000000000000000000000000000000000000", | ||
"extraData": "0x11bbe8db4e347b4e8c937c1c8370e4b5ed33adb3db69cbdb7a38e1e50b1b82fa", | ||
"gasLimit": "0x1388" | ||
}, | ||
"accounts": { | ||
"0000000000000000000000000000000000000001": { "wei": "1", "precompiled": { "name": "ecrecover", "linear": { "base": 3000, "word": 0 } } }, | ||
"0000000000000000000000000000000000000002": { "wei": "1", "precompiled": { "name": "sha256", "linear": { "base": 60, "word": 12 } } }, | ||
"0000000000000000000000000000000000000003": { "wei": "1", "precompiled": { "name": "ripemd160", "linear": { "base": 600, "word": 120 } } }, | ||
"0000000000000000000000000000000000000004": { "wei": "1", "precompiled": { "name": "identity", "linear": { "base": 15, "word": 3 } } }, | ||
"0000000000000000000000000000000000000005": { "wei": "1", "precompiled": { "name": "modexp" } } | ||
} | ||
} | ||
)E"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might there be any global variable initialization race condition problems here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
Address toAddress(std::string const& _s) | ||
{ | ||
try | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,6 +128,7 @@ void Block::resetCurrent(u256 const& _timestamp) | |
m_committedToSeal = false; | ||
|
||
performIrregularModifications(); | ||
updateBlockhashContract(); | ||
} | ||
|
||
SealEngineFace* Block::sealEngine() const | ||
|
@@ -692,6 +693,29 @@ void Block::performIrregularModifications() | |
} | ||
} | ||
|
||
void Block::updateBlockhashContract() | ||
{ | ||
u256 const blockNumber = info().number(); | ||
|
||
u256 const metropolisForkBlock = m_sealEngine->chainParams().u256Param("metropolisForkBlock"); | ||
if (blockNumber == metropolisForkBlock) | ||
{ | ||
m_state.createContract(c_blockhashContractAddress); | ||
m_state.setNewCode(c_blockhashContractAddress, bytes(c_blockhashContractCode)); | ||
m_state.commit(State::CommitBehaviour::KeepEmptyAccounts); | ||
} | ||
|
||
if (blockNumber >= metropolisForkBlock) | ||
{ | ||
u256 const nonce = transactionsFrom(SystemAddress); | ||
u256 const gas = 1000000; | ||
Transaction tx(0, 0, gas, c_blockhashContractAddress, m_previousBlock.hash().asBytes(), nonce); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again out crappy vector_ref interface, but I think you can go from h256 to bytes_ref without going through std::vector. It might look like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, that's fine then. |
||
tx.forceSender(SystemAddress); | ||
// passing empty lastHashes - assuming the contract doesn't use BLOCKHASH | ||
m_state.execute(EnvInfo(info(), {}, 0), *m_sealEngine, tx, Permanence::Committed); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this doesn't increment the nonce. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, I think it does. Shouldn't it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if it should or not. So I need to ask. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
} | ||
|
||
void Block::commitToSeal(BlockChain const& _bc, bytes const& _extraData) | ||
{ | ||
if (isSealed()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -134,3 +134,26 @@ void ExtVM::suicide(Address _a) | |
m_s.subBalance(myAddress, m_s.balance(myAddress)); | ||
ExtVMFace::suicide(_a); | ||
} | ||
|
||
h256 ExtVM::blockHash(u256 _number) | ||
{ | ||
u256 const currentNumber = envInfo().number(); | ||
|
||
if (_number >= currentNumber || _number < (std::max<u256>(256, currentNumber) - 256)) | ||
return h256(); | ||
|
||
if (currentNumber < m_sealEngine.chainParams().u256Param("metropolisForkBlock") + 256) | ||
{ | ||
assert(envInfo().lastHashes().size() > (unsigned)(currentNumber - 1 - _number)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The assertion seems to fail if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We won't get here because of the check at line 142, requested There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, seems valid. |
||
return envInfo().lastHashes()[(unsigned)(currentNumber - 1 - _number)]; | ||
} | ||
|
||
u256 const nonce = m_s.getNonce(caller); | ||
u256 const gas = 1000000; | ||
Transaction tx(0, 0, gas, c_blockhashContractAddress, toBigEndian(_number), nonce); | ||
tx.forceSender(caller); | ||
|
||
ExecutionResult res; | ||
std::tie(res, std::ignore) = m_s.execute(envInfo(), m_sealEngine, tx, Permanence::Reverted); | ||
return h256(res.output); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,6 +72,7 @@ struct EVMSchedule | |
unsigned extcodecopyGas = 20; | ||
unsigned balanceGas = 20; | ||
unsigned suicideGas = 0; | ||
unsigned blockhashGas = 20; | ||
unsigned maxCodeSize = unsigned(-1); | ||
|
||
bool staticCallDepthLimit() const { return !eip150Mode; } | ||
|
@@ -109,6 +110,7 @@ static const EVMSchedule EIP158Schedule = [] | |
static const EVMSchedule MetropolisSchedule = [] | ||
{ | ||
EVMSchedule schedule = EIP158Schedule; | ||
schedule.blockhashGas = 800; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's uncertain to me if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And I will ask in the allcoredev meeting ethereum/pm#14 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this seem to be strange. Shouldn't we just charge for the contract execution? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
schedule.haveRevert = true; | ||
schedule.haveReturnData = true; | ||
return schedule; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,14 @@ std::pair<bool, eth::owning_bytes_ref> FakeExtVM::call(CallParameters& _p) | |
return {true, eth::owning_bytes_ref{}}; // Return empty output. | ||
} | ||
|
||
h256 FakeExtVM::blockHash(u256 _number) | ||
{ | ||
if (_number < envInfo().number() && _number >= (std::max<u256>(256, envInfo().number()) - 256)) | ||
return sha3(toString(_number)); | ||
else | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There should be no |
||
return h256(); | ||
} | ||
|
||
void FakeExtVM::set(Address _a, u256 _myBalance, u256 _myNonce, map<u256, u256> const& _storage, bytes const& _code) | ||
{ | ||
get<0>(addresses[_a]) = _myBalance; | ||
|
@@ -96,7 +104,6 @@ EnvInfo FakeExtVM::importEnv(mObject& _o) | |
info.setTimestamp(toInt(_o["currentTimestamp"])); | ||
info.setAuthor(Address(_o["currentCoinbase"].get_str())); | ||
info.setNumber(toInt(_o["currentNumber"])); | ||
info.setLastHashes( lastHashes( info.number() ) ); | ||
return info; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this from the codebase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove it in a separate PR