LastHashes refactoring #4126

gumb0 · 2017-06-08T14:32:34Z

Follow-up to #4066
Resolves #4084

Refactoring basically replaces LastHashes vector constructed by Blockchain class and passed to Executive when we need to execute some VM code with LastBlockHashesFace interface provided by BlockChain class.
The new interface constructs exactly the same vector, and the behavior before Metropolis is not changed.

After Metropolis BLOCKHASH doesn't access this interface (performs a call to contract instead) and this way we avoid constructing unnecessary hash vector.

gumb0 · 2017-06-08T17:33:13Z

libdevcore/vector_ref.h

@@ -1,5 +1,7 @@
 #pragma once

+#include "db.h"


This vector_ref.h file uses DEV_LDB macro defined in db.h
This came up after reordering of #include's in another file

gumb0 · 2017-06-08T17:36:16Z

libethashseal/Ethash.h

@@ -45,7 +45,7 @@ class Ethash: public SealEngineBase

 StringHashMap jsInfo(BlockHeader const& _bi) const override;
 void verify(Strictness _s, BlockHeader const& _bi, BlockHeader const& _parent, bytesConstRef _block) const override;
- void verifyTransaction(ImportRequirements::value _ir, TransactionBase const& _t, EnvInfo const& _env) const override;
+ void verifyTransaction(ImportRequirements::value _ir, TransactionBase const& _t, BlockHeader const& _header, u256 const& _startGasUsed) const override;


The reason for this change is that I find it weird that transaction validation depends on EnvInfo which describes an enviroment for VM execution.
Also EnvInfo now contains a pointer to LastBlockHashesFace, which shouldn't have to do with transation validation.

I am happy to see these dependencies go.

codecov-io · 2017-06-08T17:41:58Z

Codecov Report

Merging #4126 into develop will increase coverage by 0.21%.
The diff coverage is 76.99%.

@@             Coverage Diff             @@
##           develop    #4126      +/-   ##
===========================================
+ Coverage    66.94%   67.16%   +0.21%     
===========================================
  Files          299      301       +2     
  Lines        23138    23126      -12     
===========================================
+ Hits         15490    15532      +42     
+ Misses        7648     7594      -54

Impacted Files	Coverage Δ
libethereum/Block.h	`60.86% <ø> (ø)`	⬆️
test/tools/jsontests/vm.h	`100% <ø> (ø)`	⬆️
libethcore/SealEngine.h	`75% <ø> (ø)`	⬆️
libethereum/Client.h	`68.42% <ø> (ø)`	⬆️
test/tools/libtesteth/ImportTest.h	`100% <ø> (ø)`	⬆️
libethereum/State.h	`95.12% <ø> (ø)`	⬆️
libethereum/Client.cpp	`25.4% <ø> (+0.84%)`	⬆️
libethashseal/Ethash.h	`42.85% <ø> (ø)`	⬆️
libethereum/Interface.h	`16.66% <ø> (ø)`	⬆️
libethereum/Executive.h	`66.66% <ø> (ø)`	⬆️
... and 30 more

gumb0 · 2017-06-08T17:45:34Z

libethereum/Client.h

@@ -100,10 +100,6 @@ class Client: public ClientBase, protected Worker
 /// Queues a block for import.
 ImportResult queueBlock(bytes const& _block, bool _isSafe = false);

- using Interface::call; // to remove warning about hiding virtual function
- /// Makes the given call. Nothing is recorded into the state. This cheats by creating a null address and endowing it with a lot of ETH.
- ExecutionResult call(Address _dest, bytes const& _data = bytes(), u256 _gas = 125000, u256 _value = 0, u256 _gasPrice = 1 * ether, Address const& _from = Address());


This is not used

gumb0 · 2017-06-08T17:48:55Z

libethereum/Interface.h

@@ -95,13 +95,6 @@ class Interface
 ExecutionResult call(Secret const& _secret, u256 _value, Address _dest, bytes const& _data, u256 _gas, u256 _gasPrice, BlockNumber _blockNumber, FudgeFactor _ff = FudgeFactor::Strict) { return call(toAddress(_secret), _value, _dest, _data, _gas, _gasPrice, _blockNumber, _ff); }
 ExecutionResult call(Secret const& _secret, u256 _value, Address _dest, bytes const& _data, u256 _gas, u256 _gasPrice, FudgeFactor _ff = FudgeFactor::Strict) { return call(toAddress(_secret), _value, _dest, _data, _gas, _gasPrice, _ff); }

- /// Does the given creation. Nothing is recorded into the state.


These were not used

winsvega · 2017-06-09T10:30:50Z

ethvm/main.cpp

@@ -313,7 +313,7 @@ int main(int argc, char** argv)

 if (mode == Mode::Statistics)
 {
- cout << "Gas used: " << res.gasUsed << " (+" << t.baseGasRequired(se->evmSchedule(envInfo)) << " for transaction, -" << res.gasRefunded << " refunded)" << endl;
+ cout << "Gas used: " << res.gasUsed << " (+" << t.baseGasRequired(se->evmSchedule(envInfo.number())) << " for transaction, -" << res.gasRefunded << " refunded)" << endl;


will this affect VMTests ?

Definitely not, this change is just giving evmSchedule the only thing it needs - block number instead of the whole EnvInfo

winsvega

tests work fine

chfast · 2017-06-13T12:08:41Z

libethcore/SealEngine.h

@@ -75,7 +75,7 @@ class SealEngineFace
 ChainOperationParams const& chainParams() const { return m_params; }
 void setChainParams(ChainOperationParams const& _params) { m_params = _params; }
 SealEngineFace* withChainParams(ChainOperationParams const& _params) { setChainParams(_params); return this; }
- virtual EVMSchedule const& evmSchedule(EnvInfo const&) const = 0;
+ virtual EVMSchedule const& evmSchedule(u256 const&) const = 0;


Name the parameter?

chfast · 2017-06-13T12:08:53Z

libethcore/SealEngine.h

@@ -105,7 +105,7 @@ class SealEngineBase: public SealEngineFace
 m_onSealGenerated(ret.out());
 }
 void onSealGenerated(std::function<void(bytes const&)> const& _f) override { m_onSealGenerated = _f; }
- EVMSchedule const& evmSchedule(EnvInfo const&) const override;
+ EVMSchedule const& evmSchedule(u256 const&) const override;


Name the parameter?

chfast · 2017-06-13T12:16:11Z

libevm/ExtVMFace.h

@@ -228,30 +227,32 @@ class EnvInfo
 {
 public:
 EnvInfo() {}
- EnvInfo(BlockHeader const& _current, LastHashes const& _lh = LastHashes(), u256 const& _gasUsed = u256()):
+ EnvInfo(BlockHeader const& _current, std::shared_ptr<LastBlockHashesFace const> _lh, u256 const& _gasUsed):


I can't to figure out the reason this is the shared_ptr.

I tried to get rid of it and pass LastBlockHashesFace const& around, but I gave up because some test code need setting & resetting this interface into EnvInfo (calling EnvInfo::setLastHashes())

This code to be exact: https:/ethereum/cpp-ethereum/pull/4126/files#diff-e01cd53eebd917cc35354ffc7719d8adR295

Probably there's a way to change this test code somehow... modifying EnvInfo is a weird thing to do in general

I don't like the shared_ptr here either, but can't tell you how to fix it.

pirapira · 2017-06-13T17:42:33Z

libevm/ExtVMFace.h

- void setLastHashes(LastHashes&& _lh) { m_lastHashes = _lh; }
-
+ void setLastHashes(std::shared_ptr<LastBlockHashesFace const> _lh) { m_lastHashes = _lh; }
+ 


Spurious space.

chfast · 2017-06-14T10:44:51Z

I like the direction it is going. And I also want less dependencies on test framework. Just can we use unique PTR instead?

gumb0 · 2017-06-14T11:20:43Z

@chfast Hm, making it unique_ptr means we'll construct the new instance of this LastBlockHashes implementation (https:/ethereum/cpp-ethereum/pull/4126/files#diff-4bd28474d53dacc284d9432a1a785c10R128) on every tx execution. Which kind of defeats the purpose of caching the returned hashes vector in LastBlockHashes::m_lastHashes

gumb0 · 2017-06-14T11:26:40Z

I'm trying to replace shared_ptr<LastBlockHashesFace const> with LastBlockHashesFace const&.
If it doesn't work out, I'll go with unique_ptr

If it does work out, it probably better should go into separate PR

gumb0 · 2017-06-15T16:17:42Z

ethvm/main.cpp

@@ -274,6 +283,8 @@ int main(int argc, char** argv)
 state.addBalance(sender, value);

 unique_ptr<SealEngineFace> se(ChainParams(genesisInfo(networkName)).createSealEngine());
+ LastBlockHashes lastBlockHashes;
+ EnvInfo const envInfo(blockHeader, lastBlockHashes, 0);


Looks like previously ethvm tool didn't work for the code containing BLOCKHASH. Now any BLOCKHASH will return 0

gumb0 · 2017-06-20T15:05:52Z

I've replaced shared_ptr<LastBlockHashes const> with LastBlockHashes const& and made EnvInfo struct immutable (now when test code needs to set a new last hashes, it recreates EnvInfo)

Tests are green now, please review the changes @chfast @gcolvin @pirapira @winsvega
(I somehow messed up making it a separate commit and it's all now in one forced-pushed commit, sorry)

pirapira · 2017-06-20T16:56:36Z

libethereum/BlockChain.cpp

+ h256s precedingHashes(h256 const& _mostRecentHash) const override
+ {
+ Guard l(m_lastHashesMutex);
+ if (m_lastHashes.empty() || m_lastHashes.back() != _mostRecentHash)


Do you mean front instead of back?

In a following line I see m_lastHashes[0] = _mostRecentHash.

Yeah, looks like it was incorrect in the original code, nice catch!

pirapira · 2017-06-20T16:57:47Z

libethereum/BlockChain.cpp

-LastHashes BlockChain::lastHashes(h256 const& _parent) const
-{
- Guard l(x_lastLastHashes);
- if (m_lastLastHashes.empty() || m_lastLastHashes.back() != _parent)


I see the back is inherited from here.

…erface.

gumb0 added the in progress label Jun 8, 2017

gumb0 force-pushed the lasthashes-refactoring2 branch 3 times, most recently from bbe1350 to 555050f Compare June 8, 2017 16:34

gumb0 commented Jun 8, 2017

View reviewed changes

gumb0 force-pushed the lasthashes-refactoring2 branch from 555050f to 6951d6d Compare June 8, 2017 17:41

gumb0 commented Jun 8, 2017

View reviewed changes

gumb0 force-pushed the lasthashes-refactoring2 branch from 6951d6d to ac33ed8 Compare June 8, 2017 18:01

gumb0 added needs review and removed in progress labels Jun 9, 2017

gumb0 added this to the Metropolis milestone Jun 9, 2017

gumb0 requested review from chfast, winsvega and chriseth June 9, 2017 09:39

winsvega reviewed Jun 9, 2017

View reviewed changes

winsvega approved these changes Jun 13, 2017

View reviewed changes

chfast reviewed Jun 13, 2017

View reviewed changes

gcolvin approved these changes Jun 13, 2017

View reviewed changes

pirapira reviewed Jun 13, 2017

View reviewed changes

gumb0 force-pushed the lasthashes-refactoring2 branch 3 times, most recently from 0a4d215 to 778418e Compare June 15, 2017 15:46

gumb0 commented Jun 15, 2017

View reviewed changes

gumb0 force-pushed the lasthashes-refactoring2 branch from 778418e to 14997eb Compare June 15, 2017 16:27

gumb0 force-pushed the lasthashes-refactoring2 branch 4 times, most recently from 6e75183 to f3e90f5 Compare June 20, 2017 13:48

pirapira reviewed Jun 20, 2017

View reviewed changes

chfast approved these changes Jun 20, 2017

View reviewed changes

Refactoring to replace LastHashes vector with LastBlockHashesFace int…

b45e406

…erface.

gumb0 force-pushed the lasthashes-refactoring2 branch from f3e90f5 to b45e406 Compare June 21, 2017 09:44

gumb0 merged commit d06c3be into develop Jun 21, 2017

gumb0 removed the needs review label Jun 21, 2017

gumb0 deleted the lasthashes-refactoring2 branch June 21, 2017 11:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LastHashes refactoring #4126

LastHashes refactoring #4126

gumb0 commented Jun 8, 2017 •

edited

Loading

gumb0 Jun 8, 2017

gumb0 Jun 8, 2017 •

edited

Loading

gcolvin Jun 13, 2017

codecov-io commented Jun 8, 2017 •

edited

Loading

gumb0 Jun 8, 2017

gumb0 Jun 8, 2017

winsvega Jun 9, 2017

gumb0 Jun 9, 2017

winsvega left a comment

chfast Jun 13, 2017

chfast Jun 13, 2017

chfast Jun 13, 2017

gumb0 Jun 13, 2017

gumb0 Jun 13, 2017 •

edited

Loading

gcolvin Jun 13, 2017

pirapira Jun 13, 2017

chfast commented Jun 14, 2017

gumb0 commented Jun 14, 2017

gumb0 commented Jun 14, 2017

gumb0 Jun 15, 2017 •

edited

Loading

gumb0 commented Jun 20, 2017

pirapira Jun 20, 2017

gumb0 Jun 20, 2017

pirapira Jun 20, 2017

		void setLastHashes(LastHashes&& _lh) { m_lastHashes = _lh; }

		void setLastHashes(std::shared_ptr<LastBlockHashesFace const> _lh) { m_lastHashes = _lh; }

LastHashes refactoring #4126

LastHashes refactoring #4126

Conversation

gumb0 commented Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

gumb0 Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 8, 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

winsvega left a 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 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented Jun 14, 2017

gumb0 commented Jun 14, 2017

gumb0 commented Jun 14, 2017

gumb0 Jun 15, 2017 • edited Loading

Choose a reason for hiding this comment

gumb0 commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Jun 8, 2017 •

edited

Loading

gumb0 Jun 8, 2017 •

edited

Loading

codecov-io commented Jun 8, 2017 •

edited

Loading

gumb0 Jun 13, 2017 •

edited

Loading

gumb0 Jun 15, 2017 •

edited

Loading