fix: remove is_vote_valid (#5363)

* fix: correct shared data hash

* fix: remove is_vote_valid

* test: fix tests

* chore: remove stale comment

* fix: only compare value, not slot number

* chore: clippy

state-chain/pallets/cf-elections/src/electoral_system.rs

@@ -141,27 +141,6 @@ pub trait ElectoralSystem: 'static + Sized {
 		true
 	}
 
-	/// This is used in the vote extrinsic to disallow a validator from providing votes that do not
-	/// pass this check. It is guaranteed that any vote values provided to
-	/// `generate_vote_properties`, or `check_consensus` have past this check.
-	///
-	/// We only pass the `PartialVote` into the validity check, instead of the `AuthorityVote` or
-	/// `Vote`, to ensure the check's logic is consistent regardless of if the authority provides a
-	/// `Vote` or `PartialVote`. If the check was different depending on if the authority voted with
-	/// a `PartialVote` or `Vote`, then check only guarantees of the intersection of the two
-	/// variations.
-	///
-	/// You should *NEVER* update the epoch during this call. And in general updating any other
-	/// state of any pallet is ill advised, and should instead be done in the 'on_finalize'
-	/// function.
-	fn is_vote_valid<ElectionAccess: ElectionReadAccess<ElectoralSystem = Self>>(
-		_election_identifier: ElectionIdentifierOf<Self>,
-		_election_access: &ElectionAccess,
-		_partial_vote: &<Self::Vote as VoteStorage>::PartialVote,
-	) -> Result<bool, CorruptStorageError> {
-		Ok(true)
-	}
-
 	/// This is called every time a vote occurs. It associates the vote with a `Properties`
 	/// value.
 	///
@@ -311,6 +290,7 @@ mod access {
 		fn state(
 			&self,
 		) -> Result<<Self::ElectoralSystem as ElectoralSystem>::ElectionState, CorruptStorageError>;
+
 		#[cfg(test)]
 		fn election_identifier(
 			&self,

state-chain/pallets/cf-elections/src/electoral_systems/composite.rs

@@ -263,22 +263,6 @@ macro_rules! generate_electoral_system_tuple_impls {
                     }
                 }
 
-
-                fn is_vote_valid<ElectionAccess: ElectionReadAccess<ElectoralSystem = Self>>(
-                    election_identifier: ElectionIdentifier<Self::ElectionIdentifierExtra>,
-                    election_access: &ElectionAccess,
-                    partial_vote: &<Self::Vote as VoteStorage>::PartialVote,
-                ) -> Result<bool, CorruptStorageError> {
-                    Ok(match (*election_identifier.extra(), partial_vote) {
-                        $((CompositeElectionIdentifierExtra::$electoral_system(extra), CompositePartialVote::$electoral_system(partial_vote)) => <$electoral_system as ElectoralSystem>::is_vote_valid(
-                            election_identifier.with_extra(extra),
-                            &CompositeElectionAccess::<tags::$electoral_system, _, ElectionAccess>::new(election_access),
-                            partial_vote,
-                        )?,)*
-                        _ => false,
-                    })
-                }
-
                 fn generate_vote_properties(
                     election_identifier: ElectionIdentifier<Self::ElectionIdentifierExtra>,
                     previous_vote: Option<(
@@ -440,6 +424,7 @@ macro_rules! generate_electoral_system_tuple_impls {
                     _ => Err(CorruptStorageError::new())
                 }
             }
+
             #[cfg(test)]
             fn election_identifier(&self) -> Result<ElectionIdentifierOf<Self::ElectoralSystem>, CorruptStorageError> {
                 let composite_identifier = self.ea.borrow().election_identifier()?;

state-chain/pallets/cf-elections/src/electoral_systems/mock.rs

@@ -193,12 +193,4 @@ impl ElectoralSystem for MockElectoralSystem {
 	) -> bool {
 		Self::vote_needed()
 	}
-
-	fn is_vote_valid<ElectionAccess: ElectionReadAccess<ElectoralSystem = Self>>(
-		_election_identifier: crate::electoral_system::ElectionIdentifierOf<Self>,
-		_election_access: &ElectionAccess,
-		_partial_vote: &<Self::Vote as VoteStorage>::PartialVote,
-	) -> Result<bool, CorruptStorageError> {
-		Ok(Self::vote_valid())
-	}
 }

state-chain/pallets/cf-elections/src/electoral_systems/mocks.rs

@@ -6,7 +6,6 @@ use frame_support::{CloneNoBound, DebugNoBound, EqNoBound, PartialEqNoBound};
 
 pub mod access;
 
-use crate::{vote_storage::VoteStorage, CorruptStorageError};
 pub use access::*;
 use itertools::Itertools;
 
@@ -217,13 +216,6 @@ impl<ES: ElectoralSystem> TestContext<ES> {
 		}
 		self
 	}
-
-	pub fn is_vote_valid(
-		&self,
-		partial_vote: &<ES::Vote as VoteStorage>::PartialVote,
-	) -> Result<bool, CorruptStorageError> {
-		self.electoral_access.is_vote_valid(self.only_election_id(), partial_vote)
-	}
 }
 
 /// Allows registering checks for an electoral system. Once registered, the checks can be used

state-chain/pallets/cf-elections/src/electoral_systems/mocks/access.rs

@@ -3,7 +3,6 @@ use crate::{
 		ConsensusStatus, ConsensusVotes, ElectionIdentifierOf, ElectionReadAccess,
 		ElectionWriteAccess, ElectoralReadAccess, ElectoralSystem, ElectoralWriteAccess,
 	},
-	vote_storage::VoteStorage,
 	CorruptStorageError, ElectionIdentifier, UniqueMonotonicIdentifier,
 };
 use codec::Encode;
@@ -204,14 +203,6 @@ impl<ES: ElectoralSystem> MockAccess<ES> {
 	pub fn next_umi(&self) -> UniqueMonotonicIdentifier {
 		self.next_election_id
 	}
-
-	pub fn is_vote_valid(
-		&self,
-		election_identifier: ElectionIdentifierOf<ES>,
-		partial_vote: &<ES::Vote as VoteStorage>::PartialVote,
-	) -> Result<bool, CorruptStorageError> {
-		ES::is_vote_valid(election_identifier, &self.election(election_identifier)?, partial_vote)
-	}
 }
 
 impl<ES: ElectoralSystem> ElectoralReadAccess for MockAccess<ES> {

state-chain/pallets/cf-elections/src/electoral_systems/monotonic_change.rs

@@ -4,15 +4,15 @@ use crate::{
 		ElectionWriteAccess, ElectoralSystem, ElectoralWriteAccess, VotePropertiesOf,
 	},
 	vote_storage::{self, VoteStorage},
-	CorruptStorageError, SharedDataHash,
+	CorruptStorageError,
 };
 use cf_runtime_utilities::log_or_panic;
 use cf_utilities::{success_threshold_from_share_count, threshold_from_share_count};
 use frame_support::{
 	pallet_prelude::{MaybeSerializeDeserialize, Member},
 	Parameter,
 };
-use sp_std::{collections::btree_map::BTreeMap, vec, vec::Vec};
+use sp_std::{collections::btree_map::BTreeMap, vec::Vec};
 
 /// This electoral system detects if a value changes. The SC can request that it detects if a
 /// particular value, the instance of which is specified by an identifier, has changed from some
@@ -101,23 +101,15 @@ impl<
 		),
 	) -> bool {
 		match current_vote {
-			AuthorityVoteOf::<Self>::Vote(current_vote) => current_vote != proposed_vote,
+			AuthorityVoteOf::<Self>::Vote(current_vote) =>
+				current_vote.value != proposed_vote.value,
 			// Could argue for either true or false. If the `PartialVote` is never reconstructed and
 			// becomes invalid, then this function will be bypassed and the vote will be considered
 			// needed. So false is safe, and true will likely result in unneeded voting.
 			_ => false,
 		}
 	}
 
-	fn is_vote_valid<ElectionAccess: ElectionReadAccess<ElectoralSystem = Self>>(
-		_election_identifier: ElectionIdentifierOf<Self>,
-		election_access: &ElectionAccess,
-		partial_vote: &<Self::Vote as VoteStorage>::PartialVote,
-	) -> Result<bool, CorruptStorageError> {
-		let (_, previous_value, previous_slot) = election_access.properties()?;
-		Ok(partial_vote.value != SharedDataHash::of(&previous_value) &&
-			partial_vote.block > previous_slot)
-	}
 	fn generate_vote_properties(
 		_election_identifier: ElectionIdentifierOf<Self>,
 		_previous_vote: Option<(VotePropertiesOf<Self>, AuthorityVoteOf<Self>)>,
@@ -143,7 +135,13 @@ impl<
 					electoral_access
 						.set_unsynchronised_state_map(identifier, Some(block_height))?;
 				} else {
-					log_or_panic!("Should be impossible to reach consensus with the same value and/or lower block_height");
+					// We don't expect this to be hit, since we should have filtered out any votes
+					// that would cause this in check_consensus.
+					log_or_panic!(
+						"No change detected for {:?}, election_identifier: {:?}",
+						identifier,
+						election_identifier
+					);
 				}
 			}
 		}
@@ -153,21 +151,23 @@ impl<
 
 	fn check_consensus<ElectionAccess: ElectionReadAccess<ElectoralSystem = Self>>(
 		_election_identifier: ElectionIdentifierOf<Self>,
-		_election_access: &ElectionAccess,
+		election_access: &ElectionAccess,
 		_previous_consensus: Option<&Self::Consensus>,
 		consensus_votes: ConsensusVotes<Self>,
 	) -> Result<Option<Self::Consensus>, CorruptStorageError> {
 		let num_authorities = consensus_votes.num_authorities();
 		let active_votes = consensus_votes.active_votes();
 		let num_active_votes = active_votes.len() as u32;
 		let success_threshold = success_threshold_from_share_count(num_authorities);
+		let (_, previous_value, previous_block) = election_access.properties()?;
+
 		Ok(if num_active_votes >= success_threshold {
 			let mut counts: BTreeMap<Value, Vec<BlockHeight>> = BTreeMap::new();
-			for vote in active_votes.clone() {
-				counts
-					.entry(vote.value)
-					.and_modify(|blocks_height| blocks_height.push(vote.block))
-					.or_insert(vec![vote.block]);
+			for vote in active_votes.clone().into_iter().filter(|monotonic_change_vote| {
+				monotonic_change_vote.block > previous_block &&
+					previous_value != monotonic_change_vote.value
+			}) {
+				counts.entry(vote.value).or_default().push(vote.block);
 			}
 
 			counts.iter().find_map(|(vote, blocks_height)| {

state-chain/pallets/cf-elections/src/electoral_systems/tests/monotonic_change.rs

@@ -2,10 +2,9 @@ use super::{mocks::*, register_checks};
 use crate::{
 	electoral_system::{ConsensusVote, ConsensusVotes},
 	electoral_systems::monotonic_change::*,
-	SharedDataHash,
 };
 
-use crate::{electoral_system::ElectoralWriteAccess, vote_storage::change::MonotonicChangeVote};
+use crate::vote_storage::change::MonotonicChangeVote;
 use cf_primitives::AuthorityCount;
 use cf_utilities::assert_panics;
 
@@ -53,9 +52,6 @@ const SUCCESS_THRESHOLD: AuthorityCount =
 fn with_default_state() -> TestContext<SimpleMonotonicChange> {
 	TestSetup::<SimpleMonotonicChange>::default().build_with_initial_election()
 }
-fn with_no_election() -> TestContext<SimpleMonotonicChange> {
-	TestSetup::<SimpleMonotonicChange>::default().build()
-}
 
 fn generate_votes(
 	correct_voters: AuthorityCount,
@@ -80,7 +76,7 @@ fn generate_votes(
 
 fn generate_votes_with_different_slots(
 	correct_voters: AuthorityCount,
-	correct_value: MonotonicChangeVote<Value, Slot>,
+	correct_value: Value,
 	incorrect_voters: AuthorityCount,
 	incorrect_value: MonotonicChangeVote<Value, Slot>,
 ) -> ConsensusVotes<SimpleMonotonicChange> {
@@ -90,7 +86,8 @@ fn generate_votes_with_different_slots(
 			.map(|(index, _)| ConsensusVote {
 				vote: Some((
 					(),
-					MonotonicChangeVote { value: correct_value.value, block: index as u32 },
+					// we add one so it's higher than 0 which is the default value.
+					MonotonicChangeVote { value: correct_value, block: (index + 1) as u32 },
 				)),
 				validator_id: (),
 			})
@@ -139,52 +136,48 @@ fn consensus_when_all_votes_the_same_but_different_blocks() {
 	with_default_state().expect_consensus(
 		generate_votes_with_different_slots(
 			SUCCESS_THRESHOLD,
-			MonotonicChangeVote { value: 1, block: 0 },
+			1,
 			3,
 			MonotonicChangeVote { value: 0, block: 0 },
 		),
-		Some((1, 6)),
+		Some((1, 7)),
 	);
 	with_default_state().expect_consensus(
 		generate_votes_with_different_slots(
 			AUTHORITY_COUNT,
-			MonotonicChangeVote { value: 1, block: 0 },
+			1,
 			0,
 			MonotonicChangeVote { value: 0, block: 0 },
 		),
-		Some((1, 6)),
+		Some((1, 7)),
 	);
 }
 
 #[test]
-fn votes_with_old_value_or_lower_block_are_rejected() {
-	const OLD_VALUE: u64 = 9;
-	const OLD_BLOCK: u32 = 7;
-	const NEW_VALUE: u64 = 10;
-	const NEW_BLOCK: u32 = 11;
-	let mut test = with_no_election();
-	let _ = test.mut_access().new_election((), ((), OLD_VALUE, OLD_BLOCK), ());
-	//new value but old block not valid
-	assert!(!test
-		.is_vote_valid(&MonotonicChangeVote {
-			value: SharedDataHash::of(&NEW_VALUE),
-			block: OLD_BLOCK
-		})
-		.unwrap());
-	//old value but new block not valid
-	assert!(!test
-		.is_vote_valid(&MonotonicChangeVote {
-			value: SharedDataHash::of(&OLD_VALUE),
-			block: NEW_BLOCK
-		})
-		.unwrap());
-	//old value and old block not valid
-	assert!(!test
-		.is_vote_valid(&MonotonicChangeVote {
-			value: SharedDataHash::of(&OLD_VALUE),
-			block: OLD_BLOCK
-		})
-		.unwrap());
+fn no_consensus_when_votes_are_filtered_because_invalid() {
+	TestSetup::<SimpleMonotonicChange>::default()
+		.with_initial_election_state((), ((), 1, 1), ())
+		.build_with_initial_election()
+		.expect_consensus(
+			generate_votes_with_different_slots(
+				0,
+				0,
+				AUTHORITY_COUNT,
+				// block is valid, but value is invalid
+				MonotonicChangeVote { value: 1, block: 2 },
+			),
+			None,
+		)
+		.expect_consensus(
+			generate_votes_with_different_slots(
+				0,
+				0,
+				AUTHORITY_COUNT,
+				// value is valid but block is invalid
+				MonotonicChangeVote { value: 2, block: 0 },
+			),
+			None,
+		);
 }
 
 #[test]

state-chain/pallets/cf-elections/src/lib.rs

@@ -1277,19 +1277,6 @@ pub mod pallet {
 					},
 				};
 
-				ensure!(
-					Self::with_electoral_access(
-						|electoral_access| -> Result<_, CorruptStorageError> {
-							<T::ElectoralSystem as ElectoralSystem>::is_vote_valid(
-								election_identifier,
-								&electoral_access.election(election_identifier)?,
-								&partial_vote,
-							)
-						}
-					)?,
-					Error::<T, I>::InvalidVote
-				);
-
 				Self::handle_corrupt_storage(Self::take_vote_and_then(
 					epoch_index,
 					unique_monotonic_identifier,