From b28f7328aca8f9e13602a60897d703b660033170 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Silva?= <andre.beat@gmail.com>
Date: Mon, 11 Feb 2019 10:39:41 +0000
Subject: [PATCH] grandpa: fix reimport of change blocks (#1754)

* core: grandpa: handle re-import of change blocks

* core: grandpa: add test for change block reimport
---
 substrate/core/finality-grandpa/src/import.rs | 13 ++++-
 substrate/core/finality-grandpa/src/tests.rs  | 47 +++++++++++++++++--
 2 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/substrate/core/finality-grandpa/src/import.rs b/substrate/core/finality-grandpa/src/import.rs
index 4398bdc82e..20f31c2cb9 100644
--- a/substrate/core/finality-grandpa/src/import.rs
+++ b/substrate/core/finality-grandpa/src/import.rs
@@ -19,7 +19,7 @@ use std::sync::Arc;
 use codec::Encode;
 use futures::sync::mpsc;
 
-use client::{CallExecutor, Client};
+use client::{blockchain, CallExecutor, Client};
 use client::backend::Backend;
 use consensus_common::{
 	BlockImport, Error as ConsensusError, ErrorKind as ConsensusErrorKind,
@@ -124,10 +124,19 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> BlockImport<Block>
 		-> Result<ImportResult, Self::Error>
 	{
 		use authorities::PendingChange;
+		use client::blockchain::HeaderBackend;
 
 		let hash = block.post_header().hash();
 		let number = block.header.number().clone();
 
+		// early exit if block already in chain, otherwise the check for
+		// authority changes will error when trying to re-import a change block
+		match self.inner.backend().blockchain().status(BlockId::Hash(hash)) {
+			Ok(blockchain::BlockStatus::InChain) => return Ok(ImportResult::AlreadyInChain),
+			Ok(blockchain::BlockStatus::Unknown) => {},
+			Err(e) => return Err(ConsensusErrorKind::ClientImport(e.to_string()).into()),
+		}
+
 		let maybe_change = self.api.runtime_api().grandpa_pending_change(
 			&BlockId::hash(*block.header.parent_hash()),
 			&block.header.digest().clone(),
@@ -156,7 +165,7 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> BlockImport<Block>
 				if *base == hash { return error(); }
 				if *base == parent_hash { return error(); }
 
-				let tree_route = ::client::blockchain::tree_route(
+				let tree_route = blockchain::tree_route(
 					self.inner.backend().blockchain(),
 					BlockId::Hash(parent_hash),
 					BlockId::Hash(*base),
diff --git a/substrate/core/finality-grandpa/src/tests.rs b/substrate/core/finality-grandpa/src/tests.rs
index f4a5e4ac20..a75a956328 100644
--- a/substrate/core/finality-grandpa/src/tests.rs
+++ b/substrate/core/finality-grandpa/src/tests.rs
@@ -30,7 +30,7 @@ use client::{
 };
 use test_client::{self, runtime::BlockNumber};
 use codec::Decode;
-use consensus_common::BlockOrigin;
+use consensus_common::{BlockOrigin, ForkChoiceStrategy, ImportBlock, ImportResult};
 use consensus_common::import_queue::{SharedBlockImport, SharedJustificationImport};
 use std::collections::{HashMap, HashSet};
 use std::result;
@@ -772,8 +772,6 @@ fn sync_justifications_on_change_blocks() {
 
 #[test]
 fn doesnt_vote_on_the_tip_of_the_chain() {
-	::env_logger::init();
-
 	let peers_a = &[Keyring::Alice, Keyring::Bob, Keyring::Charlie];
 	let voters = make_ids(peers_a);
 	let api = TestApi::new(voters);
@@ -794,3 +792,46 @@ fn doesnt_vote_on_the_tip_of_the_chain() {
 	// the highest block to be finalized will be 3/4 deep in the unfinalized chain
 	assert_eq!(highest, 75);
 }
+
+#[test]
+fn allows_reimporting_change_blocks() {
+	let peers_a = &[Keyring::Alice, Keyring::Bob, Keyring::Charlie];
+	let peers_b = &[Keyring::Alice, Keyring::Bob];
+	let voters = make_ids(peers_a);
+	let api = TestApi::new(voters);
+	let net = GrandpaTestNet::new(api.clone(), 3);
+
+	let client = net.peer(0).client().clone();
+	let (block_import, ..) = net.make_block_import(client.clone());
+
+	let builder = client.new_block_at(&BlockId::Number(0)).unwrap();
+	let block = builder.bake().unwrap();
+	api.scheduled_changes.lock().insert(*block.header.parent_hash(), ScheduledChange {
+		next_authorities: make_ids(peers_b),
+		delay: 0,
+	});
+
+	let block = || {
+		let block = block.clone();
+		ImportBlock {
+			origin: BlockOrigin::File,
+			header: block.header,
+			justification: None,
+			post_digests: Vec::new(),
+			body: Some(block.extrinsics),
+			finalized: false,
+			auxiliary: Vec::new(),
+			fork_choice: ForkChoiceStrategy::LongestChain,
+		}
+	};
+
+	assert_eq!(
+		block_import.import_block(block(), None).unwrap(),
+		ImportResult::NeedsJustification
+	);
+
+	assert_eq!(
+		block_import.import_block(block(), None).unwrap(),
+		ImportResult::AlreadyInChain
+	);
+}