From b7a94023ba6c9f68d982e9310eea2465d6f1d805 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 9 Aug 2018 01:46:42 +0800 Subject: [PATCH 01/40] Implement last_checkpoint_storage_at --- ethcore/src/state/account.rs | 18 +++++++++ ethcore/src/state/mod.rs | 73 +++++++++++++++++++++++++++++++++--- 2 files changed, 85 insertions(+), 6 deletions(-) diff --git a/ethcore/src/state/account.rs b/ethcore/src/state/account.rs index eec713a4e36..5a74b3945b4 100644 --- a/ethcore/src/state/account.rs +++ b/ethcore/src/state/account.rs @@ -204,6 +204,19 @@ impl Account { if let Some(value) = self.cached_storage_at(key) { return Ok(value); } + self.get_and_cache_storage(db, key) + } + + /// Get (and cache) the contents of the trie's storage at `key`. + /// Does not take modified storage into account. + pub fn original_storage_at(&self, db: &HashDB, key: &H256) -> TrieResult { + if let Some(value) = self.cached_original_storage_at(key) { + return Ok(value); + } + self.get_and_cache_storage(db, key) + } + + fn get_and_cache_storage(&self, db: &HashDB, key: &H256) -> TrieResult { let db = SecTrieDB::new(db, &self.storage_root)?; let panicky_decoder = |bytes:&[u8]| ::rlp::decode(&bytes).expect("decoding db value failed"); let item: U256 = db.get_with(key, panicky_decoder)?.unwrap_or_else(U256::zero); @@ -218,6 +231,11 @@ impl Account { if let Some(value) = self.storage_changes.get(key) { return Some(value.clone()) } + self.cached_original_storage_at(key) + } + + /// Get cached original storage value after last state commitment. Returns `None` if the key is not in the cache. + pub fn cached_original_storage_at(&self, key: &H256) -> Option { if let Some(value) = self.storage_cache.borrow_mut().get_mut(key) { return Some(value.clone()) } diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 323e11ccb2b..ad0cbfe3798 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -539,8 +539,49 @@ impl State { |a| a.as_ref().and_then(|account| account.storage_root().cloned())) } - /// Mutate storage of account `address` so that it is `value` for `key`. - pub fn storage_at(&self, address: &Address, key: &H256) -> TrieResult { + /// Get the value of storage at last checkpoint. + pub fn last_checkpoint_storage_at(&self, address: &Address, key: &H256) -> TrieResult { + if let Some(ref checkpoint) = self.checkpoints.borrow().last() { + if let Some(entry) = checkpoint.get(address) { + match entry { + Some(entry) => { + match entry.account { + Some(ref account) => { + if let Some(value) = account.cached_storage_at(key) { + Ok(value) + } else { + // This key has checkpoint entry, but not in the entry's cache. + // So it's not modified at all. + self.original_storage_at(address, key) + } + }, + None => { + // The account didn't exist at that point. Return empty value. + Ok(H256::new()) + }, + } + }, + None => { + // The value was not cached at that checkpoint, meaning it was not modified. + self.original_storage_at(address, key) + }, + } + } else { + // This key does not have a checkpoint entry. We use the current value. + self.storage_at(address, key) + } + } else { + // No checkpoint has been found. We use the current value. + self.storage_at(address, key) + } + } + + fn storage_at_inner( + &self, address: &Address, key: &H256, f_cached_at: FCachedStorageAt, f_at: FStorageAt, + ) -> TrieResult where + FCachedStorageAt: Fn(&Account, &H256) -> Option, + FStorageAt: Fn(&Account, &HashDB, &H256) -> TrieResult + { // Storage key search and update works like this: // 1. If there's an entry for the account in the local cache check for the key and return it if found. // 2. If there's an entry for the account in the global cache check for the key or load it into that account. @@ -553,7 +594,7 @@ impl State { if let Some(maybe_acc) = local_cache.get(address) { match maybe_acc.account { Some(ref account) => { - if let Some(value) = account.cached_storage_at(key) { + if let Some(value) = f_cached_at(account, key) { return Ok(value); } else { local_account = Some(maybe_acc); @@ -567,7 +608,7 @@ impl State { None => Ok(H256::new()), Some(a) => { let account_db = self.factories.accountdb.readonly(self.db.as_hashdb(), a.address_hash(address)); - a.storage_at(account_db.as_hashdb(), key) + f_at(a, account_db.as_hashdb(), key) } }); @@ -579,7 +620,7 @@ impl State { if let Some(ref mut acc) = local_account { if let Some(ref account) = acc.account { let account_db = self.factories.accountdb.readonly(self.db.as_hashdb(), account.address_hash(address)); - return account.storage_at(account_db.as_hashdb(), key) + return f_at(account, account_db.as_hashdb(), key) } else { return Ok(H256::new()) } @@ -595,12 +636,32 @@ impl State { let maybe_acc = db.get_with(address, from_rlp)?; let r = maybe_acc.as_ref().map_or(Ok(H256::new()), |a| { let account_db = self.factories.accountdb.readonly(self.db.as_hashdb(), a.address_hash(address)); - a.storage_at(account_db.as_hashdb(), key) + f_at(a, account_db.as_hashdb(), key) }); self.insert_cache(address, AccountEntry::new_clean(maybe_acc)); r } + /// Mutate storage of account `address` so that it is `value` for `key`. + pub fn storage_at(&self, address: &Address, key: &H256) -> TrieResult { + self.storage_at_inner( + address, + key, + |account, key| { account.cached_storage_at(key) }, + |account, db, key| { account.storage_at(db, key) }, + ) + } + + /// Get the value of storage after last state commitment. + pub fn original_storage_at(&self, address: &Address, key: &H256) -> TrieResult { + self.storage_at_inner( + address, + key, + |account, key| { account.cached_original_storage_at(key) }, + |account, db, key| { account.original_storage_at(db, key) }, + ) + } + /// Get accounts' code. pub fn code(&self, a: &Address) -> TrieResult>> { self.ensure_cached(a, RequireCache::Code, true, From a082907d53f567225944a8bd1de92bb3d6c8297f Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 9 Aug 2018 01:53:56 +0800 Subject: [PATCH 02/40] Add reverted_storage_at for externalities --- ethcore/src/externalities.rs | 6 +++++- ethcore/vm/src/ext.rs | 3 +++ ethcore/vm/src/tests.rs | 4 ++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/ethcore/src/externalities.rs b/ethcore/src/externalities.rs index 68d2c0817f0..24d7e245ff0 100644 --- a/ethcore/src/externalities.rs +++ b/ethcore/src/externalities.rs @@ -113,6 +113,10 @@ impl<'a, T: 'a, V: 'a, B: 'a> Externalities<'a, T, V, B> impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B> where T: Tracer, V: VMTracer, B: StateBackend { + fn reverted_storage_at(&self, key: &H256) -> vm::Result { + self.state.last_checkpoint_storage_at(&self.origin_info.address, key).map_err(Into::into) + } + fn storage_at(&self, key: &H256) -> vm::Result { self.state.storage_at(&self.origin_info.address, key).map_err(Into::into) } @@ -613,7 +617,7 @@ mod tests { let address = { let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract(None), &mut tracer, &mut vm_tracer, false); - + match ext.create(&U256::max_value(), &U256::zero(), &[], CreateContractAddress::FromSenderSaltAndCodeHash(H256::default())) { ContractCreateResult::Created(address, _) => address, _ => panic!("Test create failed; expected Created, got Failed/Reverted."), diff --git a/ethcore/vm/src/ext.rs b/ethcore/vm/src/ext.rs index c1ce1b79f0b..55d679a1b70 100644 --- a/ethcore/vm/src/ext.rs +++ b/ethcore/vm/src/ext.rs @@ -63,6 +63,9 @@ pub enum CreateContractAddress { /// Externalities interface for EVMs pub trait Ext { + /// Returns the storage value for a given key if reversion happens. + fn reverted_storage_at(&self, key: &H256) -> Result; + /// Returns a value for given key. fn storage_at(&self, key: &H256) -> Result; diff --git a/ethcore/vm/src/tests.rs b/ethcore/vm/src/tests.rs index 4930e4219ba..3a1701d4533 100644 --- a/ethcore/vm/src/tests.rs +++ b/ethcore/vm/src/tests.rs @@ -105,6 +105,10 @@ impl FakeExt { } impl Ext for FakeExt { + fn reverted_storage_at(&self, _key: &H256) -> Result { + Ok(H256::new()) + } + fn storage_at(&self, key: &H256) -> Result { Ok(self.store.get(key).unwrap_or(&H256::new()).clone()) } From 3ba54678038d81eee01e4056afc94463bc728874 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 9 Aug 2018 02:13:04 +0800 Subject: [PATCH 03/40] sstore_clears_count -> sstore_clears_refund --- ethcore/evm/src/interpreter/mod.rs | 3 ++- ethcore/src/executive.rs | 2 +- ethcore/src/externalities.rs | 8 ++++++-- ethcore/src/state/substate.rs | 12 ++++++------ ethcore/vm/src/ext.rs | 7 +++++-- ethcore/vm/src/schedule.rs | 4 ++++ ethcore/vm/src/tests.rs | 10 +++++++--- ethcore/wasm/src/runtime.rs | 3 ++- 8 files changed, 33 insertions(+), 16 deletions(-) diff --git a/ethcore/evm/src/interpreter/mod.rs b/ethcore/evm/src/interpreter/mod.rs index 3af1e34dd36..8865c2f09ab 100644 --- a/ethcore/evm/src/interpreter/mod.rs +++ b/ethcore/evm/src/interpreter/mod.rs @@ -516,7 +516,8 @@ impl Interpreter { let current_val = U256::from(&*ext.storage_at(&address)?); // Increase refund for clear if !self.is_zero(¤t_val) && self.is_zero(&val) { - ext.inc_sstore_clears(); + let sstore_clears_schedule = U256::from(ext.schedule().sstore_refund_gas); + ext.inc_sstore_refund(sstore_clears_schedule); } ext.set_storage(address, H256::from(&val))?; }, diff --git a/ethcore/src/executive.rs b/ethcore/src/executive.rs index b77cb050e41..fd3e2282dfc 100644 --- a/ethcore/src/executive.rs +++ b/ethcore/src/executive.rs @@ -616,7 +616,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { let schedule = self.schedule; // refunds from SSTORE nonzero -> zero - let sstore_refunds = U256::from(schedule.sstore_refund_gas) * substate.sstore_clears_count; + let sstore_refunds = substate.sstore_clears_refund; // refunds from contract suicides let suicide_refunds = U256::from(schedule.suicide_refund_gas) * U256::from(substate.suicides.len()); let refunds_bound = sstore_refunds + suicide_refunds; diff --git a/ethcore/src/externalities.rs b/ethcore/src/externalities.rs index 24d7e245ff0..9d35de46b6b 100644 --- a/ethcore/src/externalities.rs +++ b/ethcore/src/externalities.rs @@ -400,8 +400,12 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B> self.depth } - fn inc_sstore_clears(&mut self) { - self.substate.sstore_clears_count = self.substate.sstore_clears_count + U256::one(); + fn inc_sstore_refund(&mut self, value: U256) { + self.substate.sstore_clears_refund = self.substate.sstore_clears_refund + value; + } + + fn dec_sstore_refund(&mut self, value: U256) { + self.substate.sstore_clears_refund = self.substate.sstore_clears_refund - value; } fn trace_next_instruction(&mut self, pc: usize, instruction: u8, current_gas: U256) -> bool { diff --git a/ethcore/src/state/substate.rs b/ethcore/src/state/substate.rs index c2f3c62dcb5..054b8b384bb 100644 --- a/ethcore/src/state/substate.rs +++ b/ethcore/src/state/substate.rs @@ -34,8 +34,8 @@ pub struct Substate { /// Any logs. pub logs: Vec, - /// Refund counter of SSTORE nonzero -> zero. - pub sstore_clears_count: U256, + /// Refund counter of SSTORE. + pub sstore_clears_refund: U256, /// Created contracts. pub contracts_created: Vec
, @@ -52,7 +52,7 @@ impl Substate { self.suicides.extend(s.suicides); self.touched.extend(s.touched); self.logs.extend(s.logs); - self.sstore_clears_count = self.sstore_clears_count + s.sstore_clears_count; + self.sstore_clears_refund = self.sstore_clears_refund + s.sstore_clears_refund; self.contracts_created.extend(s.contracts_created); } @@ -86,7 +86,7 @@ mod tests { topics: vec![], data: vec![] }); - sub_state.sstore_clears_count = 5.into(); + sub_state.sstore_clears_refund = (15000 * 5).into(); sub_state.suicides.insert(10u64.into()); let mut sub_state_2 = Substate::new(); @@ -96,11 +96,11 @@ mod tests { topics: vec![], data: vec![] }); - sub_state_2.sstore_clears_count = 7.into(); + sub_state_2.sstore_clears_refund = (15000 * 7).into(); sub_state.accrue(sub_state_2); assert_eq!(sub_state.contracts_created.len(), 2); - assert_eq!(sub_state.sstore_clears_count, 12.into()); + assert_eq!(sub_state.sstore_clears_refund, (15000 * 12).into()); assert_eq!(sub_state.suicides.len(), 1); } } diff --git a/ethcore/vm/src/ext.rs b/ethcore/vm/src/ext.rs index 55d679a1b70..44f1a802125 100644 --- a/ethcore/vm/src/ext.rs +++ b/ethcore/vm/src/ext.rs @@ -140,8 +140,11 @@ pub trait Ext { /// then A depth is 0, B is 1, C is 2 and so on. fn depth(&self) -> usize; - /// Increments sstore refunds count by 1. - fn inc_sstore_clears(&mut self); + /// Increments sstore refunds counter. + fn inc_sstore_refund(&mut self, value: U256); + + /// Decrements sstore refunds counter. + fn dec_sstore_refund(&mut self, value: U256); /// Decide if any more operations should be traced. Passthrough for the VM trace. fn trace_next_instruction(&mut self, _pc: usize, _instruction: u8, _current_gas: U256) -> bool { false } diff --git a/ethcore/vm/src/schedule.rs b/ethcore/vm/src/schedule.rs index ec72c4683fd..dd0ca416a39 100644 --- a/ethcore/vm/src/schedule.rs +++ b/ethcore/vm/src/schedule.rs @@ -119,6 +119,8 @@ pub struct Schedule { pub kill_dust: CleanDustMode, /// Enable EIP-86 rules pub eip86: bool, + /// Enable EIP-1283 rules + pub eip1283: bool, /// Wasm extra schedule settings, if wasm activated pub wasm: Option, } @@ -248,6 +250,7 @@ impl Schedule { have_static_call: false, kill_dust: CleanDustMode::Off, eip86: false, + eip1283: false, wasm: None, } } @@ -321,6 +324,7 @@ impl Schedule { have_static_call: false, kill_dust: CleanDustMode::Off, eip86: false, + eip1283: false, wasm: None, } } diff --git a/ethcore/vm/src/tests.rs b/ethcore/vm/src/tests.rs index 3a1701d4533..c82de5e9892 100644 --- a/ethcore/vm/src/tests.rs +++ b/ethcore/vm/src/tests.rs @@ -56,7 +56,7 @@ pub struct FakeExt { pub store: HashMap, pub suicides: HashSet
, pub calls: HashSet, - pub sstore_clears: usize, + pub sstore_clears: U256, pub depth: usize, pub blockhashes: HashMap, pub codes: HashMap>, @@ -221,8 +221,12 @@ impl Ext for FakeExt { self.is_static } - fn inc_sstore_clears(&mut self) { - self.sstore_clears += 1; + fn inc_sstore_refund(&mut self, value: U256) { + self.sstore_clears = self.sstore_clears + value; + } + + fn dec_sstore_refund(&mut self, value: U256) { + self.sstore_clears = self.sstore_clears - value; } fn trace_next_instruction(&mut self, _pc: usize, _instruction: u8, _gas: U256) -> bool { diff --git a/ethcore/wasm/src/runtime.rs b/ethcore/wasm/src/runtime.rs index 347023dd97c..e6554ddb3ae 100644 --- a/ethcore/wasm/src/runtime.rs +++ b/ethcore/wasm/src/runtime.rs @@ -284,7 +284,8 @@ impl<'a> Runtime<'a> { self.ext.set_storage(key, val).map_err(|_| Error::StorageUpdateError)?; if former_val != H256::zero() && val == H256::zero() { - self.ext.inc_sstore_clears(); + let sstore_clears_schedule = U256::from(self.schedule().sstore_refund_gas); + self.ext.inc_sstore_refund(sstore_clears_schedule); } Ok(()) From 87a93c03158326020034a61857cbb3f884c517f5 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 9 Aug 2018 02:29:29 +0800 Subject: [PATCH 04/40] Implement eip1283 for evm --- ethcore/evm/src/interpreter/gasometer.rs | 69 ++++++++++++++++++++++-- ethcore/evm/src/interpreter/mod.rs | 11 ++-- 2 files changed, 72 insertions(+), 8 deletions(-) diff --git a/ethcore/evm/src/interpreter/gasometer.rs b/ethcore/evm/src/interpreter/gasometer.rs index 78a33ef3ead..f3230b04c52 100644 --- a/ethcore/evm/src/interpreter/gasometer.rs +++ b/ethcore/evm/src/interpreter/gasometer.rs @@ -124,13 +124,18 @@ impl Gasometer { let address = H256::from(stack.peek(0)); let newval = stack.peek(1); let val = U256::from(&*ext.storage_at(&address)?); + let orig = U256::from(&*ext.reverted_storage_at(&address)?); - let gas = if val.is_zero() && !newval.is_zero() { - schedule.sstore_set_gas + let gas = if schedule.eip1283 { + calculate_eip1283_sstore_gas(schedule, &orig, &val, &newval) } else { - // Refund for below case is added when actually executing sstore - // !is_zero(&val) && is_zero(newval) - schedule.sstore_reset_gas + if val.is_zero() && !newval.is_zero() { + schedule.sstore_set_gas + } else { + // Refund for below case is added when actually executing sstore + // !is_zero(&val) && is_zero(newval) + schedule.sstore_reset_gas + } }; Request::Gas(Gas::from(gas)) }, @@ -342,6 +347,60 @@ fn add_gas_usize(value: Gas, num: usize) -> (Gas, bool) { value.overflow_add(Gas::from(num)) } +#[inline] +fn calculate_eip1283_sstore_gas(schedule: &Schedule, original: &U256, current: &U256, new: &U256) -> Gas { + Gas::from( + if current == new { + schedule.sload_gas + } else { + if original == current { + if original.is_zero() { + schedule.sstore_set_gas + } else { + schedule.sstore_reset_gas + } + } else { + schedule.sload_gas + } + } + ) +} + +pub fn handle_eip1283_sstore_clears_refund(ext: &mut vm::Ext, original: &U256, current: &U256, new: &U256) { + let sstore_clears_schedule = U256::from(ext.schedule().sstore_refund_gas); + + if current == new { + // No refund + } else { + if original == current { + if original.is_zero() { + // No refund + } else { + if new.is_zero() { + ext.inc_sstore_refund(sstore_clears_schedule); + } + } + } else { + if !original.is_zero() { + if current.is_zero() { + ext.dec_sstore_refund(sstore_clears_schedule); + } else if new.is_zero() { + ext.inc_sstore_refund(sstore_clears_schedule); + } + } + if original == new { + if original.is_zero() { + let refund = U256::from(ext.schedule().sstore_set_gas - ext.schedule().sload_gas); + ext.inc_sstore_refund(refund); + } else { + let refund = U256::from(ext.schedule().sstore_reset_gas - ext.schedule().sload_gas); + ext.inc_sstore_refund(refund); + } + } + } + } +} + #[test] fn test_mem_gas_cost() { // given diff --git a/ethcore/evm/src/interpreter/mod.rs b/ethcore/evm/src/interpreter/mod.rs index 8865c2f09ab..d40d1181a9b 100644 --- a/ethcore/evm/src/interpreter/mod.rs +++ b/ethcore/evm/src/interpreter/mod.rs @@ -514,10 +514,15 @@ impl Interpreter { let val = stack.pop_back(); let current_val = U256::from(&*ext.storage_at(&address)?); + let original_val = U256::from(&*ext.reverted_storage_at(&address)?); // Increase refund for clear - if !self.is_zero(¤t_val) && self.is_zero(&val) { - let sstore_clears_schedule = U256::from(ext.schedule().sstore_refund_gas); - ext.inc_sstore_refund(sstore_clears_schedule); + if ext.schedule().eip1283 { + gasometer::handle_eip1283_sstore_clears_refund(ext, &original_val, ¤t_val, &val); + } else { + if !self.is_zero(¤t_val) && self.is_zero(&val) { + let sstore_clears_schedule = U256::from(ext.schedule().sstore_refund_gas); + ext.inc_sstore_refund(sstore_clears_schedule); + } } ext.set_storage(address, H256::from(&val))?; }, From 5f2e25c3b5de7791779511e5427606942af50cf9 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 9 Aug 2018 02:32:03 +0800 Subject: [PATCH 05/40] Add eip1283Transition params --- ethcore/src/spec/spec.rs | 7 +++++++ json/src/spec/params.rs | 3 +++ 2 files changed, 10 insertions(+) diff --git a/ethcore/src/spec/spec.rs b/ethcore/src/spec/spec.rs index e842835f7c8..645c8f297e8 100644 --- a/ethcore/src/spec/spec.rs +++ b/ethcore/src/spec/spec.rs @@ -117,6 +117,8 @@ pub struct CommonParams { pub eip145_transition: BlockNumber, /// Number of first block where EIP-1052 rules begin. pub eip1052_transition: BlockNumber, + /// Number of first block where EIP-1283 rules begin. + pub eip1283_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. @@ -181,6 +183,7 @@ impl CommonParams { 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; + schedule.eip1283 = block_number >= self.eip1283_transition; if block_number >= self.eip210_transition { schedule.blockhash_gas = 800; } @@ -285,6 +288,10 @@ impl From for CommonParams { BlockNumber::max_value, Into::into, ), + eip1283_transition: p.eip1283_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/json/src/spec/params.rs b/json/src/spec/params.rs index cf57e9af456..35d2e8e6760 100644 --- a/json/src/spec/params.rs +++ b/json/src/spec/params.rs @@ -112,6 +112,9 @@ pub struct Params { #[serde(rename="eip1052Transition")] pub eip1052_transition: Option, /// See `CommonParams` docs. + #[serde(rename="eip1283Transition")] + pub eip1283_transition: Option, + /// See `CommonParams` docs. #[serde(rename="dustProtectionTransition")] pub dust_protection_transition: Option, /// See `CommonParams` docs. From 9c56484bd717b159c1feba3a6133ee1e3d3b6ed8 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 9 Aug 2018 02:56:29 +0800 Subject: [PATCH 06/40] evm: fix tests --- ethcore/evm/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethcore/evm/src/tests.rs b/ethcore/evm/src/tests.rs index b8f0df3639d..62993ccf9d1 100644 --- a/ethcore/evm/src/tests.rs +++ b/ethcore/evm/src/tests.rs @@ -716,7 +716,7 @@ fn test_jumps(factory: super::Factory) { test_finalize(vm.exec(params, &mut ext)).unwrap() }; - assert_eq!(ext.sstore_clears, 1); + assert_eq!(ext.sstore_clears, U256::from(15_000)); assert_store(&ext, 0, "0000000000000000000000000000000000000000000000000000000000000000"); // 5! assert_store(&ext, 1, "0000000000000000000000000000000000000000000000000000000000000078"); // 5! assert_eq!(gas_left, U256::from(54_117)); From 4861e3597f13af49611255f2f4169fea134a2e1b Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 9 Aug 2018 03:18:22 +0800 Subject: [PATCH 07/40] jsontests: fix test --- ethcore/src/json_tests/executive.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/ethcore/src/json_tests/executive.rs b/ethcore/src/json_tests/executive.rs index a8fd4b45371..20f0f0e8251 100644 --- a/ethcore/src/json_tests/executive.rs +++ b/ethcore/src/json_tests/executive.rs @@ -112,6 +112,10 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for TestExt<'a, T, V, B> self.ext.storage_at(key) } + fn reverted_storage_at(&self, key: &H256) -> vm::Result { + self.ext.reverted_storage_at(key) + } + fn set_storage(&mut self, key: H256, value: H256) -> vm::Result<()> { self.ext.set_storage(key, value) } @@ -206,8 +210,12 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for TestExt<'a, T, V, B> false } - fn inc_sstore_clears(&mut self) { - self.ext.inc_sstore_clears() + fn inc_sstore_refund(&mut self, value: U256) { + self.ext.inc_sstore_refund(value) + } + + fn dec_sstore_refund(&mut self, value: U256) { + self.ext.dec_sstore_refund(value) } } From 11490b5727d673c9fd80592e98a55e200c2d73e8 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 13 Aug 2018 16:11:58 +0800 Subject: [PATCH 08/40] Return checkpoint index when creating --- ethcore/src/state/mod.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index ad0cbfe3798..c352d93b7f8 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -401,9 +401,12 @@ impl State { self.factories.vm.clone() } - /// Create a recoverable checkpoint of this state. - pub fn checkpoint(&mut self) { - self.checkpoints.get_mut().push(HashMap::new()); + /// Create a recoverable checkpoint of this state. Return the checkpoint index. + pub fn checkpoint(&mut self) -> usize { + let checkpoints = self.checkpoints.get_mut(); + let index = checkpoints.len(); + checkpoints.push(HashMap::new()); + index } /// Merge last checkpoint with previous. From 399abe8f93696b37fad0bab50922a5cff6d30c44 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 13 Aug 2018 17:05:08 +0800 Subject: [PATCH 09/40] Comply with spec Version II --- ethcore/src/executive.rs | 21 ++++++++++++++------- ethcore/src/externalities.rs | 25 ++++++++++++++----------- ethcore/src/state/mod.rs | 22 +++++++++++----------- 3 files changed, 39 insertions(+), 29 deletions(-) diff --git a/ethcore/src/executive.rs b/ethcore/src/executive.rs index fd3e2282dfc..5076f43c698 100644 --- a/ethcore/src/executive.rs +++ b/ethcore/src/executive.rs @@ -172,6 +172,7 @@ pub struct Executive<'a, B: 'a> { schedule: &'a Schedule, depth: usize, static_flag: bool, + transaction_checkpoint_index: Option, } impl<'a, B: 'a + StateBackend> Executive<'a, B> { @@ -184,11 +185,12 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { schedule: schedule, depth: 0, static_flag: false, + transaction_checkpoint_index: None, } } /// Populates executive from parent properties. Increments executive depth. - pub fn from_parent(state: &'a mut State, info: &'a EnvInfo, machine: &'a Machine, schedule: &'a Schedule, parent_depth: usize, static_flag: bool) -> Self { + pub fn from_parent(state: &'a mut State, info: &'a EnvInfo, machine: &'a Machine, schedule: &'a Schedule, parent_depth: usize, static_flag: bool, transaction_checkpoint_index: usize) -> Self { Executive { state: state, info: info, @@ -196,6 +198,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { schedule: schedule, depth: parent_depth + 1, static_flag: static_flag, + transaction_checkpoint_index: Some(transaction_checkpoint_index), } } @@ -208,9 +211,11 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { tracer: &'any mut T, vm_tracer: &'any mut V, static_call: bool, + current_checkpoint_index: usize, ) -> Externalities<'any, T, V, B> where T: Tracer, V: VMTracer { let is_static = self.static_flag || static_call; - Externalities::new(self.state, self.info, self.machine, self.schedule, self.depth, origin_info, substate, output, tracer, vm_tracer, is_static) + let transaction_checkpoint_index = self.transaction_checkpoint_index.unwrap_or(current_checkpoint_index); + Externalities::new(self.state, self.info, self.machine, self.schedule, self.depth, origin_info, substate, output, tracer, vm_tracer, is_static, transaction_checkpoint_index) } /// This function should be used to execute transaction. @@ -344,6 +349,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { params: ActionParams, unconfirmed_substate: &mut Substate, output_policy: OutputPolicy, + current_checkpoint_index: usize, tracer: &mut T, vm_tracer: &mut V ) -> vm::Result where T: Tracer, V: VMTracer { @@ -356,7 +362,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { let vm_factory = self.state.vm_factory(); let wasm = self.schedule.wasm.is_some(); trace!(target: "executive", "ext.schedule.have_delegate_call: {}", self.schedule.have_delegate_call); - let mut ext = self.as_externalities(OriginInfo::from(¶ms), unconfirmed_substate, output_policy, tracer, vm_tracer, static_call); + let mut ext = self.as_externalities(OriginInfo::from(¶ms), unconfirmed_substate, output_policy, tracer, vm_tracer, static_call, current_checkpoint_index); let mut vm = vm_factory.create(¶ms, wasm); return vm.exec(params, &mut ext).finalize(ext); } @@ -366,7 +372,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { let vm_factory = self.state.vm_factory(); let max_depth = self.schedule.max_depth; let wasm = self.schedule.wasm.is_some(); - let mut ext = self.as_externalities(OriginInfo::from(¶ms), unconfirmed_substate, output_policy, tracer, vm_tracer, static_call); + let mut ext = self.as_externalities(OriginInfo::from(¶ms), unconfirmed_substate, output_policy, tracer, vm_tracer, static_call, current_checkpoint_index); scope.builder().stack_size(::std::cmp::max(max_depth.saturating_sub(depth_threshold) * STACK_SIZE_PER_DEPTH, local_stack_size)).spawn(move || { let mut vm = vm_factory.create(¶ms, wasm); @@ -397,7 +403,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { } // backup used in case of running out of gas - self.state.checkpoint(); + let current_checkpoint_index = self.state.checkpoint(); let schedule = self.schedule; @@ -486,7 +492,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { let mut subvmtracer = vm_tracer.prepare_subtrace(params.code.as_ref().expect("scope is conditional on params.code.is_some(); qed")); let res = { - self.exec_vm(params, &mut unconfirmed_substate, OutputPolicy::Return(output, trace_output.as_mut()), &mut subtracer, &mut subvmtracer) + self.exec_vm(params, &mut unconfirmed_substate, OutputPolicy::Return(output, trace_output.as_mut()), current_checkpoint_index, &mut subtracer, &mut subvmtracer) }; vm_tracer.done_subtrace(subvmtracer); @@ -553,7 +559,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { } // backup used in case of running out of gas - self.state.checkpoint(); + let current_checkpoint_index = self.state.checkpoint(); // part of substate that may be reverted let mut unconfirmed_substate = Substate::new(); @@ -581,6 +587,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { params, &mut unconfirmed_substate, OutputPolicy::InitContract(output.as_mut().or(trace_output.as_mut())), + current_checkpoint_index, &mut subtracer, &mut subvmtracer ); diff --git a/ethcore/src/externalities.rs b/ethcore/src/externalities.rs index 9d35de46b6b..14151d593ca 100644 --- a/ethcore/src/externalities.rs +++ b/ethcore/src/externalities.rs @@ -75,6 +75,7 @@ pub struct Externalities<'a, T: 'a, V: 'a, B: 'a> { tracer: &'a mut T, vm_tracer: &'a mut V, static_flag: bool, + transaction_checkpoint_index: usize, } impl<'a, T: 'a, V: 'a, B: 'a> Externalities<'a, T, V, B> @@ -93,6 +94,7 @@ impl<'a, T: 'a, V: 'a, B: 'a> Externalities<'a, T, V, B> tracer: &'a mut T, vm_tracer: &'a mut V, static_flag: bool, + transaction_checkpoint_index: usize, ) -> Self { Externalities { state: state, @@ -106,6 +108,7 @@ impl<'a, T: 'a, V: 'a, B: 'a> Externalities<'a, T, V, B> tracer: tracer, vm_tracer: vm_tracer, static_flag: static_flag, + transaction_checkpoint_index: transaction_checkpoint_index, } } } @@ -114,7 +117,7 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B> where T: Tracer, V: VMTracer, B: StateBackend { fn reverted_storage_at(&self, key: &H256) -> vm::Result { - self.state.last_checkpoint_storage_at(&self.origin_info.address, key).map_err(Into::into) + self.state.checkpoint_storage_at(self.transaction_checkpoint_index, &self.origin_info.address, key).map(|v| v.unwrap_or(H256::zero())).map_err(Into::into) } fn storage_at(&self, key: &H256) -> vm::Result { @@ -232,7 +235,7 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B> } } } - let mut ex = Executive::from_parent(self.state, self.env_info, self.machine, self.schedule, self.depth, self.static_flag); + let mut ex = Executive::from_parent(self.state, self.env_info, self.machine, self.schedule, self.depth, self.static_flag, self.transaction_checkpoint_index); // TODO: handle internal error separately match ex.create(params, self.substate, &mut None, self.tracer, self.vm_tracer) { @@ -286,7 +289,7 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B> params.value = ActionValue::Transfer(value); } - let mut ex = Executive::from_parent(self.state, self.env_info, self.machine, self.schedule, self.depth, self.static_flag); + let mut ex = Executive::from_parent(self.state, self.env_info, self.machine, self.schedule, self.depth, self.static_flag, self.transaction_checkpoint_index); match ex.call(params, self.substate, BytesRef::Fixed(output), self.tracer, self.vm_tracer) { Ok(FinalizationResult{ gas_left, return_data, apply_state: true }) => MessageCallResult::Success(gas_left, return_data), @@ -487,7 +490,7 @@ mod tests { let mut tracer = NoopTracer; let mut vm_tracer = NoopVMTracer; - let ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract(None), &mut tracer, &mut vm_tracer, false); + let ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract(None), &mut tracer, &mut vm_tracer, false, 0); assert_eq!(ext.env_info().number, 100); } @@ -499,7 +502,7 @@ mod tests { let mut tracer = NoopTracer; let mut vm_tracer = NoopVMTracer; - let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract(None), &mut tracer, &mut vm_tracer, false); + let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract(None), &mut tracer, &mut vm_tracer, false, 0); let hash = ext.blockhash(&"0000000000000000000000000000000000000000000000000000000000120000".parse::().unwrap()); @@ -523,7 +526,7 @@ mod tests { let mut tracer = NoopTracer; let mut vm_tracer = NoopVMTracer; - let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract(None), &mut tracer, &mut vm_tracer, false); + let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract(None), &mut tracer, &mut vm_tracer, false, 0); let hash = ext.blockhash(&"0000000000000000000000000000000000000000000000000000000000120000".parse::().unwrap()); @@ -538,7 +541,7 @@ mod tests { let mut tracer = NoopTracer; let mut vm_tracer = NoopVMTracer; - let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract(None), &mut tracer, &mut vm_tracer, false); + let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract(None), &mut tracer, &mut vm_tracer, false, 0); let mut output = vec![]; @@ -566,7 +569,7 @@ mod tests { let mut vm_tracer = NoopVMTracer; { - let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract(None), &mut tracer, &mut vm_tracer, false); + let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract(None), &mut tracer, &mut vm_tracer, false, 0); ext.log(log_topics, &log_data).unwrap(); } @@ -583,7 +586,7 @@ mod tests { let mut vm_tracer = NoopVMTracer; { - let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract(None), &mut tracer, &mut vm_tracer, false); + let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract(None), &mut tracer, &mut vm_tracer, false, 0); ext.suicide(refund_account).unwrap(); } @@ -600,7 +603,7 @@ mod tests { let mut vm_tracer = NoopVMTracer; let address = { - let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract(None), &mut tracer, &mut vm_tracer, false); + let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract(None), &mut tracer, &mut vm_tracer, false, 0); match ext.create(&U256::max_value(), &U256::zero(), &[], CreateContractAddress::FromSenderAndNonce) { ContractCreateResult::Created(address, _) => address, _ => panic!("Test create failed; expected Created, got Failed/Reverted."), @@ -620,7 +623,7 @@ mod tests { let mut vm_tracer = NoopVMTracer; let address = { - let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract(None), &mut tracer, &mut vm_tracer, false); + let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract(None), &mut tracer, &mut vm_tracer, false, 0); match ext.create(&U256::max_value(), &U256::zero(), &[], CreateContractAddress::FromSenderSaltAndCodeHash(H256::default())) { ContractCreateResult::Created(address, _) => address, diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index c352d93b7f8..bc5dc6bc3c1 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -543,39 +543,39 @@ impl State { } /// Get the value of storage at last checkpoint. - pub fn last_checkpoint_storage_at(&self, address: &Address, key: &H256) -> TrieResult { - if let Some(ref checkpoint) = self.checkpoints.borrow().last() { - if let Some(entry) = checkpoint.get(address) { + pub fn checkpoint_storage_at(&self, checkpoint_index: usize, address: &Address, key: &H256) -> TrieResult> { + if let Some(ref checkpoint) = self.checkpoints.borrow().get(checkpoint_index) { + Ok(Some(if let Some(entry) = checkpoint.get(address) { match entry { Some(entry) => { match entry.account { Some(ref account) => { if let Some(value) = account.cached_storage_at(key) { - Ok(value) + value } else { // This key has checkpoint entry, but not in the entry's cache. // So it's not modified at all. - self.original_storage_at(address, key) + self.original_storage_at(address, key)? } }, None => { // The account didn't exist at that point. Return empty value. - Ok(H256::new()) + H256::new() }, } }, None => { // The value was not cached at that checkpoint, meaning it was not modified. - self.original_storage_at(address, key) + self.original_storage_at(address, key)? }, } } else { // This key does not have a checkpoint entry. We use the current value. - self.storage_at(address, key) - } + self.storage_at(address, key)? + })) } else { - // No checkpoint has been found. We use the current value. - self.storage_at(address, key) + // The checkpoint was not found. Return None. + Ok(None) } } From f57853cd402510e8eca9c2536b0e7220e4ae89ea Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 13 Aug 2018 17:07:26 +0800 Subject: [PATCH 10/40] Fix docs --- ethcore/vm/src/ext.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethcore/vm/src/ext.rs b/ethcore/vm/src/ext.rs index 44f1a802125..9aada06bdd8 100644 --- a/ethcore/vm/src/ext.rs +++ b/ethcore/vm/src/ext.rs @@ -63,7 +63,7 @@ pub enum CreateContractAddress { /// Externalities interface for EVMs pub trait Ext { - /// Returns the storage value for a given key if reversion happens. + /// Returns the storage value for a given key if reversion happens on the current transaction. fn reverted_storage_at(&self, key: &H256) -> Result; /// Returns a value for given key. From e4a6668284b6acac72e86c5feacf09aaff6f8463 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 14 Aug 2018 15:53:14 +0800 Subject: [PATCH 11/40] Fix jsontests feature compile --- ethcore/src/json_tests/executive.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ethcore/src/json_tests/executive.rs b/ethcore/src/json_tests/executive.rs index fc29a9c9a60..ef30f5c6edb 100644 --- a/ethcore/src/json_tests/executive.rs +++ b/ethcore/src/json_tests/executive.rs @@ -98,7 +98,7 @@ impl<'a, T: 'a, V: 'a, B: 'a> TestExt<'a, T, V, B> let static_call = false; Ok(TestExt { nonce: state.nonce(&address)?, - ext: Externalities::new(state, info, machine, schedule, depth, origin_info, substate, output, tracer, vm_tracer, static_call), + ext: Externalities::new(state, info, machine, schedule, depth, origin_info, substate, output, tracer, vm_tracer, static_call, 0), callcreates: vec![], sender: address, }) @@ -112,8 +112,8 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for TestExt<'a, T, V, B> self.ext.storage_at(key) } - fn reverted_storage_at(&self, key: &H256) -> vm::Result { - self.ext.reverted_storage_at(key) + fn reverted_storage_at(&self, _key: &H256) -> vm::Result { + Ok(H256::zero()) } fn set_storage(&mut self, key: H256, value: H256) -> vm::Result<()> { From e2b5899e00a97d1ea702afd30cbbf4546856d5c0 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 28 Aug 2018 17:44:12 +0800 Subject: [PATCH 12/40] Address grumbles --- ethcore/evm/src/interpreter/gasometer.rs | 4 ++++ ethcore/evm/src/tests.rs | 2 +- ethcore/src/state/mod.rs | 10 +++++++--- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/ethcore/evm/src/interpreter/gasometer.rs b/ethcore/evm/src/interpreter/gasometer.rs index f3230b04c52..8f607e38d4e 100644 --- a/ethcore/evm/src/interpreter/gasometer.rs +++ b/ethcore/evm/src/interpreter/gasometer.rs @@ -381,6 +381,7 @@ pub fn handle_eip1283_sstore_clears_refund(ext: &mut vm::Ext, original: &U256, c } } } else { + // Handle invalidated and newly validated R_RESET refunds. if !original.is_zero() { if current.is_zero() { ext.dec_sstore_refund(sstore_clears_schedule); @@ -388,11 +389,14 @@ pub fn handle_eip1283_sstore_clears_refund(ext: &mut vm::Ext, original: &U256, c ext.inc_sstore_refund(sstore_clears_schedule); } } + // Reset storage slot to its original value. if original == new { if original.is_zero() { + // Revert sstore full cost (minus sload amount) let refund = U256::from(ext.schedule().sstore_set_gas - ext.schedule().sload_gas); ext.inc_sstore_refund(refund); } else { + // Revert sstore change cost (revert of refund done in previous conditions) let refund = U256::from(ext.schedule().sstore_reset_gas - ext.schedule().sload_gas); ext.inc_sstore_refund(refund); } diff --git a/ethcore/evm/src/tests.rs b/ethcore/evm/src/tests.rs index 3fa8b6f54c5..9fd7bcd9023 100644 --- a/ethcore/evm/src/tests.rs +++ b/ethcore/evm/src/tests.rs @@ -716,7 +716,7 @@ fn test_jumps(factory: super::Factory) { test_finalize(vm.exec(&mut ext)).unwrap() }; - assert_eq!(ext.sstore_clears, U256::from(15_000)); + assert_eq!(ext.sstore_clears, U256::from(ext.schedule().sstore_refund_gas)); assert_store(&ext, 0, "0000000000000000000000000000000000000000000000000000000000000000"); // 5! assert_store(&ext, 1, "0000000000000000000000000000000000000000000000000000000000000078"); // 5! assert_eq!(gas_left, U256::from(54_117)); diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index bc5dc6bc3c1..e03a1b84185 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -542,7 +542,7 @@ impl State { |a| a.as_ref().and_then(|account| account.storage_root().cloned())) } - /// Get the value of storage at last checkpoint. + /// Get the value of storage at a specific checkpoint. pub fn checkpoint_storage_at(&self, checkpoint_index: usize, address: &Address, key: &H256) -> TrieResult> { if let Some(ref checkpoint) = self.checkpoints.borrow().get(checkpoint_index) { Ok(Some(if let Some(entry) = checkpoint.get(address) { @@ -570,8 +570,12 @@ impl State { }, } } else { - // This key does not have a checkpoint entry. We use the current value. - self.storage_at(address, key)? + // This key does not have a checkpoint entry. + if checkpoint_index == 0 { + self.original_storage_at(address, key)? + } else { + self.checkpoint_storage_at(checkpoint_index - 1, address, key)?.expect("checkpoint_index exists; checkpoint_index - 1 must exist; qed") + } })) } else { // The checkpoint was not found. Return None. From 00ae2581880ff0fa9346f685fca2322fd44213f9 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 28 Aug 2018 22:42:04 +0800 Subject: [PATCH 13/40] Fix no-checkpoint-entry case --- ethcore/src/state/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index e03a1b84185..342a8493f58 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -571,10 +571,10 @@ impl State { } } else { // This key does not have a checkpoint entry. - if checkpoint_index == 0 { - self.original_storage_at(address, key)? + if checkpoint_index >= self.checkpoints.len() - 1 { + self.storage_at(address, key)? } else { - self.checkpoint_storage_at(checkpoint_index - 1, address, key)?.expect("checkpoint_index exists; checkpoint_index - 1 must exist; qed") + self.checkpoint_storage_at(checkpoint_index + 1, address, key)?.expect("checkpoint_index exists; checkpoint_index - 1 must exist; qed") } })) } else { From fb99c745b59a50bb63ba8eb186b6e817ab122e40 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 28 Aug 2018 22:48:00 +0800 Subject: [PATCH 14/40] Remove unnecessary expect --- ethcore/src/state/mod.rs | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 342a8493f58..640dea3cc4f 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -544,39 +544,41 @@ impl State { /// Get the value of storage at a specific checkpoint. pub fn checkpoint_storage_at(&self, checkpoint_index: usize, address: &Address, key: &H256) -> TrieResult> { - if let Some(ref checkpoint) = self.checkpoints.borrow().get(checkpoint_index) { - Ok(Some(if let Some(entry) = checkpoint.get(address) { + let checkpoints = self.checkpoints.borrow(); + let checkpoints_len = checkpoints.len(); + if let Some(ref checkpoint) = checkpoints.get(checkpoint_index) { + if let Some(entry) = checkpoint.get(address) { match entry { Some(entry) => { match entry.account { Some(ref account) => { if let Some(value) = account.cached_storage_at(key) { - value + Ok(Some(value)) } else { // This key has checkpoint entry, but not in the entry's cache. // So it's not modified at all. - self.original_storage_at(address, key)? + Ok(Some(self.original_storage_at(address, key)?)) } }, None => { // The account didn't exist at that point. Return empty value. - H256::new() + Ok(Some(H256::new())) }, } }, None => { // The value was not cached at that checkpoint, meaning it was not modified. - self.original_storage_at(address, key)? + Ok(Some(self.original_storage_at(address, key)?)) }, } } else { // This key does not have a checkpoint entry. - if checkpoint_index >= self.checkpoints.len() - 1 { - self.storage_at(address, key)? + if checkpoint_index >= checkpoints_len - 1 { + Ok(Some(self.storage_at(address, key)?)) } else { - self.checkpoint_storage_at(checkpoint_index + 1, address, key)?.expect("checkpoint_index exists; checkpoint_index - 1 must exist; qed") + self.checkpoint_storage_at(checkpoint_index + 1, address, key) } - })) + } } else { // The checkpoint was not found. Return None. Ok(None) From 3149c7f5827661aa1a85a161d9a7aa2ee2be4c93 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 3 Sep 2018 14:25:32 +0800 Subject: [PATCH 15/40] Add test for State::checkpoint_storage_at --- ethcore/src/state/mod.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 640dea3cc4f..ff87e253469 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -2266,6 +2266,27 @@ mod tests { assert_eq!(state.balance(&a).unwrap(), U256::from(0)); } + #[test] + fn checkpoint_get_storage_at() { + let mut state = get_temp_state(); + let a = Address::zero(); + let k = H256::from(U256::from(0)); + let c1 = state.checkpoint(); + state.set_storage(&a, k, H256::from(U256::from(1))).unwrap(); + let c2 = state.checkpoint(); + let c3 = state.checkpoint(); + state.set_storage(&a, k, H256::from(U256::from(3))).unwrap(); + let c4 = state.checkpoint(); + state.set_storage(&a, k, H256::from(U256::from(4))).unwrap(); + let c5 = state.checkpoint(); + + assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + assert_eq!(state.checkpoint_storage_at(c2, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); + assert_eq!(state.checkpoint_storage_at(c3, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); + assert_eq!(state.checkpoint_storage_at(c4, &a, &k).unwrap(), Some(H256::from(U256::from(3)))); + assert_eq!(state.checkpoint_storage_at(c5, &a, &k).unwrap(), Some(H256::from(U256::from(4)))); + } + #[test] fn create_empty() { let mut state = get_temp_state(); From c07d12fa19252dbf2107f884f98e75d2654e2fda Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 3 Sep 2018 16:10:25 +0800 Subject: [PATCH 16/40] Add executive level test for eip1283 --- ethcore/res/ethereum/constantinople_test.json | 3 +- ethcore/src/executive.rs | 56 +++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/ethcore/res/ethereum/constantinople_test.json b/ethcore/res/ethereum/constantinople_test.json index 155b06507d0..9e7df1f7fba 100644 --- a/ethcore/res/ethereum/constantinople_test.json +++ b/ethcore/res/ethereum/constantinople_test.json @@ -33,7 +33,8 @@ "eip211Transition": "0x0", "eip214Transition": "0x0", "eip155Transition": "0x0", - "eip658Transition": "0x0" + "eip658Transition": "0x0", + "eip1283Transition": "0x0" }, "genesis": { "seal": { diff --git a/ethcore/src/executive.rs b/ethcore/src/executive.rs index c38f1fed8ce..fc50ffe680e 100644 --- a/ethcore/src/executive.rs +++ b/ethcore/src/executive.rs @@ -1620,6 +1620,62 @@ mod tests { assert_eq!(state.storage_at(&contract_address, &H256::from(&U256::zero())).unwrap(), H256::from(&U256::from(0))); } + evm_test!{test_eip1283: test_eip1283_int} + fn test_eip1283(factory: Factory) { + let x1 = Address::from(0x1000); + let x2 = Address::from(0x1001); + let y1 = Address::from(0x2001); + let y2 = Address::from(0x2002); + + let mut state = get_temp_state_with_factory(factory.clone()); + state.new_contract(&x1, U256::zero(), U256::from(1)); + state.init_code(&x1, "600160005560006000556001600055".from_hex().unwrap()).unwrap(); + state.new_contract(&x2, U256::zero(), U256::from(1)); + state.init_code(&x2, "600060005560016000556000600055".from_hex().unwrap()).unwrap(); + state.new_contract(&y1, U256::zero(), U256::from(1)); + state.init_code(&y1, "600060006000600061100062fffffff4".from_hex().unwrap()).unwrap(); + state.new_contract(&y2, U256::zero(), U256::from(1)); + state.init_code(&y2, "600060006000600061100162fffffff4".from_hex().unwrap()).unwrap(); + + let info = EnvInfo::default(); + let machine = ::ethereum::new_constantinople_test_machine(); + let schedule = machine.schedule(info.number); + + // Test a call via top-level -> y1 -> x1 + let (FinalizationResult { gas_left, .. }, refund, gas) = { + let gas = U256::from(0xffffffffffu64); + let mut params = ActionParams::default(); + params.code = Some(Arc::new("6001600055600060006000600061200163fffffffff4".from_hex().unwrap())); + params.gas = gas; + let mut substate = Substate::new(); + let mut ex = Executive::new(&mut state, &info, &machine, &schedule); + let res = ex.call(params, &mut substate, &mut NoopTracer, &mut NoopVMTracer).unwrap(); + + (res, substate.sstore_clears_refund, gas) + }; + let gas_used = gas - gas_left; + // sstore: 0 -> (1) -> () -> (1 -> 0 -> 1) + assert_eq!(gas_used, U256::from(41860)); + assert_eq!(refund, U256::from(19800)); + + // Test a call via top-level -> y2 -> x2 + let (FinalizationResult { gas_left, .. }, refund, gas) = { + let gas = U256::from(0xffffffffffu64); + let mut params = ActionParams::default(); + params.code = Some(Arc::new("6001600055600060006000600061200263fffffffff4".from_hex().unwrap())); + params.gas = gas; + let mut substate = Substate::new(); + let mut ex = Executive::new(&mut state, &info, &machine, &schedule); + let res = ex.call(params, &mut substate, &mut NoopTracer, &mut NoopVMTracer).unwrap(); + + (res, substate.sstore_clears_refund, gas) + }; + let gas_used = gas - gas_left; + // sstore: 1 -> (1) -> () -> (0 -> 1 -> 0) + assert_eq!(gas_used, U256::from(11860)); + assert_eq!(refund, U256::from(19800)); + } + fn wasm_sample_code() -> Arc> { Arc::new( "0061736d01000000010d0360027f7f0060017f0060000002270303656e7603726574000003656e760673656e646572000103656e76066d656d6f727902010110030201020404017000000501000708010463616c6c00020901000ac10101be0102057f017e4100410028020441c0006b22043602042004412c6a41106a220041003602002004412c6a41086a22014200370200200441186a41106a22024100360200200441186a41086a220342003703002004420037022c2004410036021c20044100360218200441186a1001200020022802002202360200200120032903002205370200200441106a2002360200200441086a200537030020042004290318220537022c200420053703002004411410004100200441c0006a3602040b0b0a010041040b0410c00000" From b1b7c57184de3fa4171a39ee68949af1df72b6d3 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 4 Sep 2018 16:58:52 +0800 Subject: [PATCH 17/40] Hard-code transaction_checkpoint_index to 0 --- ethcore/src/executive.rs | 21 +++++++-------------- ethcore/src/externalities.rs | 25 +++++++++++-------------- 2 files changed, 18 insertions(+), 28 deletions(-) diff --git a/ethcore/src/executive.rs b/ethcore/src/executive.rs index 387e011331d..3d4ff641a4e 100644 --- a/ethcore/src/executive.rs +++ b/ethcore/src/executive.rs @@ -173,7 +173,6 @@ pub struct Executive<'a, B: 'a> { schedule: &'a Schedule, depth: usize, static_flag: bool, - transaction_checkpoint_index: Option, } impl<'a, B: 'a + StateBackend> Executive<'a, B> { @@ -186,12 +185,11 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { schedule: schedule, depth: 0, static_flag: false, - transaction_checkpoint_index: None, } } /// Populates executive from parent properties. Increments executive depth. - pub fn from_parent(state: &'a mut State, info: &'a EnvInfo, machine: &'a Machine, schedule: &'a Schedule, parent_depth: usize, static_flag: bool, transaction_checkpoint_index: usize) -> Self { + pub fn from_parent(state: &'a mut State, info: &'a EnvInfo, machine: &'a Machine, schedule: &'a Schedule, parent_depth: usize, static_flag: bool) -> Self { Executive { state: state, info: info, @@ -199,7 +197,6 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { schedule: schedule, depth: parent_depth + 1, static_flag: static_flag, - transaction_checkpoint_index: Some(transaction_checkpoint_index), } } @@ -212,11 +209,9 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { tracer: &'any mut T, vm_tracer: &'any mut V, static_call: bool, - current_checkpoint_index: usize, ) -> Externalities<'any, T, V, B> where T: Tracer, V: VMTracer { let is_static = self.static_flag || static_call; - let transaction_checkpoint_index = self.transaction_checkpoint_index.unwrap_or(current_checkpoint_index); - Externalities::new(self.state, self.info, self.machine, self.schedule, self.depth, origin_info, substate, output, tracer, vm_tracer, is_static, transaction_checkpoint_index) + Externalities::new(self.state, self.info, self.machine, self.schedule, self.depth, origin_info, substate, output, tracer, vm_tracer, is_static) } /// This function should be used to execute transaction. @@ -358,7 +353,6 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { params: ActionParams, unconfirmed_substate: &mut Substate, output_policy: OutputPolicy, - current_checkpoint_index: usize, tracer: &mut T, vm_tracer: &mut V ) -> vm::Result where T: Tracer, V: VMTracer { @@ -372,7 +366,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { let origin_info = OriginInfo::from(¶ms); trace!(target: "executive", "ext.schedule.have_delegate_call: {}", self.schedule.have_delegate_call); let mut vm = vm_factory.create(params, self.schedule, self.depth); - let mut ext = self.as_externalities(origin_info, unconfirmed_substate, output_policy, tracer, vm_tracer, static_call, current_checkpoint_index); + let mut ext = self.as_externalities(origin_info, unconfirmed_substate, output_policy, tracer, vm_tracer, static_call); return vm.exec(&mut ext).finalize(ext); } @@ -383,7 +377,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { scope.builder().stack_size(::std::cmp::max(self.schedule.max_depth.saturating_sub(depth_threshold) * STACK_SIZE_PER_DEPTH, local_stack_size)).spawn(move || { let mut vm = vm_factory.create(params, self.schedule, self.depth); - let mut ext = self.as_externalities(origin_info, unconfirmed_substate, output_policy, tracer, vm_tracer, static_call, current_checkpoint_index); + let mut ext = self.as_externalities(origin_info, unconfirmed_substate, output_policy, tracer, vm_tracer, static_call); vm.exec(&mut ext).finalize(ext) }).expect("Sub-thread creation cannot fail; the host might run out of resources; qed") }).join() @@ -410,7 +404,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { } // backup used in case of running out of gas - let current_checkpoint_index = self.state.checkpoint(); + self.state.checkpoint(); let schedule = self.schedule; @@ -501,7 +495,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { let mut subvmtracer = vm_tracer.prepare_subtrace(params.code.as_ref().expect("scope is conditional on params.code.is_some(); qed")); let res = { - self.exec_vm(params, &mut unconfirmed_substate, OutputPolicy::Return, current_checkpoint_index, &mut subtracer, &mut subvmtracer) + self.exec_vm(params, &mut unconfirmed_substate, OutputPolicy::Return, &mut subtracer, &mut subvmtracer) }; vm_tracer.done_subtrace(subvmtracer); @@ -569,7 +563,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { } // backup used in case of running out of gas - let current_checkpoint_index = self.state.checkpoint(); + self.state.checkpoint(); // part of substate that may be reverted let mut unconfirmed_substate = Substate::new(); @@ -596,7 +590,6 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { params, &mut unconfirmed_substate, OutputPolicy::InitContract, - current_checkpoint_index, &mut subtracer, &mut subvmtracer ); diff --git a/ethcore/src/externalities.rs b/ethcore/src/externalities.rs index f7a79468c9d..e3cc28a72bc 100644 --- a/ethcore/src/externalities.rs +++ b/ethcore/src/externalities.rs @@ -75,7 +75,6 @@ pub struct Externalities<'a, T: 'a, V: 'a, B: 'a> { tracer: &'a mut T, vm_tracer: &'a mut V, static_flag: bool, - transaction_checkpoint_index: usize, } impl<'a, T: 'a, V: 'a, B: 'a> Externalities<'a, T, V, B> @@ -94,7 +93,6 @@ impl<'a, T: 'a, V: 'a, B: 'a> Externalities<'a, T, V, B> tracer: &'a mut T, vm_tracer: &'a mut V, static_flag: bool, - transaction_checkpoint_index: usize, ) -> Self { Externalities { state: state, @@ -108,7 +106,6 @@ impl<'a, T: 'a, V: 'a, B: 'a> Externalities<'a, T, V, B> tracer: tracer, vm_tracer: vm_tracer, static_flag: static_flag, - transaction_checkpoint_index: transaction_checkpoint_index, } } } @@ -117,7 +114,7 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B> where T: Tracer, V: VMTracer, B: StateBackend { fn reverted_storage_at(&self, key: &H256) -> vm::Result { - self.state.checkpoint_storage_at(self.transaction_checkpoint_index, &self.origin_info.address, key).map(|v| v.unwrap_or(H256::zero())).map_err(Into::into) + self.state.checkpoint_storage_at(0, &self.origin_info.address, key).map(|v| v.unwrap_or(H256::zero())).map_err(Into::into) } fn storage_at(&self, key: &H256) -> vm::Result { @@ -244,7 +241,7 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B> } } } - let mut ex = Executive::from_parent(self.state, self.env_info, self.machine, self.schedule, self.depth, self.static_flag, self.transaction_checkpoint_index); + let mut ex = Executive::from_parent(self.state, self.env_info, self.machine, self.schedule, self.depth, self.static_flag); // TODO: handle internal error separately match ex.create(params, self.substate, self.tracer, self.vm_tracer) { @@ -298,7 +295,7 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B> params.value = ActionValue::Transfer(value); } - let mut ex = Executive::from_parent(self.state, self.env_info, self.machine, self.schedule, self.depth, self.static_flag, self.transaction_checkpoint_index); + let mut ex = Executive::from_parent(self.state, self.env_info, self.machine, self.schedule, self.depth, self.static_flag); match ex.call(params, self.substate, self.tracer, self.vm_tracer) { Ok(FinalizationResult{ gas_left, return_data, apply_state: true }) => MessageCallResult::Success(gas_left, return_data), @@ -484,7 +481,7 @@ mod tests { let mut tracer = NoopTracer; let mut vm_tracer = NoopVMTracer; - let ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false, 0); + let ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false); assert_eq!(ext.env_info().number, 100); } @@ -496,7 +493,7 @@ mod tests { let mut tracer = NoopTracer; let mut vm_tracer = NoopVMTracer; - let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false, 0); + let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false); let hash = ext.blockhash(&"0000000000000000000000000000000000000000000000000000000000120000".parse::().unwrap()); @@ -520,7 +517,7 @@ mod tests { let mut tracer = NoopTracer; let mut vm_tracer = NoopVMTracer; - let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false, 0); + let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false); let hash = ext.blockhash(&"0000000000000000000000000000000000000000000000000000000000120000".parse::().unwrap()); @@ -535,7 +532,7 @@ mod tests { let mut tracer = NoopTracer; let mut vm_tracer = NoopVMTracer; - let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false, 0); + let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false); // this should panic because we have no balance on any account ext.call( @@ -560,7 +557,7 @@ mod tests { let mut vm_tracer = NoopVMTracer; { - let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false, 0); + let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false); ext.log(log_topics, &log_data).unwrap(); } @@ -577,7 +574,7 @@ mod tests { let mut vm_tracer = NoopVMTracer; { - let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false, 0); + let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false); ext.suicide(refund_account).unwrap(); } @@ -594,7 +591,7 @@ mod tests { let mut vm_tracer = NoopVMTracer; let address = { - let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false, 0); + let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false); match ext.create(&U256::max_value(), &U256::zero(), &[], CreateContractAddress::FromSenderAndNonce) { ContractCreateResult::Created(address, _) => address, _ => panic!("Test create failed; expected Created, got Failed/Reverted."), @@ -614,7 +611,7 @@ mod tests { let mut vm_tracer = NoopVMTracer; let address = { - let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false, 0); + let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false); match ext.create(&U256::max_value(), &U256::zero(), &[], CreateContractAddress::FromSenderSaltAndCodeHash(H256::default())) { ContractCreateResult::Created(address, _) => address, From b01c56654dec3b5529153808c8a738db49143c8a Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Tue, 4 Sep 2018 17:01:30 +0800 Subject: [PATCH 18/40] Fix jsontests --- ethcore/src/json_tests/executive.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ethcore/src/json_tests/executive.rs b/ethcore/src/json_tests/executive.rs index ef30f5c6edb..fc29a9c9a60 100644 --- a/ethcore/src/json_tests/executive.rs +++ b/ethcore/src/json_tests/executive.rs @@ -98,7 +98,7 @@ impl<'a, T: 'a, V: 'a, B: 'a> TestExt<'a, T, V, B> let static_call = false; Ok(TestExt { nonce: state.nonce(&address)?, - ext: Externalities::new(state, info, machine, schedule, depth, origin_info, substate, output, tracer, vm_tracer, static_call, 0), + ext: Externalities::new(state, info, machine, schedule, depth, origin_info, substate, output, tracer, vm_tracer, static_call), callcreates: vec![], sender: address, }) @@ -112,8 +112,8 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for TestExt<'a, T, V, B> self.ext.storage_at(key) } - fn reverted_storage_at(&self, _key: &H256) -> vm::Result { - Ok(H256::zero()) + fn reverted_storage_at(&self, key: &H256) -> vm::Result { + self.ext.reverted_storage_at(key) } fn set_storage(&mut self, key: H256, value: H256) -> vm::Result<()> { From c2b9e9741cb9d92c48eab2c200ba33f11de0290a Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Wed, 5 Sep 2018 20:17:28 +0800 Subject: [PATCH 19/40] Add tests for checkpoint discard/revert --- ethcore/src/state/mod.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index ff87e253469..30d09de4201 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -2271,10 +2271,12 @@ mod tests { let mut state = get_temp_state(); let a = Address::zero(); let k = H256::from(U256::from(0)); + let k2 = H256::from(U256::from(1)); let c1 = state.checkpoint(); state.set_storage(&a, k, H256::from(U256::from(1))).unwrap(); let c2 = state.checkpoint(); let c3 = state.checkpoint(); + state.set_storage(&a, k2, H256::from(U256::from(3))).unwrap(); state.set_storage(&a, k, H256::from(U256::from(3))).unwrap(); let c4 = state.checkpoint(); state.set_storage(&a, k, H256::from(U256::from(4))).unwrap(); @@ -2285,6 +2287,24 @@ mod tests { assert_eq!(state.checkpoint_storage_at(c3, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); assert_eq!(state.checkpoint_storage_at(c4, &a, &k).unwrap(), Some(H256::from(U256::from(3)))); assert_eq!(state.checkpoint_storage_at(c5, &a, &k).unwrap(), Some(H256::from(U256::from(4)))); + + state.discard_checkpoint(); // Commit/discard c5. + assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + assert_eq!(state.checkpoint_storage_at(c2, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); + assert_eq!(state.checkpoint_storage_at(c3, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); + assert_eq!(state.checkpoint_storage_at(c4, &a, &k).unwrap(), Some(H256::from(U256::from(3)))); + + state.revert_to_checkpoint(); // Revert to c4. + assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + assert_eq!(state.checkpoint_storage_at(c2, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); + assert_eq!(state.checkpoint_storage_at(c3, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); + + state.discard_checkpoint(); // Commit/discard c3. + assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + assert_eq!(state.checkpoint_storage_at(c2, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); + + state.revert_to_checkpoint(); // Revert to c2. + assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); } #[test] From 7f672c3db0518e4b43b18a813bb115244f191e41 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Wed, 5 Sep 2018 20:25:56 +0800 Subject: [PATCH 20/40] Require checkpoint to be empty for kill_account and commit --- ethcore/src/state/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 30d09de4201..4995309ce1d 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -503,6 +503,7 @@ impl State { /// Remove an existing account. pub fn kill_account(&mut self, account: &Address) { + assert!(self.checkpoints.borrow().is_empty()); self.insert_cache(account, AccountEntry::new_dirty(None)); } @@ -831,6 +832,7 @@ impl State { /// Commits our cached account changes into the trie. pub fn commit(&mut self) -> Result<(), Error> { + assert!(self.checkpoints.borrow().is_empty()); // first, commit the sub trees. let mut accounts = self.cache.borrow_mut(); for (address, ref mut a) in accounts.iter_mut().filter(|&(_, ref a)| a.is_dirty()) { From 6269c33c3bca4b4837d2a7bef6937debd997fb37 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Wed, 5 Sep 2018 20:39:08 +0800 Subject: [PATCH 21/40] Get code coverage --- .gitlab-ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 51ecec41a97..79c130b6913 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -85,6 +85,7 @@ test-coverage-kcov: stage: test only: - master + - pr-9319 script: - scripts/gitlab/coverage.sh tags: From 0e4ba15ac2711f986a445d00860a073c2a67127f Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Wed, 5 Sep 2018 23:11:43 +0800 Subject: [PATCH 22/40] Use saturating_add/saturating_sub --- ethcore/src/externalities.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ethcore/src/externalities.rs b/ethcore/src/externalities.rs index e3cc28a72bc..282f8a966a6 100644 --- a/ethcore/src/externalities.rs +++ b/ethcore/src/externalities.rs @@ -395,11 +395,11 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B> } fn inc_sstore_refund(&mut self, value: U256) { - self.substate.sstore_clears_refund = self.substate.sstore_clears_refund + value; + self.substate.sstore_clears_refund = self.substate.sstore_clears_refund.saturating_add(value); } fn dec_sstore_refund(&mut self, value: U256) { - self.substate.sstore_clears_refund = self.substate.sstore_clears_refund - value; + self.substate.sstore_clears_refund = self.substate.sstore_clears_refund.saturating_sub(value); } fn trace_next_instruction(&mut self, pc: usize, instruction: u8, current_gas: U256) -> bool { From 196f8317c99b1b7939a43fed3439022650d06b78 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 6 Sep 2018 18:27:33 +0800 Subject: [PATCH 23/40] Fix issues in insert_cache --- ethcore/src/state/mod.rs | 46 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 4995309ce1d..7a2a564d811 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -181,6 +181,8 @@ impl AccountEntry { Some(acc) => { if let Some(ref mut ours) = self.account { ours.overwrite_with(acc); + } else { + self.account = Some(acc); } }, None => self.account = None, @@ -419,7 +421,16 @@ impl State { **prev = checkpoint; } else { for (k, v) in checkpoint.drain() { - prev.entry(k).or_insert(v); + match prev.entry(k) { + Entry::Occupied(mut e) => { + if e.get().is_none() { + e.insert(v); + } + }, + Entry::Vacant(e) => { + e.insert(v); + } + } } } } @@ -503,7 +514,6 @@ impl State { /// Remove an existing account. pub fn kill_account(&mut self, account: &Address) { - assert!(self.checkpoints.borrow().is_empty()); self.insert_cache(account, AccountEntry::new_dirty(None)); } @@ -2274,6 +2284,15 @@ mod tests { let a = Address::zero(); let k = H256::from(U256::from(0)); let k2 = H256::from(U256::from(1)); + + state.set_storage(&a, k, H256::from(U256::from(0xffff))).unwrap(); + state.commit().unwrap(); + state.clear(); + + assert_eq!(state.storage_at(&a, &k).unwrap(), H256::from(U256::from(0xffff))); + + let c0 = state.checkpoint(); + state.new_contract(&a, U256::zero(), U256::zero()); let c1 = state.checkpoint(); state.set_storage(&a, k, H256::from(U256::from(1))).unwrap(); let c2 = state.checkpoint(); @@ -2284,6 +2303,7 @@ mod tests { state.set_storage(&a, k, H256::from(U256::from(4))).unwrap(); let c5 = state.checkpoint(); + assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0xffff)))); assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); assert_eq!(state.checkpoint_storage_at(c2, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); assert_eq!(state.checkpoint_storage_at(c3, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); @@ -2291,22 +2311,44 @@ mod tests { assert_eq!(state.checkpoint_storage_at(c5, &a, &k).unwrap(), Some(H256::from(U256::from(4)))); state.discard_checkpoint(); // Commit/discard c5. + assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0xffff)))); assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); assert_eq!(state.checkpoint_storage_at(c2, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); assert_eq!(state.checkpoint_storage_at(c3, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); assert_eq!(state.checkpoint_storage_at(c4, &a, &k).unwrap(), Some(H256::from(U256::from(3)))); state.revert_to_checkpoint(); // Revert to c4. + assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0xffff)))); assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); assert_eq!(state.checkpoint_storage_at(c2, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); assert_eq!(state.checkpoint_storage_at(c3, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); state.discard_checkpoint(); // Commit/discard c3. + assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0xffff)))); assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); assert_eq!(state.checkpoint_storage_at(c2, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); state.revert_to_checkpoint(); // Revert to c2. + assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0xffff)))); assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + + state.discard_checkpoint(); // Commit/discard c1. + assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0xffff)))); + } + + #[test] + fn kill_account_with_checkpoints() { + let mut state = get_temp_state(); + let a = Address::zero(); + let k = H256::from(U256::from(0)); + state.checkpoint(); + state.set_storage(&a, k, H256::from(U256::from(1))).unwrap(); + state.checkpoint(); + state.kill_account(&a); + + assert_eq!(state.storage_at(&a, &k).unwrap(), H256::from(U256::from(0))); + state.revert_to_checkpoint(); + assert_eq!(state.storage_at(&a, &k).unwrap(), H256::from(U256::from(1))); } #[test] From 14b89c2a43add67ce538135e3134848aefdb78cb Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 6 Sep 2018 18:32:14 +0800 Subject: [PATCH 24/40] Clear the state again --- ethcore/src/state/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 7a2a564d811..e212234b948 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -2290,6 +2290,7 @@ mod tests { state.clear(); assert_eq!(state.storage_at(&a, &k).unwrap(), H256::from(U256::from(0xffff))); + state.clear(); let c0 = state.checkpoint(); state.new_contract(&a, U256::zero(), U256::zero()); From f9af2c3016d81e3d21eb648fad2d2323856ec58c Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 6 Sep 2018 21:37:33 +0800 Subject: [PATCH 25/40] Fix original_storage_at --- ethcore/src/executive.rs | 12 ++-- ethcore/src/state/account.rs | 123 ++++++++++++++++++++++++++++------- ethcore/src/state/mod.rs | 45 +++++++++---- 3 files changed, 138 insertions(+), 42 deletions(-) diff --git a/ethcore/src/executive.rs b/ethcore/src/executive.rs index 3d4ff641a4e..113959611cc 100644 --- a/ethcore/src/executive.rs +++ b/ethcore/src/executive.rs @@ -574,9 +574,9 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { let prev_bal = self.state.balance(¶ms.address)?; if let ActionValue::Transfer(val) = params.value { self.state.sub_balance(¶ms.sender, &val, &mut substate.to_cleanup_mode(&schedule))?; - self.state.new_contract(¶ms.address, val + prev_bal, nonce_offset); + self.state.new_contract(¶ms.address, val + prev_bal, nonce_offset)?; } else { - self.state.new_contract(¶ms.address, prev_bal, nonce_offset); + self.state.new_contract(¶ms.address, prev_bal, nonce_offset)?; } let trace_info = tracer.prepare_trace_create(¶ms); @@ -1622,13 +1622,13 @@ mod tests { let y2 = Address::from(0x2002); let mut state = get_temp_state_with_factory(factory.clone()); - state.new_contract(&x1, U256::zero(), U256::from(1)); + state.new_contract(&x1, U256::zero(), U256::from(1)).unwrap(); state.init_code(&x1, "600160005560006000556001600055".from_hex().unwrap()).unwrap(); - state.new_contract(&x2, U256::zero(), U256::from(1)); + state.new_contract(&x2, U256::zero(), U256::from(1)).unwrap(); state.init_code(&x2, "600060005560016000556000600055".from_hex().unwrap()).unwrap(); - state.new_contract(&y1, U256::zero(), U256::from(1)); + state.new_contract(&y1, U256::zero(), U256::from(1)).unwrap(); state.init_code(&y1, "600060006000600061100062fffffff4".from_hex().unwrap()).unwrap(); - state.new_contract(&y2, U256::zero(), U256::from(1)); + state.new_contract(&y2, U256::zero(), U256::from(1)).unwrap(); state.init_code(&y2, "600060006000600061100162fffffff4".from_hex().unwrap()).unwrap(); let info = EnvInfo::default(); diff --git a/ethcore/src/state/account.rs b/ethcore/src/state/account.rs index 5a74b3945b4..7768adce410 100644 --- a/ethcore/src/state/account.rs +++ b/ethcore/src/state/account.rs @@ -59,6 +59,10 @@ pub struct Account { // LRU Cache of the trie-backed storage. // This is limited to `STORAGE_CACHE_ITEMS` recent queries storage_cache: RefCell>, + // LRU Cache of the trie-backed storage for original value. + // This is only used when the initial storage root is different compared to + // what is in the database. That is, it is only used for new contracts. + original_storage_cache: Option<(H256, RefCell>)>, // Modified storage. Accumulates changes to storage made in `set_storage` // Takes precedence over `storage_cache`. storage_changes: HashMap, @@ -81,6 +85,7 @@ impl From for Account { nonce: basic.nonce, storage_root: basic.storage_root, storage_cache: Self::empty_storage_cache(), + original_storage_cache: None, storage_changes: HashMap::new(), code_hash: basic.code_hash, code_size: None, @@ -100,6 +105,7 @@ impl Account { nonce: nonce, storage_root: KECCAK_NULL_RLP, storage_cache: Self::empty_storage_cache(), + original_storage_cache: None, storage_changes: storage, code_hash: keccak(&code), code_size: Some(code.len()), @@ -120,6 +126,7 @@ impl Account { nonce: pod.nonce, storage_root: KECCAK_NULL_RLP, storage_cache: Self::empty_storage_cache(), + original_storage_cache: None, storage_changes: pod.storage.into_iter().collect(), code_hash: pod.code.as_ref().map_or(KECCAK_EMPTY, |c| keccak(c)), code_filth: Filth::Dirty, @@ -136,6 +143,7 @@ impl Account { nonce: nonce, storage_root: KECCAK_NULL_RLP, storage_cache: Self::empty_storage_cache(), + original_storage_cache: None, storage_changes: HashMap::new(), code_hash: KECCAK_EMPTY, code_cache: Arc::new(vec![]), @@ -154,12 +162,17 @@ impl Account { /// Create a new contract account. /// NOTE: make sure you use `init_code` on this before `commit`ing. - pub fn new_contract(balance: U256, nonce: U256) -> Account { + pub fn new_contract(balance: U256, nonce: U256, original_storage_root: H256) -> Account { Account { balance: balance, nonce: nonce, storage_root: KECCAK_NULL_RLP, storage_cache: Self::empty_storage_cache(), + original_storage_cache: if original_storage_root == KECCAK_NULL_RLP { + None + } else { + Some((original_storage_root, Self::empty_storage_cache())) + }, storage_changes: HashMap::new(), code_hash: KECCAK_EMPTY, code_cache: Arc::new(vec![]), @@ -204,7 +217,11 @@ impl Account { if let Some(value) = self.cached_storage_at(key) { return Ok(value); } - self.get_and_cache_storage(db, key) + Self::get_and_cache_storage( + &self.storage_root, + &mut self.storage_cache.borrow_mut(), + db, + key) } /// Get (and cache) the contents of the trie's storage at `key`. @@ -213,15 +230,30 @@ impl Account { if let Some(value) = self.cached_original_storage_at(key) { return Ok(value); } - self.get_and_cache_storage(db, key) + match &self.original_storage_cache { + Some((ref original_storage_root, ref original_storage_cache)) => + Self::get_and_cache_storage( + original_storage_root, + &mut original_storage_cache.borrow_mut(), + db, + key + ), + None => + Self::get_and_cache_storage( + &self.storage_root, + &mut self.storage_cache.borrow_mut(), + db, + key + ), + } } - fn get_and_cache_storage(&self, db: &HashDB, key: &H256) -> TrieResult { - let db = SecTrieDB::new(db, &self.storage_root)?; + fn get_and_cache_storage(storage_root: &H256, storage_cache: &mut LruCache, db: &HashDB, key: &H256) -> TrieResult { + let db = SecTrieDB::new(db, storage_root)?; let panicky_decoder = |bytes:&[u8]| ::rlp::decode(&bytes).expect("decoding db value failed"); let item: U256 = db.get_with(key, panicky_decoder)?.unwrap_or_else(U256::zero); let value: H256 = item.into(); - self.storage_cache.borrow_mut().insert(key.clone(), value.clone()); + storage_cache.insert(key.clone(), value.clone()); Ok(value) } @@ -231,15 +263,32 @@ impl Account { if let Some(value) = self.storage_changes.get(key) { return Some(value.clone()) } - self.cached_original_storage_at(key) + self.cached_moved_original_storage_at(key) } /// Get cached original storage value after last state commitment. Returns `None` if the key is not in the cache. pub fn cached_original_storage_at(&self, key: &H256) -> Option { + match &self.original_storage_cache { + Some((_, ref original_storage_cache)) => { + if let Some(value) = original_storage_cache.borrow_mut().get_mut(key) { + Some(value.clone()) + } else { + None + } + }, + None => { + self.cached_moved_original_storage_at(key) + }, + } + } + + /// Get cached original storage value since last contract creation on this address. Returns `None` if the key is not in the cache. + fn cached_moved_original_storage_at(&self, key: &H256) -> Option { if let Some(value) = self.storage_cache.borrow_mut().get_mut(key) { - return Some(value.clone()) + Some(value.clone()) + } else { + None } - None } /// return the balance associated with this account. @@ -374,7 +423,27 @@ impl Account { } /// Return the storage root associated with this account or None if it has been altered via the overlay. - pub fn storage_root(&self) -> Option<&H256> { if self.storage_is_clean() {Some(&self.storage_root)} else {None} } + pub fn storage_root(&self) -> Option { + if self.storage_is_clean() { + Some(self.storage_root) + } else { + None + } + } + + /// Return the original storage root of this account. + pub fn original_storage_root(&self) -> H256 { + if let Some((original_storage_root, _)) = self.original_storage_cache { + original_storage_root + } else { + self.storage_root + } + } + + /// Storage root where the account changes are based upon. + pub fn base_storage_root(&self) -> H256 { + self.storage_root + } /// Return the storage overlay. pub fn storage_changes(&self) -> &HashMap { &self.storage_changes } @@ -409,6 +478,7 @@ impl Account { self.storage_cache.borrow_mut().insert(k, v); } + self.original_storage_cache = None; Ok(()) } @@ -446,6 +516,7 @@ impl Account { nonce: self.nonce.clone(), storage_root: self.storage_root.clone(), storage_cache: Self::empty_storage_cache(), + original_storage_cache: self.original_storage_cache.as_ref().map(|(r, _)| (*r, Self::empty_storage_cache())), storage_changes: HashMap::new(), code_hash: self.code_hash.clone(), code_size: self.code_size.clone(), @@ -466,6 +537,7 @@ impl Account { pub fn clone_all(&self) -> Account { let mut account = self.clone_dirty(); account.storage_cache = self.storage_cache.clone(); + account.original_storage_cache = self.original_storage_cache.clone(); account } @@ -481,10 +553,15 @@ impl Account { self.code_cache = other.code_cache; self.code_size = other.code_size; self.address_hash = other.address_hash; - let mut cache = self.storage_cache.borrow_mut(); - for (k, v) in other.storage_cache.into_inner() { - cache.insert(k, v); + if self.storage_root == other.storage_root { + let mut cache = self.storage_cache.borrow_mut(); + for (k, v) in other.storage_cache.into_inner() { + cache.insert(k, v); + } + } else { + self.storage_cache = other.storage_cache; } + self.original_storage_cache = other.original_storage_cache; self.storage_changes = other.storage_changes; } } @@ -543,7 +620,7 @@ mod tests { let mut db = MemoryDB::new(); let mut db = AccountDBMut::new(&mut db, &Address::new()); let rlp = { - let mut a = Account::new_contract(69.into(), 0.into()); + let mut a = Account::new_contract(69.into(), 0.into(), KECCAK_NULL_RLP); a.set_storage(0x00u64.into(), 0x1234u64.into()); a.commit_storage(&Default::default(), &mut db).unwrap(); a.init_code(vec![]); @@ -552,7 +629,7 @@ mod tests { }; let a = Account::from_rlp(&rlp).expect("decoding db value failed"); - assert_eq!(*a.storage_root().unwrap(), "c57e1afb758b07f8d2c8f13a3b6e44fa5ff94ab266facc5a4fd3f062426e50b2".into()); + assert_eq!(a.storage_root().unwrap(), "c57e1afb758b07f8d2c8f13a3b6e44fa5ff94ab266facc5a4fd3f062426e50b2".into()); assert_eq!(a.storage_at(&db.immutable(), &0x00u64.into()).unwrap(), 0x1234u64.into()); assert_eq!(a.storage_at(&db.immutable(), &0x01u64.into()).unwrap(), H256::default()); } @@ -563,7 +640,7 @@ mod tests { let mut db = AccountDBMut::new(&mut db, &Address::new()); let rlp = { - let mut a = Account::new_contract(69.into(), 0.into()); + let mut a = Account::new_contract(69.into(), 0.into(), KECCAK_NULL_RLP); a.init_code(vec![0x55, 0x44, 0xffu8]); a.commit_code(&mut db); a.rlp() @@ -578,18 +655,18 @@ mod tests { #[test] fn commit_storage() { - let mut a = Account::new_contract(69.into(), 0.into()); + let mut a = Account::new_contract(69.into(), 0.into(), KECCAK_NULL_RLP); let mut db = MemoryDB::new(); let mut db = AccountDBMut::new(&mut db, &Address::new()); a.set_storage(0.into(), 0x1234.into()); assert_eq!(a.storage_root(), None); a.commit_storage(&Default::default(), &mut db).unwrap(); - assert_eq!(*a.storage_root().unwrap(), "c57e1afb758b07f8d2c8f13a3b6e44fa5ff94ab266facc5a4fd3f062426e50b2".into()); + assert_eq!(a.storage_root().unwrap(), "c57e1afb758b07f8d2c8f13a3b6e44fa5ff94ab266facc5a4fd3f062426e50b2".into()); } #[test] fn commit_remove_commit_storage() { - let mut a = Account::new_contract(69.into(), 0.into()); + let mut a = Account::new_contract(69.into(), 0.into(), KECCAK_NULL_RLP); let mut db = MemoryDB::new(); let mut db = AccountDBMut::new(&mut db, &Address::new()); a.set_storage(0.into(), 0x1234.into()); @@ -598,12 +675,12 @@ mod tests { a.commit_storage(&Default::default(), &mut db).unwrap(); a.set_storage(1.into(), 0.into()); a.commit_storage(&Default::default(), &mut db).unwrap(); - assert_eq!(*a.storage_root().unwrap(), "c57e1afb758b07f8d2c8f13a3b6e44fa5ff94ab266facc5a4fd3f062426e50b2".into()); + assert_eq!(a.storage_root().unwrap(), "c57e1afb758b07f8d2c8f13a3b6e44fa5ff94ab266facc5a4fd3f062426e50b2".into()); } #[test] fn commit_code() { - let mut a = Account::new_contract(69.into(), 0.into()); + let mut a = Account::new_contract(69.into(), 0.into(), KECCAK_NULL_RLP); let mut db = MemoryDB::new(); let mut db = AccountDBMut::new(&mut db, &Address::new()); a.init_code(vec![0x55, 0x44, 0xffu8]); @@ -615,7 +692,7 @@ mod tests { #[test] fn reset_code() { - let mut a = Account::new_contract(69.into(), 0.into()); + let mut a = Account::new_contract(69.into(), 0.into(), KECCAK_NULL_RLP); let mut db = MemoryDB::new(); let mut db = AccountDBMut::new(&mut db, &Address::new()); a.init_code(vec![0x55, 0x44, 0xffu8]); @@ -646,7 +723,7 @@ mod tests { assert_eq!(*a.balance(), 69u8.into()); assert_eq!(*a.nonce(), 0u8.into()); assert_eq!(a.code_hash(), KECCAK_EMPTY); - assert_eq!(a.storage_root().unwrap(), &KECCAK_NULL_RLP); + assert_eq!(a.storage_root().unwrap(), KECCAK_NULL_RLP); } #[test] diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index e212234b948..321d5bb5cc4 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -508,8 +508,10 @@ impl State { /// Create a new contract at address `contract`. If there is already an account at the address /// it will have its code reset, ready for `init_code()`. - pub fn new_contract(&mut self, contract: &Address, balance: U256, nonce_offset: U256) { - self.insert_cache(contract, AccountEntry::new_dirty(Some(Account::new_contract(balance, self.account_start_nonce + nonce_offset)))); + pub fn new_contract(&mut self, contract: &Address, balance: U256, nonce_offset: U256) -> TrieResult<()> { + let original_storage_root = self.original_storage_root(contract)?; + self.insert_cache(contract, AccountEntry::new_dirty(Some(Account::new_contract(balance, self.account_start_nonce + nonce_offset, original_storage_root)))); + Ok(()) } /// Remove an existing account. @@ -550,7 +552,14 @@ impl State { /// Get the storage root of account `a`. pub fn storage_root(&self, a: &Address) -> TrieResult> { self.ensure_cached(a, RequireCache::None, true, - |a| a.as_ref().and_then(|account| account.storage_root().cloned())) + |a| a.as_ref().and_then(|account| account.storage_root())) + } + + /// Get the original storage root since last commit of account `a`. + pub fn original_storage_root(&self, a: &Address) -> TrieResult { + Ok(self.ensure_cached(a, RequireCache::None, true, + |a| a.as_ref().map(|account| account.original_storage_root()))? + .unwrap_or(KECCAK_NULL_RLP)) } /// Get the value of storage at a specific checkpoint. @@ -566,9 +575,19 @@ impl State { if let Some(value) = account.cached_storage_at(key) { Ok(Some(value)) } else { - // This key has checkpoint entry, but not in the entry's cache. - // So it's not modified at all. - Ok(Some(self.original_storage_at(address, key)?)) + // This account has checkpoint entry, but the key is not in the entry's cache. We + // can use original_storage_at if current account's original storage root is the + // same as checkpoint account's original storage root. Otherwise, we need to + // populate the checkpoint account. Note that the later case is not possible under + // current Ethereum consensus rules. + if account.base_storage_root() == self.original_storage_root(address)? { + Ok(Some(self.original_storage_at(address, key)?)) + } else { + // This account (with given storage root) is definitely not in global cache, + // because the particular storage root must have been created after last state + // commitment. The only possible case for this is an empty storage. + Ok(Some(H256::new())) + } } }, None => { @@ -578,7 +597,7 @@ impl State { } }, None => { - // The value was not cached at that checkpoint, meaning it was not modified. + // The value was not cached at that checkpoint, meaning it was not modified at all. Ok(Some(self.original_storage_at(address, key)?)) }, } @@ -752,13 +771,13 @@ impl State { /// Initialise the code of account `a` so that it is `code`. /// NOTE: Account should have been created with `new_contract`. pub fn init_code(&mut self, a: &Address, code: Bytes) -> TrieResult<()> { - self.require_or_from(a, true, || Account::new_contract(0.into(), self.account_start_nonce), |_|{})?.init_code(code); + self.require_or_from(a, true, || Account::new_contract(0.into(), self.account_start_nonce, KECCAK_NULL_RLP), |_| {})?.init_code(code); Ok(()) } /// Reset the code of account `a` so that it is `code`. pub fn reset_code(&mut self, a: &Address, code: Bytes) -> TrieResult<()> { - self.require_or_from(a, true, || Account::new_contract(0.into(), self.account_start_nonce), |_|{})?.reset_code(code); + self.require_or_from(a, true, || Account::new_contract(0.into(), self.account_start_nonce, KECCAK_NULL_RLP), |_| {})?.reset_code(code); Ok(()) } @@ -1222,7 +1241,7 @@ mod tests { use std::sync::Arc; use std::str::FromStr; use rustc_hex::FromHex; - use hash::keccak; + use hash::{keccak, KECCAK_NULL_RLP}; use super::*; use ethkey::Secret; use ethereum_types::{H256, U256, Address}; @@ -2074,7 +2093,7 @@ mod tests { let a = Address::zero(); let (root, db) = { let mut state = get_temp_state(); - state.require_or_from(&a, false, ||Account::new_contract(42.into(), 0.into()), |_|{}).unwrap(); + state.require_or_from(&a, false, || Account::new_contract(42.into(), 0.into(), KECCAK_NULL_RLP), |_|{}).unwrap(); state.init_code(&a, vec![1, 2, 3]).unwrap(); assert_eq!(state.code(&a).unwrap(), Some(Arc::new(vec![1u8, 2, 3]))); state.commit().unwrap(); @@ -2293,7 +2312,7 @@ mod tests { state.clear(); let c0 = state.checkpoint(); - state.new_contract(&a, U256::zero(), U256::zero()); + state.new_contract(&a, U256::zero(), U256::zero()).unwrap(); let c1 = state.checkpoint(); state.set_storage(&a, k, H256::from(U256::from(1))).unwrap(); let c2 = state.checkpoint(); @@ -2389,7 +2408,7 @@ mod tests { state.add_balance(&b, &100.into(), CleanupMode::ForceCreate).unwrap(); // create a dust account state.add_balance(&c, &101.into(), CleanupMode::ForceCreate).unwrap(); // create a normal account state.add_balance(&d, &99.into(), CleanupMode::ForceCreate).unwrap(); // create another dust account - state.new_contract(&e, 100.into(), 1.into()); // create a contract account + state.new_contract(&e, 100.into(), 1.into()).unwrap(); // create a contract account state.init_code(&e, vec![0x00]).unwrap(); state.commit().unwrap(); state.drop() From a718e902de1c9cbdfa093c6a64fd3f314b871bdf Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 6 Sep 2018 21:40:32 +0800 Subject: [PATCH 26/40] Early return for empty RLP trie storage --- ethcore/src/state/account.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ethcore/src/state/account.rs b/ethcore/src/state/account.rs index 7768adce410..74ff2d7bee3 100644 --- a/ethcore/src/state/account.rs +++ b/ethcore/src/state/account.rs @@ -284,6 +284,12 @@ impl Account { /// Get cached original storage value since last contract creation on this address. Returns `None` if the key is not in the cache. fn cached_moved_original_storage_at(&self, key: &H256) -> Option { + // If storage root is empty RLP, then early return zero value. Practically, this makes it so that if + // `original_storage_cache` is used, then `storage_cache` will always remain empty. + if self.storage_root == KECCAK_NULL_RLP { + return Some(H256::new()); + } + if let Some(value) = self.storage_cache.borrow_mut().get_mut(key) { Some(value.clone()) } else { From f6aa7383c5ab4530c9a58bfd930f7839aac52738 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 6 Sep 2018 22:39:12 +0800 Subject: [PATCH 27/40] Update comments --- ethcore/src/state/mod.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 321d5bb5cc4..1eef19456d0 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -583,9 +583,10 @@ impl State { if account.base_storage_root() == self.original_storage_root(address)? { Ok(Some(self.original_storage_at(address, key)?)) } else { - // This account (with given storage root) is definitely not in global cache, - // because the particular storage root must have been created after last state - // commitment. The only possible case for this is an empty storage. + // If account base storage root is different from the original storage root + // since last commit, then it can only be created from a new contract, where the + // base storage root would always be empty. Note that this branch is actually + // never called, because `cached_storage_at` handled this case. Ok(Some(H256::new())) } } From ad8dc94c4595dc7d50ffa43b92f3a67e80bf8d47 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 6 Sep 2018 22:50:05 +0800 Subject: [PATCH 28/40] Fix borrow_mut issue --- ethcore/src/state/mod.rs | 104 ++++++++++++++++++++++----------------- 1 file changed, 60 insertions(+), 44 deletions(-) diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 1eef19456d0..93d3c8d397f 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -564,55 +564,71 @@ impl State { /// Get the value of storage at a specific checkpoint. pub fn checkpoint_storage_at(&self, checkpoint_index: usize, address: &Address, key: &H256) -> TrieResult> { - let checkpoints = self.checkpoints.borrow(); - let checkpoints_len = checkpoints.len(); - if let Some(ref checkpoint) = checkpoints.get(checkpoint_index) { - if let Some(entry) = checkpoint.get(address) { - match entry { - Some(entry) => { - match entry.account { - Some(ref account) => { - if let Some(value) = account.cached_storage_at(key) { - Ok(Some(value)) - } else { - // This account has checkpoint entry, but the key is not in the entry's cache. We - // can use original_storage_at if current account's original storage root is the - // same as checkpoint account's original storage root. Otherwise, we need to - // populate the checkpoint account. Note that the later case is not possible under - // current Ethereum consensus rules. - if account.base_storage_root() == self.original_storage_root(address)? { - Ok(Some(self.original_storage_at(address, key)?)) - } else { - // If account base storage root is different from the original storage root - // since last commit, then it can only be created from a new contract, where the - // base storage root would always be empty. Note that this branch is actually - // never called, because `cached_storage_at` handled this case. - Ok(Some(H256::new())) - } - } - }, - None => { - // The account didn't exist at that point. Return empty value. - Ok(Some(H256::new())) - }, - } - }, - None => { - // The value was not cached at that checkpoint, meaning it was not modified at all. - Ok(Some(self.original_storage_at(address, key)?)) - }, - } - } else { - // This key does not have a checkpoint entry. + enum ReturnKind { + /// Use original storage at value at this address. + OriginalAt, + /// Use the downward checkpoint value. + Downward, + } + + let (checkpoints_len, kind) = { + let checkpoints = self.checkpoints.borrow(); + (checkpoints.len(), + if let Some(ref checkpoint) = checkpoints.get(checkpoint_index) { + if let Some(entry) = checkpoint.get(address) { + match entry { + Some(entry) => { + match entry.account { + Some(ref account) => { + if let Some(value) = account.cached_storage_at(key) { + return Ok(Some(value)); + } else { + // This account has checkpoint entry, but the key is not in the entry's cache. We + // can use original_storage_at if current account's original storage root is the + // same as checkpoint account's original storage root. Otherwise, we need to + // populate the checkpoint account. Note that the later case is not possible under + // current Ethereum consensus rules. + if account.base_storage_root() == self.original_storage_root(address)? { + ReturnKind::OriginalAt + } else { + // If account base storage root is different from the original storage root + // since last commit, then it can only be created from a new contract, where the + // base storage root would always be empty. Note that this branch is actually + // never called, because `cached_storage_at` handled this case. + return Ok(Some(H256::new())); + } + } + }, + None => { + // The account didn't exist at that point. Return empty value. + return Ok(Some(H256::new())); + }, + } + }, + None => { + // The value was not cached at that checkpoint, meaning it was not modified at all. + ReturnKind::OriginalAt + }, + } + } else { + // This key does not have a checkpoint entry. + ReturnKind::Downward + } + } else { + // The checkpoint was not found. Return None. + return Ok(None); + }) + }; + + match kind { + ReturnKind::Downward => { if checkpoint_index >= checkpoints_len - 1 { Ok(Some(self.storage_at(address, key)?)) } else { self.checkpoint_storage_at(checkpoint_index + 1, address, key) } - } - } else { - // The checkpoint was not found. Return None. - Ok(None) + }, + ReturnKind::OriginalAt => Ok(Some(self.original_storage_at(address, key)?)), } } From 4d5555b8079acf42a81e1d6df78443e8a9f1716c Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 6 Sep 2018 22:57:42 +0800 Subject: [PATCH 29/40] Simplify checkpoint_storage_at if branches --- ethcore/src/state/mod.rs | 84 +++++++++++++++++++--------------------- 1 file changed, 39 insertions(+), 45 deletions(-) diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 93d3c8d397f..023fa7bc3ea 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -573,51 +573,45 @@ impl State { let (checkpoints_len, kind) = { let checkpoints = self.checkpoints.borrow(); - (checkpoints.len(), - if let Some(ref checkpoint) = checkpoints.get(checkpoint_index) { - if let Some(entry) = checkpoint.get(address) { - match entry { - Some(entry) => { - match entry.account { - Some(ref account) => { - if let Some(value) = account.cached_storage_at(key) { - return Ok(Some(value)); - } else { - // This account has checkpoint entry, but the key is not in the entry's cache. We - // can use original_storage_at if current account's original storage root is the - // same as checkpoint account's original storage root. Otherwise, we need to - // populate the checkpoint account. Note that the later case is not possible under - // current Ethereum consensus rules. - if account.base_storage_root() == self.original_storage_root(address)? { - ReturnKind::OriginalAt - } else { - // If account base storage root is different from the original storage root - // since last commit, then it can only be created from a new contract, where the - // base storage root would always be empty. Note that this branch is actually - // never called, because `cached_storage_at` handled this case. - return Ok(Some(H256::new())); - } - } - }, - None => { - // The account didn't exist at that point. Return empty value. - return Ok(Some(H256::new())); - }, - } - }, - None => { - // The value was not cached at that checkpoint, meaning it was not modified at all. - ReturnKind::OriginalAt - }, - } - } else { - // This key does not have a checkpoint entry. - ReturnKind::Downward - } - } else { - // The checkpoint was not found. Return None. - return Ok(None); - }) + let checkpoints_len = checkpoints.len(); + let checkpoint = match checkpoints.get(checkpoint_index) { + Some(checkpoint) => checkpoint, + // The checkpoint was not found. Return None. + None => return Ok(None), + }; + + let kind = match checkpoint.get(address) { + // The account exists at this checkpoint. + Some(Some(AccountEntry { account: Some(ref account), .. })) => { + if let Some(value) = account.cached_storage_at(key) { + return Ok(Some(value)); + } else { + // This account has checkpoint entry, but the key is not in the entry's cache. We can use + // original_storage_at if current account's original storage root is the same as checkpoint + // account's original storage root. Otherwise, we need to populate the checkpoint account. Note + // that the later case is not possible under current Ethereum consensus rules. + if account.base_storage_root() == self.original_storage_root(address)? { + ReturnKind::OriginalAt + } else { + // If account base storage root is different from the original storage root since last + // commit, then it can only be created from a new contract, where the base storage root + // would always be empty. Note that this branch is actually never called, because + // `cached_storage_at` handled this case. + return Ok(Some(H256::new())); + } + } + }, + // The account didn't exist at that point. Return empty value. + Some(Some(AccountEntry { account: None, .. })) => { + return Ok(Some(H256::new())); + }, + // The value was not cached at that checkpoint, meaning it was not modified at all. + Some(None) => ReturnKind::OriginalAt, + // This key does not have a checkpoint entry. + None => ReturnKind::Downward, + }; + + (checkpoints_len, kind) }; match kind { From 8b31417f3a779691981ab48fcd30a4d86b72e5a3 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 6 Sep 2018 23:21:51 +0800 Subject: [PATCH 30/40] Better commenting for gas handling code --- ethcore/evm/src/interpreter/gasometer.rs | 37 ++++++++++++++++++++---- 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/ethcore/evm/src/interpreter/gasometer.rs b/ethcore/evm/src/interpreter/gasometer.rs index 8f607e38d4e..4e4ea455790 100644 --- a/ethcore/evm/src/interpreter/gasometer.rs +++ b/ethcore/evm/src/interpreter/gasometer.rs @@ -351,16 +351,32 @@ fn add_gas_usize(value: Gas, num: usize) -> (Gas, bool) { fn calculate_eip1283_sstore_gas(schedule: &Schedule, original: &U256, current: &U256, new: &U256) -> Gas { Gas::from( if current == new { + // 1. If current value equals new value (this is a no-op), 200 gas is deducted. schedule.sload_gas } else { + // 2. If current value does not equal new value if original == current { + // 2.1. If original value equals current value (this storage slot has not been changed by the current execution context) if original.is_zero() { + // 2.1.1. If original value is 0, 20000 gas is deducted. schedule.sstore_set_gas } else { + // 2.1.2. Otherwise, 5000 gas is deducted. schedule.sstore_reset_gas + + // 2.1.2.1. If new value is 0, add 15000 gas to refund counter. } } else { + // 2.2. If original value does not equal current value (this storage slot is dirty), 200 gas is deducted. Apply both of the following clauses. schedule.sload_gas + + // 2.2.1. If original value is not 0 + // 2.2.1.1. If current value is 0 (also means that new value is not 0), remove 15000 gas from refund counter. We can prove that refund counter will never go below 0. + // 2.2.1.2. If new value is 0 (also means that current value is not 0), add 15000 gas to refund counter. + + // 2.2.2. If original value equals new value (this storage slot is reset) + // 2.2.2.1. If original value is 0, add 19800 gas to refund counter. + // 2.2.2.2. Otherwise, add 4800 gas to refund counter. } } ) @@ -370,33 +386,42 @@ pub fn handle_eip1283_sstore_clears_refund(ext: &mut vm::Ext, original: &U256, c let sstore_clears_schedule = U256::from(ext.schedule().sstore_refund_gas); if current == new { - // No refund + // 1. If current value equals new value (this is a no-op), 200 gas is deducted. } else { + // 2. If current value does not equal new value if original == current { + // 2.1. If original value equals current value (this storage slot has not been changed by the current execution context) if original.is_zero() { - // No refund + // 2.1.1. If original value is 0, 20000 gas is deducted. } else { + // 2.1.2. Otherwise, 5000 gas is deducted. if new.is_zero() { + // 2.1.2.1. If new value is 0, add 15000 gas to refund counter. ext.inc_sstore_refund(sstore_clears_schedule); } } } else { - // Handle invalidated and newly validated R_RESET refunds. + // 2.2. If original value does not equal current value (this storage slot is dirty), 200 gas is deducted. Apply both of the following clauses. + if !original.is_zero() { + // 2.2.1. If original value is not 0 if current.is_zero() { + // 2.2.1.1. If current value is 0 (also means that new value is not 0), remove 15000 gas from refund counter. We can prove that refund counter will never go below 0. ext.dec_sstore_refund(sstore_clears_schedule); } else if new.is_zero() { + // 2.2.1.2. If new value is 0 (also means that current value is not 0), add 15000 gas to refund counter. ext.inc_sstore_refund(sstore_clears_schedule); } } - // Reset storage slot to its original value. + if original == new { + // 2.2.2. If original value equals new value (this storage slot is reset) if original.is_zero() { - // Revert sstore full cost (minus sload amount) + // 2.2.2.1. If original value is 0, add 19800 gas to refund counter. let refund = U256::from(ext.schedule().sstore_set_gas - ext.schedule().sload_gas); ext.inc_sstore_refund(refund); } else { - // Revert sstore change cost (revert of refund done in previous conditions) + // 2.2.2.2. Otherwise, add 4800 gas to refund counter. let refund = U256::from(ext.schedule().sstore_reset_gas - ext.schedule().sload_gas); ext.inc_sstore_refund(refund); } From 72693d3078ff2228d50d9b37e0cad0220b37aafc Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 6 Sep 2018 23:29:15 +0800 Subject: [PATCH 31/40] Address naming grumbles --- ethcore/evm/src/interpreter/gasometer.rs | 12 ++++++------ ethcore/evm/src/interpreter/mod.rs | 4 ++-- ethcore/src/externalities.rs | 6 +++--- ethcore/src/json_tests/executive.rs | 12 ++++++------ ethcore/vm/src/ext.rs | 6 +++--- ethcore/vm/src/tests.rs | 6 +++--- ethcore/wasm/src/runtime.rs | 2 +- 7 files changed, 24 insertions(+), 24 deletions(-) diff --git a/ethcore/evm/src/interpreter/gasometer.rs b/ethcore/evm/src/interpreter/gasometer.rs index 4e4ea455790..1db79c91296 100644 --- a/ethcore/evm/src/interpreter/gasometer.rs +++ b/ethcore/evm/src/interpreter/gasometer.rs @@ -124,7 +124,7 @@ impl Gasometer { let address = H256::from(stack.peek(0)); let newval = stack.peek(1); let val = U256::from(&*ext.storage_at(&address)?); - let orig = U256::from(&*ext.reverted_storage_at(&address)?); + let orig = U256::from(&*ext.initial_storage_at(&address)?); let gas = if schedule.eip1283 { calculate_eip1283_sstore_gas(schedule, &orig, &val, &newval) @@ -397,7 +397,7 @@ pub fn handle_eip1283_sstore_clears_refund(ext: &mut vm::Ext, original: &U256, c // 2.1.2. Otherwise, 5000 gas is deducted. if new.is_zero() { // 2.1.2.1. If new value is 0, add 15000 gas to refund counter. - ext.inc_sstore_refund(sstore_clears_schedule); + ext.add_sstore_refund(sstore_clears_schedule); } } } else { @@ -407,10 +407,10 @@ pub fn handle_eip1283_sstore_clears_refund(ext: &mut vm::Ext, original: &U256, c // 2.2.1. If original value is not 0 if current.is_zero() { // 2.2.1.1. If current value is 0 (also means that new value is not 0), remove 15000 gas from refund counter. We can prove that refund counter will never go below 0. - ext.dec_sstore_refund(sstore_clears_schedule); + ext.sub_sstore_refund(sstore_clears_schedule); } else if new.is_zero() { // 2.2.1.2. If new value is 0 (also means that current value is not 0), add 15000 gas to refund counter. - ext.inc_sstore_refund(sstore_clears_schedule); + ext.add_sstore_refund(sstore_clears_schedule); } } @@ -419,11 +419,11 @@ pub fn handle_eip1283_sstore_clears_refund(ext: &mut vm::Ext, original: &U256, c if original.is_zero() { // 2.2.2.1. If original value is 0, add 19800 gas to refund counter. let refund = U256::from(ext.schedule().sstore_set_gas - ext.schedule().sload_gas); - ext.inc_sstore_refund(refund); + ext.add_sstore_refund(refund); } else { // 2.2.2.2. Otherwise, add 4800 gas to refund counter. let refund = U256::from(ext.schedule().sstore_reset_gas - ext.schedule().sload_gas); - ext.inc_sstore_refund(refund); + ext.add_sstore_refund(refund); } } } diff --git a/ethcore/evm/src/interpreter/mod.rs b/ethcore/evm/src/interpreter/mod.rs index fa482f7f70f..2c2bc5ad234 100644 --- a/ethcore/evm/src/interpreter/mod.rs +++ b/ethcore/evm/src/interpreter/mod.rs @@ -628,14 +628,14 @@ impl Interpreter { let val = self.stack.pop_back(); let current_val = U256::from(&*ext.storage_at(&address)?); - let original_val = U256::from(&*ext.reverted_storage_at(&address)?); // Increase refund for clear if ext.schedule().eip1283 { + let original_val = U256::from(&*ext.initial_storage_at(&address)?); gasometer::handle_eip1283_sstore_clears_refund(ext, &original_val, ¤t_val, &val); } else { if !current_val.is_zero() && val.is_zero() { let sstore_clears_schedule = U256::from(ext.schedule().sstore_refund_gas); - ext.inc_sstore_refund(sstore_clears_schedule); + ext.add_sstore_refund(sstore_clears_schedule); } } ext.set_storage(address, H256::from(&val))?; diff --git a/ethcore/src/externalities.rs b/ethcore/src/externalities.rs index 282f8a966a6..5c7c95c6671 100644 --- a/ethcore/src/externalities.rs +++ b/ethcore/src/externalities.rs @@ -113,7 +113,7 @@ impl<'a, T: 'a, V: 'a, B: 'a> Externalities<'a, T, V, B> impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B> where T: Tracer, V: VMTracer, B: StateBackend { - fn reverted_storage_at(&self, key: &H256) -> vm::Result { + fn initial_storage_at(&self, key: &H256) -> vm::Result { self.state.checkpoint_storage_at(0, &self.origin_info.address, key).map(|v| v.unwrap_or(H256::zero())).map_err(Into::into) } @@ -394,11 +394,11 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B> self.depth } - fn inc_sstore_refund(&mut self, value: U256) { + fn add_sstore_refund(&mut self, value: U256) { self.substate.sstore_clears_refund = self.substate.sstore_clears_refund.saturating_add(value); } - fn dec_sstore_refund(&mut self, value: U256) { + fn sub_sstore_refund(&mut self, value: U256) { self.substate.sstore_clears_refund = self.substate.sstore_clears_refund.saturating_sub(value); } diff --git a/ethcore/src/json_tests/executive.rs b/ethcore/src/json_tests/executive.rs index fc29a9c9a60..78ef3e22dcf 100644 --- a/ethcore/src/json_tests/executive.rs +++ b/ethcore/src/json_tests/executive.rs @@ -112,8 +112,8 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for TestExt<'a, T, V, B> self.ext.storage_at(key) } - fn reverted_storage_at(&self, key: &H256) -> vm::Result { - self.ext.reverted_storage_at(key) + fn initial_storage_at(&self, key: &H256) -> vm::Result { + self.ext.initial_storage_at(key) } fn set_storage(&mut self, key: H256, value: H256) -> vm::Result<()> { @@ -209,12 +209,12 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for TestExt<'a, T, V, B> false } - fn inc_sstore_refund(&mut self, value: U256) { - self.ext.inc_sstore_refund(value) + fn add_sstore_refund(&mut self, value: U256) { + self.ext.add_sstore_refund(value) } - fn dec_sstore_refund(&mut self, value: U256) { - self.ext.dec_sstore_refund(value) + fn sub_sstore_refund(&mut self, value: U256) { + self.ext.sub_sstore_refund(value) } } diff --git a/ethcore/vm/src/ext.rs b/ethcore/vm/src/ext.rs index 591b62d4ad5..168640593b8 100644 --- a/ethcore/vm/src/ext.rs +++ b/ethcore/vm/src/ext.rs @@ -64,7 +64,7 @@ pub enum CreateContractAddress { /// Externalities interface for EVMs pub trait Ext { /// Returns the storage value for a given key if reversion happens on the current transaction. - fn reverted_storage_at(&self, key: &H256) -> Result; + fn initial_storage_at(&self, key: &H256) -> Result; /// Returns a value for given key. fn storage_at(&self, key: &H256) -> Result; @@ -140,10 +140,10 @@ pub trait Ext { fn depth(&self) -> usize; /// Increments sstore refunds counter. - fn inc_sstore_refund(&mut self, value: U256); + fn add_sstore_refund(&mut self, value: U256); /// Decrements sstore refunds counter. - fn dec_sstore_refund(&mut self, value: U256); + fn sub_sstore_refund(&mut self, value: U256); /// Decide if any more operations should be traced. Passthrough for the VM trace. fn trace_next_instruction(&mut self, _pc: usize, _instruction: u8, _current_gas: U256) -> bool { false } diff --git a/ethcore/vm/src/tests.rs b/ethcore/vm/src/tests.rs index ac57704b26d..b6e44f000e4 100644 --- a/ethcore/vm/src/tests.rs +++ b/ethcore/vm/src/tests.rs @@ -105,7 +105,7 @@ impl FakeExt { } impl Ext for FakeExt { - fn reverted_storage_at(&self, _key: &H256) -> Result { + fn initial_storage_at(&self, _key: &H256) -> Result { Ok(H256::new()) } @@ -220,11 +220,11 @@ impl Ext for FakeExt { self.is_static } - fn inc_sstore_refund(&mut self, value: U256) { + fn add_sstore_refund(&mut self, value: U256) { self.sstore_clears = self.sstore_clears + value; } - fn dec_sstore_refund(&mut self, value: U256) { + fn sub_sstore_refund(&mut self, value: U256) { self.sstore_clears = self.sstore_clears - value; } diff --git a/ethcore/wasm/src/runtime.rs b/ethcore/wasm/src/runtime.rs index 49f67d8360d..41c0d950da4 100644 --- a/ethcore/wasm/src/runtime.rs +++ b/ethcore/wasm/src/runtime.rs @@ -286,7 +286,7 @@ impl<'a> Runtime<'a> { if former_val != H256::zero() && val == H256::zero() { let sstore_clears_schedule = U256::from(self.schedule().sstore_refund_gas); - self.ext.inc_sstore_refund(sstore_clears_schedule); + self.ext.add_sstore_refund(sstore_clears_schedule); } Ok(()) From ab83e3689ced36c3cbd998e0e29570bcdc622664 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 6 Sep 2018 23:39:01 +0800 Subject: [PATCH 32/40] More tests --- ethcore/src/state/mod.rs | 62 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 023fa7bc3ea..80dbeea32c2 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -2308,6 +2308,61 @@ mod tests { assert_eq!(state.balance(&a).unwrap(), U256::from(0)); } + #[test] + fn checkpoint_from_empty_get_storage_at() { + let mut state = get_temp_state(); + let a = Address::zero(); + let k = H256::from(U256::from(0)); + let k2 = H256::from(U256::from(1)); + + assert_eq!(state.storage_at(&a, &k).unwrap(), H256::from(U256::from(0))); + state.clear(); + + let c0 = state.checkpoint(); + state.new_contract(&a, U256::zero(), U256::zero()).unwrap(); + let c1 = state.checkpoint(); + state.set_storage(&a, k, H256::from(U256::from(1))).unwrap(); + let c2 = state.checkpoint(); + let c3 = state.checkpoint(); + state.set_storage(&a, k2, H256::from(U256::from(3))).unwrap(); + state.set_storage(&a, k, H256::from(U256::from(3))).unwrap(); + let c4 = state.checkpoint(); + state.set_storage(&a, k, H256::from(U256::from(4))).unwrap(); + let c5 = state.checkpoint(); + + assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + assert_eq!(state.checkpoint_storage_at(c2, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); + assert_eq!(state.checkpoint_storage_at(c3, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); + assert_eq!(state.checkpoint_storage_at(c4, &a, &k).unwrap(), Some(H256::from(U256::from(3)))); + assert_eq!(state.checkpoint_storage_at(c5, &a, &k).unwrap(), Some(H256::from(U256::from(4)))); + + state.discard_checkpoint(); // Commit/discard c5. + assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + assert_eq!(state.checkpoint_storage_at(c2, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); + assert_eq!(state.checkpoint_storage_at(c3, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); + assert_eq!(state.checkpoint_storage_at(c4, &a, &k).unwrap(), Some(H256::from(U256::from(3)))); + + state.revert_to_checkpoint(); // Revert to c4. + assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + assert_eq!(state.checkpoint_storage_at(c2, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); + assert_eq!(state.checkpoint_storage_at(c3, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); + + state.discard_checkpoint(); // Commit/discard c3. + assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + assert_eq!(state.checkpoint_storage_at(c2, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); + + state.revert_to_checkpoint(); // Revert to c2. + assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + + state.discard_checkpoint(); // Commit/discard c1. + assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + } + #[test] fn checkpoint_get_storage_at() { let mut state = get_temp_state(); @@ -2322,6 +2377,7 @@ mod tests { assert_eq!(state.storage_at(&a, &k).unwrap(), H256::from(U256::from(0xffff))); state.clear(); + let cm1 = state.checkpoint(); let c0 = state.checkpoint(); state.new_contract(&a, U256::zero(), U256::zero()).unwrap(); let c1 = state.checkpoint(); @@ -2334,6 +2390,7 @@ mod tests { state.set_storage(&a, k, H256::from(U256::from(4))).unwrap(); let c5 = state.checkpoint(); + assert_eq!(state.checkpoint_storage_at(cm1, &a, &k).unwrap(), Some(H256::from(U256::from(0xffff)))); assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0xffff)))); assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); assert_eq!(state.checkpoint_storage_at(c2, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); @@ -2342,6 +2399,7 @@ mod tests { assert_eq!(state.checkpoint_storage_at(c5, &a, &k).unwrap(), Some(H256::from(U256::from(4)))); state.discard_checkpoint(); // Commit/discard c5. + assert_eq!(state.checkpoint_storage_at(cm1, &a, &k).unwrap(), Some(H256::from(U256::from(0xffff)))); assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0xffff)))); assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); assert_eq!(state.checkpoint_storage_at(c2, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); @@ -2349,21 +2407,25 @@ mod tests { assert_eq!(state.checkpoint_storage_at(c4, &a, &k).unwrap(), Some(H256::from(U256::from(3)))); state.revert_to_checkpoint(); // Revert to c4. + assert_eq!(state.checkpoint_storage_at(cm1, &a, &k).unwrap(), Some(H256::from(U256::from(0xffff)))); assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0xffff)))); assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); assert_eq!(state.checkpoint_storage_at(c2, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); assert_eq!(state.checkpoint_storage_at(c3, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); state.discard_checkpoint(); // Commit/discard c3. + assert_eq!(state.checkpoint_storage_at(cm1, &a, &k).unwrap(), Some(H256::from(U256::from(0xffff)))); assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0xffff)))); assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); assert_eq!(state.checkpoint_storage_at(c2, &a, &k).unwrap(), Some(H256::from(U256::from(1)))); state.revert_to_checkpoint(); // Revert to c2. + assert_eq!(state.checkpoint_storage_at(cm1, &a, &k).unwrap(), Some(H256::from(U256::from(0xffff)))); assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0xffff)))); assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); state.discard_checkpoint(); // Commit/discard c1. + assert_eq!(state.checkpoint_storage_at(cm1, &a, &k).unwrap(), Some(H256::from(U256::from(0xffff)))); assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0xffff)))); } From ef6a01c68e29556d510434bce4dd2ac218117b4f Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Fri, 7 Sep 2018 09:39:35 +0800 Subject: [PATCH 33/40] Fix an issue in overwrite_with --- ethcore/src/state/account.rs | 2 +- ethcore/src/state/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ethcore/src/state/account.rs b/ethcore/src/state/account.rs index 74ff2d7bee3..9741a850539 100644 --- a/ethcore/src/state/account.rs +++ b/ethcore/src/state/account.rs @@ -553,7 +553,6 @@ impl Account { pub fn overwrite_with(&mut self, other: Account) { self.balance = other.balance; self.nonce = other.nonce; - self.storage_root = other.storage_root; self.code_hash = other.code_hash; self.code_filth = other.code_filth; self.code_cache = other.code_cache; @@ -568,6 +567,7 @@ impl Account { self.storage_cache = other.storage_cache; } self.original_storage_cache = other.original_storage_cache; + self.storage_root = other.storage_root; self.storage_changes = other.storage_changes; } } diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 80dbeea32c2..55e9097d834 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -1112,7 +1112,7 @@ impl State { /// Pull account `a` in our cache from the trie DB. `require_code` requires that the code be cached, too. fn require<'a>(&'a self, a: &Address, require_code: bool) -> TrieResult> { - self.require_or_from(a, require_code, || Account::new_basic(0u8.into(), self.account_start_nonce), |_|{}) + self.require_or_from(a, require_code, || Account::new_basic(0u8.into(), self.account_start_nonce), |_| {}) } /// Pull account `a` in our cache from the trie DB. `require_code` requires that the code be cached, too. From aa6006eb5869d5777fee66c022e2398ee30f64ab Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Fri, 7 Sep 2018 09:57:46 +0800 Subject: [PATCH 34/40] Add another test --- ethcore/src/state/mod.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 55e9097d834..711861c2d7e 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -918,6 +918,7 @@ impl State { /// Clear state cache pub fn clear(&mut self) { + assert!(self.checkpoints.borrow().is_empty()); self.cache.borrow_mut().clear(); } @@ -2308,6 +2309,25 @@ mod tests { assert_eq!(state.balance(&a).unwrap(), U256::from(0)); } + #[test] + fn checkpoint_revert_to_get_storage_at() { + let mut state = get_temp_state(); + let a = Address::zero(); + let k = H256::from(U256::from(0)); + + let c0 = state.checkpoint(); + let c1 = state.checkpoint(); + state.set_storage(&a, k, H256::from(U256::from(1))).unwrap(); + + assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + assert_eq!(state.checkpoint_storage_at(c1, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + assert_eq!(state.storage_at(&a, &k).unwrap(), H256::from(U256::from(1))); + + state.revert_to_checkpoint(); // Revert to c1. + assert_eq!(state.checkpoint_storage_at(c0, &a, &k).unwrap(), Some(H256::from(U256::from(0)))); + assert_eq!(state.storage_at(&a, &k).unwrap(), H256::from(U256::from(0))); + } + #[test] fn checkpoint_from_empty_get_storage_at() { let mut state = get_temp_state(); From aabc36705f72203e9f3185e8ee73728ef57821e0 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Fri, 7 Sep 2018 15:36:56 +0800 Subject: [PATCH 35/40] Fix comment --- ethcore/src/state/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 711861c2d7e..1c1c34f94ea 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -588,8 +588,7 @@ impl State { } else { // This account has checkpoint entry, but the key is not in the entry's cache. We can use // original_storage_at if current account's original storage root is the same as checkpoint - // account's original storage root. Otherwise, we need to populate the checkpoint account. Note - // that the later case is not possible under current Ethereum consensus rules. + // account's original storage root. Otherwise, the account must be a newly created contract. if account.base_storage_root() == self.original_storage_root(address)? { ReturnKind::OriginalAt } else { From 8a573a10416a02a9e2dae77eeb22c143b4782a50 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Fri, 7 Sep 2018 15:37:43 +0800 Subject: [PATCH 36/40] Remove unnecessary bracket --- ethcore/src/state/mod.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 1c1c34f94ea..d22e8f7faff 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -601,9 +601,7 @@ impl State { } }, // The account didn't exist at that point. Return empty value. - Some(Some(AccountEntry { account: None, .. })) => { - return Ok(Some(H256::new())); - }, + Some(Some(AccountEntry { account: None, .. })) => return Ok(Some(H256::new())), // The value was not cached at that checkpoint, meaning it was not modified at all. Some(None) => ReturnKind::OriginalAt, // This key does not have a checkpoint entry. From c31cc59bc3b97dea94f199d2188393cf2b4bca2a Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Fri, 7 Sep 2018 15:48:48 +0800 Subject: [PATCH 37/40] Move orig to inner if --- ethcore/evm/src/interpreter/gasometer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethcore/evm/src/interpreter/gasometer.rs b/ethcore/evm/src/interpreter/gasometer.rs index 1db79c91296..406df19fd35 100644 --- a/ethcore/evm/src/interpreter/gasometer.rs +++ b/ethcore/evm/src/interpreter/gasometer.rs @@ -124,9 +124,9 @@ impl Gasometer { let address = H256::from(stack.peek(0)); let newval = stack.peek(1); let val = U256::from(&*ext.storage_at(&address)?); - let orig = U256::from(&*ext.initial_storage_at(&address)?); let gas = if schedule.eip1283 { + let orig = U256::from(&*ext.initial_storage_at(&address)?); calculate_eip1283_sstore_gas(schedule, &orig, &val, &newval) } else { if val.is_zero() && !newval.is_zero() { From 522f2e9483c751b421f48b29b01eac96c6dd1e33 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Fri, 7 Sep 2018 15:49:12 +0800 Subject: [PATCH 38/40] Remove test coverage for this PR --- .gitlab-ci.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 79c130b6913..51ecec41a97 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -85,7 +85,6 @@ test-coverage-kcov: stage: test only: - master - - pr-9319 script: - scripts/gitlab/coverage.sh tags: From 14283f8d58170a790b27cdb67774531b1d2a125f Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Fri, 7 Sep 2018 16:30:20 +0800 Subject: [PATCH 39/40] Add tests for executive original value --- ethcore/src/executive.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ethcore/src/executive.rs b/ethcore/src/executive.rs index 113959611cc..d2cf2f027fd 100644 --- a/ethcore/src/executive.rs +++ b/ethcore/src/executive.rs @@ -1620,6 +1620,8 @@ mod tests { let x2 = Address::from(0x1001); let y1 = Address::from(0x2001); let y2 = Address::from(0x2002); + let operating_address = Address::from(0); + let k = H256::new(); let mut state = get_temp_state_with_factory(factory.clone()); state.new_contract(&x1, U256::zero(), U256::from(1)).unwrap(); @@ -1635,6 +1637,7 @@ mod tests { let machine = ::ethereum::new_constantinople_test_machine(); let schedule = machine.schedule(info.number); + assert_eq!(state.storage_at(&operating_address, &k).unwrap(), H256::from(U256::from(0))); // Test a call via top-level -> y1 -> x1 let (FinalizationResult { gas_left, .. }, refund, gas) = { let gas = U256::from(0xffffffffffu64); @@ -1652,6 +1655,7 @@ mod tests { assert_eq!(gas_used, U256::from(41860)); assert_eq!(refund, U256::from(19800)); + assert_eq!(state.storage_at(&operating_address, &k).unwrap(), H256::from(U256::from(1))); // Test a call via top-level -> y2 -> x2 let (FinalizationResult { gas_left, .. }, refund, gas) = { let gas = U256::from(0xffffffffffu64); From ae73ed4375fac9b89ddb3554ef03c746d3fc65a3 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Fri, 7 Sep 2018 17:18:12 +0800 Subject: [PATCH 40/40] Add warn! for an unreachable cause --- ethcore/src/state/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index d22e8f7faff..fb843c9cc2e 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -596,6 +596,7 @@ impl State { // commit, then it can only be created from a new contract, where the base storage root // would always be empty. Note that this branch is actually never called, because // `cached_storage_at` handled this case. + warn!("Trying to get an account's cached storage value, but base storage root does not equal to original storage root! Assuming the value is empty."); return Ok(Some(H256::new())); } }