fix: remove is_vote_valid

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

@@ -7,7 +7,7 @@ use sp_std::vec::Vec;
 
 use crate::{
 	vote_storage::{AuthorityVote, VoteStorage},
-	CorruptStorageError, ElectionIdentifier, SharedDataHash,
+	CorruptStorageError, ElectionIdentifier,
 };
 
 pub struct ConsensusVote<ES: ElectoralSystem> {
@@ -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.
 	///
@@ -238,7 +217,6 @@ mod access {
 	//! correct ElectoralSystem implementation.
 
 	use super::{CorruptStorageError, ElectionIdentifierOf, ElectoralSystem};
-	use crate::{vote_storage::VoteStorage, SharedDataHash};
 
 	/// Represents the current consensus, and how it has changed since it was last checked (i.e.
 	/// 'check_consensus' was called).
@@ -313,11 +291,6 @@ mod access {
 			&self,
 		) -> Result<<Self::ElectoralSystem as ElectoralSystem>::ElectionState, CorruptStorageError>;
 
-		fn shared_data_hash_of(
-			&self,
-			data: <<Self::ElectoralSystem as ElectoralSystem>::Vote as VoteStorage>::SharedData,
-		) -> SharedDataHash;
-
 		#[cfg(test)]
 		fn election_identifier(
 			&self,

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

@@ -52,7 +52,6 @@ macro_rules! generate_electoral_system_tuple_impls {
             };
 
             use crate::{
-                SharedDataHash,
                 CorruptStorageError,
                 electoral_system::{
                     ElectoralSystem,
@@ -73,7 +72,7 @@ macro_rules! generate_electoral_system_tuple_impls {
                 },
                 ElectionIdentifier,
             };
-            use crate::vote_storage::composite::$module::{CompositeVoteProperties, CompositeVote, CompositePartialVote, CompositeSharedData};
+            use crate::vote_storage::composite::$module::{CompositeVoteProperties, CompositeVote, CompositePartialVote};
 
             use frame_support::{Parameter, pallet_prelude::{Member, MaybeSerializeDeserialize}};
 
@@ -264,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<(
@@ -442,11 +425,6 @@ macro_rules! generate_electoral_system_tuple_impls {
                 }
             }
 
-            // TODO: Push this into lower level
-            fn shared_data_hash_of(&self, data: <<Self::ElectoralSystem as ElectoralSystem>::Vote as VoteStorage>::SharedData) -> SharedDataHash {
-                SharedDataHash::of(&CompositeSharedData::<$(<<$electoral_system as ElectoralSystem>::Vote as VoteStorage>::SharedData),*>::$current(data))
-            }
-
             #[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,7 +4,7 @@ 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};
@@ -109,15 +109,6 @@ impl<
 		}
 	}
 
-	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 != election_access.shared_data_hash_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 +134,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,17 +150,21 @@ 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() {
+			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)
 					.and_modify(|blocks_height| blocks_height.push(vote.block))

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,
@@ -157,34 +153,22 @@ fn consensus_when_all_votes_the_same_but_different_blocks() {
 }
 
 #[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
+fn no_consensus_when_votes_are_filtered_because_invalid() {
+	with_default_state()
+		.force_consensus_update(crate::electoral_system::ConsensusStatus::Gained {
+			most_recent: Some((10, 12)),
+			new: (11, 13),
 		})
-		.unwrap());
+		.expect_consensus(
+			generate_votes_with_different_slots(
+				0,
+				MonotonicChangeVote { value: 0, block: 0 },
+				AUTHORITY_COUNT,
+				// neither the value has changed nor the block, both of which fail validity checks
+				MonotonicChangeVote { value: 11, block: 13 },
+			),
+			None,
+		);
 }
 
 #[test]

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

@@ -637,12 +637,6 @@ pub mod pallet {
 				ElectionState::<T, I>::get(self.unique_monotonic_identifier())
 					.ok_or_else(CorruptStorageError::new)
 			}
-			fn shared_data_hash_of(
-				&self,
-				shared_data: <<T::ElectoralSystem as ElectoralSystem>::Vote as VoteStorage>::SharedData,
-			) -> SharedDataHash {
-				todo!()
-			}
 			#[cfg(test)]
 			fn election_identifier(
 				&self,
@@ -1283,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,

state-chain/pallets/cf-elections/src/vote_storage/change.rs

@@ -19,6 +19,7 @@ pub struct MonotonicChange<T: Parameter + Member, S: Parameter + Member> {
 impl<T: Parameter + Member, S: Parameter + Member> VoteStorage for MonotonicChange<T, S> {
 	type Properties = ();
 	type Vote = MonotonicChangeVote<T, S>;
+	// We have sa shared data hash here. 
 	type PartialVote = MonotonicChangeVote<SharedDataHash, S>;
 	type IndividualComponent = S;
 	type BitmapComponent = SharedDataHash;