BEEFY: client support for detecting equivocated votes (#13285)

* client/beefy: detect equivocated votes

* client/beefy: make sure to persist state after voting

* client/beefy: drop never-used aux-schema v2 migration

* impl review suggestion

---------

Signed-off-by: Adrian Catangiu <adrian@parity.io>

substrate/client/beefy/Cargo.toml

@@ -39,7 +39,6 @@ sp-runtime = { version = "7.0.0", path = "../../primitives/runtime" }
 
 [dev-dependencies]
 serde = "1.0.136"
-strum = { version = "0.24.1", features = ["derive"] }
 tempfile = "3.1.0"
 tokio = "1.22.0"
 sc-block-builder = { version = "0.10.0-dev", path = "../block-builder" }

substrate/client/beefy/src/aux_schema.rs

@@ -26,25 +26,25 @@ use sp_blockchain::{Error as ClientError, Result as ClientResult};
 use sp_runtime::traits::Block as BlockT;
 
 const VERSION_KEY: &[u8] = b"beefy_auxschema_version";
-const WORKER_STATE: &[u8] = b"beefy_voter_state";
+const WORKER_STATE_KEY: &[u8] = b"beefy_voter_state";
 
 const CURRENT_VERSION: u32 = 2;
 
-pub(crate) fn write_current_version<B: AuxStore>(backend: &B) -> ClientResult<()> {
+pub(crate) fn write_current_version<BE: AuxStore>(backend: &BE) -> ClientResult<()> {
 	info!(target: LOG_TARGET, "🥩 write aux schema version {:?}", CURRENT_VERSION);
 	AuxStore::insert_aux(backend, &[(VERSION_KEY, CURRENT_VERSION.encode().as_slice())], &[])
 }
 
 /// Write voter state.
-pub(crate) fn write_voter_state<Block: BlockT, B: AuxStore>(
-	backend: &B,
-	state: &PersistedState<Block>,
+pub(crate) fn write_voter_state<B: BlockT, BE: AuxStore>(
+	backend: &BE,
+	state: &PersistedState<B>,
 ) -> ClientResult<()> {
 	trace!(target: LOG_TARGET, "🥩 persisting {:?}", state);
-	backend.insert_aux(&[(WORKER_STATE, state.encode().as_slice())], &[])
+	AuxStore::insert_aux(backend, &[(WORKER_STATE_KEY, state.encode().as_slice())], &[])
 }
 
-fn load_decode<B: AuxStore, T: Decode>(backend: &B, key: &[u8]) -> ClientResult<Option<T>> {
+fn load_decode<BE: AuxStore, T: Decode>(backend: &BE, key: &[u8]) -> ClientResult<Option<T>> {
 	match backend.get_aux(key)? {
 		None => Ok(None),
 		Some(t) => T::decode(&mut &t[..])
@@ -64,7 +64,7 @@ where
 	match version {
 		None => (),
 		Some(1) => (), // version 1 is totally obsolete and should be simply ignored
-		Some(2) => return load_decode::<_, PersistedState<B>>(backend, WORKER_STATE),
+		Some(2) => return load_decode::<_, PersistedState<B>>(backend, WORKER_STATE_KEY),
 		other =>
 			return Err(ClientError::Backend(format!("Unsupported BEEFY DB version: {:?}", other))),
 	}

substrate/client/beefy/src/communication/gossip.rs

@@ -120,7 +120,7 @@ where
 
 	/// Note a voting round.
 	///
-	/// Noting round will start a live `round`.
+	/// Noting round will track gossiped votes for `round`.
 	pub(crate) fn note_round(&self, round: NumberFor<B>) {
 		debug!(target: LOG_TARGET, "🥩 About to note gossip round #{}", round);
 		self.known_votes.write().insert(round);
@@ -242,9 +242,10 @@ mod tests {
 	use sc_network_test::Block;
 	use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr};
 
-	use crate::keystore::{tests::Keyring, BeefyKeystore};
+	use crate::keystore::BeefyKeystore;
 	use beefy_primitives::{
-		crypto::Signature, known_payloads, Commitment, MmrRootHash, Payload, VoteMessage, KEY_TYPE,
+		crypto::Signature, keyring::Keyring, known_payloads, Commitment, MmrRootHash, Payload,
+		VoteMessage, KEY_TYPE,
 	};
 
 	use super::*;

substrate/client/beefy/src/justification.rs

@@ -81,12 +81,13 @@ fn verify_with_validator_set<Block: BlockT>(
 #[cfg(test)]
 pub(crate) mod tests {
 	use beefy_primitives::{
-		known_payloads, Commitment, Payload, SignedCommitment, VersionedFinalityProof,
+		keyring::Keyring, known_payloads, Commitment, Payload, SignedCommitment,
+		VersionedFinalityProof,
 	};
 	use substrate_test_runtime_client::runtime::Block;
 
 	use super::*;
-	use crate::{keystore::tests::Keyring, tests::make_beefy_ids};
+	use crate::tests::make_beefy_ids;
 
 	pub(crate) fn new_finality_proof(
 		block_num: NumberFor<Block>,

substrate/client/beefy/src/keystore.rs

@@ -19,7 +19,6 @@
 use sp_application_crypto::RuntimeAppPublic;
 use sp_core::keccak_256;
 use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr};
-use sp_runtime::traits::Keccak256;
 
 use log::warn;
 
@@ -30,6 +29,9 @@ use beefy_primitives::{
 
 use crate::{error, LOG_TARGET};
 
+/// Hasher used for BEEFY signatures.
+pub(crate) type BeefySignatureHasher = sp_runtime::traits::Keccak256;
+
 /// A BEEFY specific keystore implemented as a `Newtype`. This is basically a
 /// wrapper around [`sp_keystore::SyncCryptoStore`] and allows to customize
 /// common cryptographic functionality.
@@ -104,7 +106,7 @@ impl BeefyKeystore {
 	///
 	/// Return `true` if the signature is authentic, `false` otherwise.
 	pub fn verify(public: &Public, sig: &Signature, message: &[u8]) -> bool {
-		BeefyAuthorityId::<Keccak256>::verify(public, sig, message)
+		BeefyAuthorityId::<BeefySignatureHasher>::verify(public, sig, message)
 	}
 }
 
@@ -119,63 +121,13 @@ pub mod tests {
 	use std::sync::Arc;
 
 	use sc_keystore::LocalKeystore;
-	use sp_core::{ecdsa, keccak_256, Pair};
-	use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr};
+	use sp_core::{ecdsa, Pair};
 
-	use beefy_primitives::{crypto, KEY_TYPE};
+	use beefy_primitives::{crypto, keyring::Keyring};
 
-	use super::BeefyKeystore;
+	use super::*;
 	use crate::error::Error;
 
-	/// Set of test accounts using [`beefy_primitives::crypto`] types.
-	#[allow(missing_docs)]
-	#[derive(Debug, Clone, Copy, PartialEq, Eq, strum::Display, strum::EnumIter)]
-	pub(crate) enum Keyring {
-		Alice,
-		Bob,
-		Charlie,
-		Dave,
-		Eve,
-		Ferdie,
-		One,
-		Two,
-	}
-
-	impl Keyring {
-		/// Sign `msg`.
-		pub fn sign(self, msg: &[u8]) -> crypto::Signature {
-			let msg = keccak_256(msg);
-			ecdsa::Pair::from(self).sign_prehashed(&msg).into()
-		}
-
-		/// Return key pair.
-		pub fn pair(self) -> crypto::Pair {
-			ecdsa::Pair::from_string(self.to_seed().as_str(), None).unwrap().into()
-		}
-
-		/// Return public key.
-		pub fn public(self) -> crypto::Public {
-			self.pair().public()
-		}
-
-		/// Return seed string.
-		pub fn to_seed(self) -> String {
-			format!("//{}", self)
-		}
-	}
-
-	impl From<Keyring> for crypto::Pair {
-		fn from(k: Keyring) -> Self {
-			k.pair()
-		}
-	}
-
-	impl From<Keyring> for ecdsa::Pair {
-		fn from(k: Keyring) -> Self {
-			k.pair().into()
-		}
-	}
-
 	fn keystore() -> SyncCryptoStorePtr {
 		Arc::new(LocalKeystore::in_memory())
 	}

substrate/client/beefy/src/metrics.rs

@@ -30,6 +30,8 @@ pub(crate) struct Metrics {
 	pub beefy_round_concluded: Gauge<U64>,
 	/// Best block finalized by BEEFY
 	pub beefy_best_block: Gauge<U64>,
+	/// Best block BEEFY voted on
+	pub beefy_best_voted: Gauge<U64>,
 	/// Next block BEEFY should vote on
 	pub beefy_should_vote_on: Gauge<U64>,
 	/// Number of sessions with lagging signed commitment on mandatory block
@@ -61,6 +63,10 @@ impl Metrics {
 				Gauge::new("substrate_beefy_best_block", "Best block finalized by BEEFY")?,
 				registry,
 			)?,
+			beefy_best_voted: register(
+				Gauge::new("substrate_beefy_best_voted", "Best block voted on by BEEFY")?,
+				registry,
+			)?,
 			beefy_should_vote_on: register(
 				Gauge::new("substrate_beefy_should_vote_on", "Next block, BEEFY should vote on")?,
 				registry,

substrate/client/beefy/src/round.rs

@@ -19,39 +19,33 @@
 use crate::LOG_TARGET;
 
 use beefy_primitives::{
-	crypto::{Public, Signature},
-	ValidatorSet, ValidatorSetId,
+	crypto::{AuthorityId, Public, Signature},
+	Commitment, SignedCommitment, ValidatorSet, ValidatorSetId, VoteMessage,
 };
 use codec::{Decode, Encode};
-use log::{debug, trace};
+use log::debug;
 use sp_runtime::traits::{Block, NumberFor};
-use std::{collections::BTreeMap, hash::Hash};
+use std::collections::BTreeMap;
 
 /// Tracks for each round which validators have voted/signed and
 /// whether the local `self` validator has voted/signed.
 ///
 /// Does not do any validation on votes or signatures, layers above need to handle that (gossip).
 #[derive(Debug, Decode, Default, Encode, PartialEq)]
-struct RoundTracker {
-	self_vote: bool,
+pub(crate) struct RoundTracker {
 	votes: BTreeMap<Public, Signature>,
 }
 
 impl RoundTracker {
-	fn add_vote(&mut self, vote: (Public, Signature), self_vote: bool) -> bool {
+	fn add_vote(&mut self, vote: (Public, Signature)) -> bool {
 		if self.votes.contains_key(&vote.0) {
 			return false
 		}
 
-		self.self_vote = self.self_vote || self_vote;
 		self.votes.insert(vote.0, vote.1);
 		true
 	}
 
-	fn has_self_vote(&self) -> bool {
-		self.self_vote
-	}
-
 	fn is_done(&self, threshold: usize) -> bool {
 		self.votes.len() >= threshold
 	}
@@ -63,27 +57,37 @@ pub fn threshold(authorities: usize) -> usize {
 	authorities - faulty
 }
 
+#[derive(Debug, PartialEq)]
+pub enum VoteImportResult<B: Block> {
+	Ok,
+	RoundConcluded(SignedCommitment<NumberFor<B>, Signature>),
+	Equivocation, /* TODO: (EquivocationProof<NumberFor<B>, Public, Signature>) */
+	Invalid,
+	Stale,
+}
+
 /// Keeps track of all voting rounds (block numbers) within a session.
 /// Only round numbers > `best_done` are of interest, all others are considered stale.
 ///
 /// Does not do any validation on votes or signatures, layers above need to handle that (gossip).
 #[derive(Debug, Decode, Encode, PartialEq)]
-pub(crate) struct Rounds<Payload, B: Block> {
-	rounds: BTreeMap<(Payload, NumberFor<B>), RoundTracker>,
+pub(crate) struct Rounds<B: Block> {
+	rounds: BTreeMap<Commitment<NumberFor<B>>, RoundTracker>,
+	previous_votes: BTreeMap<(Public, NumberFor<B>), VoteMessage<NumberFor<B>, Public, Signature>>,
 	session_start: NumberFor<B>,
 	validator_set: ValidatorSet<Public>,
 	mandatory_done: bool,
 	best_done: Option<NumberFor<B>>,
 }
 
-impl<P, B> Rounds<P, B>
+impl<B> Rounds<B>
 where
-	P: Ord + Hash + Clone,
 	B: Block,
 {
 	pub(crate) fn new(session_start: NumberFor<B>, validator_set: ValidatorSet<Public>) -> Self {
 		Rounds {
 			rounds: BTreeMap::new(),
+			previous_votes: BTreeMap::new(),
 			session_start,
 			validator_set,
 			mandatory_done: false,
@@ -111,60 +115,77 @@ where
 		self.mandatory_done
 	}
 
-	pub(crate) fn should_self_vote(&self, round: &(P, NumberFor<B>)) -> bool {
-		Some(round.1) > self.best_done &&
-			self.rounds.get(round).map(|tracker| !tracker.has_self_vote()).unwrap_or(true)
-	}
-
 	pub(crate) fn add_vote(
 		&mut self,
-		round: &(P, NumberFor<B>),
-		vote: (Public, Signature),
-		self_vote: bool,
-	) -> bool {
-		let num = round.1;
+		vote: VoteMessage<NumberFor<B>, AuthorityId, Signature>,
+	) -> VoteImportResult<B> {
+		let num = vote.commitment.block_number;
+		let vote_key = (vote.id.clone(), num);
+
 		if num < self.session_start || Some(num) <= self.best_done {
 			debug!(target: LOG_TARGET, "🥩 received vote for old stale round {:?}, ignoring", num);
-			false
-		} else if !self.validators().iter().any(|id| vote.0 == *id) {
+			return VoteImportResult::Stale
+		} else if vote.commitment.validator_set_id != self.validator_set_id() {
+			debug!(
+				target: LOG_TARGET,
+				"🥩 expected set_id {:?}, ignoring vote {:?}.",
+				self.validator_set_id(),
+				vote,
+			);
+			return VoteImportResult::Invalid
+		} else if !self.validators().iter().any(|id| &vote.id == id) {
 			debug!(
 				target: LOG_TARGET,
 				"🥩 received vote {:?} from validator that is not in the validator set, ignoring",
 				vote
 			);
-			false
-		} else {
-			self.rounds.entry(round.clone()).or_default().add_vote(vote, self_vote)
+			return VoteImportResult::Invalid
 		}
+
+		if let Some(previous_vote) = self.previous_votes.get(&vote_key) {
+			// is the same public key voting for a different payload?
+			if previous_vote.commitment.payload != vote.commitment.payload {
+				debug!(
+					target: LOG_TARGET,
+					"🥩 detected equivocated vote: 1st: {:?}, 2nd: {:?}", previous_vote, vote
+				);
+				// TODO: build `EquivocationProof` and return it here.
+				return VoteImportResult::Equivocation
+			}
+		} else {
+			// this is the first vote sent by `id` for `num`, all good
+			self.previous_votes.insert(vote_key, vote.clone());
+		}
+
+		// add valid vote
+		let round = self.rounds.entry(vote.commitment.clone()).or_default();
+		if round.add_vote((vote.id, vote.signature)) &&
+			round.is_done(threshold(self.validator_set.len()))
+		{
+			if let Some(round) = self.rounds.remove_entry(&vote.commitment) {
+				return VoteImportResult::RoundConcluded(self.signed_commitment(round))
+			}
+		}
+		VoteImportResult::Ok
 	}
 
-	pub(crate) fn should_conclude(
+	fn signed_commitment(
 		&mut self,
-		round: &(P, NumberFor<B>),
-	) -> Option<Vec<Option<Signature>>> {
-		let done = self
-			.rounds
-			.get(round)
-			.map(|tracker| tracker.is_done(threshold(self.validator_set.len())))
-			.unwrap_or(false);
-		trace!(target: LOG_TARGET, "🥩 Round #{} done: {}", round.1, done);
-
-		if done {
-			let signatures = self.rounds.remove(round)?.votes;
-			Some(
-				self.validators()
-					.iter()
-					.map(|authority_id| signatures.get(authority_id).cloned())
-					.collect(),
-			)
-		} else {
-			None
-		}
+		round: (Commitment<NumberFor<B>>, RoundTracker),
+	) -> SignedCommitment<NumberFor<B>, Signature> {
+		let votes = round.1.votes;
+		let signatures = self
+			.validators()
+			.iter()
+			.map(|authority_id| votes.get(authority_id).cloned())
+			.collect();
+		SignedCommitment { commitment: round.0, signatures }
 	}
 
 	pub(crate) fn conclude(&mut self, round_num: NumberFor<B>) {
 		// Remove this and older (now stale) rounds.
-		self.rounds.retain(|&(_, number), _| number > round_num);
+		self.rounds.retain(|commitment, _| commitment.block_number > round_num);
+		self.previous_votes.retain(|&(_, number), _| number > round_num);
 		self.mandatory_done = self.mandatory_done || round_num == self.session_start;
 		self.best_done = self.best_done.max(Some(round_num));
 		debug!(target: LOG_TARGET, "🥩 Concluded round #{}", round_num);
@@ -174,16 +195,17 @@ where
 #[cfg(test)]
 mod tests {
 	use sc_network_test::Block;
-	use sp_core::H256;
 
-	use beefy_primitives::{crypto::Public, ValidatorSet};
+	use beefy_primitives::{
+		crypto::Public, keyring::Keyring, known_payloads::MMR_ROOT_ID, Commitment, Payload,
+		SignedCommitment, ValidatorSet, VoteMessage,
+	};
 
-	use super::{threshold, Block as BlockT, Hash, RoundTracker, Rounds};
-	use crate::keystore::tests::Keyring;
+	use super::{threshold, Block as BlockT, RoundTracker, Rounds};
+	use crate::round::VoteImportResult;
 
-	impl<P, B> Rounds<P, B>
+	impl<B> Rounds<B>
 	where
-		P: Ord + Hash + Clone,
 		B: BlockT,
 	{
 		pub(crate) fn test_set_mandatory_done(&mut self, done: bool) {
@@ -197,26 +219,18 @@ mod tests {
 		let bob_vote = (Keyring::Bob.public(), Keyring::Bob.sign(b"I am committed"));
 		let threshold = 2;
 
-		// self vote not added yet
-		assert!(!rt.has_self_vote());
-
 		// adding new vote allowed
-		assert!(rt.add_vote(bob_vote.clone(), false));
+		assert!(rt.add_vote(bob_vote.clone()));
 		// adding existing vote not allowed
-		assert!(!rt.add_vote(bob_vote, false));
-
-		// self vote still not added yet
-		assert!(!rt.has_self_vote());
+		assert!(!rt.add_vote(bob_vote));
 
 		// vote is not done
 		assert!(!rt.is_done(threshold));
 
 		let alice_vote = (Keyring::Alice.public(), Keyring::Alice.sign(b"I am committed"));
 		// adding new vote (self vote this time) allowed
-		assert!(rt.add_vote(alice_vote, true));
+		assert!(rt.add_vote(alice_vote));
 
-		// self vote registered
-		assert!(rt.has_self_vote());
 		// vote is now done
 		assert!(rt.is_done(threshold));
 	}
@@ -242,7 +256,7 @@ mod tests {
 		.unwrap();
 
 		let session_start = 1u64.into();
-		let rounds = Rounds::<H256, Block>::new(session_start, validators);
+		let rounds = Rounds::<Block>::new(session_start, validators);
 
 		assert_eq!(42, rounds.validator_set_id());
 		assert_eq!(1, rounds.session_start());
@@ -266,65 +280,56 @@ mod tests {
 			Default::default(),
 		)
 		.unwrap();
-		let round = (H256::from_low_u64_le(1), 1);
+		let validator_set_id = validators.id();
 
 		let session_start = 1u64.into();
-		let mut rounds = Rounds::<H256, Block>::new(session_start, validators);
-
-		// no self vote yet, should self vote
-		assert!(rounds.should_self_vote(&round));
+		let mut rounds = Rounds::<Block>::new(session_start, validators);
 
+		let payload = Payload::from_single_entry(MMR_ROOT_ID, vec![]);
+		let block_number = 1;
+		let commitment = Commitment { block_number, payload, validator_set_id };
+		let mut vote = VoteMessage {
+			id: Keyring::Alice.public(),
+			commitment: commitment.clone(),
+			signature: Keyring::Alice.sign(b"I am committed"),
+		};
 		// add 1st good vote
-		assert!(rounds.add_vote(
-			&round,
-			(Keyring::Alice.public(), Keyring::Alice.sign(b"I am committed")),
-			true
-		));
-		// round not concluded
-		assert!(rounds.should_conclude(&round).is_none());
-		// self vote already present, should not self vote
-		assert!(!rounds.should_self_vote(&round));
+		assert_eq!(rounds.add_vote(vote.clone()), VoteImportResult::Ok);
 
-		// double voting not allowed
-		assert!(!rounds.add_vote(
-			&round,
-			(Keyring::Alice.public(), Keyring::Alice.sign(b"I am committed")),
-			true
-		));
+		// double voting (same vote), ok, no effect
+		assert_eq!(rounds.add_vote(vote.clone()), VoteImportResult::Ok);
 
+		vote.id = Keyring::Dave.public();
+		vote.signature = Keyring::Dave.sign(b"I am committed");
 		// invalid vote (Dave is not a validator)
-		assert!(!rounds.add_vote(
-			&round,
-			(Keyring::Dave.public(), Keyring::Dave.sign(b"I am committed")),
-			false
-		));
-		assert!(rounds.should_conclude(&round).is_none());
+		assert_eq!(rounds.add_vote(vote.clone()), VoteImportResult::Invalid);
 
+		vote.id = Keyring::Bob.public();
+		vote.signature = Keyring::Bob.sign(b"I am committed");
 		// add 2nd good vote
-		assert!(rounds.add_vote(
-			&round,
-			(Keyring::Bob.public(), Keyring::Bob.sign(b"I am committed")),
-			false
-		));
-		// round not concluded
-		assert!(rounds.should_conclude(&round).is_none());
+		assert_eq!(rounds.add_vote(vote.clone()), VoteImportResult::Ok);
 
-		// add 3rd good vote
-		assert!(rounds.add_vote(
-			&round,
-			(Keyring::Charlie.public(), Keyring::Charlie.sign(b"I am committed")),
-			false
-		));
-		// round concluded
-		assert!(rounds.should_conclude(&round).is_some());
-		rounds.conclude(round.1);
+		vote.id = Keyring::Charlie.public();
+		vote.signature = Keyring::Charlie.sign(b"I am committed");
+		// add 3rd good vote -> round concluded -> signatures present
+		assert_eq!(
+			rounds.add_vote(vote.clone()),
+			VoteImportResult::RoundConcluded(SignedCommitment {
+				commitment,
+				signatures: vec![
+					Some(Keyring::Alice.sign(b"I am committed")),
+					Some(Keyring::Bob.sign(b"I am committed")),
+					Some(Keyring::Charlie.sign(b"I am committed")),
+					None,
+				]
+			})
+		);
+		rounds.conclude(block_number);
 
+		vote.id = Keyring::Eve.public();
+		vote.signature = Keyring::Eve.sign(b"I am committed");
 		// Eve is a validator, but round was concluded, adding vote disallowed
-		assert!(!rounds.add_vote(
-			&round,
-			(Keyring::Eve.public(), Keyring::Eve.sign(b"I am committed")),
-			false
-		));
+		assert_eq!(rounds.add_vote(vote), VoteImportResult::Stale);
 	}
 
 	#[test]
@@ -336,30 +341,39 @@ mod tests {
 			42,
 		)
 		.unwrap();
-		let alice = (Keyring::Alice.public(), Keyring::Alice.sign(b"I am committed"));
+		let validator_set_id = validators.id();
 
+		// active rounds starts at block 10
 		let session_start = 10u64.into();
-		let mut rounds = Rounds::<H256, Block>::new(session_start, validators);
+		let mut rounds = Rounds::<Block>::new(session_start, validators);
 
-		let mut vote = (H256::from_low_u64_le(1), 9);
+		// vote on round 9
+		let block_number = 9;
+		let payload = Payload::from_single_entry(MMR_ROOT_ID, vec![]);
+		let commitment = Commitment { block_number, payload, validator_set_id };
+		let mut vote = VoteMessage {
+			id: Keyring::Alice.public(),
+			commitment,
+			signature: Keyring::Alice.sign(b"I am committed"),
+		};
 		// add vote for previous session, should fail
-		assert!(!rounds.add_vote(&vote, alice.clone(), true));
+		assert_eq!(rounds.add_vote(vote.clone()), VoteImportResult::Stale);
 		// no votes present
 		assert!(rounds.rounds.is_empty());
 
 		// simulate 11 was concluded
 		rounds.best_done = Some(11);
 		// add votes for current session, but already concluded rounds, should fail
-		vote.1 = 10;
-		assert!(!rounds.add_vote(&vote, alice.clone(), true));
-		vote.1 = 11;
-		assert!(!rounds.add_vote(&vote, alice.clone(), true));
+		vote.commitment.block_number = 10;
+		assert_eq!(rounds.add_vote(vote.clone()), VoteImportResult::Stale);
+		vote.commitment.block_number = 11;
+		assert_eq!(rounds.add_vote(vote.clone()), VoteImportResult::Stale);
 		// no votes present
 		assert!(rounds.rounds.is_empty());
 
-		// add good vote
-		vote.1 = 12;
-		assert!(rounds.add_vote(&vote, alice, true));
+		// add vote for active round 12
+		vote.commitment.block_number = 12;
+		assert_eq!(rounds.add_vote(vote), VoteImportResult::Ok);
 		// good vote present
 		assert_eq!(rounds.rounds.len(), 1);
 	}
@@ -369,88 +383,73 @@ mod tests {
 		sp_tracing::try_init_simple();
 
 		let validators = ValidatorSet::<Public>::new(
-			vec![
-				Keyring::Alice.public(),
-				Keyring::Bob.public(),
-				Keyring::Charlie.public(),
-				Keyring::Dave.public(),
-			],
+			vec![Keyring::Alice.public(), Keyring::Bob.public(), Keyring::Charlie.public()],
 			Default::default(),
 		)
 		.unwrap();
+		let validator_set_id = validators.id();
 
 		let session_start = 1u64.into();
-		let mut rounds = Rounds::<H256, Block>::new(session_start, validators);
+		let mut rounds = Rounds::<Block>::new(session_start, validators);
 
-		// round 1
-		assert!(rounds.add_vote(
-			&(H256::from_low_u64_le(1), 1),
-			(Keyring::Alice.public(), Keyring::Alice.sign(b"I am committed")),
-			true,
-		));
-		assert!(rounds.add_vote(
-			&(H256::from_low_u64_le(1), 1),
-			(Keyring::Bob.public(), Keyring::Bob.sign(b"I am committed")),
-			false,
-		));
-		assert!(rounds.add_vote(
-			&(H256::from_low_u64_le(1), 1),
-			(Keyring::Charlie.public(), Keyring::Charlie.sign(b"I am committed")),
-			false,
-		));
+		let payload = Payload::from_single_entry(MMR_ROOT_ID, vec![]);
+		let commitment = Commitment { block_number: 1, payload, validator_set_id };
+		let mut alice_vote = VoteMessage {
+			id: Keyring::Alice.public(),
+			commitment: commitment.clone(),
+			signature: Keyring::Alice.sign(b"I am committed"),
+		};
+		let mut bob_vote = VoteMessage {
+			id: Keyring::Bob.public(),
+			commitment: commitment.clone(),
+			signature: Keyring::Bob.sign(b"I am committed"),
+		};
+		let mut charlie_vote = VoteMessage {
+			id: Keyring::Charlie.public(),
+			commitment,
+			signature: Keyring::Charlie.sign(b"I am committed"),
+		};
+		let expected_signatures = vec![
+			Some(Keyring::Alice.sign(b"I am committed")),
+			Some(Keyring::Bob.sign(b"I am committed")),
+			Some(Keyring::Charlie.sign(b"I am committed")),
+		];
 
-		// round 2
-		assert!(rounds.add_vote(
-			&(H256::from_low_u64_le(2), 2),
-			(Keyring::Alice.public(), Keyring::Alice.sign(b"I am again committed")),
-			true,
-		));
-		assert!(rounds.add_vote(
-			&(H256::from_low_u64_le(2), 2),
-			(Keyring::Bob.public(), Keyring::Bob.sign(b"I am again committed")),
-			false,
-		));
-		assert!(rounds.add_vote(
-			&(H256::from_low_u64_le(2), 2),
-			(Keyring::Charlie.public(), Keyring::Charlie.sign(b"I am again committed")),
-			false,
-		));
-
-		// round 3
-		assert!(rounds.add_vote(
-			&(H256::from_low_u64_le(3), 3),
-			(Keyring::Alice.public(), Keyring::Alice.sign(b"I am still committed")),
-			true,
-		));
-		assert!(rounds.add_vote(
-			&(H256::from_low_u64_le(3), 3),
-			(Keyring::Bob.public(), Keyring::Bob.sign(b"I am still committed")),
-			false,
-		));
-		assert!(rounds.add_vote(
-			&(H256::from_low_u64_le(3), 3),
-			(Keyring::Charlie.public(), Keyring::Charlie.sign(b"I am still committed")),
-			false,
-		));
-		assert_eq!(3, rounds.rounds.len());
-
-		// conclude unknown round
-		assert!(rounds.should_conclude(&(H256::from_low_u64_le(5), 5)).is_none());
-		assert_eq!(3, rounds.rounds.len());
-
-		// conclude round 2
-		let signatures = rounds.should_conclude(&(H256::from_low_u64_le(2), 2)).unwrap();
-		rounds.conclude(2);
+		// round 1 - only 2 out of 3 vote
+		assert_eq!(rounds.add_vote(alice_vote.clone()), VoteImportResult::Ok);
+		assert_eq!(rounds.add_vote(charlie_vote.clone()), VoteImportResult::Ok);
+		// should be 1 active round
 		assert_eq!(1, rounds.rounds.len());
 
+		// round 2 - only Charlie votes
+		charlie_vote.commitment.block_number = 2;
+		assert_eq!(rounds.add_vote(charlie_vote.clone()), VoteImportResult::Ok);
+		// should be 2 active rounds
+		assert_eq!(2, rounds.rounds.len());
+
+		// round 3 - all validators vote -> round is concluded
+		alice_vote.commitment.block_number = 3;
+		bob_vote.commitment.block_number = 3;
+		charlie_vote.commitment.block_number = 3;
+		assert_eq!(rounds.add_vote(alice_vote.clone()), VoteImportResult::Ok);
+		assert_eq!(rounds.add_vote(bob_vote.clone()), VoteImportResult::Ok);
 		assert_eq!(
-			signatures,
-			vec![
-				Some(Keyring::Alice.sign(b"I am again committed")),
-				Some(Keyring::Bob.sign(b"I am again committed")),
-				Some(Keyring::Charlie.sign(b"I am again committed")),
-				None
-			]
+			rounds.add_vote(charlie_vote.clone()),
+			VoteImportResult::RoundConcluded(SignedCommitment {
+				commitment: charlie_vote.commitment,
+				signatures: expected_signatures
+			})
 		);
+		// should be only 2 active since this one auto-concluded
+		assert_eq!(2, rounds.rounds.len());
+
+		// conclude round 2
+		rounds.conclude(2);
+		// should be no more active rounds since 2 was officially concluded and round "1" is stale
+		assert!(rounds.rounds.is_empty());
+
+		// conclude round 3
+		rounds.conclude(3);
+		assert!(rounds.previous_votes.is_empty());
 	}
 }

substrate/client/beefy/src/tests.rs

@@ -26,12 +26,12 @@ use crate::{
 	},
 	gossip_protocol_name,
 	justification::*,
-	keystore::tests::Keyring as BeefyKeyring,
 	load_or_init_voter_state, wait_for_runtime_pallet, BeefyRPCLinks, BeefyVoterLinks, KnownPeers,
 	PersistedState,
 };
 use beefy_primitives::{
 	crypto::{AuthorityId, Signature},
+	keyring::Keyring as BeefyKeyring,
 	known_payloads,
 	mmr::MmrRootProvider,
 	BeefyApi, Commitment, ConsensusLog, MmrRootHash, Payload, SignedCommitment, ValidatorSet,

substrate/client/beefy/src/worker.rs

@@ -26,13 +26,13 @@ use crate::{
 	keystore::BeefyKeystore,
 	metric_inc, metric_set,
 	metrics::Metrics,
-	round::Rounds,
+	round::{Rounds, VoteImportResult},
 	BeefyVoterLinks, LOG_TARGET,
 };
 use beefy_primitives::{
 	crypto::{AuthorityId, Signature},
-	Commitment, ConsensusLog, Payload, PayloadProvider, SignedCommitment, ValidatorSet,
-	VersionedFinalityProof, VoteMessage, BEEFY_ENGINE_ID,
+	Commitment, ConsensusLog, PayloadProvider, ValidatorSet, VersionedFinalityProof, VoteMessage,
+	BEEFY_ENGINE_ID,
 };
 use codec::{Codec, Decode, Encode};
 use futures::{stream::Fuse, FutureExt, StreamExt};
@@ -55,6 +55,7 @@ use std::{
 	marker::PhantomData,
 	sync::Arc,
 };
+
 /// Bound for the number of buffered future voting rounds.
 const MAX_BUFFERED_VOTE_ROUNDS: usize = 600;
 /// Bound for the number of buffered votes per round number.
@@ -83,17 +84,14 @@ pub(crate) struct VoterOracle<B: Block> {
 	/// 3. lagging behind GRANDPA: queue has [1, N] elements, where all `mandatory_done == false`.
 	///    In this state, everytime a session gets its mandatory block BEEFY finalized, it's
 	///    popped off the queue, eventually getting to state `2. up-to-date`.
-	sessions: VecDeque<Rounds<Payload, B>>,
+	sessions: VecDeque<Rounds<B>>,
 	/// Min delta in block numbers between two blocks, BEEFY should vote on.
 	min_block_delta: u32,
 }
 
 impl<B: Block> VoterOracle<B> {
 	/// Verify provided `sessions` satisfies requirements, then build `VoterOracle`.
-	pub fn checked_new(
-		sessions: VecDeque<Rounds<Payload, B>>,
-		min_block_delta: u32,
-	) -> Option<Self> {
+	pub fn checked_new(sessions: VecDeque<Rounds<B>>, min_block_delta: u32) -> Option<Self> {
 		let mut prev_start = Zero::zero();
 		let mut prev_validator_id = None;
 		// verifies the
@@ -136,13 +134,13 @@ impl<B: Block> VoterOracle<B> {
 
 	// Return reference to rounds pertaining to first session in the queue.
 	// Voting will always happen at the head of the queue.
-	fn active_rounds(&self) -> Option<&Rounds<Payload, B>> {
+	fn active_rounds(&self) -> Option<&Rounds<B>> {
 		self.sessions.front()
 	}
 
 	// Return mutable reference to rounds pertaining to first session in the queue.
 	// Voting will always happen at the head of the queue.
-	fn active_rounds_mut(&mut self) -> Option<&mut Rounds<Payload, B>> {
+	fn active_rounds_mut(&mut self) -> Option<&mut Rounds<B>> {
 		self.sessions.front_mut()
 	}
 
@@ -157,7 +155,7 @@ impl<B: Block> VoterOracle<B> {
 	}
 
 	/// Add new observed session to the Oracle.
-	pub fn add_session(&mut self, rounds: Rounds<Payload, B>) {
+	pub fn add_session(&mut self, rounds: Rounds<B>) {
 		self.sessions.push_back(rounds);
 		// Once we add a new session we can drop/prune previous session if it's been finalized.
 		self.try_prune();
@@ -264,6 +262,8 @@ pub(crate) struct PersistedState<B: Block> {
 	best_grandpa_block_header: <B as Block>::Header,
 	/// Best block a BEEFY voting round has been concluded for.
 	best_beefy_block: NumberFor<B>,
+	/// Best block we voted on.
+	best_voted: NumberFor<B>,
 	/// Chooses which incoming votes to accept and which votes to generate.
 	/// Keeps track of voting seen for current and future rounds.
 	voting_oracle: VoterOracle<B>,
@@ -273,12 +273,13 @@ impl<B: Block> PersistedState<B> {
 	pub fn checked_new(
 		grandpa_header: <B as Block>::Header,
 		best_beefy: NumberFor<B>,
-		sessions: VecDeque<Rounds<Payload, B>>,
+		sessions: VecDeque<Rounds<B>>,
 		min_block_delta: u32,
 	) -> Option<Self> {
 		VoterOracle::checked_new(sessions, min_block_delta).map(|voting_oracle| PersistedState {
 			best_grandpa_block_header: grandpa_header,
 			best_beefy_block: best_beefy,
+			best_voted: Zero::zero(),
 			voting_oracle,
 		})
 	}
@@ -381,7 +382,7 @@ where
 		&self.persisted_state.voting_oracle
 	}
 
-	fn active_rounds(&mut self) -> Option<&Rounds<Payload, B>> {
+	fn active_rounds(&mut self) -> Option<&Rounds<B>> {
 		self.persisted_state.voting_oracle.active_rounds()
 	}
 
@@ -486,12 +487,9 @@ where
 	) -> Result<(), Error> {
 		let block_num = vote.commitment.block_number;
 		let best_grandpa = self.best_grandpa_block();
+		self.gossip_validator.note_round(block_num);
 		match self.voting_oracle().triage_round(block_num, best_grandpa)? {
-			RoundAction::Process => self.handle_vote(
-				(vote.commitment.payload, vote.commitment.block_number),
-				(vote.id, vote.signature),
-				false,
-			)?,
+			RoundAction::Process => self.handle_vote(vote)?,
 			RoundAction::Enqueue => {
 				debug!(target: LOG_TARGET, "🥩 Buffer vote for round: {:?}.", block_num);
 				if self.pending_votes.len() < MAX_BUFFERED_VOTE_ROUNDS {
@@ -546,57 +544,47 @@ where
 
 	fn handle_vote(
 		&mut self,
-		round: (Payload, NumberFor<B>),
-		vote: (AuthorityId, Signature),
-		self_vote: bool,
+		vote: VoteMessage<NumberFor<B>, AuthorityId, Signature>,
 	) -> Result<(), Error> {
-		self.gossip_validator.note_round(round.1);
-
 		let rounds = self
 			.persisted_state
 			.voting_oracle
 			.active_rounds_mut()
 			.ok_or(Error::UninitSession)?;
 
-		if rounds.add_vote(&round, vote, self_vote) {
-			if let Some(signatures) = rounds.should_conclude(&round) {
-				self.gossip_validator.conclude_round(round.1);
-
-				let block_num = round.1;
-				let commitment = Commitment {
-					payload: round.0,
-					block_number: block_num,
-					validator_set_id: rounds.validator_set_id(),
-				};
-
-				let finality_proof =
-					VersionedFinalityProof::V1(SignedCommitment { commitment, signatures });
-
-				metric_set!(self, beefy_round_concluded, block_num);
+		let block_number = vote.commitment.block_number;
+		match rounds.add_vote(vote) {
+			VoteImportResult::RoundConcluded(signed_commitment) => {
+				self.gossip_validator.conclude_round(block_number);
+				metric_set!(self, beefy_round_concluded, block_number);
 
+				let finality_proof = VersionedFinalityProof::V1(signed_commitment);
 				info!(
 					target: LOG_TARGET,
-					"🥩 Round #{} concluded, finality_proof: {:?}.", round.1, finality_proof
+					"🥩 Round #{} concluded, finality_proof: {:?}.", block_number, finality_proof
 				);
-
 				// We created the `finality_proof` and know to be valid.
 				// New state is persisted after finalization.
 				self.finalize(finality_proof)?;
-			} else {
-				let mandatory_round = self
+			},
+			VoteImportResult::Ok => {
+				// Persist state after handling mandatory block vote.
+				if self
 					.voting_oracle()
 					.mandatory_pending()
-					.map(|p| p.0 == round.1)
-					.unwrap_or(false);
-				// Persist state after handling self vote to avoid double voting in case
-				// of voter restarts.
-				// Also persist state after handling mandatory block vote.
-				if self_vote || mandatory_round {
+					.map(|(mandatory_num, _)| mandatory_num == block_number)
+					.unwrap_or(false)
+				{
 					crate::aux_schema::write_voter_state(&*self.backend, &self.persisted_state)
 						.map_err(|e| Error::Backend(e.to_string()))?;
 				}
-			}
-		}
+			},
+			VoteImportResult::Equivocation => {
+				// TODO: report returned `EquivocationProof` to chain through `pallet-beefy`.
+				()
+			},
+			VoteImportResult::Invalid | VoteImportResult::Stale => (),
+		};
 		Ok(())
 	}
 
@@ -695,11 +683,7 @@ where
 			for (num, votes) in votes_to_handle.into_iter() {
 				debug!(target: LOG_TARGET, "🥩 Handle buffered votes for: {:?}.", num);
 				for v in votes.into_iter() {
-					if let Err(err) = self.handle_vote(
-						(v.commitment.payload, v.commitment.block_number),
-						(v.id, v.signature),
-						false,
-					) {
+					if let Err(err) = self.handle_vote(v) {
 						error!(target: LOG_TARGET, "🥩 Error handling buffered vote: {}", err);
 					};
 				}
@@ -716,7 +700,9 @@ where
 			.voting_target(self.best_beefy_block(), self.best_grandpa_block())
 		{
 			metric_set!(self, beefy_should_vote_on, target);
-			self.do_vote(target)?;
+			if target > self.persisted_state.best_voted {
+				self.do_vote(target)?;
+			}
 		}
 		Ok(())
 	}
@@ -766,13 +752,6 @@ where
 			.voting_oracle
 			.active_rounds_mut()
 			.ok_or(Error::UninitSession)?;
-		if !rounds.should_self_vote(&(payload.clone(), target_number)) {
-			debug!(
-				target: LOG_TARGET,
-				"🥩 Don't double vote for block number: {:?}", target_number
-			);
-			return Ok(())
-		}
 		let (validators, validator_set_id) = (rounds.validators(), rounds.validator_set_id());
 
 		let authority_id = if let Some(id) = self.key_store.authority_id(validators) {
@@ -812,17 +791,17 @@ where
 
 		debug!(target: LOG_TARGET, "🥩 Sent vote message: {:?}", message);
 
-		if let Err(err) = self.handle_vote(
-			(message.commitment.payload, message.commitment.block_number),
-			(message.id, message.signature),
-			true,
-		) {
+		if let Err(err) = self.handle_vote(message) {
 			error!(target: LOG_TARGET, "🥩 Error handling self vote: {}", err);
 		}
 
 		self.gossip_engine.gossip_message(topic::<B>(), encoded_message, false);
 
-		Ok(())
+		// Persist state after vote to avoid double voting in case of voter restarts.
+		self.persisted_state.best_voted = target_number;
+		metric_set!(self, beefy_best_voted, target_number);
+		crate::aux_schema::write_voter_state(&*self.backend, &self.persisted_state)
+			.map_err(|e| Error::Backend(e.to_string()))
 	}
 
 	fn process_new_state(&mut self) {
@@ -848,8 +827,7 @@ where
 
 	/// Main loop for BEEFY worker.
 	///
-	/// Wait for BEEFY runtime pallet to be available, then start the main async loop
-	/// which is driven by finality notifications and gossiped votes.
+	/// Run the main async loop which is driven by finality notifications and gossiped votes.
 	pub(crate) async fn run(
 		mut self,
 		mut block_import_justif: Fuse<NotificationReceiver<BeefyVersionedFinalityProof<B>>>,
@@ -992,14 +970,15 @@ pub(crate) mod tests {
 	use super::*;
 	use crate::{
 		communication::notification::{BeefyBestBlockStream, BeefyVersionedFinalityProofStream},
-		keystore::tests::Keyring,
 		tests::{
 			create_beefy_keystore, get_beefy_streams, make_beefy_ids, BeefyPeer, BeefyTestNet,
 			TestApi,
 		},
 		BeefyRPCLinks, KnownPeers,
 	};
-	use beefy_primitives::{known_payloads, mmr::MmrRootProvider};
+	use beefy_primitives::{
+		keyring::Keyring, known_payloads, mmr::MmrRootProvider, Payload, SignedCommitment,
+	};
 	use futures::{future::poll_fn, task::Poll};
 	use parking_lot::Mutex;
 	use sc_client_api::{Backend as BackendT, HeaderBackend};
@@ -1007,7 +986,7 @@ pub(crate) mod tests {
 	use sc_network_test::TestNetFactory;
 	use sp_api::HeaderT;
 	use sp_blockchain::Backend as BlockchainBackendT;
-	use sp_runtime::traits::{One, Zero};
+	use sp_runtime::traits::One;
 	use substrate_test_runtime_client::{
 		runtime::{Block, Digest, DigestItem, Header, H256},
 		Backend,
@@ -1018,7 +997,7 @@ pub(crate) mod tests {
 			&self.voting_oracle
 		}
 
-		pub fn active_round(&self) -> Option<&Rounds<Payload, B>> {
+		pub fn active_round(&self) -> Option<&Rounds<B>> {
 			self.voting_oracle.active_rounds()
 		}
 
@@ -1032,7 +1011,7 @@ pub(crate) mod tests {
 	}
 
 	impl<B: super::Block> VoterOracle<B> {
-		pub fn sessions(&self) -> &VecDeque<Rounds<Payload, B>> {
+		pub fn sessions(&self) -> &VecDeque<Rounds<B>> {
 			&self.sessions
 		}
 	}
@@ -1090,7 +1069,7 @@ pub(crate) mod tests {
 			min_block_delta,
 		)
 		.unwrap();
-		let payload_provider = MmrRootProvider::new(api);
+		let payload_provider = MmrRootProvider::new(api.clone());
 		let worker_params = crate::worker::WorkerParams {
 			backend,
 			payload_provider,