grandpa: remove runtime checks in prove_finality (#7953)

Remove checks that involve cross checking authorities in the runtime against what we have stored in
the AuthoritySetChanges.

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

@@ -36,53 +36,29 @@
 //! finality proof (that finalizes some block C that is ancestor of the B and descendant
 //! of the U) could be returned.
 
-use std::sync::Arc;
 use log::trace;
+use std::sync::Arc;
 
-use sp_blockchain::{
-	Backend as BlockchainBackend, Error as ClientError, Result as ClientResult,
-};
-use sc_client_api::{backend::Backend, StorageProvider};
-use parity_scale_codec::{Encode, Decode};
 use finality_grandpa::BlockNumberOps;
+use parity_scale_codec::{Encode, Decode};
+use sp_blockchain::{Backend as BlockchainBackend, Error as ClientError, Result as ClientResult};
 use sp_runtime::{
 	Justification, generic::BlockId,
 	traits::{NumberFor, Block as BlockT, Header as HeaderT, Zero, One},
 };
-use sp_core::storage::StorageKey;
-use sp_finality_grandpa::{AuthorityId, AuthorityList, VersionedAuthorityList, GRANDPA_AUTHORITIES_KEY};
+use sc_client_api::backend::Backend;
+use sp_finality_grandpa::{AuthorityId, AuthorityList};
 
-use crate::justification::GrandpaJustification;
-use crate::VoterSet;
-use crate::SharedAuthoritySet;
 use crate::authorities::AuthoritySetChanges;
+use crate::justification::GrandpaJustification;
+use crate::SharedAuthoritySet;
+use crate::VoterSet;
 
 const MAX_UNKNOWN_HEADERS: usize = 100_000;
 
-pub trait AuthoritySetForFinalityProver<Block: BlockT>: Send + Sync {
-	/// Read GRANDPA_AUTHORITIES_KEY from storage at given block.
-	fn authorities(&self, block: &BlockId<Block>) -> ClientResult<AuthorityList>;
-}
-
-/// Implementation of AuthoritySetForFinalityProver.
-impl<BE, Block: BlockT> AuthoritySetForFinalityProver<Block>
-	for Arc<dyn StorageProvider<Block, BE> + Send + Sync>
-where
-	BE: Backend<Block> + Send + Sync + 'static,
-{
-	fn authorities(&self, block: &BlockId<Block>) -> ClientResult<AuthorityList> {
-		let storage_key = StorageKey(GRANDPA_AUTHORITIES_KEY.to_vec());
-		self.storage(block, &storage_key)?
-			.and_then(|encoded| VersionedAuthorityList::decode(&mut encoded.0.as_slice()).ok())
-			.map(|versioned| versioned.into())
-			.ok_or(ClientError::InvalidAuthoritiesSet)
-	}
-}
-
 /// Finality proof provider for serving network requests.
 pub struct FinalityProofProvider<BE, Block: BlockT> {
 	backend: Arc<BE>,
-	authority_provider: Arc<dyn AuthoritySetForFinalityProver<Block>>,
 	shared_authority_set: Option<SharedAuthoritySet<Block::Hash, NumberFor<Block>>>,
 }
 
@@ -95,17 +71,12 @@ where
 	/// - backend for accessing blockchain data;
 	/// - authority_provider for calling and proving runtime methods.
 	/// - shared_authority_set for accessing authority set data
-	pub fn new<P>(
+	pub fn new(
 		backend: Arc<B>,
-		authority_provider: P,
 		shared_authority_set: Option<SharedAuthoritySet<Block::Hash, NumberFor<Block>>>,
-	) -> Self
-	where
-		P: AuthoritySetForFinalityProver<Block> + 'static,
-	{
+	) -> Self {
 		FinalityProofProvider {
 			backend,
-			authority_provider: Arc::new(authority_provider),
 			shared_authority_set,
 		}
 	}
@@ -117,14 +88,9 @@ where
 	/// - shared_authority_set for accessing authority set data
 	pub fn new_for_service(
 		backend: Arc<B>,
-		storage_provider: Arc<dyn StorageProvider<Block, B> + Send + Sync>,
 		shared_authority_set: Option<SharedAuthoritySet<Block::Hash, NumberFor<Block>>>,
 	) -> Arc<Self> {
-		Arc::new(Self::new(
-			backend,
-			storage_provider,
-			shared_authority_set,
-		))
+		Arc::new(Self::new(backend, shared_authority_set))
 	}
 }
 
@@ -136,9 +102,10 @@ where
 {
 	/// Prove finality for the given block number by returning a Justification for the last block of
 	/// the authority set.
-	pub fn prove_finality(&self, block: NumberFor<Block>)
-		-> Result<Option<Vec<u8>>, FinalityProofError>
-	{
+	pub fn prove_finality(
+		&self,
+		block: NumberFor<Block>
+	) -> Result<Option<Vec<u8>>, FinalityProofError> {
 		let authority_set_changes = if let Some(changes) = self
 			.shared_authority_set
 			.as_ref()
@@ -151,7 +118,6 @@ where
 
 		prove_finality::<_, _, GrandpaJustification<Block>>(
 			&*self.backend.blockchain(),
-			&*self.authority_provider,
 			authority_set_changes,
 			block,
 		)
@@ -204,7 +170,6 @@ type AuthoritySetProof<Header> = Vec<AuthoritySetProofFragment<Header>>;
 
 fn prove_finality<Block, B, J>(
 	blockchain: &B,
-	authorities_provider: &dyn AuthoritySetForFinalityProver<Block>,
 	authority_set_changes: AuthoritySetChanges<NumberFor<Block>>,
 	block: NumberFor<Block>,
 ) -> Result<Option<Vec<u8>>, FinalityProofError>
@@ -227,7 +192,7 @@ where
 
 	// Get set_id the block belongs to, and the last block of the set which should contain a
 	// Justification we can use to prove the requested block.
-	let (set_id, last_block_for_set) = if let Some(id) = authority_set_changes.get_set_id(block) {
+	let (_, last_block_for_set) = if let Some(id) = authority_set_changes.get_set_id(block) {
 		id
 	} else {
 		trace!(
@@ -253,21 +218,6 @@ where
 			return Ok(None);
 		};
 
-
-	// Check if the justification is generated by the requested authority set
-	let block_authorities = authorities_provider.authorities(&BlockId::Number(block))?;
-	let justification_check_result =
-		J::decode_and_verify(&justification, set_id, &block_authorities);
-	if justification_check_result.is_err() {
-		trace!(
-			target: "afg",
-			"Can not provide finality proof with requested set id #{}\
-			(possible forced change?). Returning empty proof.",
-			set_id,
-		);
-		return Ok(None);
-	}
-
 	// Collect all headers from the requested block until the last block of the set
 	let unknown_headers = {
 		let mut headers = Vec::new();
@@ -276,14 +226,6 @@ where
 			if current >= last_block_for_set || headers.len() >= MAX_UNKNOWN_HEADERS {
 				break;
 			}
-			if block_authorities != authorities_provider.authorities(&BlockId::Number(current))? {
-				trace!(
-					target: "afg",
-					"Encountered new authorities when collecting unknown headers. \
-					Returning empty proof",
-				);
-				return Ok(None);
-			}
 			headers.push(blockchain.expect_header(BlockId::Number(current))?);
 			current += One::one();
 		}
@@ -653,23 +595,15 @@ impl<Header: HeaderT> WarpSyncFragmentCache<Header> {
 #[cfg(test)]
 pub(crate) mod tests {
 	use super::*;
-	use substrate_test_runtime_client::runtime::{Block, Header, H256};
+	use crate::authorities::AuthoritySetChanges;
+	use sp_core::crypto::Public;
+	use sp_finality_grandpa::AuthorityList;
 	use sc_client_api::NewBlockState;
 	use sc_client_api::in_mem::Blockchain as InMemoryBlockchain;
-	use sp_core::crypto::Public;
-	use crate::authorities::AuthoritySetChanges;
+	use substrate_test_runtime_client::runtime::{Block, Header, H256};
 
 	pub(crate) type FinalityProof = super::FinalityProof<Header>;
 
-	impl<GetAuthorities> AuthoritySetForFinalityProver<Block> for GetAuthorities
-	where
-		GetAuthorities: Send + Sync + Fn(BlockId<Block>) -> ClientResult<AuthorityList>,
-	{
-		fn authorities(&self, block: &BlockId<Block>) -> ClientResult<AuthorityList> {
-			self(*block)
-		}
-	}
-
 	#[derive(Debug, PartialEq, Encode, Decode)]
 	pub struct TestJustification(pub (u64, AuthorityList), pub Vec<u8>);
 
@@ -733,7 +667,7 @@ pub(crate) mod tests {
 	}
 
 	#[test]
-	fn finality_proof_is_none_if_no_more_last_finalized_blocks() {
+	fn finality_proof_fails_if_no_more_last_finalized_blocks() {
 		let blockchain = test_blockchain();
 		blockchain
 			.insert(header(4).hash(), header(4), Some(vec![1]), None, NewBlockState::Best)
@@ -748,7 +682,6 @@ pub(crate) mod tests {
 		// The last finalized block is 3, so we cannot provide further justifications.
 		let proof_of_4 = prove_finality::<_, _, TestJustification>(
 			&blockchain,
-			&|_| unreachable!("Should return before calling GetAuthorities"),
 			authority_set_changes,
 			*header(4).number(),
 		);
@@ -769,7 +702,6 @@ pub(crate) mod tests {
 		// => we can't prove finality of 3
 		let proof_of_3 = prove_finality::<_, _, TestJustification>(
 			&blockchain,
-			&|_| unreachable!("Should return before calling GetAuthorities"),
 			authority_set_changes,
 			*header(3).number(),
 		)
@@ -777,136 +709,6 @@ pub(crate) mod tests {
 		assert_eq!(proof_of_3, None);
 	}
 
-	#[test]
-	fn finality_proof_is_none_if_justification_is_generated_by_unknown_set() {
-		// This is the case for forced change: set_id has been forcibly increased,
-		// or when our stored authority set changes is incomplete
-		let blockchain = test_blockchain();
-		let auth = vec![(AuthorityId::from_slice(&[42u8; 32]), 1u64)];
-		let just4 = TestJustification((0, auth), vec![4]).encode();
-		blockchain
-			.insert(header(4).hash(), header(4), Some(just4), None, NewBlockState::Final)
-			.unwrap();
-
-		let mut authority_set_changes = AuthoritySetChanges::empty();
-		authority_set_changes.append(0, 4);
-
-		let proof_of_3 = prove_finality::<_, _, TestJustification>(
-			&blockchain,
-			&|_| Ok(vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)]),
-			authority_set_changes,
-			*header(3).number(),
-		)
-		.unwrap();
-		assert!(proof_of_3.is_none());
-	}
-
-	#[test]
-	fn finality_proof_is_none_if_authority_set_id_is_incorrect() {
-		let blockchain = test_blockchain();
-		let auth = vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)];
-		let just4 = TestJustification((0, auth.clone()), vec![4]).encode();
-		blockchain
-			.insert(header(4).hash(), header(4), Some(just4), None, NewBlockState::Final)
-			.unwrap();
-
-		let mut authority_set_changes = AuthoritySetChanges::empty();
-		authority_set_changes.append(0, 1);
-		authority_set_changes.append(1, 4);
-
-		// We call `prove_finality` with the wrong `authorities_set_id`, since the Justification for
-		// block 4 contains set id 0.
-		let proof_of_3 = prove_finality::<_, _, TestJustification>(
-			&blockchain,
-			&|_| Ok(auth.clone()),
-			authority_set_changes,
-			*header(3).number(),
-		)
-		.unwrap();
-		assert!(proof_of_3.is_none());
-	}
-
-	#[test]
-	fn finality_proof_is_none_for_next_set_id_with_new_the_authority_set() {
-		let blockchain = test_blockchain();
-		let auth1 = vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)];
-		let auth2 = vec![(AuthorityId::from_slice(&[2u8; 32]), 1u64)];
-		let just5 = TestJustification((0, auth1.clone()), vec![5]).encode();
-		let just6 = TestJustification((1, auth2.clone()), vec![6]).encode();
-		blockchain
-			.insert(header(4).hash(), header(4), None, None, NewBlockState::Final)
-			.unwrap();
-		blockchain
-			.insert(header(5).hash(), header(5), Some(just5), None, NewBlockState::Final)
-			.unwrap();
-		blockchain
-			.insert(header(6).hash(), header(6), Some(just6), None, NewBlockState::Final)
-			.unwrap();
-
-		let mut authority_set_changes = AuthoritySetChanges::empty();
-		authority_set_changes.append(0, 1);
-		authority_set_changes.append(1, 6);
-
-		// Trying to prove block 4 using block 6 fails as the authority set has changed
-		let proof_of_4 = prove_finality::<_, _, TestJustification>(
-			&blockchain,
-			&|block_id| match block_id {
-				BlockId::Number(4) => Ok(auth1.clone()),
-				_ => unimplemented!("No other authorities should be proved: {:?}", block_id),
-			},
-			authority_set_changes,
-			*header(4).number(),
-		)
-		.unwrap();
-		assert!(proof_of_4.is_none());
-	}
-
-	#[test]
-	fn finality_proof_is_none_if_the_authority_set_changes_and_changes_back() {
-		let blockchain = test_blockchain();
-		let auth1 = vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)];
-		let auth2 = vec![(AuthorityId::from_slice(&[2u8; 32]), 1u64)];
-		let just5 = TestJustification((0, auth1.clone()), vec![5]).encode();
-		let just6 = TestJustification((1, auth2.clone()), vec![6]).encode();
-		let just7 = TestJustification((2, auth1.clone()), vec![7]).encode();
-		blockchain
-			.insert(header(4).hash(), header(4), None, None, NewBlockState::Final)
-			.unwrap();
-		blockchain
-			.insert(header(5).hash(), header(5), Some(just5), None, NewBlockState::Final)
-			.unwrap();
-		blockchain
-			.insert(header(6).hash(), header(6), Some(just6), None, NewBlockState::Final)
-			.unwrap();
-		blockchain
-			.insert(header(7).hash(), header(7), Some(just7), None, NewBlockState::Final)
-			.unwrap();
-
-		// Set authority set changes so that they don't contain the switch, and switch back, of the
-		// authorities. As well as incorrect set_id to avoid the guard against that.
-		// This should trigger the check for walking through the headers and checking for authority
-		// set changes that are missed.
-		let mut authority_set_changes = AuthoritySetChanges::empty();
-		authority_set_changes.append(0, 1);
-		authority_set_changes.append(1, 2);
-		authority_set_changes.append(2, 7);
-
-		let proof_of_4 =
-			prove_finality::<_, _, TestJustification>(
-				&blockchain,
-				&|block_id| match block_id {
-					BlockId::Number(4) => Ok(auth1.clone()),
-					BlockId::Number(5) => Ok(auth1.clone()),
-					BlockId::Number(6) => Ok(auth2.clone()),
-					_ => unimplemented!("No other authorities should be proved: {:?}", block_id),
-				},
-				authority_set_changes,
-				*header(4).number(),
-			)
-		.unwrap();
-		assert!(proof_of_4.is_none());
-	}
-
 	#[test]
 	fn finality_proof_check_fails_when_proof_decode_fails() {
 		// When we can't decode proof from Vec<u8>
@@ -947,7 +749,7 @@ pub(crate) mod tests {
 	}
 
 	#[test]
-	fn finality_proof_using_authority_set_changes_is_none_with_undefined_start() {
+	fn finality_proof_using_authority_set_changes_fails_with_undefined_start() {
 		let blockchain = test_blockchain();
 		let auth = vec![(AuthorityId::from_slice(&[1u8; 32]), 1u64)];
 		let just4 = TestJustification((0, auth.clone()), vec![4]).encode();
@@ -972,11 +774,6 @@ pub(crate) mod tests {
 
 		let proof_of_5 = prove_finality::<_, _, TestJustification>(
 			&blockchain,
-			&|block_id| match block_id {
-				BlockId::Number(5) => Ok(auth.clone()),
-				BlockId::Number(6) => Ok(auth.clone()),
-				_ => unimplemented!("No other authorities should be proved: {:?}", block_id),
-			},
 			authority_set_changes,
 			*header(5).number(),
 		);
@@ -1009,11 +806,6 @@ pub(crate) mod tests {
 		let proof_of_5: FinalityProof = Decode::decode(
 			&mut &prove_finality::<_, _, TestJustification>(
 				&blockchain,
-				&|block_id| match block_id {
-					BlockId::Number(5) => Ok(auth.clone()),
-					BlockId::Number(6) => Ok(auth.clone()),
-					_ => unimplemented!("No other authorities should be proved: {:?}", block_id),
-				},
 				authority_set_changes,
 				*header(5).number(),
 			)