Revert "finality-grandpa: improve validation of justifications"

This reverts commit 51eb2c5.

core/client/src/client.rs

@@ -1738,6 +1738,6 @@ pub(crate) mod tests {
 		assert_eq!(
 			client.backend().blockchain().justification(BlockId::Hash(a2.hash())).unwrap(),
 			None,
-		);
+		)
 	}
 }

core/finality-grandpa/Cargo.toml

@@ -20,7 +20,7 @@ substrate-finality-grandpa-primitives = { path = "primitives" }
 rand = "0.6"
 
 [dependencies.finality-grandpa]
-version = "0.6.0"
+version = "0.5.0"
 features = ["derive-codec"]
 
 [dev-dependencies]

core/finality-grandpa/src/authorities.rs

@@ -20,7 +20,6 @@ use parking_lot::RwLock;
 use substrate_primitives::AuthorityId;
 
 use std::cmp::Ord;
-use std::collections::HashMap;
 use std::fmt::Debug;
 use std::ops::Add;
 use std::sync::Arc;
@@ -64,10 +63,6 @@ where
 	pub(crate) fn set_id(&self) -> u64 {
 		self.inner.read().set_id
 	}
-
-	pub(crate) fn current_authorities(&self) -> HashMap<AuthorityId, u64> {
-		self.inner.read().current_authorities.iter().cloned().collect()
-	}
 }
 
 impl<H, N> From<AuthoritySet<H, N>> for SharedAuthoritySet<H, N> {

core/finality-grandpa/src/lib.rs

@@ -311,17 +311,22 @@ impl<B, E, Block: BlockT<Hash=H256>, RA> BlockStatus<Block> for Arc<Client<B, E,
 	}
 }
 
-// The chain interface exposed to GRANDPA.
-#[derive(Clone)]
-struct Chain<B, E, Block: BlockT, RA> {
+/// The environment we run GRANDPA in.
+struct Environment<B, E, Block: BlockT, N: Network, RA> {
 	inner: Arc<Client<B, E, Block, RA>>,
+	voters: Arc<HashMap<AuthorityId, u64>>,
+	config: Config,
 	authority_set: SharedAuthoritySet<Block::Hash, NumberFor<Block>>,
+	network: N,
+	set_id: u64,
 }
 
-impl<Block: BlockT<Hash=H256>, B, E, RA> grandpa::Chain<Block::Hash, NumberFor<Block>> for Chain<B, E, Block, RA> where
+impl<Block: BlockT<Hash=H256>, B, E, N, RA> grandpa::Chain<Block::Hash, NumberFor<Block>> for Environment<B, E, Block, N, RA> where
 	Block: 'static,
 	B: Backend<Block, Blake2Hasher> + 'static,
 	E: CallExecutor<Block, Blake2Hasher> + 'static,
+	N: Network + 'static,
+	N::In: 'static,
 	NumberFor<Block>: BlockNumberOps,
 {
 	fn ancestry(&self, base: Block::Hash, block: Block::Hash) -> Result<Vec<Block::Hash>, GrandpaError> {
@@ -378,32 +383,6 @@ impl<Block: BlockT<Hash=H256>, B, E, RA> grandpa::Chain<Block::Hash, NumberFor<B
 	}
 }
 
-/// The environment we run GRANDPA in.
-struct Environment<B, E, Block: BlockT, N: Network, RA> {
-	chain: Chain<B, E, Block, RA>,
-	voters: Arc<HashMap<AuthorityId, u64>>,
-	config: Config,
-	network: N,
-	set_id: u64,
-}
-
-impl<Block: BlockT<Hash=H256>, B, E, N, RA> grandpa::Chain<Block::Hash, NumberFor<Block>> for Environment<B, E, Block, N, RA> where
-	Block: 'static,
-	B: Backend<Block, Blake2Hasher> + 'static,
-	E: CallExecutor<Block, Blake2Hasher> + 'static,
-	N: Network + 'static,
-	N::In: 'static,
-	NumberFor<Block>: BlockNumberOps,
-{
-	fn ancestry(&self, base: Block::Hash, block: Block::Hash) -> Result<Vec<Block::Hash>, GrandpaError> {
-		self.chain.ancestry(base, block)
-	}
-
-	fn best_chain_containing(&self, block: Block::Hash) -> Option<(Block::Hash, NumberFor<Block>)> {
-		self.chain.best_chain_containing(block)
-	}
-}
-
 /// A new authority set along with the canonical block it changed at.
 #[derive(Debug)]
 struct NewAuthoritySet<H, N> {
@@ -503,8 +482,8 @@ impl<B, E, Block: BlockT<Hash=H256>, N, RA> voter::Environment<Block::Hash, Numb
 		// schedule incoming messages from the network to be held until
 		// corresponding blocks are imported.
 		let incoming = UntilVoteTargetImported::new(
-			self.chain.inner.import_notification_stream(),
-			self.chain.inner.clone(),
+			self.inner.import_notification_stream(),
+			self.inner.clone(),
 			incoming,
 		);
 
@@ -533,7 +512,7 @@ impl<B, E, Block: BlockT<Hash=H256>, N, RA> voter::Environment<Block::Hash, Numb
 		);
 
 		let encoded_state = (round, state).encode();
-		if let Err(e) = self.chain.inner.backend()
+		if let Err(e) = self.inner.backend()
 			.insert_aux(&[(LAST_COMPLETED_KEY, &encoded_state[..])], &[])
 		{
 			warn!(target: "afg", "Shutting down voter due to error bookkeeping last completed round in DB: {:?}", e);
@@ -544,7 +523,7 @@ impl<B, E, Block: BlockT<Hash=H256>, N, RA> voter::Environment<Block::Hash, Numb
 	}
 
 	fn finalize_block(&self, hash: Block::Hash, number: NumberFor<Block>, round: u64, commit: Commit<Block>) -> Result<(), Self::Error> {
-		finalize_block(&*self.chain.inner, &self.chain.authority_set, hash, number, (round, commit).into())
+		finalize_block(&*self.inner, &self.authority_set, hash, number, (round, commit).into())
 	}
 
 	fn round_commit_timer(&self) -> Self::Timer {
@@ -580,7 +559,7 @@ impl<B, E, Block: BlockT<Hash=H256>, N, RA> voter::Environment<Block::Hash, Numb
 struct GrandpaJustification<Block: BlockT> {
 	round: u64,
 	commit: Commit<Block>,
-	votes_ancestries: Vec<Block::Header>,
+	ancestry: Vec<Block::Header>,
 }
 
 impl<Block: BlockT<Hash=H256>> GrandpaJustification<Block> {
@@ -594,8 +573,8 @@ impl<Block: BlockT<Hash=H256>> GrandpaJustification<Block> {
 		E: CallExecutor<Block, Blake2Hasher> + Send + Sync,
 		RA: Send + Sync,
 	{
-		let mut votes_ancestries_hashes = HashSet::new();
-		let mut votes_ancestries = Vec::new();
+		let mut ancestry_hashes = HashSet::new();
+		let mut ancestry = Vec::new();
 
 		let error = {
 			let msg = "invalid precommits for target commit".to_string();
@@ -614,8 +593,8 @@ impl<Block: BlockT<Hash=H256>> GrandpaJustification<Block> {
 						}
 
 						let parent_hash = current_header.parent_hash().clone();
-						if votes_ancestries_hashes.insert(current_hash) {
-							votes_ancestries.push(current_header);
+						if ancestry_hashes.insert(current_hash) {
+							ancestry.push(current_header);
 						}
 						current_hash = parent_hash;
 					},
@@ -624,19 +603,14 @@ impl<Block: BlockT<Hash=H256>> GrandpaJustification<Block> {
 			}
 		}
 
-		Ok(GrandpaJustification { round, commit, votes_ancestries })
+		Ok(GrandpaJustification { round, commit, ancestry })
 	}
 
 	// commit decoded as block justification must be verified and validated
-	fn decode_and_verify<C>(
+	fn decode_and_verify(
 		encoded: Vec<u8>,
 		set_id: u64,
-		voters: &HashMap<AuthorityId, u64>,
-		chain: &C,
-	) -> Result<GrandpaJustification<Block>, ClientError> where
-		NumberFor<Block>: grandpa::BlockNumberOps,
-		C: grandpa::Chain<Block::Hash, NumberFor<Block>>,
-	{
+	) -> Result<GrandpaJustification<Block>, ClientError> {
 		let justification = match GrandpaJustification::decode(&mut &*encoded) {
 			Some(justification) => justification,
 			_ => {
@@ -645,26 +619,12 @@ impl<Block: BlockT<Hash=H256>> GrandpaJustification<Block> {
 			}
 		};
 
-		match voter::validate_commit(
-			&justification.commit,
-			voters,
-			None,
-			chain,
-		) {
-			Ok(Some(_)) => {},
-			_ => {
-				let msg = "invalid commit in grandpa justification".to_string();
-				return Err(ClientErrorKind::BadJustification(msg).into());
-			}
-		}
-
-		let votes_ancestries: HashMap<_, _> = justification.votes_ancestries
+		let ancestry: HashMap<_, _> = justification.ancestry
 			.iter()
 			.cloned()
 			.map(|h: Block::Header| (h.hash(), h))
 			.collect();
 
-		let mut visited_hashes = HashSet::new();
 		for signed in justification.commit.precommits.iter() {
 			if let Err(_) = communication::check_message_sig::<Block>(
 				&grandpa::Message::Precommit(signed.precommit.clone()),
@@ -680,24 +640,18 @@ impl<Block: BlockT<Hash=H256>> GrandpaJustification<Block> {
 			let mut current_hash = signed.precommit.target_hash.clone();
 			loop {
 				if current_hash == justification.commit.target_hash { break; }
-				match votes_ancestries.get(&current_hash) {
+				match ancestry.get(&current_hash) {
 					Some(current_header) if *current_header.number() > justification.commit.target_number => {
 						current_hash = current_header.parent_hash().clone();
-						visited_hashes.insert(current_hash);
 					}
 					_ => {
 						return Err(ClientErrorKind::BadJustification(
-							"invalid precommit ancestry proof in grandpa justification".to_string()).into());
+							"invalid ancestry proof in grandpa justification".to_string()).into());
 					}
 				}
 			}
 		}
 
-		if visited_hashes != votes_ancestries.into_iter().map(|(k, _)| k).collect() {
-			return Err(ClientErrorKind::BadJustification(
-				"invalid precommit ancestries in grandpa justification with unused headers".to_string()).into());
-		}
-
 		Ok(justification)
 	}
 }
@@ -876,15 +830,15 @@ impl<Block: BlockT> SharedGrandpaOracle<Block> {
 /// When using GRANDPA, the block import worker should be using this block import
 /// object.
 pub struct GrandpaBlockImport<B, E, Block: BlockT<Hash=H256>, RA, PRA> {
-	chain: Chain<B, E, Block, RA>,
+	inner: Arc<Client<B, E, Block, RA>>,
+	authority_set: SharedAuthoritySet<Block::Hash, NumberFor<Block>>,
 	authority_set_change: mpsc::UnboundedSender<NewAuthoritySet<Block::Hash, NumberFor<Block>>>,
 	authority_set_oracle: SharedGrandpaOracle<Block>,
 	api: Arc<PRA>,
 }
 
 impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> BlockImport<Block>
 	for GrandpaBlockImport<B, E, Block, RA, PRA> where
-		NumberFor<Block>: grandpa::BlockNumberOps,
 		B: Backend<Block, Blake2Hasher> + 'static,
 		E: CallExecutor<Block, Blake2Hasher> + 'static + Clone + Send + Sync,
 		DigestFor<Block>: Encode,
@@ -907,7 +861,7 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> BlockImport<Block>
 		let digest = block.header.digest().clone();
 		let is_live = self.authority_set_oracle.is_live();
 
-		let import_result = self.chain.inner.import_block(block, new_authorities)?;
+		let import_result = self.inner.import_block(block, new_authorities)?;
 		if import_result != ImportResult::Queued {
 			return Ok(import_result);
 		}
@@ -918,7 +872,7 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> BlockImport<Block>
 		)?;
 
 		if let Some(change) = maybe_change {
-			let mut authorities = self.chain.authority_set.inner().write();
+			let mut authorities = self.authority_set.inner().write();
 			authorities.add_pending_change(PendingChange {
 				next_authorities: change.next_authorities,
 				finalization_depth: change.delay,
@@ -927,11 +881,11 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> BlockImport<Block>
 			});
 
 			let encoded = authorities.encode();
-			self.chain.inner.backend().insert_aux(&[(AUTHORITY_SET_KEY, &encoded[..])], &[])?;
+			self.inner.backend().insert_aux(&[(AUTHORITY_SET_KEY, &encoded[..])], &[])?;
 		};
 
-		let enacts_change = self.chain.authority_set.inner().read().enacts_change(number, |canon_number| {
-			canonical_at_height(&self.chain.inner, (hash, number), canon_number)
+		let enacts_change = self.authority_set.inner().read().enacts_change(number, |canon_number| {
+			canonical_at_height(&self.inner, (hash, number), canon_number)
 		})?;
 
 		// a pending change is enacted by the given block, if the current
@@ -942,14 +896,12 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> BlockImport<Block>
 				if enacts_change && !is_live {
 					let justification = GrandpaJustification::decode_and_verify(
 						justification,
-						self.chain.authority_set.set_id(),
-						&self.chain.authority_set.current_authorities(),
-						&self.chain,
+						self.authority_set.set_id(),
 					)?;
 
 					let result = finalize_block(
-						&*self.chain.inner,
-						&self.chain.authority_set,
+						&*self.inner,
+						&self.authority_set,
 						hash,
 						number,
 						justification.into(),
@@ -1035,7 +987,7 @@ where
 
 	type Error = <Client<B, E, Block, RA> as Authorities<Block>>::Error;
 	fn authorities(&self, at: &BlockId<Block>) -> Result<Vec<AuthorityId>, Self::Error> {
-		self.chain.inner.authorities_at(at)
+		self.inner.authorities_at(at)
 	}
 }
 
@@ -1090,14 +1042,10 @@ pub fn block_import<B, E, Block: BlockT<Hash=H256>, RA, PRA>(
 
 	let authority_set_oracle = SharedGrandpaOracle::empty();
 
-	let chain = Chain {
-		inner: client.clone(),
-		authority_set: authority_set.clone(),
-	};
-
 	Ok((
 		GrandpaBlockImport {
-			chain,
+			inner: client.clone(),
+			authority_set: authority_set.clone(),
 			authority_set_change: authority_set_change_tx,
 			authority_set_oracle: authority_set_oracle.clone(),
 			api
@@ -1204,19 +1152,17 @@ pub fn run_grandpa<B, E, Block: BlockT<Hash=H256>, N, RA>(
 			))?
 	};
 
-	let voters = authority_set.current_authorities();
-
-	let chain = Chain {
-		inner: client.clone(),
-		authority_set: authority_set.clone(),
-	};
+	let voters = authority_set.inner().read().current().1.iter()
+		.cloned()
+		.collect();
 
 	let initial_environment = Arc::new(Environment {
-		chain,
+		inner: client.clone(),
 		config: config.clone(),
 		voters: Arc::new(voters),
 		network: network.clone(),
 		set_id: authority_set.set_id(),
+		authority_set: authority_set.clone(),
 	});
 
 	let initial_state = (initial_environment, last_round_number, last_state, authority_set_change.into_future());
@@ -1265,17 +1211,13 @@ pub fn run_grandpa<B, E, Block: BlockT<Hash=H256>, N, RA>(
 		let authority_set = authority_set.clone();
 
 		let trigger_authority_set_change = |new: NewAuthoritySet<_, _>, authority_set_change| {
-			let chain = Chain {
-				inner: client,
-				authority_set,
-			};
-
 			let env = Arc::new(Environment {
-				chain,
+				inner: client,
 				config,
 				voters: Arc::new(new.authorities.into_iter().collect()),
 				set_id: new.set_id,
 				network,
+				authority_set,
 			});
 
 			// start the new authority set using the block where the