Merge pull request #115 from gavofyork/executive_revert_fix

Executive revert fix

src/account.rs

@@ -334,4 +334,4 @@ mod tests {
 		assert_eq!(a.rlp().to_hex(), "f8448045a056e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421a0c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470");
 	}
 
-}
+}

src/env_info.rs

@@ -47,7 +47,7 @@ impl FromJson for EnvInfo {
 			difficulty: xjson!(&json["currentDifficulty"]),
 			gas_limit: xjson!(&json["currentGasLimit"]),
 			timestamp: xjson!(&json["currentTimestamp"]),
-			last_hashes: (1..257).map(|i| format!("{}", current_number - i).as_bytes().sha3()).collect(),
+			last_hashes: (1..cmp::min(current_number + 1, 257)).map(|i| format!("{}", current_number - i).as_bytes().sha3()).collect(),
 			gas_used: x!(0),
 		}
 	}

src/evm/ext.rs

@@ -21,15 +21,14 @@ pub trait Ext {
 
 	/// Creates new contract.
 	/// 
-	/// If contract creation is successfull, return gas_left and contract address,
-	/// If depth is too big or transfer value exceeds balance, return None
-	/// Otherwise return appropriate `Error`.
-	fn create(&mut self, gas: &U256, value: &U256, code: &[u8]) -> Result<(U256, Option<Address>), Error>;
+	/// Returns gas_left and contract address if contract creation was succesfull.
+	fn create(&mut self, gas: &U256, value: &U256, code: &[u8]) -> (U256, Option<Address>);
 
 	/// Message call.
 	/// 
-	/// If call is successfull, returns gas left.
-	/// otherwise `Error`.
+	/// Returns Err, if we run out of gas.
+	/// Otherwise returns call_result which contains gas left 
+	/// and true if subcall was successfull.
 	fn call(&mut self, 
 			gas: &U256, 
 			call_gas: &U256, 
@@ -37,7 +36,7 @@ pub trait Ext {
 			value: &U256, 
 			data: &[u8], 
 			code_address: &Address, 
-			output: &mut [u8]) -> Result<U256, Error>;
+			output: &mut [u8]) -> Result<(U256, bool), Error>;
 
 	/// Returns code at given address
 	fn extcode(&self, address: &Address) -> Vec<u8>;

src/evm/jit.rs

@@ -209,23 +209,12 @@ impl<'a> evmjit::Ext for ExtAdapter<'a> {
 			  init_size: u64,
 			  address: *mut evmjit::H256) {
 		unsafe {
-			match self.ext.create(&U256::from(*io_gas), &U256::from_jit(&*endowment), slice::from_raw_parts(init_beg, init_size as usize)) {
-				Ok((gas_left, opt)) => {
-					*io_gas = gas_left.low_u64();
-					*address = match opt {
-						Some(addr) => addr.into_jit(),
-						_ => Address::new().into_jit()
-					};
-				},
-				Err(err @ evm::Error::OutOfGas) => {
-					*self.err = Some(err);
-					// hack to propagate `OutOfGas` to evmjit and stop
-					// the execution immediately.
-					// Works, cause evmjit uses i64, not u64
-					*io_gas = -1i64 as u64;
-				},
-				Err(err) => *self.err = Some(err)
-			}
+			let (gas_left, opt_addr) = self.ext.create(&U256::from(*io_gas), &U256::from_jit(&*endowment), slice::from_raw_parts(init_beg, init_size as usize));
+			*io_gas = gas_left.low_u64();
+			*address = match opt_addr {
+				Some(addr) => addr.into_jit(),
+				_ => Address::new().into_jit()
+			};
 		}
 	}
 
@@ -247,22 +236,21 @@ impl<'a> evmjit::Ext for ExtAdapter<'a> {
 									slice::from_raw_parts(in_beg, in_size as usize),
 									&Address::from_jit(&*code_address),
 									slice::from_raw_parts_mut(out_beg, out_size as usize));
-
 			match res {
-				Ok(gas_left) => {
+				Ok((gas_left, ok)) => {
 					*io_gas = gas_left.low_u64();
-					true
-				},
-				Err(err @ evm::Error::OutOfGas) => {
-					*self.err = Some(err);
-					// hack to propagate `OutOfGas` to evmjit and stop
-					// the execution immediately.
-					// Works, cause evmjit uses i64, not u64
+					ok
+				}
+				Err(evm::Error::OutOfGas) => {
+					// hack to propagate out_of_gas to evmjit.
+					// must be negative
 					*io_gas = -1i64 as u64;
 					false
 				},
 				Err(err) => {
+					// internal error.
 					*self.err = Some(err);
+					*io_gas = -1i64 as u64;
 					false
 				}
 			}

src/evm/mod.rs

@@ -11,6 +11,6 @@ mod jit;
 mod tests;
 
 pub use self::evm::{Evm, Error, Result};
-pub use self::ext::Ext;
+pub use self::ext::{Ext};
 pub use self::factory::Factory;
 pub use self::schedule::Schedule;

src/evm/tests.rs

@@ -42,7 +42,7 @@ impl Ext for FakeExt {
 		self.blockhashes.get(number).unwrap_or(&H256::new()).clone()
 	}
 
-	fn create(&mut self, _gas: &U256, _value: &U256, _code: &[u8]) -> result::Result<(U256, Option<Address>), evm::Error> {
+	fn create(&mut self, _gas: &U256, _value: &U256, _code: &[u8]) -> (U256, Option<Address>) {
 		unimplemented!();
 	}
 
@@ -53,7 +53,7 @@ impl Ext for FakeExt {
 			_value: &U256, 
 			_data: &[u8], 
 			_code_address: &Address, 
-			_output: &mut [u8]) -> result::Result<U256, evm::Error> {
+			_output: &mut [u8]) -> result::Result<(U256, bool), evm::Error> {
 		unimplemented!();
 	}
 

src/executive.rs

@@ -21,6 +21,8 @@ pub struct Substate {
 	logs: Vec<LogEntry>,
 	/// Refund counter of SSTORE nonzero->zero.
 	refunds_count: U256,
+	/// True if transaction, or one of its subcalls runs out of gas.
+	out_of_gas: bool,
 	/// Created contracts.
 	contracts_created: Vec<Address>
 }
@@ -32,9 +34,12 @@ impl Substate {
 			suicides: HashSet::new(),
 			logs: vec![],
 			refunds_count: U256::zero(),
+			out_of_gas: false,
 			contracts_created: vec![]
 		}
 	}
+
+	pub fn out_of_gas(&self) -> bool { self.out_of_gas }
 }
 
 /// Transaction execution receipt.
@@ -135,7 +140,6 @@ impl<'a> Executive<'a> {
 		self.state.sub_balance(&sender, &U256::from(gas_cost));
 
 		let mut substate = Substate::new();
-		let backup = self.state.clone();
 
 		let schedule = self.engine.schedule(self.info);
 		let init_gas = t.gas - U256::from(t.gas_required(&schedule));
@@ -172,7 +176,7 @@ impl<'a> Executive<'a> {
 		};
 
 		// finalize here!
-		Ok(try!(self.finalize(t, substate, backup, res)))
+		Ok(try!(self.finalize(t, substate, res)))
 	}
 
 	/// Calls contract function with given contract params.
@@ -180,6 +184,9 @@ impl<'a> Executive<'a> {
 	/// Modifies the substate and the output.
 	/// Returns either gas_left or `evm::Error`.
 	pub fn call(&mut self, params: &ActionParams, substate: &mut Substate, mut output: BytesRef) -> evm::Result {
+		// backup used in case of running out of gas
+		let backup = self.state.clone();
+
 		// at first, transfer value to destination
 		self.state.transfer_balance(&params.sender, &params.address, &params.value);
 
@@ -191,13 +198,19 @@ impl<'a> Executive<'a> {
 					self.engine.execute_builtin(&params.address, &params.data, &mut output);
 					Ok(params.gas - cost)
 				},
-				false => Err(evm::Error::OutOfGas)
+				// just drain the whole gas
+				false => Ok(U256::zero())
 			}
 		} else if params.code.len() > 0 {
 			// if destination is a contract, do normal message call
-			let mut ext = Externalities::from_executive(self, params, substate, OutputPolicy::Return(output));
-			let evm = Factory::create();
-			evm.exec(&params, &mut ext)
+			
+			let res = {
+				let mut ext = Externalities::from_executive(self, params, substate, OutputPolicy::Return(output));
+				let evm = Factory::create();
+				evm.exec(&params, &mut ext)
+			};
+			self.revert_if_needed(&res, substate, backup);
+			res
 		} else {
 			// otherwise, nothing
 			Ok(params.gas)
@@ -208,32 +221,28 @@ impl<'a> Executive<'a> {
 	/// NOTE. It does not finalize the transaction (doesn't do refunds, nor suicides).
 	/// Modifies the substate.
 	fn create(&mut self, params: &ActionParams, substate: &mut Substate) -> evm::Result {
+		// backup used in case of running out of gas
+		let backup = self.state.clone();
+
 		// at first create new contract
 		self.state.new_contract(&params.address);
+
 		// then transfer value to it
 		self.state.transfer_balance(&params.sender, &params.address, &params.value);
 
-		let mut ext = Externalities::from_executive(self, params, substate, OutputPolicy::InitContract);
-		let evm = Factory::create();
-		evm.exec(&params, &mut ext)
+		let res = {
+			let mut ext = Externalities::from_executive(self, params, substate, OutputPolicy::InitContract);
+			let evm = Factory::create();
+			evm.exec(&params, &mut ext)
+		};
+		self.revert_if_needed(&res, substate, backup);
+		res
 	}
 
 	/// Finalizes the transaction (does refunds and suicides).
-	fn finalize(&mut self, t: &Transaction, substate: Substate, backup: State, result: evm::Result) -> ExecutionResult {
+	fn finalize(&mut self, t: &Transaction, substate: Substate, result: evm::Result) -> ExecutionResult {
 		match result { 
 			Err(evm::Error::Internal) => Err(ExecutionError::Internal),
-			Err(evm::Error::OutOfGas) => {
-				*self.state = backup;
-				Ok(Executed {
-					gas: t.gas,
-					gas_used: t.gas,
-					refunded: U256::zero(),
-					cumulative_gas_used: self.info.gas_used + t.gas,
-					logs: vec![],
-					out_of_gas: true,
-					contracts_created: vec![]
-				})
-			},
 			Ok(gas_left) => {
 				let schedule = self.engine.schedule(self.info);
 
@@ -265,12 +274,37 @@ impl<'a> Executive<'a> {
 					refunded: refund,
 					cumulative_gas_used: self.info.gas_used + gas_used,
 					logs: substate.logs,
-					out_of_gas: false,
+					out_of_gas: substate.out_of_gas,
 					contracts_created: substate.contracts_created
 				})
+			},
+			_err => {
+				Ok(Executed {
+					gas: t.gas,
+					gas_used: t.gas,
+					refunded: U256::zero(),
+					cumulative_gas_used: self.info.gas_used + t.gas,
+					logs: vec![],
+					out_of_gas: true,
+					contracts_created: vec![]
+				})
 			}
 		}
 	}
+
+	fn revert_if_needed(&mut self, result: &evm::Result, substate: &mut Substate, backup: State) {
+		// TODO: handle other evm::Errors same as OutOfGas once they are implemented
+		match &result {
+			&Err(evm::Error::OutOfGas) => {
+				substate.out_of_gas = true;
+				self.state.revert(backup);
+			},
+			&Err(evm::Error::Internal) => (),
+			&Ok(_) => ()
+			
+		}
+		result
+	}
 }
 
 /// Policy for handling output data on `RETURN` opcode.
@@ -354,10 +388,10 @@ impl<'a> Ext for Externalities<'a> {
 		}
 	}
 
-	fn create(&mut self, gas: &U256, value: &U256, code: &[u8]) -> Result<(U256, Option<Address>), evm::Error> {
+	fn create(&mut self, gas: &U256, value: &U256, code: &[u8]) -> (U256, Option<Address>) {
 		// if balance is insufficient or we are to deep, return
 		if self.state.balance(&self.params.address) < *value || self.depth >= self.schedule.max_depth {
-			return Ok((*gas, None));
+			return (*gas, None);
 		}
 
 		// create new contract address
@@ -377,10 +411,20 @@ impl<'a> Ext for Externalities<'a> {
 
 		let mut ex = Executive::from_parent(self.state, self.info, self.engine, self.depth);
 		ex.state.inc_nonce(&self.params.address);
-		ex.create(&params, self.substate).map(|gas_left| (gas_left, Some(address)))
+		match ex.create(&params, self.substate) {
+			Ok(gas_left) => (gas_left, Some(address)),
+			_ => (U256::zero(), None)
+		}
 	}
 
-	fn call(&mut self, gas: &U256, call_gas: &U256, receive_address: &Address, value: &U256, data: &[u8], code_address: &Address, output: &mut [u8]) -> Result<U256, evm::Error> {
+	fn call(&mut self, 
+			gas: &U256, 
+			call_gas: &U256, 
+			receive_address: &Address, 
+			value: &U256, 
+			data: &[u8], 
+			code_address: &Address, 
+			output: &mut [u8]) -> Result<(U256, bool), evm::Error> {
 		let mut gas_cost = *call_gas;
 		let mut call_gas = *call_gas;
 
@@ -396,14 +440,14 @@ impl<'a> Ext for Externalities<'a> {
 		}
 
 		if gas_cost > *gas {
-			return Err(evm::Error::OutOfGas)
+			return Err(evm::Error::OutOfGas);
 		}
 
 		let gas = *gas - gas_cost;
 
 		// if balance is insufficient or we are to deep, return
 		if self.state.balance(&self.params.address) < *value || self.depth >= self.schedule.max_depth {
-			return Ok(gas + call_gas)
+			return Ok((gas + call_gas, true));
 		}
 
 		let params = ActionParams {
@@ -418,7 +462,10 @@ impl<'a> Ext for Externalities<'a> {
 		};
 
 		let mut ex = Executive::from_parent(self.state, self.info, self.engine, self.depth);
-		ex.call(&params, self.substate, BytesRef::Fixed(output)).map(|gas_left| gas + gas_left)
+		match ex.call(&params, self.substate, BytesRef::Fixed(output)) {
+			Ok(gas_left) => Ok((gas + gas_left, true)), //Some(CallResult::new(gas + gas_left, true)),
+			_ => Ok((gas, false))
+		}
 	}
 
 	fn extcode(&self, address: &Address) -> Vec<u8> {
@@ -832,9 +879,9 @@ mod tests {
 		};
 
 		assert_eq!(executed.gas, U256::from(100_000));
-		assert_eq!(executed.gas_used, U256::from(20_025));
-		assert_eq!(executed.refunded, U256::from(79_975));
-		assert_eq!(executed.cumulative_gas_used, U256::from(20_025));
+		assert_eq!(executed.gas_used, U256::from(41_301));
+		assert_eq!(executed.refunded, U256::from(58_699));
+		assert_eq!(executed.cumulative_gas_used, U256::from(41_301));
 		assert_eq!(executed.logs.len(), 0);
 		assert_eq!(executed.out_of_gas, false);
 		assert_eq!(executed.contracts_created.len(), 0);

src/state.rs

@@ -127,7 +127,7 @@ impl State {
 
 	/// Mutate storage of account `a` so that it is `value` for `key`.
 	pub fn set_storage(&mut self, a: &Address, key: H256, value: H256) {
-		self.require(a, false).set_storage(key, value);
+		self.require(a, false).set_storage(key, value)
 	}
 
 	/// Initialise the code of account `a` so that it is `value` for `key`.
@@ -140,10 +140,15 @@ impl State {
 	/// This will change the state accordingly.
 	pub fn apply(&mut self, env_info: &EnvInfo, engine: &Engine, t: &Transaction) -> ApplyResult {
 		let e = try!(Executive::new(self, env_info, engine).transact(t));
+		//println!("Executed: {:?}", e);
 		self.commit();
 		Ok(Receipt::new(self.root().clone(), e.gas_used, e.logs))
 	}
 
+	pub fn revert(&mut self, backup: State) {
+		self.cache = backup.cache;
+	}
+
 	/// Convert into a JSON representation.
 	pub fn as_json(&self) -> String {
 		unimplemented!();

src/tests/executive.rs

@@ -9,14 +9,14 @@ use ethereum;
 
 struct TestEngine {
 	spec: Spec,
-	stack_limit: usize
+	max_depth: usize
 }
 
 impl TestEngine {
-	fn new(stack_limit: usize) -> TestEngine {
+	fn new(max_depth: usize) -> TestEngine {
 		TestEngine {
 			spec: ethereum::new_frontier_test(),
-			stack_limit: stack_limit 
+			max_depth: max_depth 
 		}
 	}
 }
@@ -26,14 +26,14 @@ impl Engine for TestEngine {
 	fn spec(&self) -> &Spec { &self.spec }
 	fn schedule(&self, _env_info: &EnvInfo) -> Schedule { 
 		let mut schedule = Schedule::new_frontier();
-		schedule.stack_limit = self.stack_limit; 
+		schedule.max_depth = self.max_depth; 
 		schedule
 	}
 }
 
 struct CallCreate {
 	data: Bytes,
-	destination: Address,
+	destination: Option<Address>,
 	_gas_limit: U256,
 	value: U256
 }
@@ -71,33 +71,33 @@ impl<'a> Ext for TestExt<'a> {
 		self.ext.blockhash(number)
 	}
 
-	fn create(&mut self, gas: &U256, value: &U256, code: &[u8]) -> Result<(U256, Option<Address>), evm::Error> {
+	fn create(&mut self, gas: &U256, value: &U256, code: &[u8]) -> (U256, Option<Address>) {
 		// in call and create we need to check if we exited with insufficient balance or max limit reached.
 		// in case of reaching max depth, we should store callcreates. Otherwise, ignore.
 		let res = self.ext.create(gas, value, code);
 		let ext = &self.ext;
 		match res {
 			// just record call create
-			Ok((gas_left, Some(address))) => {
+			(gas_left, Some(address)) => {
 				self.callcreates.push(CallCreate {
 					data: code.to_vec(),
-					destination: address.clone(),
+					destination: Some(address.clone()),
 					_gas_limit: *gas,
 					value: *value
 				});
-				Ok((gas_left, Some(address)))
+				(gas_left, Some(address))
 			},
-			// creation failed only due to reaching stack_limit
-			Ok((gas_left, None)) if ext.state.balance(&ext.params.address) >= *value => {
-				let address = contract_address(&ext.params.address, &ext.state.nonce(&ext.params.address));
+			// creation failed only due to reaching max_depth
+			(gas_left, None) if ext.state.balance(&ext.params.address) >= *value => {
 				self.callcreates.push(CallCreate {
 					data: code.to_vec(),
-					// TODO: address is not stored here?
-					destination: Address::new(),
+					// callcreate test does not need an address
+					destination: None,
 					_gas_limit: *gas,
 					value: *value
 				});
-				Ok((gas_left, Some(address)))
+				let address = contract_address(&ext.params.address, &ext.state.nonce(&ext.params.address));
+				(gas_left, Some(address))
 			},
 			other => other
 		}
@@ -110,21 +110,20 @@ impl<'a> Ext for TestExt<'a> {
 			value: &U256, 
 			data: &[u8], 
 			code_address: &Address, 
-			output: &mut [u8]) -> Result<U256, evm::Error> {
+			output: &mut [u8]) -> Result<(U256, bool), evm::Error> {
 		let res = self.ext.call(gas, call_gas, receive_address, value, data, code_address, output);
 		let ext = &self.ext;
-		match res {
-			Ok(gas_left) if ext.state.balance(&ext.params.address) >= *value => {
+		if let &Ok(_some) = &res {
+			if ext.state.balance(&ext.params.address) >= *value {
 				self.callcreates.push(CallCreate {
 					data: data.to_vec(),
-					destination: receive_address.clone(),
+					destination: Some(receive_address.clone()),
 					_gas_limit: *call_gas,
 					value: *value
 				});
-				Ok(gas_left)
-			},
-			other => other
+			}
 		}
+		res
 	}
 
 	fn extcode(&self, address: &Address) -> Vec<u8> {
@@ -156,6 +155,7 @@ fn do_json_test(json_data: &[u8]) -> Vec<String> {
 	let json = Json::from_str(::std::str::from_utf8(json_data).unwrap()).expect("Json is invalid");
 	let mut failed = Vec::new();
 	for (name, test) in json.as_object().unwrap() {
+		println!("name: {:?}", name);
 		// sync io is usefull when something crashes in jit
 		//::std::io::stdout().write(&name.as_bytes());
 		//::std::io::stdout().write(b"\n");

src/tests/state.rs

@@ -11,6 +11,7 @@ fn do_json_test(json_data: &[u8]) -> Vec<String> {
 	let engine = ethereum::new_frontier_test().to_engine().unwrap();
 
 	for (name, test) in json.as_object().unwrap() {
+		println!("name: {:?}", name);
 		let mut fail = false;
 		let mut fail_unless = |cond: bool| if !cond && !fail { failed.push(name.to_string()); fail = true; true } else {false};
 
@@ -58,4 +59,7 @@ fn do_json_test(json_data: &[u8]) -> Vec<String> {
 }
 
 declare_test!{StateTests_stExample, "StateTests/stExample"}
-declare_test!{StateTests_stLogTests, "StateTests/stLogTests"}
+declare_test!{StateTests_stBlockHashTest, "StateTests/stBlockHashTest"}
+declare_test!{StateTests_stLogTests, "StateTests/stLogTests"}
+declare_test!{StateTests_stCallCodes, "StateTests/stCallCodes"}
+declare_test_ignore!{StateTests_stCallCreateCallCodeTest, "StateTests/stCallCreateCallCodeTest"}