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

substrate/Cargo.lock

@@ -6309,6 +6309,7 @@ dependencies = [
  "sc-client-api",
  "sc-executor",
  "sc-state-db",
+ "sp-arithmetic",
  "sp-blockchain",
  "sp-consensus",
  "sp-core",

substrate/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<Block: BlockT>: 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<Block: BlockT>: AuxStore + Send + Sync {
 	/// Returns state backend with post-state of given block.
 	fn state_at(&self, block: BlockId<Block>) -> sp_blockchain::Result<Self::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.
+	/// 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<Block>,
 		revert_finalized: bool,
-	) -> sp_blockchain::Result<NumberFor<Block>>;
+	) -> sp_blockchain::Result<(NumberFor<Block>, HashSet<Block::Hash>)>;
 
 	/// Insert auxiliary data into key-value store.
 	fn insert_aux<

substrate/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<Block: BlockT> backend::Backend<Block> for Backend<Block> 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<Block: BlockT> backend::Backend<Block> for Backend<Block> where Block::Hash
 		&self,
 		_n: NumberFor<Block>,
 		_revert_finalized: bool,
-	) -> sp_blockchain::Result<NumberFor<Block>> {
-		Ok(Zero::zero())
+	) -> sp_blockchain::Result<(NumberFor<Block>, HashSet<Block::Hash>)> {
+		Ok((Zero::zero(), HashSet::new()))
 	}
 
 	fn get_import_lock(&self) -> &RwLock<()> {

substrate/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" }

substrate/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<Block: BlockT> Backend<Block> {
 		// 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<Block: BlockT> Backend<Block> {
 		Ok(())
 	}
 
-	fn try_commit_operation(&self, mut operation: BlockImportOperation<Block>)
-		-> ClientResult<()>
-	{
+	fn try_commit_operation(
+		&self,
+		mut operation: BlockImportOperation<Block>,
+	) -> ClientResult<()> {
 		let mut transaction = Transaction::new();
 		let mut finalization_displaced_leaves = None;
 
@@ -1404,7 +1406,10 @@ impl<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
 		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<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
 		}
 	}
 
-	fn finalize_block(&self, block: BlockId<Block>, justification: Option<Justification>)
-		-> ClientResult<()>
-	{
+	fn finalize_block(
+		&self,
+		block: BlockId<Block>,
+		justification: Option<Justification>,
+	) -> 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<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
 		})
 	}
 
-	fn revert(&self, n: NumberFor<Block>, revert_finalized: bool) -> ClientResult<NumberFor<Block>> {
+	fn revert(
+		&self,
+		n: NumberFor<Block>,
+		revert_finalized: bool,
+	) -> ClientResult<(NumberFor<Block>, HashSet<Block::Hash>)> {
+		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<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
 					return Ok(c.saturated_into::<NumberFor<Block>>())
 				}
 				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::<NumberFor<Block>>())
+				}
+
 				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<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
 							),
 						)?;
 						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<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
 
 		revert_leaves()?;
 
-		Ok(reverted)
+		Ok((reverted, reverted_finalized))
 	}
 
 	fn blockchain(&self) -> &BlockchainDb<Block> {
@@ -1986,7 +2014,6 @@ pub(crate) mod tests {
 
 			backend.commit_operation(op).unwrap();
 
-
 			assert!(backend.storage.db.get(
 				columns::STATE,
 				&sp_trie::prefixed_key::<BlakeTwo256>(&key, EMPTY_PREFIX)

substrate/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<S, Block> ClientBackend<Block> for Backend<S, HashFor<Block>>
 		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<S, Block> ClientBackend<Block> for Backend<S, HashFor<Block>>
 		&self,
 		_n: NumberFor<Block>,
 		_revert_finalized: bool,
-	) -> ClientResult<NumberFor<Block>> {
+	) -> ClientResult<(NumberFor<Block>, HashSet<Block::Hash>)> {
 		Err(ClientError::NotAvailableOnLightClient)
 	}
 

substrate/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(())
 }

substrate/client/service/src/client/block_rules.rs

@@ -30,7 +30,7 @@ use sc_client_api::{ForkBlocks, BadBlocks};
 pub enum LookupResult<B: BlockT> {
 	/// 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<B: BlockT> BlockRules<B> {
 		}
 	}
 
+	/// 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<B>, hash: &B::Hash) -> LookupResult<B> {
 		if let Some(hash_for_height) = self.forks.get(&number) {
@@ -66,7 +71,7 @@ impl<B: BlockT> BlockRules<B> {
 		}
 
 		if self.bad.contains(hash) {
-			return LookupResult::KnownBad;
+			return LookupResult::KnownBad
 		}
 
 		LookupResult::NotSpecial

substrate/client/service/src/client/client.rs

@@ -1054,20 +1054,31 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 	/// reverted past the last finalized block. Returns the number of blocks
 	/// that were successfully reverted.
 	pub fn revert(&self, n: NumberFor<Block>) -> sp_blockchain::Result<NumberFor<Block>> {
-		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<Block>) -> sp_blockchain::Result<NumberFor<Block>> {
-		Ok(self.backend.revert(n, true)?)
+	pub fn unsafe_revert(
+		&mut self,
+		n: NumberFor<Block>,
+		blacklist: bool,
+	) -> sp_blockchain::Result<NumberFor<Block>> {
+		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<B, E, Block, RA> BlockBackend<Block> for Client<B, E, Block, RA>
 
 	fn block_hash(&self, number: NumberFor<Block>) -> sp_blockchain::Result<Option<Block::Hash>> {
 		self.backend.blockchain().hash(number)
-	}	
+	}
 }
 
 impl<B, E, Block, RA> backend::AuxStore for Client<B, E, Block, RA>