EVM-C v3 #3510

chfast · 2017-01-18T22:50:05Z

This implements EVM-C v3 on the cpp-ethereum side, specified ethereum/evmjit#110.

codecov-io · 2017-04-05T10:27:13Z

Codecov Report

Merging #3510 into develop will decrease coverage by 0.07%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #3510      +/-   ##
===========================================
- Coverage     66.1%   66.03%   -0.08%     
===========================================
  Files          308      308              
  Lines        22896    22903       +7     
===========================================
- Hits         15135    15123      -12     
- Misses        7761     7780      +19

Impacted Files	Coverage Δ
libevm/JitVM.cpp	`90.29% <100%> (+0.37%)`	⬆️
libdevcore/FixedHash.h	`93.81% <100%> (+0.13%)`	⬆️
test/tools/fuzzTesting/fuzzHelper.cpp	`33.33% <0%> (-5.4%)`	⬇️
libp2p/Host.cpp	`72.36% <0%> (-1.06%)`	⬇️
libp2p/Common.cpp	`52% <0%> (-0.8%)`	⬇️
libp2p/NodeTable.cpp	`39.52% <0%> (-0.6%)`	⬇️

axic · 2017-05-05T09:12:31Z

I'm not really familiar with cpp-ethereum internals so cannot comment much on this.

gumb0 · 2017-05-08T10:12:06Z

libevm/JitVM.cpp

@@ -21,9 +21,9 @@ inline evm_uint160be toEvmC(Address _addr)
 	return *reinterpret_cast<evm_uint160be*>(&_addr);
 }

-inline Address fromEvmC(evm_uint160be _addr)
+inline Address fromEvmC(evm_uint160be const* _addr)


Could we pass the const reference here (and in asUint) and reinterpret_cast the reference?
I think it'd be cleaner to dereference the pointer on the calling side, the thing like asUint(ptr) gave me a microsecond of confusion, it's not immediately clear that the function converts the value pointed to and not the pointer

Yes, that make sense. I recently have noticed they look bad. I will also try to use std::copy instead of reinterpret_cast if proven to generate the same code.

gumb0 · 2017-05-08T10:44:54Z

libevm/JitVM.cpp

 }

-void evm_update(
+void updateState(


Why do you rename this but not evm_query?
and also evm_call & evm_get_tx_context are snake_case, while updateState & getBlockHash are camelCase

With this PR I renamed some changed function to match our naming convention. I will do the same for the rest.

gumb0 · 2017-05-08T13:38:13Z

libevm/JitVM.cpp

 	{
 		assert(_outputSize == 20);
-		u256 gas = _gas;
+		u256 gas = _msg->gas;
+		// TODO: How it knows who the sender is?


for create ExtVM gets it from myAddress member initialized in constructor

gumb0 · 2017-05-08T14:11:05Z

libevm/JitVM.cpp

 {
-	return *reinterpret_cast<evm_uint160be*>(&_addr);
+	return *reinterpret_cast<evm_uint160be const*>(&_addr);


Can we do just return reinterpret_cast<evm_uint160be const&>(_addr); ?

I was so long in C world, I forgot this is possible. I've changed that, however it feels a bit awkward.

gumb0 · 2017-05-08T14:22:37Z

libevm/JitVM.cpp

 	switch (_key)
 	{
 	case EVM_SSTORE:
 	{
-		auto index = asUint(_arg1->uint256be);
-		auto value = asUint(_arg2->uint256be);
+		auto index = fromEvmC(_arg1->uint256be);


auto & fromEvmC name combination is a little confusing here, it's not clear to what we convert

gumb0 · 2017-05-08T14:28:09Z

libevm/JitVM.cpp

+	params.gas = _msg->gas;
+	params.apparentValue = value;
+	params.valueTransfer = _msg->kind == EVM_DELEGATECALL ? 0 : params.apparentValue;
+	params.senderAddress = fromEvmC(_msg->sender);


Why did we get rid of conditional here and in apparentValue?

Because it is now VM responsibility to prepare correct message object. E.g. VM has to set correct sender depending on it is executing DELEGATECALL or CALL. At the moment EVMJIT handles that similarly in https://github.com/ethereum/evmjit/pull/112/files#diff-0ee981fc82c3fceb0467c22ad927823cR166. But it can be done better.

gumb0 · 2017-05-09T09:22:25Z

Windows build fails on std::copy(addr.begin(), addr.end(), _outputData)

chfast · 2017-05-09T12:09:38Z

Fixed. Fancy array iterator MSVC has :)

gumb0 · 2017-05-09T12:41:34Z

GCC is not happy now :)

gumb0 · 2017-05-09T12:44:20Z

Maybe something like just std::copy_n(addr.data(), Address::size, _outputData) ?

chfast · 2017-05-09T12:49:19Z

I will fix it :)

gumb0 · 2017-05-09T13:55:10Z

libdevcore/FixedHash.h

@@ -141,6 +141,12 @@ class FixedHash
 	/// @returns a constant byte pointer to the object's data.
 	byte const* data() const { return m_data.data(); }

+	/// @returns begin iterator.
+	auto begin() const -> typename std::array<byte, N>::const_iterator { return m_data.begin(); }


Shouldn't we call these cbegin & cend ?

I don't think some, cbegin() is only useful when you have a non-const object, with begin() const and begin() and you want to enforce getting const iterator.

chfast added in progress labels Jan 18, 2017

chfast mentioned this pull request Jan 18, 2017

EVM-C v3 ethereum/evmjit#112

Merged

chfast added 10 commits January 27, 2017 16:30

EVM-C: use evm_factory

9339e34

EVM-C: callback arguments passed by pointer

8998053

EVM-C: update evm_query_fn prototype

22ed06e

Update EVMJIT: fixes and cleanups

53b6559

Fix EVM constant on Windows

9af0593

EVM-C v3

2fd29a3

EVM-C: remove message items from query

f5c1bc0

EVM-C: Implement evm_get_tx_context()

c5e5f22

EVM-C: Remove transaction context items from query key

a446151

EVM-C: Update state access

fce9066

chfast force-pushed the evmc-v3 branch from 905dc2e to ee68610 Compare January 27, 2017 15:33

EVM-C: change evm_call to include message struct

57bfa5a

chfast force-pushed the evmc-v3 branch from ee68610 to 57bfa5a Compare January 27, 2017 16:50

chfast added needs review and removed in progress labels Apr 5, 2017

gumb0 reviewed May 8, 2017

View reviewed changes

chfast added 2 commits May 8, 2017 14:29

Merge remote-tracking branch 'origin/develop' into evmc-v3

2539cb2

EVM-C: update EVMJIT

25aa0c9

gumb0 reviewed May 8, 2017

View reviewed changes

JitVM: cleanup EVM-C conversion functions

319e3a4

chfast force-pushed the evmc-v3 branch from 4cb9b10 to 319e3a4 Compare May 8, 2017 13:42

gumb0 reviewed May 8, 2017

View reviewed changes

chfast force-pushed the evmc-v3 branch from bfd7ca1 to fd49fcc Compare May 9, 2017 09:50

JitVM: cleanup EVM-C conversion functions

26ea4a2

chfast force-pushed the evmc-v3 branch from fd49fcc to 26ea4a2 Compare May 9, 2017 12:54

gumb0 reviewed May 9, 2017

View reviewed changes

gumb0 approved these changes May 10, 2017

View reviewed changes

chfast merged commit db8a4a2 into develop May 10, 2017

chfast removed needs review labels May 10, 2017

chfast deleted the evmc-v3 branch May 10, 2017 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EVM-C v3 #3510

EVM-C v3 #3510

chfast commented Jan 18, 2017 •

edited

Loading

codecov-io commented Apr 5, 2017 •

edited

Loading

axic commented May 5, 2017

gumb0 May 8, 2017

chfast May 8, 2017

gumb0 May 8, 2017

chfast May 8, 2017

gumb0 May 8, 2017

gumb0 May 8, 2017

chfast May 8, 2017

gumb0 May 8, 2017 •

edited

Loading

gumb0 May 8, 2017 •

edited

Loading

chfast May 8, 2017

gumb0 commented May 9, 2017

chfast commented May 9, 2017

gumb0 commented May 9, 2017

gumb0 commented May 9, 2017 •

edited

Loading

chfast commented May 9, 2017

gumb0 May 9, 2017 •

edited

Loading

chfast May 9, 2017

EVM-C v3 #3510

EVM-C v3 #3510

Conversation

chfast commented Jan 18, 2017 • edited Loading

codecov-io commented Apr 5, 2017 • edited Loading

Codecov Report

axic commented May 5, 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 May 8, 2017 • edited Loading

Choose a reason for hiding this comment

gumb0 May 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented May 9, 2017

chfast commented May 9, 2017

gumb0 commented May 9, 2017

gumb0 commented May 9, 2017 • edited Loading

chfast commented May 9, 2017

gumb0 May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented Jan 18, 2017 •

edited

Loading

codecov-io commented Apr 5, 2017 •

edited

Loading

gumb0 May 8, 2017 •

edited

Loading

gumb0 May 8, 2017 •

edited

Loading

gumb0 commented May 9, 2017 •

edited

Loading

gumb0 May 9, 2017 •

edited

Loading