Implement EIP-1052 (EXTCODEHASH) and fix several issues in state account cache (#9234)

* Implement EIP-1052 and fix several issues related to account cache

* Fix jsontests

* Merge two matches together

* Avoid making unnecessary Arc<Vec>

* Address grumbles

ethcore/src/client/client.rs

@@ -1339,7 +1339,7 @@ impl BlockInfo for Client {
 	}
 
 	fn code_hash(&self, address: &Address, id: BlockId) -> Option<H256> {
-		self.state_at(id).and_then(|s| s.code_hash(address).ok())
+		self.state_at(id).and_then(|s| s.code_hash(address).unwrap_or(None))
 	}
 }
 

ethcore/src/executive.rs

@@ -323,7 +323,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
 					gas_price: t.gas_price,
 					value: ActionValue::Transfer(t.value),
 					code: self.state.code(address)?,
-					code_hash: Some(self.state.code_hash(address)?),
+					code_hash: self.state.code_hash(address)?,
 					data: Some(t.data.clone()),
 					call_type: CallType::Call,
 					params_type: vm::ParamsType::Separate,

ethcore/src/externalities.rs

@@ -165,7 +165,7 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B>
 				gas: self.machine.params().eip210_contract_gas,
 				gas_price: 0.into(),
 				code: code,
-				code_hash: Some(code_hash),
+				code_hash: code_hash,
 				data: Some(H256::from(number).to_vec()),
 				call_type: CallType::Call,
 				params_type: vm::ParamsType::Separate,
@@ -272,7 +272,7 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B>
 			gas: *gas,
 			gas_price: self.origin_info.gas_price,
 			code: code,
-			code_hash: Some(code_hash),
+			code_hash: code_hash,
 			data: Some(data.to_vec()),
 			call_type: call_type,
 			params_type: vm::ParamsType::Separate,
@@ -291,12 +291,16 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B>
 		}
 	}
 
-	fn extcode(&self, address: &Address) -> vm::Result<Arc<Bytes>> {
-		Ok(self.state.code(address)?.unwrap_or_else(|| Arc::new(vec![])))
+	fn extcode(&self, address: &Address) -> vm::Result<Option<Arc<Bytes>>> {
+		Ok(self.state.code(address)?)
 	}
 
-	fn extcodesize(&self, address: &Address) -> vm::Result<usize> {
-		Ok(self.state.code_size(address)?.unwrap_or(0))
+	fn extcodehash(&self, address: &Address) -> vm::Result<Option<H256>> {
+		Ok(self.state.code_hash(address)?)
+	}
+
+	fn extcodesize(&self, address: &Address) -> vm::Result<Option<usize>> {
+		Ok(self.state.code_size(address)?)
 	}
 
 	fn ret(mut self, gas: &U256, data: &ReturnData, apply_state: bool) -> vm::Result<U256>

ethcore/src/json_tests/executive.rs

@@ -166,14 +166,18 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for TestExt<'a, T, V, B>
 		MessageCallResult::Success(*gas, ReturnData::empty())
 	}
 
-	fn extcode(&self, address: &Address) -> vm::Result<Arc<Bytes>>  {
+	fn extcode(&self, address: &Address) -> vm::Result<Option<Arc<Bytes>>>  {
 		self.ext.extcode(address)
 	}
 
-	fn extcodesize(&self, address: &Address) -> vm::Result<usize> {
+	fn extcodesize(&self, address: &Address) -> vm::Result<Option<usize>> {
 		self.ext.extcodesize(address)
 	}
 
+	fn extcodehash(&self, address: &Address) -> vm::Result<Option<H256>> {
+		self.ext.extcodehash(address)
+	}
+
 	fn log(&mut self, topics: Vec<H256>, data: &[u8]) -> vm::Result<()> {
 		self.ext.log(topics, data)
 	}

ethcore/src/machine.rs

@@ -140,7 +140,7 @@ impl EthereumMachine {
 			gas_price: 0.into(),
 			value: ActionValue::Transfer(0.into()),
 			code: state.code(&contract_address)?,
-			code_hash: Some(state.code_hash(&contract_address)?),
+			code_hash: state.code_hash(&contract_address)?,
 			data: data,
 			call_type: CallType::Call,
 			params_type: ParamsType::Separate,

ethcore/src/spec/spec.rs

@@ -115,6 +115,8 @@ pub struct CommonParams {
 	pub eip214_transition: BlockNumber,
 	/// Number of first block where EIP-145 rules begin.
 	pub eip145_transition: BlockNumber,
+	/// Number of first block where EIP-1052 rules begin.
+	pub eip1052_transition: BlockNumber,
 	/// Number of first block where dust cleanup rules (EIP-168 and EIP169) begin.
 	pub dust_protection_transition: BlockNumber,
 	/// Nonce cap increase per block. Nonce cap is only checked if dust protection is enabled.
@@ -174,6 +176,7 @@ impl CommonParams {
 		schedule.have_static_call = block_number >= self.eip214_transition;
 		schedule.have_return_data = block_number >= self.eip211_transition;
 		schedule.have_bitwise_shifting = block_number >= self.eip145_transition;
+		schedule.have_extcodehash = block_number >= self.eip1052_transition;
 		if block_number >= self.eip210_transition {
 			schedule.blockhash_gas = 800;
 		}
@@ -270,6 +273,10 @@ impl From<ethjson::spec::Params> for CommonParams {
 				BlockNumber::max_value,
 				Into::into,
 			),
+			eip1052_transition: p.eip1052_transition.map_or_else(
+				BlockNumber::max_value,
+				Into::into,
+			),
 			dust_protection_transition: p.dust_protection_transition.map_or_else(
 				BlockNumber::max_value,
 				Into::into,

ethcore/src/state/account.rs

@@ -278,12 +278,13 @@ impl Account {
 		!self.code_cache.is_empty() || (self.code_cache.is_empty() && self.code_hash == KECCAK_EMPTY)
 	}
 
-	/// Provide a database to get `code_hash`. Should not be called if it is a contract without code.
+	/// Provide a database to get `code_hash`. Should not be called if it is a contract without code. Returns the cached code, if successful.
+	#[must_use]
 	pub fn cache_code(&mut self, db: &HashDB<KeccakHasher>) -> Option<Arc<Bytes>> {
 		// TODO: fill out self.code_cache;
 		trace!("Account::cache_code: ic={}; self.code_hash={:?}, self.code_cache={}", self.is_cached(), self.code_hash, self.code_cache.pretty());
 
-		if self.is_cached() { return Some(self.code_cache.clone()) }
+		if self.is_cached() { return Some(self.code_cache.clone()); }
 
 		match db.get(&self.code_hash) {
 			Some(x) => {
@@ -298,8 +299,7 @@ impl Account {
 		}
 	}
 
-	/// Provide code to cache. For correctness, should be the correct code for the
-	/// account.
+	/// Provide code to cache. For correctness, should be the correct code for the account.
 	pub fn cache_given_code(&mut self, code: Arc<Bytes>) {
 		trace!("Account::cache_given_code: ic={}; self.code_hash={:?}, self.code_cache={}", self.is_cached(), self.code_hash, self.code_cache.pretty());
 
@@ -307,7 +307,9 @@ impl Account {
 		self.code_cache = code;
 	}
 
-	/// Provide a database to get `code_size`. Should not be called if it is a contract without code.
+	/// Provide a database to get `code_size`. Should not be called if it is a contract without code. Returns whether
+	/// the cache succeeds.
+	#[must_use]
 	pub fn cache_code_size(&mut self, db: &HashDB<KeccakHasher>) -> bool {
 		// TODO: fill out self.code_cache;
 		trace!("Account::cache_code_size: ic={}; self.code_hash={:?}, self.code_cache={}", self.is_cached(), self.code_hash, self.code_cache.pretty());
@@ -324,7 +326,9 @@ impl Account {
 					},
 				}
 			} else {
-				false
+				// If the code hash is empty hash, then the code size is zero.
+				self.code_size = Some(0);
+				true
 			}
 	}
 

ethcore/src/state/mod.rs

@@ -608,9 +608,9 @@ impl<B: Backend> State<B> {
 	}
 
 	/// Get an account's code hash.
-	pub fn code_hash(&self, a: &Address) -> TrieResult<H256> {
+	pub fn code_hash(&self, a: &Address) -> TrieResult<Option<H256>> {
 		self.ensure_cached(a, RequireCache::None, true,
-			|a| a.as_ref().map_or(KECCAK_EMPTY, |a| a.code_hash()))
+			|a| a.as_ref().map(|a| a.code_hash()))
 	}
 
 	/// Get accounts' code size.
@@ -911,31 +911,38 @@ impl<B: Backend> State<B> {
 		Ok(pod_state::diff_pod(&pod_state_pre, &pod_state_post))
 	}
 
-	// load required account data from the databases.
-	fn update_account_cache(require: RequireCache, account: &mut Account, state_db: &B, db: &HashDB<KeccakHasher>) {
+	/// Load required account data from the databases. Returns whether the cache succeeds.
+	#[must_use]
+	fn update_account_cache(require: RequireCache, account: &mut Account, state_db: &B, db: &HashDB<KeccakHasher>) -> bool {
 		if let RequireCache::None = require {
-			return;
+			return true;
 		}
 
 		if account.is_cached() {
-			return;
+			return true;
 		}
 
 		// if there's already code in the global cache, always cache it localy
 		let hash = account.code_hash();
 		match state_db.get_cached_code(&hash) {
-			Some(code) => account.cache_given_code(code),
+			Some(code) => {
+				account.cache_given_code(code);
+				true
+			},
 			None => match require {
-				RequireCache::None => {},
+				RequireCache::None => true,
 				RequireCache::Code => {
 					if let Some(code) = account.cache_code(db) {
 						// propagate code loaded from the database to
 						// the global code cache.
-						state_db.cache_code(hash, code)
+						state_db.cache_code(hash, code);
+						true
+					} else {
+						false
 					}
 				},
 				RequireCache::CodeSize => {
-					account.cache_code_size(db);
+					account.cache_code_size(db)
 				}
 			}
 		}
@@ -950,8 +957,11 @@ impl<B: Backend> State<B> {
 		if let Some(ref mut maybe_acc) = self.cache.borrow_mut().get_mut(a) {
 			if let Some(ref mut account) = maybe_acc.account {
 				let accountdb = self.factories.accountdb.readonly(self.db.as_hashdb(), account.address_hash(a));
-				Self::update_account_cache(require, account, &self.db, accountdb.as_hashdb());
-				return Ok(f(Some(account)));
+				if Self::update_account_cache(require, account, &self.db, accountdb.as_hashdb()) {
+					return Ok(f(Some(account)));
+				} else {
+					return Err(Box::new(TrieError::IncompleteDatabase(H256::from(a))));
+				}
 			}
 			return Ok(f(None));
 		}
@@ -959,12 +969,14 @@ impl<B: Backend> State<B> {
 		let result = self.db.get_cached(a, |mut acc| {
 			if let Some(ref mut account) = acc {
 				let accountdb = self.factories.accountdb.readonly(self.db.as_hashdb(), account.address_hash(a));
-				Self::update_account_cache(require, account, &self.db, accountdb.as_hashdb());
+				if !Self::update_account_cache(require, account, &self.db, accountdb.as_hashdb()) {
+					return Err(Box::new(TrieError::IncompleteDatabase(H256::from(a))));
+				}
 			}
-			f(acc.map(|a| &*a))
+			Ok(f(acc.map(|a| &*a)))
 		});
 		match result {
-			Some(r) => Ok(r),
+			Some(r) => Ok(r?),
 			None => {
 				// first check if it is not in database for sure
 				if check_null && self.db.is_known_null(a) { return Ok(f(None)); }
@@ -975,7 +987,9 @@ impl<B: Backend> State<B> {
 				let mut maybe_acc = db.get_with(a, from_rlp)?;
 				if let Some(ref mut account) = maybe_acc.as_mut() {
 					let accountdb = self.factories.accountdb.readonly(self.db.as_hashdb(), account.address_hash(a));
-					Self::update_account_cache(require, account, &self.db, accountdb.as_hashdb());
+					if !Self::update_account_cache(require, account, &self.db, accountdb.as_hashdb()) {
+						return Err(Box::new(TrieError::IncompleteDatabase(H256::from(a))));
+					}
 				}
 				let r = f(maybe_acc.as_ref());
 				self.insert_cache(a, AccountEntry::new_clean(maybe_acc));