From 9ae3a1ce15dfaaf4187152d90f39d7be66b51bbf Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Fri, 31 Jul 2020 14:32:13 +0200 Subject: [PATCH] Allow blacklisting blocks from being finalized again after block revert (#6301) * Allow blacklisting blocks from being finalized again after block revert * Use BlockRules for storing unfinalized and add have_state_at in revert * Move finalization_check in finalize_block upward * Directly mark finalization blacklist as badblocks * Remove obselete comment --- Cargo.lock | 1 + client/api/src/backend.rs | 18 +++-- client/api/src/in_mem.rs | 11 ++-- client/db/Cargo.toml | 1 + client/db/src/lib.rs | 69 ++++++++++++++------ client/light/src/backend.rs | 9 ++- client/service/src/chain_ops/revert_chain.rs | 4 +- client/service/src/client/block_rules.rs | 9 ++- client/service/src/client/client.rs | 33 ++++++---- 9 files changed, 105 insertions(+), 50 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 525ea78d9d4e2..b86507cf072f2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6309,6 +6309,7 @@ dependencies = [ "sc-client-api", "sc-executor", "sc-state-db", + "sp-arithmetic", "sp-blockchain", "sp-consensus", "sp-core", diff --git a/client/api/src/backend.rs b/client/api/src/backend.rs index 9482a6118d71a..efc5ca4ee8ca0 100644 --- a/client/api/src/backend.rs +++ b/client/api/src/backend.rs @@ -19,7 +19,7 @@ //! Substrate Client data backend use std::sync::Arc; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use sp_core::ChangesTrieConfigurationRange; use sp_core::offchain::{OffchainStorage,storage::OffchainOverlayedChanges}; use sp_runtime::{generic::BlockId, Justification, Storage}; @@ -418,7 +418,10 @@ pub trait Backend: AuxStore + Send + Sync { ) -> sp_blockchain::Result<()>; /// Commit block insertion. - fn commit_operation(&self, transaction: Self::BlockImportOperation) -> sp_blockchain::Result<()>; + fn commit_operation( + &self, + transaction: Self::BlockImportOperation, + ) -> sp_blockchain::Result<()>; /// Finalize block with given Id. /// @@ -449,16 +452,17 @@ pub trait Backend: AuxStore + Send + Sync { /// Returns state backend with post-state of given block. fn state_at(&self, block: BlockId) -> sp_blockchain::Result; - /// Attempts to revert the chain by `n` blocks. If `revert_finalized` is set - /// it will attempt to revert past any finalized block, this is unsafe and - /// can potentially leave the node in an inconsistent state. + /// Attempts to revert the chain by `n` blocks. If `revert_finalized` is set it will attempt to + /// revert past any finalized block, this is unsafe and can potentially leave the node in an + /// inconsistent state. /// - /// Returns the number of blocks that were successfully reverted. + /// Returns the number of blocks that were successfully reverted and the list of finalized + /// blocks that has been reverted. fn revert( &self, n: NumberFor, revert_finalized: bool, - ) -> sp_blockchain::Result>; + ) -> sp_blockchain::Result<(NumberFor, HashSet)>; /// Insert auxiliary data into key-value store. fn insert_aux< diff --git a/client/api/src/in_mem.rs b/client/api/src/in_mem.rs index 7d27326678f58..306c3c2b2f10c 100644 --- a/client/api/src/in_mem.rs +++ b/client/api/src/in_mem.rs @@ -18,7 +18,7 @@ //! In memory client backend -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::ptr; use std::sync::Arc; use parking_lot::RwLock; @@ -646,7 +646,10 @@ impl backend::Backend for Backend where Block::Hash Ok(()) } - fn commit_operation(&self, operation: Self::BlockImportOperation) -> sp_blockchain::Result<()> { + fn commit_operation( + &self, + operation: Self::BlockImportOperation, + ) -> sp_blockchain::Result<()> { if !operation.finalized_blocks.is_empty() { for (block, justification) in operation.finalized_blocks { self.blockchain.finalize_header(block, justification)?; @@ -722,8 +725,8 @@ impl backend::Backend for Backend where Block::Hash &self, _n: NumberFor, _revert_finalized: bool, - ) -> sp_blockchain::Result> { - Ok(Zero::zero()) + ) -> sp_blockchain::Result<(NumberFor, HashSet)> { + Ok((Zero::zero(), HashSet::new())) } fn get_import_lock(&self) -> &RwLock<()> { diff --git a/client/db/Cargo.toml b/client/db/Cargo.toml index c26f7121493cf..50e14fcaae602 100644 --- a/client/db/Cargo.toml +++ b/client/db/Cargo.toml @@ -24,6 +24,7 @@ codec = { package = "parity-scale-codec", version = "1.3.4", features = ["derive blake2-rfc = "0.2.18" sc-client-api = { version = "2.0.0-rc5", path = "../api" } +sp-arithmetic = { version = "2.0.0-rc5", path = "../../primitives/arithmetic" } sp-core = { version = "2.0.0-rc5", path = "../../primitives/core" } sp-runtime = { version = "2.0.0-rc5", path = "../../primitives/runtime" } sp-state-machine = { version = "0.8.0-rc5", path = "../../primitives/state-machine" } diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 086db73728f1b..d854c80bf3535 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -50,8 +50,7 @@ mod subdb; use std::sync::Arc; use std::path::{Path, PathBuf}; use std::io; -use std::collections::HashMap; - +use std::collections::{HashMap, HashSet}; use sc_client_api::{ UsageInfo, MemoryInfo, IoInfo, MemorySize, @@ -70,6 +69,7 @@ use parking_lot::RwLock; use sp_core::ChangesTrieConfiguration; use sp_core::offchain::storage::{OffchainOverlayedChange, OffchainOverlayedChanges}; use sp_core::storage::{well_known_keys, ChildInfo}; +use sp_arithmetic::traits::Saturating; use sp_runtime::{generic::BlockId, Justification, Storage}; use sp_runtime::traits::{ Block as BlockT, Header as HeaderT, NumberFor, Zero, One, SaturatedConversion, HashFor, @@ -962,6 +962,7 @@ impl Backend { // TODO: ensure best chain contains this block. let number = *header.number(); self.ensure_sequential_finalization(header, last_finalized)?; + self.note_finalized( transaction, false, @@ -1015,9 +1016,10 @@ impl Backend { Ok(()) } - fn try_commit_operation(&self, mut operation: BlockImportOperation) - -> ClientResult<()> - { + fn try_commit_operation( + &self, + mut operation: BlockImportOperation, + ) -> ClientResult<()> { let mut transaction = Transaction::new(); let mut finalization_displaced_leaves = None; @@ -1404,7 +1406,10 @@ impl sc_client_api::backend::Backend for Backend { Ok(()) } - fn commit_operation(&self, operation: Self::BlockImportOperation) -> ClientResult<()> { + fn commit_operation( + &self, + operation: Self::BlockImportOperation, + ) -> ClientResult<()> { let usage = operation.old_state.usage_info(); self.state_usage.merge_sm(usage); @@ -1420,9 +1425,11 @@ impl sc_client_api::backend::Backend for Backend { } } - fn finalize_block(&self, block: BlockId, justification: Option) - -> ClientResult<()> - { + fn finalize_block( + &self, + block: BlockId, + justification: Option, + ) -> ClientResult<()> { let mut transaction = Transaction::new(); let hash = self.blockchain.expect_block_hash_from_id(&block)?; let header = self.blockchain.expect_header(block)?; @@ -1488,7 +1495,13 @@ impl sc_client_api::backend::Backend for Backend { }) } - fn revert(&self, n: NumberFor, revert_finalized: bool) -> ClientResult> { + fn revert( + &self, + n: NumberFor, + revert_finalized: bool, + ) -> ClientResult<(NumberFor, HashSet)> { + let mut reverted_finalized = HashSet::new(); + let mut best_number = self.blockchain.info().best_number; let mut best_hash = self.blockchain.info().best_hash; @@ -1507,18 +1520,28 @@ impl sc_client_api::backend::Backend for Backend { return Ok(c.saturated_into::>()) } let mut transaction = Transaction::new(); + let removed_number = best_number; + let removed = self.blockchain.header(BlockId::Number(best_number))?.ok_or_else( + || sp_blockchain::Error::UnknownBlock( + format!("Error reverting to {}. Block hash not found.", best_number)))?; + let removed_hash = removed.hash(); + + let prev_number = best_number.saturating_sub(One::one()); + let prev_hash = self.blockchain.hash(prev_number)?.ok_or_else( + || sp_blockchain::Error::UnknownBlock( + format!("Error reverting to {}. Block hash not found.", best_number)) + )?; + + if !self.have_state_at(&prev_hash, prev_number) { + return Ok(c.saturated_into::>()) + } + match self.storage.state_db.revert_one() { Some(commit) => { apply_state_commit(&mut transaction, commit); - let removed_number = best_number; - let removed = self.blockchain.header(BlockId::Number(best_number))?.ok_or_else( - || sp_blockchain::Error::UnknownBlock( - format!("Error reverting to {}. Block hash not found.", best_number)))?; - best_number -= One::one(); // prev block - best_hash = self.blockchain.hash(best_number)?.ok_or_else( - || sp_blockchain::Error::UnknownBlock( - format!("Error reverting to {}. Block hash not found.", best_number)))?; + best_number = prev_number; + best_hash = prev_hash; let update_finalized = best_number < finalized; @@ -1531,7 +1554,12 @@ impl sc_client_api::backend::Backend for Backend { ), )?; if update_finalized { - transaction.set_from_vec(columns::META, meta_keys::FINALIZED_BLOCK, key.clone()); + transaction.set_from_vec( + columns::META, + meta_keys::FINALIZED_BLOCK, + key.clone() + ); + reverted_finalized.insert(removed_hash); } transaction.set_from_vec(columns::META, meta_keys::BEST_BLOCK, key); transaction.remove(columns::KEY_LOOKUP, removed.hash().as_ref()); @@ -1562,7 +1590,7 @@ impl sc_client_api::backend::Backend for Backend { revert_leaves()?; - Ok(reverted) + Ok((reverted, reverted_finalized)) } fn blockchain(&self) -> &BlockchainDb { @@ -1986,7 +2014,6 @@ pub(crate) mod tests { backend.commit_operation(op).unwrap(); - assert!(backend.storage.db.get( columns::STATE, &sp_trie::prefixed_key::(&key, EMPTY_PREFIX) diff --git a/client/light/src/backend.rs b/client/light/src/backend.rs index 2cf994d3f5993..be7953e528bd8 100644 --- a/client/light/src/backend.rs +++ b/client/light/src/backend.rs @@ -19,7 +19,7 @@ //! Light client backend. Only stores headers and justifications of blocks. //! Everything else is requested from full nodes on demand. -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::sync::Arc; use parking_lot::RwLock; @@ -146,7 +146,10 @@ impl ClientBackend for Backend> Ok(()) } - fn commit_operation(&self, mut operation: Self::BlockImportOperation) -> ClientResult<()> { + fn commit_operation( + &self, + mut operation: Self::BlockImportOperation, + ) -> ClientResult<()> { if !operation.finalized_blocks.is_empty() { for block in operation.finalized_blocks { self.blockchain.storage().finalize_header(block)?; @@ -231,7 +234,7 @@ impl ClientBackend for Backend> &self, _n: NumberFor, _revert_finalized: bool, - ) -> ClientResult> { + ) -> ClientResult<(NumberFor, HashSet)> { Err(ClientError::NotAvailableOnLightClient) } diff --git a/client/service/src/chain_ops/revert_chain.rs b/client/service/src/chain_ops/revert_chain.rs index 129aea0408685..eaee2c03f9b31 100644 --- a/client/service/src/chain_ops/revert_chain.rs +++ b/client/service/src/chain_ops/revert_chain.rs @@ -34,10 +34,10 @@ where let reverted = backend.revert(blocks, false)?; let info = client.usage_info().chain; - if reverted.is_zero() { + if reverted.0.is_zero() { info!("There aren't any non-finalized blocks to revert."); } else { - info!("Reverted {} blocks. Best: #{} ({})", reverted, info.best_number, info.best_hash); + info!("Reverted {} blocks. Best: #{} ({})", reverted.0, info.best_number, info.best_hash); } Ok(()) } diff --git a/client/service/src/client/block_rules.rs b/client/service/src/client/block_rules.rs index e862379a56414..be84614c2a590 100644 --- a/client/service/src/client/block_rules.rs +++ b/client/service/src/client/block_rules.rs @@ -30,7 +30,7 @@ use sc_client_api::{ForkBlocks, BadBlocks}; pub enum LookupResult { /// Specification rules do not contain any special rules about this block NotSpecial, - /// The bock is known to be bad and should not be imported + /// The block is known to be bad and should not be imported KnownBad, /// There is a specified canonical block hash for the given height Expected(B::Hash) @@ -57,6 +57,11 @@ impl BlockRules { } } + /// Mark a new block as bad. + pub fn mark_bad(&mut self, hash: B::Hash) { + self.bad.insert(hash); + } + /// Check if there's any rule affecting the given block. pub fn lookup(&self, number: NumberFor, hash: &B::Hash) -> LookupResult { if let Some(hash_for_height) = self.forks.get(&number) { @@ -66,7 +71,7 @@ impl BlockRules { } if self.bad.contains(hash) { - return LookupResult::KnownBad; + return LookupResult::KnownBad } LookupResult::NotSpecial diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index b152415a4a89d..d0859f4ee0392 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -1054,20 +1054,31 @@ impl Client where /// reverted past the last finalized block. Returns the number of blocks /// that were successfully reverted. pub fn revert(&self, n: NumberFor) -> sp_blockchain::Result> { - Ok(self.backend.revert(n, false)?) + let (number, _) = self.backend.revert(n, false)?; + Ok(number) } - /// Attempts to revert the chain by `n` blocks disregarding finality. This - /// method will revert any finalized blocks as requested and can potentially - /// leave the node in an inconsistent state. Other modules in the system that - /// persist data and that rely on finality (e.g. consensus parts) will be - /// unaffected by the revert. Use this method with caution and making sure - /// that no other data needs to be reverted for consistency aside from the - /// block data. + /// Attempts to revert the chain by `n` blocks disregarding finality. This method will revert + /// any finalized blocks as requested and can potentially leave the node in an inconsistent + /// state. Other modules in the system that persist data and that rely on finality + /// (e.g. consensus parts) will be unaffected by the revert. Use this method with caution and + /// making sure that no other data needs to be reverted for consistency aside from the block + /// data. If `blacklist` is set to true, will also blacklist reverted blocks from finalizing + /// again. The blacklist is reset upon client restart. /// /// Returns the number of blocks that were successfully reverted. - pub fn unsafe_revert(&self, n: NumberFor) -> sp_blockchain::Result> { - Ok(self.backend.revert(n, true)?) + pub fn unsafe_revert( + &mut self, + n: NumberFor, + blacklist: bool, + ) -> sp_blockchain::Result> { + let (number, reverted) = self.backend.revert(n, true)?; + if blacklist { + for b in reverted { + self.block_rules.mark_bad(b); + } + } + Ok(number) } /// Get blockchain info. @@ -1921,7 +1932,7 @@ impl BlockBackend for Client fn block_hash(&self, number: NumberFor) -> sp_blockchain::Result> { self.backend.blockchain().hash(number) - } + } } impl backend::AuxStore for Client