From d8655bb31fcbf5d9355811864d32e5e8ac71b0e2 Mon Sep 17 00:00:00 2001 From: kylezs Date: Fri, 18 Oct 2024 11:37:09 +0200 Subject: [PATCH 01/18] refactor: split out electoral system runner trait --- engine/src/elections.rs | 21 +- engine/src/elections/voter_api.rs | 30 +- .../client/electoral_api.rs | 21 +- engine/src/witness/sol.rs | 4 +- .../cf-elections/src/electoral_system.rs | 59 +- .../src/electoral_system_runner.rs | 309 ++++++++++ .../blockchain/delta_based_ingress.rs | 32 +- .../src/electoral_systems/composite.rs | 288 +++------ .../src/electoral_systems/egress_success.rs | 14 +- .../src/electoral_systems/liveness.rs | 11 +- .../src/electoral_systems/monotonic_median.rs | 28 +- .../src/electoral_systems/unsafe_median.rs | 12 +- state-chain/pallets/cf-elections/src/lib.rs | 572 ++++++++---------- state-chain/pallets/cf-elections/src/mock.rs | 2 +- state-chain/pallets/cf-elections/src/tests.rs | 4 +- .../runtime/src/chainflip/solana_elections.rs | 266 ++++---- state-chain/runtime/src/lib.rs | 2 +- 17 files changed, 884 insertions(+), 791 deletions(-) create mode 100644 state-chain/pallets/cf-elections/src/electoral_system_runner.rs diff --git a/engine/src/elections.rs b/engine/src/elections.rs index 650696ada3..9da45256d4 100644 --- a/engine/src/elections.rs +++ b/engine/src/elections.rs @@ -13,9 +13,9 @@ use cf_primitives::MILLISECONDS_PER_BLOCK; use cf_utilities::{future_map::FutureMap, task_scope::Scope, UnendingStream}; use futures::{stream, StreamExt, TryStreamExt}; use pallet_cf_elections::{ - electoral_system::{ElectionIdentifierOf, ElectoralSystem}, vote_storage::{AuthorityVote, VoteStorage}, - SharedDataHash, MAXIMUM_VOTES_PER_EXTRINSIC, + CompositeElectionIdentifierOf, ElectoralSystemRunner, SharedDataHash, + MAXIMUM_VOTES_PER_EXTRINSIC, }; use rand::Rng; use std::{ @@ -23,7 +23,8 @@ use std::{ sync::Arc, }; use tracing::{error, info, warn}; -use voter_api::VoterApi; +use utilities::{future_map::FutureMap, task_scope::Scope, UnendingStream}; +use voter_api::CompositeVoterApi; const MAXIMUM_CONCURRENT_FILTER_REQUESTS: usize = 16; const LIFETIME_OF_SHARED_DATA_IN_CACHE: std::time::Duration = std::time::Duration::from_secs(90); @@ -34,7 +35,7 @@ const INITIAL_VOTER_REQUEST_TIMEOUT: std::time::Duration = std::time::Duration:: pub struct Voter< Instance: 'static, StateChainClient: ElectoralApi + SignedExtrinsicApi + ChainApi, - VoterClient: VoterApi<>::ElectoralSystem> + Send + Sync + 'static, + VoterClient: CompositeVoterApi<>::ElectoralSystemRunner> + Send + Sync + 'static, > where state_chain_runtime::Runtime: pallet_cf_elections::Config, @@ -47,7 +48,7 @@ pub struct Voter< impl< Instance: Send + Sync + 'static, StateChainClient: ElectoralApi + SignedExtrinsicApi + ChainApi, - VoterClient: VoterApi<>::ElectoralSystem> + Clone + Send + Sync + 'static, + VoterClient: CompositeVoterApi<>::ElectoralSystemRunner> + Clone + Send + Sync + 'static, > Voter where state_chain_runtime::Runtime: @@ -111,17 +112,17 @@ where std::time::Duration::from_millis(MILLISECONDS_PER_BLOCK); let mut submit_interval = tokio::time::interval(BLOCK_TIME); let mut pending_submissions = BTreeMap::< - ElectionIdentifierOf<>::ElectoralSystem>, + CompositeElectionIdentifierOf<>::ElectoralSystemRunner>, ( - <<>::ElectoralSystem as ElectoralSystem>::Vote as VoteStorage>::PartialVote, - <<>::ElectoralSystem as ElectoralSystem>::Vote as VoteStorage>::Vote, + <<>::ElectoralSystemRunner as ElectoralSystemRunner>::Vote as VoteStorage>::PartialVote, + <<>::ElectoralSystemRunner as ElectoralSystemRunner>::Vote as VoteStorage>::Vote, ) >::default(); let mut vote_tasks = FutureMap::default(); let mut shared_data_cache = HashMap::< SharedDataHash, ( - <<>::ElectoralSystem as ElectoralSystem>::Vote as VoteStorage>::SharedData, + <<>::ElectoralSystemRunner as ElectoralSystemRunner>::Vote as VoteStorage>::SharedData, std::time::Instant, ) >::default(); @@ -163,7 +164,7 @@ where Ok(vote) => { info!("Voting task for election: '{:?}' succeeded.", election_identifier); // Create the partial_vote early so that SharedData can be provided as soon as the vote has been generated, rather than only after it is submitted. - let partial_vote = <<>::ElectoralSystem as ElectoralSystem>::Vote as VoteStorage>::vote_into_partial_vote(&vote, |shared_data| { + let partial_vote = <<>::ElectoralSystemRunner as ElectoralSystemRunner>::Vote as VoteStorage>::vote_into_partial_vote(&vote, |shared_data| { let shared_data_hash = SharedDataHash::of(&shared_data); if shared_data_cache.len() > MAXIMUM_SHARED_DATA_CACHE_ITEMS { for shared_data_hash in shared_data_cache.keys().cloned().take(shared_data_cache.len() - MAXIMUM_SHARED_DATA_CACHE_ITEMS).collect::>() { diff --git a/engine/src/elections/voter_api.rs b/engine/src/elections/voter_api.rs index c38a15ada5..0b0dd50a4a 100644 --- a/engine/src/elections/voter_api.rs +++ b/engine/src/elections/voter_api.rs @@ -4,8 +4,10 @@ use frame_support::{ }; use pallet_cf_elections::{ electoral_system::ElectoralSystem, - electoral_systems::composite::{self, Composite}, + electoral_system_runner::RunnerStorageAccessTrait, + electoral_systems::composite::{self, CompositeRunner}, vote_storage::{self, VoteStorage}, + ElectoralSystemRunner, }; #[async_trait::async_trait] @@ -17,33 +19,43 @@ pub trait VoterApi { ) -> Result<<::Vote as VoteStorage>::Vote, anyhow::Error>; } -pub struct CompositeVoter { +pub struct CompositeVoter { voters: Voters, - _phantom: core::marker::PhantomData, + _phantom: core::marker::PhantomData, } -impl Clone for CompositeVoter { +impl Clone for CompositeVoter { fn clone(&self) -> Self { Self { voters: self.voters.clone(), _phantom: Default::default() } } } -impl CompositeVoter { +impl CompositeVoter { pub fn new(voters: Voters) -> Self { Self { voters, _phantom: Default::default() } } } +#[async_trait::async_trait] +pub trait CompositeVoterApi { + async fn vote( + &self, + settings: ::ElectoralSettings, + properties: ::ElectionProperties, + ) -> Result<<::Vote as VoteStorage>::Vote, anyhow::Error>; +} + +// TODO Combine this into the composite macro PRO-1736 macro_rules! generate_voter_api_tuple_impls { ($module:ident: ($(($electoral_system:ident, $voter:ident)),*$(,)?)) => { #[allow(non_snake_case)] #[async_trait::async_trait] - impl<$($voter: VoterApi<$electoral_system> + Send + Sync),*, $($electoral_system : ElectoralSystem + Send + Sync + 'static),*, ValidatorId: MaybeSerializeDeserialize + Member + Parameter, Hooks: Send + Sync + 'static + composite::$module::Hooks<$($electoral_system,)*>> VoterApi> for CompositeVoter, ($($voter,)*)> { + impl<$($voter: VoterApi<$electoral_system> + Send + Sync),*, $($electoral_system : ElectoralSystem + Send + Sync + 'static),*, ValidatorId: MaybeSerializeDeserialize + Member + Parameter, StorageAccess: RunnerStorageAccessTrait> + Send + Sync + 'static, Hooks: Send + Sync + 'static + composite::$module::Hooks<$($electoral_system,)*>> CompositeVoterApi> for CompositeVoter, ($($voter,)*)> { async fn vote( &self, - settings: as ElectoralSystem>::ElectoralSettings, - properties: as ElectoralSystem>::ElectionProperties, + settings: as ElectoralSystemRunner>::ElectoralSettings, + properties: as ElectoralSystemRunner>::ElectionProperties, ) -> Result< - < as ElectoralSystem>::Vote as VoteStorage>::Vote, + < as ElectoralSystemRunner>::Vote as VoteStorage>::Vote, anyhow::Error, > { use vote_storage::composite::$module::CompositeVote; diff --git a/engine/src/state_chain_observer/client/electoral_api.rs b/engine/src/state_chain_observer/client/electoral_api.rs index 3f50d9ea92..61460b7766 100644 --- a/engine/src/state_chain_observer/client/electoral_api.rs +++ b/engine/src/state_chain_observer/client/electoral_api.rs @@ -4,9 +4,8 @@ use crate::state_chain_observer::client::{ }; use codec::{Decode, Encode}; use pallet_cf_elections::{ - electoral_system::{ElectionIdentifierOf, ElectoralSystem}, - vote_storage::VoteStorage, - ElectoralDataFor, + electoral_system_runner::CompositeElectionIdentifierOf, vote_storage::VoteStorage, + ElectoralDataFor, ElectoralSystemRunner, }; use state_chain_runtime::SolanaInstance; use std::collections::{BTreeMap, BTreeSet}; @@ -29,10 +28,10 @@ where fn filter_votes( &self, proposed_votes: BTreeMap< - ElectionIdentifierOf<>::ElectoralSystem>, - <<>::ElectoralSystem as ElectoralSystem>::Vote as VoteStorage>::Vote, + CompositeElectionIdentifierOf<>::ElectoralSystemRunner>, + <<>::ElectoralSystemRunner as ElectoralSystemRunner>::Vote as VoteStorage>::Vote, >, - ) -> impl std::future::Future>::ElectoralSystem>>> + Send + 'static; + ) -> impl std::future::Future>::ElectoralSystemRunner>>> + Send + 'static; } impl< @@ -68,10 +67,10 @@ impl< fn filter_votes( &self, proposed_votes: BTreeMap< - ElectionIdentifierOf<>::ElectoralSystem>, - <<>::ElectoralSystem as ElectoralSystem>::Vote as VoteStorage>::Vote, + CompositeElectionIdentifierOf<>::ElectoralSystemRunner>, + <<>::ElectoralSystemRunner as ElectoralSystemRunner>::Vote as VoteStorage>::Vote, >, - ) -> impl std::future::Future>::ElectoralSystem>>> + Send + 'static{ + ) -> impl std::future::Future>::ElectoralSystemRunner>>> + Send + 'static{ let base_rpc_client = self.base_rpc_client.clone(); let account_id = self.signed_extrinsic_client.account_id(); async move { @@ -82,10 +81,10 @@ impl< .map_err(anyhow::Error::from) .and_then(|electoral_data| { >::ElectoralSystem, + >>::ElectoralSystemRunner, >, > as Decode>::decode(&mut &electoral_data[..]) .map_err(Into::into) diff --git a/engine/src/witness/sol.rs b/engine/src/witness/sol.rs index 407e373051..8449bf96bf 100644 --- a/engine/src/witness/sol.rs +++ b/engine/src/witness/sol.rs @@ -21,7 +21,7 @@ use futures::FutureExt; use pallet_cf_elections::{electoral_system::ElectoralSystem, vote_storage::VoteStorage}; use state_chain_runtime::{ chainflip::solana_elections::{ - SolanaBlockHeightTracking, SolanaEgressWitnessing, SolanaElectoralSystem, + SolanaBlockHeightTracking, SolanaEgressWitnessing, SolanaElectoralSystemRunner, SolanaFeeTracking, SolanaIngressTracking, SolanaLiveness, SolanaNonceTracking, TransactionSuccessDetails, }, @@ -195,7 +195,7 @@ where crate::elections::Voter::new( scope, state_chain_client, - CompositeVoter::::new(( + CompositeVoter::::new(( SolanaBlockHeightTrackingVoter { client: client.clone() }, SolanaFeeTrackingVoter { client: client.clone() }, SolanaIngressTrackingVoter { client: client.clone() }, diff --git a/state-chain/pallets/cf-elections/src/electoral_system.rs b/state-chain/pallets/cf-elections/src/electoral_system.rs index 06583ffca3..93f88a6178 100644 --- a/state-chain/pallets/cf-elections/src/electoral_system.rs +++ b/state-chain/pallets/cf-elections/src/electoral_system.rs @@ -118,7 +118,6 @@ pub trait ElectoralSystem: 'static + Sized { /// to vote in a given Election. Validators are expected to call this indirectly via RPC once /// per state-chain block, for each active election. fn is_vote_desired>( - _election_identifier_with_extra: ElectionIdentifierOf, _election_access: &ElectionAccess, current_vote: Option<(VotePropertiesOf, AuthorityVoteOf)>, ) -> Result { @@ -155,8 +154,7 @@ pub trait ElectoralSystem: 'static + Sized { /// This is called during the pallet's `on_finalize` callback, if elections aren't paused and /// the CorruptStorage error hasn't occurred. - fn on_finalize>( - electoral_access: &mut ElectoralAccess, + fn on_finalize + 'static>( election_identifiers: Vec>, context: &Self::OnFinalizeContext, ) -> Result; @@ -168,10 +166,8 @@ pub trait ElectoralSystem: 'static + Sized { /// 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. - #[allow(clippy::type_complexity)] fn check_consensus>( - election_identifier: ElectionIdentifierOf, - electoral_access: &ElectionAccess, + election_access: &ElectionAccess, // This is the consensus as of the last time the consensus was checked. Note this is *NOT* // the "last" consensus, i.e. this can be `None` even if on some previous check we had // consensus, but it was subsequently lost. @@ -291,10 +287,7 @@ mod access { &self, ) -> Result<::ElectionState, CorruptStorageError>; - #[cfg(test)] - fn election_identifier( - &self, - ) -> Result, CorruptStorageError>; + fn election_identifier(&self) -> ElectionIdentifierOf; } /// A trait allowing write access to the details about a single election @@ -304,10 +297,10 @@ mod access { /// to ensure that in situations where `check_consensus` depends on the `state` that we will /// correctly recalculate the consensus if needed. fn set_state( - &mut self, + &self, state: ::ElectionState, ) -> Result<(), CorruptStorageError>; - fn clear_votes(&mut self); + fn clear_votes(&self); fn delete(self); /// This will change the `ElectionIdentifierExtra` value of the election, and allows you to /// optionally change the properties. Note the `extra` must be strictly greater than the @@ -322,7 +315,7 @@ mod access { /// will be invalidated by this. fn refresh( &mut self, - extra: ::ElectionIdentifierExtra, + new_extra: ::ElectionIdentifierExtra, properties: ::ElectionProperties, ) -> Result<(), CorruptStorageError>; @@ -330,7 +323,7 @@ mod access { /// votes/state. This also returns information about the difference in the consensus between /// the last call to `check_consensus`. fn check_consensus( - &mut self, + &self, ) -> Result< ConsensusStatus<::Consensus>, CorruptStorageError, @@ -340,28 +333,18 @@ mod access { /// A trait allowing read access to the details about the electoral system and its elections pub trait ElectoralReadAccess { type ElectoralSystem: ElectoralSystem; - type ElectionReadAccess<'a>: ElectionReadAccess - where - Self: 'a; + type ElectionReadAccess: ElectionReadAccess; - fn election( - &self, - id: ElectionIdentifierOf, - ) -> Result, CorruptStorageError>; - fn unsynchronised_settings( - &self, - ) -> Result< + fn election(id: ElectionIdentifierOf) -> Self::ElectionReadAccess; + fn unsynchronised_settings() -> Result< ::ElectoralUnsynchronisedSettings, CorruptStorageError, >; - fn unsynchronised_state( - &self, - ) -> Result< + fn unsynchronised_state() -> Result< ::ElectoralUnsynchronisedState, CorruptStorageError, >; fn unsynchronised_state_map( - &self, key: &::ElectoralUnsynchronisedStateMapKey, ) -> Result< Option< @@ -373,28 +356,22 @@ mod access { /// A trait allowing write access to the details about the electoral system and its elections pub trait ElectoralWriteAccess: ElectoralReadAccess { - type ElectionWriteAccess<'a>: ElectionWriteAccess - where - Self: 'a; + type ElectionWriteAccess: ElectionWriteAccess; fn new_election( - &mut self, extra: ::ElectionIdentifierExtra, properties: ::ElectionProperties, state: ::ElectionState, - ) -> Result, CorruptStorageError>; + ) -> Result; fn election_mut( - &mut self, id: ElectionIdentifierOf, - ) -> Result, CorruptStorageError>; + ) -> Self::ElectionWriteAccess; fn set_unsynchronised_state( - &mut self, unsynchronised_state: ::ElectoralUnsynchronisedState, ) -> Result<(), CorruptStorageError>; /// Inserts or removes a value from the unsynchronised state map of the electoral system. fn set_unsynchronised_state_map( - &mut self, key: ::ElectoralUnsynchronisedStateMapKey, value: Option< ::ElectoralUnsynchronisedStateMapValue, @@ -408,16 +385,14 @@ mod access { fn mutate_unsynchronised_state< T, F: for<'a> FnOnce( - &mut Self, &'a mut ::ElectoralUnsynchronisedState, ) -> Result, >( - &mut self, f: F, ) -> Result { - let mut unsynchronised_state = self.unsynchronised_state()?; - let t = f(self, &mut unsynchronised_state)?; - self.set_unsynchronised_state(unsynchronised_state)?; + let mut unsynchronised_state = Self::unsynchronised_state()?; + let t = f(&mut unsynchronised_state)?; + Self::set_unsynchronised_state(unsynchronised_state)?; Ok(t) } } diff --git a/state-chain/pallets/cf-elections/src/electoral_system_runner.rs b/state-chain/pallets/cf-elections/src/electoral_system_runner.rs new file mode 100644 index 0000000000..9f0e2b1410 --- /dev/null +++ b/state-chain/pallets/cf-elections/src/electoral_system_runner.rs @@ -0,0 +1,309 @@ +use frame_support::{ + pallet_prelude::{MaybeSerializeDeserialize, Member}, + Parameter, +}; +use sp_std::vec::Vec; + +use crate::{ + vote_storage::{AuthorityVote, VoteStorage}, + CorruptStorageError, ElectionIdentifier, +}; + +use crate::electoral_system::ConsensusStatus; + +#[allow(type_alias_bounds)] +pub type CompositeElectionIdentifierOf = + ElectionIdentifier<::ElectionIdentifierExtra>; + +#[allow(type_alias_bounds)] +pub type AuthorityVoteOf = AuthorityVote< + <::Vote as VoteStorage>::PartialVote, + <::Vote as VoteStorage>::Vote, +>; +#[allow(type_alias_bounds)] +pub type IndividualComponentOf = + <::Vote as VoteStorage>::IndividualComponent; +#[allow(type_alias_bounds)] +pub type BitmapComponentOf = + <::Vote as VoteStorage>::BitmapComponent; +#[allow(type_alias_bounds)] +pub type VotePropertiesOf = + <::Vote as VoteStorage>::Properties; + +pub struct CompositeConsensusVote { + // If the validator hasn't voted, they will get a None. + pub vote: Option<(VotePropertiesOf, ::Vote)>, + pub validator_id: ES::ValidatorId, +} + +pub struct CompositeConsensusVotes { + pub votes: Vec>, +} + +pub trait ElectoralSystemRunner: 'static + Sized { + type ValidatorId: Parameter + Member + MaybeSerializeDeserialize; + + /// This is intended for storing any internal state of the ElectoralSystem. It is not + /// synchronised and therefore should only be used by the ElectoralSystem, and not be consumed + /// by the engine. + /// + /// Also note that if this state is changed that will not cause election's consensus to be + /// retested. + type ElectoralUnsynchronisedState: Parameter + Member + MaybeSerializeDeserialize; + /// This is intended for storing any internal state of the ElectoralSystem. It is not + /// synchronised and therefore should only be used by the ElectoralSystem, and not be consumed + /// by the engine. + /// + /// Also note that if this state is changed that will not cause election's consensus to be + /// retested. + type ElectoralUnsynchronisedStateMapKey: Parameter + Member; + /// This is intended for storing any internal state of the ElectoralSystem. It is not + /// synchronised and therefore should only be used by the ElectoralSystem, and not be consumed + /// by the engine. + /// + /// Also note that if this state is changed that will not cause election's consensus to be + /// retested. + type ElectoralUnsynchronisedStateMapValue: Parameter + Member; + + /// Settings of the electoral system. These can be changed at any time by governance, and + /// are not synchronised with elections, and therefore there is not a universal mapping from + /// elections to these settings values. Therefore it should only be used for internal + /// state, i.e. the engines should not consume this data. + /// + /// Also note that if these settings are changed that will not cause election's consensus to be + /// retested. + type ElectoralUnsynchronisedSettings: Parameter + Member + MaybeSerializeDeserialize; + + /// Settings of the electoral system. These settings are synchronised with + /// elections, so all engines will have a consistent view of the electoral settings to use for a + /// given election. + type ElectoralSettings: Parameter + Member + MaybeSerializeDeserialize + Eq; + + /// Extra data stored along with the UniqueMonotonicIdentifier as part of the + /// ElectionIdentifier. This is used by composite electoral systems to identify which variant of + /// election it is working with, without needing to reading in further election + /// state/properties/etc. + type ElectionIdentifierExtra: Parameter + Member + Copy + Eq + Ord; + + /// The properties of a single election, for example this could describe which block of the + /// external chain the election is associated with and what needs to be witnessed. + type ElectionProperties: Parameter + Member; + + /// Per-election state needed by the ElectoralSystem. This state is not synchronised across + /// engines, and may change during the lifetime of a election. + type ElectionState: Parameter + Member; + + /// A description of the validator's view of the election's topic. For example a list of all + /// ingresses the validator has observed in the block the election is about. + type Vote: VoteStorage; + + /// This is the information that results from consensus. Typically this will be the same as the + /// `Vote` type, but with more complex consensus models the result of an election may not be + /// sensibly represented in the same form as a single vote. + type Consensus: Parameter + Member + Eq; + + /// This is not used by the pallet, but is used to tell a validator that it should attempt + /// to vote in a given Election. Validators are expected to call this indirectly via RPC once + /// per state-chain block, for each active election. + fn is_vote_desired( + _election_identifier_with_extra: CompositeElectionIdentifierOf, + current_vote: Option<(VotePropertiesOf, AuthorityVoteOf)>, + ) -> Result { + Ok(current_vote.is_none()) + } + + /// This is not used by the pallet, but is used to tell a validator if they should submit vote. + /// This is a way to decrease the amount of extrinsics a validator needs to send. + fn is_vote_needed( + _current_vote: ( + VotePropertiesOf, + ::PartialVote, + AuthorityVoteOf, + ), + _proposed_vote: ( + ::PartialVote, + ::Vote, + ), + ) -> bool { + 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: CompositeElectionIdentifierOf, + _partial_vote: &::PartialVote, + ) -> Result { + Ok(true) + } + + /// This is called every time a vote occurs. It associates the vote with a `Properties` + /// value. + /// + /// 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 generate_vote_properties( + election_identifier: CompositeElectionIdentifierOf, + previous_vote: Option<(VotePropertiesOf, AuthorityVoteOf)>, + vote: &::PartialVote, + ) -> Result, CorruptStorageError>; + + /// This is called during the pallet's `on_finalize` callback, if elections aren't paused and + /// the CorruptStorage error hasn't occurred. + fn on_finalize( + election_identifiers: Vec>, + ) -> Result<(), CorruptStorageError>; + + /// This function determines if the votes we have received form a consensus. It is called as + /// part of the Election pallet's `on_finalize` callback when the Election's votes or state have + /// changed since the previous call. + /// + /// 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. + #[allow(clippy::type_complexity)] + fn check_consensus( + election_identifier: CompositeElectionIdentifierOf, + // This is the consensus as of the last time the consensus was checked. Note this is *NOT* + // the "last" consensus, i.e. this can be `None` even if on some previous check we had + // consensus, but it was subsequently lost. + previous_consensus: Option<&Self::Consensus>, + votes: CompositeConsensusVotes, + ) -> Result, CorruptStorageError>; +} + +use crate::UniqueMonotonicIdentifier; + +/// A trait allowing access to a storage layer for electoral sytem runners. +// TODO: rename +pub trait RunnerStorageAccessTrait { + type ElectoralSystemRunner: ElectoralSystemRunner; + + fn electoral_settings_for_election( + unique_monotonic_identifier: UniqueMonotonicIdentifier, + ) -> Result< + ::ElectoralSettings, + CorruptStorageError, + >; + fn election_properties( + election_identifier: CompositeElectionIdentifierOf, + ) -> Result< + ::ElectionProperties, + CorruptStorageError, + >; + fn election_state( + unique_monotonic_identifier: UniqueMonotonicIdentifier, + ) -> Result< + ::ElectionState, + CorruptStorageError, + >; + + /// Sets a new `state` value for the election. This will invalid the current Consensus, and + /// thereby force it to be recalculated, when `check_consensus` is next called. We do this + /// to ensure that in situations where `check_consensus` depends on the `state` that we will + /// correctly recalculate the consensus if needed. + fn set_election_state( + unique_monotonic_identifier: UniqueMonotonicIdentifier, + state: ::ElectionState, + ) -> Result<(), CorruptStorageError>; + + // Clear the votes of a particular election + fn clear_election_votes(unique_monotonic_identifier: UniqueMonotonicIdentifier); + + fn delete_election( + composite_election_identifier: CompositeElectionIdentifierOf, + ); + /// This will change the `ElectionIdentifierExtra` value of the election, and allows you to + /// optionally change the properties. Note the `extra` must be strictly greater than the + /// previous value of this election, this function will return `Err` if it is not. This + /// ensures that all `Self::ElectoralSystemRunner::ElectionIdentifierExtra` ever used by a + /// particular election are unique. The purpose of this function to in effect allow the + /// deletion and recreation of an election so you can change its `Properties`, while + /// efficiently transferring the existing election's votes to the new election. The only + /// difference is that here the elections `Settings` will not be updated to the latest. This + /// could create a problem if you never delete elections, as old `Settings` values will be + /// stored until any elections referencing them are deleted. Any in-flight authority votes + /// will be invalidated by this. + fn refresh_election( + election_identifier: CompositeElectionIdentifierOf, + new_extra: ::ElectionIdentifierExtra, + properties: ::ElectionProperties, + ) -> Result<(), CorruptStorageError>; + + /// This returns the current consensus which will always be up to date with the latest + /// votes/state. This also returns information about the difference in the consensus between + /// the last call to `check_consensus`. + fn check_election_consensus( + election_identifier: CompositeElectionIdentifierOf, + ) -> Result< + ConsensusStatus<::Consensus>, + CorruptStorageError, + >; + + fn unsynchronised_settings() -> Result< + ::ElectoralUnsynchronisedSettings, + CorruptStorageError, + >; + fn unsynchronised_state() -> Result< + ::ElectoralUnsynchronisedState, + CorruptStorageError, + >; + fn unsynchronised_state_map( + key: &::ElectoralUnsynchronisedStateMapKey, + ) -> Result< + Option< + ::ElectoralUnsynchronisedStateMapValue, + >, + CorruptStorageError, + >; + + fn new_election( + extra: ::ElectionIdentifierExtra, + properties: ::ElectionProperties, + state: ::ElectionState, + ) -> Result, CorruptStorageError>; + + fn set_unsynchronised_state( + unsynchronised_state: ::ElectoralUnsynchronisedState, + ) -> Result<(), CorruptStorageError>; + + /// Inserts or removes a value from the unsynchronised state map of the electoral system. + fn set_unsynchronised_state_map( + key: ::ElectoralUnsynchronisedStateMapKey, + value: Option< + ::ElectoralUnsynchronisedStateMapValue, + >, + ) -> Result<(), CorruptStorageError>; + + /// Allows you to mutate the unsynchronised state. This is more efficient than a read + /// (`unsynchronised_state`) and then a write (`set_unsynchronised_state`) in the case of + /// composite ElectoralSystems, as a write from one of the sub-ElectoralSystems internally + /// will require an additional read. Therefore this function should be preferred. + fn mutate_unsynchronised_state< + T, + F: for<'a> FnOnce( + &Self, + &'a ::ElectoralUnsynchronisedState, + ) -> Result, + >( + &self, + f: F, + ) -> Result { + let mut unsynchronised_state = Self::unsynchronised_state()?; + let t = f(self, &mut unsynchronised_state)?; + Self::set_unsynchronised_state(unsynchronised_state)?; + Ok(t) + } +} diff --git a/state-chain/pallets/cf-elections/src/electoral_systems/blockchain/delta_based_ingress.rs b/state-chain/pallets/cf-elections/src/electoral_systems/blockchain/delta_based_ingress.rs index d92516010c..e67170885e 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/blockchain/delta_based_ingress.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/blockchain/delta_based_ingress.rs @@ -55,23 +55,22 @@ where ::Account: Ord, ValidatorId: Member + Parameter + Ord + MaybeSerializeDeserialize, { - pub fn open_channel>( + pub fn open_channel + 'static>( election_identifiers: Vec< ElectionIdentifier<::ElectionIdentifierExtra>, >, - electoral_access: &mut ElectoralAccess, channel: Sink::Account, asset: Sink::Asset, close_block: Sink::BlockNumber, ) -> Result<(), CorruptStorageError> { let channel_details = ( OpenChannelDetails { asset, close_block }, - electoral_access.unsynchronised_state_map(&(channel.clone(), asset))?.unwrap_or( + ElectoralAccess::unsynchronised_state_map(&(channel.clone(), asset))?.unwrap_or( ChannelTotalIngressed { block_number: Zero::zero(), amount: Zero::zero() }, ), ); if let Some(election_identifier) = election_identifiers.last() { - let mut election_access = electoral_access.election_mut(*election_identifier)?; + let mut election_access = ElectoralAccess::election_mut(*election_identifier); let mut channels = election_access.properties()?; if channels.len() < MAXIMUM_CHANNELS_PER_ELECTION as usize { channels.insert(channel, channel_details); @@ -86,7 +85,7 @@ where } } - electoral_access.new_election( + ElectoralAccess::new_election( Default::default(), /* We use the lowest value, so we can refresh the elections the * maximum number of times */ [(channel, channel_details)].into_iter().collect(), @@ -139,7 +138,6 @@ where type OnFinalizeReturn = (); fn is_vote_desired>( - _election_identifier_with_extra: ElectionIdentifier, _election_access: &ElectionAccess, _current_vote: Option<(VotePropertiesOf, AuthorityVoteOf)>, ) -> Result { @@ -168,14 +166,13 @@ where Ok(()) } - fn on_finalize>( - electoral_access: &mut ElectoralAccess, + fn on_finalize + 'static>( election_identifiers: Vec>, chain_tracking: &Self::OnFinalizeContext, ) -> Result { for election_identifier in election_identifiers { let (mut channels, mut pending_ingress_totals, option_consensus) = { - let mut election_access = electoral_access.election_mut(election_identifier)?; + let election_access = ElectoralAccess::election_mut(election_identifier); ( election_access.properties()?, election_access.state()?, @@ -220,11 +217,13 @@ where }; if let Some(ingress_total) = option_ingress_total_before_chain_tracking { - let previous_amount = electoral_access - .unsynchronised_state_map(&(account.clone(), details.asset))? - .map_or(Zero::zero(), |previous_total_ingressed| { - previous_total_ingressed.amount - }); + let previous_amount = ElectoralAccess::unsynchronised_state_map(&( + account.clone(), + details.asset, + ))? + .map_or(Zero::zero(), |previous_total_ingressed| { + previous_total_ingressed.amount + }); match previous_amount.cmp(&ingress_total.amount) { Ordering::Less => { Sink::on_ingress( @@ -234,7 +233,7 @@ where ingress_total.block_number, (), ); - electoral_access.set_unsynchronised_state_map( + ElectoralAccess::set_unsynchronised_state_map( (account.clone(), details.asset), Some(ingress_total), )?; @@ -261,7 +260,7 @@ where } } - let mut election_access = electoral_access.election_mut(election_identifier)?; + let mut election_access = ElectoralAccess::election_mut(election_identifier); if !closed_channels.is_empty() { for closed_channel in closed_channels { pending_ingress_totals.remove(&closed_channel); @@ -291,7 +290,6 @@ where } fn check_consensus>( - _election_identifier: ElectionIdentifier, election_access: &ElectionAccess, _previous_consensus: Option<&Self::Consensus>, consensus_votes: ConsensusVotes, 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 49c830e30e..e49e6066b6 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/composite.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/composite.rs @@ -1,31 +1,12 @@ -use crate::electoral_system::{ElectoralSystem, ElectoralWriteAccess}; - /// Allows the composition of multiple ElectoralSystems while allowing the ability to configure the /// `on_finalize` behaviour without exposing the internal composite types. -pub struct Composite> { - _phantom: core::marker::PhantomData<(T, ValidatorId, H)>, -} - -pub struct DefaultHooks { - _phantom: core::marker::PhantomData, -} - -pub trait Translator { - type ElectoralSystem: ElectoralSystem; - type ElectionAccess<'a>: ElectoralWriteAccess - where - Self: 'a, - GenericElectoralAccess: 'a; - - fn translate_electoral_access<'a>( - &'a self, - generic_electoral_access: &'a mut GenericElectoralAccess, - ) -> Self::ElectionAccess<'a>; +pub struct CompositeRunner { + _phantom: core::marker::PhantomData<(T, ValidatorId, StorageAccess, H)>, } /// The access wrappers need to impl the access traits once for each variant, /// these tags ensure these trait impls don't overlap. -mod tags { +pub mod tags { pub struct A; pub struct B; pub struct C; @@ -45,9 +26,7 @@ macro_rules! generate_electoral_system_tuple_impls { #[allow(unused_variables)] pub mod $module { use super::{ - Translator, - Composite, - DefaultHooks, + CompositeRunner, tags, }; @@ -55,17 +34,17 @@ macro_rules! generate_electoral_system_tuple_impls { CorruptStorageError, electoral_system::{ ElectoralSystem, - ConsensusStatus, ConsensusVote, ElectionReadAccess, ElectionWriteAccess, ElectoralReadAccess, ElectoralWriteAccess, - AuthorityVoteOf, - VotePropertiesOf, - ElectionIdentifierOf, ConsensusVotes, + ElectionIdentifierOf, + ConsensusStatus, }, + electoral_system_runner::{ElectoralSystemRunner, AuthorityVoteOf, RunnerStorageAccessTrait, + VotePropertiesOf, CompositeConsensusVotes, CompositeElectionIdentifierOf, CompositeConsensusVote}, vote_storage::{ AuthorityVote, VoteStorage @@ -78,44 +57,13 @@ macro_rules! generate_electoral_system_tuple_impls { use codec::{Encode, Decode}; use scale_info::TypeInfo; - use core::borrow::Borrow; use sp_std::vec::Vec; - /// This trait specifies the behaviour of the composite's `ElectoralSystem::on_finalize` without that code being exposed to the internals of the composite by using the Translator trait to obtain ElectoralAccess objects that abstract those details. + /// This trait specifies the behaviour of the composite's `ElectoralSystem::on_finalize` function. pub trait Hooks<$($electoral_system: ElectoralSystem,)*> { - /// The `OnFinalizeContext` of the composite's ElectoralSystem implementation. - type OnFinalizeContext; - - /// The 'OnFinalizeReturn' of the composite's ElectoralSystem implementation. - type OnFinalizeReturn; - - fn on_finalize),*>( - generic_electoral_access: &mut GenericElectoralAccess, - electoral_access_translators: ($($electoral_system_alt_name_0,)*), + fn on_finalize( election_identifiers: ($(Vec>,)*), - context: &Self::OnFinalizeContext, - ) -> Result; - } - - impl,)*> Hooks<$($electoral_system,)*> for DefaultHooks { - type OnFinalizeContext = OnFinalizeContext; - type OnFinalizeReturn = (); - - fn on_finalize),*>( - generic_electoral_access: &mut GenericElectoralAccess, - electoral_access_translators: ($($electoral_system_alt_name_0,)*), - election_identifiers: ($(Vec>,)*), - context: &Self::OnFinalizeContext, - ) -> Result { - let ($($electoral_system,)*) = electoral_access_translators; - let ($($electoral_system_alt_name_0,)*) = election_identifiers; - - $( - $electoral_system::on_finalize(&mut $electoral_system.translate_electoral_access(generic_electoral_access), $electoral_system_alt_name_0, &context)?; - )* - - Ok(()) - } + ) -> Result<(), CorruptStorageError>; } #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Encode, Decode, TypeInfo)] @@ -143,13 +91,13 @@ macro_rules! generate_electoral_system_tuple_impls { $($electoral_system($electoral_system),)* } - impl<$($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static> Composite<($($electoral_system,)*), ValidatorId, H> { + impl<$($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, StorageAccess: RunnerStorageAccessTrait + 'static, H: Hooks<$($electoral_system),*> + 'static> CompositeRunner<($($electoral_system,)*), ValidatorId, StorageAccess, H> { pub fn with_identifiers FnOnce( ($( Vec>, )*) ) -> R>( - election_identifiers: Vec>, + election_identifiers: Vec>, f: F, ) -> R { $(let mut $electoral_system_alt_name_0 = Vec::new();)* @@ -166,21 +114,9 @@ macro_rules! generate_electoral_system_tuple_impls { $($electoral_system_alt_name_0,)* )) } - - pub fn with_access_translators FnOnce( - ($( - ElectoralAccessTranslator, - )*) - ) -> R>( - f: F, - ) -> R { - f(( - $(ElectoralAccessTranslator::::new(),)* - )) - } } - impl<$($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static> ElectoralSystem for Composite<($($electoral_system,)*), ValidatorId, H> { + impl<$($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, StorageAccess: RunnerStorageAccessTrait + 'static, H: Hooks<$($electoral_system),*> + 'static> ElectoralSystemRunner for CompositeRunner<($($electoral_system,)*), ValidatorId, StorageAccess, H> { type ValidatorId = ValidatorId; type ElectoralUnsynchronisedState = ($(<$electoral_system as ElectoralSystem>::ElectoralUnsynchronisedState,)*); type ElectoralUnsynchronisedStateMapKey = CompositeElectoralUnsynchronisedStateMapKey<$(<$electoral_system as ElectoralSystem>::ElectoralUnsynchronisedStateMapKey,)*>; @@ -194,12 +130,8 @@ macro_rules! generate_electoral_system_tuple_impls { type Vote = ($(<$electoral_system as ElectoralSystem>::Vote,)*); type Consensus = CompositeConsensus<$(<$electoral_system as ElectoralSystem>::Consensus,)*>; - type OnFinalizeContext = H::OnFinalizeContext; - type OnFinalizeReturn = H::OnFinalizeReturn; - - fn is_vote_desired>( + fn is_vote_desired( election_identifier: ElectionIdentifier, - election_access: &ElectionAccess, current_vote: Option<( VotePropertiesOf, AuthorityVoteOf, @@ -208,8 +140,7 @@ macro_rules! generate_electoral_system_tuple_impls { match *election_identifier.extra() { $(CompositeElectionIdentifierExtra::$electoral_system(extra) => { <$electoral_system as ElectoralSystem>::is_vote_desired( - election_identifier.with_extra(extra), - &CompositeElectionAccess::::new(election_access), + &CompositeElectionAccess::::new(election_identifier.with_extra(extra)), current_vote.map(|(properties, vote)| { Ok(( match properties { @@ -295,34 +226,25 @@ macro_rules! generate_electoral_system_tuple_impls { } } - fn on_finalize>( - electoral_access: &mut ElectoralAccess, + fn on_finalize( election_identifiers: Vec>, - context: &Self::OnFinalizeContext, - ) -> Result { - Self::with_access_translators(|access_translators| { - Self::with_identifiers(election_identifiers, |election_identifiers| { - H::on_finalize( - electoral_access, - access_translators, - election_identifiers, - context - ) - }) + ) -> Result<(), CorruptStorageError> { + Self::with_identifiers(election_identifiers, |election_identifiers| { + H::on_finalize( + election_identifiers, + ) }) } - fn check_consensus>( + fn check_consensus( election_identifier: ElectionIdentifier, - election_access: &ElectionAccess, previous_consensus: Option<&Self::Consensus>, - consensus_votes: ConsensusVotes, + consensus_votes: CompositeConsensusVotes, ) -> Result, CorruptStorageError> { Ok(match *election_identifier.extra() { $(CompositeElectionIdentifierExtra::$electoral_system(extra) => { <$electoral_system as ElectoralSystem>::check_consensus( - election_identifier.with_extra(extra), - &CompositeElectionAccess::::new(election_access), + &CompositeElectionAccess::::new(election_identifier.with_extra(extra)), previous_consensus.map(|previous_consensus| { match previous_consensus { CompositeConsensus::$electoral_system(previous_consensus) => Ok(previous_consensus), @@ -330,7 +252,7 @@ macro_rules! generate_electoral_system_tuple_impls { } }).transpose()?, ConsensusVotes { - votes: consensus_votes.votes.into_iter().map(|ConsensusVote { vote, validator_id }| { + votes: consensus_votes.votes.into_iter().map(|CompositeConsensusVote { vote, validator_id }| { if let Some((properties, vote)) = vote { match (properties, vote) { ( @@ -357,41 +279,22 @@ macro_rules! generate_electoral_system_tuple_impls { } } - pub struct CompositeElectionAccess { - ea: BorrowEA, - _phantom: core::marker::PhantomData<(Tag, EA)>, - } - impl,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*>, BorrowEA: Borrow, EA: ElectionReadAccess>> CompositeElectionAccess { - fn new(ea: BorrowEA) -> Self { - Self { - ea, - _phantom: Default::default(), - } - } - } - pub struct CompositeElectoralAccess<'a, Tag, EA> { - ea: &'a mut EA, - _phantom: core::marker::PhantomData, - } - impl<'a, Tag, EA> CompositeElectoralAccess<'a, Tag, EA> { - fn new(ea: &'a mut EA) -> Self { - Self { - ea, - _phantom: Default::default(), - } - } + pub struct CompositeElectionAccess { + id: ElectionIdentifierOf, + _phantom: core::marker::PhantomData<(Tag, ES, StorageAccess)>, } - pub struct ElectoralAccessTranslator { - _phantom: core::marker::PhantomData<(Tag, EA)>, - } - impl ElectoralAccessTranslator { - fn new() -> Self { + impl CompositeElectionAccess { + fn new(id: ElectionIdentifierOf) -> Self { Self { + id, _phantom: Default::default(), } } } + pub struct CompositeElectoralAccess { + _phantom: core::marker::PhantomData<(Tag, ES, StorageAccess)>, + } // This macro solves the problem of taking a repeating argument and generating the // product of the arguments elements. As we need to be able to refer to every element @@ -401,15 +304,16 @@ macro_rules! generate_electoral_system_tuple_impls { }; (@ $($previous:ident,)*;: $($electoral_system:ident,)*) => {}; (@ $($previous:ident,)*; $current:ident, $($remaining:ident,)*: $($electoral_system:ident,)*) => { - impl<$($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static, BorrowEA: Borrow, EA: ElectionReadAccess>> ElectionReadAccess for CompositeElectionAccess { + + impl<'a, $($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static, StorageAccess: RunnerStorageAccessTrait> + 'static> ElectionReadAccess for CompositeElectionAccess { type ElectoralSystem = $current; fn settings(&self) -> Result<$current::ElectoralSettings, CorruptStorageError> { - let ($($previous,)* settings, $($remaining,)*) =self.ea.borrow().settings()?; + let ($($previous,)* settings, $($remaining,)*) = StorageAccess::electoral_settings_for_election(*self.id.unique_monotonic())?; Ok(settings) } fn properties(&self) -> Result<$current::ElectionProperties, CorruptStorageError> { - match self.ea.borrow().properties()? { + match StorageAccess::election_properties(self.id.with_extra(CompositeElectionIdentifierExtra::$current(*self.id.extra())))? { CompositeElectionProperties::$current(properties) => { Ok(properties) }, @@ -417,7 +321,7 @@ macro_rules! generate_electoral_system_tuple_impls { } } fn state(&self) -> Result<$current::ElectionState, CorruptStorageError> { - match self.ea.borrow().state()? { + match StorageAccess::election_state(*self.id.unique_monotonic())? { CompositeElectionState::$current(state) => { Ok(state) }, @@ -425,81 +329,71 @@ macro_rules! generate_electoral_system_tuple_impls { } } - #[cfg(test)] - fn election_identifier(&self) -> Result, CorruptStorageError> { - let composite_identifier = self.ea.borrow().election_identifier()?; - let extra = match composite_identifier.extra() { - CompositeElectionIdentifierExtra::$current(extra) => Ok(extra), - _ => Err(CorruptStorageError::new()), - }?; - Ok(composite_identifier.with_extra(*extra)) + fn election_identifier(&self) -> ElectionIdentifierOf { + self.id } } - impl<$($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static, EA: ElectionWriteAccess>> ElectionWriteAccess for CompositeElectionAccess { - fn set_state(&mut self, state: $current::ElectionState) -> Result<(), CorruptStorageError> { - self.ea.set_state(CompositeElectionState::$current(state)) + + impl<$($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static, StorageAccess: RunnerStorageAccessTrait> + 'static> ElectionWriteAccess for CompositeElectionAccess { + fn set_state(&self, state: $current::ElectionState) -> Result<(), CorruptStorageError> { + StorageAccess::set_election_state(*self.id.unique_monotonic(), CompositeElectionState::$current(state)) } - fn clear_votes(&mut self) { - self.ea.clear_votes() + fn clear_votes(&self) { + StorageAccess::clear_election_votes(*self.id.unique_monotonic()); } fn delete(self) { - self.ea.delete(); + StorageAccess::delete_election(self.id.with_extra(CompositeElectionIdentifierExtra::$current(*self.id.extra()))); } fn refresh( &mut self, - extra: $current::ElectionIdentifierExtra, + new_extra: $current::ElectionIdentifierExtra, properties: $current::ElectionProperties, ) -> Result<(), CorruptStorageError> { - self.ea.refresh( - CompositeElectionIdentifierExtra::$current(extra), + StorageAccess::refresh_election( + self.id.with_extra(CompositeElectionIdentifierExtra::$current(*self.id.extra())), + CompositeElectionIdentifierExtra::$current(new_extra), CompositeElectionProperties::$current(properties), - ) + )?; + self.id = self.id.with_extra(new_extra); + Ok(()) } fn check_consensus( - &mut self, + &self, ) -> Result, CorruptStorageError> { - self.ea.check_consensus().and_then(|consensus_status| { + StorageAccess::check_election_consensus(self.id.with_extra(CompositeElectionIdentifierExtra::$current(*self.id.extra()))).and_then(|consensus_status| { consensus_status.try_map(|consensus| { match consensus { CompositeConsensus::$current(consensus) => Ok(consensus), _ => Err(CorruptStorageError::new()), } }) - }) } } - impl<'a, $($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static, EA: ElectoralReadAccess>> ElectoralReadAccess for CompositeElectoralAccess<'a, tags::$current, EA> { + + impl<$($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static, StorageAccess: RunnerStorageAccessTrait> + 'static> ElectoralReadAccess for CompositeElectoralAccess { type ElectoralSystem = $current; - type ElectionReadAccess<'b> = CompositeElectionAccess::ElectionReadAccess<'b>, ::ElectionReadAccess<'b>> - where - Self: 'b; + type ElectionReadAccess = CompositeElectionAccess; fn election( - &self, id: ElectionIdentifier<<$current as ElectoralSystem>::ElectionIdentifierExtra>, - ) -> Result, CorruptStorageError> { - self.ea.election(id.with_extra(CompositeElectionIdentifierExtra::$current(*id.extra()))).map(|election_access| { - CompositeElectionAccess::::ElectionReadAccess<'_>>::new(election_access) - }) + ) -> Self::ElectionReadAccess { + CompositeElectionAccess::::new(id) } fn unsynchronised_settings( - &self, ) -> Result<$current::ElectoralUnsynchronisedSettings, CorruptStorageError> { - let ($($previous,)* unsynchronised_settings, $($remaining,)*) = self.ea.unsynchronised_settings()?; + let ($($previous,)* unsynchronised_settings, $($remaining,)*) = StorageAccess::unsynchronised_settings()?; Ok(unsynchronised_settings) } fn unsynchronised_state( - &self, ) -> Result<$current::ElectoralUnsynchronisedState, CorruptStorageError> { - let ($($previous,)* unsynchronised_state, $($remaining,)*) = self.ea.unsynchronised_state()?; + let ($($previous,)* unsynchronised_state, $($remaining,)*) = StorageAccess::unsynchronised_state()?; Ok(unsynchronised_state) } fn unsynchronised_state_map( - &self, key: &$current::ElectoralUnsynchronisedStateMapKey, ) -> Result, CorruptStorageError> { - match self.ea.unsynchronised_state_map(&CompositeElectoralUnsynchronisedStateMapKey::$current(key.clone()))? { + match StorageAccess::unsynchronised_state_map(&CompositeElectoralUnsynchronisedStateMapKey::$current(key.clone()))? { Some(CompositeElectoralUnsynchronisedStateMapValue::$current(value)) => Ok(Some(value)), None => Ok(None), _ => Err(CorruptStorageError::new()), @@ -507,43 +401,36 @@ macro_rules! generate_electoral_system_tuple_impls { } } - impl<'a, $($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static, EA: ElectoralWriteAccess>> ElectoralWriteAccess for CompositeElectoralAccess<'a, tags::$current, EA> { - type ElectionWriteAccess<'b> = CompositeElectionAccess::ElectionWriteAccess<'b>, ::ElectionWriteAccess<'b>> - where - Self: 'b; + impl<'a, $($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static, StorageAccess: RunnerStorageAccessTrait> + 'static> ElectoralWriteAccess for CompositeElectoralAccess { + type ElectionWriteAccess = CompositeElectionAccess; fn new_election( - &mut self, extra: $current::ElectionIdentifierExtra, properties: $current::ElectionProperties, state: $current::ElectionState, - ) -> Result, CorruptStorageError> { - self.ea.new_election(CompositeElectionIdentifierExtra::$current(extra), CompositeElectionProperties::$current(properties), CompositeElectionState::$current(state)).map(|election_access| { - CompositeElectionAccess::new(election_access) - }) + ) -> Result { + let election_identifier = StorageAccess::new_election(CompositeElectionIdentifierExtra::$current(extra), CompositeElectionProperties::$current(properties), CompositeElectionState::$current(state))?; + Ok(Self::ElectionWriteAccess::new(election_identifier.with_extra(extra))) } + fn election_mut( - &mut self, id: ElectionIdentifier<$current::ElectionIdentifierExtra>, - ) -> Result, CorruptStorageError> { - self.ea.election_mut(id.with_extra(CompositeElectionIdentifierExtra::$current(*id.extra()))).map(|election_access| { - CompositeElectionAccess::new(election_access) - }) + ) -> Self::ElectionWriteAccess { + Self::ElectionWriteAccess::new(id) } + fn set_unsynchronised_state( - &mut self, unsynchronised_state: $current::ElectoralUnsynchronisedState, ) -> Result<(), CorruptStorageError> { - let ($($previous,)* _, $($remaining,)*) = self.ea.unsynchronised_state()?; - self.ea.set_unsynchronised_state(($($previous,)* unsynchronised_state, $($remaining,)*)) + let ($($previous,)* _, $($remaining,)*) = StorageAccess::unsynchronised_state()?; + StorageAccess::set_unsynchronised_state(($($previous,)* unsynchronised_state, $($remaining,)*)) } fn set_unsynchronised_state_map( - &mut self, key: $current::ElectoralUnsynchronisedStateMapKey, value: Option<$current::ElectoralUnsynchronisedStateMapValue>, ) -> Result<(), CorruptStorageError> { - self.ea.set_unsynchronised_state_map( + StorageAccess::set_unsynchronised_state_map( CompositeElectoralUnsynchronisedStateMapKey::$current(key), value.map(CompositeElectoralUnsynchronisedStateMapValue::$current), ) @@ -552,31 +439,18 @@ macro_rules! generate_electoral_system_tuple_impls { fn mutate_unsynchronised_state< T, F: for<'b> FnOnce( - &mut Self, &'b mut $current::ElectoralUnsynchronisedState, ) -> Result, >( - &mut self, f: F, ) -> Result { - let ($($previous,)* mut unsynchronised_state, $($remaining,)*) = self.ea.unsynchronised_state()?; - let t = f(self, &mut unsynchronised_state)?; - self.ea.set_unsynchronised_state(($($previous,)* unsynchronised_state, $($remaining,)*))?; + let ($($previous,)* mut unsynchronised_state, $($remaining,)*) = StorageAccess::unsynchronised_state()?; + let t = f( &mut unsynchronised_state)?; + StorageAccess::set_unsynchronised_state(($($previous,)* unsynchronised_state, $($remaining,)*))?; Ok(t) } } - impl<$($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static, EA: ElectoralWriteAccess>> Translator for ElectoralAccessTranslator> { - type ElectoralSystem = $current; - type ElectionAccess<'a> = CompositeElectoralAccess<'a, tags::$current, EA> - where - Self: 'a, EA: 'a; - - fn translate_electoral_access<'a>(&'a self, generic_electoral_access: &'a mut EA) -> Self::ElectionAccess<'a> { - Self::ElectionAccess::<'a>::new(generic_electoral_access) - } - } - generate_electoral_system_tuple_impls!(@ $($previous,)* $current,; $($remaining,)*: $($electoral_system,)*); }; } diff --git a/state-chain/pallets/cf-elections/src/electoral_systems/egress_success.rs b/state-chain/pallets/cf-elections/src/electoral_systems/egress_success.rs index bd8f07989d..4e212c4756 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/egress_success.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/egress_success.rs @@ -31,11 +31,12 @@ impl< ValidatorId: Member + Parameter + Ord + MaybeSerializeDeserialize, > EgressSuccess { - pub fn watch_for_egress>( - electoral_access: &mut ElectoralAccess, + pub fn watch_for_egress< + ElectoralAccess: ElectoralWriteAccess + 'static, + >( identifier: Identifier, ) -> Result<(), CorruptStorageError> { - electoral_access.new_election((), identifier, ())?; + ElectoralAccess::new_election((), identifier, ())?; Ok(()) } } @@ -72,20 +73,18 @@ impl< } fn is_vote_desired>( - _election_identifier: ElectionIdentifierOf, _election_access: &ElectionAccess, _current_vote: Option<(VotePropertiesOf, AuthorityVoteOf)>, ) -> Result { Ok(true) } - fn on_finalize>( - electoral_access: &mut ElectoralAccess, + fn on_finalize + 'static>( election_identifiers: Vec>, _context: &Self::OnFinalizeContext, ) -> Result { for election_identifier in election_identifiers { - let mut election_access = electoral_access.election_mut(election_identifier)?; + let election_access = ElectoralAccess::election_mut(election_identifier); if let Some(egress_data) = election_access.check_consensus()?.has_consensus() { let identifier = election_access.properties()?; election_access.delete(); @@ -97,7 +96,6 @@ impl< } fn check_consensus>( - _election_identifier: ElectionIdentifierOf, _election_access: &ElectionAccess, _previous_consensus: Option<&Self::Consensus>, consensus_votes: ConsensusVotes, diff --git a/state-chain/pallets/cf-elections/src/electoral_systems/liveness.rs b/state-chain/pallets/cf-elections/src/electoral_systems/liveness.rs index 120c0ec808..7d71b0d690 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/liveness.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/liveness.rs @@ -76,15 +76,13 @@ impl< } fn is_vote_desired>( - _election_identifier: ElectionIdentifierOf, _election_access: &ElectionAccess, _current_vote: Option<(VotePropertiesOf, AuthorityVoteOf)>, ) -> Result { Ok(true) } - fn on_finalize>( - electoral_access: &mut ElectoralAccess, + fn on_finalize + 'static>( election_identifiers: Vec>, (current_sc_block, current_chain_tracking_number): &Self::OnFinalizeContext, ) -> Result { @@ -108,7 +106,7 @@ impl< .at_most_one() .map_err(|_| CorruptStorageError::new())? { - let mut election_access = electoral_access.election_mut(election_identifier)?; + let election_access = ElectoralAccess::election_mut(election_identifier); // Is the block the election started at + the duration we want the check to stay open // for less than or equal to the current SC block? @@ -121,14 +119,14 @@ impl< } } election_access.delete(); - electoral_access.new_election( + ElectoralAccess::new_election( (), block_number_to_check(*current_chain_tracking_number), *current_sc_block, )?; } } else { - electoral_access.new_election( + ElectoralAccess::new_election( (), block_number_to_check(*current_chain_tracking_number), *current_sc_block, @@ -139,7 +137,6 @@ impl< } fn check_consensus>( - _election_identifier: ElectionIdentifierOf, _election_access: &ElectionAccess, _previous_consensus: Option<&Self::Consensus>, consensus_votes: ConsensusVotes, diff --git a/state-chain/pallets/cf-elections/src/electoral_systems/monotonic_median.rs b/state-chain/pallets/cf-elections/src/electoral_systems/monotonic_median.rs index 9802e7dbba..f674caf880 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/monotonic_median.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/monotonic_median.rs @@ -63,8 +63,7 @@ impl< Ok(()) } - fn on_finalize>( - electoral_access: &mut ElectoralAccess, + fn on_finalize + 'static>( election_identifiers: Vec>, _context: &Self::OnFinalizeContext, ) -> Result { @@ -73,30 +72,27 @@ impl< .at_most_one() .map_err(|_| CorruptStorageError::new())? { - let mut election_access = electoral_access.election_mut(election_identifier)?; + let election_access = ElectoralAccess::election_mut(election_identifier); if let Some(consensus) = election_access.check_consensus()?.has_consensus() { election_access.delete(); - electoral_access.new_election((), (), ())?; - electoral_access.mutate_unsynchronised_state( - |_electoral_access, unsynchronised_state| { - if consensus > *unsynchronised_state { - *unsynchronised_state = consensus.clone(); - Hook::on_change(consensus); - } + ElectoralAccess::new_election((), (), ())?; + ElectoralAccess::mutate_unsynchronised_state(|unsynchronised_state| { + if consensus > *unsynchronised_state { + *unsynchronised_state = consensus.clone(); + Hook::on_change(consensus); + } - Ok(()) - }, - )?; + Ok(()) + })?; } } else { - electoral_access.new_election((), (), ())?; + ElectoralAccess::new_election((), (), ())?; } - electoral_access.unsynchronised_state() + ElectoralAccess::unsynchronised_state() } fn check_consensus>( - _election_identifier: ElectionIdentifier, _election_access: &ElectionAccess, _previous_consensus: Option<&Self::Consensus>, consensus_votes: ConsensusVotes, diff --git a/state-chain/pallets/cf-elections/src/electoral_systems/unsafe_median.rs b/state-chain/pallets/cf-elections/src/electoral_systems/unsafe_median.rs index 87fbd6356d..b362cd820d 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/unsafe_median.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/unsafe_median.rs @@ -58,8 +58,7 @@ impl< Ok(()) } - fn on_finalize>( - electoral_access: &mut ElectoralAccess, + fn on_finalize + 'static>( election_identifiers: Vec>, _context: &Self::OnFinalizeContext, ) -> Result { @@ -68,21 +67,20 @@ impl< .at_most_one() .map_err(|_| CorruptStorageError::new())? { - let mut election_access = electoral_access.election_mut(election_identifier)?; + let election_access = ElectoralAccess::election_mut(election_identifier); if let Some(consensus) = election_access.check_consensus()?.has_consensus() { election_access.delete(); - electoral_access.set_unsynchronised_state(consensus)?; - electoral_access.new_election((), (), ())?; + ElectoralAccess::set_unsynchronised_state(consensus)?; + ElectoralAccess::new_election((), (), ())?; } } else { - electoral_access.new_election((), (), ())?; + ElectoralAccess::new_election((), (), ())?; } Ok(()) } fn check_consensus>( - _election_identifier: ElectionIdentifier, _election_access: &ElectionAccess, _previous_consensus: Option<&Self::Consensus>, votes: ConsensusVotes, diff --git a/state-chain/pallets/cf-elections/src/lib.rs b/state-chain/pallets/cf-elections/src/lib.rs index a294226fff..a714ed09f8 100644 --- a/state-chain/pallets/cf-elections/src/lib.rs +++ b/state-chain/pallets/cf-elections/src/lib.rs @@ -108,6 +108,7 @@ #![doc = include_str!("../../cf-doc-head.md")] pub mod electoral_system; +pub mod electoral_system_runner; pub mod electoral_systems; mod mock; mod tests; @@ -136,14 +137,18 @@ pub mod pallet { use cf_primitives::{AuthorityCount, EpochIndex}; use cf_traits::{AccountRoleRegistry, Chainflip, EpochInfo}; - use crate::electoral_system::{ConsensusVote, ConsensusVotes}; - use access_impls::{ElectionAccess, ElectoralAccess}; + use crate::electoral_system::ConsensusStatus; + pub use access_impls::RunnerStorageAccess; + + use crate::electoral_system_runner::{ + CompositeConsensusVote, CompositeConsensusVotes, RunnerStorageAccessTrait, + }; use bitmap_components::ElectionBitmapComponents; - use electoral_system::{ - AuthorityVoteOf, ConsensusStatus, ElectionIdentifierOf, ElectionReadAccess, - ElectionWriteAccess, ElectoralReadAccess, ElectoralSystem, ElectoralWriteAccess, + pub use electoral_system_runner::{ + AuthorityVoteOf, CompositeElectionIdentifierOf, ElectoralSystemRunner, IndividualComponentOf, VotePropertiesOf, }; + use frame_support::{ sp_runtime::traits::BlockNumberProvider, storage::bounded_btree_map::BoundedBTreeMap, Deserialize, Serialize, StorageDoubleMap as _, @@ -184,10 +189,10 @@ pub mod pallet { /// when to vote. #[allow(type_alias_bounds)] pub type ElectoralDataFor, I: 'static> = ElectoralData< - ElectionIdentifierOf, - ::ElectoralSettings, - ::ElectionProperties, - AuthorityVoteOf, + CompositeElectionIdentifierOf, + ::ElectoralSettings, + ::ElectionProperties, + AuthorityVoteOf, BlockNumberFor, >; @@ -315,9 +320,9 @@ pub mod pallet { #[allow(type_alias_bounds)] pub type InitialStateOf, I: 'static> = InitialState< - ::ElectoralUnsynchronisedState, - ::ElectoralUnsynchronisedSettings, - ::ElectoralSettings, + ::ElectoralUnsynchronisedState, + ::ElectoralUnsynchronisedSettings, + ::ElectoralSettings, >; #[pallet::genesis_config] @@ -354,8 +359,7 @@ pub mod pallet { type RuntimeEvent: From> + IsType<::RuntimeEvent>; - type ElectoralSystem: ElectoralSystem< - OnFinalizeContext = (), + type ElectoralSystemRunner: ElectoralSystemRunner< ValidatorId = ::ValidatorId, >; @@ -441,7 +445,7 @@ pub mod pallet { _, Identity, SharedDataHash, - <::Vote as VoteStorage>::SharedData, + <::Vote as VoteStorage>::SharedData, OptionQuery, >; @@ -464,10 +468,15 @@ pub mod pallet { UniqueMonotonicIdentifier, Identity, T::ValidatorId, - (VotePropertiesOf, IndividualComponentOf), + ( + VotePropertiesOf, + IndividualComponentOf, + ), OptionQuery, >; + // TODO: rename this storage item to be specific to umi. + // election identifier is used elsewhere to mean umi + extra. /// Stores the next valid election identifier. #[pallet::storage] pub(crate) type NextElectionIdentifier, I: 'static = ()> = @@ -478,7 +487,7 @@ pub mod pallet { #[pallet::storage] pub(crate) type ElectoralUnsynchronisedSettings, I: 'static = ()> = StorageValue< _, - ::ElectoralUnsynchronisedSettings, + ::ElectoralUnsynchronisedSettings, OptionQuery, >; @@ -486,7 +495,7 @@ pub mod pallet { #[pallet::storage] pub(crate) type ElectoralUnsynchronisedState, I: 'static = ()> = StorageValue< _, - ::ElectoralUnsynchronisedState, + ::ElectoralUnsynchronisedState, OptionQuery, >; @@ -495,8 +504,8 @@ pub mod pallet { type ElectoralUnsynchronisedStateMap, I: 'static = ()> = StorageMap< _, Twox64Concat, - ::ElectoralUnsynchronisedStateMapKey, - ::ElectoralUnsynchronisedStateMapValue, + ::ElectoralUnsynchronisedStateMapKey, + ::ElectoralUnsynchronisedStateMapValue, OptionQuery, >; @@ -507,7 +516,7 @@ pub mod pallet { _, Twox64Concat, UniqueMonotonicIdentifier, - ::ElectoralSettings, + ::ElectoralSettings, OptionQuery, >; @@ -517,8 +526,8 @@ pub mod pallet { type ElectionProperties, I: 'static = ()> = StorageMap< _, Twox64Concat, - ElectionIdentifierOf, - ::ElectionProperties, + CompositeElectionIdentifierOf, + ::ElectionProperties, OptionQuery, >; @@ -528,19 +537,19 @@ pub mod pallet { _, Twox64Concat, UniqueMonotonicIdentifier, - ::ElectionState, + ::ElectionState, OptionQuery, >; /// Stores the most recent consensus, i.e. the most recent result of - /// `ElectoralSystem::check_consensus` that returned `Some(...)`, and whether it is `current` / - /// has not been `lost` since. + /// `ElectoralSystemRunner::check_consensus` that returned `Some(...)`, and whether it is + /// `current` / has not been `lost` since. #[pallet::storage] type ElectionConsensusHistory, I: 'static = ()> = StorageMap< _, Twox64Concat, UniqueMonotonicIdentifier, - ConsensusHistory<::Consensus>, + ConsensusHistory<::Consensus>, OptionQuery, >; @@ -581,30 +590,44 @@ pub mod pallet { // ---------------------------------------------------------------------------------------- // - pub mod access_impls { + pub(crate) mod access_impls { + use electoral_system_runner::RunnerStorageAccessTrait; + use super::*; - /// Implements traits to allow electoral systems to read/write an Election's details. - pub struct ElectionAccess, I: 'static> { - election_identifier: ElectionIdentifierOf, + // Provides access to the storage layer in a controlled, and consistent way. + pub struct RunnerStorageAccess, I: 'static> { _phantom: core::marker::PhantomData<(T, I)>, } - impl, I: 'static> ElectionAccess { - pub fn new(election_identifier: ElectionIdentifierOf) -> Self { - Self { election_identifier, _phantom: Default::default() } - } - pub(crate) fn unique_monotonic_identifier(&self) -> UniqueMonotonicIdentifier { - *self.election_identifier.unique_monotonic() + impl, I: 'static> RunnerStorageAccessTrait for RunnerStorageAccess { + type ElectoralSystemRunner = T::ElectoralSystemRunner; + + fn new_election( + extra: ::ElectionIdentifierExtra, + properties: ::ElectionProperties, + state: ::ElectionState, + ) -> Result< + CompositeElectionIdentifierOf, + CorruptStorageError, + > { + let unique_monotonic_identifier = NextElectionIdentifier::::get(); + let election_identifier = ElectionIdentifier(unique_monotonic_identifier, extra); + NextElectionIdentifier::::set(UniqueMonotonicIdentifier( + unique_monotonic_identifier + .0 + .checked_add(1) + .ok_or_else(CorruptStorageError::new)?, + )); + ElectionProperties::::insert(election_identifier, properties); + ElectionState::::insert(unique_monotonic_identifier, state); + Ok(election_identifier) } - } - impl, I: 'static> ElectionReadAccess for ElectionAccess { - type ElectoralSystem = T::ElectoralSystem; - fn settings( - &self, + fn electoral_settings_for_election( + unique_monotonic_identifier: UniqueMonotonicIdentifier, ) -> Result< - ::ElectoralSettings, + ::ElectoralSettings, CorruptStorageError, > { let mut settings_boundaries = @@ -613,93 +636,90 @@ pub mod pallet { let settings_boundary = settings_boundaries .iter() .rev() - .find(|settings_boundary| { - **settings_boundary <= self.unique_monotonic_identifier() - }) + .find(|settings_boundary| **settings_boundary <= unique_monotonic_identifier) .ok_or_else(CorruptStorageError::new)?; ElectoralSettings::::get(settings_boundary) .ok_or_else(CorruptStorageError::new) } - fn properties( - &self, + fn election_properties( + election_identifier: CompositeElectionIdentifierOf, ) -> Result< - ::ElectionProperties, + ::ElectionProperties, CorruptStorageError, > { - ElectionProperties::::get(self.election_identifier) + ElectionProperties::::get(election_identifier) .ok_or_else(CorruptStorageError::new) } - fn state( - &self, - ) -> Result<::ElectionState, CorruptStorageError> - { - ElectionState::::get(self.unique_monotonic_identifier()) + fn election_state( + unique_monotonic_identifier: UniqueMonotonicIdentifier, + ) -> Result< + ::ElectionState, + CorruptStorageError, + > { + ElectionState::::get(unique_monotonic_identifier) .ok_or_else(CorruptStorageError::new) } - #[cfg(test)] - fn election_identifier( - &self, - ) -> Result, CorruptStorageError> { - Ok(self.election_identifier) - } - } - impl, I: 'static> ElectionWriteAccess for ElectionAccess { - fn set_state( - &mut self, - state: ::ElectionState, + + fn set_election_state( + unique_monotonic_identifier: UniqueMonotonicIdentifier, + state: ::ElectionState, ) -> Result<(), CorruptStorageError> { - if self.state()? != state { - ElectionState::::insert(self.unique_monotonic_identifier(), state); - ElectionConsensusHistoryUpToDate::::remove( - self.unique_monotonic_identifier(), - ); + if Self::election_state(unique_monotonic_identifier)? != state { + ElectionState::::insert(unique_monotonic_identifier, state); + ElectionConsensusHistoryUpToDate::::remove(unique_monotonic_identifier); } Ok(()) } - fn clear_votes(&mut self) { - let unique_monotonic_identifier = self.unique_monotonic_identifier(); + fn clear_election_votes(unique_monotonic_identifier: UniqueMonotonicIdentifier) { ElectionBitmapComponents::::clear(unique_monotonic_identifier); for (_, (_, individual_component)) in IndividualComponents::::drain_prefix(unique_monotonic_identifier) { - <::Vote as VoteStorage>::visit_shared_data_references_in_individual_component(&individual_component, |shared_data_hash| { - Pallet::::remove_shared_data_reference(shared_data_hash, unique_monotonic_identifier); + <::Vote as + VoteStorage>::visit_shared_data_references_in_individual_component(& + individual_component, |shared_data_hash| { Pallet::::remove_shared_data_reference(shared_data_hash, unique_monotonic_identifier); }); } ElectionConsensusHistoryUpToDate::::remove(unique_monotonic_identifier); } - fn delete(mut self) { - self.clear_votes(); - ElectionProperties::::remove(self.election_identifier); - let unique_monotonic_identifier = self.unique_monotonic_identifier(); + fn delete_election( + composite_election_identifier: CompositeElectionIdentifierOf< + Self::ElectoralSystemRunner, + >, + ) { + let unique_monotonic_identifier = composite_election_identifier.unique_monotonic(); + Self::clear_election_votes(*unique_monotonic_identifier); + ElectionProperties::::remove(composite_election_identifier); ElectionState::::remove(unique_monotonic_identifier); ElectionConsensusHistory::::remove(unique_monotonic_identifier); } - fn refresh( - &mut self, - extra: ::ElectionIdentifierExtra, - properties: ::ElectionProperties, + + fn refresh_election( + election_identifier: CompositeElectionIdentifierOf, + new_extra: ::ElectionIdentifierExtra, + properties: ::ElectionProperties, ) -> Result<(), CorruptStorageError> { - if extra <= *self.election_identifier.extra() { + if new_extra <= *election_identifier.extra() { Err(CorruptStorageError::new()) } else { - ElectionProperties::::remove(self.election_identifier); - self.election_identifier = - ElectionIdentifier::new(self.unique_monotonic_identifier(), extra); - ElectionProperties::::insert(self.election_identifier, properties); + ElectionProperties::::remove(election_identifier); + let new_election_identifier = + ElectionIdentifier::new(*election_identifier.unique_monotonic(), new_extra); + ElectionProperties::::insert(new_election_identifier, properties); Ok(()) } } - fn check_consensus( - &mut self, + fn check_election_consensus( + election_identifier: CompositeElectionIdentifierOf, ) -> Result< - ConsensusStatus<::Consensus>, + ConsensusStatus<::Consensus>, CorruptStorageError, > { let epoch_index = T::EpochInfo::epoch_index(); - let unique_monotonic_identifier = self.unique_monotonic_identifier(); + let unique_monotonic_identifier = election_identifier.unique_monotonic(); let option_consensus_history = ElectionConsensusHistory::::get(unique_monotonic_identifier); Ok( @@ -720,7 +740,7 @@ pub mod pallet { let bitmap_components = ElectionBitmapComponents::::with( epoch_index, - unique_monotonic_identifier, + *unique_monotonic_identifier, |election_bitmap_components| { election_bitmap_components.get_all(¤t_authorities) }, @@ -729,45 +749,56 @@ pub mod pallet { IndividualComponents::::iter_prefix(unique_monotonic_identifier) .collect::>(); - let votes = current_authorities.into_iter().map(|validator_id| - ( - VoteComponents { - bitmap_component: bitmap_components.get(&validator_id).cloned(), - individual_component: individual_components.remove(&validator_id), - }, - validator_id, - ) - ).map(|(vote_components, validator_id)| { - if ContributingAuthorities::::contains_key(&validator_id) { - match <::Vote as VoteStorage>::components_into_authority_vote(vote_components, |shared_data_hash| { - // We don't bother to check if the reference has expired, as if we have the data we may as well use it, even if it was provided after the shared data reference expired (But before the reference was cleaned up `on_finalize`). + let votes = current_authorities + .into_iter() + .map(|validator_id| { + ( + VoteComponents { + bitmap_component: bitmap_components + .get(&validator_id) + .cloned(), + individual_component: individual_components + .remove(&validator_id), + }, + validator_id, + ) + }) + .map(|(vote_components, validator_id)| { + if ContributingAuthorities::::contains_key(&validator_id) { + match <::Vote as + VoteStorage>::components_into_authority_vote(vote_components, |shared_data_hash| + { + // We don't bother to check if the reference has expired, as if we have the + // data we may as well use it, even if it was provided after the shared data + // reference expired (But before the reference was cleaned up `on_finalize`). Ok(SharedData::::get(shared_data_hash)) }) { // Only a full vote can count towards consensus. - Ok(Some((properties, AuthorityVote::Vote(vote)))) => Ok(Some((properties, vote))), - Ok(Some((_properties, AuthorityVote::PartialVote(_)))) => Ok(None), + Ok(Some((properties, AuthorityVote::Vote(vote)))) => Ok(Some((properties, + vote))), Ok(Some((_properties, AuthorityVote::PartialVote(_)))) => Ok(None), Ok(None) => Ok(None), Err(e) => Err(e), } - } else { - Ok(None) - }.map(|props_and_vote| - ConsensusVote { - vote: props_and_vote, - validator_id + } else { + Ok(None) } - ) - }).collect::, _>>()?; + .map(|props_and_vote| CompositeConsensusVote { + vote: props_and_vote, + validator_id, + }) + }) + .collect::, _>>()?; debug_assert!(votes.len() == current_authorities_count as usize); // Remove individual components from non-authorities for (validator_id, (_, individual_component)) in individual_components { - <::Vote as VoteStorage>::visit_shared_data_references_in_individual_component( + <::Vote as + VoteStorage>::visit_shared_data_references_in_individual_component( &individual_component, |shared_data_hash| { - Pallet::::remove_shared_data_reference(shared_data_hash, unique_monotonic_identifier); - }, + Pallet::::remove_shared_data_reference(shared_data_hash, + *unique_monotonic_identifier); }, ); IndividualComponents::::remove( unique_monotonic_identifier, @@ -776,9 +807,8 @@ pub mod pallet { } let option_new_consensus = - ::check_consensus( - self.election_identifier, - &*self, /* Disallow recursive calls to this function */ + ::check_consensus( + election_identifier, option_consensus_history.as_ref().and_then(|consensus_history| { if consensus_history.lost_since { None @@ -786,7 +816,7 @@ pub mod pallet { Some(&consensus_history.most_recent) } }), - ConsensusVotes { votes }, + CompositeConsensusVotes { votes }, )?; ElectionConsensusHistory::::set( @@ -838,99 +868,44 @@ pub mod pallet { }, ) } - } - - /// Implements traits to allow election systems to read/write multiple Election's details. - pub struct ElectoralAccess, I: 'static> { - _phantom: core::marker::PhantomData<(T, I)>, - } - impl, I: 'static> ElectoralAccess { - pub(super) fn new() -> Self { - Self { _phantom: Default::default() } - } - } - impl, I: 'static> ElectoralReadAccess for ElectoralAccess { - type ElectoralSystem = T::ElectoralSystem; - type ElectionReadAccess<'a> = ElectionAccess; - - fn election( - &self, - id: ElectionIdentifierOf, - ) -> Result, CorruptStorageError> { - Ok(ElectionAccess::new(id)) - } fn unsynchronised_settings( - &self, ) -> Result< - ::ElectoralUnsynchronisedSettings, + ::ElectoralUnsynchronisedSettings, CorruptStorageError, - > { + >{ ElectoralUnsynchronisedSettings::::get().ok_or_else(CorruptStorageError::new) } - fn unsynchronised_state( - &self, - ) -> Result< - ::ElectoralUnsynchronisedState, + fn unsynchronised_state() -> Result< + ::ElectoralUnsynchronisedState, CorruptStorageError, > { ElectoralUnsynchronisedState::::get().ok_or_else(CorruptStorageError::new) } fn unsynchronised_state_map( - &self, - key: &::ElectoralUnsynchronisedStateMapKey, + key: &::ElectoralUnsynchronisedStateMapKey, ) -> Result< Option< - ::ElectoralUnsynchronisedStateMapValue, + ::ElectoralUnsynchronisedStateMapValue, >, CorruptStorageError, - > { + >{ Ok(ElectoralUnsynchronisedStateMap::::get(key)) } - } - impl, I: 'static> ElectoralWriteAccess for ElectoralAccess { - type ElectionWriteAccess<'a> = ElectionAccess; - - fn new_election( - &mut self, - extra: ::ElectionIdentifierExtra, - properties: ::ElectionProperties, - state: ::ElectionState, - ) -> Result, CorruptStorageError> { - let unique_monotonic_identifier = NextElectionIdentifier::::get(); - let election_identifier = ElectionIdentifier(unique_monotonic_identifier, extra); - NextElectionIdentifier::::set(UniqueMonotonicIdentifier( - unique_monotonic_identifier - .0 - .checked_add(1) - .ok_or_else(CorruptStorageError::new)?, - )); - ElectionProperties::::insert(election_identifier, properties); - ElectionState::::insert(unique_monotonic_identifier, state); - self.election_mut(election_identifier) - } - fn election_mut( - &mut self, - id: ElectionIdentifierOf, - ) -> Result, CorruptStorageError> { - Ok(ElectionAccess::new(id)) - } fn set_unsynchronised_state( - &mut self, - unsynchronised_state: ::ElectoralUnsynchronisedState, + unsynchronised_state: ::ElectoralUnsynchronisedState, ) -> Result<(), CorruptStorageError> { ElectoralUnsynchronisedState::::put(unsynchronised_state); Ok(()) } fn set_unsynchronised_state_map( - &mut self, - key: ::ElectoralUnsynchronisedStateMapKey, + key: ::ElectoralUnsynchronisedStateMapKey, value: Option< - ::ElectoralUnsynchronisedStateMapValue, + ::ElectoralUnsynchronisedStateMapValue, >, ) -> Result<(), CorruptStorageError> { ElectoralUnsynchronisedStateMap::::set(key, value); @@ -946,7 +921,7 @@ pub mod pallet { BitmapComponents, Config, CorruptStorageError, Pallet, UniqueMonotonicIdentifier, }; use crate::{ - electoral_system::{BitmapComponentOf, ElectoralSystem}, + electoral_system_runner::{BitmapComponentOf, ElectoralSystemRunner}, vote_storage::VoteStorage, }; use bitvec::prelude::*; @@ -962,7 +937,8 @@ pub mod pallet { pub(crate) struct ElectionBitmapComponents, I: 'static> { epoch: EpochIndex, #[allow(clippy::type_complexity)] - bitmaps: Vec<(BitmapComponentOf, BitVec)>, + bitmaps: + Vec<(BitmapComponentOf, BitVec)>, #[codec(skip)] _phantom: core::marker::PhantomData<(T, I)>, } @@ -1070,7 +1046,7 @@ pub mod pallet { this.bitmaps.retain(|(bitmap_component, bitmap)| { let retain = bitmap.any(); if !retain { - <::Vote as VoteStorage>::visit_shared_data_references_in_bitmap_component( + <::Vote as VoteStorage>::visit_shared_data_references_in_bitmap_component( bitmap_component, |shared_data_hash| { Pallet::::remove_shared_data_reference( @@ -1117,7 +1093,7 @@ pub mod pallet { pub(super) fn add( &mut self, authority_index: AuthorityCount, - bitmap_component: BitmapComponentOf, + bitmap_component: BitmapComponentOf, unique_monotonic_identifier: UniqueMonotonicIdentifier, block_number: BlockNumberFor, ) -> Result<(), CorruptStorageError> { @@ -1140,7 +1116,7 @@ pub mod pallet { true; bitmap })); - <::Vote as VoteStorage>::visit_shared_data_references_in_bitmap_component( + <::Vote as VoteStorage>::visit_shared_data_references_in_bitmap_component( &bitmap_component, |shared_data_hash| { Pallet::::add_shared_data_reference( @@ -1158,7 +1134,8 @@ pub mod pallet { pub(super) fn take( &mut self, authority_index: AuthorityCount, - ) -> Result>, CorruptStorageError> { + ) -> Result>, CorruptStorageError> + { let authority_index = authority_index as usize; Ok(self .bitmaps @@ -1175,7 +1152,8 @@ pub mod pallet { pub(super) fn get( &self, authority_index: AuthorityCount, - ) -> Result>, CorruptStorageError> { + ) -> Result>, CorruptStorageError> + { Ok(self .bitmaps .iter() @@ -1191,7 +1169,7 @@ pub mod pallet { &self, current_authorities: &[T::ValidatorId], ) -> Result< - BTreeMap>, + BTreeMap>, CorruptStorageError, > { self.debug_assert_authorities_in_order_of_indices(current_authorities); @@ -1211,7 +1189,7 @@ pub mod pallet { pub(super) fn clear(unique_monotonic_identifier: UniqueMonotonicIdentifier) { if let Some(this) = BitmapComponents::::get(unique_monotonic_identifier) { for (bitmap_component, _) in this.bitmaps { - <::Vote as VoteStorage>::visit_shared_data_references_in_bitmap_component( + <::Vote as VoteStorage>::visit_shared_data_references_in_bitmap_component( &bitmap_component, |shared_data_hash| { Pallet::::remove_shared_data_reference( @@ -1244,11 +1222,13 @@ pub mod pallet { pub fn vote( origin: OriginFor, authority_votes: BoundedBTreeMap< - ElectionIdentifierOf, - AuthorityVoteOf, + CompositeElectionIdentifierOf, + AuthorityVoteOf, ConstU32, >, ) -> DispatchResult { + Self::ensure_initialized()?; + let (epoch_index, authority, authority_index) = Self::ensure_can_vote(origin)?; ensure!(!authority_votes.is_empty(), Error::::NoVotesSpecified); @@ -1267,7 +1247,7 @@ pub mod pallet { }, AuthorityVote::Vote(vote) => { ( - <::Vote as VoteStorage>::vote_into_partial_vote( + <::Vote as VoteStorage>::vote_into_partial_vote( &vote, |shared_data| SharedDataHash::of(&shared_data) ), @@ -1282,8 +1262,8 @@ pub mod pallet { &authority, authority_index, |option_existing_vote, election_bitmap_components| { - let components = <::Vote as VoteStorage>::partial_vote_into_components( - ::generate_vote_properties( + let components = <::Vote as VoteStorage>::partial_vote_into_components( + ::generate_vote_properties( election_identifier, option_existing_vote, &partial_vote, @@ -1305,7 +1285,7 @@ pub mod pallet { components.individual_component { // Update shared data reference counts - <::Vote as VoteStorage>::visit_shared_data_references_in_individual_component( + <::Vote as VoteStorage>::visit_shared_data_references_in_individual_component( &individual_component, |shared_data_hash| Self::add_shared_data_reference(shared_data_hash, unique_monotonic_identifier, block_number), ); @@ -1323,7 +1303,7 @@ pub mod pallet { // Insert any `SharedData` provided as part of the `Vote`. if let Some(vote) = option_vote { - <::Vote as VoteStorage>::visit_shared_data_in_vote( + <::Vote as VoteStorage>::visit_shared_data_in_vote( vote, |shared_data| Self::inner_provide_shared_data(shared_data), ) @@ -1345,7 +1325,7 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::provide_shared_data())] pub fn provide_shared_data( origin: OriginFor, - shared_data: <::Vote as VoteStorage>::SharedData, + shared_data: <::Vote as VoteStorage>::SharedData, ) -> DispatchResult { Self::ensure_can_vote(origin)?; Self::inner_provide_shared_data(shared_data)?; @@ -1381,7 +1361,7 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::delete_vote())] pub fn delete_vote( origin: OriginFor, - election_identifier: ElectionIdentifierOf, + election_identifier: CompositeElectionIdentifierOf, ) -> DispatchResult { let (epoch_index, authority, authority_index) = Self::ensure_can_vote(origin)?; let unique_monotonic_identifier = Self::ensure_election_exists(election_identifier)?; @@ -1414,9 +1394,11 @@ pub mod pallet { pub fn update_settings( origin: OriginFor, unsynchronised_settings: Option< - ::ElectoralUnsynchronisedSettings, + ::ElectoralUnsynchronisedSettings, + >, + settings: Option< + ::ElectoralSettings, >, - settings: Option<::ElectoralSettings>, ignore_corrupt_storage: CorruptStorageAdherance, ) -> DispatchResult { Self::ensure_governance(origin, ignore_corrupt_storage)?; @@ -1452,7 +1434,7 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::clear_election_votes())] pub fn clear_election_votes( origin: OriginFor, - election_identifier: ElectionIdentifierOf, + election_identifier: CompositeElectionIdentifierOf, ignore_corrupt_storage: CorruptStorageAdherance, check_election_exists: bool, ) -> DispatchResult { @@ -1461,7 +1443,9 @@ pub mod pallet { Self::ensure_election_exists(election_identifier)?; } - ElectionAccess::::new(election_identifier).clear_votes(); + RunnerStorageAccess::::clear_election_votes( + *election_identifier.unique_monotonic(), + ); Ok(()) } @@ -1470,7 +1454,7 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::invalidate_election_consensus_cache())] pub fn invalidate_election_consensus_cache( origin: OriginFor, - election_identifier: ElectionIdentifierOf, + election_identifier: CompositeElectionIdentifierOf, ignore_corrupt_storage: CorruptStorageAdherance, check_election_exists: bool, ) -> DispatchResult { @@ -1598,51 +1582,45 @@ pub mod pallet { Self::deposit_event(Event::::CorruptStorage); }, ElectionPalletStatus::Running => { - let _ = Self::with_electoral_access_and_identifiers( - |electoral_access, election_identifiers| { - if Into::::into(block_number) % - BLOCKS_BETWEEN_CLEANUP == sp_core::U256::zero() - { - let minimum_election_identifiers = election_identifiers - .iter() - .copied() - .map(|election_identifier| { - *election_identifier.unique_monotonic() - }) - .min() - .unwrap_or_default(); - let mut settings_boundaries = - ElectoralSettings::::iter_keys().collect::>(); - settings_boundaries.sort(); - for setting_boundary in &settings_boundaries + let _ = Self::with_election_identifiers(|election_identifiers| { + if Into::::into(block_number) % BLOCKS_BETWEEN_CLEANUP == + sp_core::U256::zero() + { + let minimum_election_identifiers = election_identifiers + .iter() + .copied() + .map(|election_identifier| { + *election_identifier.unique_monotonic() + }) + .min() + .unwrap_or_default(); + let mut settings_boundaries = + ElectoralSettings::::iter_keys().collect::>(); + settings_boundaries.sort(); + for setting_boundary in &settings_boundaries [..settings_boundaries[..] .partition_point(|&setting_boundary| { setting_boundary <= minimum_election_identifiers }) .saturating_sub(1) /*Keep the latest settings lower than the minimum election identifier, i.e. the settings referenced by the election with the minimum election identifier*/] - { - ElectoralSettings::::remove(setting_boundary); - } + { + ElectoralSettings::::remove(setting_boundary); + } - let current_authorities = T::EpochInfo::current_authorities(); - for validator in ContributingAuthorities::::iter_keys() - .collect::>() - { - if !current_authorities.contains(&validator) { - ContributingAuthorities::::remove(validator); - } + let current_authorities = T::EpochInfo::current_authorities(); + for validator in + ContributingAuthorities::::iter_keys().collect::>() + { + if !current_authorities.contains(&validator) { + ContributingAuthorities::::remove(validator); } } + } - T::ElectoralSystem::on_finalize( - electoral_access, - election_identifiers, - &(), - )?; + T::ElectoralSystemRunner::on_finalize(election_identifiers)?; - Ok(()) - }, - ); + Ok(()) + }); }, } } @@ -1688,20 +1666,9 @@ pub mod pallet { Ok(()) } - /// Provides access into the ElectoralSystem's storage. - /// - /// Ideally we would avoid introducing re-entrance (also with - /// `with_electoral_access_and_identifiers`), so the ElectoralAccess object can internally - /// cache storage which is possibly helpful particularly in the case of composites. - pub fn with_electoral_access< - R, - F: FnOnce(&mut ElectoralAccess) -> Result, - >( - f: F, - ) -> Result { + pub fn ensure_initialized() -> Result<(), DispatchError> { if Status::::get().is_some() { - Self::handle_corrupt_storage(f(&mut ElectoralAccess::::new())) - .map_err(Into::into) + Ok(()) } else { Self::deposit_event(Event::::Uninitialized); Err(Error::::Uninitialized.into()) @@ -1711,14 +1678,13 @@ pub mod pallet { /// Provides access into the ElectoralSystem's storage, and also all the current election /// identifiers. /// - /// Ideally we would avoid introducing re-entrance (also with `with_electoral_access`), so + /// Ideally we would avoid introducing re-entrance (also with `with_storage_access`), so /// the ElectoralAccess object can internally cache storage which is possibly helpful /// particularly in the case of composites. - pub fn with_electoral_access_and_identifiers< + pub fn with_election_identifiers< R, F: FnOnce( - &mut ElectoralAccess, - Vec>, + Vec>, ) -> Result, >( f: F, @@ -1727,11 +1693,7 @@ pub mod pallet { let mut election_identifiers = ElectionProperties::::iter_keys().collect::>(); election_identifiers.sort(); - Self::handle_corrupt_storage(f( - &mut ElectoralAccess::::new(), - election_identifiers, - )) - .map_err(Into::into) + Self::handle_corrupt_storage(f(election_identifiers)).map_err(Into::into) } else { Self::deposit_event(Event::::Uninitialized); Err(Error::::Uninitialized.into()) @@ -1750,8 +1712,8 @@ pub mod pallet { let block_number = frame_system::Pallet::::current_block_number(); Some(ElectoralData { - current_elections: Self::with_electoral_access_and_identifiers( - |electoral_access, election_identifiers| { + current_elections: Self::with_election_identifiers( + |election_identifiers| { election_identifiers .into_iter() .map(|election_identifier| { @@ -1781,27 +1743,23 @@ pub mod pallet { }, )?; - let election_access = - electoral_access.election(election_identifier)?; - Ok(( - election_identifier, - AuthorityElectionData { - settings: election_access.settings()?, - properties: election_access.properties()?, - // We report the vote to the engine even though it is timeouted so the engine - // knows to delete it. As it may still later to reconstructed if the right - // `SharedData` is provided, unless it is delete. - option_existing_vote: option_current_authority_vote.as_ref().map(|(_, authority_vote)| { - authority_vote.clone() - }), - is_vote_desired: ::is_vote_desired( election_identifier, - &election_access, - option_current_authority_vote.filter(|_| !contains_timed_out_shared_data_references), - )?, - }, - )) + AuthorityElectionData { + settings: RunnerStorageAccess::::electoral_settings_for_election(*election_identifier.unique_monotonic())?, + properties: RunnerStorageAccess::::election_properties(election_identifier)?, + // We report the vote to the engine even though it is timeouted so the engine + // knows to delete it. As it may still later to reconstructed if the right + // `SharedData` is provided, unless it is delete. + option_existing_vote: option_current_authority_vote.as_ref().map(|(_, authority_vote)| { + authority_vote.clone() + }), + is_vote_desired: ::is_vote_desired( + election_identifier, + option_current_authority_vote.filter(|_| !contains_timed_out_shared_data_references), + )?, + }, + )) }) .collect::, _>>() }, @@ -1843,10 +1801,10 @@ pub mod pallet { pub fn filter_votes( validator_id: &T::ValidatorId, proposed_votes: BTreeMap< - ElectionIdentifierOf, - <::Vote as VoteStorage>::Vote, + CompositeElectionIdentifierOf, + <::Vote as VoteStorage>::Vote, >, - ) -> BTreeSet> { + ) -> BTreeSet> { use frame_support::traits::OriginTrait; if let Ok((epoch_index, authority, authority_index)) = @@ -1854,8 +1812,8 @@ pub mod pallet { { let block_number = frame_system::Pallet::::current_block_number(); - Self::with_electoral_access_and_identifiers( - |_electoral_access, election_identifiers| { + Self::with_election_identifiers( + |election_identifiers| { election_identifiers .into_iter() .map(|election_identifier| { @@ -1896,12 +1854,12 @@ pub mod pallet { )) = option_current_authority_vote .filter(|_| !contains_timed_out_shared_data_references) { - ::is_vote_needed( + ::is_vote_needed( ( existing_vote_properties, match &existing_authority_vote { AuthorityVote::Vote(existing_vote) => { - <::Vote as VoteStorage>::vote_into_partial_vote( + <::Vote as VoteStorage>::vote_into_partial_vote( existing_vote, |shared_data| SharedDataHash::of(&shared_data) ) @@ -1911,7 +1869,7 @@ pub mod pallet { existing_authority_vote, ), ( - <::Vote as VoteStorage>::vote_into_partial_vote( + <::Vote as VoteStorage>::vote_into_partial_vote( proposed_vote, |shared_data| SharedDataHash::of(&shared_data) ), @@ -1963,7 +1921,10 @@ pub mod pallet { fn take_vote_and_then< R, F: for<'a> FnOnce( - Option<(VotePropertiesOf, AuthorityVoteOf)>, + Option<( + VotePropertiesOf, + AuthorityVoteOf, + )>, &'a mut ElectionBitmapComponents, ) -> Result, >( @@ -1981,7 +1942,7 @@ pub mod pallet { IndividualComponents::::take(unique_monotonic_identifier, authority); let r = f( - <::Vote as VoteStorage>::components_into_authority_vote( + <::Vote as VoteStorage>::components_into_authority_vote( VoteComponents { bitmap_component: election_bitmap_components.take(authority_index)?, individual_component: individual_component.clone(), @@ -1994,7 +1955,7 @@ pub mod pallet { // Remove references late to avoid deleting shared data that we will add // references to inside `f`. if let Some((_properties, individual_component)) = individual_component { - <::Vote as VoteStorage>::visit_shared_data_references_in_individual_component( + <::Vote as VoteStorage>::visit_shared_data_references_in_individual_component( &individual_component, |shared_data_hash| Self::remove_shared_data_reference(shared_data_hash, unique_monotonic_identifier), ); @@ -2019,10 +1980,13 @@ pub mod pallet { authority_index: AuthorityCount, mut visit_unprovided_shared_data: VisitUnprovidedSharedData, ) -> Result< - Option<(VotePropertiesOf, AuthorityVoteOf)>, + Option<( + VotePropertiesOf, + AuthorityVoteOf, + )>, CorruptStorageError, > { - <::Vote as VoteStorage>::components_into_authority_vote( + <::Vote as VoteStorage>::components_into_authority_vote( VoteComponents { bitmap_component: ElectionBitmapComponents::::with( epoch_index, @@ -2079,7 +2043,7 @@ pub mod pallet { } } fn ensure_election_exists( - election_identifier: ElectionIdentifierOf, + election_identifier: CompositeElectionIdentifierOf, ) -> Result> { ensure!( ElectionProperties::::contains_key(election_identifier), @@ -2088,7 +2052,7 @@ pub mod pallet { Ok(*election_identifier.unique_monotonic()) } - fn handle_corrupt_storage( + pub fn handle_corrupt_storage( result: Result, ) -> Result> { match result { @@ -2152,7 +2116,7 @@ pub mod pallet { } } fn inner_provide_shared_data( - shared_data: <::Vote as VoteStorage>::SharedData, + shared_data: <::Vote as VoteStorage>::SharedData, ) -> Result<(), Error> { let shared_data_hash = SharedDataHash::of(&shared_data); let (unique_monotonic_identifiers, reference_details): (Vec<_>, Vec<_>) = diff --git a/state-chain/pallets/cf-elections/src/mock.rs b/state-chain/pallets/cf-elections/src/mock.rs index 3d3ea08aad..f58ba4aeed 100644 --- a/state-chain/pallets/cf-elections/src/mock.rs +++ b/state-chain/pallets/cf-elections/src/mock.rs @@ -24,7 +24,7 @@ impl pallet_cf_elections::Config for Test { type RuntimeEvent = RuntimeEvent; // TODO: Use Settings? - type ElectoralSystem = crate::electoral_systems::mock::MockElectoralSystem; + type ElectoralSystemRunner = crate::electoral_systems::mock::MockElectoralSystem; type WeightInfo = (); } diff --git a/state-chain/pallets/cf-elections/src/tests.rs b/state-chain/pallets/cf-elections/src/tests.rs index 8df579c108..48673b1d7b 100644 --- a/state-chain/pallets/cf-elections/src/tests.rs +++ b/state-chain/pallets/cf-elections/src/tests.rs @@ -143,7 +143,7 @@ impl ElectoralSystemTestExt for TestRunner { #[track_caller] |mut ctx| { let unique_monotonic_identifier = - *Pallet::::with_electoral_access(|electoral_access| { + *Pallet::::with_storage_access(|electoral_access| { electoral_access.new_election((), (), ()) }) .expect("New election should not corrupt storage.") @@ -153,7 +153,7 @@ impl ElectoralSystemTestExt for TestRunner { assert_eq!(Status::::get(), Some(ElectionPalletStatus::Running)); - Pallet::::with_electoral_access(|electoral_access| { + Pallet::::with_storage_access(|electoral_access| { electoral_access .election(ElectionIdentifier::new(unique_monotonic_identifier, ())) }) diff --git a/state-chain/runtime/src/chainflip/solana_elections.rs b/state-chain/runtime/src/chainflip/solana_elections.rs index 19eb845ab5..76ccc1d2d5 100644 --- a/state-chain/runtime/src/chainflip/solana_elections.rs +++ b/state-chain/runtime/src/chainflip/solana_elections.rs @@ -4,10 +4,7 @@ use crate::{ }; use cf_chains::{ instances::ChainInstanceAlias, - sol::{ - api::SolanaTransactionType, SolAddress, SolAmount, SolHash, SolSignature, SolTrackedData, - SolanaCrypto, - }, + sol::{api::SolanaTransactionType, SolAddress, SolAmount, SolHash, SolSignature, SolTrackedData, SolanaCrypto}, Chain, FeeEstimationApi, ForeignChain, Solana, }; use cf_runtime_utilities::log_or_panic; @@ -22,13 +19,14 @@ use pallet_cf_elections::{ electoral_system::{ElectoralReadAccess, ElectoralSystem}, electoral_systems::{ self, - composite::{tuple_6_impls::Hooks, Composite, Translator}, + change::OnChangeHook, + composite::{tuple_6_impls::Hooks, CompositeRunner}, egress_success::OnEgressSuccess, liveness::OnCheckComplete, monotonic_change::OnChangeHook, monotonic_median::MedianChangeHook, }, - CorruptStorageError, ElectionIdentifier, InitialState, InitialStateOf, + CorruptStorageError, ElectionIdentifier, InitialState, InitialStateOf, RunnerStorageAccess, }; use scale_info::TypeInfo; @@ -42,7 +40,7 @@ use sol_prim::SlotNumber; type Instance = ::Instance; -pub type SolanaElectoralSystem = Composite< +pub type SolanaElectoralSystemRunner = CompositeRunner< ( SolanaBlockHeightTracking, SolanaFeeTracking, @@ -52,6 +50,7 @@ pub type SolanaElectoralSystem = Composite< SolanaLiveness, ), ::ValidatorId, + RunnerStorageAccess, SolanaElectionHooks, >; @@ -228,34 +227,7 @@ impl SolanaLiveness, > for SolanaElectionHooks { - type OnFinalizeContext = (); - type OnFinalizeReturn = (); - - fn on_finalize< - GenericElectoralAccess, - BlockHeightTranslator: Translator, - FeeTranslator: Translator, - IngressTranslator: Translator, - NonceTrackingTranslator: Translator, - EgressWitnessingTranslator: Translator, - LivenessTranslator: Translator, - >( - generic_electoral_access: &mut GenericElectoralAccess, - ( - block_height_translator, - fee_translator, - ingress_translator, - nonce_tracking_translator, - egress_witnessing_translator, - liveness_translator, - ): ( - BlockHeightTranslator, - FeeTranslator, - IngressTranslator, - NonceTrackingTranslator, - EgressWitnessingTranslator, - LivenessTranslator, - ), + fn on_finalize( ( block_height_identifiers, fee_identifiers, @@ -289,38 +261,49 @@ impl >, Vec::ElectionIdentifierExtra>>, ), - _context: &Self::OnFinalizeContext, - ) -> Result { - let block_height = SolanaBlockHeightTracking::on_finalize( - &mut block_height_translator.translate_electoral_access(generic_electoral_access), - block_height_identifiers, - &(), - )?; - SolanaLiveness::on_finalize( - &mut liveness_translator.translate_electoral_access(generic_electoral_access), - liveness_identifiers, - &(crate::System::block_number(), block_height), - )?; - SolanaFeeTracking::on_finalize( - &mut fee_translator.translate_electoral_access(generic_electoral_access), - fee_identifiers, - &(), - )?; - SolanaNonceTracking::on_finalize( - &mut nonce_tracking_translator.translate_electoral_access(generic_electoral_access), - nonce_tracking_identifiers, - &(), - )?; - SolanaEgressWitnessing::on_finalize( - &mut egress_witnessing_translator.translate_electoral_access(generic_electoral_access), - egress_witnessing_identifiers, - &(), - )?; - SolanaIngressTracking::on_finalize( - &mut ingress_translator.translate_electoral_access(generic_electoral_access), - ingress_identifiers, - &block_height, - )?; + ) -> Result<(), CorruptStorageError> { + let block_height = SolanaBlockHeightTracking::on_finalize::< + CompositeElectoralAccess< + _, + SolanaBlockHeightTracking, + RunnerStorageAccess, + >, + >(block_height_identifiers, &())?; + SolanaLiveness::on_finalize::< + CompositeElectoralAccess< + _, + SolanaLiveness, + RunnerStorageAccess, + >, + >(liveness_identifiers, &(crate::System::block_number(), block_height))?; + SolanaFeeTracking::on_finalize::< + CompositeElectoralAccess< + _, + SolanaFeeTracking, + RunnerStorageAccess, + >, + >(fee_identifiers, &())?; + SolanaNonceTracking::on_finalize::< + CompositeElectoralAccess< + _, + SolanaNonceTracking, + RunnerStorageAccess, + >, + >(nonce_tracking_identifiers, &())?; + SolanaEgressWitnessing::on_finalize::< + CompositeElectoralAccess< + _, + SolanaEgressWitnessing, + RunnerStorageAccess, + >, + >(egress_witnessing_identifiers, &())?; + SolanaIngressTracking::on_finalize::< + CompositeElectoralAccess< + _, + SolanaIngressTracking, + RunnerStorageAccess, + >, + >(ingress_identifiers, &block_height)?; Ok(()) } } @@ -353,19 +336,16 @@ impl BenchmarkValue for SolanaIngressSettings { } } +use pallet_cf_elections::electoral_systems::composite::tuple_6_impls::CompositeElectoralAccess; + pub struct SolanaChainTrackingProvider; impl GetBlockHeight for SolanaChainTrackingProvider { fn get_block_height() -> ::ChainBlockNumber { - pallet_cf_elections::Pallet::::with_electoral_access( - |electoral_access| { - SolanaElectoralSystem::with_access_translators(|access_translators| { - let (access_translator, ..) = &access_translators; - access_translator - .translate_electoral_access(electoral_access) - .unsynchronised_state() - }) - }, - ) + CompositeElectoralAccess::< + _, + SolanaBlockHeightTracking, + RunnerStorageAccess, + >::unsynchronised_state() .unwrap_or_else(|err| { log_or_panic!("Failed to obtain Solana block height: '{err:?}'."); // We use default in error case as it is preferable to panicking, and in @@ -375,19 +355,15 @@ impl GetBlockHeight for SolanaChainTrackingProvider { }) } } + impl SolanaChainTrackingProvider { pub fn priority_fee() -> Option<::ChainAmount> { - pallet_cf_elections::Pallet::::with_electoral_access( - |electoral_access| { - SolanaElectoralSystem::with_access_translators(|access_translators| { - let (_, access_translator, ..) = &access_translators; - let electoral_access = - access_translator.translate_electoral_access(electoral_access); - electoral_access.unsynchronised_state() - }) - }, - ) - .ok() + CompositeElectoralAccess::< + _, + SolanaFeeTracking, + RunnerStorageAccess, + >::unsynchronised_state() + .ok() } fn with_tracked_data_then_apply_fee_multiplier< @@ -395,25 +371,26 @@ impl SolanaChainTrackingProvider { >( f: F, ) -> ::ChainAmount { - pallet_cf_elections::Pallet::::with_electoral_access( - |electoral_access| { - SolanaElectoralSystem::with_access_translators(|access_translators| { - let (_, access_translator, ..) = &access_translators; - let electoral_access = - access_translator.translate_electoral_access(electoral_access); - Ok(electoral_access - .unsynchronised_settings()? - .fee_multiplier - .saturating_mul_int(f(SolTrackedData { - priority_fee: electoral_access.unsynchronised_state()?, - }))) + CompositeElectoralAccess::< + _, + SolanaFeeTracking, + RunnerStorageAccess, + >::unsynchronised_state() + .and_then(|priority_fee| { + CompositeElectoralAccess::< + _, + SolanaFeeTracking, + RunnerStorageAccess, + >::unsynchronised_settings().map(|fees| { + { + fees.fee_multiplier.saturating_mul_int(f(SolTrackedData { priority_fee })) + } }) - }, - ) - .unwrap_or_else(|err| { - log_or_panic!("Failed to obtain Solana fee: '{err:?}'."); - Default::default() - }) + }) + .unwrap_or_else(|err| { + log_or_panic!("Failed to obtain Solana fee: '{err:?}'."); + Default::default() + }) } } impl AdjustedFeeEstimationApi for SolanaChainTrackingProvider { @@ -441,22 +418,19 @@ impl IngressSource for SolanaIngress { asset: ::ChainAsset, close_block: ::ChainBlockNumber, ) -> DispatchResult { - pallet_cf_elections::Pallet::::with_electoral_access_and_identifiers( - |electoral_access, election_identifiers| { - SolanaElectoralSystem::with_identifiers( - election_identifiers, - |election_identifiers| { - SolanaElectoralSystem::with_access_translators(|access_translators| { - let (_, _, access_translator, ..) = &access_translators; - let (_, _, election_identifiers, ..) = election_identifiers; - SolanaIngressTracking::open_channel( - election_identifiers, - &mut access_translator.translate_electoral_access(electoral_access), - channel, - asset, - close_block, - ) - }) + pallet_cf_elections::Pallet::::with_election_identifiers( + |composite_election_identifiers| { + SolanaElectoralSystemRunner::with_identifiers( + composite_election_identifiers, + |grouped_election_identifiers| { + let (_, _, election_identifiers, ..) = grouped_election_identifiers; + SolanaIngressTracking::open_channel::< + CompositeElectoralAccess< + _, + SolanaIngressTracking, + RunnerStorageAccess, + >, + >(election_identifiers, channel, asset, close_block) }, ) }, @@ -471,20 +445,18 @@ impl SolanaNonceWatch for SolanaNonceTrackingTrigger { nonce_account: SolAddress, previous_nonce_value: SolHash, ) -> DispatchResult { - pallet_cf_elections::Pallet::::with_electoral_access( - |electoral_access| { - SolanaElectoralSystem::with_access_translators(|access_translators| { - let (_, _, _, access_translator, ..) = &access_translators; - let mut electoral_access = - access_translator.translate_electoral_access(electoral_access); - SolanaNonceTracking::watch_for_change( - &mut electoral_access, - nonce_account, - previous_nonce_value, - ) - }) - }, - ) + // TODO: Check if safe. We are not checking if initialised or not here - we were before in + // with_electoral_access + // TODO: Look at handling the corrupt storage elsewhere. + Ok(pallet_cf_elections::Pallet::::handle_corrupt_storage( + SolanaNonceTracking::watch_for_change::< + CompositeElectoralAccess< + _, + SolanaNonceTracking, + RunnerStorageAccess, + >, + >(nonce_account, previous_nonce_value), + )?) } } @@ -494,16 +466,16 @@ impl ElectionEgressWitnesser for SolanaEgressWitnessingTrigger { type Chain = SolanaCrypto; fn watch_for_egress_success(signature: SolSignature) -> DispatchResult { - pallet_cf_elections::Pallet::::with_electoral_access( - |electoral_access| { - SolanaElectoralSystem::with_access_translators(|access_translators| { - let (_, _, _, _, access_translator, ..) = &access_translators; - let mut electoral_access = - access_translator.translate_electoral_access(electoral_access); - - SolanaEgressWitnessing::watch_for_egress(&mut electoral_access, signature) - }) - }, - ) + // TODO: Check if safe. We are not checking if initialised or not here - we were before in + // with_electoral_access + Ok(pallet_cf_elections::Pallet::::handle_corrupt_storage( + SolanaEgressWitnessing::watch_for_egress::< + CompositeElectoralAccess< + _, + SolanaEgressWitnessing, + RunnerStorageAccess, + >, + >(signature), + )?) } } diff --git a/state-chain/runtime/src/lib.rs b/state-chain/runtime/src/lib.rs index c70a746158..7373f1d4f8 100644 --- a/state-chain/runtime/src/lib.rs +++ b/state-chain/runtime/src/lib.rs @@ -1000,7 +1000,7 @@ impl pallet_cf_chain_tracking::Config for Runtime { impl pallet_cf_elections::Config for Runtime { type RuntimeEvent = RuntimeEvent; - type ElectoralSystem = chainflip::solana_elections::SolanaElectoralSystem; + type ElectoralSystemRunner = chainflip::solana_elections::SolanaElectoralSystemRunner; type WeightInfo = pallet_cf_elections::weights::PalletWeight; } From 679ae9329b70f4f7d17e3d76ac2d3fa5fe81df2f Mon Sep 17 00:00:00 2001 From: kylezs Date: Wed, 23 Oct 2024 15:00:33 +0200 Subject: [PATCH 02/18] tests and clean up --- .../pallets/cf-elections/src/benchmarking.rs | 58 ++- .../cf-elections/src/electoral_system.rs | 5 +- .../src/electoral_system_runner.rs | 28 +- .../src/electoral_systems/composite.rs | 40 +- .../src/electoral_systems/mock.rs | 55 +- .../src/electoral_systems/mocks.rs | 148 +++--- .../src/electoral_systems/mocks/access.rs | 479 ++++++++++++------ .../src/electoral_systems/tests/liveness.rs | 9 +- .../tests/monotonic_median.rs | 15 +- .../electoral_systems/tests/unsafe_median.rs | 10 +- state-chain/pallets/cf-elections/src/lib.rs | 30 +- state-chain/pallets/cf-elections/src/mock.rs | 2 +- state-chain/pallets/cf-elections/src/tests.rs | 49 +- .../runtime/src/chainflip/solana_elections.rs | 32 +- 14 files changed, 575 insertions(+), 385 deletions(-) diff --git a/state-chain/pallets/cf-elections/src/benchmarking.rs b/state-chain/pallets/cf-elections/src/benchmarking.rs index 079ada4f57..6a0803ff2b 100644 --- a/state-chain/pallets/cf-elections/src/benchmarking.rs +++ b/state-chain/pallets/cf-elections/src/benchmarking.rs @@ -1,8 +1,8 @@ use crate::{ bitmap_components::ElectionBitmapComponents, - electoral_system::{ - AuthorityVoteOf, ElectionIdentifierOf, ElectoralSystem, IndividualComponentOf, - VotePropertiesOf, + electoral_system_runner::{ + CompositeAuthorityVoteOf, CompositeIndividualComponentOf, CompositeVotePropertiesOf, + ElectoralSystemRunner, }, vote_storage::VoteStorage, *, @@ -25,13 +25,13 @@ use crate::Call; #[allow(clippy::multiple_bound_locations)] #[instance_benchmarks( where - <<>::ElectoralSystem as ElectoralSystem>::Vote as VoteStorage>::Vote: BenchmarkValue, - <<>::ElectoralSystem as ElectoralSystem>::Vote as VoteStorage>::SharedData: BenchmarkValue, - <<>::ElectoralSystem as ElectoralSystem>::Vote as VoteStorage>::Properties: BenchmarkValue, - <<>::ElectoralSystem as ElectoralSystem>::Vote as VoteStorage>::IndividualComponent: BenchmarkValue, + <<>::ElectoralSystemRunner as ElectoralSystemRunner>::Vote as VoteStorage>::Vote: BenchmarkValue, + <<>::ElectoralSystemRunner as ElectoralSystemRunner>::Vote as VoteStorage>::SharedData: BenchmarkValue, + <<>::ElectoralSystemRunner as ElectoralSystemRunner>::Vote as VoteStorage>::Properties: BenchmarkValue, + <<>::ElectoralSystemRunner as ElectoralSystemRunner>::Vote as VoteStorage>::IndividualComponent: BenchmarkValue, InitialStateOf: BenchmarkValue, - ::ElectoralUnsynchronisedSettings: BenchmarkValue, - ::ElectoralSettings: BenchmarkValue, + ::ElectoralUnsynchronisedSettings: BenchmarkValue, + ::ElectoralSettings: BenchmarkValue, )] mod benchmarks { use super::*; @@ -64,8 +64,8 @@ mod benchmarks { fn setup_validators_and_vote, I: 'static>( validator_counts: u32, - vote_value: <<>::ElectoralSystem as ElectoralSystem>::Vote as VoteStorage>::Vote, - ) -> ElectionIdentifierOf { + vote_value: <<>::ElectoralSystemRunner as ElectoralSystemRunner>::Vote as VoteStorage>::Vote, + ) -> CompositeElectionIdentifierOf { // Setup a validator set of 150 as in the case of Mainnet. let validators = ready_validator_for_vote::(validator_counts); let caller = validators[0].clone(); @@ -82,7 +82,9 @@ mod benchmarks { BoundedBTreeMap::try_from( [( election_identifier, - AuthorityVoteOf::::Vote(vote_value.clone()), + CompositeAuthorityVoteOf::::Vote( + vote_value.clone() + ), )] .into_iter() .collect::>(), @@ -106,7 +108,9 @@ mod benchmarks { BoundedBTreeMap::try_from( iter::repeat(( next_election.0, - AuthorityVoteOf::::Vote(BenchmarkValue::benchmark_value()), + CompositeAuthorityVoteOf::::Vote( + BenchmarkValue::benchmark_value(), + ), )) .take(n as usize) .collect::>(), @@ -166,7 +170,9 @@ mod benchmarks { BoundedBTreeMap::try_from( [( next_election.0, - AuthorityVoteOf::::Vote(BenchmarkValue::benchmark_value()), + CompositeAuthorityVoteOf::::Vote( + BenchmarkValue::benchmark_value() + ), )] .into_iter() .collect::>(), @@ -201,7 +207,9 @@ mod benchmarks { BoundedBTreeMap::try_from( [( next_election.0, - AuthorityVoteOf::::Vote(BenchmarkValue::benchmark_value()), + CompositeAuthorityVoteOf::::Vote( + BenchmarkValue::benchmark_value() + ), )] .into_iter() .collect::>(), @@ -239,7 +247,9 @@ mod benchmarks { BoundedBTreeMap::try_from( [( election_identifier, - AuthorityVoteOf::::Vote(BenchmarkValue::benchmark_value()), + CompositeAuthorityVoteOf::::Vote( + BenchmarkValue::benchmark_value() + ), )] .into_iter() .collect::>(), @@ -252,7 +262,7 @@ mod benchmarks { assert_eq!( SharedData::::get(SharedDataHash::of::< - <<>::ElectoralSystem as ElectoralSystem>::Vote as VoteStorage>::Vote, + <<>::ElectoralSystemRunner as ElectoralSystemRunner>::Vote as VoteStorage>::Vote, >(&BenchmarkValue::benchmark_value())), Some(BenchmarkValue::benchmark_value()) ); @@ -351,8 +361,7 @@ mod benchmarks { } assert!(!ElectionConsensusHistoryUpToDate::::contains_key( - access_impls::ElectionAccess::::new(election_identifier) - .unique_monotonic_identifier() + election_identifier.unique_monotonic() )); } @@ -369,8 +378,7 @@ mod benchmarks { }; let epoch = T::EpochInfo::epoch_index(); - let monotonic_identifier = access_impls::ElectionAccess::::new(election_identifier) - .unique_monotonic_identifier(); + let monotonic_identifier = election_identifier.unique_monotonic(); Pallet::::on_finalize(frame_system::Pallet::::block_number()); assert_eq!( @@ -489,7 +497,7 @@ mod benchmarks { (0..b).for_each(|i| { SharedData::::insert( SharedDataHash::of(&i), - <::Vote as VoteStorage>::SharedData::benchmark_value()); + <::Vote as VoteStorage>::SharedData::benchmark_value()); }); (0..c).for_each(|i| { @@ -506,8 +514,8 @@ mod benchmarks { UniqueMonotonicIdentifier::from_u64(i as u64), T::ValidatorId::from(validators[i as usize].clone()), ( - VotePropertiesOf::::benchmark_value(), - IndividualComponentOf::::benchmark_value(), + CompositeVotePropertiesOf::::benchmark_value(), + CompositeIndividualComponentOf::::benchmark_value(), ), ); }); @@ -538,7 +546,7 @@ mod benchmarks { mod tests { use super::*; - use crate::{mock::*, tests::ElectoralSystemTestExt, Instance1}; + use crate::{mock::*, tests::ElectoralSystemRunnerTestExt, Instance1}; macro_rules! benchmark_tests { ( $( $test_name:ident: $test_fn:ident ( $( $arg:expr ),* ) ),+ $(,)? ) => { diff --git a/state-chain/pallets/cf-elections/src/electoral_system.rs b/state-chain/pallets/cf-elections/src/electoral_system.rs index 93f88a6178..840e12d332 100644 --- a/state-chain/pallets/cf-elections/src/electoral_system.rs +++ b/state-chain/pallets/cf-elections/src/electoral_system.rs @@ -214,9 +214,12 @@ mod access { use super::{CorruptStorageError, ElectionIdentifierOf, ElectoralSystem}; + #[cfg(test)] + use codec::{Decode, Encode}; + /// Represents the current consensus, and how it has changed since it was last checked (i.e. /// 'check_consensus' was called). - #[cfg_attr(test, derive(Clone, Debug, PartialEq, Eq))] + #[cfg_attr(test, derive(Clone, Debug, PartialEq, Eq, Encode, Decode))] pub enum ConsensusStatus { /// You did not have consensus when previously checked, but now consensus has been gained. Gained { diff --git a/state-chain/pallets/cf-elections/src/electoral_system_runner.rs b/state-chain/pallets/cf-elections/src/electoral_system_runner.rs index 9f0e2b1410..f640491979 100644 --- a/state-chain/pallets/cf-elections/src/electoral_system_runner.rs +++ b/state-chain/pallets/cf-elections/src/electoral_system_runner.rs @@ -16,23 +16,23 @@ pub type CompositeElectionIdentifierOf = ElectionIdentifier<::ElectionIdentifierExtra>; #[allow(type_alias_bounds)] -pub type AuthorityVoteOf = AuthorityVote< +pub type CompositeAuthorityVoteOf = AuthorityVote< <::Vote as VoteStorage>::PartialVote, <::Vote as VoteStorage>::Vote, >; #[allow(type_alias_bounds)] -pub type IndividualComponentOf = +pub type CompositeIndividualComponentOf = <::Vote as VoteStorage>::IndividualComponent; #[allow(type_alias_bounds)] pub type BitmapComponentOf = <::Vote as VoteStorage>::BitmapComponent; #[allow(type_alias_bounds)] -pub type VotePropertiesOf = +pub type CompositeVotePropertiesOf = <::Vote as VoteStorage>::Properties; pub struct CompositeConsensusVote { // If the validator hasn't voted, they will get a None. - pub vote: Option<(VotePropertiesOf, ::Vote)>, + pub vote: Option<(CompositeVotePropertiesOf, ::Vote)>, pub validator_id: ES::ValidatorId, } @@ -40,6 +40,16 @@ pub struct CompositeConsensusVotes { pub votes: Vec>, } +#[cfg(test)] +impl CompositeConsensusVotes { + pub fn active_votes(self) -> Vec<::Vote> { + self.votes + .into_iter() + .filter_map(|CompositeConsensusVote { vote, .. }| vote.map(|v| v.1)) + .collect() + } +} + pub trait ElectoralSystemRunner: 'static + Sized { type ValidatorId: Parameter + Member + MaybeSerializeDeserialize; @@ -107,7 +117,7 @@ pub trait ElectoralSystemRunner: 'static + Sized { /// per state-chain block, for each active election. fn is_vote_desired( _election_identifier_with_extra: CompositeElectionIdentifierOf, - current_vote: Option<(VotePropertiesOf, AuthorityVoteOf)>, + current_vote: Option<(CompositeVotePropertiesOf, CompositeAuthorityVoteOf)>, ) -> Result { Ok(current_vote.is_none()) } @@ -116,9 +126,9 @@ pub trait ElectoralSystemRunner: 'static + Sized { /// This is a way to decrease the amount of extrinsics a validator needs to send. fn is_vote_needed( _current_vote: ( - VotePropertiesOf, + CompositeVotePropertiesOf, ::PartialVote, - AuthorityVoteOf, + CompositeAuthorityVoteOf, ), _proposed_vote: ( ::PartialVote, @@ -156,9 +166,9 @@ pub trait ElectoralSystemRunner: 'static + Sized { /// function. fn generate_vote_properties( election_identifier: CompositeElectionIdentifierOf, - previous_vote: Option<(VotePropertiesOf, AuthorityVoteOf)>, + previous_vote: Option<(CompositeVotePropertiesOf, CompositeAuthorityVoteOf)>, vote: &::PartialVote, - ) -> Result, CorruptStorageError>; + ) -> Result, CorruptStorageError>; /// This is called during the pallet's `on_finalize` callback, if elections aren't paused and /// the CorruptStorage error hasn't occurred. 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 e49e6066b6..1cea032383 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/composite.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/composite.rs @@ -43,8 +43,8 @@ macro_rules! generate_electoral_system_tuple_impls { ElectionIdentifierOf, ConsensusStatus, }, - electoral_system_runner::{ElectoralSystemRunner, AuthorityVoteOf, RunnerStorageAccessTrait, - VotePropertiesOf, CompositeConsensusVotes, CompositeElectionIdentifierOf, CompositeConsensusVote}, + electoral_system_runner::{ElectoralSystemRunner, CompositeAuthorityVoteOf, RunnerStorageAccessTrait, + CompositeVotePropertiesOf, CompositeConsensusVotes, CompositeElectionIdentifierOf, CompositeConsensusVote}, vote_storage::{ AuthorityVote, VoteStorage @@ -133,14 +133,14 @@ macro_rules! generate_electoral_system_tuple_impls { fn is_vote_desired( election_identifier: ElectionIdentifier, current_vote: Option<( - VotePropertiesOf, - AuthorityVoteOf, + CompositeVotePropertiesOf, + CompositeAuthorityVoteOf, )>, ) -> Result { match *election_identifier.extra() { $(CompositeElectionIdentifierExtra::$electoral_system(extra) => { <$electoral_system as ElectoralSystem>::is_vote_desired( - &CompositeElectionAccess::::new(election_identifier.with_extra(extra)), + &DerivedElectionAccess::::new(election_identifier.with_extra(extra)), current_vote.map(|(properties, vote)| { Ok(( match properties { @@ -160,7 +160,7 @@ macro_rules! generate_electoral_system_tuple_impls { } fn is_vote_needed( - (current_vote_properties, current_partial_vote, current_authority_vote): (VotePropertiesOf, ::PartialVote, AuthorityVoteOf), + (current_vote_properties, current_partial_vote, current_authority_vote): (CompositeVotePropertiesOf, ::PartialVote, CompositeAuthorityVoteOf), (proposed_partial_vote, proposed_vote): (::PartialVote, ::Vote), ) -> bool { match (current_vote_properties, current_partial_vote, current_authority_vote, proposed_partial_vote, proposed_vote) { @@ -197,11 +197,11 @@ macro_rules! generate_electoral_system_tuple_impls { fn generate_vote_properties( election_identifier: ElectionIdentifier, previous_vote: Option<( - VotePropertiesOf, - AuthorityVoteOf, + CompositeVotePropertiesOf, + CompositeAuthorityVoteOf, )>, partial_vote: &::PartialVote, - ) -> Result, CorruptStorageError> { + ) -> Result, CorruptStorageError> { match (*election_identifier.extra(), partial_vote) { $((CompositeElectionIdentifierExtra::$electoral_system(extra), CompositePartialVote::$electoral_system(partial_vote)) => { <$electoral_system as ElectoralSystem>::generate_vote_properties( @@ -244,7 +244,7 @@ macro_rules! generate_electoral_system_tuple_impls { Ok(match *election_identifier.extra() { $(CompositeElectionIdentifierExtra::$electoral_system(extra) => { <$electoral_system as ElectoralSystem>::check_consensus( - &CompositeElectionAccess::::new(election_identifier.with_extra(extra)), + &DerivedElectionAccess::::new(election_identifier.with_extra(extra)), previous_consensus.map(|previous_consensus| { match previous_consensus { CompositeConsensus::$electoral_system(previous_consensus) => Ok(previous_consensus), @@ -279,12 +279,12 @@ macro_rules! generate_electoral_system_tuple_impls { } } - pub struct CompositeElectionAccess { + pub struct DerivedElectionAccess { id: ElectionIdentifierOf, _phantom: core::marker::PhantomData<(Tag, ES, StorageAccess)>, } - impl CompositeElectionAccess { + impl DerivedElectionAccess { fn new(id: ElectionIdentifierOf) -> Self { Self { id, @@ -292,7 +292,7 @@ macro_rules! generate_electoral_system_tuple_impls { } } } - pub struct CompositeElectoralAccess { + pub struct DerivedElectoralAccess { _phantom: core::marker::PhantomData<(Tag, ES, StorageAccess)>, } @@ -305,7 +305,7 @@ macro_rules! generate_electoral_system_tuple_impls { (@ $($previous:ident,)*;: $($electoral_system:ident,)*) => {}; (@ $($previous:ident,)*; $current:ident, $($remaining:ident,)*: $($electoral_system:ident,)*) => { - impl<'a, $($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static, StorageAccess: RunnerStorageAccessTrait> + 'static> ElectionReadAccess for CompositeElectionAccess { + impl<'a, $($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static, StorageAccess: RunnerStorageAccessTrait> + 'static> ElectionReadAccess for DerivedElectionAccess { type ElectoralSystem = $current; fn settings(&self) -> Result<$current::ElectoralSettings, CorruptStorageError> { @@ -334,7 +334,7 @@ macro_rules! generate_electoral_system_tuple_impls { } } - impl<$($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static, StorageAccess: RunnerStorageAccessTrait> + 'static> ElectionWriteAccess for CompositeElectionAccess { + impl<$($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static, StorageAccess: RunnerStorageAccessTrait> + 'static> ElectionWriteAccess for DerivedElectionAccess { fn set_state(&self, state: $current::ElectionState) -> Result<(), CorruptStorageError> { StorageAccess::set_election_state(*self.id.unique_monotonic(), CompositeElectionState::$current(state)) } @@ -371,14 +371,14 @@ macro_rules! generate_electoral_system_tuple_impls { } } - impl<$($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static, StorageAccess: RunnerStorageAccessTrait> + 'static> ElectoralReadAccess for CompositeElectoralAccess { + impl<$($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static, StorageAccess: RunnerStorageAccessTrait> + 'static> ElectoralReadAccess for DerivedElectoralAccess { type ElectoralSystem = $current; - type ElectionReadAccess = CompositeElectionAccess; + type ElectionReadAccess = DerivedElectionAccess; fn election( id: ElectionIdentifier<<$current as ElectoralSystem>::ElectionIdentifierExtra>, ) -> Self::ElectionReadAccess { - CompositeElectionAccess::::new(id) + DerivedElectionAccess::::new(id) } fn unsynchronised_settings( ) -> Result<$current::ElectoralUnsynchronisedSettings, CorruptStorageError> { @@ -401,8 +401,8 @@ macro_rules! generate_electoral_system_tuple_impls { } } - impl<'a, $($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static, StorageAccess: RunnerStorageAccessTrait> + 'static> ElectoralWriteAccess for CompositeElectoralAccess { - type ElectionWriteAccess = CompositeElectionAccess; + impl<'a, $($electoral_system: ElectoralSystem,)* ValidatorId: MaybeSerializeDeserialize + Parameter + Member, H: Hooks<$($electoral_system),*> + 'static, StorageAccess: RunnerStorageAccessTrait> + 'static> ElectoralWriteAccess for DerivedElectoralAccess { + type ElectionWriteAccess = DerivedElectionAccess; fn new_election( extra: $current::ElectionIdentifierExtra, 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 d613a5c15e..796540c520 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/mock.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/mock.rs @@ -1,14 +1,18 @@ use crate::{ electoral_system::{ - AuthorityVoteOf, ConsensusStatus, ConsensusVotes, ElectionReadAccess, ElectionWriteAccess, - ElectoralSystem, ElectoralWriteAccess, VotePropertiesOf, + ConsensusStatus, ConsensusVotes, ElectionReadAccess, ElectionWriteAccess, ElectoralSystem, + ElectoralWriteAccess, }, + electoral_system_runner::{CompositeConsensusVotes, RunnerStorageAccessTrait}, mock::Test, vote_storage::{self, VoteStorage}, - CorruptStorageError, ElectionIdentifier, UniqueMonotonicIdentifier, + CompositeAuthorityVoteOf, CompositeElectionIdentifierOf, CompositeVotePropertiesOf, + CorruptStorageError, ElectionIdentifier, ElectoralSystemRunner, RunnerStorageAccess, + UniqueMonotonicIdentifier, }; use cf_primitives::AuthorityCount; use cf_traits::Chainflip; +use frame_support::instances::Instance1; use sp_std::vec::Vec; use std::{cell::RefCell, collections::BTreeMap}; @@ -32,7 +36,7 @@ thread_local! { /// - `consensus_status` is set to `None` by default. /// /// If assume_consensus is set to `true`, then the consensus value will be the number of votes. -pub struct MockElectoralSystem; +pub struct MockElectoralSystemRunner; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum BehaviourUpdate { @@ -65,7 +69,7 @@ impl BehaviourUpdate { } } -impl MockElectoralSystem { +impl MockElectoralSystemRunner { pub fn vote_desired() -> bool { VOTE_DESIRED.with(|v| *v.borrow()) } @@ -115,7 +119,7 @@ impl MockElectoralSystem { } } -impl ElectoralSystem for MockElectoralSystem { +impl ElectoralSystemRunner for MockElectoralSystemRunner { type ValidatorId = ::ValidatorId; type ElectoralUnsynchronisedState = (); type ElectoralUnsynchronisedStateMapKey = (); @@ -130,40 +134,34 @@ impl ElectoralSystem for MockElectoralSystem { type Vote = vote_storage::individual::Individual<(), vote_storage::individual::shared::Shared<()>>; type Consensus = AuthorityCount; - type OnFinalizeContext = (); - type OnFinalizeReturn = (); fn generate_vote_properties( - _election_identifier: ElectionIdentifier, - _previous_vote: Option<(VotePropertiesOf, AuthorityVoteOf)>, + _election_identifier: CompositeElectionIdentifierOf, + _previous_vote: Option<(CompositeVotePropertiesOf, CompositeAuthorityVoteOf)>, _vote: &::PartialVote, ) -> Result<(), CorruptStorageError> { Ok(()) } - fn on_finalize>( - electoral_access: &mut ElectoralAccess, - election_identifiers: Vec>, - _context: &Self::OnFinalizeContext, - ) -> Result { + fn on_finalize( + election_identifiers: Vec>, + ) -> Result<(), CorruptStorageError> { for id in election_identifiers { - // Read the current consensus status and save it. - let mut election = electoral_access.election_mut(id)?; - let consensus = election.check_consensus()?; + let consensus = + RunnerStorageAccess::::check_election_consensus(id).unwrap(); Self::set_consensus_status(*id.unique_monotonic(), consensus.clone()); if consensus.has_consensus().is_some() && Self::should_delete_on_finalize_consensus() { - election.delete(); + RunnerStorageAccess::::delete_election(id); } } Ok(()) } - fn check_consensus>( - _election_identifier: ElectionIdentifier, - _election_access: &ElectionAccess, + fn check_consensus( + _election_identifier: CompositeElectionIdentifierOf, _previous_consensus: Option<&Self::Consensus>, - consensus_votes: ConsensusVotes, + consensus_votes: CompositeConsensusVotes, ) -> Result, CorruptStorageError> { Ok(if Self::should_assume_consensus() { Some(consensus_votes.active_votes().len() as AuthorityCount) @@ -172,19 +170,18 @@ impl ElectoralSystem for MockElectoralSystem { }) } - fn is_vote_desired>( - _election_identifier_with_extra: crate::electoral_system::ElectionIdentifierOf, - _election_access: &ElectionAccess, - _current_vote: Option<(VotePropertiesOf, AuthorityVoteOf)>, + fn is_vote_desired( + _election_identifier: CompositeElectionIdentifierOf, + _current_vote: Option<(CompositeVotePropertiesOf, CompositeAuthorityVoteOf)>, ) -> Result { Ok(Self::vote_desired()) } fn is_vote_needed( _current_vote: ( - VotePropertiesOf, + CompositeVotePropertiesOf, ::PartialVote, - AuthorityVoteOf, + CompositeAuthorityVoteOf, ), _proposed_vote: ( ::PartialVote, 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 4f28f85440..a0e6e880d7 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/mocks.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/mocks.rs @@ -1,6 +1,11 @@ -use crate::electoral_system::{ - ConsensusStatus, ConsensusVotes, ElectionIdentifierOf, ElectoralReadAccess, ElectoralSystem, - ElectoralWriteAccess, +use std::collections::BTreeMap; + +use crate::{ + electoral_system::{ + ConsensusStatus, ConsensusVotes, ElectionIdentifierOf, ElectionReadAccess, + ElectoralReadAccess, ElectoralSystem, ElectoralWriteAccess, + }, + UniqueMonotonicIdentifier, }; use frame_support::{CloneNoBound, DebugNoBound, EqNoBound, PartialEqNoBound}; @@ -37,7 +42,6 @@ where #[derive(CloneNoBound, DebugNoBound, PartialEqNoBound, EqNoBound)] pub struct TestContext { setup: TestSetup, - electoral_access: MockAccess, } impl TestSetup @@ -79,36 +83,48 @@ where // Useful for testing check_consensus since we already have an election. pub fn build_with_initial_election(self) -> TestContext { let setup = self.clone(); - let mut electoral_access = MockAccess::::new( - self.unsynchronised_state, - self.unsynchronised_settings, - self.electoral_settings, - ); + + // We need to clear the storage at every build so if there are multiple test contexts used + // within a single test they do not conflict. + MockStorageAccess::clear_storage(); let (election_identifier_extra, election_properties, election_state) = self.initial_election_state.unwrap_or_default(); - let election = electoral_access - .new_election(election_identifier_extra, election_properties, election_state) - .unwrap(); + // These are synchronised with respect to an election, so for simplicity in the tests + // we just assign them to an election. + MockStorageAccess::set_electoral_settings::(setup.electoral_settings.clone()); + MockStorageAccess::set_unsynchronised_state::(setup.unsynchronised_state.clone()); + MockStorageAccess::set_unsynchronised_settings::(setup.unsynchronised_settings.clone()); + + let election = MockAccess::::new_election( + election_identifier_extra, + election_properties, + election_state, + ) + .unwrap(); + + println!( + "Election identifier in with initial election: {:?}", + election.election_identifier() + ); // A new election should not have consensus at any authority count. assert_eq!(election.check_consensus(None, ConsensusVotes { votes: vec![] }).unwrap(), None); - TestContext { setup, electoral_access } + TestContext { setup } } // We may want to test initialisation of elections within on finalise, so *don't* want to // initialise an election in the utilities. pub fn build(self) -> TestContext { - TestContext { - setup: self.clone(), - electoral_access: MockAccess::::new( - self.unsynchronised_state, - self.unsynchronised_settings, - self.electoral_settings, - ), - } + // We need to clear the storage at every build so if there are multiple test contexts used + // within a single test they do not conflict. + MockStorageAccess::clear_storage(); + + MockStorageAccess::set_electoral_settings::(self.electoral_settings.clone()); + + TestContext { setup: self.clone() } } } @@ -129,10 +145,7 @@ impl TestContext { // Expect only one election. let current_election_id = self.only_election_id(); - let new_consensus = self - .electoral_access - .election(current_election_id) - .unwrap() + let new_consensus = MockAccess::::election(current_election_id) .check_consensus(None, consensus_votes) .unwrap(); @@ -157,7 +170,7 @@ impl TestContext { } pub fn all_election_ids(&self) -> Vec> { - self.electoral_access.election_identifiers() + MockStorageAccess::election_identifiers::() } /// Update the current consensus without processing any votes. @@ -166,27 +179,15 @@ impl TestContext { self.inner_force_consensus_update(id, new_consensus) } - pub fn access(&self) -> &MockAccess { - &self.electoral_access - } - - pub fn mut_access(&mut self) -> &mut MockAccess { - &mut self.electoral_access - } - #[track_caller] fn inner_force_consensus_update( self, election_id: ElectionIdentifierOf, new_consensus: ConsensusStatus, ) -> Self { - let mut electoral_access = self.electoral_access.clone(); - electoral_access - .election_mut(election_id) - .unwrap() - .set_consensus_status(new_consensus); + MockStorageAccess::set_consensus_status::(election_id, new_consensus); - Self { electoral_access, ..self } + self } /// Test the finalization of the election. @@ -199,18 +200,22 @@ impl TestContext { /// See [register_checks] and #[track_caller] pub fn test_on_finalize( - mut self, + self, on_finalize_context: &ES::OnFinalizeContext, - pre_finalize_checks: impl FnOnce(&MockAccess), + pre_finalize_checks: impl FnOnce(&ElectoralSystemState), post_finalize_checks: impl IntoIterator>, ) -> Self { - let pre_finalize = self.electoral_access.clone(); - // TODO: Move 'hook' static local checks into MockAccess so we can remove this. + let pre_finalize = ElectoralSystemState::::load_state(); + // TODO: Move 'hook' static local checks into ElectoralSystemState so we can remove this. pre_finalize_checks(&pre_finalize); - self.electoral_access.finalize_elections(on_finalize_context).unwrap(); + ES::on_finalize::>( + MockStorageAccess::election_identifiers::(), + on_finalize_context, + ) + .unwrap(); - let post_finalize = self.electoral_access.clone(); + let post_finalize = ElectoralSystemState::::load_state(); for check in post_finalize_checks { check.check(&pre_finalize, &post_finalize); } @@ -294,36 +299,55 @@ register_checks! { assert_unchanged(pre_finalize, post_finalize) { assert_eq!(pre_finalize, post_finalize); }, + // check state is deleted, rather than can't construct election last_election_deleted(pre_finalize, post_finalize) { - let last_election_id = *pre_finalize.election_identifiers().last().expect("Expected an election before finalization."); + let last_election_id = pre_finalize.election_identifiers.last().expect("Expected an election before finalization"); assert!( - post_finalize.election(last_election_id).is_err(), - "Expected election {:?} to be deleted. Elections before: {:?}. After: {:?}", - last_election_id, - pre_finalize.election_identifiers(), - post_finalize.election_identifiers(), + !post_finalize.election_identifiers.contains(last_election_id), + "Last election should have been deleted.", ); }, election_id_incremented(pre_finalize, post_finalize) { assert_eq!( - pre_finalize.next_umi().next_identifier().unwrap(), - post_finalize.next_umi(), + pre_finalize.next_umi.next_identifier().unwrap(), + post_finalize.next_umi, "Expected the election id to be incremented.", ); }, all_elections_deleted(pre_finalize, post_finalize) { assert!( - !pre_finalize.election_identifiers().is_empty(), + !pre_finalize.election_identifiers.is_empty(), "Expected elections before finalization. This check makes no sense otherwise.", ); assert!( - post_finalize.election_identifiers().is_empty(), + post_finalize.election_identifiers.is_empty(), "Expected no elections after finalization.", ); }, } -type CheckFn = Box, &MockAccess)>; +#[derive(CloneNoBound, DebugNoBound, PartialEqNoBound, EqNoBound)] +pub struct ElectoralSystemState { + pub unsynchronised_state: ES::ElectoralUnsynchronisedState, + pub unsynchronised_state_map: BTreeMap, Option>>, + pub unsynchronised_settings: ES::ElectoralUnsynchronisedSettings, + pub election_identifiers: Vec>, + pub next_umi: UniqueMonotonicIdentifier, +} + +impl ElectoralSystemState { + pub fn load_state() -> Self { + Self { + unsynchronised_settings: MockStorageAccess::unsynchronised_settings::(), + unsynchronised_state: MockStorageAccess::unsynchronised_state::(), + unsynchronised_state_map: MockStorageAccess::raw_unsynchronised_state_map::(), + election_identifiers: MockStorageAccess::election_identifiers::(), + next_umi: MockStorageAccess::next_umi(), + } + } +} + +type CheckFn = Box, &ElectoralSystemState)>; /// Checks that can be applied post-finalization. pub struct Check { @@ -331,11 +355,17 @@ pub struct Check { } impl Check { - pub fn new(check_fn: impl Fn(&MockAccess, &MockAccess) + 'static) -> Self { + pub fn new( + check_fn: impl Fn(&ElectoralSystemState, &ElectoralSystemState) + 'static, + ) -> Self { Self { check_fn: Box::new(check_fn) } } - pub fn check(&self, pre_finalize: &MockAccess, post_finalize: &MockAccess) { + pub fn check( + &self, + pre_finalize: &ElectoralSystemState, + post_finalize: &ElectoralSystemState, + ) { (self.check_fn)(pre_finalize, post_finalize) } } 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 74500c97f7..adae194de9 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 @@ -5,55 +5,23 @@ use crate::{ }, CorruptStorageError, ElectionIdentifier, UniqueMonotonicIdentifier, }; -use codec::Encode; -use frame_support::{ - ensure, CloneNoBound, DebugNoBound, EqNoBound, PartialEqNoBound, StorageHasher, Twox64Concat, -}; +use codec::{Decode, Encode}; +use core::cell::RefCell; +use frame_support::{ensure, CloneNoBound, DebugNoBound, EqNoBound, PartialEqNoBound}; use std::collections::BTreeMap; -pub struct MockReadAccess<'es, ES: ElectoralSystem> { - election_identifier: ElectionIdentifierOf, - electoral_system: &'es MockAccess, -} -pub struct MockWriteAccess<'es, ES: ElectoralSystem> { +// TODO: Create a new() so no need to pass mock storage access in each time. same below. +pub struct MockReadAccess { election_identifier: ElectionIdentifierOf, - electoral_system: &'es mut MockAccess, } -#[derive(CloneNoBound, DebugNoBound, PartialEqNoBound, EqNoBound)] -pub struct MockElection { - properties: ES::ElectionProperties, - state: ES::ElectionState, - settings: ES::ElectoralSettings, - consensus_status: ConsensusStatus, +pub struct MockWriteAccess { + election_identifier: ElectionIdentifierOf, } #[derive(CloneNoBound, DebugNoBound, PartialEqNoBound, EqNoBound)] pub struct MockAccess { - electoral_settings: ES::ElectoralSettings, - unsynchronised_state: ES::ElectoralUnsynchronisedState, - unsynchronised_state_map: BTreeMap, Option>, - elections: BTreeMap, MockElection>, - unsynchronised_settings: ES::ElectoralUnsynchronisedSettings, - next_election_id: UniqueMonotonicIdentifier, -} - -impl MockAccess { - fn election_read_access( - &self, - id: ElectionIdentifierOf, - ) -> Result, CorruptStorageError> { - ensure!(self.elections.contains_key(&id), CorruptStorageError::new()); - Ok(MockReadAccess { election_identifier: id, electoral_system: self }) - } - - fn election_write_access( - &mut self, - id: ElectionIdentifierOf, - ) -> Result, CorruptStorageError> { - ensure!(self.elections.contains_key(&id), CorruptStorageError::new()); - Ok(MockWriteAccess { election_identifier: id, electoral_system: self }) - } + _phantom: core::marker::PhantomData, } macro_rules! impl_read_access { @@ -67,7 +35,7 @@ macro_rules! impl_read_access { ::ElectoralSettings, CorruptStorageError, > { - self.with_election(|e| e.settings.clone()) + Ok(MockStorageAccess::electoral_settings_for_election::(self.identifier())) } fn properties( @@ -76,7 +44,7 @@ macro_rules! impl_read_access { ::ElectionProperties, CorruptStorageError, > { - self.with_election(|e| e.properties.clone()) + Ok(MockStorageAccess::election_properties::(self.identifier())) } fn state( @@ -85,27 +53,15 @@ macro_rules! impl_read_access { ::ElectionState, CorruptStorageError, > { - self.with_election(|e| e.state.clone()) + Ok(MockStorageAccess::election_state::(self.identifier())) } - fn election_identifier( - &self, - ) -> Result, CorruptStorageError> { - Ok(self.identifier()) + fn election_identifier(&self) -> ElectionIdentifierOf { + self.identifier() } } impl $t { - fn with_election) -> R, R>( - &self, - f: F, - ) -> Result { - self.electoral_system - .elections - .get(&self.identifier()) - .map(f) - .ok_or_else(CorruptStorageError::new) - } pub fn identifier(&self) -> ElectionIdentifierOf { self.election_identifier } @@ -114,183 +70,386 @@ macro_rules! impl_read_access { previous_consensus: Option<&ES::Consensus>, votes: ConsensusVotes, ) -> Result, CorruptStorageError> { - ES::check_consensus(self.identifier(), self, previous_consensus, votes) + println!("Calling check consensus on the electoral system struct"); + ES::check_consensus(self, previous_consensus, votes) } } }; } -impl_read_access!(MockReadAccess<'_, ES>); -impl_read_access!(MockWriteAccess<'_, ES>); +impl_read_access!(MockReadAccess); +impl_read_access!(MockWriteAccess); -impl MockWriteAccess<'_, ES> { - fn with_election_mut) -> R, R>( - &mut self, - f: F, - ) -> Result { - self.electoral_system - .elections - .get_mut(&self.identifier()) - .map(f) - .ok_or_else(CorruptStorageError::new) - } - pub fn set_consensus_status(&mut self, consensus_status: ConsensusStatus) { - self.with_election_mut(|e| e.consensus_status = consensus_status) - .expect("Cannot set consensus status for non-existent election"); - } +thread_local! { + pub static ELECTION_STATE: RefCell, Vec>> = const { RefCell::new(BTreeMap::new()) }; + pub static ELECTION_PROPERTIES: RefCell, Vec>> = const { RefCell::new(BTreeMap::new()) }; + pub static ELECTORAL_SETTINGS: RefCell> = const { RefCell::new(Vec::new()) }; + // The electoral settings for a particular election + pub static ELECTION_SETTINGS: RefCell, Vec>> = const { RefCell::new(BTreeMap::new()) }; + pub static ELECTORAL_UNSYNCHRONISED_SETTINGS: RefCell> = const { RefCell::new(Vec::new()) }; + pub static ELECTORAL_UNSYNCHRONISED_STATE: RefCell> = const { RefCell::new(Vec::new()) }; + pub static ELECTORAL_UNSYNCHRONISED_STATE_MAP: RefCell, Option>>> = const { RefCell::new(BTreeMap::new()) }; + pub static CONSENSUS_STATUS: RefCell, Vec>> = const { RefCell::new(BTreeMap::new()) }; + pub static NEXT_ELECTION_ID: RefCell = const { RefCell::new(UniqueMonotonicIdentifier::from_u64(0)) }; } -impl ElectionWriteAccess for MockWriteAccess<'_, ES> { +impl ElectionWriteAccess for MockWriteAccess { fn set_state( - &mut self, + &self, state: ::ElectionState, ) -> Result<(), CorruptStorageError> { - self.with_election_mut(|e| e.state = state)?; + MockStorageAccess::set_state::(self.identifier(), state); Ok(()) } - fn clear_votes(&mut self) { + fn clear_votes(&self) { // nothing } fn delete(self) { - self.electoral_system.elections.remove(&self.identifier()); + MockStorageAccess::delete_election::(self.identifier()); } fn refresh( &mut self, - _extra: ::ElectionIdentifierExtra, + new_extra: ::ElectionIdentifierExtra, properties: ::ElectionProperties, ) -> Result<(), CorruptStorageError> { - self.with_election_mut(|e| e.properties = properties)?; + self.election_identifier = self.election_identifier.with_extra(new_extra); + MockStorageAccess::set_election_properties::(self.identifier(), properties); Ok(()) } fn check_consensus( - &mut self, + &self, ) -> Result< ConsensusStatus<::Consensus>, CorruptStorageError, > { - self.with_election_mut(|e| e.consensus_status.clone()) - } -} - -impl MockAccess { - pub fn new( - unsynchronised_state: ES::ElectoralUnsynchronisedState, - unsynchronised_settings: ES::ElectoralUnsynchronisedSettings, - electoral_settings: ES::ElectoralSettings, - ) -> Self { - Self { - electoral_settings, - unsynchronised_state, - unsynchronised_settings, - unsynchronised_state_map: Default::default(), - elections: Default::default(), - next_election_id: Default::default(), - } - } - - pub fn finalize_elections( - &mut self, - context: &ES::OnFinalizeContext, - ) -> Result { - ES::on_finalize(self, self.elections.keys().cloned().collect(), context) - } - - pub fn election_identifiers(&self) -> Vec> { - self.elections.keys().cloned().collect() - } - - pub fn next_umi(&self) -> UniqueMonotonicIdentifier { - self.next_election_id + Ok(MockStorageAccess::consensus_status::(self.identifier())) } } impl ElectoralReadAccess for MockAccess { type ElectoralSystem = ES; - type ElectionReadAccess<'es> = MockReadAccess<'es, ES>; + type ElectionReadAccess = MockReadAccess; - fn election( - &self, - id: ElectionIdentifierOf, - ) -> Result, CorruptStorageError> { - self.election_read_access(id) + fn election(id: ElectionIdentifierOf) -> Self::ElectionReadAccess { + MockReadAccess { election_identifier: id } } - fn unsynchronised_settings( - &self, - ) -> Result< + fn unsynchronised_settings() -> Result< ::ElectoralUnsynchronisedSettings, CorruptStorageError, > { - Ok(self.unsynchronised_settings.clone()) + Ok(MockStorageAccess::unsynchronised_settings::()) } - fn unsynchronised_state( - &self, - ) -> Result< + fn unsynchronised_state() -> Result< ::ElectoralUnsynchronisedState, CorruptStorageError, > { - Ok(self.unsynchronised_state.clone()) + Ok(MockStorageAccess::unsynchronised_state::()) } fn unsynchronised_state_map( - &self, key: &::ElectoralUnsynchronisedStateMapKey, ) -> Result< Option<::ElectoralUnsynchronisedStateMapValue>, CorruptStorageError, > { - self.unsynchronised_state_map - .get(&key.using_encoded(Twox64Concat::hash)) - .ok_or_else(CorruptStorageError::new) - .cloned() + Ok(MockStorageAccess::unsynchronised_state_map::(key)) } } impl ElectoralWriteAccess for MockAccess { - type ElectionWriteAccess<'a> = MockWriteAccess<'a, ES>; + type ElectionWriteAccess = MockWriteAccess; fn new_election( - &mut self, extra: ::ElectionIdentifierExtra, properties: ::ElectionProperties, state: ::ElectionState, - ) -> Result, CorruptStorageError> { - let election_identifier = ElectionIdentifier::new(self.next_election_id, extra); - self.next_election_id = self.next_election_id.next_identifier().unwrap(); - self.elections.insert( - election_identifier, - MockElection { - properties, - state, - settings: self.electoral_settings.clone(), - consensus_status: ConsensusStatus::None, - }, - ); - self.election_write_access(election_identifier) + ) -> Result { + Ok(Self::election_mut(MockStorageAccess::new_election::(extra, properties, state))) } - fn election_mut( - &mut self, - id: ElectionIdentifierOf, - ) -> Result, CorruptStorageError> { - self.election_write_access(id) + fn election_mut(id: ElectionIdentifierOf) -> Self::ElectionWriteAccess { + MockWriteAccess { election_identifier: id } } fn set_unsynchronised_state( - &mut self, unsynchronised_state: ::ElectoralUnsynchronisedState, ) -> Result<(), CorruptStorageError> { - self.unsynchronised_state = unsynchronised_state; + MockStorageAccess::set_unsynchronised_state::(unsynchronised_state); Ok(()) } /// Inserts or removes a value from the unsynchronised state map of the electoral system. fn set_unsynchronised_state_map( - &mut self, key: ::ElectoralUnsynchronisedStateMapKey, value: Option< ::ElectoralUnsynchronisedStateMapValue, >, ) -> Result<(), CorruptStorageError> { - self.unsynchronised_state_map - .insert(key.using_encoded(Twox64Concat::hash), value); + MockStorageAccess::set_unsynchronised_state_map::(key, value); Ok(()) } } + +pub struct MockStorageAccess; + +impl MockStorageAccess { + pub fn clear_storage() { + ELECTION_STATE.with(|state| { + let mut state_ref = state.borrow_mut(); + state_ref.clear(); + }); + ELECTION_PROPERTIES.with(|properties| { + let mut properties_ref = properties.borrow_mut(); + properties_ref.clear(); + }); + ELECTION_SETTINGS.with(|settings| { + let mut settings_ref = settings.borrow_mut(); + settings_ref.clear(); + }); + ELECTORAL_UNSYNCHRONISED_SETTINGS.with(|settings| { + let mut settings_ref = settings.borrow_mut(); + settings_ref.clear(); + }); + ELECTORAL_UNSYNCHRONISED_STATE.with(|state| { + let mut state_ref = state.borrow_mut(); + state_ref.clear(); + }); + ELECTORAL_UNSYNCHRONISED_STATE_MAP.with(|state_map| { + let mut state_map_ref = state_map.borrow_mut(); + state_map_ref.clear(); + }); + CONSENSUS_STATUS.with(|consensus| { + let mut consensus_ref = consensus.borrow_mut(); + consensus_ref.clear(); + }); + NEXT_ELECTION_ID.with(|next_id| { + let mut next_id_ref = next_id.borrow_mut(); + *next_id_ref = UniqueMonotonicIdentifier::from_u64(0); + }); + } + + pub fn next_umi() -> UniqueMonotonicIdentifier { + NEXT_ELECTION_ID.with(|next_id| next_id.borrow().clone()) + } + + pub fn increment_next_umi() { + NEXT_ELECTION_ID.with(|next_id| { + let mut next_id_ref = next_id.borrow_mut(); + *next_id_ref = next_id_ref.next_identifier().unwrap(); + }); + } + + pub fn set_electoral_settings( + settings: ::ElectoralSettings, + ) { + ELECTORAL_SETTINGS.with(|old_settings| { + let mut settings_ref = old_settings.borrow_mut(); + *settings_ref = settings.encode(); + }); + } + + pub fn electoral_settings() -> ES::ElectoralSettings { + ELECTORAL_SETTINGS.with(|settings| { + let settings_ref = settings.borrow(); + ES::ElectoralSettings::decode(&mut &settings_ref[..]).unwrap() + }) + } + + pub fn set_electoral_settings_for_election( + identifier: ElectionIdentifierOf, + settings: ::ElectoralSettings, + ) { + ELECTION_SETTINGS.with(|old_settings| { + let mut settings_ref = old_settings.borrow_mut(); + settings_ref.insert(identifier.encode(), settings.encode()); + }); + } + + pub fn electoral_settings_for_election( + identifier: ElectionIdentifierOf, + ) -> ::ElectoralSettings { + ELECTION_SETTINGS.with(|settings| { + let settings_ref = settings.borrow(); + settings_ref + .get(&identifier.encode()) + .clone() + .map(|v| ES::ElectoralSettings::decode(&mut &v[..]).unwrap()) + .unwrap() + }) + } + + pub fn delete_election(identifier: ElectionIdentifierOf) { + ELECTION_PROPERTIES.with(|properties| { + let mut properties_ref = properties.borrow_mut(); + properties_ref.remove(&identifier.encode()); + }); + ELECTION_STATE.with(|state| { + let mut state_ref = state.borrow_mut(); + state_ref.remove(&identifier.encode()); + }); + } + + pub fn set_state( + identifier: ElectionIdentifierOf, + state: ES::ElectionState, + ) { + println!("Setting election state for identifier: {:?}", identifier); + ELECTION_STATE.with(|old_state| { + let mut state_ref = old_state.borrow_mut(); + state_ref.insert(identifier.encode(), state.encode()); + }); + } + + pub fn election_state( + identifier: ElectionIdentifierOf, + ) -> ES::ElectionState { + ELECTION_STATE.with(|old_state| { + let state_ref = old_state.borrow(); + state_ref + .get(&identifier.encode()) + .map(|v| ES::ElectionState::decode(&mut &v[..]).unwrap()) + .unwrap() + }) + } + + pub fn set_election_properties( + identifier: ElectionIdentifierOf, + properties: ES::ElectionProperties, + ) { + ELECTION_PROPERTIES.with(|old_properties| { + let mut properties_ref = old_properties.borrow_mut(); + properties_ref.insert(identifier.encode(), properties.encode()); + }); + } + + pub fn election_properties( + identifier: ElectionIdentifierOf, + ) -> ES::ElectionProperties { + ELECTION_PROPERTIES.with(|old_properties| { + let properties_ref = old_properties.borrow(); + properties_ref + .get(&identifier.encode()) + .map(|v| ES::ElectionProperties::decode(&mut &v[..]).unwrap()) + .unwrap() + }) + } + + pub fn set_unsynchronised_state( + unsynchronised_state: ES::ElectoralUnsynchronisedState, + ) { + ELECTORAL_UNSYNCHRONISED_STATE.with(|old_state| { + let mut state_ref = old_state.borrow_mut(); + state_ref.clear(); + state_ref.extend_from_slice(&unsynchronised_state.encode()); + }); + } + + pub fn set_unsynchronised_settings( + unsynchronised_settings: ES::ElectoralUnsynchronisedSettings, + ) { + ELECTORAL_UNSYNCHRONISED_SETTINGS.with(|old_settings| { + let mut settings_ref = old_settings.borrow_mut(); + settings_ref.clear(); + settings_ref.extend_from_slice(&unsynchronised_settings.encode()); + }); + } + + pub fn unsynchronised_settings() -> ES::ElectoralUnsynchronisedSettings { + ELECTORAL_UNSYNCHRONISED_SETTINGS.with(|old_settings| { + let settings_ref = old_settings.borrow(); + ES::ElectoralUnsynchronisedSettings::decode(&mut &settings_ref[..]).unwrap() + }) + } + + pub fn unsynchronised_state() -> ES::ElectoralUnsynchronisedState { + ELECTORAL_UNSYNCHRONISED_STATE.with(|old_state| { + let state_ref = old_state.borrow(); + ES::ElectoralUnsynchronisedState::decode(&mut &state_ref[..]).unwrap() + }) + } + + pub fn unsynchronised_state_map( + key: &ES::ElectoralUnsynchronisedStateMapKey, + ) -> Option { + ELECTORAL_UNSYNCHRONISED_STATE_MAP.with(|old_state_map| { + let state_map_ref = old_state_map.borrow(); + state_map_ref + .get(&key.encode()) + .expect("Key should exist") + .clone() + .map(|v| ES::ElectoralUnsynchronisedStateMapValue::decode(&mut &v[..]).unwrap()) + }) + } + + pub fn raw_unsynchronised_state_map() -> BTreeMap, Option>> + { + ELECTORAL_UNSYNCHRONISED_STATE_MAP.with(|old_state_map| { + let state_map_ref = old_state_map.borrow(); + state_map_ref.clone() + }) + } + + pub fn set_unsynchronised_state_map( + key: ES::ElectoralUnsynchronisedStateMapKey, + value: Option, + ) { + ELECTORAL_UNSYNCHRONISED_STATE_MAP.with(|old_state_map| { + let mut state_map_ref = old_state_map.borrow_mut(); + state_map_ref.insert(key.encode(), value.map(|v| v.encode())); + }); + } + + pub fn election_identifiers() -> Vec> { + ELECTION_PROPERTIES.with(|properties| { + let properties_ref = properties.borrow(); + properties_ref + .keys() + .map(|k| ElectionIdentifierOf::::decode(&mut &k[..]).unwrap()) + .collect() + }) + } + + pub fn set_consensus_status( + identifier: ElectionIdentifierOf, + status: ConsensusStatus, + ) { + println!("Setting consensus status to {:?} for {:?}", status, identifier); + CONSENSUS_STATUS.with(|old_consensus| { + let mut consensus_ref = old_consensus.borrow_mut(); + consensus_ref.insert(identifier.encode(), status.encode()); + }); + } + + pub fn consensus_status( + identifier: ElectionIdentifierOf, + ) -> ConsensusStatus { + CONSENSUS_STATUS + .with(|old_consensus| { + let consensus_ref = old_consensus.borrow(); + consensus_ref + .get(&identifier.encode()) + .map(|v| ConsensusStatus::::decode(&mut &v[..]).unwrap()) + }) + .unwrap_or(ConsensusStatus::None) + } + + pub fn new_election( + extra: ::ElectionIdentifierExtra, + properties: ::ElectionProperties, + state: ::ElectionState, + ) -> ElectionIdentifierOf { + let next_umi = Self::next_umi(); + let election_identifier = ElectionIdentifier::new(next_umi, extra); + Self::increment_next_umi(); + + Self::set_election_properties::(election_identifier, properties); + Self::set_state::(election_identifier, state); + // These are normally stored once and synchronised by election identifier. In the tests we + // simplify this by just storing the electoral settings (that would be fetched by + // resolving the synchronisation) alongside the election. + Self::set_electoral_settings_for_election::( + election_identifier, + Self::electoral_settings::(), + ); + + election_identifier + } +} diff --git a/state-chain/pallets/cf-elections/src/electoral_systems/tests/liveness.rs b/state-chain/pallets/cf-elections/src/electoral_systems/tests/liveness.rs index e602553afd..9c3767ce8f 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/tests/liveness.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/tests/liveness.rs @@ -35,15 +35,12 @@ type SimpleLiveness = register_checks! { SimpleLiveness { only_one_election(_pre, post) { - let election_ids = post.election_identifiers(); - assert_eq!(election_ids.len(), 1, "Only one election should exist."); + assert_eq!(post.election_identifiers.len(), 1, "Only one election should exist."); }, hook_called_once(_pre, _post) { - assert_eq!(HOOK_CALLED_COUNT.with(|hook_called| hook_called.get()), 1, "Hook should have been called once so far!"); - }, + assert_eq!(HOOK_CALLED_COUNT.with(|hook_called| hook_called.get()), 1, "Hook should have been called once so far!"); }, hook_called_twice(_pre, _post) { - assert_eq!(HOOK_CALLED_COUNT.with(|hook_called| hook_called.get()), 2, "Hook should have been called twice so far!"); - }, + assert_eq!(HOOK_CALLED_COUNT.with(|hook_called| hook_called.get()), 2, "Hook should have been called twice so far!"); }, hook_not_called(_pre, _post) { assert_eq!( HOOK_CALLED_COUNT.with(|hook_called| hook_called.get()), diff --git a/state-chain/pallets/cf-elections/src/electoral_systems/tests/monotonic_median.rs b/state-chain/pallets/cf-elections/src/electoral_systems/tests/monotonic_median.rs index 3628ab180b..449ae61ab2 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/tests/monotonic_median.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/tests/monotonic_median.rs @@ -1,5 +1,5 @@ use super::{ - mocks::{Check, TestContext, TestSetup}, + mocks::{Check, MockAccess, TestContext, TestSetup}, register_checks, }; use crate::{ @@ -43,8 +43,7 @@ impl MockHook { register_checks! { MonotonicMedianTest { monotonically_increasing_state(pre_finalize, post_finalize) { - assert!(post_finalize.unsynchronised_state().unwrap() >= pre_finalize.unsynchronised_state().unwrap(), - "Unsynchronised state can not decrease!"); + assert!(post_finalize.unsynchronised_state >= pre_finalize.unsynchronised_state, "Unsynchronised state can not decrease!"); }, hook_called(_pre, _post) { assert!(MockHook::has_been_called(), "Hook should have been called!"); @@ -90,7 +89,7 @@ fn too_few_votes_consensus_not_possible() { #[test] fn finalize_election_with_incremented_state() { let test = with_default_context(); - let initial_state = test.access().unsynchronised_state().unwrap(); + let initial_state = MockAccess::::unsynchronised_state().unwrap(); let new_unsynchronised_state = initial_state + 1; test.force_consensus_update(ConsensusStatus::Gained { @@ -109,8 +108,8 @@ fn finalize_election_with_incremented_state() { Check::monotonically_increasing_state(), Check::::hook_called(), Check::new(move |pre, post| { - assert_eq!(pre.unsynchronised_state().unwrap(), initial_state); - assert_eq!(post.unsynchronised_state().unwrap(), new_unsynchronised_state); + assert_eq!(pre.unsynchronised_state, initial_state); + assert_eq!(post.unsynchronised_state, new_unsynchronised_state); }), Check::last_election_deleted(), Check::election_id_incremented(), @@ -148,8 +147,8 @@ fn finalize_election_state_can_not_decrease() { // The hook should not be called if the state is not updated. Check::::hook_not_called(), Check::new(|pre, post| { - assert_eq!(pre.unsynchronised_state().unwrap(), INTITIAL_STATE); - assert_eq!(post.unsynchronised_state().unwrap(), INTITIAL_STATE); + assert_eq!(pre.unsynchronised_state, INTITIAL_STATE); + assert_eq!(post.unsynchronised_state, INTITIAL_STATE); }), Check::last_election_deleted(), Check::election_id_incremented(), diff --git a/state-chain/pallets/cf-elections/src/electoral_systems/tests/unsafe_median.rs b/state-chain/pallets/cf-elections/src/electoral_systems/tests/unsafe_median.rs index 28f07e626c..dff2e35e31 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/tests/unsafe_median.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/tests/unsafe_median.rs @@ -2,7 +2,7 @@ use cf_primitives::AuthorityCount; use super::{mocks::*, register_checks}; use crate::{ - electoral_system::{ConsensusStatus, ElectoralReadAccess}, + electoral_system::ConsensusStatus, electoral_systems::{tests::utils::generate_votes, unsafe_median::*}, }; @@ -21,23 +21,23 @@ fn with_default_context() -> TestContext { register_checks! { SimpleUnsafeMedian { - started_at_initial_state(pre_finalize, _post) { + started_at_initial_state(pre_finalize, _a) { assert_eq!( - pre_finalize.unsynchronised_state().unwrap(), + pre_finalize.unsynchronised_state, INIT_UNSYNCHRONISED_STATE, "Expected initial state pre-finalization." ); }, ended_at_initial_state(_pre, post_finalize) { assert_eq!( - post_finalize.unsynchronised_state().unwrap(), + post_finalize.unsynchronised_state, INIT_UNSYNCHRONISED_STATE, "Expected initial state post-finalization." ); }, ended_at_new_state(_pre, post_finalize) { assert_eq!( - post_finalize.unsynchronised_state().unwrap(), + post_finalize.unsynchronised_state, NEW_UNSYNCHRONISED_STATE, "Expected new state post-finalization." ); diff --git a/state-chain/pallets/cf-elections/src/lib.rs b/state-chain/pallets/cf-elections/src/lib.rs index a714ed09f8..b2f7190896 100644 --- a/state-chain/pallets/cf-elections/src/lib.rs +++ b/state-chain/pallets/cf-elections/src/lib.rs @@ -145,8 +145,8 @@ pub mod pallet { }; use bitmap_components::ElectionBitmapComponents; pub use electoral_system_runner::{ - AuthorityVoteOf, CompositeElectionIdentifierOf, ElectoralSystemRunner, - IndividualComponentOf, VotePropertiesOf, + CompositeAuthorityVoteOf, CompositeElectionIdentifierOf, CompositeIndividualComponentOf, + CompositeVotePropertiesOf, ElectoralSystemRunner, }; use frame_support::{ @@ -192,7 +192,7 @@ pub mod pallet { CompositeElectionIdentifierOf, ::ElectoralSettings, ::ElectionProperties, - AuthorityVoteOf, + CompositeAuthorityVoteOf, BlockNumberFor, >; @@ -208,8 +208,8 @@ pub mod pallet { self.0.checked_add(1).map(Self) } - #[cfg(feature = "runtime-benchmarks")] - pub fn from_u64(value: u64) -> Self { + #[cfg(any(feature = "runtime-benchmarks", test))] + pub const fn from_u64(value: u64) -> Self { Self(value) } } @@ -469,8 +469,8 @@ pub mod pallet { Identity, T::ValidatorId, ( - VotePropertiesOf, - IndividualComponentOf, + CompositeVotePropertiesOf, + CompositeIndividualComponentOf, ), OptionQuery, >; @@ -1223,7 +1223,7 @@ pub mod pallet { origin: OriginFor, authority_votes: BoundedBTreeMap< CompositeElectionIdentifierOf, - AuthorityVoteOf, + CompositeAuthorityVoteOf, ConstU32, >, ) -> DispatchResult { @@ -1675,12 +1675,8 @@ pub mod pallet { } } - /// Provides access into the ElectoralSystem's storage, and also all the current election + /// Provides access into the ElectoralSystem's current election /// identifiers. - /// - /// Ideally we would avoid introducing re-entrance (also with `with_storage_access`), so - /// the ElectoralAccess object can internally cache storage which is possibly helpful - /// particularly in the case of composites. pub fn with_election_identifiers< R, F: FnOnce( @@ -1922,8 +1918,8 @@ pub mod pallet { R, F: for<'a> FnOnce( Option<( - VotePropertiesOf, - AuthorityVoteOf, + CompositeVotePropertiesOf, + CompositeAuthorityVoteOf, )>, &'a mut ElectionBitmapComponents, ) -> Result, @@ -1981,8 +1977,8 @@ pub mod pallet { mut visit_unprovided_shared_data: VisitUnprovidedSharedData, ) -> Result< Option<( - VotePropertiesOf, - AuthorityVoteOf, + CompositeVotePropertiesOf, + CompositeAuthorityVoteOf, )>, CorruptStorageError, > { diff --git a/state-chain/pallets/cf-elections/src/mock.rs b/state-chain/pallets/cf-elections/src/mock.rs index f58ba4aeed..dd67d7257e 100644 --- a/state-chain/pallets/cf-elections/src/mock.rs +++ b/state-chain/pallets/cf-elections/src/mock.rs @@ -24,7 +24,7 @@ impl pallet_cf_elections::Config for Test { type RuntimeEvent = RuntimeEvent; // TODO: Use Settings? - type ElectoralSystemRunner = crate::electoral_systems::mock::MockElectoralSystem; + type ElectoralSystemRunner = crate::electoral_systems::mock::MockElectoralSystemRunner; type WeightInfo = (); } diff --git a/state-chain/pallets/cf-elections/src/tests.rs b/state-chain/pallets/cf-elections/src/tests.rs index 48673b1d7b..020f95028d 100644 --- a/state-chain/pallets/cf-elections/src/tests.rs +++ b/state-chain/pallets/cf-elections/src/tests.rs @@ -4,7 +4,11 @@ use cf_primitives::AuthorityCount; use electoral_system::{ AuthorityVoteOf, ConsensusStatus, ElectionReadAccess, ElectoralReadAccess, ElectoralWriteAccess, }; -use electoral_systems::mock::{BehaviourUpdate, MockElectoralSystem}; +use electoral_system_runner::RunnerStorageAccessTrait; +use electoral_systems::{ + mock::{BehaviourUpdate, MockElectoralSystemRunner}, + mocks::MockStorageAccess, +}; use frame_support::traits::OriginTrait; use mock::Test; use std::collections::BTreeMap; @@ -117,7 +121,7 @@ fn ensure_can_vote() { }); } -pub trait ElectoralSystemTestExt: Sized { +pub trait ElectoralSystemRunnerTestExt: Sized { fn update_settings(self, updates: &[BehaviourUpdate]) -> Self; fn expect_consensus_after_next_block(self, expected: ConsensusStatus) -> Self; fn assume_consensus(self) -> Self; @@ -127,39 +131,28 @@ pub trait ElectoralSystemTestExt: Sized { fn submit_votes( self, validator_ids: &[u64], - vote: AuthorityVoteOf, + vote: CompositeAuthorityVoteOf, expected_outcome: Result<(), Error>, ) -> Self where - Test: Config, + Test: Config, ::RuntimeCall: From>; } -impl ElectoralSystemTestExt for TestRunner { +impl ElectoralSystemRunnerTestExt for TestRunner { /// Starts a new election, adding its unique monotonic identifier to the test context. #[track_caller] fn new_election(self) -> Self { self.then_execute_with( #[track_caller] |mut ctx| { - let unique_monotonic_identifier = - *Pallet::::with_storage_access(|electoral_access| { - electoral_access.new_election((), (), ()) - }) - .expect("New election should not corrupt storage.") - .election_identifier() - .expect("New election should have an identifier.") - .unique_monotonic(); + let identifier = + RunnerStorageAccess::::new_election((), (), ()).unwrap(); + let unique_monotonic_identifier = identifier.unique_monotonic(); assert_eq!(Status::::get(), Some(ElectionPalletStatus::Running)); - Pallet::::with_storage_access(|electoral_access| { - electoral_access - .election(ElectionIdentifier::new(unique_monotonic_identifier, ())) - }) - .expect("Expected an initial election."); - - ctx.umis.push(unique_monotonic_identifier); + ctx.umis.push(*unique_monotonic_identifier); ctx }, @@ -167,7 +160,7 @@ impl ElectoralSystemTestExt for TestRunner { } fn update_settings(self, updates: &[BehaviourUpdate]) -> Self { - MockElectoralSystem::update(updates); + MockElectoralSystemRunner::update(updates); self } @@ -185,11 +178,11 @@ impl ElectoralSystemTestExt for TestRunner { fn submit_votes( self, validator_ids: &[u64], - vote: AuthorityVoteOf, + vote: CompositeAuthorityVoteOf, expected_outcome: Result<(), Error>, ) -> Self where - Test: Config, + Test: Config, ::RuntimeCall: From>, { self.then_apply_extrinsics( @@ -204,7 +197,9 @@ impl ElectoralSystemTestExt for TestRunner { Call::::vote { authority_votes: BoundedBTreeMap::try_from( sp_std::iter::once(( - ElectionIdentifier::new(*umi, ()), + CompositeElectionIdentifierOf::< + MockElectoralSystemRunner, + >::new(*umi, ()), vote.clone(), )) .collect::>(), @@ -228,7 +223,7 @@ impl ElectoralSystemTestExt for TestRunner { assert!(!umis.is_empty(), "Asserted consensus on empty election set."); for umi in umis { - let actual = MockElectoralSystem::consensus_status(*umi); + let actual = MockElectoralSystemRunner::consensus_status(*umi); assert_eq!( actual, expected, @@ -249,7 +244,7 @@ impl ElectoralSystemTestExt for TestRunner { #[test] fn consensus_state_transitions() { - const VOTE: AuthorityVoteOf = AuthorityVote::Vote(()); + const VOTE: CompositeAuthorityVoteOf = AuthorityVote::Vote(()); election_test_ext(TestSetup { num_non_contributing_authorities: 2, ..Default::default() }) .new_election() @@ -301,7 +296,7 @@ fn consensus_state_transitions() { #[test] fn authority_removes_and_re_adds_itself_from_contributing_set() { - const VOTE: AuthorityVoteOf = AuthorityVote::Vote(()); + const VOTE: CompositeAuthorityVoteOf = AuthorityVote::Vote(()); election_test_ext(Default::default()) .new_election() diff --git a/state-chain/runtime/src/chainflip/solana_elections.rs b/state-chain/runtime/src/chainflip/solana_elections.rs index 76ccc1d2d5..2a08895f7f 100644 --- a/state-chain/runtime/src/chainflip/solana_elections.rs +++ b/state-chain/runtime/src/chainflip/solana_elections.rs @@ -263,42 +263,38 @@ impl ), ) -> Result<(), CorruptStorageError> { let block_height = SolanaBlockHeightTracking::on_finalize::< - CompositeElectoralAccess< + DerivedElectoralAccess< _, SolanaBlockHeightTracking, RunnerStorageAccess, >, >(block_height_identifiers, &())?; SolanaLiveness::on_finalize::< - CompositeElectoralAccess< - _, - SolanaLiveness, - RunnerStorageAccess, - >, + DerivedElectoralAccess<_, SolanaLiveness, RunnerStorageAccess>, >(liveness_identifiers, &(crate::System::block_number(), block_height))?; SolanaFeeTracking::on_finalize::< - CompositeElectoralAccess< + DerivedElectoralAccess< _, SolanaFeeTracking, RunnerStorageAccess, >, >(fee_identifiers, &())?; SolanaNonceTracking::on_finalize::< - CompositeElectoralAccess< + DerivedElectoralAccess< _, SolanaNonceTracking, RunnerStorageAccess, >, >(nonce_tracking_identifiers, &())?; SolanaEgressWitnessing::on_finalize::< - CompositeElectoralAccess< + DerivedElectoralAccess< _, SolanaEgressWitnessing, RunnerStorageAccess, >, >(egress_witnessing_identifiers, &())?; SolanaIngressTracking::on_finalize::< - CompositeElectoralAccess< + DerivedElectoralAccess< _, SolanaIngressTracking, RunnerStorageAccess, @@ -336,12 +332,12 @@ impl BenchmarkValue for SolanaIngressSettings { } } -use pallet_cf_elections::electoral_systems::composite::tuple_6_impls::CompositeElectoralAccess; +use pallet_cf_elections::electoral_systems::composite::tuple_6_impls::DerivedElectoralAccess; pub struct SolanaChainTrackingProvider; impl GetBlockHeight for SolanaChainTrackingProvider { fn get_block_height() -> ::ChainBlockNumber { - CompositeElectoralAccess::< + DerivedElectoralAccess::< _, SolanaBlockHeightTracking, RunnerStorageAccess, @@ -358,7 +354,7 @@ impl GetBlockHeight for SolanaChainTrackingProvider { impl SolanaChainTrackingProvider { pub fn priority_fee() -> Option<::ChainAmount> { - CompositeElectoralAccess::< + DerivedElectoralAccess::< _, SolanaFeeTracking, RunnerStorageAccess, @@ -371,13 +367,13 @@ impl SolanaChainTrackingProvider { >( f: F, ) -> ::ChainAmount { - CompositeElectoralAccess::< + DerivedElectoralAccess::< _, SolanaFeeTracking, RunnerStorageAccess, >::unsynchronised_state() .and_then(|priority_fee| { - CompositeElectoralAccess::< + DerivedElectoralAccess::< _, SolanaFeeTracking, RunnerStorageAccess, @@ -425,7 +421,7 @@ impl IngressSource for SolanaIngress { |grouped_election_identifiers| { let (_, _, election_identifiers, ..) = grouped_election_identifiers; SolanaIngressTracking::open_channel::< - CompositeElectoralAccess< + DerivedElectoralAccess< _, SolanaIngressTracking, RunnerStorageAccess, @@ -450,7 +446,7 @@ impl SolanaNonceWatch for SolanaNonceTrackingTrigger { // TODO: Look at handling the corrupt storage elsewhere. Ok(pallet_cf_elections::Pallet::::handle_corrupt_storage( SolanaNonceTracking::watch_for_change::< - CompositeElectoralAccess< + DerivedElectoralAccess< _, SolanaNonceTracking, RunnerStorageAccess, @@ -470,7 +466,7 @@ impl ElectionEgressWitnesser for SolanaEgressWitnessingTrigger { // with_electoral_access Ok(pallet_cf_elections::Pallet::::handle_corrupt_storage( SolanaEgressWitnessing::watch_for_egress::< - CompositeElectoralAccess< + DerivedElectoralAccess< _, SolanaEgressWitnessing, RunnerStorageAccess, From b54f6a5fee8e7be77b62726cfa10050daea7252a Mon Sep 17 00:00:00 2001 From: kylezs Date: Wed, 23 Oct 2024 15:17:41 +0200 Subject: [PATCH 03/18] fix: handle status check and corrupt storage better --- state-chain/pallets/cf-elections/src/lib.rs | 22 +++++++++++-------- .../runtime/src/chainflip/solana_elections.rs | 17 +++++--------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/state-chain/pallets/cf-elections/src/lib.rs b/state-chain/pallets/cf-elections/src/lib.rs index b2f7190896..8d3639d499 100644 --- a/state-chain/pallets/cf-elections/src/lib.rs +++ b/state-chain/pallets/cf-elections/src/lib.rs @@ -1667,12 +1667,7 @@ pub mod pallet { } pub fn ensure_initialized() -> Result<(), DispatchError> { - if Status::::get().is_some() { - Ok(()) - } else { - Self::deposit_event(Event::::Uninitialized); - Err(Error::::Uninitialized.into()) - } + Self::with_status_check(|| Ok(())) } /// Provides access into the ElectoralSystem's current election @@ -1685,11 +1680,20 @@ pub mod pallet { >( f: F, ) -> Result { - if Status::::get().is_some() { + Self::with_status_check(|| { let mut election_identifiers = ElectionProperties::::iter_keys().collect::>(); election_identifiers.sort(); - Self::handle_corrupt_storage(f(election_identifiers)).map_err(Into::into) + f(election_identifiers) + }) + } + + // TODO: make handle_corrupt storage private + pub fn with_status_check Result>( + f: F, + ) -> Result { + if Status::::get().is_some() { + Self::handle_corrupt_storage(f()).map_err(Into::into) } else { Self::deposit_event(Event::::Uninitialized); Err(Error::::Uninitialized.into()) @@ -2048,7 +2052,7 @@ pub mod pallet { Ok(*election_identifier.unique_monotonic()) } - pub fn handle_corrupt_storage( + fn handle_corrupt_storage( result: Result, ) -> Result> { match result { diff --git a/state-chain/runtime/src/chainflip/solana_elections.rs b/state-chain/runtime/src/chainflip/solana_elections.rs index 2a08895f7f..4b54370b2e 100644 --- a/state-chain/runtime/src/chainflip/solana_elections.rs +++ b/state-chain/runtime/src/chainflip/solana_elections.rs @@ -441,18 +441,15 @@ impl SolanaNonceWatch for SolanaNonceTrackingTrigger { nonce_account: SolAddress, previous_nonce_value: SolHash, ) -> DispatchResult { - // TODO: Check if safe. We are not checking if initialised or not here - we were before in - // with_electoral_access - // TODO: Look at handling the corrupt storage elsewhere. - Ok(pallet_cf_elections::Pallet::::handle_corrupt_storage( + Ok(pallet_cf_elections::Pallet::::with_status_check(|| { SolanaNonceTracking::watch_for_change::< DerivedElectoralAccess< _, SolanaNonceTracking, RunnerStorageAccess, >, - >(nonce_account, previous_nonce_value), - )?) + >(nonce_account, previous_nonce_value) + })?) } } @@ -462,16 +459,14 @@ impl ElectionEgressWitnesser for SolanaEgressWitnessingTrigger { type Chain = SolanaCrypto; fn watch_for_egress_success(signature: SolSignature) -> DispatchResult { - // TODO: Check if safe. We are not checking if initialised or not here - we were before in - // with_electoral_access - Ok(pallet_cf_elections::Pallet::::handle_corrupt_storage( + Ok(pallet_cf_elections::Pallet::::with_status_check(|| { SolanaEgressWitnessing::watch_for_egress::< DerivedElectoralAccess< _, SolanaEgressWitnessing, RunnerStorageAccess, >, - >(signature), - )?) + >(signature) + })?) } } From e65d1d6916a0713f72c888904b43081a5d7ba489 Mon Sep 17 00:00:00 2001 From: kylezs Date: Wed, 23 Oct 2024 16:32:37 +0200 Subject: [PATCH 04/18] rebase fixes remove comment --- state-chain/cf-integration-tests/src/solana.rs | 17 +++++++++-------- .../src/electoral_systems/monotonic_change.rs | 17 ++++------------- state-chain/pallets/cf-elections/src/lib.rs | 2 +- .../runtime/src/chainflip/solana_elections.rs | 8 +++++--- 4 files changed, 19 insertions(+), 25 deletions(-) diff --git a/state-chain/cf-integration-tests/src/solana.rs b/state-chain/cf-integration-tests/src/solana.rs index 9933045f16..0d6652ca74 100644 --- a/state-chain/cf-integration-tests/src/solana.rs +++ b/state-chain/cf-integration-tests/src/solana.rs @@ -29,7 +29,7 @@ use frame_support::{ use pallet_cf_elections::{ electoral_system::{ElectionIdentifierOf, ElectoralSystem}, vote_storage::{composite::tuple_6_impls::CompositeVote, AuthorityVote, VoteStorage}, - MAXIMUM_VOTES_PER_EXTRINSIC, + CompositeAuthorityVoteOf, CompositeElectionIdentifierOf, MAXIMUM_VOTES_PER_EXTRINSIC, }; use pallet_cf_ingress_egress::{DepositWitness, FetchOrTransfer}; use pallet_cf_validator::RotationPhase; @@ -58,11 +58,12 @@ const BOB: AccountId = AccountId::new([0x44; 32]); const DEPOSIT_AMOUNT: u64 = 5_000_000_000u64; // 5 Sol const FALLBACK_ADDRESS: SolAddress = SolAddress([0xf0; 32]); -type SolanaElectionVote = BoundedBTreeMap::< - ElectionIdentifierOf<>::ElectoralSystem>, - AuthorityVote< - <<>::ElectoralSystem as ElectoralSystem>::Vote as VoteStorage>::PartialVote, - <<>::ElectoralSystem as ElectoralSystem>::Vote as VoteStorage>::Vote, +type SolanaElectionVote = BoundedBTreeMap< + CompositeElectionIdentifierOf< + >::ElectoralSystemRunner, + >, + CompositeAuthorityVoteOf< + >::ElectoralSystemRunner, >, ConstU32, >; @@ -729,8 +730,8 @@ fn solana_ccm_execution_error_can_trigger_fallback() { let ccm_broadcast_id = pallet_cf_broadcast::PendingBroadcasts::::get().into_iter().next().unwrap(); // Get the election identifier of the Solana egress. - let election_id = SolanaElections::with_electoral_access_and_identifiers( - |_, election_identifiers| { + let election_id = SolanaElections::with_election_identifiers( + |election_identifiers| { Ok(election_identifiers.last().cloned().unwrap()) }, ).unwrap(); 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 9ea3054981..f3a948f077 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 @@ -43,17 +43,12 @@ impl< > MonotonicChange { pub fn watch_for_change>( - electoral_access: &mut ElectoralAccess, identifier: Identifier, previous_value: Value, ) -> Result<(), CorruptStorageError> { let previous_block_height = - electoral_access.unsynchronised_state_map(&identifier)?.unwrap_or_default(); - electoral_access.new_election( - (), - (identifier, previous_value, previous_block_height), - (), - )?; + ElectoralAccess::unsynchronised_state_map(&identifier)?.unwrap_or_default(); + ElectoralAccess::new_election((), (identifier, previous_value, previous_block_height), ())?; Ok(()) } } @@ -82,7 +77,6 @@ impl< type OnFinalizeReturn = (); fn is_vote_desired>( - _election_identifier: ElectionIdentifierOf, _election_access: &ElectionAccess, _current_vote: Option<(VotePropertiesOf, AuthorityVoteOf)>, ) -> Result { @@ -119,12 +113,11 @@ impl< } fn on_finalize>( - electoral_access: &mut ElectoralAccess, election_identifiers: Vec>, _context: &Self::OnFinalizeContext, ) -> Result { for election_identifier in election_identifiers { - let mut election_access = electoral_access.election_mut(election_identifier)?; + let election_access = ElectoralAccess::election_mut(election_identifier); if let Some((value, block_height)) = election_access.check_consensus()?.has_consensus() { let (identifier, previous_value, previous_block_height) = @@ -132,8 +125,7 @@ impl< if previous_value != value && block_height > previous_block_height { election_access.delete(); Hook::on_change(identifier.clone(), value); - electoral_access - .set_unsynchronised_state_map(identifier, Some(block_height))?; + ElectoralAccess::set_unsynchronised_state_map(identifier, Some(block_height))?; } else { // We don't expect this to be hit, since we should have filtered out any votes // that would cause this in check_consensus. @@ -150,7 +142,6 @@ impl< } fn check_consensus>( - _election_identifier: ElectionIdentifierOf, election_access: &ElectionAccess, _previous_consensus: Option<&Self::Consensus>, consensus_votes: ConsensusVotes, diff --git a/state-chain/pallets/cf-elections/src/lib.rs b/state-chain/pallets/cf-elections/src/lib.rs index 8d3639d499..5455584981 100644 --- a/state-chain/pallets/cf-elections/src/lib.rs +++ b/state-chain/pallets/cf-elections/src/lib.rs @@ -590,7 +590,7 @@ pub mod pallet { // ---------------------------------------------------------------------------------------- // - pub(crate) mod access_impls { + pub mod access_impls { use electoral_system_runner::RunnerStorageAccessTrait; use super::*; diff --git a/state-chain/runtime/src/chainflip/solana_elections.rs b/state-chain/runtime/src/chainflip/solana_elections.rs index 4b54370b2e..639de2aa97 100644 --- a/state-chain/runtime/src/chainflip/solana_elections.rs +++ b/state-chain/runtime/src/chainflip/solana_elections.rs @@ -3,8 +3,11 @@ use crate::{ SolanaIngressEgress, SolanaThresholdSigner, }; use cf_chains::{ - instances::ChainInstanceAlias, - sol::{api::SolanaTransactionType, SolAddress, SolAmount, SolHash, SolSignature, SolTrackedData, SolanaCrypto}, + instances::{ChainInstanceAlias, SolanaInstance}, + sol::{ + api::SolanaTransactionType, SolAddress, SolAmount, SolHash, SolSignature, SolTrackedData, + SolanaCrypto, + }, Chain, FeeEstimationApi, ForeignChain, Solana, }; use cf_runtime_utilities::log_or_panic; @@ -19,7 +22,6 @@ use pallet_cf_elections::{ electoral_system::{ElectoralReadAccess, ElectoralSystem}, electoral_systems::{ self, - change::OnChangeHook, composite::{tuple_6_impls::Hooks, CompositeRunner}, egress_success::OnEgressSuccess, liveness::OnCheckComplete, From fa750daf46a90c77fe2a718509fd1ec5095a82d3 Mon Sep 17 00:00:00 2001 From: kylezs Date: Wed, 23 Oct 2024 16:35:42 +0200 Subject: [PATCH 05/18] chore: clippy --- state-chain/runtime/src/chainflip/solana_elections.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/state-chain/runtime/src/chainflip/solana_elections.rs b/state-chain/runtime/src/chainflip/solana_elections.rs index 639de2aa97..792e52661b 100644 --- a/state-chain/runtime/src/chainflip/solana_elections.rs +++ b/state-chain/runtime/src/chainflip/solana_elections.rs @@ -443,7 +443,7 @@ impl SolanaNonceWatch for SolanaNonceTrackingTrigger { nonce_account: SolAddress, previous_nonce_value: SolHash, ) -> DispatchResult { - Ok(pallet_cf_elections::Pallet::::with_status_check(|| { + pallet_cf_elections::Pallet::::with_status_check(|| { SolanaNonceTracking::watch_for_change::< DerivedElectoralAccess< _, @@ -451,7 +451,7 @@ impl SolanaNonceWatch for SolanaNonceTrackingTrigger { RunnerStorageAccess, >, >(nonce_account, previous_nonce_value) - })?) + }) } } @@ -461,7 +461,7 @@ impl ElectionEgressWitnesser for SolanaEgressWitnessingTrigger { type Chain = SolanaCrypto; fn watch_for_egress_success(signature: SolSignature) -> DispatchResult { - Ok(pallet_cf_elections::Pallet::::with_status_check(|| { + pallet_cf_elections::Pallet::::with_status_check(|| { SolanaEgressWitnessing::watch_for_egress::< DerivedElectoralAccess< _, @@ -469,6 +469,6 @@ impl ElectionEgressWitnesser for SolanaEgressWitnessingTrigger { RunnerStorageAccess, >, >(signature) - })?) + }) } } From 8845b0403dfe5cf0bb7e02c29d93998137dc77d5 Mon Sep 17 00:00:00 2001 From: kylezs Date: Wed, 23 Oct 2024 16:48:22 +0200 Subject: [PATCH 06/18] chore: clean up --- .../pallets/cf-elections/src/electoral_systems/mocks.rs | 1 - .../cf-elections/src/electoral_systems/mocks/access.rs | 7 +++++-- state-chain/pallets/cf-elections/src/lib.rs | 1 - 3 files changed, 5 insertions(+), 4 deletions(-) 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 a0e6e880d7..672cbc78a0 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/mocks.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/mocks.rs @@ -299,7 +299,6 @@ register_checks! { assert_unchanged(pre_finalize, post_finalize) { assert_eq!(pre_finalize, post_finalize); }, - // check state is deleted, rather than can't construct election last_election_deleted(pre_finalize, post_finalize) { let last_election_id = pre_finalize.election_identifiers.last().expect("Expected an election before finalization"); assert!( 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 adae194de9..26474b9659 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 @@ -7,10 +7,9 @@ use crate::{ }; use codec::{Decode, Encode}; use core::cell::RefCell; -use frame_support::{ensure, CloneNoBound, DebugNoBound, EqNoBound, PartialEqNoBound}; +use frame_support::{CloneNoBound, DebugNoBound, EqNoBound, PartialEqNoBound}; use std::collections::BTreeMap; -// TODO: Create a new() so no need to pass mock storage access in each time. same below. pub struct MockReadAccess { election_identifier: ElectionIdentifierOf, } @@ -200,6 +199,10 @@ impl MockStorageAccess { let mut properties_ref = properties.borrow_mut(); properties_ref.clear(); }); + ELECTORAL_SETTINGS.with(|settings| { + let mut settings_ref = settings.borrow_mut(); + settings_ref.clear(); + }); ELECTION_SETTINGS.with(|settings| { let mut settings_ref = settings.borrow_mut(); settings_ref.clear(); diff --git a/state-chain/pallets/cf-elections/src/lib.rs b/state-chain/pallets/cf-elections/src/lib.rs index 5455584981..6c02ba174b 100644 --- a/state-chain/pallets/cf-elections/src/lib.rs +++ b/state-chain/pallets/cf-elections/src/lib.rs @@ -1688,7 +1688,6 @@ pub mod pallet { }) } - // TODO: make handle_corrupt storage private pub fn with_status_check Result>( f: F, ) -> Result { From ab3961944dcf9bcc3c6f7ac89cd6b358a75ede86 Mon Sep 17 00:00:00 2001 From: kylezs Date: Wed, 23 Oct 2024 16:48:43 +0200 Subject: [PATCH 07/18] tmp: full bouncer and upgrade-test --- .github/workflows/ci-development.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci-development.yml b/.github/workflows/ci-development.yml index bc66706607..e94539aeca 100644 --- a/.github/workflows/ci-development.yml +++ b/.github/workflows/ci-development.yml @@ -69,7 +69,7 @@ jobs: uses: ./.github/workflows/_40_post_check.yml secrets: inherit with: - full-bouncer: false + full_bouncer: true ngrok: true test-benchmarks: needs: [build-benchmarks] @@ -79,7 +79,7 @@ jobs: uses: ./.github/workflows/upgrade-test.yml secrets: inherit with: - run-job: false + run-job: true publish: needs: [package] uses: ./.github/workflows/_30_publish.yml From af5de8536425d78d1ea03dd74312b0583648e722 Mon Sep 17 00:00:00 2001 From: kylezs Date: Wed, 23 Oct 2024 17:01:47 +0200 Subject: [PATCH 08/18] chore: clippy --- state-chain/cf-integration-tests/src/solana.rs | 3 +-- .../pallets/cf-elections/src/electoral_systems/mock.rs | 8 ++------ .../pallets/cf-elections/src/electoral_systems/mocks.rs | 2 +- .../cf-elections/src/electoral_systems/mocks/access.rs | 6 ++---- state-chain/pallets/cf-elections/src/tests.rs | 9 ++------- 5 files changed, 8 insertions(+), 20 deletions(-) diff --git a/state-chain/cf-integration-tests/src/solana.rs b/state-chain/cf-integration-tests/src/solana.rs index 0d6652ca74..d6753189e9 100644 --- a/state-chain/cf-integration-tests/src/solana.rs +++ b/state-chain/cf-integration-tests/src/solana.rs @@ -27,8 +27,7 @@ use frame_support::{ traits::{OnFinalize, UnfilteredDispatchable}, }; use pallet_cf_elections::{ - electoral_system::{ElectionIdentifierOf, ElectoralSystem}, - vote_storage::{composite::tuple_6_impls::CompositeVote, AuthorityVote, VoteStorage}, + vote_storage::{composite::tuple_6_impls::CompositeVote, AuthorityVote}, CompositeAuthorityVoteOf, CompositeElectionIdentifierOf, MAXIMUM_VOTES_PER_EXTRINSIC, }; use pallet_cf_ingress_egress::{DepositWitness, FetchOrTransfer}; 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 796540c520..f87dcd9a89 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/mock.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/mock.rs @@ -1,14 +1,10 @@ use crate::{ - electoral_system::{ - ConsensusStatus, ConsensusVotes, ElectionReadAccess, ElectionWriteAccess, ElectoralSystem, - ElectoralWriteAccess, - }, + electoral_system::ConsensusStatus, electoral_system_runner::{CompositeConsensusVotes, RunnerStorageAccessTrait}, mock::Test, vote_storage::{self, VoteStorage}, CompositeAuthorityVoteOf, CompositeElectionIdentifierOf, CompositeVotePropertiesOf, - CorruptStorageError, ElectionIdentifier, ElectoralSystemRunner, RunnerStorageAccess, - UniqueMonotonicIdentifier, + CorruptStorageError, ElectoralSystemRunner, RunnerStorageAccess, UniqueMonotonicIdentifier, }; use cf_primitives::AuthorityCount; use cf_traits::Chainflip; 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 672cbc78a0..5b7dbdf634 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/mocks.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/mocks.rs @@ -339,7 +339,7 @@ impl ElectoralSystemState { Self { unsynchronised_settings: MockStorageAccess::unsynchronised_settings::(), unsynchronised_state: MockStorageAccess::unsynchronised_state::(), - unsynchronised_state_map: MockStorageAccess::raw_unsynchronised_state_map::(), + unsynchronised_state_map: MockStorageAccess::raw_unsynchronised_state_map(), election_identifiers: MockStorageAccess::election_identifiers::(), next_umi: MockStorageAccess::next_umi(), } 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 26474b9659..577b876ff6 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 @@ -230,7 +230,7 @@ impl MockStorageAccess { } pub fn next_umi() -> UniqueMonotonicIdentifier { - NEXT_ELECTION_ID.with(|next_id| next_id.borrow().clone()) + NEXT_ELECTION_ID.with(|next_id| *next_id.borrow()) } pub fn increment_next_umi() { @@ -273,7 +273,6 @@ impl MockStorageAccess { let settings_ref = settings.borrow(); settings_ref .get(&identifier.encode()) - .clone() .map(|v| ES::ElectoralSettings::decode(&mut &v[..]).unwrap()) .unwrap() }) @@ -382,8 +381,7 @@ impl MockStorageAccess { }) } - pub fn raw_unsynchronised_state_map() -> BTreeMap, Option>> - { + pub fn raw_unsynchronised_state_map() -> BTreeMap, Option>> { ELECTORAL_UNSYNCHRONISED_STATE_MAP.with(|old_state_map| { let state_map_ref = old_state_map.borrow(); state_map_ref.clone() diff --git a/state-chain/pallets/cf-elections/src/tests.rs b/state-chain/pallets/cf-elections/src/tests.rs index 020f95028d..9d4083e8eb 100644 --- a/state-chain/pallets/cf-elections/src/tests.rs +++ b/state-chain/pallets/cf-elections/src/tests.rs @@ -1,14 +1,9 @@ #![cfg(test)] use crate::{mock::*, *}; use cf_primitives::AuthorityCount; -use electoral_system::{ - AuthorityVoteOf, ConsensusStatus, ElectionReadAccess, ElectoralReadAccess, ElectoralWriteAccess, -}; +use electoral_system::ConsensusStatus; use electoral_system_runner::RunnerStorageAccessTrait; -use electoral_systems::{ - mock::{BehaviourUpdate, MockElectoralSystemRunner}, - mocks::MockStorageAccess, -}; +use electoral_systems::mock::{BehaviourUpdate, MockElectoralSystemRunner}; use frame_support::traits::OriginTrait; use mock::Test; use std::collections::BTreeMap; From c0ded803708fa451a9e70f081568b0594c2daf24 Mon Sep 17 00:00:00 2001 From: kylezs Date: Thu, 24 Oct 2024 11:44:59 +0200 Subject: [PATCH 09/18] doc: comment electoral system runner --- .../pallets/cf-elections/src/electoral_system_runner.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/state-chain/pallets/cf-elections/src/electoral_system_runner.rs b/state-chain/pallets/cf-elections/src/electoral_system_runner.rs index f640491979..fb0d5226cb 100644 --- a/state-chain/pallets/cf-elections/src/electoral_system_runner.rs +++ b/state-chain/pallets/cf-elections/src/electoral_system_runner.rs @@ -50,6 +50,13 @@ impl CompositeConsensusVotes { } } +/// A trait used to define a runner of electoral systems. An object implementing this trait is +/// injected into an elections pallet, which then executes the necessary logic to run each electoral +/// system's logic. +/// The primary implementation of this trait is the `CompositeRunner`. This should be the *only* +/// implementation of this trait. This ensures that the storage and access is consistent across all +/// electoral systems. i.e. we always wrap the storage types. Which leads to consistent and +/// therefore simpler migration logic. pub trait ElectoralSystemRunner: 'static + Sized { type ValidatorId: Parameter + Member + MaybeSerializeDeserialize; From e041f0e784341b3037c92e875e1839429633e5fc Mon Sep 17 00:00:00 2001 From: kylezs Date: Thu, 24 Oct 2024 14:59:54 +0200 Subject: [PATCH 10/18] chore: remove unnecessary Results --- .../pallets/cf-elections/src/electoral_system.rs | 2 +- .../cf-elections/src/electoral_system_runner.rs | 13 +++++-------- .../blockchain/delta_based_ingress.rs | 2 +- .../cf-elections/src/electoral_systems/composite.rs | 11 ++++++----- .../src/electoral_systems/mocks/access.rs | 3 +-- .../src/electoral_systems/monotonic_change.rs | 2 +- state-chain/pallets/cf-elections/src/lib.rs | 12 ++++-------- 7 files changed, 19 insertions(+), 26 deletions(-) diff --git a/state-chain/pallets/cf-elections/src/electoral_system.rs b/state-chain/pallets/cf-elections/src/electoral_system.rs index 840e12d332..4ad624c090 100644 --- a/state-chain/pallets/cf-elections/src/electoral_system.rs +++ b/state-chain/pallets/cf-elections/src/electoral_system.rs @@ -379,7 +379,7 @@ mod access { value: Option< ::ElectoralUnsynchronisedStateMapValue, >, - ) -> Result<(), CorruptStorageError>; + ); /// Allows you to mutate the unsynchronised state. This is more efficient than a read /// (`unsynchronised_state`) and then a write (`set_unsynchronised_state`) in the case of diff --git a/state-chain/pallets/cf-elections/src/electoral_system_runner.rs b/state-chain/pallets/cf-elections/src/electoral_system_runner.rs index fb0d5226cb..0a6d1ef1aa 100644 --- a/state-chain/pallets/cf-elections/src/electoral_system_runner.rs +++ b/state-chain/pallets/cf-elections/src/electoral_system_runner.rs @@ -279,12 +279,9 @@ pub trait RunnerStorageAccessTrait { >; fn unsynchronised_state_map( key: &::ElectoralUnsynchronisedStateMapKey, - ) -> Result< - Option< + ) -> Option< ::ElectoralUnsynchronisedStateMapValue, - >, - CorruptStorageError, - >; + >; fn new_election( extra: ::ElectionIdentifierExtra, @@ -294,7 +291,7 @@ pub trait RunnerStorageAccessTrait { fn set_unsynchronised_state( unsynchronised_state: ::ElectoralUnsynchronisedState, - ) -> Result<(), CorruptStorageError>; + ); /// Inserts or removes a value from the unsynchronised state map of the electoral system. fn set_unsynchronised_state_map( @@ -302,7 +299,7 @@ pub trait RunnerStorageAccessTrait { value: Option< ::ElectoralUnsynchronisedStateMapValue, >, - ) -> Result<(), CorruptStorageError>; + ); /// Allows you to mutate the unsynchronised state. This is more efficient than a read /// (`unsynchronised_state`) and then a write (`set_unsynchronised_state`) in the case of @@ -320,7 +317,7 @@ pub trait RunnerStorageAccessTrait { ) -> Result { let mut unsynchronised_state = Self::unsynchronised_state()?; let t = f(self, &mut unsynchronised_state)?; - Self::set_unsynchronised_state(unsynchronised_state)?; + Self::set_unsynchronised_state(unsynchronised_state); Ok(t) } } diff --git a/state-chain/pallets/cf-elections/src/electoral_systems/blockchain/delta_based_ingress.rs b/state-chain/pallets/cf-elections/src/electoral_systems/blockchain/delta_based_ingress.rs index e67170885e..2868f2391a 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/blockchain/delta_based_ingress.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/blockchain/delta_based_ingress.rs @@ -236,7 +236,7 @@ where ElectoralAccess::set_unsynchronised_state_map( (account.clone(), details.asset), Some(ingress_total), - )?; + ); }, Ordering::Greater => { Sink::on_ingress_reverted( 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 1cea032383..64ebad3a73 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/composite.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/composite.rs @@ -393,7 +393,7 @@ macro_rules! generate_electoral_system_tuple_impls { fn unsynchronised_state_map( key: &$current::ElectoralUnsynchronisedStateMapKey, ) -> Result, CorruptStorageError> { - match StorageAccess::unsynchronised_state_map(&CompositeElectoralUnsynchronisedStateMapKey::$current(key.clone()))? { + match StorageAccess::unsynchronised_state_map(&CompositeElectoralUnsynchronisedStateMapKey::$current(key.clone())) { Some(CompositeElectoralUnsynchronisedStateMapValue::$current(value)) => Ok(Some(value)), None => Ok(None), _ => Err(CorruptStorageError::new()), @@ -423,17 +423,18 @@ macro_rules! generate_electoral_system_tuple_impls { unsynchronised_state: $current::ElectoralUnsynchronisedState, ) -> Result<(), CorruptStorageError> { let ($($previous,)* _, $($remaining,)*) = StorageAccess::unsynchronised_state()?; - StorageAccess::set_unsynchronised_state(($($previous,)* unsynchronised_state, $($remaining,)*)) + StorageAccess::set_unsynchronised_state(($($previous,)* unsynchronised_state, $($remaining,)*)); + Ok(()) } fn set_unsynchronised_state_map( key: $current::ElectoralUnsynchronisedStateMapKey, value: Option<$current::ElectoralUnsynchronisedStateMapValue>, - ) -> Result<(), CorruptStorageError> { + ) { StorageAccess::set_unsynchronised_state_map( CompositeElectoralUnsynchronisedStateMapKey::$current(key), value.map(CompositeElectoralUnsynchronisedStateMapValue::$current), - ) + ); } fn mutate_unsynchronised_state< @@ -446,7 +447,7 @@ macro_rules! generate_electoral_system_tuple_impls { ) -> Result { let ($($previous,)* mut unsynchronised_state, $($remaining,)*) = StorageAccess::unsynchronised_state()?; let t = f( &mut unsynchronised_state)?; - StorageAccess::set_unsynchronised_state(($($previous,)* unsynchronised_state, $($remaining,)*))?; + StorageAccess::set_unsynchronised_state(($($previous,)* unsynchronised_state, $($remaining,)*)); Ok(t) } } 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 577b876ff6..5fe233962f 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 @@ -181,9 +181,8 @@ impl ElectoralWriteAccess for MockAccess { value: Option< ::ElectoralUnsynchronisedStateMapValue, >, - ) -> Result<(), CorruptStorageError> { + ) { MockStorageAccess::set_unsynchronised_state_map::(key, value); - Ok(()) } } 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 f3a948f077..3ef6c454c2 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 @@ -125,7 +125,7 @@ impl< if previous_value != value && block_height > previous_block_height { election_access.delete(); Hook::on_change(identifier.clone(), value); - ElectoralAccess::set_unsynchronised_state_map(identifier, Some(block_height))?; + ElectoralAccess::set_unsynchronised_state_map(identifier, Some(block_height)); } else { // We don't expect this to be hit, since we should have filtered out any votes // that would cause this in check_consensus. diff --git a/state-chain/pallets/cf-elections/src/lib.rs b/state-chain/pallets/cf-elections/src/lib.rs index 6c02ba174b..14fb70bcdd 100644 --- a/state-chain/pallets/cf-elections/src/lib.rs +++ b/state-chain/pallets/cf-elections/src/lib.rs @@ -886,20 +886,17 @@ pub mod pallet { fn unsynchronised_state_map( key: &::ElectoralUnsynchronisedStateMapKey, - ) -> Result< + ) -> Option< ::ElectoralUnsynchronisedStateMapValue, - >, - CorruptStorageError, >{ - Ok(ElectoralUnsynchronisedStateMap::::get(key)) + ElectoralUnsynchronisedStateMap::::get(key) } fn set_unsynchronised_state( unsynchronised_state: ::ElectoralUnsynchronisedState, - ) -> Result<(), CorruptStorageError> { + ) { ElectoralUnsynchronisedState::::put(unsynchronised_state); - Ok(()) } fn set_unsynchronised_state_map( @@ -907,9 +904,8 @@ pub mod pallet { value: Option< ::ElectoralUnsynchronisedStateMapValue, >, - ) -> Result<(), CorruptStorageError> { + ) { ElectoralUnsynchronisedStateMap::::set(key, value); - Ok(()) } } } From a9e693b778f0235a7127ba89f56d44fc4cb710ac Mon Sep 17 00:00:00 2001 From: kylezs Date: Thu, 24 Oct 2024 15:40:49 +0200 Subject: [PATCH 11/18] chore: clean up --- .../src/electoral_systems/composite.rs | 4 +++- state-chain/pallets/cf-elections/src/lib.rs | 15 ++++++--------- 2 files changed, 9 insertions(+), 10 deletions(-) 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 64ebad3a73..8d7d3fdc7f 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/composite.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/composite.rs @@ -350,7 +350,9 @@ macro_rules! generate_electoral_system_tuple_impls { properties: $current::ElectionProperties, ) -> Result<(), CorruptStorageError> { StorageAccess::refresh_election( + // The current election id + extra that we want to refresh. self.id.with_extra(CompositeElectionIdentifierExtra::$current(*self.id.extra())), + // The new extra we want to use. CompositeElectionIdentifierExtra::$current(new_extra), CompositeElectionProperties::$current(properties), )?; @@ -410,7 +412,7 @@ macro_rules! generate_electoral_system_tuple_impls { state: $current::ElectionState, ) -> Result { let election_identifier = StorageAccess::new_election(CompositeElectionIdentifierExtra::$current(extra), CompositeElectionProperties::$current(properties), CompositeElectionState::$current(state))?; - Ok(Self::ElectionWriteAccess::new(election_identifier.with_extra(extra))) + Ok(Self::election_mut(election_identifier.with_extra(extra))) } fn election_mut( diff --git a/state-chain/pallets/cf-elections/src/lib.rs b/state-chain/pallets/cf-elections/src/lib.rs index 14fb70bcdd..e921169833 100644 --- a/state-chain/pallets/cf-elections/src/lib.rs +++ b/state-chain/pallets/cf-elections/src/lib.rs @@ -706,7 +706,10 @@ pub mod pallet { } else { ElectionProperties::::remove(election_identifier); let new_election_identifier = - ElectionIdentifier::new(*election_identifier.unique_monotonic(), new_extra); + CompositeElectionIdentifierOf::::new( + *election_identifier.unique_monotonic(), + new_extra, + ); ElectionProperties::::insert(new_election_identifier, properties); Ok(()) } @@ -774,8 +777,8 @@ pub mod pallet { Ok(SharedData::::get(shared_data_hash)) }) { // Only a full vote can count towards consensus. - Ok(Some((properties, AuthorityVote::Vote(vote)))) => Ok(Some((properties, - vote))), Ok(Some((_properties, AuthorityVote::PartialVote(_)))) => Ok(None), + Ok(Some((properties, AuthorityVote::Vote(vote)))) => Ok(Some((properties, vote))), + Ok(Some((_properties, AuthorityVote::PartialVote(_)))) => Ok(None), Ok(None) => Ok(None), Err(e) => Err(e), } @@ -1223,8 +1226,6 @@ pub mod pallet { ConstU32, >, ) -> DispatchResult { - Self::ensure_initialized()?; - let (epoch_index, authority, authority_index) = Self::ensure_can_vote(origin)?; ensure!(!authority_votes.is_empty(), Error::::NoVotesSpecified); @@ -1662,10 +1663,6 @@ pub mod pallet { Ok(()) } - pub fn ensure_initialized() -> Result<(), DispatchError> { - Self::with_status_check(|| Ok(())) - } - /// Provides access into the ElectoralSystem's current election /// identifiers. pub fn with_election_identifiers< From 5f5aa27c338a21a434c93613066d62ce6338181e Mon Sep 17 00:00:00 2001 From: kylezs Date: Mon, 28 Oct 2024 11:58:54 +0100 Subject: [PATCH 12/18] chore: fix rebase issue --- engine/src/elections.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/engine/src/elections.rs b/engine/src/elections.rs index 9da45256d4..8d7eea006d 100644 --- a/engine/src/elections.rs +++ b/engine/src/elections.rs @@ -23,7 +23,6 @@ use std::{ sync::Arc, }; use tracing::{error, info, warn}; -use utilities::{future_map::FutureMap, task_scope::Scope, UnendingStream}; use voter_api::CompositeVoterApi; const MAXIMUM_CONCURRENT_FILTER_REQUESTS: usize = 16; From 75ed38456f6b28529d7a3a851564c33fdab30e09 Mon Sep 17 00:00:00 2001 From: kylezs Date: Wed, 30 Oct 2024 14:27:36 +0100 Subject: [PATCH 13/18] fix ci typo --- .github/workflows/ci-development.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci-development.yml b/.github/workflows/ci-development.yml index e94539aeca..e11bed65df 100644 --- a/.github/workflows/ci-development.yml +++ b/.github/workflows/ci-development.yml @@ -69,7 +69,7 @@ jobs: uses: ./.github/workflows/_40_post_check.yml secrets: inherit with: - full_bouncer: true + full-bouncer: true ngrok: true test-benchmarks: needs: [build-benchmarks] From c59e83755e198134f81455fa95b4ca09988e56f0 Mon Sep 17 00:00:00 2001 From: kylezs Date: Mon, 11 Nov 2024 14:22:42 +0100 Subject: [PATCH 14/18] chore: remove printlns --- .../pallets/cf-elections/src/electoral_systems/mocks.rs | 5 ----- .../cf-elections/src/electoral_systems/mocks/access.rs | 1 - 2 files changed, 6 deletions(-) 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 5b7dbdf634..d413eb269d 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/mocks.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/mocks.rs @@ -104,11 +104,6 @@ where ) .unwrap(); - println!( - "Election identifier in with initial election: {:?}", - election.election_identifier() - ); - // A new election should not have consensus at any authority count. assert_eq!(election.check_consensus(None, ConsensusVotes { votes: vec![] }).unwrap(), None); 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 5fe233962f..af32e582ee 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 @@ -69,7 +69,6 @@ macro_rules! impl_read_access { previous_consensus: Option<&ES::Consensus>, votes: ConsensusVotes, ) -> Result, CorruptStorageError> { - println!("Calling check consensus on the electoral system struct"); ES::check_consensus(self, previous_consensus, votes) } } From e64cb9400d49a56a42157c868aa4fc592dfff906 Mon Sep 17 00:00:00 2001 From: kylezs Date: Mon, 11 Nov 2024 14:23:35 +0100 Subject: [PATCH 15/18] chore: fmt --- .../cf-elections/src/electoral_systems/tests/liveness.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/state-chain/pallets/cf-elections/src/electoral_systems/tests/liveness.rs b/state-chain/pallets/cf-elections/src/electoral_systems/tests/liveness.rs index 9c3767ce8f..53a6a7364b 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/tests/liveness.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/tests/liveness.rs @@ -38,9 +38,11 @@ register_checks! { assert_eq!(post.election_identifiers.len(), 1, "Only one election should exist."); }, hook_called_once(_pre, _post) { - assert_eq!(HOOK_CALLED_COUNT.with(|hook_called| hook_called.get()), 1, "Hook should have been called once so far!"); }, + assert_eq!(HOOK_CALLED_COUNT.with(|hook_called| hook_called.get()), 1, "Hook should have been called once so far!"); + }, hook_called_twice(_pre, _post) { - assert_eq!(HOOK_CALLED_COUNT.with(|hook_called| hook_called.get()), 2, "Hook should have been called twice so far!"); }, + assert_eq!(HOOK_CALLED_COUNT.with(|hook_called| hook_called.get()), 2, "Hook should have been called twice so far!"); + }, hook_not_called(_pre, _post) { assert_eq!( HOOK_CALLED_COUNT.with(|hook_called| hook_called.get()), From 0b24c7a2a383f3e3c609b4259eb0dd07b899b4b5 Mon Sep 17 00:00:00 2001 From: kylezs Date: Mon, 11 Nov 2024 14:25:56 +0100 Subject: [PATCH 16/18] doc: improve electoral settings comment --- .../pallets/cf-elections/src/electoral_systems/mocks.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) 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 d413eb269d..f1c70643e8 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/mocks.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/mocks.rs @@ -91,9 +91,13 @@ where let (election_identifier_extra, election_properties, election_state) = self.initial_election_state.unwrap_or_default(); - // These are synchronised with respect to an election, so for simplicity in the tests - // we just assign them to an election. + // The ElectoralSettings are synchronised with an election, by election identifier in the + // actual implementation. Here we simplify by storing the settings in the electoral + // system, and upon the creation of new election, we store the ElectoralSettings that were + // in storage with the election directly. This duplicates the settings, but is fine for + // testing. MockStorageAccess::set_electoral_settings::(setup.electoral_settings.clone()); + MockStorageAccess::set_unsynchronised_state::(setup.unsynchronised_state.clone()); MockStorageAccess::set_unsynchronised_settings::(setup.unsynchronised_settings.clone()); From 1abdd6453c4115daaec4f1863ee3b16d6a1ef5de Mon Sep 17 00:00:00 2001 From: kylezs Date: Mon, 11 Nov 2024 15:25:50 +0100 Subject: [PATCH 17/18] chore: clippy --- .../pallets/cf-elections/src/electoral_systems/mocks.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 f1c70643e8..0ea98d014f 100644 --- a/state-chain/pallets/cf-elections/src/electoral_systems/mocks.rs +++ b/state-chain/pallets/cf-elections/src/electoral_systems/mocks.rs @@ -2,8 +2,8 @@ use std::collections::BTreeMap; use crate::{ electoral_system::{ - ConsensusStatus, ConsensusVotes, ElectionIdentifierOf, ElectionReadAccess, - ElectoralReadAccess, ElectoralSystem, ElectoralWriteAccess, + ConsensusStatus, ConsensusVotes, ElectionIdentifierOf, ElectoralReadAccess, + ElectoralSystem, ElectoralWriteAccess, }, UniqueMonotonicIdentifier, }; From c6907d999ef7c336db6359e723255c842ad8c6c4 Mon Sep 17 00:00:00 2001 From: kylezs Date: Mon, 11 Nov 2024 15:26:17 +0100 Subject: [PATCH 18/18] chore: reset dev ci flags --- .github/workflows/ci-development.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci-development.yml b/.github/workflows/ci-development.yml index e11bed65df..bc66706607 100644 --- a/.github/workflows/ci-development.yml +++ b/.github/workflows/ci-development.yml @@ -69,7 +69,7 @@ jobs: uses: ./.github/workflows/_40_post_check.yml secrets: inherit with: - full-bouncer: true + full-bouncer: false ngrok: true test-benchmarks: needs: [build-benchmarks] @@ -79,7 +79,7 @@ jobs: uses: ./.github/workflows/upgrade-test.yml secrets: inherit with: - run-job: true + run-job: false publish: needs: [package] uses: ./.github/workflows/_30_publish.yml