diff --git a/substrate/client/finality-grandpa/src/environment.rs b/substrate/client/finality-grandpa/src/environment.rs index 7f9e966c9a..9b3a656d0c 100644 --- a/substrate/client/finality-grandpa/src/environment.rs +++ b/substrate/client/finality-grandpa/src/environment.rs @@ -16,18 +16,18 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashMap}; use std::iter::FromIterator; +use std::marker::PhantomData; use std::pin::Pin; use std::sync::Arc; use std::time::Duration; -use log::{debug, warn}; -use parity_scale_codec::{Decode, Encode}; use futures::prelude::*; use futures_timer::Delay; +use log::{debug, warn}; +use parity_scale_codec::{Decode, Encode}; use parking_lot::RwLock; -use std::marker::PhantomData; use sc_client_api::{backend::{Backend, apply_aux}, utils::is_descendent_of}; use finality_grandpa::{ @@ -331,7 +331,11 @@ impl HasVoted { /// A voter set state meant to be shared safely across multiple owners. #[derive(Clone)] pub struct SharedVoterSetState { + /// The inner shared `VoterSetState`. inner: Arc>>, + /// A tracker for the rounds that we are actively participating on (i.e. voting) + /// and the authority id under which we are doing it. + voting: Arc>>, } impl From> for SharedVoterSetState { @@ -343,7 +347,10 @@ impl From> for SharedVoterSetState { impl SharedVoterSetState { /// Create a new shared voter set tracker with the given state. pub(crate) fn new(state: VoterSetState) -> Self { - SharedVoterSetState { inner: Arc::new(RwLock::new(state)) } + SharedVoterSetState { + inner: Arc::new(RwLock::new(state)), + voting: Arc::new(RwLock::new(HashMap::new())), + } } /// Read the inner voter set state. @@ -351,6 +358,23 @@ impl SharedVoterSetState { self.inner.read() } + /// Get the authority id that we are using to vote on the given round, if any. + pub(crate) fn voting_on(&self, round: RoundNumber) -> Option { + self.voting.read().get(&round).cloned() + } + + /// Note that we started voting on the give round with the given authority id. + pub(crate) fn started_voting_on(&self, round: RoundNumber, local_id: AuthorityId) { + self.voting.write().insert(round, local_id); + } + + /// Note that we have finished voting on the given round. If we were voting on + /// the given round, the authority id that we were using to do it will be + /// cleared. + pub(crate) fn finished_voting_on(&self, round: RoundNumber) { + self.voting.write().remove(&round); + } + /// Return vote status information for the current round. pub(crate) fn has_voted(&self, round: RoundNumber) -> HasVoted { match &*self.inner.read() { @@ -471,7 +495,7 @@ where &self, equivocation: Equivocation>, ) -> Result<(), Error> { - if let Some(local_id) = local_authority_id(&self.voters, self.config.keystore.as_ref()) { + if let Some(local_id) = self.voter_set_state.voting_on(equivocation.round_number()) { if *equivocation.offender() == local_id { return Err(Error::Safety( "Refraining from sending equivocation report for our own equivocation.".into(), @@ -745,6 +769,17 @@ where HasVoted::No => HasVoted::No, }; + // NOTE: we cache the local authority id that we'll be using to vote on the + // given round. this is done to make sure we only check for available keys + // from the keystore in this method when beginning the round, otherwise if + // the keystore state changed during the round (e.g. a key was removed) it + // could lead to internal state inconsistencies in the voter environment + // (e.g. we wouldn't update the voter set state after prevoting since there's + // no local authority id). + if let Some(id) = local_id.as_ref() { + self.voter_set_state.started_voting_on(round, id.clone()); + } + // 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_id.as_ref(), self.config.keystore.as_ref()) { @@ -783,10 +818,12 @@ where } } - fn proposed(&self, round: RoundNumber, propose: PrimaryPropose) -> Result<(), Self::Error> { - let local_id = local_authority_id(&self.voters, self.config.keystore.as_ref()); - - let local_id = match local_id { + fn proposed( + &self, + round: RoundNumber, + propose: PrimaryPropose, + ) -> Result<(), Self::Error> { + let local_id = match self.voter_set_state.voting_on(round) { Some(id) => id, None => return Ok(()), }; @@ -823,9 +860,7 @@ where } fn prevoted(&self, round: RoundNumber, prevote: Prevote) -> Result<(), Self::Error> { - let local_id = local_authority_id(&self.voters, self.config.keystore.as_ref()); - - let local_id = match local_id { + let local_id = match self.voter_set_state.voting_on(round) { Some(id) => id, None => return Ok(()), }; @@ -884,9 +919,7 @@ where round: RoundNumber, precommit: Precommit, ) -> Result<(), Self::Error> { - let local_id = local_authority_id(&self.voters, self.config.keystore.as_ref()); - - let local_id = match local_id { + let local_id = match self.voter_set_state.voting_on(round) { Some(id) => id, None => return Ok(()), }; @@ -1010,6 +1043,9 @@ where Ok(Some(set_state)) })?; + // clear any cached local authority id associated with this round + self.voter_set_state.finished_voting_on(round); + Ok(()) } diff --git a/substrate/client/finality-grandpa/src/tests.rs b/substrate/client/finality-grandpa/src/tests.rs index cf1b2ef986..175c5360b2 100644 --- a/substrate/client/finality-grandpa/src/tests.rs +++ b/substrate/client/finality-grandpa/src/tests.rs @@ -1713,6 +1713,10 @@ fn grandpa_environment_never_overwrites_round_voter_state() { assert_eq!(get_current_round(2).unwrap(), HasVoted::No); + // we need to call `round_data` for the next round to pick up + // from the keystore which authority id we'll be using to vote + environment.round_data(2); + let info = peer.client().info(); let prevote = finality_grandpa::Prevote { @@ -1816,6 +1820,8 @@ fn imports_justification_for_regular_blocks_on_import() { #[test] fn grandpa_environment_doesnt_send_equivocation_reports_for_itself() { + use finality_grandpa::voter::Environment; + let alice = Ed25519Keyring::Alice; let voters = make_ids(&[alice]); @@ -1845,6 +1851,10 @@ fn grandpa_environment_doesnt_send_equivocation_reports_for_itself() { second: signed_prevote.clone(), }; + // we need to call `round_data` to pick up from the keystore which + // authority id we'll be using to vote + environment.round_data(1); + // 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()); diff --git a/substrate/primitives/finality-grandpa/src/lib.rs b/substrate/primitives/finality-grandpa/src/lib.rs index 2c569fafda..0426dad946 100644 --- a/substrate/primitives/finality-grandpa/src/lib.rs +++ b/substrate/primitives/finality-grandpa/src/lib.rs @@ -252,6 +252,14 @@ impl Equivocation { Equivocation::Precommit(ref equivocation) => &equivocation.identity, } } + + /// Returns the round number when the equivocation happened. + pub fn round_number(&self) -> RoundNumber { + match self { + Equivocation::Prevote(ref equivocation) => equivocation.round_number, + Equivocation::Precommit(ref equivocation) => equivocation.round_number, + } + } } /// Verifies the equivocation proof by making sure that both votes target