grandpa: always store justification for the latest finalized block

client/finality-grandpa-warp-sync/src/lib.rs

@@ -120,14 +120,14 @@ impl<TBlock: BlockT, TBackend: Backend<TBlock>> GrandpaWarpSyncRequestHandler<TB
 	fn handle_request(
 		&self,
 		payload: Vec<u8>,
-		pending_response: oneshot::Sender<OutgoingResponse>
+		pending_response: oneshot::Sender<OutgoingResponse>,
 	) -> Result<(), HandleRequestError>
 		where NumberFor<TBlock>: sc_finality_grandpa::BlockNumberOps,
 	{
 		let request = Request::<TBlock>::decode(&mut &payload[..])?;
 
 		let proof = WarpSyncProof::generate(
-			self.backend.blockchain(),
+			&*self.backend,
 			request.begin,
 			&self.authority_set.authority_set_changes(),
 		)?;

client/finality-grandpa-warp-sync/src/proof.rs

@@ -16,14 +16,15 @@
 
 use codec::{Decode, Encode};
 
+use sc_client_api::Backend as ClientBackend;
 use sc_finality_grandpa::{
 	find_scheduled_change, AuthoritySetChanges, BlockNumberOps, GrandpaJustification,
 };
-use sp_blockchain::Backend as BlockchainBackend;
+use sp_blockchain::{Backend as BlockchainBackend, HeaderBackend};
 use sp_finality_grandpa::{AuthorityList, SetId, GRANDPA_ENGINE_ID};
 use sp_runtime::{
 	generic::BlockId,
-	traits::{Block as BlockT, NumberFor},
+	traits::{Block as BlockT, NumberFor, One},
 };
 
 use crate::HandleRequestError;
@@ -42,11 +43,21 @@ pub struct AuthoritySetChangeProof<Block: BlockT> {
 	pub justification: GrandpaJustification<Block>,
 }
 
+/// Represents the current state of the warp sync, namely whether it is considered
+/// finished, i.e. we have proved everything up until the latest authority set, or not.
+/// When the warp sync is finished we might optionally provide a justification for the
+/// latest finalized block, which should be checked against the latest authority set.
+#[derive(Debug, Decode, Encode)]
+pub enum WarpSyncFinished<Block: BlockT> {
+	No,
+	Yes(Option<GrandpaJustification<Block>>),
+}
+
 /// An accumulated proof of multiple authority set changes.
 #[derive(Decode, Encode)]
 pub struct WarpSyncProof<Block: BlockT> {
 	proofs: Vec<AuthoritySetChangeProof<Block>>,
-	is_finished: bool,
+	is_finished: WarpSyncFinished<Block>,
 }
 
 impl<Block: BlockT> WarpSyncProof<Block> {
@@ -59,21 +70,22 @@ impl<Block: BlockT> WarpSyncProof<Block> {
 		set_changes: &AuthoritySetChanges<NumberFor<Block>>,
 	) -> Result<WarpSyncProof<Block>, HandleRequestError>
 	where
-		Backend: BlockchainBackend<Block>,
+		Backend: ClientBackend<Block>,
 	{
 		// TODO: cache best response (i.e. the one with lowest begin_number)
+		let blockchain = backend.blockchain();
 
-		let begin_number = backend
+		let begin_number = blockchain
 			.block_number_from_id(&BlockId::Hash(begin))?
 			.ok_or_else(|| HandleRequestError::InvalidRequest("Missing start block".to_string()))?;
 
-		if begin_number > backend.info().finalized_number {
+		if begin_number > blockchain.info().finalized_number {
 			return Err(HandleRequestError::InvalidRequest(
 				"Start block is not finalized".to_string(),
 			));
 		}
 
-		let canon_hash = backend.hash(begin_number)?.expect(
+		let canon_hash = blockchain.hash(begin_number)?.expect(
 			"begin number is lower than finalized number; \
 			 all blocks below finalized number must have been imported; \
 			 qed.",
@@ -86,7 +98,6 @@ impl<Block: BlockT> WarpSyncProof<Block> {
 		}
 
 		let mut proofs = Vec::new();
-
 		let mut proof_limit_reached = false;
 
 		for (_, last_block) in set_changes.iter_from(begin_number) {
@@ -95,7 +106,7 @@ impl<Block: BlockT> WarpSyncProof<Block> {
 				break;
 			}
 
-			let header = backend.header(BlockId::Number(*last_block))?.expect(
+			let header = blockchain.header(BlockId::Number(*last_block))?.expect(
 				"header number comes from previously applied set changes; must exist in db; qed.",
 			);
 
@@ -108,7 +119,7 @@ impl<Block: BlockT> WarpSyncProof<Block> {
 				break;
 			}
 
-			let justification = backend
+			let justification = blockchain
 				.justifications(BlockId::Number(*last_block))?
 				.and_then(|just| just.into_justification(GRANDPA_ENGINE_ID))
 				.expect(
@@ -125,9 +136,29 @@ impl<Block: BlockT> WarpSyncProof<Block> {
 			});
 		}
 
+		let is_finished = if proof_limit_reached {
+			WarpSyncFinished::No
+		} else {
+			let latest =
+				sc_finality_grandpa::best_justification(backend)?.filter(|justification| {
+					// the existing best justification must be for a block higher than the
+					// last authority set change. if we didn't prove any authority set
+					// change then we fallback to make sure it's higher or equal to the
+					// initial warp sync block.
+					let limit = proofs
+						.last()
+						.map(|proof| proof.justification.target().0 + One::one())
+						.unwrap_or(begin_number);
+
+					justification.target().0 >= limit
+				});
+
+			WarpSyncFinished::Yes(latest)
+		};
+
 		Ok(WarpSyncProof {
 			proofs,
-			is_finished: !proof_limit_reached,
+			is_finished,
 		})
 	}
 
@@ -160,6 +191,12 @@ impl<Block: BlockT> WarpSyncProof<Block> {
 			current_set_id += 1;
 		}
 
+		if let WarpSyncFinished::Yes(Some(ref justification)) = self.is_finished {
+			justification
+				.verify(current_set_id, &current_authorities)
+				.map_err(|err| HandleRequestError::InvalidProof(err.to_string()))?;
+		}
+
 		Ok((current_set_id, current_authorities))
 	}
 }
@@ -170,7 +207,6 @@ mod tests {
 	use codec::Encode;
 	use rand::prelude::*;
 	use sc_block_builder::BlockBuilderProvider;
-	use sc_client_api::Backend;
 	use sc_finality_grandpa::{AuthoritySetChanges, GrandpaJustification};
 	use sp_blockchain::HeaderBackend;
 	use sp_consensus::BlockOrigin;
@@ -295,8 +331,7 @@ mod tests {
 		let genesis_hash = client.hash(0).unwrap().unwrap();
 
 		let warp_sync_proof =
-			WarpSyncProof::generate(backend.blockchain(), genesis_hash, &authority_set_changes)
-				.unwrap();
+			WarpSyncProof::generate(&*backend, genesis_hash, &authority_set_changes).unwrap();
 
 		// verifying the proof should yield the last set id and authorities
 		let (new_set_id, new_authorities) = warp_sync_proof.verify(0, genesis_authorities).unwrap();

client/finality-grandpa/src/aux_schema.rs

@@ -19,27 +19,30 @@
 //! Schema for stuff in the aux-db.
 
 use std::fmt::Debug;
-use parity_scale_codec::{Encode, Decode};
-use sc_client_api::backend::AuxStore;
-use sp_blockchain::{Result as ClientResult, Error as ClientError};
-use fork_tree::ForkTree;
+
 use finality_grandpa::round::State as RoundState;
-use sp_runtime::traits::{Block as BlockT, NumberFor};
 use log::{info, warn};
-use sp_finality_grandpa::{AuthorityList, SetId, RoundNumber};
+use parity_scale_codec::{Decode, Encode};
+
+use fork_tree::ForkTree;
+use sc_client_api::backend::AuxStore;
+use sp_blockchain::{Error as ClientError, Result as ClientResult};
+use sp_finality_grandpa::{AuthorityList, RoundNumber, SetId};
+use sp_runtime::traits::{Block as BlockT, NumberFor};
 
 use crate::authorities::{
-	AuthoritySet, AuthoritySetChanges, SharedAuthoritySet, PendingChange, DelayKind,
+	AuthoritySet, AuthoritySetChanges, DelayKind, PendingChange, SharedAuthoritySet,
 };
 use crate::environment::{
 	CompletedRound, CompletedRounds, CurrentRounds, HasVoted, SharedVoterSetState, VoterSetState,
 };
-use crate::NewAuthoritySet;
+use crate::{GrandpaJustification, NewAuthoritySet};
 
 const VERSION_KEY: &[u8] = b"grandpa_schema_version";
 const SET_STATE_KEY: &[u8] = b"grandpa_completed_round";
 const CONCLUDED_ROUNDS: &[u8] = b"grandpa_concluded_rounds";
 const AUTHORITY_SET_KEY: &[u8] = b"grandpa_voters";
+const BEST_JUSTIFICATION: &[u8] = b"grandpa_best_justification";
 
 const CURRENT_VERSION: u32 = 3;
 
@@ -464,7 +467,7 @@ where
 pub(crate) fn update_authority_set<Block: BlockT, F, R>(
 	set: &AuthoritySet<Block::Hash, NumberFor<Block>>,
 	new_set: Option<&NewAuthoritySet<Block::Hash, NumberFor<Block>>>,
-	write_aux: F
+	write_aux: F,
 ) -> R
 where
 	F: FnOnce(&[(&'static [u8], &[u8])]) -> R,
@@ -492,6 +495,33 @@ where
 	}
 }
 
+/// Update the justification for the latest finalized block on-disk.
+///
+/// We always keep around the justification for the best finalized block and overwrite it
+/// as we finalize new blocks, this makes sure that we don't store useless justifications
+/// but can always prove finality of the latest block.
+pub(crate) fn update_best_justification<Block: BlockT, F, R>(
+	justification: &GrandpaJustification<Block>,
+	write_aux: F,
+) -> R
+where
+	F: FnOnce(&[(&'static [u8], &[u8])]) -> R,
+{
+	let encoded_justification = justification.encode();
+	write_aux(&[(BEST_JUSTIFICATION, &encoded_justification[..])])
+}
+
+/// Fetch the justification for the latest block finalized by GRANDPA, if any.
+pub fn best_justification<B, Block>(
+	backend: &B,
+) -> ClientResult<Option<GrandpaJustification<Block>>>
+where
+	B: AuxStore,
+	Block: BlockT,
+{
+	load_decode::<_, GrandpaJustification<Block>>(backend, BEST_JUSTIFICATION)
+}
+
 /// Write voter set state.
 pub(crate) fn write_voter_set_state<Block: BlockT, B: AuxStore>(
 	backend: &B,
@@ -517,10 +547,9 @@ pub(crate) fn write_concluded_round<Block: BlockT, B: AuxStore>(
 
 #[cfg(test)]
 pub(crate) fn load_authorities<B: AuxStore, H: Decode, N: Decode + Clone + Ord>(
-	backend: &B
+	backend: &B,
 ) -> Option<AuthoritySet<H, N>> {
-	load_decode::<_, AuthoritySet<H, N>>(backend, AUTHORITY_SET_KEY)
-		.expect("backend error")
+	load_decode::<_, AuthoritySet<H, N>>(backend, AUTHORITY_SET_KEY).expect("backend error")
 }
 
 #[cfg(test)]

client/finality-grandpa/src/environment.rs

@@ -1275,11 +1275,8 @@ where
 		// `N+1`. this assumption is required to make sure we store
 		// justifications for transition blocks which will be requested by
 		// syncing clients.
-		let justification = match justification_or_commit {
-			JustificationOrCommit::Justification(justification) => {
-				notify_justification(justification_sender, || Ok(justification.clone()));
-				Some(justification.encode())
-			},
+		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
@@ -1297,49 +1294,47 @@ where
 					}
 				}
 
-				// NOTE: the code below is a bit more verbose because we
-				// really want to avoid creating a justification if it isn't
-				// needed (e.g. if there's no subscribers), and also to avoid
-				// creating it twice. depending on the vote tree for the round,
-				// creating a justification might require multiple fetches of
-				// headers from the database.
-				let justification = || GrandpaJustification::from_commit(
+				let justification = GrandpaJustification::from_commit(
 					&client,
 					round_number,
 					commit,
-				);
-
-				if justification_required {
-					let justification = justification()?;
-					notify_justification(justification_sender, || Ok(justification.clone()));
-
-					Some(justification.encode())
-				} else {
-					notify_justification(justification_sender, justification);
+				)?;
 
-					None
-				}
+				(justification_required, justification)
 			},
 		};
 
-		debug!(target: "afg", "Finalizing blocks up to ({:?}, {})", number, hash);
+		notify_justification(justification_sender, || Ok(justification.clone()));
+
+		let persisted_justification = if justification_required {
+			Some((GRANDPA_ENGINE_ID, justification.encode()))
+		} else {
+			None
+		};
 
 		// ideally some handle to a synchronization oracle would be used
 		// to avoid unconditionally notifying.
-		let justification = justification.map(|j| (GRANDPA_ENGINE_ID, j.clone()));
 		client
-			.apply_finality(import_op, BlockId::Hash(hash), justification, true)
+			.apply_finality(import_op, BlockId::Hash(hash), persisted_justification, true)
 			.map_err(|e| {
 				warn!(target: "afg", "Error applying finality to block {:?}: {:?}", (hash, number), e);
 				e
 			})?;
+
+		debug!(target: "afg", "Finalizing blocks up to ({:?}, {})", number, hash);
+
 		telemetry!(
 			telemetry;
 			CONSENSUS_INFO;
 			"afg.finalized_blocks_up_to";
 			"number" => ?number, "hash" => ?hash,
 		);
 
+		crate::aux_schema::update_best_justification(
+			&justification,
+			|insert| apply_aux(import_op, insert, &[]),
+		)?;
+
 		let new_authorities = if let Some((canon_hash, canon_number)) = status.new_set_block {
 			// the authority set has changed.
 			let (new_id, set_ref) = authority_set.current();

client/finality-grandpa/src/justification.rs

@@ -195,6 +195,11 @@ impl<Block: BlockT> GrandpaJustification<Block> {
 
 		Ok(())
 	}
+
+	/// The target block number and hash that this justifications proves finality for.
+	pub fn target(&self) -> (NumberFor<Block>, Block::Hash) {
+		(self.commit.target_number, self.commit.target_hash)
+	}
 }
 
 /// A utility trait implementing `finality_grandpa::Chain` using a given set of headers.

client/finality-grandpa/src/lib.rs

@@ -122,6 +122,7 @@ mod until_imported;
 mod voting_rule;
 
 pub use authorities::{AuthoritySet, AuthoritySetChanges, SharedAuthoritySet};
+pub use aux_schema::best_justification;
 pub use finality_proof::{FinalityProof, FinalityProofProvider, FinalityProofError};
 pub use notification::{GrandpaJustificationSender, GrandpaJustificationStream};
 pub use import::{find_scheduled_change, find_forced_change, GrandpaBlockImport};

client/finality-grandpa/src/tests.rs

@@ -451,10 +451,19 @@ fn finalize_3_voters_1_full_observer() {
 	}
 
 	// wait for all finalized on each.
-	let wait_for = futures::future::join_all(finality_notifications)
-		.map(|_| ());
+	let wait_for = futures::future::join_all(finality_notifications).map(|_| ());
 
 	block_until_complete(wait_for, &net, &mut runtime);
+
+	// all peers should have stored the justification for the best finalized block #20
+	for peer_id in 0..4 {
+		let client = net.lock().peers[peer_id].client().as_full().unwrap();
+		let justification = crate::aux_schema::best_justification::<_, Block>(&*client)
+			.unwrap()
+			.unwrap();
+
+		assert_eq!(justification.commit.target_number, 20);
+	}
 }
 
 #[test]