EIP-210 BLOCKHASH changes #5505

arkpar · 2017-04-25T17:00:10Z

NikVolf · 2017-05-23T14:11:02Z

ethcore/src/state/mod.rs

@@ -310,6 +311,11 @@ impl<B: Backend> State<B> {
 		Ok(state)
 	}

+	/// Get a VM factory that can execute on this state.
+	pub fn vm_factory(&self) -> EvmFactory {


what was the motivation to move vm factory in the state?
how the decision of choosing the vm executor depends on state?

I did not move it in the state, it is there already. The state is currently constructed with all the Factories that are going to operate on it. I would agree that the state should not be concerned about factories, but refactoring this is out of the scope of this PR.

Yep, it's mostly there because of "State::apply" which should really be a fn apply(&mut State, VMFactory) in some other module.

rphmeier · 2017-05-23T19:07:03Z

ethcore/src/block.rs

@@ -278,10 +282,55 @@ impl<'x> OpenBlock<'x> {
 		let gas_floor_target = cmp::max(gas_range_target.0, engine.params().min_gas_limit);
 		let gas_ceil_target = cmp::max(gas_range_target.1, gas_floor_target);
 		engine.populate_from_parent(&mut r.block.header, parent, gas_floor_target, gas_ceil_target);
+		Self::push_last_hash(&mut r.block, last_hashes, engine, &parent.hash())?;


shouldn't this be something to be done in engine.populate_from_parent?

a utility for engines to call contract functions before and after the transactions within a block as a part of the state transitions seems like it would be useful generally

populate_from_parent is supposed to populate just the header. It is a good idea to move it to the engine though.

rphmeier · 2017-05-23T19:11:38Z

ethcore/src/block.rs

+			let mut ex = Executive::new(&mut state, &env_info, engine);
+			let mut substate = Substate::new();
+			let mut output = [];
+			if let Err(e) = ex.call(params, &mut substate, BytesRef::Fixed(&mut output), &mut NoopTracer, &mut NoopVMTracer) {


applying the substate, collecting garbage, etc?

As far as I understand the EIP this is more of a call with preserved state changes. Finalization is not required.

rphmeier · 2017-05-23T19:13:15Z

ethcore/src/evm/evm.rs

-			BuiltIn { .. } => "Built-in failed",
-			Internal(ref msg) => msg,
+			OutOfGas => "Out of gas".into(),
+			BadJumpDestination { destination } => format!("Bad jump destination {:x}", destination),


all of these should be write!(f, "...", fmt_args)

gavofyork · 2017-05-29T13:22:00Z

code looks fine. did anyone cross check this against either the YP or the EIP spec?

gavofyork · 2017-05-29T16:45:52Z

@NikVolf / @rphmeier ^^^ ?

NikVolf · 2017-05-29T18:03:19Z

@gavofyork yep!

apart from the contract bytecode which cannot be reviewed obviously

arkpar · 2017-05-29T18:06:47Z

The bytecode is in the EIP. You can just check that it matches :)

rphmeier · 2017-05-29T18:16:57Z

ethcore/src/engines/mod.rs

@@ -53,6 +53,9 @@ use evm::CreateContractAddress;
 use ethkey::Signature;
 use util::*;

+/// Default EIP-210 contrat code
+pub const DEFAULT_BLOCKHASH_CONTRACT: &'static str = "73fffffffffffffffffffffffffffffffffffffffe33141561007a57600143036020526000356101006020510755600061010060205107141561005057600035610100610100602051050761010001555b6000620100006020510714156100755760003561010062010000602051050761020001555b610161565b436000351215801561008c5780610095565b623567e0600035125b9050156100a757600060605260206060f35b610100600035430312156100ca57610100600035075460805260206080f3610160565b62010000600035430312156100e857600061010060003507146100eb565b60005b1561010d576101006101006000350507610100015460a052602060a0f361015f565b63010000006000354303121561012d576000620100006000350714610130565b60005b1561015357610100620100006000350507610200015460c052602060c0f361015e565b600060e052602060e0f35b5b5b5b5b";


could a comment include a permanent link (to commit diff, not PR diff) of the EIP?

pirapira · 2017-06-12T14:17:00Z

ethcore/src/externalities.rs

-				H256::zero()
-			},
+	fn blockhash(&mut self, number: &U256) -> H256 {
+		if self.env_info.number + 256 >= self.engine.params().eip210_transition {


I think + 256 should appear in the right hand side, not in the left hand side.

pirapira · 2017-06-12T14:24:11Z

ethcore/src/externalities.rs

+				Ok((code, hash)) => (code, hash),
+				Err(_) => return H256::zero(),
+			};
+


According to this comment ethereum/EIPs#210 (comment) it was decided that BLOCKHASH instruction should return zero for blocks more-than-256 blocks old. So here should be a check on number.

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

arkpar mentioned this pull request Apr 26, 2017

Byzantium release #4833

Closed

12 tasks

arkpar force-pushed the eip210 branch from 9282c19 to b681d1f Compare April 27, 2017 10:45

arkpar mentioned this pull request May 22, 2017

EIP-211 RETURNDATACOPY and RETURNDATASIZE #5678

Merged

arkpar force-pushed the eip210 branch from b681d1f to 5ddf85e Compare May 23, 2017 13:58

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

NikVolf reviewed May 23, 2017

View reviewed changes

NikVolf added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 23, 2017

arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels May 23, 2017

NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 23, 2017

rphmeier reviewed May 23, 2017

View reviewed changes

arkpar added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A8-looksgood 🦄 Pull request is reviewed well. labels May 23, 2017

arkpar force-pushed the eip210 branch 2 times, most recently from 06b852b to d0b0310 Compare May 24, 2017 13:03

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

arkpar force-pushed the eip210 branch from d0b0310 to d0312ac Compare May 25, 2017 11:37

EIP-210

9aee9c5

arkpar force-pushed the eip210 branch from d0312ac to 9aee9c5 Compare May 26, 2017 09:15

NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 29, 2017

rphmeier reviewed May 29, 2017

View reviewed changes

comment

a881711

gavofyork merged commit e6a31e7 into master May 30, 2017

gavofyork deleted the eip210 branch May 30, 2017 09:52

pirapira reviewed Jun 12, 2017

View reviewed changes

This was referenced Jul 27, 2018

Constantinople Release #8427

Closed

Constantinople Progress Tracker ethereum/pm#53

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP-210 BLOCKHASH changes #5505

EIP-210 BLOCKHASH changes #5505

arkpar commented Apr 25, 2017 •

edited

Loading

NikVolf May 23, 2017

arkpar May 23, 2017

rphmeier May 23, 2017

rphmeier May 23, 2017

rphmeier May 23, 2017

arkpar May 23, 2017

rphmeier May 23, 2017

arkpar May 23, 2017

rphmeier May 23, 2017

gavofyork commented May 29, 2017

gavofyork commented May 29, 2017

NikVolf commented May 29, 2017

arkpar commented May 29, 2017

rphmeier May 29, 2017

arkpar May 29, 2017

pirapira Jun 12, 2017

pirapira Jun 12, 2017

EIP-210 BLOCKHASH changes #5505

EIP-210 BLOCKHASH changes #5505

Conversation

arkpar commented Apr 25, 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

gavofyork commented May 29, 2017

gavofyork commented May 29, 2017

NikVolf commented May 29, 2017

arkpar commented May 29, 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

arkpar commented Apr 25, 2017 •

edited

Loading