grandpa: fix creation of justification with equivocating precommits in commit (#11302)

* grandpa: fix creation of justification ancestry

we would reject commits that have precommits targeting blocks lower than the
commit target. when there is an equivocation (or if it the commit is not
minimal) it is possible to have such precommits and we should assume that they
are the round base.

* grandpa: bump to 0.16.0

* grandpa: add test for justification with equivocation

* grandpa: fix failing test

substrate/client/finality-grandpa/Cargo.toml

@@ -17,7 +17,7 @@ targets = ["x86_64-unknown-linux-gnu"]
 ahash = "0.7.6"
 async-trait = "0.1.50"
 dyn-clone = "1.0"
-finality-grandpa = { version = "0.15.0", features = ["derive-codec"] }
+finality-grandpa = { version = "0.16.0", features = ["derive-codec"] }
 futures = "0.3.21"
 futures-timer = "3.0.1"
 hex = "0.4.2"
@@ -50,7 +50,7 @@ sp-runtime = { version = "6.0.0", path = "../../primitives/runtime" }
 
 [dev-dependencies]
 assert_matches = "1.3.0"
-finality-grandpa = { version = "0.15.0", features = [
+finality-grandpa = { version = "0.16.0", features = [
 	"derive-codec",
 	"test-helpers",
 ] }

substrate/client/finality-grandpa/rpc/Cargo.toml

@@ -10,7 +10,7 @@ readme = "README.md"
 homepage = "https://substrate.io"
 
 [dependencies]
-finality-grandpa = { version = "0.15.0", features = ["derive-codec"] }
+finality-grandpa = { version = "0.16.0", features = ["derive-codec"] }
 futures = "0.3.16"
 jsonrpsee = { version = "0.13.0", features = ["server", "macros"] }
 log = "0.4.8"

substrate/client/finality-grandpa/src/finality_proof.rs

@@ -237,7 +237,7 @@ where
 }
 
 #[cfg(test)]
-pub(crate) mod tests {
+mod tests {
 	use super::*;
 	use crate::{authorities::AuthoritySetChanges, BlockNumberOps, ClientError, SetId};
 	use futures::executor::block_on;
@@ -271,6 +271,7 @@ pub(crate) mod tests {
 		let justification: GrandpaJustification<Block> =
 			Decode::decode(&mut &proof.justification[..])
 				.map_err(|_| ClientError::JustificationDecode)?;
+
 		justification.verify(current_set_id, &current_authorities)?;
 
 		Ok(proof)
@@ -370,7 +371,7 @@ pub(crate) mod tests {
 
 	#[test]
 	fn finality_proof_check_fails_with_incomplete_justification() {
-		let (client, _, blocks) = test_blockchain(8, &[4, 5, 8]);
+		let (_, _, blocks) = test_blockchain(8, &[4, 5, 8]);
 
 		// Create a commit without precommits
 		let commit = finality_grandpa::Commit {
@@ -378,7 +379,9 @@ pub(crate) mod tests {
 			target_number: *blocks[7].header().number(),
 			precommits: Vec::new(),
 		};
-		let grandpa_just = GrandpaJustification::from_commit(&client, 8, commit).unwrap();
+
+		let grandpa_just =
+			GrandpaJustification::<Block> { round: 8, votes_ancestries: Vec::new(), commit };
 
 		let finality_proof = FinalityProof {
 			block: header(2).hash(),

substrate/client/finality-grandpa/src/justification.rs

@@ -42,9 +42,9 @@ use crate::{AuthorityList, Commit, Error};
 /// nodes, and are used by syncing nodes to prove authority set handoffs.
 #[derive(Clone, Encode, Decode, PartialEq, Eq, Debug)]
 pub struct GrandpaJustification<Block: BlockT> {
-	round: u64,
+	pub(crate) round: u64,
 	pub(crate) commit: Commit<Block>,
-	votes_ancestries: Vec<Block::Header>,
+	pub(crate) votes_ancestries: Vec<Block::Header>,
 }
 
 impl<Block: BlockT> GrandpaJustification<Block> {
@@ -66,16 +66,33 @@ impl<Block: BlockT> GrandpaJustification<Block> {
 			Err(Error::Client(ClientError::BadJustification(msg)))
 		};
 
+		// we pick the precommit for the lowest block as the base that
+		// should serve as the root block for populating ancestry (i.e.
+		// collect all headers from all precommit blocks to the base)
+		let (base_hash, base_number) = match commit
+			.precommits
+			.iter()
+			.map(|signed| &signed.precommit)
+			.min_by_key(|precommit| precommit.target_number)
+			.map(|precommit| (precommit.target_hash.clone(), precommit.target_number))
+		{
+			None => return error(),
+			Some(base) => base,
+		};
+
 		for signed in commit.precommits.iter() {
 			let mut current_hash = signed.precommit.target_hash;
 			loop {
-				if current_hash == commit.target_hash {
+				if current_hash == base_hash {
 					break
 				}
 
 				match client.header(BlockId::Hash(current_hash))? {
 					Some(current_header) => {
-						if *current_header.number() <= commit.target_number {
+						// NOTE: this should never happen as we pick the lowest block
+						// as base and only traverse backwards from the other blocks
+						// in the commit. but better be safe to avoid an unbound loop.
+						if *current_header.number() <= base_number {
 							return error()
 						}
 
@@ -83,6 +100,7 @@ impl<Block: BlockT> GrandpaJustification<Block> {
 						if votes_ancestries_hashes.insert(current_hash) {
 							votes_ancestries.push(current_header);
 						}
+
 						current_hash = parent_hash;
 					},
 					_ => return error(),
@@ -142,13 +160,30 @@ impl<Block: BlockT> GrandpaJustification<Block> {
 		let ancestry_chain = AncestryChain::<Block>::new(&self.votes_ancestries);
 
 		match finality_grandpa::validate_commit(&self.commit, voters, &ancestry_chain) {
-			Ok(ref result) if result.ghost().is_some() => {},
+			Ok(ref result) if result.is_valid() => {},
 			_ => {
 				let msg = "invalid commit in grandpa justification".to_string();
 				return Err(ClientError::BadJustification(msg))
 			},
 		}
 
+		// we pick the precommit for the lowest block as the base that
+		// should serve as the root block for populating ancestry (i.e.
+		// collect all headers from all precommit blocks to the base)
+		let base_hash = self
+			.commit
+			.precommits
+			.iter()
+			.map(|signed| &signed.precommit)
+			.min_by_key(|precommit| precommit.target_number)
+			.map(|precommit| precommit.target_hash.clone())
+			.expect(
+				"can only fail if precommits is empty; \
+				 commit has been validated above; \
+				 valid commits must include precommits; \
+				 qed.",
+			);
+
 		let mut buf = Vec::new();
 		let mut visited_hashes = HashSet::new();
 		for signed in self.commit.precommits.iter() {
@@ -165,11 +200,11 @@ impl<Block: BlockT> GrandpaJustification<Block> {
 				))
 			}
 
-			if self.commit.target_hash == signed.precommit.target_hash {
+			if base_hash == signed.precommit.target_hash {
 				continue
 			}
 
-			match ancestry_chain.ancestry(self.commit.target_hash, signed.precommit.target_hash) {
+			match ancestry_chain.ancestry(base_hash, signed.precommit.target_hash) {
 				Ok(route) => {
 					// ancestry starts from parent hash but the precommit target hash has been
 					// visited

substrate/client/finality-grandpa/src/observer.rs

@@ -116,7 +116,7 @@ where
 			Err(e) => return future::err(e.into()),
 		};
 
-		if validation_result.ghost().is_some() {
+		if validation_result.is_valid() {
 			let finalized_hash = commit.target_hash;
 			let finalized_number = commit.target_number;
 

substrate/client/finality-grandpa/src/tests.rs

@@ -1569,6 +1569,73 @@ fn grandpa_environment_never_overwrites_round_voter_state() {
 	assert_matches!(get_current_round(2).unwrap(), HasVoted::Yes(_, _));
 }
 
+#[test]
+fn justification_with_equivocation() {
+	use sp_application_crypto::Pair;
+
+	// we have 100 authorities
+	let pairs = (0..100).map(|n| AuthorityPair::from_seed(&[n; 32])).collect::<Vec<_>>();
+	let voters = pairs.iter().map(AuthorityPair::public).map(|id| (id, 1)).collect::<Vec<_>>();
+	let api = TestApi::new(voters.clone());
+	let mut net = GrandpaTestNet::new(api.clone(), 1, 0);
+
+	// we create a basic chain with 3 blocks (no forks)
+	net.peer(0).push_blocks(3, false);
+
+	let client = net.peer(0).client().clone();
+	let block1 = client.header(&BlockId::Number(1)).ok().flatten().unwrap();
+	let block2 = client.header(&BlockId::Number(2)).ok().flatten().unwrap();
+	let block3 = client.header(&BlockId::Number(3)).ok().flatten().unwrap();
+
+	let set_id = 0;
+	let justification = {
+		let round = 1;
+
+		let make_precommit = |target_hash, target_number, pair: &AuthorityPair| {
+			let precommit = finality_grandpa::Precommit { target_hash, target_number };
+
+			let msg = finality_grandpa::Message::Precommit(precommit.clone());
+			let encoded = sp_finality_grandpa::localized_payload(round, set_id, &msg);
+
+			let precommit = finality_grandpa::SignedPrecommit {
+				precommit: precommit.clone(),
+				signature: pair.sign(&encoded[..]),
+				id: pair.public(),
+			};
+
+			precommit
+		};
+
+		let mut precommits = Vec::new();
+
+		// we have 66/100 votes for block #3 and therefore do not have threshold to finalize
+		for pair in pairs.iter().take(66) {
+			let precommit = make_precommit(block3.hash(), *block3.number(), pair);
+			precommits.push(precommit);
+		}
+
+		// we create an equivocation for the 67th validator targetting blocks #1 and #2.
+		// this should be accounted as "voting for all blocks" and therefore block #3 will
+		// have 67/100 votes, reaching finality threshold.
+		{
+			precommits.push(make_precommit(block1.hash(), *block1.number(), &pairs[66]));
+			precommits.push(make_precommit(block2.hash(), *block2.number(), &pairs[66]));
+		}
+
+		let commit = finality_grandpa::Commit {
+			target_hash: block3.hash(),
+			target_number: *block3.number(),
+			precommits,
+		};
+
+		GrandpaJustification::from_commit(&client.as_client(), round, commit).unwrap()
+	};
+
+	// the justification should include the minimal necessary vote ancestry and
+	// the commit should be valid
+	assert!(justification.verify(set_id, &voters).is_ok());
+}
+
 #[test]
 fn imports_justification_for_regular_blocks_on_import() {
 	// NOTE: this is a regression test since initially we would only import