grandpa: avoid importing unnecessary justifications (paritytech#14423)

* grandpa: avoid importing unnecessary justifications

* grandpa: make justification_import_period configurable

* grandpa: keep the first justification

* grandpa: add test for justification import period

* grandpa: fix test

bin/node-template/node/src/service.rs

@@ -35,6 +35,11 @@ pub(crate) type FullClient =
 type FullBackend = sc_service::TFullBackend<Block>;
 type FullSelectChain = sc_consensus::LongestChain<FullBackend, Block>;
 
+/// The minimum period of blocks on which justifications will be
+/// imported and generated.
+const GRANDPA_JUSTIFICATION_PERIOD: u32 = 512;
+
+#[allow(clippy::type_complexity)]
 pub fn new_partial(
 	config: &Configuration,
 ) -> Result<
@@ -94,7 +99,8 @@ pub fn new_partial(
 
 	let (grandpa_block_import, grandpa_link) = sc_consensus_grandpa::block_import(
 		client.clone(),
-		&(client.clone() as Arc<_>),
+		GRANDPA_JUSTIFICATION_PERIOD,
+		&client,
 		select_chain.clone(),
 		telemetry.as_ref().map(|x| x.handle()),
 	)?;
@@ -275,7 +281,7 @@ pub fn new_full(config: Configuration) -> Result<TaskManager, ServiceError> {
 		let grandpa_config = sc_consensus_grandpa::Config {
 			// FIXME #1578 make this available through chainspec
 			gossip_duration: Duration::from_millis(333),
-			justification_period: 512,
+			justification_generation_period: GRANDPA_JUSTIFICATION_PERIOD,
 			name: Some(name),
 			observer_enabled: false,
 			keystore,

bin/node/cli/src/service.rs

@@ -53,6 +53,10 @@ type FullGrandpaBlockImport =
 /// The transaction pool type defintion.
 pub type TransactionPool = sc_transaction_pool::FullPool<Block, FullClient>;
 
+/// The minimum period of blocks on which justifications will be
+/// imported and generated.
+const GRANDPA_JUSTIFICATION_PERIOD: u32 = 512;
+
 /// Fetch the nonce of the given `account` from the chain state.
 ///
 /// Note: Should only be used for tests.
@@ -192,6 +196,7 @@ pub fn new_partial(
 
 	let (grandpa_block_import, grandpa_link) = grandpa::block_import(
 		client.clone(),
+		GRANDPA_JUSTIFICATION_PERIOD,
 		&(client.clone() as Arc<_>),
 		select_chain.clone(),
 		telemetry.as_ref().map(|x| x.handle()),
@@ -528,7 +533,7 @@ pub fn new_full_base(
 	let config = grandpa::Config {
 		// FIXME #1578 make this available through chainspec
 		gossip_duration: std::time::Duration::from_millis(333),
-		justification_period: 512,
+		justification_generation_period: GRANDPA_JUSTIFICATION_PERIOD,
 		name: Some(name),
 		observer_enabled: false,
 		keystore,

client/consensus/grandpa/src/communication/gossip.rs

@@ -1700,7 +1700,7 @@ mod tests {
 	fn config() -> crate::Config {
 		crate::Config {
 			gossip_duration: Duration::from_millis(10),
-			justification_period: 256,
+			justification_generation_period: 256,
 			keystore: None,
 			name: None,
 			local_role: Role::Authority,

client/consensus/grandpa/src/communication/tests.rs

@@ -244,7 +244,7 @@ impl Tester {
 fn config() -> crate::Config {
 	crate::Config {
 		gossip_duration: std::time::Duration::from_millis(10),
-		justification_period: 256,
+		justification_generation_period: 256,
 		keystore: None,
 		name: None,
 		local_role: Role::Authority,

client/consensus/grandpa/src/environment.rs

@@ -1094,7 +1094,7 @@ where
 		finalize_block(
 			self.client.clone(),
 			&self.authority_set,
-			Some(self.config.justification_period.into()),
+			Some(self.config.justification_generation_period),
 			hash,
 			number,
 			(round, commit).into(),
@@ -1307,14 +1307,46 @@ where
 		.or_else(|| Some((target_header.hash(), *target_header.number()))))
 }
 
+/// Whether we should process a justification for the given block.
+///
+/// This can be used to decide whether to import a justification (when
+/// importing a block), or whether to generate a justification from a
+/// commit (when validating). Justifications for blocks that change the
+/// authority set will always be processed, otherwise we'll only process
+/// justifications if the last one was `justification_period` blocks ago.
+pub(crate) fn should_process_justification<BE, Block, Client>(
+	client: &Client,
+	justification_period: u32,
+	number: NumberFor<Block>,
+	enacts_change: bool,
+) -> bool
+where
+	Block: BlockT,
+	BE: BackendT<Block>,
+	Client: ClientForGrandpa<Block, BE>,
+{
+	if enacts_change {
+		return true
+	}
+
+	let last_finalized_number = client.info().finalized_number;
+
+	// keep the first justification before reaching the justification period
+	if last_finalized_number.is_zero() {
+		return true
+	}
+
+	last_finalized_number / justification_period.into() != number / justification_period.into()
+}
+
 /// Finalize the given block and apply any authority set changes. If an
 /// authority set change is enacted then a justification is created (if not
 /// given) and stored with the block when finalizing it.
 /// This method assumes that the block being finalized has already been imported.
 pub(crate) fn finalize_block<BE, Block, Client>(
 	client: Arc<Client>,
 	authority_set: &SharedAuthoritySet<Block::Hash, NumberFor<Block>>,
-	justification_period: Option<NumberFor<Block>>,
+	justification_generation_period: Option<u32>,
 	hash: Block::Hash,
 	number: NumberFor<Block>,
 	justification_or_commit: JustificationOrCommit<Block>,
@@ -1385,22 +1417,13 @@ where
 		let (justification_required, justification) = match justification_or_commit {
 			JustificationOrCommit::Justification(justification) => (true, justification),
 			JustificationOrCommit::Commit((round_number, commit)) => {
-				let mut justification_required =
-					// justification is always required when block that enacts new authorities
-					// set is finalized
-					status.new_set_block.is_some();
-
-				// justification is required every N blocks to be able to prove blocks
-				// finalization to remote nodes
-				if !justification_required {
-					if let Some(justification_period) = justification_period {
-						let last_finalized_number = client.info().finalized_number;
-						justification_required = (!last_finalized_number.is_zero() ||
-							number - last_finalized_number == justification_period) &&
-							(last_finalized_number / justification_period !=
-								number / justification_period);
-					}
-				}
+				let enacts_change = status.new_set_block.is_some();
+
+				let justification_required = justification_generation_period
+					.map(|period| {
+						should_process_justification(&*client, period, number, enacts_change)
+					})
+					.unwrap_or(enacts_change);
 
 				let justification =
 					GrandpaJustification::from_commit(&client, round_number, commit)?;

client/consensus/grandpa/src/import.rs

@@ -41,7 +41,7 @@ use sp_runtime::{
 
 use crate::{
 	authorities::{AuthoritySet, DelayKind, PendingChange, SharedAuthoritySet},
-	environment::finalize_block,
+	environment,
 	justification::GrandpaJustification,
 	notification::GrandpaJustificationSender,
 	AuthoritySetChanges, ClientForGrandpa, CommandOrError, Error, NewAuthoritySet, VoterCommand,
@@ -59,6 +59,7 @@ use crate::{
 /// object.
 pub struct GrandpaBlockImport<Backend, Block: BlockT, Client, SC> {
 	inner: Arc<Client>,
+	justification_import_period: u32,
 	select_chain: SC,
 	authority_set: SharedAuthoritySet<Block::Hash, NumberFor<Block>>,
 	send_voter_commands: TracingUnboundedSender<VoterCommand<Block::Hash, NumberFor<Block>>>,
@@ -74,6 +75,7 @@ impl<Backend, Block: BlockT, Client, SC: Clone> Clone
 	fn clone(&self) -> Self {
 		GrandpaBlockImport {
 			inner: self.inner.clone(),
+			justification_import_period: self.justification_import_period,
 			select_chain: self.select_chain.clone(),
 			authority_set: self.authority_set.clone(),
 			send_voter_commands: self.send_voter_commands.clone(),
@@ -648,26 +650,39 @@ where
 
 		match grandpa_justification {
 			Some(justification) => {
-				let import_res = self.import_justification(
-					hash,
+				if environment::should_process_justification(
+					&*self.inner,
+					self.justification_import_period,
 					number,
-					(GRANDPA_ENGINE_ID, justification),
 					needs_justification,
-					initial_sync,
-				);
+				) {
+					let import_res = self.import_justification(
+						hash,
+						number,
+						(GRANDPA_ENGINE_ID, justification),
+						needs_justification,
+						initial_sync,
+					);
 
-				import_res.unwrap_or_else(|err| {
-					if needs_justification {
-						debug!(
-							target: LOG_TARGET,
-							"Requesting justification from peers due to imported block #{} that enacts authority set change with invalid justification: {}",
-							number,
-							err
-						);
-						imported_aux.bad_justification = true;
-						imported_aux.needs_justification = true;
-					}
-				});
+					import_res.unwrap_or_else(|err| {
+						if needs_justification {
+							debug!(
+								target: LOG_TARGET,
+								"Requesting justification from peers due to imported block #{} that enacts authority set change with invalid justification: {}",
+								number,
+								err
+							);
+							imported_aux.bad_justification = true;
+							imported_aux.needs_justification = true;
+						}
+					});
+				} else {
+					debug!(
+						target: LOG_TARGET,
+						"Ignoring unnecessary justification for block #{}",
+						number,
+					);
+				}
 			},
 			None =>
 				if needs_justification {
@@ -695,6 +710,7 @@ where
 impl<Backend, Block: BlockT, Client, SC> GrandpaBlockImport<Backend, Block, Client, SC> {
 	pub(crate) fn new(
 		inner: Arc<Client>,
+		justification_import_period: u32,
 		select_chain: SC,
 		authority_set: SharedAuthoritySet<Block::Hash, NumberFor<Block>>,
 		send_voter_commands: TracingUnboundedSender<VoterCommand<Block::Hash, NumberFor<Block>>>,
@@ -733,6 +749,7 @@ impl<Backend, Block: BlockT, Client, SC> GrandpaBlockImport<Backend, Block, Clie
 
 		GrandpaBlockImport {
 			inner,
+			justification_import_period,
 			select_chain,
 			authority_set,
 			send_voter_commands,
@@ -783,7 +800,7 @@ where
 			Ok(justification) => justification,
 		};
 
-		let result = finalize_block(
+		let result = environment::finalize_block(
 			self.inner.clone(),
 			&self.authority_set,
 			None,

client/consensus/grandpa/src/lib.rs

@@ -214,10 +214,10 @@ impl Clone for SharedVoterState {
 pub struct Config {
 	/// The expected duration for a message to be gossiped across the network.
 	pub gossip_duration: Duration,
-	/// Justification generation period (in blocks). GRANDPA will try to generate justifications
-	/// at least every justification_period blocks. There are some other events which might cause
-	/// justification generation.
-	pub justification_period: u32,
+	/// Justification generation period (in blocks). GRANDPA will try to generate
+	/// justifications at least every justification_generation_period blocks. There
+	/// are some other events which might cause justification generation.
+	pub justification_generation_period: u32,
 	/// Whether the GRANDPA observer protocol is live on the network and thereby
 	/// a full-node not running as a validator is running the GRANDPA observer
 	/// protocol (we will only issue catch-up requests to authorities when the
@@ -495,8 +495,16 @@ where
 
 /// Make block importer and link half necessary to tie the background voter
 /// to it.
+///
+/// The `justification_import_period` sets the minimum period on which
+/// justifications will be imported.  When importing a block, if it includes a
+/// justification it will only be processed if it fits within this period,
+/// otherwise it will be ignored (and won't be validated). This is to avoid
+/// slowing down sync by a peer serving us unnecessary justifications which
+/// aren't trivial to validate.
 pub fn block_import<BE, Block: BlockT, Client, SC>(
 	client: Arc<Client>,
+	justification_import_period: u32,
 	genesis_authorities_provider: &dyn GenesisAuthoritySetProvider<Block>,
 	select_chain: SC,
 	telemetry: Option<TelemetryHandle>,
@@ -508,6 +516,7 @@ where
 {
 	block_import_with_authority_set_hard_forks(
 		client,
+		justification_import_period,
 		genesis_authorities_provider,
 		select_chain,
 		Default::default(),
@@ -540,6 +549,7 @@ pub struct AuthoritySetHardFork<Block: BlockT> {
 /// given static authorities.
 pub fn block_import_with_authority_set_hard_forks<BE, Block: BlockT, Client, SC>(
 	client: Arc<Client>,
+	justification_import_period: u32,
 	genesis_authorities_provider: &dyn GenesisAuthoritySetProvider<Block>,
 	select_chain: SC,
 	authority_set_hard_forks: Vec<AuthoritySetHardFork<Block>>,
@@ -599,6 +609,7 @@ where
 	Ok((
 		GrandpaBlockImport::new(
 			client.clone(),
+			justification_import_period,
 			select_chain.clone(),
 			persistent_data.authority_set.clone(),
 			voter_commands_tx,