grandpa: store the authority id that is used for voting per round (#7454)

* grandpa: store the authority id that is used for voting per round

* grandpa: fix tests

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 <https://www.gnu.org/licenses/>.
 
-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<Block: BlockT> HasVoted<Block> {
 /// A voter set state meant to be shared safely across multiple owners.
 #[derive(Clone)]
 pub struct SharedVoterSetState<Block: BlockT> {
+	/// The inner shared `VoterSetState`.
 	inner: Arc<RwLock<VoterSetState<Block>>>,
+	/// 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<RwLock<HashMap<RoundNumber, AuthorityId>>>,
 }
 
 impl<Block: BlockT> From<VoterSetState<Block>> for SharedVoterSetState<Block> {
@@ -343,7 +347,10 @@ impl<Block: BlockT> From<VoterSetState<Block>> for SharedVoterSetState<Block> {
 impl<Block: BlockT> SharedVoterSetState<Block> {
 	/// Create a new shared voter set tracker with the given state.
 	pub(crate) fn new(state: VoterSetState<Block>) -> 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<Block: BlockT> SharedVoterSetState<Block> {
 		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<AuthorityId> {
+		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<Block> {
 		match &*self.inner.read() {
@@ -471,7 +495,7 @@ where
 		&self,
 		equivocation: Equivocation<Block::Hash, NumberFor<Block>>,
 	) -> 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<Block>) -> 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<Block>,
+	) -> 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<Block>) -> 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<Block>,
 	) -> 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(())
 	}
 

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());

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

@@ -252,6 +252,14 @@ impl<H, N> Equivocation<H, N> {
 			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