EIP-116 (214), #4833 #4851

debris · 2017-03-10T09:56:15Z

Everything should be working, but there are no tests. Opened a pr to test all existing tests && to get second pair of eyes on this code.

side-notes, executive / externalities types need to be decoupled from ipc types. I got the impression that some global imports are misused and it's quite annoying

debris · 2017-03-10T10:24:56Z

ethcore/src/evm/interpreter/mod.rs

+						instruction: instruction
+					});
+				}
+


after looking at Arkadiy eip86, I guess this should be under verify_instruction

tomusdrw

Couple of issues noted.

Also one thing bothers me:
I think currently STATICCALL will kill empty accounts when it touches them - that is arguably a state alteration. Since we don't allow empty accounts to be created any more it shouldn't cause a problem if there are no empty accounts already existing on chain, but nevertheless I think we should either avoid killing empty accounts in such case or clarify that in the specification.

tomusdrw · 2017-03-10T10:15:29Z

ethcore/src/client/test_client.rs

@@ -352,7 +352,7 @@ pub fn get_temp_state_db() -> GuardedTempResult<StateDB> {

 impl MiningBlockChainClient for TestBlockChainClient {
 	fn latest_schedule(&self) -> Schedule {
-		Schedule::new_post_eip150(24576, true, true, true)


IMHO this is reaching a point where a struct would look much better than the 4 boolean flags.

Schedule::new_post_eip150(PostEip150 { max_code_size: 24576, fix_exp: true, no_empty: true, kill_empty: true, static_call: true, });

I will change that after #4697 is merged, cause includes the same changes

#4757 also changes this. https://github.com/ethcore/parity/pull/4757/files#diff-ef2687136b4e032043a492e9e53dcb7cR177

tomusdrw · 2017-03-10T10:18:37Z

ethcore/src/evm/ext.rs

@@ -94,15 +94,15 @@ pub trait Ext {
 	fn extcodesize(&self, address: &Address) -> trie::Result<usize>;


Shouldn't all functions here return evm::Result instead of trie::Result?

I agree, it visually looks better 👍

Really it should be an associated type Error. Externalities are lower level than the machine itself, so it doesn't make that much sense to return an evm::Result there.

tomusdrw · 2017-03-10T10:29:06Z

ethcore/src/executive.rs

@@ -253,13 +259,17 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
 		// backup used in case of running out of gas
 		self.state.checkpoint();

+		trace!("Executive::call(params={:?}) self.env_info={:?}", params, self.info);
+		if (params.call_type == CallType::StaticCall || self.static_flag) && params.value.value() > 0.into() {
+			return Err(evm::Error::MutableCallInStaticContext);


Two issues:

Shouldn't such calls be traced anyway?

Seems that state.checkpoint is being made, but since we are not enacting_result it will not be reverted, is that correct?

tomusdrw · 2017-03-10T10:31:24Z

ethcore/src/executive.rs

@@ -359,6 +369,10 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
 		// backup used in case of running out of gas
 		self.state.checkpoint();

+		if params.call_type == CallType::StaticCall || self.static_flag {
+			return Err(evm::Error::MutableCallInStaticContext);


Same issues with tracing and state.checkpoint()

tomusdrw · 2017-03-10T10:34:57Z

ethcore/src/evm/interpreter/mod.rs

@@ -292,14 +292,21 @@ impl<Cost: CostType> Interpreter<Cost> {
 					}
 				};
 			},
-			instructions::CALL | instructions::CALLCODE | instructions::DELEGATECALL => {
+			instructions::CALL | instructions::CALLCODE | instructions::DELEGATECALL | instructions::STATICCALL => {


It seems that gas calculation for staticcall is not implemented (I suppose it should be the same as for CALL)

thanks for pointing that out!

imo, it actually should be the same as DELEGATECALL, since we are not creating new accounts, nor transfering value

applied requested changes

gavofyork · 2017-03-30T18:21:20Z

ethcore/res/ethereum/eip150_test.json

@@ -23,6 +23,7 @@
 		"maximumExtraDataSize": "0x20",
 		"minGasLimit": "0x1388",
 		"networkID" : "0x1",
+		"eip116Transition": "0x7fffffffffffff",


stick to 0x7fffffffffffffff for consistency

gavofyork · 2017-03-30T18:21:35Z

ethcore/res/ethereum/eip161_test.json

@@ -23,6 +23,7 @@
 		"maximumExtraDataSize": "0x20",
 		"minGasLimit": "0x1388",
 		"networkID" : "0x1",
+		"eip116Transition": "0x7fffffffffffff",


gavofyork · 2017-03-30T18:23:06Z

ethcore/src/evm/interpreter/mod.rs

 				stack.pop_back();
 				let call_gas = provided.expect("`provided` comes through Self::exec from `Gasometer::get_gas_cost_mem`; `gas_gas_mem_cost` guarantees `Some` when instruction is `CALL`/`CALLCODE`/`DELEGATECALL`/`CREATE`; this is one of `CALL`/`CALLCODE`/`DELEGATECALL`; qed");
 				let code_address = stack.pop_back();
 				let code_address = u256_to_address(&code_address);

-				let value = if instruction == instructions::DELEGATECALL {
+				let value = if instruction == instructions::DELEGATECALL || instruction ==  instructions::STATICCALL {


double after ==

gavofyork · 2017-03-30T18:41:57Z

ethcore/src/executive.rs

@@ -250,6 +256,13 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
 		tracer: &mut T,
 		vm_tracer: &mut V
 	) -> evm::Result<U256> where T: Tracer, V: VMTracer {
+		trace!("Executive::call(params={:?}) self.env_info={:?}", params, self.info);
+		if (params.call_type == CallType::StaticCall || self.static_flag) && params.value.value() > 0.into() {


params are for this call, no? in which case, if this call type is StaticCall (or DelegateCall, for that matter) then it should be fine even if it is a static context (static_flag == true)

gavofyork · 2017-03-30T18:44:11Z

@tomusdrw we'll have to see how the yellow paper prefers it. i think it'll end up being easier to consider STATIC_CALL as "touching" accounts, even if it's not allowing direct state changes.

debris · 2017-03-30T19:28:40Z

@gavofyork touching accounts has been already clarified -> ethereum/EIPs#214 (comment)

my Yellow Paper PR would also kill dead accounts that are touched during a STATICCALL ethereum/yellowpaper#270

rphmeier · 2017-06-16T12:21:10Z

ethcore/src/evm/schedule.rs

+ 	pub fn apply_params(&mut self, block_number: u64, params: &CommonParams) {
+		self.have_create2 = block_number >= params.eip86_transition;
+		self.have_revert = block_number >= params.eip140_transition;
+		self.have_static_call = block_number >= params.eip214_transition;


nice, this is better than have_metropolis_instructions

rphmeier · 2017-06-16T12:24:10Z

ethcore/src/externalities.rs

-		self.state.set_storage(&self.origin_info.address, key, value)
+	fn set_storage(&mut self, key: H256, value: H256) -> evm::Result<()> {
+		if self.static_flag {
+			Err(evm::Error::MutableCallInStaticContext)


it would be interesting if the static mode just had immutable references so mutable calls could literally not be made -- but this works too.

rphmeier · 2017-06-16T12:28:20Z

LGTM, but tests would be nice.

pirapira · 2017-06-21T14:51:33Z

ethcore/src/evm/schedule.rs

+		self.have_revert = block_number >= params.eip140_transition;
+		self.have_static_call = block_number >= params.eip214_transition;
+		if block_number >= params.eip210_transition {
+			self.blockhash_gas = 350;


I think the new gas cost is 800. https://github.com/ethereum/EIPs/pull/210/files#diff-e02a92c2fb96c1a1bfb05e4c6e2ef5daR42

thanks, added to #5855

debris added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Mar 10, 2017

debris requested a review from tomusdrw March 10, 2017 09:56

debris commented Mar 10, 2017

View reviewed changes

tomusdrw previously requested changes Mar 10, 2017

View reviewed changes

debris mentioned this pull request Mar 10, 2017

New opcode: STATICCALL ethereum/EIPs#214

Merged

debris changed the title ~~EIP-214, #4833~~ EIP-116 (214), #4833 Mar 10, 2017

debris added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Mar 10, 2017

arkpar mentioned this pull request Mar 10, 2017

Byzantium release #4833

Closed

12 tasks

gavofyork reviewed Mar 30, 2017

View reviewed changes

gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 30, 2017

gavofyork reviewed Mar 30, 2017

View reviewed changes

gavofyork added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Mar 30, 2017

gavofyork closed this Mar 30, 2017

gavofyork reopened this Mar 30, 2017

arkpar force-pushed the eip214 branch 2 times, most recently from edba366 to debc1e2 Compare May 31, 2017 11:55

arkpar added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels May 31, 2017

eip214, #4833

b85e75b

arkpar force-pushed the eip214 branch from debc1e2 to b85e75b Compare June 14, 2017 17:56

arkpar added the A0-pleasereview 🤓 Pull request needs code review. label Jun 14, 2017

arkpar removed the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Jun 14, 2017

rphmeier reviewed Jun 16, 2017

View reviewed changes

rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 16, 2017

gavofyork merged commit 575c51f into master Jun 19, 2017

gavofyork deleted the eip214 branch June 19, 2017 09:41

pirapira reviewed Jun 21, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP-116 (214), #4833 #4851

EIP-116 (214), #4833 #4851

debris commented Mar 10, 2017 •

edited

Loading

debris Mar 10, 2017

tomusdrw left a comment •

edited

Loading

tomusdrw Mar 10, 2017

debris Mar 10, 2017 •

edited

Loading

arkpar Mar 10, 2017

tomusdrw Mar 10, 2017

debris Mar 10, 2017 •

edited

Loading

rphmeier Mar 10, 2017

tomusdrw Mar 10, 2017

tomusdrw Mar 10, 2017

tomusdrw Mar 10, 2017

debris Mar 10, 2017

debris Mar 10, 2017

gavofyork Mar 30, 2017

gavofyork Mar 30, 2017

gavofyork Mar 30, 2017

gavofyork Mar 30, 2017

gavofyork commented Mar 30, 2017

debris commented Mar 30, 2017

rphmeier Jun 16, 2017

rphmeier Jun 16, 2017

rphmeier commented Jun 16, 2017

pirapira Jun 21, 2017

arkpar Jun 21, 2017

		@@ -94,15 +94,15 @@ pub trait Ext {
		fn extcodesize(&self, address: &Address) -> trie::Result<usize>;

EIP-116 (214), #4833 #4851

EIP-116 (214), #4833 #4851

Conversation

debris commented Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

tomusdrw left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debris Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debris Mar 10, 2017 • edited Loading

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

gavofyork commented Mar 30, 2017

debris commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debris commented Mar 10, 2017 •

edited

Loading

tomusdrw left a comment •

edited

Loading

debris Mar 10, 2017 •

edited

Loading

debris Mar 10, 2017 •

edited

Loading