grandpa: minor cleanups in communication module (#6371)

* grandpa: replace Result<(), ()> with Option<()>

* grandpa: replace &Option<T> with Option<&T>

* grandpa: cleanup local id and keystore usages

* grandpa: return bool on check_message_signature

* grandpa: fix erroneous log message on startup

* grandpa: fix test

substrate/client/finality-grandpa/src/communication/gossip.rs

@@ -750,7 +750,11 @@ impl<Block: BlockT> Inner<Block> {
 					Round(1),
 				)),
 				Some(ref mut v) => if v.set_id == set_id {
-					if self.authorities != authorities {
+					let diff_authorities =
+						self.authorities.iter().collect::<HashSet<_>>() !=
+						authorities.iter().collect();
+
+					if diff_authorities {
 						debug!(target: "afg",
 							"Gossip validator noted set {:?} twice with different authorities. \
 							Was the authority set hard forked?",
@@ -829,7 +833,7 @@ impl<Block: BlockT> Inner<Block> {
 			return Action::Discard(cost::UNKNOWN_VOTER);
 		}
 
-		if let Err(()) = sp_finality_grandpa::check_message_signature(
+		if !sp_finality_grandpa::check_message_signature(
 			&full.message.message,
 			&full.message.id,
 			&full.message.signature,
@@ -2620,12 +2624,12 @@ mod tests {
 	fn allow_noting_different_authorities_for_same_set() {
 		let (val, _) = GossipValidator::<Block>::new(config(), voter_set_state(), None);
 
-		let a1 = vec![AuthorityId::default()];
+		let a1 = vec![AuthorityId::from_slice(&[0; 32])];
 		val.note_set(SetId(1), a1.clone(), |_, _| {});
 
 		assert_eq!(val.inner().read().authorities, a1);
 
-		let a2 = vec![AuthorityId::default(), AuthorityId::default()];
+		let a2 = vec![AuthorityId::from_slice(&[1; 32]), AuthorityId::from_slice(&[2; 32])];
 		val.note_set(SetId(1), a2.clone(), |_, _| {});
 
 		assert_eq!(val.inner().read().authorities, a2);

substrate/client/finality-grandpa/src/communication/mod.rs

@@ -105,6 +105,34 @@ mod benefit {
 	pub(super) const PER_EQUIVOCATION: i32 = 10;
 }
 
+/// A type that ties together our local authority id and a keystore where it is
+/// available for signing.
+pub struct LocalIdKeystore((AuthorityId, BareCryptoStorePtr));
+
+impl LocalIdKeystore {
+	/// Returns a reference to our local authority id.
+	fn local_id(&self) -> &AuthorityId {
+		&(self.0).0
+	}
+
+	/// Returns a reference to the keystore.
+	fn keystore(&self) -> &BareCryptoStorePtr {
+		&(self.0).1
+	}
+}
+
+impl AsRef<BareCryptoStorePtr> for LocalIdKeystore {
+	fn as_ref(&self) -> &BareCryptoStorePtr {
+		self.keystore()
+	}
+}
+
+impl From<(AuthorityId, BareCryptoStorePtr)> for LocalIdKeystore {
+	fn from(inner: (AuthorityId, BareCryptoStorePtr)) -> LocalIdKeystore {
+		LocalIdKeystore(inner)
+	}
+}
+
 /// If the voter set is larger than this value some telemetry events are not
 /// sent to avoid increasing usage resource on the node and flooding the
 /// telemetry server (e.g. received votes, received commits.)
@@ -272,11 +300,10 @@ impl<B: BlockT, N: Network<B>> NetworkBridge<B, N> {
 	/// network all within the current set.
 	pub(crate) fn round_communication(
 		&self,
-		keystore: Option<BareCryptoStorePtr>,
+		keystore: Option<LocalIdKeystore>,
 		round: Round,
 		set_id: SetId,
 		voters: Arc<VoterSet<AuthorityId>>,
-		local_key: Option<AuthorityId>,
 		has_voted: HasVoted<B>,
 	) -> (
 		impl Stream<Item = SignedMessage<B>> + Unpin,
@@ -288,9 +315,10 @@ impl<B: BlockT, N: Network<B>> NetworkBridge<B, N> {
 			&*voters,
 		);
 
-		let local_id = local_key.and_then(|id| {
-			if voters.contains(&id) {
-				Some(id)
+		let keystore = keystore.and_then(|ks| {
+			let id = ks.local_id();
+			if voters.contains(id) {
+				Some(ks)
 			} else {
 				None
 			}
@@ -350,11 +378,10 @@ impl<B: BlockT, N: Network<B>> NetworkBridge<B, N> {
 
 		let (tx, out_rx) = mpsc::channel(0);
 		let outgoing = OutgoingMessages::<B> {
-			keystore: keystore.clone(),
+			keystore,
 			round: round.0,
 			set_id: set_id.0,
 			network: self.gossip_engine.clone(),
-			local_id,
 			sender: tx,
 			has_voted,
 		};
@@ -629,11 +656,10 @@ pub struct SetId(pub SetIdNumber);
 pub(crate) struct OutgoingMessages<Block: BlockT> {
 	round: RoundNumber,
 	set_id: SetIdNumber,
-	local_id: Option<AuthorityId>,
+	keystore: Option<LocalIdKeystore>,
 	sender: mpsc::Sender<SignedMessage<Block>>,
 	network: Arc<Mutex<GossipEngine<Block>>>,
 	has_voted: HasVoted<Block>,
-	keystore: Option<BareCryptoStorePtr>,
 }
 
 impl<B: BlockT> Unpin for OutgoingMessages<B> {}
@@ -667,19 +693,12 @@ impl<Block: BlockT> Sink<Message<Block>> for OutgoingMessages<Block>
 		}
 
 		// when locals exist, sign messages on import
-		if let Some(ref public) = self.local_id {
-			let keystore = match &self.keystore {
-				Some(keystore) => keystore.clone(),
-				None => {
-					return Err(Error::Signing("Cannot sign without a keystore".to_string()))
-				}
-			};
-
+		if let Some(ref keystore) = self.keystore {
 			let target_hash = *(msg.target().0);
 			let signed = sp_finality_grandpa::sign_message(
-				keystore,
+				keystore.as_ref(),
 				msg,
-				public.clone(),
+				keystore.local_id().clone(),
 				self.round,
 				self.set_id,
 			).ok_or(
@@ -774,7 +793,7 @@ fn check_compact_commit<Block: BlockT>(
 		use crate::communication::gossip::Misbehavior;
 		use finality_grandpa::Message as GrandpaMessage;
 
-		if let Err(()) = sp_finality_grandpa::check_message_signature_with_buffer(
+		if !sp_finality_grandpa::check_message_signature_with_buffer(
 			&GrandpaMessage::Precommit(precommit.clone()),
 			id,
 			sig,
@@ -862,7 +881,7 @@ fn check_catch_up<Block: BlockT>(
 		for (msg, id, sig) in messages {
 			signatures_checked += 1;
 
-			if let Err(()) = sp_finality_grandpa::check_message_signature_with_buffer(
+			if !sp_finality_grandpa::check_message_signature_with_buffer(
 				&msg,
 				id,
 				sig,

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

@@ -716,12 +716,18 @@ where
 			HasVoted::No => HasVoted::No,
 		};
 
+		// we can only sign when we have a local key in the authority set
+		// and we have a reference to the keystore.
+		let keystore = match (local_key.as_ref(), self.config.keystore.as_ref()) {
+			(Some(id), Some(keystore)) => Some((id.clone(), keystore.clone()).into()),
+			_ => None,
+		};
+
 		let (incoming, outgoing) = self.network.round_communication(
-			self.config.keystore.clone(),
+			keystore,
 			crate::communication::Round(round),
 			crate::communication::SetId(self.set_id),
 			self.voters.clone(),
-			local_key.clone(),
 			has_voted,
 		);
 

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

@@ -133,14 +133,14 @@ impl<Block: BlockT> GrandpaJustification<Block> {
 		let mut buf = Vec::new();
 		let mut visited_hashes = HashSet::new();
 		for signed in self.commit.precommits.iter() {
-			if sp_finality_grandpa::check_message_signature_with_buffer(
+			if !sp_finality_grandpa::check_message_signature_with_buffer(
 				&finality_grandpa::Message::Precommit(signed.precommit.clone()),
 				&signed.id,
 				&signed.signature,
 				self.round,
 				set_id,
 				&mut buf,
-			).is_err() {
+			) {
 				return Err(ClientError::BadJustification(
 					"invalid signature for precommit in grandpa justification".to_string()));
 			}

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

@@ -593,7 +593,7 @@ fn global_communication<BE, Block: BlockT, C, N>(
 	voters: &Arc<VoterSet<AuthorityId>>,
 	client: Arc<C>,
 	network: &NetworkBridge<Block, N>,
-	keystore: &Option<BareCryptoStorePtr>,
+	keystore: Option<&BareCryptoStorePtr>,
 	metrics: Option<until_imported::Metrics>,
 ) -> (
 	impl Stream<
@@ -609,7 +609,7 @@ fn global_communication<BE, Block: BlockT, C, N>(
 	N: NetworkT<Block>,
 	NumberFor<Block>: BlockNumberOps,
 {
-	let is_voter = is_voter(voters, keystore.as_ref()).is_some();
+	let is_voter = is_voter(voters, keystore).is_some();
 
 	// verification stream
 	let (global_in, global_out) = network.global_communication(
@@ -907,7 +907,7 @@ where
 					&self.env.voters,
 					self.env.client.clone(),
 					&self.env.network,
-					&self.env.config.keystore,
+					self.env.config.keystore.as_ref(),
 					self.metrics.as_ref().map(|m| m.until_imported.clone()),
 				);
 

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

@@ -260,7 +260,7 @@ where
 			&voters,
 			self.client.clone(),
 			&self.network,
-			&self.keystore,
+			self.keystore.as_ref(),
 			None,
 		);
 

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

@@ -1160,11 +1160,10 @@ fn voter_persists_its_votes() {
 		);
 
 		let (round_rx, round_tx) = network.round_communication(
-			Some(keystore),
+			Some((peers[1].public().into(), keystore).into()),
 			communication::Round(1),
 			communication::SetId(0),
 			Arc::new(VoterSet::new(voters).unwrap()),
-			Some(peers[1].public().into()),
 			HasVoted::No,
 		);
 

substrate/frame/grandpa/src/equivocation.rs

@@ -145,7 +145,7 @@ where
 
 		// validate equivocation proof (check votes are different and
 		// signatures are valid).
-		if let Err(_) = sp_finality_grandpa::check_equivocation_proof(equivocation_proof.clone()) {
+		if !sp_finality_grandpa::check_equivocation_proof(equivocation_proof.clone()) {
 			return Err(ReportEquivocationValidityError::InvalidEquivocationProof.into());
 		}
 

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

@@ -257,7 +257,7 @@ impl<H, N> Equivocation<H, N> {
 
 /// Verifies the equivocation proof by making sure that both votes target
 /// different blocks and that its signatures are valid.
-pub fn check_equivocation_proof<H, N>(report: EquivocationProof<H, N>) -> Result<(), ()>
+pub fn check_equivocation_proof<H, N>(report: EquivocationProof<H, N>) -> bool
 where
 	H: Clone + Encode + PartialEq,
 	N: Clone + Encode + PartialEq,
@@ -270,27 +270,27 @@ where
 			if $equivocation.first.0.target_hash == $equivocation.second.0.target_hash &&
 				$equivocation.first.0.target_number == $equivocation.second.0.target_number
 			{
-				return Err(());
+				return false;
 			}
 
 			// check signatures on both votes are valid
-			check_message_signature(
+			let valid_first = check_message_signature(
 				&$message($equivocation.first.0),
 				&$equivocation.identity,
 				&$equivocation.first.1,
 				$equivocation.round_number,
 				report.set_id,
-			)?;
+			);
 
-			check_message_signature(
+			let valid_second = check_message_signature(
 				&$message($equivocation.second.0),
 				&$equivocation.identity,
 				&$equivocation.second.1,
 				$equivocation.round_number,
 				report.set_id,
-			)?;
+			);
 
-			return Ok(());
+			return valid_first && valid_second;
 		};
 	}
 
@@ -332,7 +332,7 @@ pub fn check_message_signature<H, N>(
 	signature: &AuthoritySignature,
 	round: RoundNumber,
 	set_id: SetId,
-) -> Result<(), ()>
+) -> bool
 where
 	H: Encode,
 	N: Encode,
@@ -351,7 +351,7 @@ pub fn check_message_signature_with_buffer<H, N>(
 	round: RoundNumber,
 	set_id: SetId,
 	buf: &mut Vec<u8>,
-) -> Result<(), ()>
+) -> bool
 where
 	H: Encode,
 	N: Encode,
@@ -360,20 +360,20 @@ where
 
 	localized_payload_with_buffer(round, set_id, message, buf);
 
-	if id.verify(&buf, signature) {
-		Ok(())
-	} else {
+	let valid = id.verify(&buf, signature);
+
+	if !valid {
 		#[cfg(feature = "std")]
 		debug!(target: "afg", "Bad signature on message from {:?}", id);
-
-		Err(())
 	}
+
+	valid
 }
 
 /// Localizes the message to the given set and round and signs the payload.
 #[cfg(feature = "std")]
 pub fn sign_message<H, N>(
-	keystore: BareCryptoStorePtr,
+	keystore: &BareCryptoStorePtr,
 	message: grandpa::Message<H, N>,
 	public: AuthorityId,
 	round: RoundNumber,