client: refuse to import blocks that revert finality (#3432)

* client: don't import blocks that revert finality

* client: test import of blocks that revert finality

* client: replace tempdir with tempfile in test

substrate/Cargo.lock

@@ -4366,6 +4366,7 @@ dependencies = [
  "substrate-telemetry 2.0.0",
  "substrate-test-runtime-client 2.0.0",
  "substrate-trie 2.0.0",
+ "tempfile 3.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
 [[package]]

substrate/core/client/Cargo.toml

@@ -29,6 +29,7 @@ sr-api-macros = { path = "../sr-api-macros" }
 
 [dev-dependencies]
 env_logger = "0.6"
+tempfile = "3.1"
 test-client = { package = "substrate-test-runtime-client", path = "../test-runtime/client" }
 kvdb-memorydb = { git = "https://github.com/paritytech/parity-common", rev="b0317f649ab2c665b7987b8475878fc4d2e1f81d" }
 

substrate/core/client/src/client.rs

@@ -916,10 +916,26 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 			blockchain::BlockStatus::Unknown => {},
 		}
 
-		let (last_best, last_best_number) = {
+		let info = self.backend.blockchain().info();
-			let info = self.backend.blockchain().info();
+
-			(info.best_hash, info.best_number)
+		// the block is lower than our last finalized block so it must revert
-		};
+		// finality, refusing import.
+		if *import_headers.post().number() <= info.finalized_number {
+			return Err(error::Error::NotInFinalizedChain);
+		}
+
+		// find tree route from last finalized to given block.
+		let route_from_finalized = crate::blockchain::tree_route(
+			self.backend.blockchain(),
+			BlockId::Hash(info.finalized_hash),
+			BlockId::Hash(parent_hash),
+		)?;
+
+		// the block being imported retracts the last finalized block, refusing to
+		// import.
+		if !route_from_finalized.retracted().is_empty() {
+			return Err(error::Error::NotInFinalizedChain);
+		}
 
 		// this is a fairly arbitrary choice of where to draw the line on making notifications,
 		// but the general goal is to only make notifications when we are already fully synced
@@ -934,16 +950,23 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 		// ensure parent block is finalized to maintain invariant that
 		// finality is called sequentially.
 		if finalized {
-			self.apply_finality_with_block_hash(operation, parent_hash, None, last_best, make_notifications)?;
+			self.apply_finality_with_block_hash(operation, parent_hash, None, info.best_hash, make_notifications)?;
 		}
 
 		// FIXME #1232: correct path logic for when to execute this function
-		let (storage_update,changes_update,storage_changes) = self.block_execution(&operation.op, &import_headers, origin, hash, body.clone())?;
+		let (storage_update, changes_update, storage_changes) = self.block_execution(
+			&operation.op,
+			&import_headers,
+			origin,
+			hash,
+			body.clone(),
+		)?;
 
 		let is_new_best = finalized || match fork_choice {
-			ForkChoiceStrategy::LongestChain => import_headers.post().number() > &last_best_number,
+			ForkChoiceStrategy::LongestChain => import_headers.post().number() > &info.best_number,
 			ForkChoiceStrategy::Custom(v) => v,
 		};
+
 		let leaf_state = if finalized {
 			crate::backend::NewBlockState::Final
 		} else if is_new_best {
@@ -1871,6 +1894,7 @@ pub(crate) mod tests {
 	use test_client::{
 		prelude::*,
 		client::backend::Backend as TestBackend,
+		client_db::{Backend, DatabaseSettings, PruningMode},
 		runtime::{self, Block, Transfer, RuntimeApi, TestAPI},
 	};
 
@@ -2790,4 +2814,95 @@ pub(crate) mod tests {
 		client.set_head(BlockId::hash(b1.hash())).unwrap();
 		assert_eq!(950, current_balance());
 	}
+
+	#[test]
+	fn doesnt_import_blocks_that_revert_finality() {
+		let _ = env_logger::try_init();
+		let tmp = tempfile::tempdir().unwrap();
+
+		// we need to run with archive pruning to avoid pruning non-canonical
+		// states
+		let backend = Arc::new(Backend::new(
+			DatabaseSettings {
+				cache_size: None,
+				state_cache_size: 1 << 20,
+				state_cache_child_ratio: None,
+				path: tmp.path().into(),
+				pruning: PruningMode::ArchiveAll,
+			},
+			u64::max_value(),
+		).unwrap());
+
+		let client = TestClientBuilder::with_backend(backend).build();
+
+		//    -> C1
+		//   /
+		// G -> A1 -> A2
+		//   \
+		//    -> B1 -> B2 -> B3
+
+		let a1 = client.new_block_at(&BlockId::Number(0), Default::default())
+			.unwrap().bake().unwrap();
+		client.import(BlockOrigin::Own, a1.clone()).unwrap();
+
+		let a2 = client.new_block_at(&BlockId::Hash(a1.hash()), Default::default())
+			.unwrap().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: 1,
+			nonce: 0,
+		}).unwrap();
+		let b1 = b1.bake().unwrap();
+		client.import(BlockOrigin::Own, b1.clone()).unwrap();
+
+		let b2 = client.new_block_at(&BlockId::Hash(b1.hash()), Default::default())
+			.unwrap().bake().unwrap();
+		client.import(BlockOrigin::Own, b2.clone()).unwrap();
+
+		// we will finalize A2 which should make it impossible to import a new
+		// B3 at the same height but that doesnt't include it
+		client.finalize_block(BlockId::Hash(a2.hash()), None, false).unwrap();
+
+		let b3 = client.new_block_at(&BlockId::Hash(b2.hash()), Default::default())
+			.unwrap().bake().unwrap();
+
+		let import_err = client.import(BlockOrigin::Own, b3).err().unwrap();
+		let expected_err = ConsensusError::ClientImport(
+			error::Error::NotInFinalizedChain.to_string()
+		);
+
+		assert_eq!(
+			import_err.to_string(),
+			expected_err.to_string(),
+		);
+
+		// adding a C1 block which is lower than the last finalized should also
+		// fail (with a cheaper check that doesn't require checking ancestry).
+		let mut c1 = client.new_block_at(&BlockId::Number(0), Default::default()).unwrap();
+
+		// needed to make sure C1 gets a different hash from A1 and B1
+		c1.push_transfer(Transfer {
+			from: AccountKeyring::Alice.into(),
+			to: AccountKeyring::Ferdie.into(),
+			amount: 2,
+			nonce: 0,
+		}).unwrap();
+		let c1 = c1.bake().unwrap();
+
+		let import_err = client.import(BlockOrigin::Own, c1).err().unwrap();
+		let expected_err = ConsensusError::ClientImport(
+			error::Error::NotInFinalizedChain.to_string()
+		);
+
+		assert_eq!(
+			import_err.to_string(),
+			expected_err.to_string(),
+		);
+	}
 }