From 4cf1b4fa7b6d22ea317030836cdbc6848573e33b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= <123550+andresilva@users.noreply.github.com> Date: Tue, 30 Jun 2020 18:59:36 +0100 Subject: [PATCH] grandpa: minor cleanups in communication module (#6371) * grandpa: replace Result<(), ()> with Option<()> * grandpa: replace &Option 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 --- .../src/communication/gossip.rs | 12 ++-- .../finality-grandpa/src/communication/mod.rs | 61 ++++++++++++------- .../finality-grandpa/src/environment.rs | 10 ++- .../finality-grandpa/src/justification.rs | 4 +- substrate/client/finality-grandpa/src/lib.rs | 6 +- .../client/finality-grandpa/src/observer.rs | 2 +- .../client/finality-grandpa/src/tests.rs | 3 +- substrate/frame/grandpa/src/equivocation.rs | 2 +- .../primitives/finality-grandpa/src/lib.rs | 30 ++++----- 9 files changed, 79 insertions(+), 51 deletions(-) diff --git a/substrate/client/finality-grandpa/src/communication/gossip.rs b/substrate/client/finality-grandpa/src/communication/gossip.rs index c96301ede8..7d9fe4e7f2 100644 --- a/substrate/client/finality-grandpa/src/communication/gossip.rs +++ b/substrate/client/finality-grandpa/src/communication/gossip.rs @@ -750,7 +750,11 @@ impl Inner { Round(1), )), Some(ref mut v) => if v.set_id == set_id { - if self.authorities != authorities { + let diff_authorities = + self.authorities.iter().collect::>() != + 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 Inner { 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::::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); diff --git a/substrate/client/finality-grandpa/src/communication/mod.rs b/substrate/client/finality-grandpa/src/communication/mod.rs index e331d8b089..b7bbad9f8e 100644 --- a/substrate/client/finality-grandpa/src/communication/mod.rs +++ b/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 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> NetworkBridge { /// network all within the current set. pub(crate) fn round_communication( &self, - keystore: Option, + keystore: Option, round: Round, set_id: SetId, voters: Arc>, - local_key: Option, has_voted: HasVoted, ) -> ( impl Stream> + Unpin, @@ -288,9 +315,10 @@ impl> NetworkBridge { &*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> NetworkBridge { let (tx, out_rx) = mpsc::channel(0); let outgoing = OutgoingMessages:: { - 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 { round: RoundNumber, set_id: SetIdNumber, - local_id: Option, + keystore: Option, sender: mpsc::Sender>, network: Arc>>, has_voted: HasVoted, - keystore: Option, } impl Unpin for OutgoingMessages {} @@ -667,19 +693,12 @@ impl Sink> for OutgoingMessages } // 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( 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( 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, diff --git a/substrate/client/finality-grandpa/src/environment.rs b/substrate/client/finality-grandpa/src/environment.rs index 6db854bacc..cc6497fc72 100644 --- a/substrate/client/finality-grandpa/src/environment.rs +++ b/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, ); diff --git a/substrate/client/finality-grandpa/src/justification.rs b/substrate/client/finality-grandpa/src/justification.rs index b4db81f8a4..0e51a230c5 100644 --- a/substrate/client/finality-grandpa/src/justification.rs +++ b/substrate/client/finality-grandpa/src/justification.rs @@ -133,14 +133,14 @@ impl GrandpaJustification { 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())); } diff --git a/substrate/client/finality-grandpa/src/lib.rs b/substrate/client/finality-grandpa/src/lib.rs index 481544b5c6..fa2a6fedd8 100644 --- a/substrate/client/finality-grandpa/src/lib.rs +++ b/substrate/client/finality-grandpa/src/lib.rs @@ -593,7 +593,7 @@ fn global_communication( voters: &Arc>, client: Arc, network: &NetworkBridge, - keystore: &Option, + keystore: Option<&BareCryptoStorePtr>, metrics: Option, ) -> ( impl Stream< @@ -609,7 +609,7 @@ fn global_communication( N: NetworkT, NumberFor: 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()), ); diff --git a/substrate/client/finality-grandpa/src/observer.rs b/substrate/client/finality-grandpa/src/observer.rs index f7179d70e7..6a7a1f07b0 100644 --- a/substrate/client/finality-grandpa/src/observer.rs +++ b/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, ); diff --git a/substrate/client/finality-grandpa/src/tests.rs b/substrate/client/finality-grandpa/src/tests.rs index b94c37d07e..50f9e8eba2 100644 --- a/substrate/client/finality-grandpa/src/tests.rs +++ b/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, ); diff --git a/substrate/frame/grandpa/src/equivocation.rs b/substrate/frame/grandpa/src/equivocation.rs index 7c6e5c6d66..1cc1620125 100644 --- a/substrate/frame/grandpa/src/equivocation.rs +++ b/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()); } diff --git a/substrate/primitives/finality-grandpa/src/lib.rs b/substrate/primitives/finality-grandpa/src/lib.rs index 889468a352..f99880041c 100644 --- a/substrate/primitives/finality-grandpa/src/lib.rs +++ b/substrate/primitives/finality-grandpa/src/lib.rs @@ -257,7 +257,7 @@ impl Equivocation { /// Verifies the equivocation proof by making sure that both votes target /// different blocks and that its signatures are valid. -pub fn check_equivocation_proof(report: EquivocationProof) -> Result<(), ()> +pub fn check_equivocation_proof(report: EquivocationProof) -> 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( 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( round: RoundNumber, set_id: SetId, buf: &mut Vec, -) -> 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( - keystore: BareCryptoStorePtr, + keystore: &BareCryptoStorePtr, message: grandpa::Message, public: AuthorityId, round: RoundNumber,