From 4725c6edf23fb333bd0f7e8f724277741c9fd06d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 21 Oct 2020 23:11:30 +0100 Subject: [PATCH 1/3] grandpa: don't send equivocation reports for local identities --- client/finality-grandpa/src/environment.rs | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 95d7adb9578c5..7f9e966c9acc2 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -42,8 +42,8 @@ use sp_runtime::traits::{ use sc_telemetry::{telemetry, CONSENSUS_DEBUG, CONSENSUS_INFO}; use crate::{ - CommandOrError, Commit, Config, Error, Precommit, Prevote, - PrimaryPropose, SignedMessage, NewAuthoritySet, VoterCommand, + local_authority_id, CommandOrError, Commit, Config, Error, NewAuthoritySet, Precommit, Prevote, + PrimaryPropose, SignedMessage, VoterCommand, }; use sp_consensus::SelectChain; @@ -467,10 +467,18 @@ where /// extrinsic to report the equivocation. In particular, the session membership /// proof must be generated at the block at which the given set was active which /// isn't necessarily the best block if there are pending authority set changes. - fn report_equivocation( + pub(crate) fn report_equivocation( &self, equivocation: Equivocation>, ) -> Result<(), Error> { + if let Some(local_id) = local_authority_id(&self.voters, self.config.keystore.as_ref()) { + if *equivocation.offender() == local_id { + return Err(Error::Safety( + "Refraining from sending equivocation report for our own equivocation.".into(), + )); + } + } + let is_descendent_of = is_descendent_of(&*self.client, None); let best_header = self.select_chain @@ -724,7 +732,7 @@ where let prevote_timer = Delay::new(self.config.gossip_duration * 2); let precommit_timer = Delay::new(self.config.gossip_duration * 4); - let local_id = crate::local_authority_id(&self.voters, self.config.keystore.as_ref()); + let local_id = local_authority_id(&self.voters, self.config.keystore.as_ref()); let has_voted = match self.voter_set_state.has_voted(round) { HasVoted::Yes(id, vote) => { @@ -776,7 +784,7 @@ where } fn proposed(&self, round: RoundNumber, propose: PrimaryPropose) -> Result<(), Self::Error> { - let local_id = crate::local_authority_id(&self.voters, self.config.keystore.as_ref()); + let local_id = local_authority_id(&self.voters, self.config.keystore.as_ref()); let local_id = match local_id { Some(id) => id, @@ -815,7 +823,7 @@ where } fn prevoted(&self, round: RoundNumber, prevote: Prevote) -> Result<(), Self::Error> { - let local_id = crate::local_authority_id(&self.voters, self.config.keystore.as_ref()); + let local_id = local_authority_id(&self.voters, self.config.keystore.as_ref()); let local_id = match local_id { Some(id) => id, @@ -876,7 +884,7 @@ where round: RoundNumber, precommit: Precommit, ) -> Result<(), Self::Error> { - let local_id = crate::local_authority_id(&self.voters, self.config.keystore.as_ref()); + let local_id = local_authority_id(&self.voters, self.config.keystore.as_ref()); let local_id = match local_id { Some(id) => id, From 3c923efd8befca41c484a4bc9405326045454736 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 21 Oct 2020 23:12:25 +0100 Subject: [PATCH 2/3] grandpa: add test for self-report --- client/finality-grandpa/src/tests.rs | 46 ++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index c9d9f717cdcec..8a1c3878aed4c 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -1813,3 +1813,49 @@ fn imports_justification_for_regular_blocks_on_import() { client.justification(&BlockId::Hash(block_hash)).unwrap().is_some(), ); } + +#[test] +fn grandpa_environment_doesnt_send_equivocation_reports_for_itself() { + let alice = Ed25519Keyring::Alice; + let voters = make_ids(&[alice]); + + let environment = { + let mut net = GrandpaTestNet::new(TestApi::new(voters), 1); + let peer = net.peer(0); + let network_service = peer.network_service().clone(); + let link = peer.data.lock().take().unwrap(); + let (keystore, _keystore_path) = create_keystore(alice); + test_environment(&link, Some(keystore), network_service.clone(), ()) + }; + + let signed_prevote = { + let prevote = finality_grandpa::Prevote { + target_hash: H256::random(), + target_number: 1, + }; + + let signed = alice.sign(&[]).into(); + (prevote, signed) + }; + + let mut equivocation = finality_grandpa::Equivocation { + round_number: 1, + identity: alice.public().into(), + first: signed_prevote.clone(), + second: signed_prevote.clone(), + }; + + // reporting the equivocation should fail since the offender is a local + // authority (i.e. we have keys in our keystore for the given id) + let equivocation_proof = sp_finality_grandpa::Equivocation::Prevote(equivocation.clone()); + assert!(matches!( + environment.report_equivocation(equivocation_proof), + Err(Error::Safety(_)), + )); + + // if we set the equivocation offender to another id for which we don't have + // keys it should work + equivocation.identity = Default::default(); + let equivocation_proof = sp_finality_grandpa::Equivocation::Prevote(equivocation); + assert!(environment.report_equivocation(equivocation_proof).is_ok()); +} From 1a5c4c85fbbe973ac36da42f75df03e5170a9fc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Thu, 22 Oct 2020 01:01:46 +0100 Subject: [PATCH 3/3] grandpa: fix test compilation this works on rust nightly but breaks on ci which is using rust stable --- client/finality-grandpa/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 8a1c3878aed4c..cf1b2ef986277 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -1850,7 +1850,7 @@ fn grandpa_environment_doesnt_send_equivocation_reports_for_itself() { let equivocation_proof = sp_finality_grandpa::Equivocation::Prevote(equivocation.clone()); assert!(matches!( environment.report_equivocation(equivocation_proof), - Err(Error::Safety(_)), + Err(Error::Safety(_)) )); // if we set the equivocation offender to another id for which we don't have