EIP214: STATICCALL #4057

yann300 · 2017-04-24T12:44:11Z

Resolves #3616.

codecov-io · 2017-04-24T13:34:03Z

Codecov Report

Merging #4057 into develop will decrease coverage by 0.17%.
The diff coverage is 64.28%.

@@            Coverage Diff             @@
##           develop   #4057      +/-   ##
==========================================
- Coverage    65.87%   65.7%   -0.18%     
==========================================
  Files          308     308              
  Lines        22913   22808     -105     
==========================================
- Hits         15095   14986     -109     
- Misses        7818    7822       +4

Impacted Files	Coverage Δ
libevmcore/Instruction.cpp	`38.46% <ø> (-9.04%)`	⬇️
libevm/VM.h	`76.92% <ø> (ø)`	⬆️
libevmcore/Instruction.h	`100% <ø> (ø)`	⬆️
libevm/VMFace.h	`84.61% <0%> (-7.06%)`	⬇️
libevmcore/EVMSchedule.h	`100% <100%> (ø)`	⬆️
libevm/ExtVMFace.h	`80% <100%> (-0.31%)`	⬇️
libethereum/Executive.cpp	`57.43% <100%> (ø)`	⬆️
libethereum/ExtVM.h	`100% <100%> (ø)`	⬆️
libevm/ExtVMFace.cpp	`100% <100%> (ø)`	⬆️
test/tools/jsontests/vm.cpp	`65.51% <100%> (ø)`	⬆️
... and 28 more

chfast · 2017-04-24T15:57:04Z

libevm/VM.cpp

@@ -214,19 +214,25 @@ void VM::interpretCases()

 		CASE(CREATE)
 		{
+			if (m_schedule->haveStaticCall && m_ext->staticCall)
+				throwBadInstruction();


Should be throwDisallowedStateChange().

chfast · 2017-04-24T15:57:54Z

libevm/VM.cpp

 		CASE(CALL)
 		CASE(CALLCODE)
 		{
+			// Pre-homestead


Remove this comment. It does not provided any information any more, as we are so used to hard-forks.

chfast · 2017-04-25T08:30:14Z

clang has showed an important issue. We missed something...

chfast · 2017-04-25T09:47:49Z

libethereum/ExtVM.h

@@ -94,6 +94,7 @@ class ExtVM: public ExtVMFace
 private:
 	State& m_s;  ///< A reference to the base state.
 	SealEngineFace const& m_sealEngine;
+	bool m_staticCall;


This is not needed.

chfast · 2017-04-25T09:48:27Z

libevm/ExtVMFace.h

@@ -201,6 +201,7 @@ struct CallParameters
 	u256 apparentValue;
 	u256 gas;
 	bytesConstRef data;
+	bool staticCall;


Initialize it here too: bool staticCall = false.

chfast

Looks good. @winsvega can you generate tests for this PR to check them before merging this.

gumb0 · 2017-04-28T14:48:47Z

libevm/VM.cpp

@@ -214,19 +214,24 @@ void VM::interpretCases()

 		CASE(CREATE)
 		{
+			if (m_schedule->haveStaticCall && m_ext->staticCall)


Do we really need a check for m_schedule->haveStaticCall here and in other places?
I think it's redundant, it can't be a static call if don't have it 😄

I agree. Keep only m_ext->staticCall check.
You can add before if: assert(!staticCall || haveStaticCall) which checks implication staticCall => haveStaticCall.

winsvega · 2017-05-19T08:44:15Z

CALLDATA for staticcall is not transfered to the subcall
strange gasUsed count when using different values for memory arguments of STATICCALL

Obsolate

chfast · 2017-05-19T11:24:55Z

Do you want me to help with rebase and cleaning up the history?

chfast · 2017-05-19T11:40:47Z

libethereum/Executive.cpp

@@ -240,7 +240,7 @@ bool Executive::execute()

 bool Executive::call(Address _receiveAddress, Address _senderAddress, u256 _value, u256 _gasPrice, bytesConstRef _data, u256 _gas)
 {
-	CallParameters params{_senderAddress, _receiveAddress, _receiveAddress, _value, _value, _gas, _data, {}};
+	CallParameters params(_senderAddress, _receiveAddress, _receiveAddress, _value, _value, _gas, _data, {});


This seems to be unnecessary change.

chfast · 2017-05-19T11:46:47Z

libevm/VM.cpp

@@ -214,19 +214,24 @@ void VM::interpretCases()

 		CASE(CREATE)
 		{
+			if (m_schedule->haveStaticCall && m_ext->staticCall)


I agree. Keep only m_ext->staticCall check.
You can add before if: assert(!staticCall || haveStaticCall) which checks implication staticCall => haveStaticCall.

chfast · 2017-05-19T11:47:38Z

libevm/VM.cpp

@@ -265,6 +270,9 @@ void VM::interpretCases()

 		CASE(SUICIDE)
 		{
+			if (m_schedule->haveStaticCall && m_ext->staticCall)


chfast · 2017-05-19T11:47:44Z

libevm/VM.cpp

@@ -340,6 +348,9 @@ void VM::interpretCases()

 		CASE(LOG0)
 		{
+			if (m_schedule->haveStaticCall && m_ext->staticCall)


chfast · 2017-05-19T11:49:50Z

libevm/VMCalls.cpp

-	if (m_OP == Instruction::CALL && !m_ext->exists(asAddress(m_SP[1])))
+	callParams->staticCall = (m_OP == Instruction::STATICCALL || m_ext->staticCall);
+
+	if ((m_OP == Instruction::CALL || m_OP == Instruction::STATICCALL) && !m_ext->exists(asAddress(m_SP[1])))


STATICCALL does not have a value argument, so this check does not apply to it.
cc @pirapira.

That's right.

chfast · 2017-05-19T12:40:32Z

test/tools/jsontests/StateTests.cpp

@@ -178,6 +178,7 @@ BOOST_AUTO_TEST_CASE(stRevertTest){}

 //Metropolis Tests
 BOOST_AUTO_TEST_CASE(stStackTests){}
+BOOST_AUTO_TEST_CASE(stStaticCall){}


@winsvega you forgot to update jsontests.

chriseth · 2017-05-19T13:08:15Z

libevm/VMCalls.cpp

 		if (m_SP[2] > 0 || m_schedule->zeroValueTransferChargesNewAccountGas())
 			m_runGas += toInt63(m_schedule->callNewAccountGas);

-	if (m_OP != Instruction::DELEGATECALL && m_SP[2] > 0)
+	if ((m_OP == Instruction::CALL || m_OP == Instruction::CALLCODE) && m_SP[2] > 0)


I hope this won't get us into trouble: We change for value transfer for callcode, although we do not consider it state changing.

What would be the test case for this???

CALLCODE with positive value transfer... probably we already have it... but worth trying during STATICCALL.

Added a test case in the Metropolis testing table https://docs.google.com/spreadsheets/d/1xat7UI8GtB4ZGVdlK5_XQSHJZaMThi4SrlcL8XMZb5Q/edit#gid=1412823514&range=C21

chriseth · 2017-05-19T13:10:13Z

Does it make sense to add a check somewhere which verifies that the state root is unchanged after a staticcall? Is that possible from an architectural standpoint?

chfast · 2017-05-19T13:12:39Z

@chriseth, it might be hard because we don't update the state root until the end of a transaction.

winsvega · 2017-05-19T13:40:40Z

we could check that state root is not changed in tests.
although in most tests I call staticcall after I've already changed something in state. otherwise it is not clear if the test has worked or not

yann300 added the in progress label Apr 24, 2017

chfast mentioned this pull request Apr 24, 2017

[META] Byzantium implementation progress #4050

Closed

17 tasks

yann300 changed the title ~~EIP214 - STATICCALL~~ EIP214 - STATICCALL [WIP] Apr 24, 2017

chfast changed the title ~~EIP214 - STATICCALL [WIP]~~ EIP214: STATICCALL Apr 24, 2017

chfast added this to the Metropolis milestone Apr 24, 2017

chfast suggested changes Apr 24, 2017

View reviewed changes

chfast suggested changes Apr 25, 2017

View reviewed changes

chfast previously approved these changes Apr 25, 2017

View reviewed changes

chfast assigned winsvega Apr 26, 2017

gumb0 reviewed Apr 28, 2017

View reviewed changes

winsvega force-pushed the staticcall branch from 1429443 to babba9b Compare May 19, 2017 08:41

chfast suggested changes May 19, 2017

View reviewed changes

chfast force-pushed the staticcall branch from e1bfa0e to f4f8d83 Compare May 19, 2017 12:39

chfast reviewed May 19, 2017

View reviewed changes

chfast force-pushed the staticcall branch from f4f8d83 to 9fa30ad Compare May 19, 2017 12:47

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

yann300 and others added 4 commits May 22, 2017 14:38

Add support for STATICCALL opcode

0173469

fix the offset for staticcall for memory allocation

388bb95

fix calldata transfer in staticcall

2b5f678

Fix some issues around STATICCALL implementation

c3ac297

chfast force-pushed the staticcall branch from 9fa30ad to c3ac297 Compare May 22, 2017 12:38

chfast added needs review and removed in progress labels May 22, 2017

winsvega force-pushed the staticcall branch from 899bb96 to 8ff8fce Compare May 23, 2017 15:19

winsvega and others added 2 commits May 24, 2017 10:30

Update JSON tests for STATICCALL

5b12f36

Travis CI: Disable EVMJIT tests

4e38b30

chfast force-pushed the staticcall branch from 8ff8fce to 4e38b30 Compare May 24, 2017 08:30

chfast merged commit 49cdc48 into develop May 24, 2017

chfast deleted the staticcall branch May 24, 2017 11:32

chfast removed the needs review label May 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP214: STATICCALL #4057

EIP214: STATICCALL #4057

yann300 commented Apr 24, 2017 •

edited by chfast

Loading

codecov-io commented Apr 24, 2017 •

edited

Loading

chfast Apr 24, 2017

chfast Apr 24, 2017

chfast commented Apr 25, 2017

chfast Apr 25, 2017

chfast Apr 25, 2017

chfast left a comment

gumb0 Apr 28, 2017

chfast May 19, 2017

winsvega commented May 19, 2017

chfast commented May 19, 2017

chfast May 19, 2017

chfast May 19, 2017

chfast May 19, 2017

chfast May 19, 2017

chfast May 19, 2017

pirapira May 19, 2017

chfast May 19, 2017

chriseth May 19, 2017

pirapira May 19, 2017

pirapira May 19, 2017 •

edited

Loading

pirapira May 19, 2017

chriseth commented May 19, 2017

chfast commented May 19, 2017

winsvega commented May 19, 2017 •

edited

Loading

EIP214: STATICCALL #4057

EIP214: STATICCALL #4057

Conversation

yann300 commented Apr 24, 2017 • edited by chfast Loading

codecov-io commented Apr 24, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winsvega commented May 19, 2017

chfast commented May 19, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirapira May 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented May 19, 2017

chfast commented May 19, 2017

winsvega commented May 19, 2017 • edited Loading

yann300 commented Apr 24, 2017 •

edited by chfast

Loading

codecov-io commented Apr 24, 2017 •

edited

Loading

pirapira May 19, 2017 •

edited

Loading

winsvega commented May 19, 2017 •

edited

Loading