From 1cc009bd42142177a8023a5789515f7d179e1435 Mon Sep 17 00:00:00 2001 From: kylezs Date: Wed, 30 Oct 2024 12:07:24 +0100 Subject: [PATCH] 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 --- .../cf-elections/src/electoral_system.rs | 22 +----- .../src/electoral_systems/composite.rs | 17 +---- .../src/electoral_systems/mock.rs | 8 --- .../src/electoral_systems/mocks.rs | 8 --- .../src/electoral_systems/mocks/access.rs | 9 --- .../src/electoral_systems/monotonic_change.rs | 38 +++++----- .../tests/monotonic_change.rs | 71 +++++++++---------- state-chain/pallets/cf-elections/src/lib.rs | 13 ---- 8 files changed, 53 insertions(+), 133 deletions(-) diff --git a/state-chain/pallets/cf-elections/src/electoral_system.rs b/state-chain/pallets/cf-elections/src/electoral_system.rs index 06a63ec2ef..06583ffca3 100644 --- a/state-chain/pallets/cf-elections/src/electoral_system.rs +++ b/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>( - _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. /// @@ -311,6 +290,7 @@ mod access { fn state( &self, ) -> Result<::ElectionState, CorruptStorageError>; + #[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 c2da030d6c..49c830e30e 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/composite.rs +++ b/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>( - 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<( @@ -440,6 +424,7 @@ macro_rules! generate_electoral_system_tuple_impls { _ => Err(CorruptStorageError::new()) } } + #[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 dad3538d90..d613a5c15e 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 577471c824..4f28f85440 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 79e008e569..74500c97f7 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 7ab63e57b1..9ea3054981 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}; @@ -12,7 +12,7 @@ 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,7 +101,8 @@ impl< ), ) -> bool { match current_vote { - AuthorityVoteOf::::Vote(current_vote) => current_vote != proposed_vote, + AuthorityVoteOf::::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. @@ -109,15 +110,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 != SharedDataHash::of(&previous_value) && - partial_vote.block > previous_slot) - } fn generate_vote_properties( _election_identifier: ElectionIdentifierOf, _previous_vote: Option<(VotePropertiesOf, AuthorityVoteOf)>, @@ -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,7 +151,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,13 +159,15 @@ 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() { - 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)| { 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 5e7ec14eb3..3c3bd0a798 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, @@ -80,7 +76,7 @@ fn generate_votes( fn generate_votes_with_different_slots( correct_voters: AuthorityCount, - correct_value: MonotonicChangeVote, + correct_value: Value, incorrect_voters: AuthorityCount, incorrect_value: MonotonicChangeVote, ) -> ConsensusVotes { @@ -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::::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] diff --git a/state-chain/pallets/cf-elections/src/lib.rs b/state-chain/pallets/cf-elections/src/lib.rs index 12e8900a04..2f497525bf 100644 --- a/state-chain/pallets/cf-elections/src/lib.rs +++ b/state-chain/pallets/cf-elections/src/lib.rs @@ -1277,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,