diff --git a/state-chain/pallets/cf-elections/src/electoral_system.rs b/state-chain/pallets/cf-elections/src/electoral_system.rs index 6d510b1ac80..06583ffca36 100644 --- a/state-chain/pallets/cf-elections/src/electoral_system.rs +++ b/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 { @@ -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>( - _election_identifier: ElectionIdentifierOf, - _election_access: &ElectionAccess, - _partial_vote: &::PartialVote, - ) -> Result { - 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<::ElectionState, CorruptStorageError>; - fn shared_data_hash_of( - &self, - data: <::Vote as VoteStorage>::SharedData, - ) -> SharedDataHash; - #[cfg(test)] fn election_identifier( &self, diff --git a/state-chain/pallets/cf-elections/src/electoral_systems/composite.rs b/state-chain/pallets/cf-elections/src/electoral_systems/composite.rs index 8dd6f90862f..49c830e30e7 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/composite.rs +++ b/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>( - election_identifier: ElectionIdentifier, - election_access: &ElectionAccess, - partial_vote: &::PartialVote, - ) -> Result { - 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::::new(election_access), - partial_vote, - )?,)* - _ => false, - }) - } - fn generate_vote_properties( election_identifier: ElectionIdentifier, 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: <::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, CorruptStorageError> { let composite_identifier = self.ea.borrow().election_identifier()?; diff --git a/state-chain/pallets/cf-elections/src/electoral_systems/mock.rs b/state-chain/pallets/cf-elections/src/electoral_systems/mock.rs index dad3538d908..d613a5c15eb 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/mock.rs +++ b/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>( - _election_identifier: crate::electoral_system::ElectionIdentifierOf, - _election_access: &ElectionAccess, - _partial_vote: &::PartialVote, - ) -> Result { - Ok(Self::vote_valid()) - } } diff --git a/state-chain/pallets/cf-elections/src/electoral_systems/mocks.rs b/state-chain/pallets/cf-elections/src/electoral_systems/mocks.rs index 577471c8243..4f28f85440f 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/mocks.rs +++ b/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 TestContext { } self } - - pub fn is_vote_valid( - &self, - partial_vote: &::PartialVote, - ) -> Result { - 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 diff --git a/state-chain/pallets/cf-elections/src/electoral_systems/mocks/access.rs b/state-chain/pallets/cf-elections/src/electoral_systems/mocks/access.rs index 79e008e569b..74500c97f70 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/mocks/access.rs +++ b/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 MockAccess { pub fn next_umi(&self) -> UniqueMonotonicIdentifier { self.next_election_id } - - pub fn is_vote_valid( - &self, - election_identifier: ElectionIdentifierOf, - partial_vote: &::PartialVote, - ) -> Result { - ES::is_vote_valid(election_identifier, &self.election(election_identifier)?, partial_vote) - } } impl ElectoralReadAccess for MockAccess { diff --git a/state-chain/pallets/cf-elections/src/electoral_systems/monotonic_change.rs b/state-chain/pallets/cf-elections/src/electoral_systems/monotonic_change.rs index df77fe680ba..7aea8cc48cc 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/monotonic_change.rs +++ b/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>( - _election_identifier: ElectionIdentifierOf, - election_access: &ElectionAccess, - partial_vote: &::PartialVote, - ) -> Result { - 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, _previous_vote: Option<(VotePropertiesOf, AuthorityVoteOf)>, @@ -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,7 +150,7 @@ impl< fn check_consensus>( _election_identifier: ElectionIdentifierOf, - _election_access: &ElectionAccess, + election_access: &ElectionAccess, _previous_consensus: Option<&Self::Consensus>, consensus_votes: ConsensusVotes, ) -> Result, CorruptStorageError> { @@ -161,9 +158,13 @@ impl< 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> = 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)) diff --git a/state-chain/pallets/cf-elections/src/electoral_systems/tests/monotonic_change.rs b/state-chain/pallets/cf-elections/src/electoral_systems/tests/monotonic_change.rs index 5e7ec14eb34..2a830b472f0 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/tests/monotonic_change.rs +++ b/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 { TestSetup::::default().build_with_initial_election() } -fn with_no_election() -> TestContext { - TestSetup::::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] diff --git a/state-chain/pallets/cf-elections/src/lib.rs b/state-chain/pallets/cf-elections/src/lib.rs index 5b3c425ce91..2f497525bf5 100644 --- a/state-chain/pallets/cf-elections/src/lib.rs +++ b/state-chain/pallets/cf-elections/src/lib.rs @@ -637,12 +637,6 @@ pub mod pallet { ElectionState::::get(self.unique_monotonic_identifier()) .ok_or_else(CorruptStorageError::new) } - fn shared_data_hash_of( - &self, - shared_data: <::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> { - ::is_vote_valid( - election_identifier, - &electoral_access.election(election_identifier)?, - &partial_vote, - ) - } - )?, - Error::::InvalidVote - ); - Self::handle_corrupt_storage(Self::take_vote_and_then( epoch_index, unique_monotonic_identifier, diff --git a/state-chain/pallets/cf-elections/src/vote_storage/change.rs b/state-chain/pallets/cf-elections/src/vote_storage/change.rs index 0d6190c5ff0..8cf4347ac8f 100644 --- a/state-chain/pallets/cf-elections/src/vote_storage/change.rs +++ b/state-chain/pallets/cf-elections/src/vote_storage/change.rs @@ -19,6 +19,7 @@ pub struct MonotonicChange { impl VoteStorage for MonotonicChange { type Properties = (); type Vote = MonotonicChangeVote; + // We have sa shared data hash here. type PartialVote = MonotonicChangeVote; type IndividualComponent = S; type BitmapComponent = SharedDataHash;