EIP211: RETURNDATA #4062

chfast · 2017-04-25T13:33:49Z

codecov-io · 2017-05-05T10:36:34Z

Codecov Report

Merging #4062 into develop will decrease coverage by 0.06%.
The diff coverage is 53.33%.

@@             Coverage Diff             @@
##           develop    #4062      +/-   ##
===========================================
- Coverage    65.92%   65.85%   -0.07%     
===========================================
  Files          308      308              
  Lines        22905    22913       +8     
===========================================
- Hits         15100    15090      -10     
- Misses        7805     7823      +18

Impacted Files	Coverage Δ
libevmcore/Instruction.cpp	`47.5% <ø> (+9.03%)`	⬆️
libevm/VM.h	`76.92% <ø> (ø)`	⬆️
test/tools/jsontests/vm.h	`100% <ø> (ø)`	⬆️
libethereum/ExtVM.h	`100% <ø> (ø)`	⬆️
libevm/VMOpt.cpp	`87.23% <ø> (ø)`	⬆️
libevm/ExtVMFace.h	`80.3% <ø> (+1.19%)`	⬆️
libevmcore/Instruction.h	`100% <ø> (ø)`	⬆️
libevm/VM.cpp	`91.4% <0%> (-2.91%)`	⬇️
libevmcore/EVMSchedule.h	`100% <100%> (ø)`	⬆️
libethereum/ExtVM.cpp	`95.12% <100%> (ø)`	⬆️
... and 7 more

chfast · 2017-05-05T10:37:45Z

@winsvega can you generate tests for this?

pirapira · 2017-05-09T10:00:40Z

libevm/VM.h

@@ -104,6 +104,9 @@ class VM: public VMFace
 	// space for code
 	bytes m_code;

+	/// Space for return data.


Why three /s? (I was not looking for them but I saw them.)

/// starts a Doxygen comment.

Maybe clarify that this is for the data returned from the subcall.
Because now we have m_output described as "return bytes" and m_returnedData described as "return data", this is confusing if you don't know what's it all about

pirapira · 2017-05-09T10:05:31Z

libevmcore/Instruction.cpp

@@ -204,6 +54,8 @@ static const std::map<Instruction,  InstructionInfo> c_instructionInfo =
 	{ Instruction::ADDMOD,       { "ADDMOD",         0,     3,    1,  false,       Tier::Mid } },
 	{ Instruction::MULMOD,       { "MULMOD",         0,     3,    1,  false,       Tier::Mid } },
 	{ Instruction::SIGNEXTEND,   { "SIGNEXTEND",     0,     2,    1,  false,       Tier::Low } },
+	{ Instruction::RETURNDATASIZE,{"RETURNDATASIZE", 0,     0,    1,  false,       Tier::Low } },
+	{ Instruction::RETURNDATACOPY,{"RETURNDATACOPY", 0,     3,    0,  false,       Tier::Low } },


Why do the Tiers look different from those of CALLDATASIZE and CALLDATACOPY?

I think I was so focused on setting the number of args right I forgot about the costs.

gumb0 · 2017-05-10T09:05:22Z

libevmcore/Instruction.cpp

@@ -204,6 +54,8 @@ static const std::map<Instruction,  InstructionInfo> c_instructionInfo =
 	{ Instruction::ADDMOD,       { "ADDMOD",         0,     3,    1,  false,       Tier::Mid } },
 	{ Instruction::MULMOD,       { "MULMOD",         0,     3,    1,  false,       Tier::Mid } },
 	{ Instruction::SIGNEXTEND,   { "SIGNEXTEND",     0,     2,    1,  false,       Tier::Low } },
+	{ Instruction::RETURNDATASIZE,{"RETURNDATASIZE", 0,     0,    1,  false,       Tier::VeryLow } },


Are the prices still not right? Tier::VeryLow is 3, but RETURNDATASIZE should be 2, which is Tier::Base, like CALLDATASIZE
And RETURNDATACOPY should be Tier::VeryLow ?

Of course. I'm really sorry, I must have been distracted. BTW, "side effect" is not used, but this should be addressed in #4072.

gumb0 · 2017-05-10T09:05:42Z

libevmcore/Instruction.cpp

@@ -204,6 +54,8 @@ static const std::map<Instruction,  InstructionInfo> c_instructionInfo =
 	{ Instruction::ADDMOD,       { "ADDMOD",         0,     3,    1,  false,       Tier::Mid } },
 	{ Instruction::MULMOD,       { "MULMOD",         0,     3,    1,  false,       Tier::Mid } },
 	{ Instruction::SIGNEXTEND,   { "SIGNEXTEND",     0,     2,    1,  false,       Tier::Low } },
+	{ Instruction::RETURNDATASIZE,{"RETURNDATASIZE", 0,     0,    1,  false,       Tier::VeryLow } },
+	{ Instruction::RETURNDATACOPY,{"RETURNDATACOPY", 0,     3,    0,  false,       Tier::Low } },


I guess it should have true for side-effects like CALLDATACOPY

gumb0 · 2017-05-10T13:02:30Z

libevm/VMCalls.cpp

 	bytesRef output;
 	if (caseCallSetup(callParams.get(), output))
 	{
 		std::pair<bool, owning_bytes_ref> callResult = m_ext->call(*callParams);
 		callResult.second.copyTo(output);
+		m_returnData.assign(callResult.second.begin(), callResult.second.end());


Could we define m_returnData as owning_bytes_ref and then move here instead of copying?

(if not, using helpers like owning_bytes_ref::toBytes probably would make this expression simpler)

I'm not a big fun of helpers like toBytes, usually C++ std lib should be enough. But the question is: should we keep the full memory buffer (owning_bytes_ref) or copy only the return buffer.

Anyway, refactored.

pirapira · 2017-05-10T15:58:37Z

libevm/VMCalls.cpp

+		bool success = false;
+		owning_bytes_ref outputRef;
+		std::tie(success, outputRef) = m_ext->call(*callParams);
+		outputRef.copyTo(output);


Am I mistaken that output is only written to but not read from? (This didn't change in this commit, but I started wondering now.)

Ah, it's the caller's memory!

Magic. The output is actually a memory reference initialized in caseCallSetup(). This is exactly CALL output buffer, might be of length 0 depending on CALL arguments. Here we copy the data there according to what the CALL requested.

chfast · 2017-05-11T09:36:41Z

Big thanks for reviewing this.
Should we wait for tests and finalization of the spec (opcode numbers are to be changes, and decision about copying out of buffer)?

gumb0 · 2017-05-11T09:39:37Z

I would better merge now, then fix what changed later

axic · 2017-05-11T09:41:02Z

libevm/VM.cpp

+			ON_OP();
+			updateIOGas();
+
+			copyDataToMemory(&m_returnData, m_SP);


Not strictly connected to this PR, but probably it would be better to have the offset + size as explicit parameters in copyDataToMemory.

pirapira · 2017-05-11T09:44:17Z

libevm/VMCalls.cpp

@@ -120,6 +120,9 @@ void VM::caseCreate()
 	uint64_t initOff = (uint64_t)m_SP[1];
 	uint64_t initSize = (uint64_t)m_SP[2];

+	// Clear the return data buffer. This will not free the memory.
+	m_returnData.clear();
+


When the contract initialization executes REVERT, m_returnData has to be set.

See the last sentence on this line: https://github.com/ethereum/EIPs/pull/211/files#diff-fd8c393953fe66162d8bb518d87bfb57R32

(That's something I learned while doing YP.)

Implemented.

gumb0

Need to update return buffer after CREATE

Introduce RETURNDATASIZE and RETURNDATACOPY opcodes, they do nothing useful at the moment.

pirapira · 2017-05-16T11:57:49Z

libevm/VMCalls.cpp

+		owning_bytes_ref revertOutput;
+		std::tie(addr, revertOutput) = m_ext->create(endowment, gas, bytesConstRef(m_mem.data() + initOff, initSize), m_onOp);
+		m_SPP[0] = (u160)addr;
+		m_returnData = revertOutput.toBytes();


When the creation succeeds, does this set the deployed code in the buffer?

No. Executive handles this. For CREATE, the VM output is only taken in case of REVERT. https://github.com/ethereum/cpp-ethereum/blob/develop/libethereum/Executive.cpp#L411.

Ah, I see. I've opened the same file in the editor but I was not quite seeing it.

That's my role to explain the changes.

gumb0 · 2017-05-17T10:09:54Z

libevm/JitVM.cpp

-		auto addr = env.create(value, gas, input, {});
+		h160 addr;
+		owning_bytes_ref revertOutput;
+		std::tie(addr, revertOutput) = env.create(value, gas, input, {});


revertOutput is not used after create, is this correct?

At the moment it is. EVM-C does not support "revert output" yet so there is not option to pass this back to EVMJIT. But EVM-C v4 changes are in progress.

I'd use std::ignore while this is the case.

pirapira · 2017-05-17T12:05:21Z

libevmcore/Instruction.h

@@ -47,6 +47,9 @@ enum class Instruction: uint8_t
 	EXP,                ///< exponential operation
 	SIGNEXTEND,         ///< extend length of signed integer

+	RETURNDATASIZE = 0x0d,  ///< size of data returned from previous call
+	RETURNDATACOPY = 0x0e,  ///< copy data returned from previous call to memory


The opcodes seem to have changed: ethereum/solidity#2275 (comment)

Not in the EIP PR.

gumb0 · 2017-05-17T12:42:47Z

libevm/VMCalls.cpp

 	if (m_ext->balance(m_ext->myAddress) >= endowment && m_ext->depth < 1024)
 	{
 		*m_io_gas_p = m_io_gas;
 		u256 createGas = *m_io_gas_p;
 		if (!m_schedule->staticCallDepthLimit())
 			createGas -= createGas / 64;
 		u256 gas = createGas;
-		m_SPP[0] = (u160)m_ext->create(endowment, gas, bytesConstRef(m_mem.data() + initOff, initSize), m_onOp);
+		h160 addr;
+		owning_bytes_ref revertOutput;


Maybe name it just output? Because revertOutput is kind of assuming how create works internally and that only REVERT can return data. Which is the case now, but could change in the furure

gumb0 · 2017-05-17T12:44:15Z

Looks good now, just a couple of minor comments.

gumb0 · 2017-05-17T12:47:25Z

@yann300 take a look at changes in ExtVM::create here, you'll probably need to rebase on that if we merge this first

chfast · 2017-05-17T12:53:32Z

Ok, I will try to apply suggestions as soon as possible.

chfast · 2017-05-18T09:34:22Z

Ready to land?

pirapira · 2017-05-18T09:50:02Z

@chriseth is changing the EIP.

chfast · 2017-05-18T16:59:14Z

@gumb0 @pirapira @chriseth I'm for merging this as is, and implement the final decision about throwing on out-of-range reads when the decision is made. If you agree, please press the Merge button.

chfast added this to the Metropolis milestone Apr 25, 2017

chfast added the in progress label Apr 25, 2017

yann300 self-assigned this Apr 27, 2017

chfast self-assigned this May 5, 2017

chfast assigned winsvega and unassigned chfast and yann300 May 5, 2017

chfast requested review from gcolvin, chriseth and gumb0 May 8, 2017 21:49

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

pirapira reviewed May 9, 2017

View reviewed changes

gumb0 reviewed May 10, 2017

View reviewed changes

chfast force-pushed the returndata branch from 2ae2f47 to 9ea6c44 Compare May 10, 2017 09:24

gumb0 reviewed May 10, 2017

View reviewed changes

pirapira reviewed May 10, 2017

View reviewed changes

gumb0 approved these changes May 11, 2017

View reviewed changes

axic reviewed May 11, 2017

View reviewed changes

pirapira reviewed May 11, 2017

View reviewed changes

gumb0 suggested changes May 11, 2017

View reviewed changes

pirapira mentioned this pull request May 12, 2017

[experiment; don't merge] adding a test for RETURNDATACOPY #4076

Closed

seusher mentioned this pull request May 13, 2017

Cleanup Instruction table #4072

Closed

VM: Add return data buffer

3fae652

chfast added 7 commits May 16, 2017 10:26

Extend EVM Schedule with "return data" flag

ec797dd

EVM: remove disassembly feature

ce59f0c

Introduce RETURNDATA* opcodes

f07808e

Introduce RETURNDATASIZE and RETURNDATACOPY opcodes, they do nothing useful at the moment.

Implement RETURNDATASIZE

798322d

Implement RETURNDATACOPY

d30ddfb

Fix RETURNDATACOPY and RETURNDATASIZE prices

3ee15d0

Refactor code handling return buffer

05e438e

chfast force-pushed the returndata branch from ca5a36c to 890a110 Compare May 16, 2017 10:42

pirapira reviewed May 16, 2017

View reviewed changes

gumb0 reviewed May 17, 2017

View reviewed changes

pirapira reviewed May 17, 2017

View reviewed changes

gumb0 reviewed May 17, 2017

View reviewed changes

gumb0 approved these changes May 17, 2017

View reviewed changes

Add support for REVERT in CREATE

861a9ea

chfast force-pushed the returndata branch from 890a110 to 861a9ea Compare May 17, 2017 12:59

Change RETURNDATA opcode numbers

e18d839

gumb0 merged commit acfbe83 into develop May 18, 2017

gumb0 removed the needs review label May 18, 2017

pirapira deleted the returndata branch May 24, 2017 13:04

pirapira mentioned this pull request Jul 28, 2017

EIP 5/8 Add RETURNDATACOPY and RETURNDATASIZE instructions #3611

Closed

This pull request was closed.

EIP211: RETURNDATA #4062

EIP211: RETURNDATA #4062

Conversation

chfast commented Apr 25, 2017 • edited Loading

codecov-io commented May 5, 2017 • edited Loading

Codecov Report

chfast 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

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

chfast commented May 11, 2017

gumb0 commented May 11, 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

gumb0 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

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

gumb0 commented May 17, 2017

chfast commented May 17, 2017

chfast commented May 18, 2017

pirapira commented May 18, 2017

chfast commented May 18, 2017

chfast commented Apr 25, 2017 •

edited

Loading

codecov-io commented May 5, 2017 •

edited

Loading