From 29baccd857fea3d2d3298528cb3c3327b0b5af22 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 31 Jul 2018 13:27:57 +0800 Subject: [PATCH] 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 * Address grumbles --- ethcore/evm/src/instructions.rs | 3 ++ ethcore/evm/src/interpreter/gasometer.rs | 3 ++ ethcore/evm/src/interpreter/mod.rs | 18 +++++++--- ethcore/src/client/client.rs | 2 +- ethcore/src/executive.rs | 2 +- ethcore/src/externalities.rs | 16 +++++---- ethcore/src/json_tests/executive.rs | 8 +++-- ethcore/src/machine.rs | 2 +- ethcore/src/spec/spec.rs | 7 ++++ ethcore/src/state/account.rs | 16 +++++---- ethcore/src/state/mod.rs | 46 +++++++++++++++--------- ethcore/vm/src/ext.rs | 7 ++-- ethcore/vm/src/schedule.rs | 8 +++++ ethcore/vm/src/tests.rs | 13 ++++--- json/src/spec/params.rs | 3 ++ 15 files changed, 111 insertions(+), 43 deletions(-) diff --git a/ethcore/evm/src/instructions.rs b/ethcore/evm/src/instructions.rs index 7a41ed33c..3be455694 100644 --- a/ethcore/evm/src/instructions.rs +++ b/ethcore/evm/src/instructions.rs @@ -134,6 +134,8 @@ enum_with_from_u8! { RETURNDATASIZE = 0x3d, #[doc = "copy return data buffer to memory"] RETURNDATACOPY = 0x3e, + #[doc = "return the keccak256 hash of contract code"] + EXTCODEHASH = 0x3f, #[doc = "get hash of most recent complete block"] BLOCKHASH = 0x40, @@ -492,6 +494,7 @@ lazy_static! { arr[CALLDATALOAD as usize] = Some(InstructionInfo::new("CALLDATALOAD", 1, 1, GasPriceTier::VeryLow)); arr[CALLDATASIZE as usize] = Some(InstructionInfo::new("CALLDATASIZE", 0, 1, GasPriceTier::Base)); arr[CALLDATACOPY as usize] = Some(InstructionInfo::new("CALLDATACOPY", 3, 0, GasPriceTier::VeryLow)); + arr[EXTCODEHASH as usize] = Some(InstructionInfo::new("EXTCODEHASH", 1, 1, GasPriceTier::Special)); arr[CODESIZE as usize] = Some(InstructionInfo::new("CODESIZE", 0, 1, GasPriceTier::Base)); arr[CODECOPY as usize] = Some(InstructionInfo::new("CODECOPY", 3, 0, GasPriceTier::VeryLow)); arr[GASPRICE as usize] = Some(InstructionInfo::new("GASPRICE", 0, 1, GasPriceTier::Base)); diff --git a/ethcore/evm/src/interpreter/gasometer.rs b/ethcore/evm/src/interpreter/gasometer.rs index dbb333837..943a8320b 100644 --- a/ethcore/evm/src/interpreter/gasometer.rs +++ b/ethcore/evm/src/interpreter/gasometer.rs @@ -143,6 +143,9 @@ impl Gasometer { instructions::EXTCODESIZE => { Request::Gas(Gas::from(schedule.extcodesize_gas)) }, + instructions::EXTCODEHASH => { + Request::Gas(Gas::from(schedule.extcodehash_gas)) + }, instructions::SUICIDE => { let mut gas = Gas::from(schedule.suicide_gas); diff --git a/ethcore/evm/src/interpreter/mod.rs b/ethcore/evm/src/interpreter/mod.rs index 111986913..6b8abcc17 100644 --- a/ethcore/evm/src/interpreter/mod.rs +++ b/ethcore/evm/src/interpreter/mod.rs @@ -230,8 +230,9 @@ impl Interpreter { (instruction == instructions::STATICCALL && !schedule.have_static_call) || ((instruction == instructions::RETURNDATACOPY || instruction == instructions::RETURNDATASIZE) && !schedule.have_return_data) || (instruction == instructions::REVERT && !schedule.have_revert) || - ((instruction == instructions::SHL || instruction == instructions::SHR || instruction == instructions::SAR) && !schedule.have_bitwise_shifting) { - + ((instruction == instructions::SHL || instruction == instructions::SHR || instruction == instructions::SAR) && !schedule.have_bitwise_shifting) || + (instruction == instructions::EXTCODEHASH && !schedule.have_extcodehash) + { return Err(vm::Error::BadInstruction { instruction: instruction as u8 }); @@ -568,9 +569,14 @@ impl Interpreter { }, instructions::EXTCODESIZE => { let address = u256_to_address(&stack.pop_back()); - let len = ext.extcodesize(&address)?; + let len = ext.extcodesize(&address)?.unwrap_or(0); stack.push(U256::from(len)); }, + instructions::EXTCODEHASH => { + let address = u256_to_address(&stack.pop_back()); + let hash = ext.extcodehash(&address)?.unwrap_or_else(H256::zero); + stack.push(U256::from(hash)); + }, instructions::CALLDATACOPY => { Self::copy_data_to_memory(&mut self.mem, stack, params.data.as_ref().map_or_else(|| &[] as &[u8], |d| &*d as &[u8])); }, @@ -591,7 +597,11 @@ impl Interpreter { instructions::EXTCODECOPY => { let address = u256_to_address(&stack.pop_back()); let code = ext.extcode(&address)?; - Self::copy_data_to_memory(&mut self.mem, stack, &code); + Self::copy_data_to_memory( + &mut self.mem, + stack, + code.as_ref().map(|c| &(*c)[..]).unwrap_or(&[]) + ); }, instructions::GASPRICE => { stack.push(params.gas_price.clone()); diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 47974356f..a68c1e495 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -1339,7 +1339,7 @@ impl BlockInfo for Client { } fn code_hash(&self, address: &Address, id: BlockId) -> Option { - 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)) } } diff --git a/ethcore/src/executive.rs b/ethcore/src/executive.rs index 1243eeea3..9bb0bc8f9 100644 --- a/ethcore/src/executive.rs +++ b/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, diff --git a/ethcore/src/externalities.rs b/ethcore/src/externalities.rs index 9aa308a23..c1bd785c4 100644 --- a/ethcore/src/externalities.rs +++ b/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> { - Ok(self.state.code(address)?.unwrap_or_else(|| Arc::new(vec![]))) + fn extcode(&self, address: &Address) -> vm::Result>> { + Ok(self.state.code(address)?) } - fn extcodesize(&self, address: &Address) -> vm::Result { - Ok(self.state.code_size(address)?.unwrap_or(0)) + fn extcodehash(&self, address: &Address) -> vm::Result> { + Ok(self.state.code_hash(address)?) + } + + fn extcodesize(&self, address: &Address) -> vm::Result> { + Ok(self.state.code_size(address)?) } fn ret(mut self, gas: &U256, data: &ReturnData, apply_state: bool) -> vm::Result diff --git a/ethcore/src/json_tests/executive.rs b/ethcore/src/json_tests/executive.rs index 3fc14be2a..a8fd4b453 100644 --- a/ethcore/src/json_tests/executive.rs +++ b/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> { + fn extcode(&self, address: &Address) -> vm::Result>> { self.ext.extcode(address) } - fn extcodesize(&self, address: &Address) -> vm::Result { + fn extcodesize(&self, address: &Address) -> vm::Result> { self.ext.extcodesize(address) } + fn extcodehash(&self, address: &Address) -> vm::Result> { + self.ext.extcodehash(address) + } + fn log(&mut self, topics: Vec, data: &[u8]) -> vm::Result<()> { self.ext.log(topics, data) } diff --git a/ethcore/src/machine.rs b/ethcore/src/machine.rs index 633f837dc..d58a3878d 100644 --- a/ethcore/src/machine.rs +++ b/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, diff --git a/ethcore/src/spec/spec.rs b/ethcore/src/spec/spec.rs index 015755f83..d79775f78 100644 --- a/ethcore/src/spec/spec.rs +++ b/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 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, diff --git a/ethcore/src/state/account.rs b/ethcore/src/state/account.rs index 359b84b1e..eec713a4e 100644 --- a/ethcore/src/state/account.rs +++ b/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) -> Option> { // 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) { 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) -> 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 } } diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 796ec0c1d..323e11ccb 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -608,9 +608,9 @@ impl State { } /// Get an account's code hash. - pub fn code_hash(&self, a: &Address) -> TrieResult { + pub fn code_hash(&self, a: &Address) -> TrieResult> { 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 State { 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) { + /// 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) -> 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 State { 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 State { 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 State { 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)); diff --git a/ethcore/vm/src/ext.rs b/ethcore/vm/src/ext.rs index 166e8712a..b6d41a395 100644 --- a/ethcore/vm/src/ext.rs +++ b/ethcore/vm/src/ext.rs @@ -106,10 +106,13 @@ pub trait Ext { ) -> MessageCallResult; /// Returns code at given address - fn extcode(&self, address: &Address) -> Result>; + fn extcode(&self, address: &Address) -> Result>>; + + /// Returns code hash at given address + fn extcodehash(&self, address: &Address) -> Result>; /// Returns code size at given address - fn extcodesize(&self, address: &Address) -> Result; + fn extcodesize(&self, address: &Address) -> Result>; /// Creates log entry with given topics and data fn log(&mut self, topics: Vec, data: &[u8]) -> Result<()>; diff --git a/ethcore/vm/src/schedule.rs b/ethcore/vm/src/schedule.rs index 960821e72..6215aed78 100644 --- a/ethcore/vm/src/schedule.rs +++ b/ethcore/vm/src/schedule.rs @@ -26,6 +26,8 @@ pub struct Schedule { pub have_create2: bool, /// Does it have a REVERT instruction pub have_revert: bool, + /// Does it have a EXTCODEHASH instruction + pub have_extcodehash: bool, /// VM stack limit pub stack_limit: usize, /// Max number of nested calls/creates @@ -92,6 +94,8 @@ pub struct Schedule { pub extcodecopy_base_gas: usize, /// Price of BALANCE pub balance_gas: usize, + /// Price of EXTCODEHASH + pub extcodehash_gas: usize, /// Price of SUICIDE pub suicide_gas: usize, /// Amount of additional gas to pay when SUICIDE credits a non-existant account @@ -197,6 +201,7 @@ impl Schedule { have_revert: false, have_return_data: false, have_bitwise_shifting: false, + have_extcodehash: false, stack_limit: 1024, max_depth: 1024, tier_step_gas: [0, 2, 3, 5, 8, 10, 20, 0], @@ -229,6 +234,7 @@ impl Schedule { copy_gas: 3, extcodesize_gas: 700, extcodecopy_base_gas: 700, + extcodehash_gas: 400, balance_gas: 400, suicide_gas: 5000, suicide_to_new_account_cost: 25000, @@ -268,6 +274,7 @@ impl Schedule { have_revert: false, have_return_data: false, have_bitwise_shifting: false, + have_extcodehash: false, stack_limit: 1024, max_depth: 1024, tier_step_gas: [0, 2, 3, 5, 8, 10, 20, 0], @@ -300,6 +307,7 @@ impl Schedule { copy_gas: 3, extcodesize_gas: 20, extcodecopy_base_gas: 20, + extcodehash_gas: 400, balance_gas: 20, suicide_gas: 0, suicide_to_new_account_cost: 0, diff --git a/ethcore/vm/src/tests.rs b/ethcore/vm/src/tests.rs index 9a17e0d3d..d83e6881a 100644 --- a/ethcore/vm/src/tests.rs +++ b/ethcore/vm/src/tests.rs @@ -24,6 +24,7 @@ use { ReturnData, Ext, ContractCreateResult, MessageCallResult, CreateContractAddress, Result, GasLeft, }; +use hash::keccak; pub struct FakeLogEntry { pub topics: Vec, @@ -168,12 +169,16 @@ impl Ext for FakeExt { MessageCallResult::Success(*gas, ReturnData::empty()) } - fn extcode(&self, address: &Address) -> Result> { - Ok(self.codes.get(address).unwrap_or(&Arc::new(Bytes::new())).clone()) + fn extcode(&self, address: &Address) -> Result>> { + Ok(self.codes.get(address).cloned()) } - fn extcodesize(&self, address: &Address) -> Result { - Ok(self.codes.get(address).map_or(0, |c| c.len())) + fn extcodesize(&self, address: &Address) -> Result> { + Ok(self.codes.get(address).map(|c| c.len())) + } + + fn extcodehash(&self, address: &Address) -> Result> { + Ok(self.codes.get(address).map(|c| keccak(c.as_ref()))) } fn log(&mut self, topics: Vec, data: &[u8]) -> Result<()> { diff --git a/json/src/spec/params.rs b/json/src/spec/params.rs index e03fe7081..a37e4f23b 100644 --- a/json/src/spec/params.rs +++ b/json/src/spec/params.rs @@ -109,6 +109,9 @@ pub struct Params { #[serde(rename="eip658Transition")] pub eip658_transition: Option, /// See `CommonParams` docs. + #[serde(rename="eip1052Transition")] + pub eip1052_transition: Option, + /// See `CommonParams` docs. #[serde(rename="dustProtectionTransition")] pub dust_protection_transition: Option, /// See `CommonParams` docs.