From e019a0b49a00d0828cc0fdd8c7beb741a994f81f Mon Sep 17 00:00:00 2001 From: kylezs Date: Tue, 29 Oct 2024 14:55:02 +0100 Subject: [PATCH 1/6] fix: correct shared data hash --- state-chain/pallets/cf-elections/src/electoral_system.rs | 9 ++++++++- .../cf-elections/src/electoral_systems/composite.rs | 9 ++++++++- .../src/electoral_systems/monotonic_change.rs | 2 +- state-chain/pallets/cf-elections/src/lib.rs | 6 ++++++ 4 files changed, 23 insertions(+), 3 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..6d510b1ac8 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, + CorruptStorageError, ElectionIdentifier, SharedDataHash, }; pub struct ConsensusVote { @@ -238,6 +238,7 @@ 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). @@ -311,6 +312,12 @@ mod access { fn state( &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 c2da030d6c..8dd6f90862 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/composite.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/composite.rs @@ -52,6 +52,7 @@ macro_rules! generate_electoral_system_tuple_impls { }; use crate::{ + SharedDataHash, CorruptStorageError, electoral_system::{ ElectoralSystem, @@ -72,7 +73,7 @@ macro_rules! generate_electoral_system_tuple_impls { }, ElectionIdentifier, }; - use crate::vote_storage::composite::$module::{CompositeVoteProperties, CompositeVote, CompositePartialVote}; + use crate::vote_storage::composite::$module::{CompositeVoteProperties, CompositeVote, CompositePartialVote, CompositeSharedData}; use frame_support::{Parameter, pallet_prelude::{Member, MaybeSerializeDeserialize}}; @@ -440,6 +441,12 @@ macro_rules! generate_electoral_system_tuple_impls { _ => Err(CorruptStorageError::new()) } } + + // 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/monotonic_change.rs b/state-chain/pallets/cf-elections/src/electoral_systems/monotonic_change.rs index 7ab63e57b1..df77fe680b 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 @@ -115,7 +115,7 @@ impl< partial_vote: &::PartialVote, ) -> Result { let (_, previous_value, previous_slot) = election_access.properties()?; - Ok(partial_vote.value != SharedDataHash::of(&previous_value) && + Ok(partial_vote.value != election_access.shared_data_hash_of(previous_value) && partial_vote.block > previous_slot) } fn generate_vote_properties( diff --git a/state-chain/pallets/cf-elections/src/lib.rs b/state-chain/pallets/cf-elections/src/lib.rs index 12e8900a04..5b3c425ce9 100644 --- a/state-chain/pallets/cf-elections/src/lib.rs +++ b/state-chain/pallets/cf-elections/src/lib.rs @@ -637,6 +637,12 @@ 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, From 685ca04f849536deb5bea13580e45140a592ce07 Mon Sep 17 00:00:00 2001 From: kylezs Date: Tue, 29 Oct 2024 15:46:28 +0100 Subject: [PATCH 2/6] fix: remove is_vote_valid --- .../cf-elections/src/electoral_system.rs | 29 +---------- .../src/electoral_systems/composite.rs | 24 +--------- .../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 | 27 ++++++----- .../tests/monotonic_change.rs | 48 +++++++------------ state-chain/pallets/cf-elections/src/lib.rs | 19 -------- 8 files changed, 32 insertions(+), 140 deletions(-) diff --git a/state-chain/pallets/cf-elections/src/electoral_system.rs b/state-chain/pallets/cf-elections/src/electoral_system.rs index 6d510b1ac8..06583ffca3 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 8dd6f90862..49c830e30e 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 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 df77fe680b..7aea8cc48c 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 5e7ec14eb3..2a830b472f 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 5b3c425ce9..2f497525bf 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, From 4cd5a1ef50b97387d06730d2319e0ab2601125e0 Mon Sep 17 00:00:00 2001 From: kylezs Date: Tue, 29 Oct 2024 16:34:13 +0100 Subject: [PATCH 3/6] test: fix tests --- .../src/electoral_systems/monotonic_change.rs | 8 ++-- .../tests/monotonic_change.rs | 38 ++++++++++++------- 2 files changed, 27 insertions(+), 19 deletions(-) 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 7aea8cc48c..23fded403f 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 @@ -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 @@ -159,16 +159,14 @@ impl< 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().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)) - .or_insert(vec![vote.block]); + counts.entry(vote.value).or_insert_with(Vec::new).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 2a830b472f..7726247a77 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 @@ -76,17 +76,19 @@ 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 { ConsensusVotes { + // we start from 1 so don't get a Default::default() conflict on vote validity. votes: (0..correct_voters) .enumerate() .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: (), }) @@ -135,37 +137,45 @@ 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 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), - }) + TestSetup::::default() + .with_initial_election_state((), ((), 1, 1), ()) + .build_with_initial_election() .expect_consensus( generate_votes_with_different_slots( 0, - MonotonicChangeVote { value: 0, block: 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, - // neither the value has changed nor the block, both of which fail validity checks - MonotonicChangeVote { value: 11, block: 13 }, + // value is valid but block is invalid + MonotonicChangeVote { value: 2, block: 0 }, ), None, ); From 8aaeccac9a40f88c68628ef442f4025eb0b8773c Mon Sep 17 00:00:00 2001 From: kylezs Date: Wed, 30 Oct 2024 09:49:30 +0100 Subject: [PATCH 4/6] chore: remove stale comment --- .../cf-elections/src/electoral_systems/tests/monotonic_change.rs | 1 - 1 file changed, 1 deletion(-) 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 7726247a77..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 @@ -81,7 +81,6 @@ fn generate_votes_with_different_slots( incorrect_value: MonotonicChangeVote, ) -> ConsensusVotes { ConsensusVotes { - // we start from 1 so don't get a Default::default() conflict on vote validity. votes: (0..correct_voters) .enumerate() .map(|(index, _)| ConsensusVote { From 7b6bce56f1f35e35c37df9c5268f0cfb64816f2c Mon Sep 17 00:00:00 2001 From: kylezs Date: Wed, 30 Oct 2024 09:53:00 +0100 Subject: [PATCH 5/6] fix: only compare value, not slot number --- .../cf-elections/src/electoral_systems/monotonic_change.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 23fded403f..a0c97230eb 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 @@ -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. From d0727cfcb10055c95c01faa9c11277d13000d15b Mon Sep 17 00:00:00 2001 From: kylezs Date: Wed, 30 Oct 2024 11:39:18 +0100 Subject: [PATCH 6/6] chore: clippy --- .../cf-elections/src/electoral_systems/monotonic_change.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 a0c97230eb..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 @@ -167,7 +167,7 @@ impl< monotonic_change_vote.block > previous_block && previous_value != monotonic_change_vote.value }) { - counts.entry(vote.value).or_insert_with(Vec::new).push(vote.block); + counts.entry(vote.value).or_default().push(vote.block); } counts.iter().find_map(|(vote, blocks_height)| {