Cleaner GRANDPA RPC API for proving finality (#7339)

* grandpa: persist block number for last block of authority set

* grandpa: fix authority_set_changes field in tests

* grandpa: fix date on copyright notice

* grandpa-rpc: implement cleaner api for prove finality rpc

* grandpa-rpc: replace the old prove_finality with the new one

* grandpa: undo accidental whitespace change

* grandpa-rpc: start work on redo of the finality_proof RPC API

* grandpa: manual impl of Decode for AuthoritySet

* grandpa: add comment about appending changes for forced changes

* grandpa: flip order in set changes, tidy up some comments

* grandpa: update some of the doc comments

* grandpa: store authority set changes when applying forced changes

* grandpa: simplify finality_proof.rs

* grandpa: move checks and extend tests in finality_proof

* grandpa: address first set of review comments

* grandpa: check that set changes have well-defined start

* grandpa: rework prove_finality and assocated tests

* grandpa: make AuthoritySetChanges tuple struct

* grandpa: add assertions for tracking auth set changes

* grandpa: remove StorageAndProofProvider trait

* grandpa: return more informative results for unexpected input to RPC

* grandpa: tiny tweak to error msg

* grandpa: fix tests

* grandpa: add error specific to finality_proof

* grandpa: fix review comments

* grandpa: proper migration to new AuthoritySet

* grandpa: fix long lines

* grandpa: fix unused warning after merge

Co-authored-by: André Silva <andrerfosilva@gmail.com>

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

@@ -109,8 +109,11 @@ pub fn new_partial(config: &Configuration) -> Result<sc_service::PartialComponen
 		let shared_voter_state = grandpa::SharedVoterState::empty();
 		let rpc_setup = shared_voter_state.clone();
 
-		let finality_proof_provider =
+		let finality_proof_provider = grandpa::FinalityProofProvider::new_for_service(
-			grandpa::FinalityProofProvider::new_for_service(backend.clone(), client.clone());
+			backend.clone(),
+			client.clone(),
+			Some(shared_authority_set.clone()),
+		);
 
 		let babe_config = babe_link.config().clone();
 		let shared_epoch_changes = babe_link.epoch_changes().clone();

substrate/client/finality-grandpa/rpc/src/error.rs

@@ -30,7 +30,7 @@ pub enum Error {
 	VoterStateReportsUnreasonablyLargeNumbers,
 	/// GRANDPA prove finality failed.
 	#[display(fmt = "GRANDPA prove finality rpc failed: {}", _0)]
-	ProveFinalityFailed(sp_blockchain::Error),
+	ProveFinalityFailed(sc_finality_grandpa::FinalityProofError),
 }
 
 /// The error codes returned by jsonrpc.

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

@@ -22,18 +22,16 @@ use sc_finality_grandpa::FinalityProofProvider;
 use sp_runtime::traits::{Block as BlockT, NumberFor};
 
 #[derive(Serialize, Deserialize)]
-pub struct EncodedFinalityProofs(pub sp_core::Bytes);
+pub struct EncodedFinalityProof(pub sp_core::Bytes);
 
 /// Local trait mainly to allow mocking in tests.
 pub trait RpcFinalityProofProvider<Block: BlockT> {
-	/// Return finality proofs for the given authorities set id, if it is provided, otherwise the
+	/// Prove finality for the given block number by returning a Justification for the last block of
-	/// current one will be used.
+	/// the authority set.
 	fn rpc_prove_finality(
 		&self,
-		begin: Block::Hash,
+		block: NumberFor<Block>,
-		end: Block::Hash,
+	) -> Result<Option<EncodedFinalityProof>, sc_finality_grandpa::FinalityProofError>;
-		authorities_set_id: u64,
-	) -> Result<Option<EncodedFinalityProofs>, sp_blockchain::Error>;
 }
 
 impl<B, Block> RpcFinalityProofProvider<Block> for FinalityProofProvider<B, Block>
@@ -44,11 +42,9 @@ where
 {
 	fn rpc_prove_finality(
 		&self,
-		begin: Block::Hash,
+		block: NumberFor<Block>,
-		end: Block::Hash,
+	) -> Result<Option<EncodedFinalityProof>, sc_finality_grandpa::FinalityProofError> {
-		authorities_set_id: u64,
+		self.prove_finality(block)
-	) -> Result<Option<EncodedFinalityProofs>, sp_blockchain::Error> {
+			.map(|x| x.map(|y| EncodedFinalityProof(y.into())))
-		self.prove_finality(begin, end, authorities_set_id)
-			.map(|x| x.map(|y| EncodedFinalityProofs(y.into())))
 	}
 }

substrate/client/finality-grandpa/rpc/src/lib.rs

@@ -37,9 +37,9 @@ mod notification;
 mod report;
 
 use sc_finality_grandpa::GrandpaJustificationStream;
-use sp_runtime::traits::Block as BlockT;
+use sp_runtime::traits::{Block as BlockT, NumberFor};
 
-use finality::{EncodedFinalityProofs, RpcFinalityProofProvider};
+use finality::{EncodedFinalityProof, RpcFinalityProofProvider};
 use report::{ReportAuthoritySet, ReportVoterState, ReportedRoundStates};
 use notification::JustificationNotification;
 
@@ -48,7 +48,7 @@ type FutureResult<T> =
 
 /// Provides RPC methods for interacting with GRANDPA.
 #[rpc]
-pub trait GrandpaApi<Notification, Hash> {
+pub trait GrandpaApi<Notification, Hash, Number> {
 	/// RPC Metadata
 	type Metadata;
 
@@ -82,15 +82,13 @@ pub trait GrandpaApi<Notification, Hash> {
 		id: SubscriptionId
 	) -> jsonrpc_core::Result<bool>;
 
-	/// Prove finality for the range (begin; end] hash. Returns None if there are no finalized blocks
+	/// Prove finality for the given block number by returning the Justification for the last block
-	/// unknown in the range. If no authorities set is provided, the current one will be attempted.
+	/// in the set and all the intermediary headers to link them together.
 	#[rpc(name = "grandpa_proveFinality")]
 	fn prove_finality(
 		&self,
-		begin: Hash,
+		block: Number,
-		end: Hash,
+	) -> FutureResult<Option<EncodedFinalityProof>>;
-		authorities_set_id: Option<u64>,
-	) -> FutureResult<Option<EncodedFinalityProofs>>;
 }
 
 /// Implements the GrandpaApi RPC trait for interacting with GRANDPA.
@@ -127,7 +125,8 @@ impl<AuthoritySet, VoterState, Block: BlockT, ProofProvider>
 	}
 }
 
-impl<AuthoritySet, VoterState, Block, ProofProvider> GrandpaApi<JustificationNotification, Block::Hash>
+impl<AuthoritySet, VoterState, Block, ProofProvider>
+	GrandpaApi<JustificationNotification, Block::Hash, NumberFor<Block>>
 	for GrandpaRpcHandler<AuthoritySet, VoterState, Block, ProofProvider>
 where
 	VoterState: ReportVoterState + Send + Sync + 'static,
@@ -171,16 +170,9 @@ where
 
 	fn prove_finality(
 		&self,
-		begin: Block::Hash,
+		block: NumberFor<Block>,
-		end: Block::Hash,
+	) -> FutureResult<Option<EncodedFinalityProof>> {
-		authorities_set_id: Option<u64>,
+		let result = self.finality_proof_provider.rpc_prove_finality(block);
-	) -> FutureResult<Option<EncodedFinalityProofs>> {
-		// If we are not provided a set_id, try with the current one.
-		let authorities_set_id = authorities_set_id
-			.unwrap_or_else(|| self.authority_set.get().0);
-		let result = self
-			.finality_proof_provider
-			.rpc_prove_finality(begin, end, authorities_set_id);
 		let future = async move { result }.boxed();
 		Box::new(
 			future
@@ -204,7 +196,7 @@ mod tests {
 	use sc_block_builder::BlockBuilder;
 	use sc_finality_grandpa::{
 		report, AuthorityId, GrandpaJustificationSender, GrandpaJustification,
-		FinalityProofFragment,
+		FinalityProof,
 	};
 	use sp_blockchain::HeaderBackend;
 	use sp_consensus::RecordProof;
@@ -223,7 +215,7 @@ mod tests {
 	struct EmptyVoterState;
 
 	struct TestFinalityProofProvider {
-		finality_proofs: Vec<FinalityProofFragment<Header>>,
+		finality_proof: Option<FinalityProof<Header>>,
 	}
 
 	fn voters() -> HashSet<AuthorityId> {
@@ -262,11 +254,15 @@ mod tests {
 	impl<Block: BlockT> RpcFinalityProofProvider<Block> for TestFinalityProofProvider {
 		fn rpc_prove_finality(
 			&self,
-			_begin: Block::Hash,
+			_block: NumberFor<Block>
-			_end: Block::Hash,
+		) -> Result<Option<EncodedFinalityProof>, sc_finality_grandpa::FinalityProofError> {
-			_authoritites_set_id: u64,
+			Ok(Some(EncodedFinalityProof(
-		) -> Result<Option<EncodedFinalityProofs>, sp_blockchain::Error> {
+				self.finality_proof
-			Ok(Some(EncodedFinalityProofs(self.finality_proofs.encode().into())))
+					.as_ref()
+					.expect("Don't call rpc_prove_finality without setting the FinalityProof")
+					.encode()
+					.into()
+			)))
 		}
 	}
 
@@ -308,12 +304,12 @@ mod tests {
 	) where
 		VoterState: ReportVoterState + Send + Sync + 'static,
 	{
-		setup_io_handler_with_finality_proofs(voter_state, Default::default())
+		setup_io_handler_with_finality_proofs(voter_state, None)
 	}
 
 	fn setup_io_handler_with_finality_proofs<VoterState>(
 		voter_state: VoterState,
-		finality_proofs: Vec<FinalityProofFragment<Header>>,
+		finality_proof: Option<FinalityProof<Header>>,
 	) -> (
 		jsonrpc_core::MetaIoHandler<sc_rpc::Metadata>,
 		GrandpaJustificationSender<Block>,
@@ -321,7 +317,7 @@ mod tests {
 		VoterState: ReportVoterState + Send + Sync + 'static,
 	{
 		let (justification_sender, justification_stream) = GrandpaJustificationStream::channel();
-		let finality_proof_provider = Arc::new(TestFinalityProofProvider { finality_proofs });
+		let finality_proof_provider = Arc::new(TestFinalityProofProvider { finality_proof });
 
 		let handler = GrandpaRpcHandler::new(
 			TestAuthoritySet,
@@ -520,29 +516,24 @@ mod tests {
 
 	#[test]
 	fn prove_finality_with_test_finality_proof_provider() {
-		let finality_proofs = vec![FinalityProofFragment {
+		let finality_proof = FinalityProof {
 			block: header(42).hash(),
 			justification: create_justification().encode(),
 			unknown_headers: vec![header(2)],
-			authorities_proof: None,
+		};
-		}];
 		let (io,  _) = setup_io_handler_with_finality_proofs(
 			TestVoterState,
-			finality_proofs.clone(),
+			Some(finality_proof.clone()),
 		);
 
-		let request = "{\"jsonrpc\":\"2.0\",\"method\":\"grandpa_proveFinality\",\"params\":[\
+		let request =
-			\"0x0000000000000000000000000000000000000000000000000000000000000000\",\
+			"{\"jsonrpc\":\"2.0\",\"method\":\"grandpa_proveFinality\",\"params\":[42],\"id\":1}";
-			\"0x0000000000000000000000000000000000000000000000000000000000000001\",\
-			42\
-		],\"id\":1}";
 
 		let meta = sc_rpc::Metadata::default();
 		let resp = io.handle_request_sync(request, meta);
 		let mut resp: serde_json::Value = serde_json::from_str(&resp.unwrap()).unwrap();
 		let result: sp_core::Bytes = serde_json::from_value(resp["result"].take()).unwrap();
-		let fragments: Vec<FinalityProofFragment<Header>> =
+		let finality_proof_rpc: FinalityProof<Header> = Decode::decode(&mut &result[..]).unwrap();
-			Decode::decode(&mut &result[..]).unwrap();
+		assert_eq!(finality_proof_rpc, finality_proof);
-		assert_eq!(fragments, finality_proofs);
 	}
 }

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

@@ -114,6 +114,11 @@ where N: Add<Output=N> + Ord + Clone + Debug,
 	pub fn clone_inner(&self) -> AuthoritySet<H, N> {
 		self.inner.read().clone()
 	}
+
+	/// Clone the inner `AuthoritySetChanges`.
+	pub fn authority_set_changes(&self) -> AuthoritySetChanges<N> {
+		self.inner.read().authority_set_changes.clone()
+	}
 }
 
 impl<H, N> From<AuthoritySet<H, N>> for SharedAuthoritySet<H, N> {
@@ -152,12 +157,16 @@ pub struct AuthoritySet<H, N> {
 	/// is lower than the last finalized block (as signaled in the forced
 	/// change) must be applied beforehand.
 	pending_forced_changes: Vec<PendingChange<H, N>>,
+	/// Track at which blocks the set id changed. This is useful when we need to prove finality for a
+	/// given block since we can figure out what set the block belongs to and when the set
+	/// started/ended.
+	authority_set_changes: AuthoritySetChanges<N>,
 }
 
 impl<H, N> AuthoritySet<H, N>
 where
 	H: PartialEq,
-	N: Ord,
+	N: Ord + Clone,
 {
 	// authority sets must be non-empty and all weights must be greater than 0
 	fn invalid_authority_list(authorities: &AuthorityList) -> bool {
@@ -175,6 +184,7 @@ where
 			set_id: 0,
 			pending_standard_changes: ForkTree::new(),
 			pending_forced_changes: Vec::new(),
+			authority_set_changes: AuthoritySetChanges::empty(),
 		})
 	}
 
@@ -184,6 +194,7 @@ where
 		set_id: u64,
 		pending_standard_changes: ForkTree<H, N, PendingChange<H, N>>,
 		pending_forced_changes: Vec<PendingChange<H, N>>,
+		authority_set_changes: AuthoritySetChanges<N>,
 	) -> Option<Self> {
 		if Self::invalid_authority_list(&authorities) {
 			return None;
@@ -194,6 +205,7 @@ where
 			set_id,
 			pending_standard_changes,
 			pending_forced_changes,
+			authority_set_changes,
 		})
 	}
 
@@ -454,6 +466,9 @@ where
 					"block" => ?change.canon_height
 				);
 
+				let mut authority_set_changes = self.authority_set_changes.clone();
+				authority_set_changes.append(self.set_id, median_last_finalized.clone());
+
 				new_set = Some((
 					median_last_finalized,
 					AuthoritySet {
@@ -461,6 +476,7 @@ where
 						set_id: self.set_id + 1,
 						pending_standard_changes: ForkTree::new(), // new set, new changes.
 						pending_forced_changes: Vec::new(),
+						authority_set_changes,
 					},
 				));
 
@@ -532,6 +548,9 @@ where
 						"block" => ?change.canon_height
 					);
 
+					// Store the set_id together with the last block_number for the set
+					self.authority_set_changes.append(self.set_id, finalized_number.clone());
+
 					self.current_authorities = change.next_authorities;
 					self.set_id += 1;
 
@@ -631,6 +650,45 @@ impl<H, N: Add<Output=N> + Clone> PendingChange<H, N> {
 	}
 }
 
+// Tracks historical authority set changes. We store the block numbers for the first block of each
+// authority set, once they have been finalized.
+#[derive(Debug, Encode, Decode, Clone, PartialEq)]
+pub struct AuthoritySetChanges<N>(pub Vec<(u64, N)>);
+
+impl<N: Ord + Clone> AuthoritySetChanges<N> {
+	pub(crate) fn empty() -> Self {
+		Self(Default::default())
+	}
+
+	pub(crate) fn append(&mut self, set_id: u64, block_number: N) {
+		self.0.push((set_id, block_number));
+	}
+
+	pub(crate) fn get_set_id(&self, block_number: N) -> Option<(u64, N)> {
+		let idx = self.0
+			.binary_search_by_key(&block_number, |(_, n)| n.clone())
+			.unwrap_or_else(|b| b);
+		if idx < self.0.len() {
+			let (set_id, block_number) = self.0[idx].clone();
+			// To make sure we have the right set we need to check that the one before it also exists.
+			if idx > 0 {
+				let (prev_set_id, _) = self.0[idx - 1usize];
+				if set_id != prev_set_id + 1u64 {
+					// Without the preceding set_id we don't have a well-defined start.
+					return None;
+				}
+			} else if set_id != 0 {
+				// If this is the first index, yet not the first set id then it's not well-defined
+				// that we are in the right set id.
+				return None;
+			}
+			Some((set_id, block_number))
+		} else {
+			None
+		}
+	}
+}
+
 #[cfg(test)]
 mod tests {
 	use super::*;
@@ -657,6 +715,7 @@ mod tests {
 			set_id: 0,
 			pending_standard_changes: ForkTree::new(),
 			pending_forced_changes: Vec::new(),
+			authority_set_changes: AuthoritySetChanges::empty(),
 		};
 
 		let change = |height| {
@@ -704,6 +763,7 @@ mod tests {
 			set_id: 0,
 			pending_standard_changes: ForkTree::new(),
 			pending_forced_changes: Vec::new(),
+			authority_set_changes: AuthoritySetChanges::empty(),
 		};
 
 		let change_a = PendingChange {
@@ -772,6 +832,7 @@ mod tests {
 			set_id: 0,
 			pending_standard_changes: ForkTree::new(),
 			pending_forced_changes: Vec::new(),
+			authority_set_changes: AuthoritySetChanges::empty(),
 		};
 
 		let set_a = vec![(AuthorityId::from_slice(&[1; 32]), 5)];
@@ -820,6 +881,7 @@ mod tests {
 			authorities.pending_changes().collect::<Vec<_>>(),
 			vec![&change_a],
 		);
+		assert_eq!(authorities.authority_set_changes, AuthoritySetChanges::empty());
 
 		// finalizing "hash_d" will enact the change signaled at "hash_a"
 		let status = authorities.apply_standard_changes(
@@ -838,6 +900,7 @@ mod tests {
 		assert_eq!(authorities.current_authorities, set_a);
 		assert_eq!(authorities.set_id, 1);
 		assert_eq!(authorities.pending_changes().count(), 0);
+		assert_eq!(authorities.authority_set_changes, AuthoritySetChanges(vec![(0, 15)]));
 	}
 
 	#[test]
@@ -847,6 +910,7 @@ mod tests {
 			set_id: 0,
 			pending_standard_changes: ForkTree::new(),
 			pending_forced_changes: Vec::new(),
+			authority_set_changes: AuthoritySetChanges::empty(),
 		};
 
 		let set_a = vec![(AuthorityId::from_slice(&[1; 32]), 5)];
@@ -889,6 +953,7 @@ mod tests {
 			authorities.apply_standard_changes("hash_d", 40, &is_descendent_of, false),
 			Err(Error::ForkTree(fork_tree::Error::UnfinalizedAncestor))
 		));
+		assert_eq!(authorities.authority_set_changes, AuthoritySetChanges::empty());
 
 		let status = authorities.apply_standard_changes(
 			"hash_b",
@@ -902,6 +967,7 @@ mod tests {
 
 		assert_eq!(authorities.current_authorities, set_a);
 		assert_eq!(authorities.set_id, 1);
+		assert_eq!(authorities.authority_set_changes, AuthoritySetChanges(vec![(0, 15)]));
 
 		// after finalizing `change_a` it should be possible to finalize `change_c`
 		let status = authorities.apply_standard_changes(
@@ -916,6 +982,7 @@ mod tests {
 
 		assert_eq!(authorities.current_authorities, set_c);
 		assert_eq!(authorities.set_id, 2);
+		assert_eq!(authorities.authority_set_changes, AuthoritySetChanges(vec![(0, 15), (1, 40)]));
 	}
 
 	#[test]
@@ -925,6 +992,7 @@ mod tests {
 			set_id: 0,
 			pending_standard_changes: ForkTree::new(),
 			pending_forced_changes: Vec::new(),
+			authority_set_changes: AuthoritySetChanges::empty(),
 		};
 
 		let set_a = vec![(AuthorityId::from_slice(&[1; 32]), 5)];
@@ -991,6 +1059,7 @@ mod tests {
 			set_id: 0,
 			pending_standard_changes: ForkTree::new(),
 			pending_forced_changes: Vec::new(),
+			authority_set_changes: AuthoritySetChanges::empty(),
 		};
 
 		let set_a = vec![(AuthorityId::from_slice(&[1; 32]), 5)];
@@ -1074,6 +1143,7 @@ mod tests {
 					set_id: 1,
 					pending_standard_changes: ForkTree::new(),
 					pending_forced_changes: Vec::new(),
+					authority_set_changes: AuthoritySetChanges(vec![(0, 42)]),
 				},
 			)
 		);
@@ -1087,6 +1157,7 @@ mod tests {
 			set_id: 0,
 			pending_standard_changes: ForkTree::new(),
 			pending_forced_changes: Vec::new(),
+			authority_set_changes: AuthoritySetChanges::empty(),
 		};
 
 		let set_a = vec![(AuthorityId::from_slice(&[1; 32]), 5)];
@@ -1125,6 +1196,7 @@ mod tests {
 			set_id: 0,
 			pending_standard_changes: ForkTree::new(),
 			pending_forced_changes: Vec::new(),
+			authority_set_changes: AuthoritySetChanges::empty(),
 		};
 
 		// effective at #15
@@ -1179,22 +1251,26 @@ mod tests {
 			authorities.apply_forced_changes("hash_d45", 45, &static_is_descendent_of(true), false),
 			Err(Error::ForcedAuthoritySetChangeDependencyUnsatisfied(15))
 		));
+		assert_eq!(authorities.authority_set_changes, AuthoritySetChanges::empty());
 
 		// we apply the first pending standard change at #15
 		authorities
 			.apply_standard_changes("hash_a15", 15, &static_is_descendent_of(true), false)
 			.unwrap();
+		assert_eq!(authorities.authority_set_changes, AuthoritySetChanges(vec![(0, 15)]));
 
 		// but the forced change still depends on the next standard change
 		assert!(matches!(
 			authorities.apply_forced_changes("hash_d", 45, &static_is_descendent_of(true), false),
 			Err(Error::ForcedAuthoritySetChangeDependencyUnsatisfied(20))
 		));
+		assert_eq!(authorities.authority_set_changes, AuthoritySetChanges(vec![(0, 15)]));
 
 		// we apply the pending standard change at #20
 		authorities
 			.apply_standard_changes("hash_b", 20, &static_is_descendent_of(true), false)
 			.unwrap();
+		assert_eq!(authorities.authority_set_changes, AuthoritySetChanges(vec![(0, 15), (1, 20)]));
 
 		// afterwards the forced change at #45 can already be applied since it signals
 		// that finality stalled at #31, and the next pending standard change is effective
@@ -1211,9 +1287,11 @@ mod tests {
 					set_id: 3,
 					pending_standard_changes: ForkTree::new(),
 					pending_forced_changes: Vec::new(),
+					authority_set_changes: AuthoritySetChanges(vec![(0, 15), (1, 20), (2, 31)]),
 				}
 			),
 		);
+		assert_eq!(authorities.authority_set_changes, AuthoritySetChanges(vec![(0, 15), (1, 20)]));
 	}
 
 	#[test]
@@ -1225,6 +1303,7 @@ mod tests {
 			set_id: 0,
 			pending_standard_changes: ForkTree::new(),
 			pending_forced_changes: Vec::new(),
+			authority_set_changes: AuthoritySetChanges::empty(),
 		};
 
 		let new_set = current_authorities.clone();
@@ -1343,7 +1422,13 @@ mod tests {
 		// empty authority lists are invalid
 		assert_eq!(AuthoritySet::<(), ()>::genesis(vec![]), None);
 		assert_eq!(
-			AuthoritySet::<(), ()>::new(vec![], 0, ForkTree::new(), Vec::new()),
+			AuthoritySet::<(), ()>::new(
+				vec![],
+				0,
+				ForkTree::new(),
+				Vec::new(),
+				AuthoritySetChanges::empty(),
+			),
 			None,
 		);
 
@@ -1362,7 +1447,8 @@ mod tests {
 				invalid_authorities_weight.clone(),
 				0,
 				ForkTree::new(),
-				Vec::new()
+				Vec::new(),
+				AuthoritySetChanges::empty(),
 			),
 			None,
 		);
@@ -1417,6 +1503,7 @@ mod tests {
 			set_id: 0,
 			pending_standard_changes: ForkTree::new(),
 			pending_forced_changes: Vec::new(),
+			authority_set_changes: AuthoritySetChanges::empty(),
 		};
 
 		let new_set = current_authorities.clone();
@@ -1512,4 +1599,32 @@ mod tests {
 			"D"
 		);
 	}
+
+	#[test]
+	fn authority_set_changes_for_complete_data() {
+		let mut authority_set_changes = AuthoritySetChanges::empty();
+		authority_set_changes.append(0, 41);
+		authority_set_changes.append(1, 81);
+		authority_set_changes.append(2, 121);
+
+		assert_eq!(authority_set_changes.get_set_id(20), Some((0, 41)));
+		assert_eq!(authority_set_changes.get_set_id(40), Some((0, 41)));
+		assert_eq!(authority_set_changes.get_set_id(41), Some((0, 41)));
+		assert_eq!(authority_set_changes.get_set_id(42), Some((1, 81)));
+		assert_eq!(authority_set_changes.get_set_id(141), None);
+	}
+
+	#[test]
+	fn authority_set_changes_for_incomplete_data() {
+		let mut authority_set_changes = AuthoritySetChanges::empty();
+		authority_set_changes.append(2, 41);
+		authority_set_changes.append(3, 81);
+		authority_set_changes.append(4, 121);
+
+		assert_eq!(authority_set_changes.get_set_id(20), None);
+		assert_eq!(authority_set_changes.get_set_id(40), None);
+		assert_eq!(authority_set_changes.get_set_id(41), None);
+		assert_eq!(authority_set_changes.get_set_id(42), Some((3, 81)));
+		assert_eq!(authority_set_changes.get_set_id(141), None);
+	}
 }

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

@@ -28,7 +28,9 @@ use sp_runtime::traits::{Block as BlockT, NumberFor};
 use log::{info, warn};
 use sp_finality_grandpa::{AuthorityList, SetId, RoundNumber};
 
-use crate::authorities::{AuthoritySet, SharedAuthoritySet, PendingChange, DelayKind};
+use crate::authorities::{
+	AuthoritySet, AuthoritySetChanges, SharedAuthoritySet, PendingChange, DelayKind,
+};
 use crate::environment::{
 	CompletedRound, CompletedRounds, CurrentRounds, HasVoted, SharedVoterSetState, VoterSetState,
 };
@@ -39,7 +41,7 @@ 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 CURRENT_VERSION: u32 = 2;
+const CURRENT_VERSION: u32 = 3;
 
 /// The voter set state.
 #[derive(Debug, Clone, Encode, Decode)]
@@ -69,8 +71,9 @@ struct V0AuthoritySet<H, N> {
 }
 
 impl<H, N> Into<AuthoritySet<H, N>> for V0AuthoritySet<H, N>
-where H: Clone + Debug + PartialEq,
+where
-	  N: Clone + Debug + Ord,
+	H: Clone + Debug + PartialEq,
+	N: Clone + Debug + Ord,
 {
 	fn into(self) -> AuthoritySet<H, N> {
 		let mut pending_standard_changes = ForkTree::new();
@@ -101,19 +104,46 @@ where H: Clone + Debug + PartialEq,
 			self.set_id,
 			pending_standard_changes,
 			Vec::new(),
+			AuthoritySetChanges::empty(),
 		);
 
 		authority_set.expect("current_authorities is non-empty and weights are non-zero; qed.")
 	}
 }
 
-pub(crate) fn load_decode<B: AuxStore, T: Decode>(backend: &B, key: &[u8]) -> ClientResult<Option<T>> {
+impl<H, N> Into<AuthoritySet<H, N>> for V2AuthoritySet<H, N>
+where
+	H: Clone + Debug + PartialEq,
+	N: Clone + Debug + Ord,
+{
+	fn into(self) -> AuthoritySet<H, N> {
+		AuthoritySet::new(
+			self.current_authorities,
+			self.set_id,
+			self.pending_standard_changes,
+			self.pending_forced_changes,
+			AuthoritySetChanges::empty(),
+		)
+		.expect("current_authorities is non-empty and weights are non-zero; qed.")
+	}
+}
+
+#[derive(Debug, Clone, Encode, Decode, PartialEq)]
+struct V2AuthoritySet<H, N> {
+	current_authorities: AuthorityList,
+	set_id: u64,
+	pending_standard_changes: ForkTree<H, N, PendingChange<H, N>>,
+	pending_forced_changes: Vec<PendingChange<H, N>>,
+}
+
+pub(crate) fn load_decode<B: AuxStore, T: Decode>(
+	backend: &B, 
+	key: &[u8]
+) -> ClientResult<Option<T>> {
 	match backend.get_aux(key)? {
 		None => Ok(None),
 		Some(t) => T::decode(&mut &t[..])
-			.map_err(
+			.map_err(|e| ClientError::Backend(format!("GRANDPA DB is corrupted: {}", e.what())))
-				|e| ClientError::Backend(format!("GRANDPA DB is corrupted: {}", e.what())),
-			)
 			.map(Some)
 	}
 }
@@ -127,11 +157,15 @@ pub(crate) struct PersistentData<Block: BlockT> {
 fn migrate_from_version0<Block: BlockT, B, G>(
 	backend: &B,
 	genesis_round: &G,
-) -> ClientResult<Option<(
+) -> ClientResult<
-	AuthoritySet<Block::Hash, NumberFor<Block>>,
+	Option<(
-	VoterSetState<Block>,
+		AuthoritySet<Block::Hash, NumberFor<Block>>,
-)>> where B: AuxStore,
+		VoterSetState<Block>,
-		  G: Fn() -> RoundState<Block::Hash, NumberFor<Block>>,
+	)>,
+>
+where
+	B: AuxStore,
+	G: Fn() -> RoundState<Block::Hash, NumberFor<Block>>,
 {
 	CURRENT_VERSION.using_encoded(|s|
 		backend.insert_aux(&[(VERSION_KEY, s)], &[])
@@ -144,18 +178,20 @@ fn migrate_from_version0<Block: BlockT, B, G>(
 		let new_set: AuthoritySet<Block::Hash, NumberFor<Block>> = old_set.into();
 		backend.insert_aux(&[(AUTHORITY_SET_KEY, new_set.encode().as_slice())], &[])?;
 
-		let (last_round_number, last_round_state) = match load_decode::<_, V0VoterSetState<Block::Hash, NumberFor<Block>>>(
+		let (last_round_number, last_round_state) = match load_decode::<
-			backend,
+			_,
-			SET_STATE_KEY,
+			V0VoterSetState<Block::Hash, NumberFor<Block>>,
-		)? {
+		>(backend, SET_STATE_KEY)?
+		{
 			Some((number, state)) => (number, state),
 			None => (0, genesis_round()),
 		};
 
 		let set_id = new_set.set_id;
 
-		let base = last_round_state.prevote_ghost
+		let base = last_round_state.prevote_ghost.expect(
-			.expect("state is for completed round; completed rounds must have a prevote ghost; qed.");
+			"state is for completed round; completed rounds must have a prevote ghost; qed."
+		);
 
 		let mut current_rounds = CurrentRounds::new();
 		current_rounds.insert(last_round_number + 1, HasVoted::No);
@@ -185,11 +221,15 @@ fn migrate_from_version0<Block: BlockT, B, G>(
 fn migrate_from_version1<Block: BlockT, B, G>(
 	backend: &B,
 	genesis_round: &G,
-) -> ClientResult<Option<(
+) -> ClientResult<
-	AuthoritySet<Block::Hash, NumberFor<Block>>,
+	Option<(
-	VoterSetState<Block>,
+		AuthoritySet<Block::Hash, NumberFor<Block>>,
-)>> where B: AuxStore,
+		VoterSetState<Block>,
-		  G: Fn() -> RoundState<Block::Hash, NumberFor<Block>>,
+	)>,
+>
+where
+	B: AuxStore,
+	G: Fn() -> RoundState<Block::Hash, NumberFor<Block>>,
 {
 	CURRENT_VERSION.using_encoded(|s|
 		backend.insert_aux(&[(VERSION_KEY, s)], &[])
@@ -257,17 +297,64 @@ fn migrate_from_version1<Block: BlockT, B, G>(
 	Ok(None)
 }
 
+fn migrate_from_version2<Block: BlockT, B, G>(
+	backend: &B,
+	genesis_round: &G,
+) -> ClientResult<
+	Option<(
+		AuthoritySet<Block::Hash, NumberFor<Block>>,
+		VoterSetState<Block>,
+	)>,
+>
+where
+	B: AuxStore,
+	G: Fn() -> RoundState<Block::Hash, NumberFor<Block>>,
+{
+	CURRENT_VERSION.using_encoded(|s|
+		backend.insert_aux(&[(VERSION_KEY, s)], &[])
+	)?;
+
+	if let Some(old_set) = load_decode::<_, V2AuthoritySet<Block::Hash, NumberFor<Block>>>(
+		backend,
+		AUTHORITY_SET_KEY,
+	)? {
+		let new_set: AuthoritySet<Block::Hash, NumberFor<Block>> = old_set.into();
+		backend.insert_aux(&[(AUTHORITY_SET_KEY, new_set.encode().as_slice())], &[])?;
+
+		let set_state = match load_decode::<_, VoterSetState<Block>>(
+			backend,
+			SET_STATE_KEY,
+		)? {
+			Some(state) => state,
+			None => {
+				let state = genesis_round();
+				let base = state.prevote_ghost
+					.expect("state is for completed round; completed rounds must have a prevote ghost; qed.");
+
+				VoterSetState::live(
+					new_set.set_id,
+					&new_set,
+					base,
+				)
+			}
+		};
+
+		return Ok(Some((new_set, set_state)));
+	}
+
+	Ok(None)
+}
+
 /// Load or initialize persistent data from backend.
 pub(crate) fn load_persistent<Block: BlockT, B, G>(
 	backend: &B,
 	genesis_hash: Block::Hash,
 	genesis_number: NumberFor<Block>,
 	genesis_authorities: G,
-)
+) -> ClientResult<PersistentData<Block>>
-	-> ClientResult<PersistentData<Block>>
+where
-	where
+	B: AuxStore,
-		B: AuxStore,
+	G: FnOnce() -> ClientResult<AuthorityList>,
-		G: FnOnce() -> ClientResult<AuthorityList>,
 {
 	let version: Option<u32> = load_decode(backend, VERSION_KEY)?;
 
@@ -275,7 +362,9 @@ pub(crate) fn load_persistent<Block: BlockT, B, G>(
 
 	match version {
 		None => {
-			if let Some((new_set, set_state)) = migrate_from_version0::<Block, _, _>(backend, &make_genesis_round)? {
+			if let Some((new_set, set_state)) =
+				migrate_from_version0::<Block, _, _>(backend, &make_genesis_round)?
+			{
 				return Ok(PersistentData {
 					authority_set: new_set.into(),
 					set_state: set_state.into(),
@@ -283,7 +372,9 @@ pub(crate) fn load_persistent<Block: BlockT, B, G>(
 			}
 		},
 		Some(1) => {
-			if let Some((new_set, set_state)) = migrate_from_version1::<Block, _, _>(backend, &make_genesis_round)? {
+			if let Some((new_set, set_state)) =
+				migrate_from_version1::<Block, _, _>(backend, &make_genesis_round)?
+			{
 				return Ok(PersistentData {
 					authority_set: new_set.into(),
 					set_state: set_state.into(),
@@ -291,6 +382,16 @@ pub(crate) fn load_persistent<Block: BlockT, B, G>(
 			}
 		},
 		Some(2) => {
+			if let Some((new_set, set_state)) =
+				migrate_from_version2::<Block, _, _>(backend, &make_genesis_round)?
+			{
+				return Ok(PersistentData {
+					authority_set: new_set.into(),
+					set_state: set_state.into(),
+				});
+			}
+		}
+		Some(3) => {
 			if let Some(set) = load_decode::<_, AuthoritySet<Block::Hash, NumberFor<Block>>>(
 				backend,
 				AUTHORITY_SET_KEY,
@@ -364,7 +465,8 @@ 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
-) -> R where
+) -> R
+where
 	F: FnOnce(&[(&'static [u8], &[u8])]) -> R,
 {
 	// write new authority set state to disk.
@@ -414,8 +516,9 @@ pub(crate) fn write_concluded_round<Block: BlockT, B: AuxStore>(
 }
 
 #[cfg(test)]
-pub(crate) fn load_authorities<B: AuxStore, H: Decode, N: Decode>(backend: &B)
+pub(crate) fn load_authorities<B: AuxStore, H: Decode, N: Decode + Clone + Ord>(
-	-> Option<AuthoritySet<H, N>> {
+	backend: &B
+) -> Option<AuthoritySet<H, N>> {
 	load_decode::<_, AuthoritySet<H, N>>(backend, AUTHORITY_SET_KEY)
 		.expect("backend error")
 }
@@ -474,10 +577,14 @@ mod test {
 
 		assert_eq!(
 			load_decode::<_, u32>(&client, VERSION_KEY).unwrap(),
-			Some(2),
+			Some(3),
 		);
 
-		let PersistentData { authority_set, set_state, .. } = load_persistent::<substrate_test_runtime_client::runtime::Block, _, _>(
+		let PersistentData {
+			authority_set,
+			set_state,
+			..
+		} = load_persistent::<substrate_test_runtime_client::runtime::Block, _, _>(
 			&client,
 			H256::random(),
 			0,
@@ -491,6 +598,7 @@ mod test {
 				set_id,
 				ForkTree::new(),
 				Vec::new(),
+				AuthoritySetChanges::empty(),
 			).unwrap(),
 		);
 
@@ -535,6 +643,7 @@ mod test {
 				set_id,
 				ForkTree::new(),
 				Vec::new(),
+				AuthoritySetChanges::empty(),
 			).unwrap();
 
 			let voter_set_state = V1VoterSetState::Live(round_number, round_state.clone());
@@ -564,10 +673,14 @@ mod test {
 
 		assert_eq!(
 			load_decode::<_, u32>(&client, VERSION_KEY).unwrap(),
-			Some(2),
+			Some(3),
 		);
 
-		let PersistentData { authority_set, set_state, .. } = load_persistent::<substrate_test_runtime_client::runtime::Block, _, _>(
+		let PersistentData {
+			authority_set,
+			set_state,
+			..
+		} = load_persistent::<substrate_test_runtime_client::runtime::Block, _, _>(
 			&client,
 			H256::random(),
 			0,
@@ -581,6 +694,7 @@ mod test {
 				set_id,
 				ForkTree::new(),
 				Vec::new(),
+				AuthoritySetChanges::empty(),
 			).unwrap(),
 		);
 
@@ -605,6 +719,79 @@ mod test {
 		);
 	}
 
+	#[test]
+	fn load_decode_from_v2_migrates_data_format() {
+		let client = substrate_test_runtime_client::new();
+
+		let authorities = vec![(AuthorityId::default(), 100)];
+		let set_id = 3;
+
+		{
+			let authority_set = V2AuthoritySet::<H256, u64> {
+				current_authorities: authorities.clone(),
+				set_id,
+				pending_standard_changes: ForkTree::new(),
+				pending_forced_changes: Vec::new(),
+			};
+
+			let genesis_state = (H256::random(), 32);
+			let voter_set_state: VoterSetState<substrate_test_runtime_client::runtime::Block> =
+				VoterSetState::live(
+					set_id,
+					&authority_set.clone().into(), // Note the conversion!
+					genesis_state
+				);
+
+			client.insert_aux(
+				&[
+					(AUTHORITY_SET_KEY, authority_set.encode().as_slice()),
+					(SET_STATE_KEY, voter_set_state.encode().as_slice()),
+					(VERSION_KEY, 2u32.encode().as_slice()),
+				],
+				&[],
+			).unwrap();
+		}
+
+		assert_eq!(
+			load_decode::<_, u32>(&client, VERSION_KEY).unwrap(),
+			Some(2),
+		);
+
+		// should perform the migration
+		load_persistent::<substrate_test_runtime_client::runtime::Block, _, _>(
+			&client,
+			H256::random(),
+			0,
+			|| unreachable!(),
+		).unwrap();
+
+		assert_eq!(
+			load_decode::<_, u32>(&client, VERSION_KEY).unwrap(),
+			Some(3),
+		);
+
+		let PersistentData {
+			authority_set,
+			..
+		 } = load_persistent::<substrate_test_runtime_client::runtime::Block, _, _>(
+			&client,
+			H256::random(),
+			0,
+			|| unreachable!(),
+		).unwrap();
+
+		assert_eq!(
+			*authority_set.inner().read(),
+			AuthoritySet::new(
+				authorities.clone(),
+				set_id,
+				ForkTree::new(),
+				Vec::new(),
+				AuthoritySetChanges::empty(),
+			).unwrap(),
+		);
+	}
+
 	#[test]
 	fn write_read_concluded_rounds() {
 		let client = substrate_test_runtime_client::new();
@@ -625,7 +812,9 @@ mod test {
 		round_number.using_encoded(|n| key.extend(n));
 
 		assert_eq!(
-			load_decode::<_, CompletedRound::<substrate_test_runtime_client::runtime::Block>>(&client, &key).unwrap(),
+			load_decode::<_, CompletedRound::<substrate_test_runtime_client::runtime::Block>>(
+				&client, &key
+			).unwrap(),
 			Some(completed_round),
 		);
 	}

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

@@ -122,7 +122,7 @@ mod until_imported;
 mod voting_rule;
 
 pub use authorities::{SharedAuthoritySet, AuthoritySet};
-pub use finality_proof::{FinalityProofFragment, FinalityProofProvider, StorageAndProofProvider};
+pub use finality_proof::{FinalityProof, FinalityProofProvider, FinalityProofError};
 pub use notification::{GrandpaJustificationSender, GrandpaJustificationStream};
 pub use import::GrandpaBlockImport;
 pub use justification::GrandpaJustification;

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

@@ -32,25 +32,20 @@ use tokio::runtime::{Runtime, Handle};
 use sp_keyring::Ed25519Keyring;
 use sc_client_api::backend::TransactionFor;
 use sp_blockchain::Result;
-use sp_api::{ApiRef, StorageProof, ProvideRuntimeApi};
+use sp_api::{ApiRef, ProvideRuntimeApi};
 use substrate_test_runtime_client::runtime::BlockNumber;
 use sp_consensus::{
 	BlockOrigin, ForkChoiceStrategy, ImportedAux, BlockImportParams, ImportResult, BlockImport,
 	import_queue::BoxJustificationImport,
 };
 use std::{collections::{HashMap, HashSet}, pin::Pin};
-use parity_scale_codec::Decode;
+use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
-use sp_runtime::traits::{Block as BlockT, Header as HeaderT, HashFor};
 use sp_runtime::generic::{BlockId, DigestItem};
 use sp_core::H256;
 use sp_keystore::{SyncCryptoStorePtr, SyncCryptoStore};
 use sp_finality_grandpa::{GRANDPA_ENGINE_ID, AuthorityList, EquivocationProof, GrandpaApi, OpaqueKeyOwnershipProof};
-use sp_state_machine::{InMemoryBackend, prove_read, read_proof_check};
 
 use authorities::AuthoritySet;
-use finality_proof::{
-	AuthoritySetForFinalityProver, AuthoritySetForFinalityChecker,
-};
 use sc_block_builder::BlockBuilderProvider;
 use sc_consensus::LongestChain;
 use sc_keystore::LocalKeystore;
@@ -207,43 +202,6 @@ impl GenesisAuthoritySetProvider<Block> for TestApi {
 	}
 }
 
-impl AuthoritySetForFinalityProver<Block> for TestApi {
-	fn authorities(&self, _block: &BlockId<Block>) -> Result<AuthorityList> {
-		Ok(self.genesis_authorities.clone())
-	}
-
-	fn prove_authorities(&self, block: &BlockId<Block>) -> Result<StorageProof> {
-		let authorities = self.authorities(block)?;
-		let backend = <InMemoryBackend<HashFor<Block>>>::from(vec![
-			(None, vec![(b"authorities".to_vec(), Some(authorities.encode()))])
-		]);
-		let proof = prove_read(backend, vec![b"authorities"])
-			.expect("failure proving read from in-memory storage backend");
-		Ok(proof)
-	}
-}
-
-impl AuthoritySetForFinalityChecker<Block> for TestApi {
-	fn check_authorities_proof(
-		&self,
-		_hash: <Block as BlockT>::Hash,
-		header: <Block as BlockT>::Header,
-		proof: StorageProof,
-	) -> Result<AuthorityList> {
-		let results = read_proof_check::<HashFor<Block>, _>(
-			*header.state_root(), proof, vec![b"authorities"]
-		)
-			.expect("failure checking read proof for authorities");
-		let encoded = results.get(&b"authorities"[..])
-			.expect("returned map must contain all proof keys")
-			.as_ref()
-			.expect("authorities in proof is None");
-		let authorities = Decode::decode(&mut &encoded[..])
-			.expect("failure decoding authorities read from proof");
-		Ok(authorities)
-	}
-}
-
 const TEST_GOSSIP_DURATION: Duration = Duration::from_millis(500);
 
 fn make_ids(keys: &[Ed25519Keyring]) -> AuthorityList {