From 6d860c69365afb360c2fe917ea557c1a1f38a4a7 Mon Sep 17 00:00:00 2001 From: arkpar Date: Mon, 29 Jul 2019 07:31:07 +0200 Subject: [PATCH 1/6] Reorg test --- core/client/src/client.rs | 65 +++++++++++++++++++++++++++++++++++++++ core/state-db/src/lib.rs | 7 +++-- 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/core/client/src/client.rs b/core/client/src/client.rs index 6decdfc9cacdd..fe8b3a3334f8f 100644 --- a/core/client/src/client.rs +++ b/core/client/src/client.rs @@ -2690,4 +2690,69 @@ pub(crate) mod tests { let id = BlockId::::Number(72340207214430721); client.header(&id).expect_err("invalid block number overflows u32"); } + + #[test] + fn state_reverted_on_reorg() { + use test_client::blockchain::HeaderBackend; + + let client = test_client::new(); + + // G -> A1 -> A2 + // \ + // -> B1 + let alice = blake2_256(&runtime::system::balance_of_key(AccountKeyring::Alice.into())).to_vec(); + let current_balance = || + u64::decode( + &mut client.storage(&BlockId::number(client.current_height()), &StorageKey(alice.clone())) + .unwrap().unwrap().0.as_slice() + ).unwrap(); + + let mut a1 = client.new_block_at(&BlockId::Number(0), Default::default()).unwrap(); + a1.push_transfer(Transfer { + from: AccountKeyring::Alice.into(), + to: AccountKeyring::Bob.into(), + amount: 10, + nonce: 0, + }).unwrap(); + let a1 = a1.bake().unwrap(); + client.import(BlockOrigin::Own, a1.clone()).unwrap(); + + let mut a2 = client.new_block_at(&BlockId::Hash(a1.hash()), Default::default()).unwrap(); + a2.push_transfer(Transfer { + from: AccountKeyring::Alice.into(), + to: AccountKeyring::Charlie.into(), + amount: 10, + nonce: 1, + }).unwrap(); + let a2 = a2.bake().unwrap(); + client.import(BlockOrigin::Own, a2.clone()).unwrap(); + + let mut b1 = client.new_block_at(&BlockId::Number(0), Default::default()).unwrap(); + // needed to make sure B1 gets a different hash from A1 + b1.push_transfer(Transfer { + from: AccountKeyring::Alice.into(), + to: AccountKeyring::Ferdie.into(), + amount: 50, + nonce: 0, + }).unwrap(); + let b1 = b1.bake().unwrap(); + + #[allow(deprecated)] + let blockchain = client.backend().blockchain(); + + // A2 is the current best since it's the longest chain + assert_eq!( + blockchain.info().best_hash, + a2.hash(), + ); + + //assert_eq!(980, current_balance()); + + // importing B1 as finalized should trigger a re-org and set it as new best + let justification = vec![1, 2, 3]; + client.import_justified(BlockOrigin::Own, b1.clone(), justification).unwrap(); + assert_eq!( blockchain.info().best_hash, b1.hash()); + assert_eq!( blockchain.info().finalized_hash, b1.hash()); + assert_eq!(950, current_balance()); + } } diff --git a/core/state-db/src/lib.rs b/core/state-db/src/lib.rs index 8986dda32d7ca..4434fa4ef6298 100644 --- a/core/state-db/src/lib.rs +++ b/core/state-db/src/lib.rs @@ -292,8 +292,11 @@ impl StateDbSync { } pub fn pin(&mut self, hash: &BlockHash) { - trace!(target: "state-db", "Pinned block: {:?}", hash); - *self.pinned.entry(hash.clone()).or_default() += 1; + let refs = self.pinned.entry(hash.clone()).or_default(); + if *refs == 0 { + trace!(target: "state-db", "Pinned block: {:?}", hash); + } + *refs += 1 } pub fn unpin(&mut self, hash: &BlockHash) { From 8f6e0c721f7a6490f46d651327e8077e55263beb Mon Sep 17 00:00:00 2001 From: arkpar Date: Mon, 29 Jul 2019 23:49:09 +0200 Subject: [PATCH 2/6] Fixed informant misreporting reorgs --- core/cli/src/informant.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/core/cli/src/informant.rs b/core/cli/src/informant.rs index ca4be4bd1be95..b13394a7e9920 100644 --- a/core/cli/src/informant.rs +++ b/core/cli/src/informant.rs @@ -49,15 +49,15 @@ where C: Components { }); let client = service.client(); - let mut last = { + let mut last_best = { let info = client.info(); Some((info.chain.best_number, info.chain.best_hash)) }; let display_block_import = client.import_notification_stream().map(|v| Ok::<_, ()>(v)).compat().for_each(move |n| { // detect and log reorganizations. - if let Some((ref last_num, ref last_hash)) = last { - if n.header.parent_hash() != last_hash { + if let Some((ref last_num, ref last_hash)) = last_best { + if n.header.parent_hash() != last_hash && n.is_new_best { let tree_route = ::client::blockchain::tree_route( #[allow(deprecated)] client.backend().blockchain(), @@ -78,7 +78,9 @@ where C: Components { } } - last = Some((n.header.number().clone(), n.hash.clone())); + if n.is_new_best { + last_best = Some((n.header.number().clone(), n.hash.clone())); + } info!(target: "substrate", "Imported #{} ({})", n.header.number(), n.hash); Ok(()) From 54d62e3687ddbfc46d312bb2062297cf6711c48e Mon Sep 17 00:00:00 2001 From: arkpar Date: Mon, 29 Jul 2019 23:52:56 +0200 Subject: [PATCH 3/6] Update cache when reorg is caused by applying finality --- Cargo.lock | 1 + core/client/Cargo.toml | 1 + core/client/db/src/lib.rs | 13 +++- core/client/db/src/storage_cache.rs | 117 +++++++++++++++------------- core/client/src/client.rs | 48 ++++-------- core/test-client/src/client_ext.rs | 22 ++++++ 6 files changed, 115 insertions(+), 87 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e1d9a497939b7..b83554ab87173 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4224,6 +4224,7 @@ name = "substrate-client" version = "2.0.0" dependencies = [ "derive_more 0.14.1 (registry+https://github.com/rust-lang/crates.io-index)", + "env_logger 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)", "fnv 1.0.6 (registry+https://github.com/rust-lang/crates.io-index)", "futures-preview 0.3.0-alpha.17 (registry+https://github.com/rust-lang/crates.io-index)", "hash-db 0.14.0 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/core/client/Cargo.toml b/core/client/Cargo.toml index f140b2f343f74..bffe2ae7084b2 100644 --- a/core/client/Cargo.toml +++ b/core/client/Cargo.toml @@ -28,6 +28,7 @@ inherents = { package = "substrate-inherents", path = "../inherents", default-fe sr-api-macros = { path = "../sr-api-macros" } [dev-dependencies] +env_logger = "0.6" test-client = { package = "substrate-test-runtime-client", path = "../test-runtime/client" } kvdb-memorydb = { git = "https://github.com/paritytech/parity-common", rev="b0317f649ab2c665b7987b8475878fc4d2e1f81d" } diff --git a/core/client/db/src/lib.rs b/core/client/db/src/lib.rs index e73d3df62c9c0..2ed6b60bcdacb 100644 --- a/core/client/db/src/lib.rs +++ b/core/client/db/src/lib.rs @@ -1108,21 +1108,24 @@ impl> Backend { None }; - if let Some(set_head) = operation.set_head { + let cache_update = if let Some(set_head) = operation.set_head { if let Some(header) = ::client::blockchain::HeaderBackend::header(&self.blockchain, set_head)? { let number = header.number(); let hash = header.hash(); - self.set_head_with_transaction( + let (enacted, retracted) = self.set_head_with_transaction( &mut transaction, hash.clone(), (number.clone(), hash.clone()) )?; meta_updates.push((hash, *number, true, false)); + Some((enacted, retracted)) } else { return Err(client::error::Error::UnknownBlock(format!("Cannot set head {:?}", set_head))) } - } + } else { + None + }; let write_result = self.storage.db.write(transaction).map_err(db_err); @@ -1152,6 +1155,10 @@ impl> Backend { ); } + if let Some((enacted, retracted)) = cache_update { + self.shared_cache.lock().sync(&enacted, &retracted); + } + for (hash, number, is_best, is_finalized) in meta_updates { self.blockchain.update_meta(hash, number, is_best, is_finalized); } diff --git a/core/client/db/src/storage_cache.rs b/core/client/db/src/storage_cache.rs index 7df1472ece038..f3eaf33a832f0 100644 --- a/core/client/db/src/storage_cache.rs +++ b/core/client/db/src/storage_cache.rs @@ -151,6 +151,65 @@ impl Cache { + self.lru_child_storage.used_size() // ignore small hashes storage and self.lru_hashes.used_size() } + + /// Synchronize the shared cache with the best block state. + /// This function updates the shared cache by removing entries + /// that are invalidated by chain reorganization. It should be + /// be called when chain reorg rappens without importing a new block. + pub fn sync(&mut self, enacted: &[B::Hash], retracted: &[B::Hash]) { + trace!("Syncing shared cache, enacted = {:?}, retracted = {:?}", enacted, retracted); + + // Purge changes from re-enacted and retracted blocks. + // Filter out commiting block if any. + let mut clear = false; + for block in enacted { + clear = clear || { + if let Some(ref mut m) = self.modifications.iter_mut().find(|m| &m.hash == block) { + trace!("Reverting enacted block {:?}", block); + m.is_canon = true; + for a in &m.storage { + trace!("Reverting enacted key {:?}", a); + self.lru_storage.remove(a); + } + for a in &m.child_storage { + trace!("Reverting enacted child key {:?}", a); + self.lru_child_storage.remove(a); + } + false + } else { + true + } + }; + } + + for block in retracted { + clear = clear || { + if let Some(ref mut m) = self.modifications.iter_mut().find(|m| &m.hash == block) { + trace!("Retracting block {:?}", block); + m.is_canon = false; + for a in &m.storage { + trace!("Retracted key {:?}", a); + self.lru_storage.remove(a); + } + for a in &m.child_storage { + trace!("Retracted child key {:?}", a); + self.lru_child_storage.remove(a); + } + false + } else { + true + } + }; + } + if clear { + // We don't know anything about the block; clear everything + trace!("Wiping cache"); + self.lru_storage.clear(); + self.lru_child_storage.clear(); + self.lru_hashes.clear(); + self.modifications.clear(); + } + } } pub type SharedCache = Arc>>; @@ -247,58 +306,12 @@ impl CacheChanges { let is_best = is_best(); trace!("Syncing cache, id = (#{:?}, {:?}), parent={:?}, best={}", commit_number, commit_hash, self.parent_hash, is_best); let cache = &mut *cache; - - // Purge changes from re-enacted and retracted blocks. - // Filter out commiting block if any. - let mut clear = false; - for block in enacted.iter().filter(|h| commit_hash.as_ref().map_or(true, |p| *h != p)) { - clear = clear || { - if let Some(ref mut m) = cache.modifications.iter_mut().find(|m| &m.hash == block) { - trace!("Reverting enacted block {:?}", block); - m.is_canon = true; - for a in &m.storage { - trace!("Reverting enacted key {:?}", a); - cache.lru_storage.remove(a); - } - for a in &m.child_storage { - trace!("Reverting enacted child key {:?}", a); - cache.lru_child_storage.remove(a); - } - false - } else { - true - } - }; - } - - for block in retracted { - clear = clear || { - if let Some(ref mut m) = cache.modifications.iter_mut().find(|m| &m.hash == block) { - trace!("Retracting block {:?}", block); - m.is_canon = false; - for a in &m.storage { - trace!("Retracted key {:?}", a); - cache.lru_storage.remove(a); - } - for a in &m.child_storage { - trace!("Retracted child key {:?}", a); - cache.lru_child_storage.remove(a); - } - false - } else { - true - } - }; - } - if clear { - // We don't know anything about the block; clear everything - trace!("Wiping cache"); - cache.lru_storage.clear(); - cache.lru_child_storage.clear(); - cache.lru_hashes.clear(); - cache.modifications.clear(); - } - + let enacted: Vec<_> = enacted + .iter() + .filter(|h| commit_hash.as_ref().map_or(true, |p| *h != p)) + .cloned() + .collect(); + cache.sync(&enacted, retracted); // 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) diff --git a/core/client/src/client.rs b/core/client/src/client.rs index fe8b3a3334f8f..b14294667a479 100644 --- a/core/client/src/client.rs +++ b/core/client/src/client.rs @@ -2693,13 +2693,9 @@ pub(crate) mod tests { #[test] fn state_reverted_on_reorg() { - use test_client::blockchain::HeaderBackend; - + let _ = env_logger::try_init(); let client = test_client::new(); - // G -> A1 -> A2 - // \ - // -> B1 let alice = blake2_256(&runtime::system::balance_of_key(AccountKeyring::Alice.into())).to_vec(); let current_balance = || u64::decode( @@ -2707,6 +2703,9 @@ pub(crate) mod tests { .unwrap().unwrap().0.as_slice() ).unwrap(); + // G -> A1 -> A2 + // \ + // -> B1 let mut a1 = client.new_block_at(&BlockId::Number(0), Default::default()).unwrap(); a1.push_transfer(Transfer { from: AccountKeyring::Alice.into(), @@ -2717,18 +2716,7 @@ pub(crate) mod tests { let a1 = a1.bake().unwrap(); client.import(BlockOrigin::Own, a1.clone()).unwrap(); - let mut a2 = client.new_block_at(&BlockId::Hash(a1.hash()), Default::default()).unwrap(); - a2.push_transfer(Transfer { - from: AccountKeyring::Alice.into(), - to: AccountKeyring::Charlie.into(), - amount: 10, - nonce: 1, - }).unwrap(); - let a2 = a2.bake().unwrap(); - client.import(BlockOrigin::Own, a2.clone()).unwrap(); - let mut b1 = client.new_block_at(&BlockId::Number(0), Default::default()).unwrap(); - // needed to make sure B1 gets a different hash from A1 b1.push_transfer(Transfer { from: AccountKeyring::Alice.into(), to: AccountKeyring::Ferdie.into(), @@ -2736,23 +2724,19 @@ pub(crate) mod tests { nonce: 0, }).unwrap(); let b1 = b1.bake().unwrap(); + // Reorg to B1 + client.import_as_best(BlockOrigin::Own, b1.clone()).unwrap(); - #[allow(deprecated)] - let blockchain = client.backend().blockchain(); - - // A2 is the current best since it's the longest chain - assert_eq!( - blockchain.info().best_hash, - a2.hash(), - ); - - //assert_eq!(980, current_balance()); - - // importing B1 as finalized should trigger a re-org and set it as new best - let justification = vec![1, 2, 3]; - client.import_justified(BlockOrigin::Own, b1.clone(), justification).unwrap(); - assert_eq!( blockchain.info().best_hash, b1.hash()); - assert_eq!( blockchain.info().finalized_hash, b1.hash()); assert_eq!(950, current_balance()); + let mut a2 = client.new_block_at(&BlockId::Hash(a1.hash()), Default::default()).unwrap(); + a2.push_transfer(Transfer { + from: AccountKeyring::Alice.into(), + to: AccountKeyring::Charlie.into(), + amount: 10, + nonce: 1, + }).unwrap(); + // Re-org to A2 + client.import_as_best(BlockOrigin::Own, a2.bake().unwrap()).unwrap(); + assert_eq!(980, current_balance()); } } diff --git a/core/test-client/src/client_ext.rs b/core/test-client/src/client_ext.rs index cb0d5c1736e1d..35fe24f725d0b 100644 --- a/core/test-client/src/client_ext.rs +++ b/core/test-client/src/client_ext.rs @@ -34,6 +34,10 @@ pub trait ClientExt: Sized { fn import(&self, origin: BlockOrigin, block: Block) -> Result<(), ConsensusError>; + /// Import a block and make it our best block if possible. + fn import_as_best(&self, origin: BlockOrigin, block: Block) + -> Result<(), ConsensusError>; + /// Import block with justification, finalizes block. fn import_justified( &self, @@ -78,6 +82,24 @@ impl ClientExt for Client BlockImport::import_block(&mut (&*self), import, HashMap::new()).map(|_| ()) } + fn import_as_best(&self, origin: BlockOrigin, block: Block) + -> Result<(), ConsensusError> + { + let (header, extrinsics) = block.deconstruct(); + let import = BlockImportParams { + origin, + header, + justification: None, + post_digests: vec![], + body: Some(extrinsics), + finalized: false, + auxiliary: Vec::new(), + fork_choice: ForkChoiceStrategy::Custom(true), + }; + + BlockImport::import_block(&mut (&*self), import, HashMap::new()).map(|_| ()) + } + fn import_justified( &self, origin: BlockOrigin, From a76c68bc99698665a8f0d8f909948909f207e98c Mon Sep 17 00:00:00 2001 From: arkpar Date: Tue, 30 Jul 2019 09:33:00 +0200 Subject: [PATCH 4/6] Test for finality reorg --- core/client/src/client.rs | 40 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/core/client/src/client.rs b/core/client/src/client.rs index b14294667a479..956574a578f7f 100644 --- a/core/client/src/client.rs +++ b/core/client/src/client.rs @@ -2739,4 +2739,44 @@ pub(crate) mod tests { client.import_as_best(BlockOrigin::Own, a2.bake().unwrap()).unwrap(); assert_eq!(980, current_balance()); } + + #[test] + fn state_reverted_on_set_head() { + let _ = env_logger::try_init(); + let client = test_client::new(); + + let alice = blake2_256(&runtime::system::balance_of_key(AccountKeyring::Alice.into())).to_vec(); + let current_balance = || + u64::decode( + &mut client.storage(&BlockId::number(client.current_height()), &StorageKey(alice.clone())) + .unwrap().unwrap().0.as_slice() + ).unwrap(); + + // G -> A1 + // \ + // -> B1 + let mut a1 = client.new_block_at(&BlockId::Number(0), Default::default()).unwrap(); + a1.push_transfer(Transfer { + from: AccountKeyring::Alice.into(), + to: AccountKeyring::Bob.into(), + amount: 10, + nonce: 0, + }).unwrap(); + let a1 = a1.bake().unwrap(); + client.import(BlockOrigin::Own, a1.clone()).unwrap(); + + let mut b1 = client.new_block_at(&BlockId::Number(0), Default::default()).unwrap(); + b1.push_transfer(Transfer { + from: AccountKeyring::Alice.into(), + to: AccountKeyring::Ferdie.into(), + amount: 50, + nonce: 0, + }).unwrap(); + let b1 = b1.bake().unwrap(); + client.import(BlockOrigin::Own, b1.clone()).unwrap(); + assert_eq!(990, current_balance()); + // Set B1 as new best + client.set_head(BlockId::hash(b1.hash())).unwrap(); + assert_eq!(950, current_balance()); + } } From 491e854731fa7f73721b0943b55e28d6c527cf93 Mon Sep 17 00:00:00 2001 From: arkpar Date: Tue, 30 Jul 2019 12:58:18 +0200 Subject: [PATCH 5/6] Simplified test --- core/client/src/client.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/core/client/src/client.rs b/core/client/src/client.rs index 956574a578f7f..2b016791081ae 100644 --- a/core/client/src/client.rs +++ b/core/client/src/client.rs @@ -2696,11 +2696,9 @@ pub(crate) mod tests { let _ = env_logger::try_init(); let client = test_client::new(); - let alice = blake2_256(&runtime::system::balance_of_key(AccountKeyring::Alice.into())).to_vec(); let current_balance = || - u64::decode( - &mut client.storage(&BlockId::number(client.current_height()), &StorageKey(alice.clone())) - .unwrap().unwrap().0.as_slice() + client.runtime_api().balance_of( + &BlockId::number(client.current_height()), AccountKeyring::Alice.into() ).unwrap(); // G -> A1 -> A2 @@ -2745,11 +2743,9 @@ pub(crate) mod tests { let _ = env_logger::try_init(); let client = test_client::new(); - let alice = blake2_256(&runtime::system::balance_of_key(AccountKeyring::Alice.into())).to_vec(); let current_balance = || - u64::decode( - &mut client.storage(&BlockId::number(client.current_height()), &StorageKey(alice.clone())) - .unwrap().unwrap().0.as_slice() + client.runtime_api().balance_of( + &BlockId::number(client.current_height()), AccountKeyring::Alice.into() ).unwrap(); // G -> A1 From c8f70f1938fd6f3bf536a95ee3bbccc72e200a68 Mon Sep 17 00:00:00 2001 From: Arkadiy Paronyan Date: Tue, 30 Jul 2019 13:26:21 +0200 Subject: [PATCH 6/6] Typo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: André Silva --- core/client/db/src/storage_cache.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/client/db/src/storage_cache.rs b/core/client/db/src/storage_cache.rs index f3eaf33a832f0..a66948a0e7c45 100644 --- a/core/client/db/src/storage_cache.rs +++ b/core/client/db/src/storage_cache.rs @@ -155,7 +155,7 @@ impl Cache { /// Synchronize the shared cache with the best block state. /// This function updates the shared cache by removing entries /// that are invalidated by chain reorganization. It should be - /// be called when chain reorg rappens without importing a new block. + /// be called when chain reorg happens without importing a new block. pub fn sync(&mut self, enacted: &[B::Hash], retracted: &[B::Hash]) { trace!("Syncing shared cache, enacted = {:?}, retracted = {:?}", enacted, retracted);