grandpa: fix creation of justification with equivocating precommits in commit

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",
 ] }

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"

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,15 +371,17 @@ 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 {
 			target_hash: blocks[7].hash(),
 			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(),

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,23 +66,41 @@ 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()
 						}
 
 						let parent_hash = *current_header.parent_hash();
 						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

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;
 

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

frame/grandpa/Cargo.toml

@@ -31,7 +31,7 @@ sp-staking = { version = "4.0.0-dev", default-features = false, path = "../../pr
 sp-std = { version = "4.0.0", default-features = false, path = "../../primitives/std" }
 
 [dev-dependencies]
-grandpa = { package = "finality-grandpa", version = "0.15.0", features = ["derive-codec"] }
+grandpa = { package = "finality-grandpa", version = "0.16.0", features = ["derive-codec"] }
 frame-benchmarking = { version = "4.0.0-dev", path = "../benchmarking" }
 frame-election-provider-support = { version = "4.0.0-dev", path = "../election-provider-support" }
 pallet-balances = { version = "4.0.0-dev", path = "../balances" }

primitives/finality-grandpa/Cargo.toml

@@ -15,7 +15,7 @@ targets = ["x86_64-unknown-linux-gnu"]
 
 [dependencies]
 codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false, features = ["derive"] }
-grandpa = { package = "finality-grandpa", version = "0.15.0", default-features = false, features = ["derive-codec"] }
+grandpa = { package = "finality-grandpa", version = "0.16.0", default-features = false, features = ["derive-codec"] }
 log = { version = "0.4.17", optional = true }
 scale-info = { version = "2.1.1", default-features = false, features = ["derive"] }
 serde = { version = "1.0.136", features = ["derive"], optional = true }