grandpa: avoid importing unnecessary justifications (#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

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

@@ -37,6 +37,10 @@ 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,
@@ -97,6 +101,7 @@ pub fn new_partial(
 
 	let (grandpa_block_import, grandpa_link) = sc_consensus_grandpa::block_import(
 		client.clone(),
+		GRANDPA_JUSTIFICATION_PERIOD,
 		&client,
 		select_chain.clone(),
 		telemetry.as_ref().map(|x| x.handle()),
@@ -290,7 +295,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,

substrate/bin/node/cli/src/service.rs

@@ -54,6 +54,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.
@@ -193,6 +197,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()),
@@ -524,7 +529,7 @@ pub fn new_full_base(
 	let grandpa_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,

substrate/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,

substrate/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,

substrate/client/consensus/grandpa/src/environment.rs

@@ -1102,7 +1102,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(),
@@ -1315,6 +1315,38 @@ 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.
@@ -1322,7 +1354,7 @@ where
 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>,
@@ -1393,22 +1425,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();
+				let enacts_change = 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 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)?;

substrate/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,

substrate/client/consensus/grandpa/src/lib.rs

@@ -215,10 +215,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,

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

@@ -69,6 +69,8 @@ type GrandpaBlockImport = crate::GrandpaBlockImport<
 	LongestChain<substrate_test_runtime_client::Backend, Block>,
 >;
 
+const JUSTIFICATION_IMPORT_PERIOD: u32 = 32;
+
 #[derive(Default)]
 struct GrandpaTestNet {
 	peers: Vec<GrandpaPeer>,
@@ -126,6 +128,7 @@ impl TestNetFactory for GrandpaTestNet {
 		let (client, backend) = (client.as_client(), client.as_backend());
 		let (import, link) = block_import(
 			client.clone(),
+			JUSTIFICATION_IMPORT_PERIOD,
 			&self.test_config,
 			LongestChain::new(backend.clone()),
 			None,
@@ -318,7 +321,7 @@ fn initialize_grandpa(
 		let grandpa_params = GrandpaParams {
 			config: Config {
 				gossip_duration: TEST_GOSSIP_DURATION,
-				justification_period: 32,
+				justification_generation_period: 32,
 				keystore: Some(keystore),
 				name: Some(format!("peer#{}", peer_id)),
 				local_role: Role::Authority,
@@ -446,11 +449,14 @@ async fn finalize_3_voters_no_observers() {
 	let net = Arc::new(Mutex::new(net));
 	run_to_completion(20, net.clone(), peers).await;
 
-	// normally there's no justification for finalized blocks
-	assert!(
-		net.lock().peer(0).client().justifications(hashof20).unwrap().is_none(),
-		"Extra justification for block#1",
-	);
+	// all peers should have stored the justification for the best finalized block #20
+	for peer_id in 0..3 {
+		let client = net.lock().peers[peer_id].client().as_client();
+		let justification =
+			crate::aux_schema::best_justification::<_, Block>(&*client).unwrap().unwrap();
+
+		assert_eq!(justification.justification.commit.target_number, 20);
+	}
 }
 
 #[tokio::test]
@@ -470,7 +476,7 @@ async fn finalize_3_voters_1_full_observer() {
 		let grandpa_params = GrandpaParams {
 			config: Config {
 				gossip_duration: TEST_GOSSIP_DURATION,
-				justification_period: 32,
+				justification_generation_period: 32,
 				keystore: None,
 				name: Some(format!("peer#{}", peer_id)),
 				local_role: Role::Authority,
@@ -565,7 +571,7 @@ async fn transition_3_voters_twice_1_full_observer() {
 		let grandpa_params = GrandpaParams {
 			config: Config {
 				gossip_duration: TEST_GOSSIP_DURATION,
-				justification_period: 32,
+				justification_generation_period: 32,
 				keystore: Some(keystore),
 				name: Some(format!("peer#{}", peer_id)),
 				local_role: Role::Authority,
@@ -695,8 +701,8 @@ async fn justification_is_generated_periodically() {
 	let net = Arc::new(Mutex::new(net));
 	run_to_completion(32, net.clone(), peers).await;
 
-	// when block#32 (justification_period) is finalized, justification
-	// is required => generated
+	// when block#32 (justification_generation_period) is finalized,
+	// justification is required => generated
 	for i in 0..3 {
 		assert!(net.lock().peer(i).client().justifications(hashof32).unwrap().is_some());
 	}
@@ -993,7 +999,7 @@ async fn voter_persists_its_votes() {
 	let bob_network = {
 		let config = Config {
 			gossip_duration: TEST_GOSSIP_DURATION,
-			justification_period: 32,
+			justification_generation_period: 32,
 			keystore: Some(bob_keystore.clone()),
 			name: Some(format!("peer#{}", 1)),
 			local_role: Role::Authority,
@@ -1035,7 +1041,7 @@ async fn voter_persists_its_votes() {
 		let grandpa_params = GrandpaParams {
 			config: Config {
 				gossip_duration: TEST_GOSSIP_DURATION,
-				justification_period: 32,
+				justification_generation_period: 32,
 				keystore: Some(keystore),
 				name: Some(format!("peer#{}", 0)),
 				local_role: Role::Authority,
@@ -1081,7 +1087,7 @@ async fn voter_persists_its_votes() {
 		let grandpa_params = GrandpaParams {
 			config: Config {
 				gossip_duration: TEST_GOSSIP_DURATION,
-				justification_period: 32,
+				justification_generation_period: 32,
 				keystore: Some(keystore),
 				name: Some(format!("peer#{}", 0)),
 				local_role: Role::Authority,
@@ -1246,7 +1252,7 @@ async fn finalize_3_voters_1_light_observer() {
 	let observer = observer::run_grandpa_observer(
 		Config {
 			gossip_duration: TEST_GOSSIP_DURATION,
-			justification_period: 32,
+			justification_generation_period: 32,
 			keystore: None,
 			name: Some("observer".to_string()),
 			local_role: Role::Full,
@@ -1294,7 +1300,7 @@ async fn voter_catches_up_to_latest_round_when_behind() {
 		let grandpa_params = GrandpaParams {
 			config: Config {
 				gossip_duration: TEST_GOSSIP_DURATION,
-				justification_period: 32,
+				justification_generation_period: 32,
 				keystore,
 				name: Some(format!("peer#{}", peer_id)),
 				local_role: Role::Authority,
@@ -1409,7 +1415,7 @@ where
 
 	let config = Config {
 		gossip_duration: TEST_GOSSIP_DURATION,
-		justification_period: 32,
+		justification_generation_period: 32,
 		keystore,
 		name: None,
 		local_role: Role::Authority,
@@ -1903,24 +1909,19 @@ async fn imports_justification_for_regular_blocks_on_import() {
 
 	let client = net.peer(0).client().clone();
 	let (mut block_import, ..) = net.make_block_import(client.clone());
-
 	let full_client = client.as_client();
-	let builder = full_client
-		.new_block_at(full_client.chain_info().genesis_hash, Default::default(), false)
-		.unwrap();
-	let block = builder.build().unwrap().block;
 
-	let block_hash = block.hash();
+	// create a new block (without importing it)
+	let generate_block = |parent| {
+		let builder = full_client.new_block_at(parent, Default::default(), false).unwrap();
+		builder.build().unwrap().block
+	};
 
 	// create a valid justification, with one precommit targeting the block
-	let justification = {
-		let round = 1;
+	let make_justification = |round, hash, number| {
 		let set_id = 0;
 
-		let precommit = finality_grandpa::Precommit {
-			target_hash: block_hash,
-			target_number: *block.header.number(),
-		};
+		let precommit = finality_grandpa::Precommit { target_hash: hash, target_number: number };
 
 		let msg = finality_grandpa::Message::Precommit(precommit.clone());
 		let encoded = sp_consensus_grandpa::localized_payload(round, set_id, &msg);
@@ -1933,33 +1934,59 @@ async fn imports_justification_for_regular_blocks_on_import() {
 		};
 
 		let commit = finality_grandpa::Commit {
-			target_hash: block_hash,
-			target_number: *block.header.number(),
+			target_hash: hash,
+			target_number: number,
 			precommits: vec![precommit],
 		};
 
 		GrandpaJustification::from_commit(&full_client, round, commit).unwrap()
 	};
 
-	// we import the block with justification attached
-	let mut import = BlockImportParams::new(BlockOrigin::File, block.header);
-	import.justifications = Some((GRANDPA_ENGINE_ID, justification.encode()).into());
-	import.body = Some(block.extrinsics);
-	import.fork_choice = Some(ForkChoiceStrategy::LongestChain);
+	let mut generate_and_import_block_with_justification = |parent| {
+		// we import the block with justification attached
+		let block = generate_block(parent);
+		let block_hash = block.hash();
+		let justification = make_justification(1, block_hash, *block.header.number());
 
-	assert_eq!(
-		block_import.import_block(import).await.unwrap(),
-		ImportResult::Imported(ImportedAux {
-			needs_justification: false,
-			clear_justification_requests: false,
-			bad_justification: false,
-			is_new_best: true,
-			..Default::default()
-		}),
-	);
+		let mut import = BlockImportParams::new(BlockOrigin::File, block.header);
+		import.justifications = Some((GRANDPA_ENGINE_ID, justification.encode()).into());
+		import.body = Some(block.extrinsics);
+		import.fork_choice = Some(ForkChoiceStrategy::LongestChain);
+
+		assert_eq!(
+			// NOTE: we use `block_on` here because async closures are
+			// unsupported and it doesn't matter if we block in a test
+			futures::executor::block_on(block_import.import_block(import)).unwrap(),
+			ImportResult::Imported(ImportedAux {
+				needs_justification: false,
+				clear_justification_requests: false,
+				bad_justification: false,
+				is_new_best: true,
+				..Default::default()
+			}),
+		);
+
+		block_hash
+	};
+
+	let block1 =
+		generate_and_import_block_with_justification(full_client.chain_info().genesis_hash);
 
 	// the justification should be imported and available from the client
-	assert!(client.justifications(block_hash).unwrap().is_some());
+	assert!(client.justifications(block1).unwrap().is_some());
+
+	// subsequent justifications should be ignored and not imported
+	let mut parent = block1;
+	for _ in 2..JUSTIFICATION_IMPORT_PERIOD {
+		parent = generate_and_import_block_with_justification(parent);
+		assert!(client.justifications(parent).unwrap().is_none());
+	}
+
+	let block32 = generate_and_import_block_with_justification(parent);
+
+	// until we reach a block in the next justification import period, at
+	// which point we should import it
+	assert!(client.justifications(block32).unwrap().is_some());
 }
 
 #[tokio::test]