From d972013c03c762c8d81d9331e008a3637748384d Mon Sep 17 00:00:00 2001 From: Marcio Diaz Date: Thu, 31 Oct 2019 23:43:14 +0100 Subject: [PATCH 1/4] Comment local_cache propagation --- core/client/db/src/storage_cache.rs | 42 ++++++++++++++--------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/core/client/db/src/storage_cache.rs b/core/client/db/src/storage_cache.rs index af8c9e379c4c1..693b75f03aad1 100644 --- a/core/client/db/src/storage_cache.rs +++ b/core/client/db/src/storage_cache.rs @@ -321,27 +321,27 @@ impl CacheChanges { // Propagate cache only if committing on top of the latest canonical state // blocks are ordered by number and only one block with a given number is marked as canonical // (contributed to canonical state cache) - if let Some(_) = self.parent_hash { - let mut local_cache = self.local_cache.write(); - if is_best { - trace!( - "Committing {} local, {} hashes, {} modified root entries, {} modified child entries", - local_cache.storage.len(), - local_cache.hashes.len(), - changes.len(), - child_changes.iter().map(|v|v.1.len()).sum::(), - ); - for (k, v) in local_cache.storage.drain() { - cache.lru_storage.add(k, v); - } - for (k, v) in local_cache.child_storage.drain() { - cache.lru_child_storage.add(k, v); - } - for (k, v) in local_cache.hashes.drain() { - cache.lru_hashes.add(k, OptionHOut(v)); - } - } - } + // if let Some(_) = self.parent_hash { + // let mut local_cache = self.local_cache.write(); + // if is_best { + // trace!( + // "Committing {} local, {} hashes, {} modified root entries, {} modified child entries", + // local_cache.storage.len(), + // local_cache.hashes.len(), + // changes.len(), + // child_changes.iter().map(|v|v.1.len()).sum::(), + // ); + // for (k, v) in local_cache.storage.drain() { + // cache.lru_storage.add(k, v); + // } + // for (k, v) in local_cache.child_storage.drain() { + // cache.lru_child_storage.add(k, v); + // } + // for (k, v) in local_cache.hashes.drain() { + // cache.lru_hashes.add(k, OptionHOut(v)); + // } + // } + // } if let ( Some(ref number), Some(ref hash), Some(ref parent)) From 88b88da8b83ba902ae22a0c591090d013eb53e57 Mon Sep 17 00:00:00 2001 From: Marcio Diaz Date: Fri, 1 Nov 2019 02:18:56 +0100 Subject: [PATCH 2/4] Add test --- core/client/db/src/storage_cache.rs | 85 ++++++++++++++++++++++------- 1 file changed, 64 insertions(+), 21 deletions(-) diff --git a/core/client/db/src/storage_cache.rs b/core/client/db/src/storage_cache.rs index 693b75f03aad1..d97af8bfbd712 100644 --- a/core/client/db/src/storage_cache.rs +++ b/core/client/db/src/storage_cache.rs @@ -321,27 +321,29 @@ impl CacheChanges { // Propagate cache only if committing on top of the latest canonical state // blocks are ordered by number and only one block with a given number is marked as canonical // (contributed to canonical state cache) - // if let Some(_) = self.parent_hash { - // let mut local_cache = self.local_cache.write(); - // if is_best { - // trace!( - // "Committing {} local, {} hashes, {} modified root entries, {} modified child entries", - // local_cache.storage.len(), - // local_cache.hashes.len(), - // changes.len(), - // child_changes.iter().map(|v|v.1.len()).sum::(), - // ); - // for (k, v) in local_cache.storage.drain() { - // cache.lru_storage.add(k, v); - // } - // for (k, v) in local_cache.child_storage.drain() { - // cache.lru_child_storage.add(k, v); - // } - // for (k, v) in local_cache.hashes.drain() { - // cache.lru_hashes.add(k, OptionHOut(v)); - // } - // } - // } + + // TODO: Uncomment this if block to make the test pass. + if let Some(_) = self.parent_hash { + let mut local_cache = self.local_cache.write(); + if is_best { + trace!( + "Committing {} local, {} hashes, {} modified root entries, {} modified child entries", + local_cache.storage.len(), + local_cache.hashes.len(), + changes.len(), + child_changes.iter().map(|v|v.1.len()).sum::(), + ); + for (k, v) in local_cache.storage.drain() { + cache.lru_storage.add(k, v); + } + for (k, v) in local_cache.child_storage.drain() { + cache.lru_child_storage.add(k, v); + } + for (k, v) in local_cache.hashes.drain() { + cache.lru_hashes.add(k, OptionHOut(v)); + } + } + } if let ( Some(ref number), Some(ref hash), Some(ref parent)) @@ -728,4 +730,45 @@ mod tests { // 32 key, 2 byte size assert_eq!(shared.lock().used_storage_cache_size(), 34 /* bytes */); } + + #[test] + fn fix_storage_mismatch_issue() { + let root_parent = H256::random(); + + let key = H256::random()[..].to_vec(); + + let h0 = H256::random(); + let h1 = H256::random(); + + let shared = new_shared_cache::(256*1024, (0,1)); + let mut s = CachingState::new(InMemory::::default(), shared.clone(), Some(root_parent.clone())); + s.cache.sync_cache(&[], &[], vec![(key.clone(), Some(vec![2]))], vec![], Some(h0.clone()), Some(0), || true); + + let mut s = CachingState::new(InMemory::::default(), shared.clone(), Some(h0.clone())); + s.cache.sync_cache(&[], &[], vec![(key.clone(), Some(vec![3]))], vec![], Some(h1.clone()), Some(1), || true); + + let mut s = CachingState::new(InMemory::::default(), shared.clone(), Some(h1.clone())); + assert_eq!(s.storage(&key).unwrap(), Some(vec![3])); + + // Restart (or unknown block?), clear caches. + { + let mut cache = s.cache.shared_cache.lock(); + let cache = &mut *cache; + cache.lru_storage.clear(); + cache.lru_hashes.clear(); + cache.lru_child_storage.clear(); + cache.modifications.clear(); + } + + // New value is written because of cache miss. + s.cache.local_cache.write().storage.insert(key.clone(), Some(vec![42])); + + // New value is propagated. + s.cache.sync_cache(&[], &[], vec![], vec![], None, None, || true); + + // Wrongly get new value 42. It should instead return None or old value 3 at block h1. + let mut s = CachingState::new(InMemory::::default(), shared.clone(), Some(h1.clone())); + s.cache.sync_cache(&[], &[], vec![], vec![], None, None, || false); + assert_eq!(s.storage(&key).unwrap(), Some(vec![42])); + } } From f93b511d08438de3f41b7a90792f6beb867c951c Mon Sep 17 00:00:00 2001 From: arkpar Date: Fri, 1 Nov 2019 12:06:06 +0100 Subject: [PATCH 3/4] Deny cache when modifications are unknown --- core/client/db/src/storage_cache.rs | 42 ++++++++++++----------------- 1 file changed, 17 insertions(+), 25 deletions(-) diff --git a/core/client/db/src/storage_cache.rs b/core/client/db/src/storage_cache.rs index d97af8bfbd712..f7874ee230e9a 100644 --- a/core/client/db/src/storage_cache.rs +++ b/core/client/db/src/storage_cache.rs @@ -22,6 +22,7 @@ use parking_lot::{Mutex, RwLock, RwLockUpgradableReadGuard}; use linked_hash_map::{LinkedHashMap, Entry}; use hash_db::Hasher; use sr_primitives::traits::{Block as BlockT, Header}; +use primitives::hexdisplay::HexDisplay; use state_machine::{backend::Backend as StateBackend, TrieBackend}; use log::trace; use super::{StorageCollection, ChildStorageCollection}; @@ -168,7 +169,7 @@ impl Cache { trace!("Reverting enacted block {:?}", block); m.is_canon = true; for a in &m.storage { - trace!("Reverting enacted key {:?}", a); + trace!("Reverting enacted key {:?}", HexDisplay::from(a)); self.lru_storage.remove(a); } for a in &m.child_storage { @@ -188,7 +189,7 @@ impl Cache { trace!("Retracting block {:?}", block); m.is_canon = false; for a in &m.storage { - trace!("Retracted key {:?}", a); + trace!("Retracted key {:?}", HexDisplay::from(a)); self.lru_storage.remove(a); } for a in &m.child_storage { @@ -321,8 +322,6 @@ impl CacheChanges { // Propagate cache only if committing on top of the latest canonical state // blocks are ordered by number and only one block with a given number is marked as canonical // (contributed to canonical state cache) - - // TODO: Uncomment this if block to make the test pass. if let Some(_) = self.parent_hash { let mut local_cache = self.local_cache.write(); if is_best { @@ -418,21 +417,16 @@ impl, B: BlockT> CachingState { key: Option<&[u8]>, child_key: Option<&ChildStorageKey>, parent_hash: &Option, - modifications: - &VecDeque> + modifications: &VecDeque> ) -> bool { let mut parent = match *parent_hash { None => { - trace!("Cache lookup skipped for {:?}: no parent hash", key); + trace!("Cache lookup skipped for {:?}: no parent hash", key.as_ref().map(HexDisplay::from)); return false; } Some(ref parent) => parent, }; - if modifications.is_empty() { - trace!("Cache lookup allowed for {:?}", key); - return true; - } // Ignore all storage modified in later blocks // Modifications contains block ordered by the number // We search for our parent in that list first and then for @@ -447,7 +441,7 @@ impl, B: BlockT> CachingState { } if let Some(key) = key { if m.storage.contains(key) { - trace!("Cache lookup skipped for {:?}: modified in a later block", key); + trace!("Cache lookup skipped for {:?}: modified in a later block", HexDisplay::from(&key)); return false; } } @@ -458,7 +452,7 @@ impl, B: BlockT> CachingState { } } } - trace!("Cache lookup skipped for {:?}: parent hash is unknown", key); + trace!("Cache lookup skipped for {:?}: parent hash is unknown", key.as_ref().map(HexDisplay::from)); false } @@ -477,17 +471,17 @@ impl, B: BlockT> StateBackend for CachingState< let local_cache = self.cache.local_cache.upgradable_read(); // Note that local cache makes that lru is not refreshed if let Some(entry) = local_cache.storage.get(key).cloned() { - trace!("Found in local cache: {:?}", key); + trace!("Found in local cache: {:?}", HexDisplay::from(&key)); return Ok(entry) } let mut cache = self.cache.shared_cache.lock(); if Self::is_allowed(Some(key), None, &self.cache.parent_hash, &cache.modifications) { if let Some(entry) = cache.lru_storage.get(key).map(|a| a.clone()) { - trace!("Found in shared cache: {:?}", key); + trace!("Found in shared cache: {:?}", HexDisplay::from(&key)); return Ok(entry) } } - trace!("Cache miss: {:?}", key); + trace!("Cache miss: {:?}", HexDisplay::from(&key)); let value = self.state.storage(key)?; RwLockUpgradableReadGuard::upgrade(local_cache).storage.insert(key.to_vec(), value.clone()); Ok(value) @@ -496,17 +490,17 @@ impl, B: BlockT> StateBackend for CachingState< fn storage_hash(&self, key: &[u8]) -> Result, Self::Error> { let local_cache = self.cache.local_cache.upgradable_read(); if let Some(entry) = local_cache.hashes.get(key).cloned() { - trace!("Found hash in local cache: {:?}", key); + trace!("Found hash in local cache: {:?}", HexDisplay::from(&key)); return Ok(entry) } let mut cache = self.cache.shared_cache.lock(); if Self::is_allowed(Some(key), None, &self.cache.parent_hash, &cache.modifications) { if let Some(entry) = cache.lru_hashes.get(key).map(|a| a.0.clone()) { - trace!("Found hash in shared cache: {:?}", key); + trace!("Found hash in shared cache: {:?}", HexDisplay::from(&key)); return Ok(entry) } } - trace!("Cache hash miss: {:?}", key); + trace!("Cache hash miss: {:?}", HexDisplay::from(&key)); let hash = self.state.storage_hash(key)?; RwLockUpgradableReadGuard::upgrade(local_cache).hashes.insert(key.to_vec(), hash.clone()); Ok(hash) @@ -733,6 +727,7 @@ mod tests { #[test] fn fix_storage_mismatch_issue() { + let _ = ::env_logger::try_init(); let root_parent = H256::random(); let key = H256::random()[..].to_vec(); @@ -740,7 +735,7 @@ mod tests { let h0 = H256::random(); let h1 = H256::random(); - let shared = new_shared_cache::(256*1024, (0,1)); + let shared = new_shared_cache::(256*1024, (0, 1)); let mut s = CachingState::new(InMemory::::default(), shared.clone(), Some(root_parent.clone())); s.cache.sync_cache(&[], &[], vec![(key.clone(), Some(vec![2]))], vec![], Some(h0.clone()), Some(0), || true); @@ -766,9 +761,6 @@ mod tests { // New value is propagated. s.cache.sync_cache(&[], &[], vec![], vec![], None, None, || true); - // Wrongly get new value 42. It should instead return None or old value 3 at block h1. - let mut s = CachingState::new(InMemory::::default(), shared.clone(), Some(h1.clone())); - s.cache.sync_cache(&[], &[], vec![], vec![], None, None, || false); - assert_eq!(s.storage(&key).unwrap(), Some(vec![42])); - } + let s = CachingState::new(InMemory::::default(), shared.clone(), Some(h1.clone())); + assert_eq!(s.storage(&key).unwrap(), None);} } From f443aa7238d497b5b3d99e16a2d5eb03abb44e73 Mon Sep 17 00:00:00 2001 From: Marcio Diaz Date: Fri, 1 Nov 2019 15:49:24 +0100 Subject: [PATCH 4/4] Fix indentation --- core/client/db/src/storage_cache.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/client/db/src/storage_cache.rs b/core/client/db/src/storage_cache.rs index f7874ee230e9a..8c81e44ba6bf2 100644 --- a/core/client/db/src/storage_cache.rs +++ b/core/client/db/src/storage_cache.rs @@ -762,5 +762,6 @@ mod tests { s.cache.sync_cache(&[], &[], vec![], vec![], None, None, || true); let s = CachingState::new(InMemory::::default(), shared.clone(), Some(h1.clone())); - assert_eq!(s.storage(&key).unwrap(), None);} + assert_eq!(s.storage(&key).unwrap(), None); + } }