grandpa: update to finality-grandpa v0.12.0 (#5853)

* grandpa: update to v0.12.0

* grandpa: fix tests

* grandpa: better validation of authority set invariants

* grandpa: avoid duplicating invalid authority list check

* grandpa: add missing doc

* grandpa: better validation of expect proofs

* grandpa: fix test compilation

* grandpa: fix tests

* grandpa: add test for AuthoritySet invariants

* grandpa: bump finality-grandpa to v0.12.0

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

@@ -29,6 +29,15 @@ use std::fmt::Debug;
 use std::ops::Add;
 use std::sync::Arc;
 
+/// Error type returned on operations on the `AuthoritySet`.
+#[derive(Debug, derive_more::Display, derive_more::From)]
+pub enum Error<E> {
+	#[display("Invalid authority set, either empty or with an authority weight set to 0.")]
+	InvalidAuthoritySet,
+	#[display(fmt = "Invalid operation in the pending changes tree: {}", _0)]
+	ForkTree(fork_tree::Error<E>),
+}
+
 /// A shared authority set.
 pub(crate) struct SharedAuthoritySet<H, N> {
 	inner: Arc<RwLock<AuthoritySet<H, N>>>,
@@ -64,7 +73,11 @@ where N: Add<Output=N> + Ord + Clone + Debug,
 
 	/// Get the current authorities and their weights (for the current set ID).
 	pub(crate) fn current_authorities(&self) -> VoterSet<AuthorityId> {
-		self.inner.read().current_authorities.iter().cloned().collect()
+		VoterSet::new(self.inner.read().current_authorities.iter().cloned()).expect(
+			"current_authorities is non-empty and weights are non-zero; \
+			 constructor and all mutating operations on `AuthoritySet` ensure this; \
+			 qed.",
+		)
 	}
 }
 
@@ -88,7 +101,7 @@ pub(crate) struct Status<H, N> {
 #[derive(Debug, Clone, Encode, Decode, PartialEq)]
 pub(crate) struct AuthoritySet<H, N> {
 	pub(crate) current_authorities: AuthorityList,
-	pub(crate) set_id: u64,
+	set_id: u64,
 	// Tree of pending standard changes across forks. Standard changes are
 	// enacted on finality and must be enacted (i.e. finalized) in-order across
 	// a given branch
@@ -96,21 +109,49 @@ pub(crate) struct AuthoritySet<H, N> {
 	// Pending forced changes across different forks (at most one per fork).
 	// Forced changes are enacted on block depth (not finality), for this reason
 	// only one forced change should exist per fork.
-	pub(crate) pending_forced_changes: Vec<PendingChange<H, N>>,
+	pending_forced_changes: Vec<PendingChange<H, N>>,
 }
 
 impl<H, N> AuthoritySet<H, N>
 where H: PartialEq,
 	  N: Ord,
 {
+	// authority sets must be non-empty and all weights must be greater than 0
+	fn invalid_authority_list(authorities: &AuthorityList) -> bool {
+		authorities.is_empty() || authorities.iter().any(|(_, w)| *w == 0)
+	}
+
 	/// Get a genesis set with given authorities.
-	pub(crate) fn genesis(initial: AuthorityList) -> Self {
-		AuthoritySet {
+	pub(crate) fn genesis(initial: AuthorityList) -> Option<Self> {
+		if Self::invalid_authority_list(&initial) {
+			return None;
+		}
+
+		Some(AuthoritySet {
 			current_authorities: initial,
 			set_id: 0,
 			pending_standard_changes: ForkTree::new(),
 			pending_forced_changes: Vec::new(),
+		})
+	}
+
+	/// Create a new authority set.
+	pub(crate) fn new(
+		authorities: AuthorityList,
+		set_id: u64,
+		pending_standard_changes: ForkTree<H, N, PendingChange<H, N>>,
+		pending_forced_changes: Vec<PendingChange<H, N>>,
+	) -> Option<Self> {
+		if Self::invalid_authority_list(&authorities) {
+			return None;
 		}
+
+		Some(AuthoritySet {
+			current_authorities: authorities,
+			set_id,
+			pending_standard_changes,
+			pending_forced_changes,
+		})
 	}
 
 	/// Get the current set id and a reference to the current authority set.
@@ -128,9 +169,9 @@ where
 		&mut self,
 		pending: PendingChange<H, N>,
 		is_descendent_of: &F,
-	) -> Result<(), fork_tree::Error<E>> where
+	) -> Result<(), Error<E>> where
 		F: Fn(&H, &H) -> Result<bool, E>,
-		E:  std::error::Error,
+		E: std::error::Error,
 	{
 		let hash = pending.canon_hash.clone();
 		let number = pending.canon_height.clone();
@@ -159,15 +200,16 @@ where
 		&mut self,
 		pending: PendingChange<H, N>,
 		is_descendent_of: &F,
-	) -> Result<(), fork_tree::Error<E>> where
+	) -> Result<(), Error<E>> where
 		F: Fn(&H, &H) -> Result<bool, E>,
-		E:  std::error::Error,
+		E: std::error::Error,
 	{
 		for change in self.pending_forced_changes.iter() {
 			if change.canon_hash == pending.canon_hash ||
-				is_descendent_of(&change.canon_hash, &pending.canon_hash)?
+				is_descendent_of(&change.canon_hash, &pending.canon_hash)
+					.map_err(fork_tree::Error::Client)?
 			{
-				return Err(fork_tree::Error::UnfinalizedAncestor);
+				return Err(fork_tree::Error::UnfinalizedAncestor.into());
 			}
 		}
 
@@ -201,10 +243,14 @@ where
 		&mut self,
 		pending: PendingChange<H, N>,
 		is_descendent_of: &F,
-	) -> Result<(), fork_tree::Error<E>> where
+	) -> Result<(), Error<E>> where
 		F: Fn(&H, &H) -> Result<bool, E>,
-		E:  std::error::Error,
+		E: std::error::Error,
 	{
+		if Self::invalid_authority_list(&pending.next_authorities) {
+			return Err(Error::InvalidAuthoritySet);
+		}
+
 		match pending.delay_kind {
 			DelayKind::Best { .. } => {
 				self.add_forced_change(pending, is_descendent_of)
@@ -309,9 +355,9 @@ where
 		finalized_number: N,
 		is_descendent_of: &F,
 		initial_sync: bool,
-	) -> Result<Status<H, N>, fork_tree::Error<E>>
-		where F: Fn(&H, &H) -> Result<bool, E>,
-			  E: std::error::Error,
+	) -> Result<Status<H, N>, Error<E>> where
+		F: Fn(&H, &H) -> Result<bool, E>,
+		E: std::error::Error,
 	{
 		let mut status = Status {
 			changed: false,
@@ -370,16 +416,16 @@ where
 		finalized_hash: H,
 		finalized_number: N,
 		is_descendent_of: &F,
-	) -> Result<Option<bool>, fork_tree::Error<E>>
-	where F: Fn(&H, &H) -> Result<bool, E>,
-		  E: std::error::Error,
+	) -> Result<Option<bool>, Error<E>> where
+		F: Fn(&H, &H) -> Result<bool, E>,
+		E: std::error::Error,
 	{
 		self.pending_standard_changes.finalizes_any_with_descendent_if(
 			&finalized_hash,
 			finalized_number.clone(),
 			is_descendent_of,
 			|change| change.effective_number() == finalized_number
-		)
+		).map_err(Error::ForkTree)
 	}
 }
 
@@ -457,8 +503,10 @@ mod tests {
 
 	#[test]
 	fn current_limit_filters_min() {
+		let current_authorities = vec![(AuthorityId::from_slice(&[1; 32]), 1)];
+
 		let mut authorities = AuthoritySet {
-			current_authorities: Vec::new(),
+			current_authorities: current_authorities.clone(),
 			set_id: 0,
 			pending_standard_changes: ForkTree::new(),
 			pending_forced_changes: Vec::new(),
@@ -466,7 +514,7 @@ mod tests {
 
 		let change = |height| {
 			PendingChange {
-				next_authorities: Vec::new(),
+				next_authorities: current_authorities.clone(),
 				delay: 0,
 				canon_height: height,
 				canon_hash: height.to_string(),
@@ -502,15 +550,17 @@ mod tests {
 
 	#[test]
 	fn changes_iterated_in_pre_order() {
+		let current_authorities = vec![(AuthorityId::from_slice(&[1; 32]), 1)];
+
 		let mut authorities = AuthoritySet {
-			current_authorities: Vec::new(),
+			current_authorities: current_authorities.clone(),
 			set_id: 0,
 			pending_standard_changes: ForkTree::new(),
 			pending_forced_changes: Vec::new(),
 		};
 
 		let change_a = PendingChange {
-			next_authorities: Vec::new(),
+			next_authorities: current_authorities.clone(),
 			delay: 10,
 			canon_height: 5,
 			canon_hash: "hash_a",
@@ -518,7 +568,7 @@ mod tests {
 		};
 
 		let change_b = PendingChange {
-			next_authorities: Vec::new(),
+			next_authorities: current_authorities.clone(),
 			delay: 0,
 			canon_height: 5,
 			canon_hash: "hash_b",
@@ -526,7 +576,7 @@ mod tests {
 		};
 
 		let change_c = PendingChange {
-			next_authorities: Vec::new(),
+			next_authorities: current_authorities.clone(),
 			delay: 5,
 			canon_height: 10,
 			canon_hash: "hash_c",
@@ -543,7 +593,7 @@ mod tests {
 
 		// forced changes are iterated last
 		let change_d = PendingChange {
-			next_authorities: Vec::new(),
+			next_authorities: current_authorities.clone(),
 			delay: 2,
 			canon_height: 1,
 			canon_hash: "hash_d",
@@ -551,7 +601,7 @@ mod tests {
 		};
 
 		let change_e = PendingChange {
-			next_authorities: Vec::new(),
+			next_authorities: current_authorities.clone(),
 			delay: 2,
 			canon_height: 0,
 			canon_hash: "hash_e",
@@ -689,7 +739,7 @@ mod tests {
 		// trying to finalize past `change_c` without finalizing `change_a` first
 		assert!(matches!(
 			authorities.apply_standard_changes("hash_d", 40, &is_descendent_of, false),
-			Err(fork_tree::Error::UnfinalizedAncestor)
+			Err(Error::ForkTree(fork_tree::Error::UnfinalizedAncestor))
 		));
 
 		let status = authorities.apply_standard_changes(
@@ -871,4 +921,74 @@ mod tests {
 			}),
 		);
 	}
+
+	#[test]
+	fn maintains_authority_list_invariants() {
+		// empty authority lists are invalid
+		assert_eq!(AuthoritySet::<(), ()>::genesis(vec![]), None);
+		assert_eq!(
+			AuthoritySet::<(), ()>::new(vec![], 0, ForkTree::new(), Vec::new()),
+			None,
+		);
+
+		let invalid_authorities_weight = vec![
+			(AuthorityId::from_slice(&[1; 32]), 5),
+			(AuthorityId::from_slice(&[2; 32]), 0),
+		];
+
+		// authority weight of zero is invalid
+		assert_eq!(
+			AuthoritySet::<(), ()>::genesis(invalid_authorities_weight.clone()),
+			None
+		);
+		assert_eq!(
+			AuthoritySet::<(), ()>::new(
+				invalid_authorities_weight.clone(),
+				0,
+				ForkTree::new(),
+				Vec::new()
+			),
+			None,
+		);
+
+		let mut authority_set =
+			AuthoritySet::<(), u64>::genesis(vec![(AuthorityId::from_slice(&[1; 32]), 5)]).unwrap();
+
+		let invalid_change_empty_authorities = PendingChange {
+			next_authorities: vec![],
+			delay: 10,
+			canon_height: 5,
+			canon_hash: (),
+			delay_kind: DelayKind::Finalized,
+		};
+
+		// pending change contains an empty authority set
+		assert!(matches!(
+			authority_set.add_pending_change(
+				invalid_change_empty_authorities.clone(),
+				&static_is_descendent_of(false)
+			),
+			Err(Error::InvalidAuthoritySet)
+		));
+
+		let invalid_change_authorities_weight = PendingChange {
+			next_authorities: invalid_authorities_weight,
+			delay: 10,
+			canon_height: 5,
+			canon_hash: (),
+			delay_kind: DelayKind::Best {
+				median_last_finalized: 0,
+			},
+		};
+
+		// pending change contains an an authority set
+		// where one authority has weight of 0
+		assert!(matches!(
+			authority_set.add_pending_change(
+				invalid_change_authorities_weight,
+				&static_is_descendent_of(false)
+			),
+			Err(Error::InvalidAuthoritySet)
+		));
+	}
 }