diff --git a/substrate/frame/election-provider-multi-block/src/lib.rs b/substrate/frame/election-provider-multi-block/src/lib.rs index e09cae6ae7af..df3b70d9b3ce 100644 --- a/substrate/frame/election-provider-multi-block/src/lib.rs +++ b/substrate/frame/election-provider-multi-block/src/lib.rs @@ -128,10 +128,6 @@ pub use types::*; pub use crate::{unsigned::miner, verifier::Verifier, weights::WeightInfo}; -/// Internal crate re-exports to use across benchmarking and tests. -#[cfg(any(test, feature = "runtime-benchmarks"))] -use crate::verifier::Pallet as PalletVerifier; - /// Log target for this the core EPM-MB pallet. const LOG_TARGET: &'static str = "runtime::multiblock-election"; diff --git a/substrate/frame/election-provider-multi-block/src/mock/mod.rs b/substrate/frame/election-provider-multi-block/src/mock/mod.rs index dbe1a8c01862..636f3acae1aa 100644 --- a/substrate/frame/election-provider-multi-block/src/mock/mod.rs +++ b/substrate/frame/election-provider-multi-block/src/mock/mod.rs @@ -291,7 +291,7 @@ pub struct ExtBuilder { impl Default for ExtBuilder { fn default() -> Self { - Self { core_try_state: true, signed_try_state: true, minimum_score: Some(Default::default()) } + Self { core_try_state: true, signed_try_state: true, minimum_score: None } } } @@ -438,6 +438,7 @@ impl ExtBuilder { pub(crate) fn build_and_execute(self, test: impl FnOnce() -> ()) { let mut ext = self.build(); + ext.execute_with(|| roll_one()); ext.execute_with(test); ext.execute_with(|| { diff --git a/substrate/frame/election-provider-multi-block/src/signed/mod.rs b/substrate/frame/election-provider-multi-block/src/signed/mod.rs index 06d6b53c3396..eabcd8e4a4ec 100644 --- a/substrate/frame/election-provider-multi-block/src/signed/mod.rs +++ b/substrate/frame/election-provider-multi-block/src/signed/mod.rs @@ -146,7 +146,7 @@ type BalanceOf = <::Currency as FnInspect>>::Bala pub type CreditOf = Credit, ::Currency>; /// Release strategy for currency held by this pallet. -#[derive(Encode, Decode, MaxEncodedLen, TypeInfo, RuntimeDebugNoBound, PartialEq)] +#[derive(Encode, Decode, MaxEncodedLen, TypeInfo, RuntimeDebugNoBound, PartialEq, Clone)] pub(crate) enum ReleaseStrategy { /// Releases all held deposit. All, @@ -165,7 +165,7 @@ impl Default for ReleaseStrategy { } /// Metadata of a registered submission. -#[derive(Encode, Decode, MaxEncodedLen, TypeInfo, Default, RuntimeDebugNoBound)] +#[derive(Encode, Decode, MaxEncodedLen, TypeInfo, Default, RuntimeDebugNoBound, Clone)] #[cfg_attr(test, derive(frame_support::PartialEqNoBound, frame_support::EqNoBound))] #[codec(mel_bound(T: Config))] #[scale_info(skip_type_params(T))] diff --git a/substrate/frame/election-provider-multi-block/src/signed/tests.rs b/substrate/frame/election-provider-multi-block/src/signed/tests.rs index 629558b5f283..00702462cfd6 100644 --- a/substrate/frame/election-provider-multi-block/src/signed/tests.rs +++ b/substrate/frame/election-provider-multi-block/src/signed/tests.rs @@ -16,12 +16,7 @@ // limitations under the License. use super::*; -use crate::{ - mock::*, - signed::Error::{CannotClear, NotAcceptingSubmissions, SubmissionNotRegistered}, - verifier::SolutionDataProvider, - CurrentPhase, Phase, Verifier, -}; +use crate::{mock::*, verifier::SolutionDataProvider, Phase, Verifier}; use frame_support::{assert_noop, assert_ok, testing_prelude::*}; use sp_npos_elections::ElectionScore; use sp_runtime::traits::Convert; @@ -34,7 +29,6 @@ fn clear_submission_of_works() { mod calls { use super::*; use sp_core::bounded_vec; - use sp_runtime::traits::BadOrigin; #[test] fn register_works() { @@ -263,11 +257,12 @@ mod calls { } #[test] - fn register_and_submit_page_and_bail_prohibitted_in_phase_other_than_signed() { + fn calls_in_phase_other_than_signed() { ExtBuilder::default().build_and_execute(|| { let account_id = 99; let phases = vec![ + Phase::Halted, Phase::Off, Phase::SignedValidation(1), Phase::Unsigned(1), @@ -281,7 +276,7 @@ mod calls { assert_err!( SignedPallet::register(RuntimeOrigin::signed(account_id), Default::default()), - NotAcceptingSubmissions::, + Error::::NotAcceptingSubmissions ); assert_err!( @@ -290,12 +285,12 @@ mod calls { 0, Some(Default::default()) ), - NotAcceptingSubmissions::, + Error::::NotAcceptingSubmissions ); assert_err!( SignedPallet::bail(RuntimeOrigin::signed(account_id)), - NotAcceptingSubmissions::, + Error::::CannotBail ); } }) @@ -345,17 +340,7 @@ mod calls { // account 1 submitted nothing, so bail should have no effect and return error assert_noop!( SignedPallet::bail(RuntimeOrigin::signed(bailing_account_id)), - SubmissionNotRegistered:: - ); - }) - } - - #[test] - fn force_clear_submission_fails_if_called_by_account_none() { - ExtBuilder::default().build_and_execute(|| { - assert_err!( - SignedPallet::force_clear_submission(RuntimeOrigin::none(), 0, 99), - BadOrigin + Error::::SubmissionNotRegistered ); }) } @@ -363,15 +348,12 @@ mod calls { #[test] fn force_clear_submission_fails_if_called_in_phase_other_than_off() { ExtBuilder::default().build_and_execute(|| { - let some_bn = 0; - let some_page_index = 0; - let phases = vec![ Phase::Signed, - Phase::Snapshot(some_page_index), - Phase::SignedValidation(some_bn), - Phase::Unsigned(some_bn), - Phase::Export(some_bn), + Phase::Snapshot(0), + Phase::SignedValidation(0), + Phase::Unsigned(0), + Phase::Export(0), Phase::Emergency, ]; @@ -381,7 +363,7 @@ mod calls { assert_err!( SignedPallet::force_clear_submission(RuntimeOrigin::root(), 0, account_id), - CannotClear::, + Error::::CannotClear, ); } }) @@ -395,14 +377,13 @@ mod calls { assert_err!( SignedPallet::force_clear_submission(RuntimeOrigin::root(), 0, account_id), - CannotClear:: + Error::::NoSubmission, ); }) } #[test] - fn force_clear_submission_fails_if_submitter_done_submissions_for_another_round_than_requested() - { + fn force_clear_submission_fails_if_submitter_if_different_round() { ExtBuilder::default().build_and_execute(|| { let account_id = 99; let current_round = MultiPhase::current_round(); @@ -415,42 +396,7 @@ mod calls { current_round + 1, account_id ), - CannotClear:: - ); - }) - } - - #[test] - fn force_clear_submission_removes_both_metadata_and_submission_pages() { - ExtBuilder::default().build_and_execute(|| { - let account_id = 99; - let current_round = MultiPhase::current_round(); - - // do_register and try_mutate_page used directly so as not to switch phases in the test - assert_ok!(Pallet::::do_register( - &account_id, - Default::default(), - current_round - )); - - assert_ok!(Submissions::::try_mutate_page( - &account_id, - current_round, - 0, - Some(Default::default()) - )); - - roll_to_phase(Phase::Off); - - assert_ok!(SignedPallet::force_clear_submission( - RuntimeOrigin::root(), - current_round, - account_id - )); - - assert!(Submissions::::metadata_for(current_round, &account_id).is_none()); - assert!( - Submissions::::page_submission_for(current_round, account_id, 0).is_none() + Error::::NoSubmission, ); }) } @@ -513,7 +459,7 @@ mod calls { ); // force phase Off. - CurrentPhase::::set(Phase::Off); + set_phase_to(Phase::Off); assert_ok!(SignedPallet::force_clear_submission(RuntimeOrigin::signed(99), round, 99)); // 3 pages submitted have been cleared from storage. @@ -543,14 +489,14 @@ mod calls { Phase::Export(0), Phase::Emergency, ] { - CurrentPhase::::set(disabled_phase); + set_phase_to(disabled_phase); assert_noop!( SignedPallet::force_clear_submission(RuntimeOrigin::signed(99), round, 99), Error::::CannotClear ); } - CurrentPhase::::set(Phase::Off); + set_phase_to(Phase::Off); // request force clear of a non existing submission. assert_noop!( @@ -704,19 +650,6 @@ mod solution_data_provider { }) } - #[test] - fn returns_none_given_invalid_page_index() { - ExtBuilder::default().build_and_execute(|| { - let origin = RuntimeOrigin::signed(99); - roll_to_phase(Phase::Signed); - - assert_ok!(SignedPallet::register(origin.clone(), Default::default())); - assert_ok!(SignedPallet::submit_page(origin, 0, Some(Default::default()))); - - assert_eq!(::get_paged_solution(12345), None) - }) - } - #[test] fn returns_none_if_there_are_no_submissions() { ExtBuilder::default().build_and_execute(|| { diff --git a/substrate/frame/election-provider-multi-block/src/tests.rs b/substrate/frame/election-provider-multi-block/src/tests.rs index 0bda8d1ae712..cf365d29a2df 100644 --- a/substrate/frame/election-provider-multi-block/src/tests.rs +++ b/substrate/frame/election-provider-multi-block/src/tests.rs @@ -466,10 +466,10 @@ mod election_provider { .unwrap(); let supports_page_zero = - PalletVerifier::::feasibility_check(results.solution_pages[0].clone(), 0) + VerifierPallet::feasibility_check(results.solution_pages[0].clone(), 0) .unwrap(); let supports_page_one = - PalletVerifier::::feasibility_check(results.solution_pages[1].clone(), 1) + VerifierPallet::feasibility_check(results.solution_pages[1].clone(), 1) .unwrap(); let s0: Supports = vec![ @@ -520,10 +520,10 @@ mod election_provider { .unwrap(); let supports_page_zero = - PalletVerifier::::feasibility_check(results.solution_pages[0].clone(), 0) + VerifierPallet::feasibility_check(results.solution_pages[0].clone(), 0) .unwrap(); let supports_page_one = - PalletVerifier::::feasibility_check(results.solution_pages[1].clone(), 1) + VerifierPallet::feasibility_check(results.solution_pages[1].clone(), 1) .unwrap(); // consume supports and checks they fit within the max backers per winner bounds. diff --git a/substrate/frame/election-provider-multi-block/src/unsigned/tests.rs b/substrate/frame/election-provider-multi-block/src/unsigned/tests.rs index aecf8050dc3e..ff23469a4cf7 100644 --- a/substrate/frame/election-provider-multi-block/src/unsigned/tests.rs +++ b/substrate/frame/election-provider-multi-block/src/unsigned/tests.rs @@ -273,51 +273,6 @@ mod miner { }); } - #[test] - fn mining_done_but_no_solution_found() { - ExtBuilder::default().pages(0).build_and_execute(|| { - let round = Default::default(); - let pages = Pages::get(); - - let mut all_voter_pages: BoundedVec< - BoundedVec, mock::VoterSnapshotPerBlock>, - mock::Pages, - > = Default::default(); - - let mut voters_page = BoundedVec::new(); - - // one voter with accountId 12 that votes for validator 123 - let mut voters_votes: BoundedVec = - BoundedVec::new(); - let _ = voters_votes.try_push(123); - let voter = (12, 1, voters_votes); - - // one voters page with the voter 12 - let _ = voters_page.try_push(voter); - let _ = all_voter_pages.try_push(voters_page); - - // one election target with accountId 0 - let mut all_targets: BoundedVec = - Default::default(); - let _ = all_targets.try_push(0); - - // the election should result with one target chosen - let desired_targets = 1; - - let solution = Miner::::mine_paged_solution_with_snapshot( - &all_voter_pages, - &all_targets, - pages, - round, - desired_targets, - false, - ); - - assert_ok!(solution.clone()); - assert_eq!(solution.unwrap().0.solution_pages.len(), 0); - }); - } - #[test] fn mining_done_solution_calculated() { ExtBuilder::default() @@ -441,45 +396,18 @@ mod pallet { use super::*; - // currently fails even though the store should be cleaned #[test] - fn phase_unsigned_but_no_msp_results_only_with_cache_clean_up() { + fn cached_results_clean_up_at_export_phase() { let (mut ext, _) = ExtBuilder::default().build_offchainify(0); ext.execute_with(|| { - set_phase_to(Phase::Unsigned(0)); - assert_eq!(::next_missing_solution_page(), None); - - // there's something in the cache before worker run - assert_eq!( - StorageValueRef::persistent( - &OffchainWorkerMiner::::OFFCHAIN_CACHED_SCORE, - ) - .get::() - .iter() - .count(), - 1 - ); - - assert_ok!(UnsignedPallet::do_sync_offchain_worker(0)); + set_phase_to(Phase::Export(0)); - assert_eq!( - StorageValueRef::persistent( - &OffchainWorkerMiner::::OFFCHAIN_CACHED_SCORE, - ) - .get::() - .iter() - .count(), - 0 + let score_storage = StorageValueRef::persistent( + &OffchainWorkerMiner::::OFFCHAIN_CACHED_SCORE, ); - }); - } - // currently fails even though the store should be cleaned - #[test] - fn past_phase_unsigned_results_only_with_cache_clean_up() { - let (mut ext, _) = ExtBuilder::default().build_offchainify(0); - ext.execute_with(|| { - set_phase_to(Phase::Export(0)); + // add some score to cache. + assert_ok!(score_storage.mutate::<_, (), _>(|_| Ok(ElectionScore::default()))); // there's something in the cache before worker run assert_eq!( @@ -487,11 +415,11 @@ mod pallet { &OffchainWorkerMiner::::OFFCHAIN_CACHED_SCORE, ) .get::() - .iter() - .count(), - 1 + .unwrap(), + Some(ElectionScore::default()) ); + // call sync offchain workers in Export phase will clear up the cache. assert_ok!(UnsignedPallet::do_sync_offchain_worker(0)); assert_eq!( @@ -499,9 +427,8 @@ mod pallet { &OffchainWorkerMiner::::OFFCHAIN_CACHED_SCORE, ) .get::() - .iter() - .count(), - 0 + .unwrap(), + None ); }); } diff --git a/substrate/frame/election-provider-multi-block/src/verifier/impls.rs b/substrate/frame/election-provider-multi-block/src/verifier/impls.rs index c3713c332a11..c05f0d543c89 100644 --- a/substrate/frame/election-provider-multi-block/src/verifier/impls.rs +++ b/substrate/frame/election-provider-multi-block/src/verifier/impls.rs @@ -62,19 +62,19 @@ pub(crate) mod pallet { #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] - pub enum Event { + pub enum Event { /// A verificaction failed at the given page. - VerificationFailed(PageIndex, FeasibilityError), + VerificationFailed { page: PageIndex, error: FeasibilityError }, /// The final verifications of the `finalize_verification` failed. If this error happened, /// all the single pages passed the feasibility checks. - FinalVerificationFailed(FeasibilityError), + FinalVerificationFailed { error: FeasibilityError }, /// The given page has been correctly verified, with the number of backers that are part of /// the page. - Verified(PageIndex, u32), + Verified { page: PageIndex, backers: u32 }, /// A new solution with the given score has replaced the previous best solution, if any. - Queued(ElectionScore, Option), + Queued { score: ElectionScore, old_score: Option }, /// The solution data was not available for a specific page. - SolutionDataUnavailable(PageIndex), + SolutionDataUnavailable { page: PageIndex }, } /// A wrapper type of the storage items related to the queued solution. @@ -377,8 +377,11 @@ impl Verifier for Pallet { partial_score, page ); - Self::deposit_event(Event::::Verified(page, supports.len() as u32)); - Self::deposit_event(Event::::Queued(partial_score, maybe_current_score)); + Self::deposit_event(Event::::Verified { page, backers: supports.len() as u32 }); + Self::deposit_event(Event::::Queued { + score: partial_score, + old_score: maybe_current_score, + }); Ok(supports) }, Err(err) => { @@ -389,7 +392,7 @@ impl Verifier for Pallet { err, page ); - Self::deposit_event(Event::::VerificationFailed(page, err.clone())); + Self::deposit_event(Event::::VerificationFailed { page, error: err.clone() }); Err(err) }, } @@ -448,10 +451,10 @@ impl AsyncVerifier for Pallet { let claimed_score = Self::SolutionDataProvider::get_score().unwrap_or_default(); if Self::ensure_score_quality(claimed_score).is_err() { - Self::deposit_event(Event::::VerificationFailed( - CorePallet::::msp(), - FeasibilityError::ScoreTooLow, - )); + Self::deposit_event(Event::::VerificationFailed { + page: CorePallet::::msp(), + error: FeasibilityError::ScoreTooLow, + }); // report to the solution data provider that the page verification failed. Self::SolutionDataProvider::report_result(VerificationResult::Rejected); // despite the verification failed, this was a successful `start` operation. @@ -528,7 +531,7 @@ impl Pallet { VerificationStatus::::put(Status::Nothing); T::SolutionDataProvider::report_result(VerificationResult::DataUnavailable); - Self::deposit_event(Event::::SolutionDataUnavailable(current_page)); + Self::deposit_event(Event::::SolutionDataUnavailable { page: current_page }); return ::WeightInfo::on_initialize_ongoing_failed( max_backers_winner, @@ -542,7 +545,10 @@ impl Pallet { // TODO: can refator out some of these code blocks to clean up the code. let weight_consumed = match maybe_supports { Ok(supports) => { - Self::deposit_event(Event::::Verified(current_page, supports.len() as u32)); + Self::deposit_event(Event::::Verified { + page: current_page, + backers: supports.len() as u32, + }); QueuedSolution::::set_page(current_page, supports); if current_page > CorePallet::::lsp() { @@ -586,9 +592,12 @@ impl Pallet { } } }, - Err(err) => { + Err(error) => { // the paged solution is invalid. - Self::deposit_event(Event::::VerificationFailed(current_page, err)); + Self::deposit_event(Event::::VerificationFailed { + page: current_page, + error, + }); VerificationStatus::::put(Status::Nothing); QueuedSolution::::clear_invalid_and_backings(); T::SolutionDataProvider::report_result(VerificationResult::Rejected); @@ -639,10 +648,10 @@ impl Pallet { match (final_score == claimed_score, winner_count <= desired_targets) { (true, true) => { QueuedSolution::::finalize_solution(final_score); - Self::deposit_event(Event::::Queued( - final_score, - QueuedSolution::::queued_score(), - )); + Self::deposit_event(Event::::Queued { + score: final_score, + old_score: QueuedSolution::::queued_score(), + }); Ok(()) }, @@ -653,7 +662,10 @@ impl Pallet { }) .map_err(|err| { sublog!(warn, "verifier", "finalizing the solution was invalid due to {:?}", err); - Self::deposit_event(Event::::VerificationFailed(Zero::zero(), err.clone())); + Self::deposit_event(Event::::VerificationFailed { + page: Zero::zero(), + error: err.clone(), + }); err }); diff --git a/substrate/frame/election-provider-multi-block/src/verifier/tests.rs b/substrate/frame/election-provider-multi-block/src/verifier/tests.rs index e3234105a09d..983fefb79a16 100644 --- a/substrate/frame/election-provider-multi-block/src/verifier/tests.rs +++ b/substrate/frame/election-provider-multi-block/src/verifier/tests.rs @@ -1,4 +1,4 @@ -// This file is part of Substrate. +// Threports_result_rejection_workilpart of Substrate. // Copyright (C) 2022 Parity Technologies (UK) Ltd. // SPDX-License-Identifier: Apache-2.0 @@ -15,12 +15,13 @@ // See the License for the specific language governing permissions and // limitations under the License. +use super::*; use crate::{ mock::*, - verifier::{impls::pallet::*, SolutionDataProvider, *}, + verifier::{impls::pallet::*, SolutionDataProvider}, Phase, }; -use frame_support::{assert_err, assert_noop, assert_ok, testing_prelude::*, StorageMap}; +use frame_support::testing_prelude::*; use sp_npos_elections::ElectionScore; use sp_runtime::Perbill; @@ -416,7 +417,6 @@ mod async_verifier { } #[test] - #[should_panic(expected = "unexpected: selected leader without active submissions.")] fn reports_result_rejection_no_metadata_fails() { ExtBuilder::default() .minimum_score(ElectionScore { @@ -507,7 +507,10 @@ mod async_verifier { // score too low event submitted. assert_eq!( verifier_events(), - vec![Event::::VerificationFailed(2, FeasibilityError::ScoreTooLow,)] + vec![Event::::VerificationFailed { + page: 2, + error: FeasibilityError::ScoreTooLow, + }] ); }) } @@ -535,17 +538,8 @@ mod hooks { use frame_support::traits::Hooks; #[test] - fn on_initialize_status_nothing_returns_default_value() { - ExtBuilder::default().build_and_execute(|| { - ::set_status(Status::Nothing); - assert_eq!(VerifierPallet::on_initialize(0), Default::default()); - }); - } - - #[test] - fn on_initialize_solution_infeasible() { - ExtBuilder::default().build_and_execute(|| { - // solution insertion + fn on_initialize_ongoing_fails() { + ExtBuilder::default().pages(1).build_and_execute(|| { let round = current_round(); let score = ElectionScore { minimal_stake: 10, sum_stake: 10, sum_stake_squared: 10 }; let metadata = Submissions::submission_metadata_from( @@ -554,25 +548,35 @@ mod hooks { Default::default(), Default::default(), ); - Submissions::::insert_score_and_metadata(round, 1, Some(score), Some(metadata)); + Submissions::::insert_score_and_metadata( + round, + 1, + Some(score), + Some(metadata.clone()), + ); + // force ongoing status. ::set_status(Status::Ongoing(0)); assert_eq!(::status(), Status::Ongoing(0)); - assert_eq!(VerifierPallet::on_initialize(0), Default::default()); + // no events yet. + assert!(verifier_events().is_empty()); + + // progress the block. + roll_one(); - // TODO: zebedeusz - for some reason events list is empty even though the event deposit - // is executed assert_eq!( verifier_events(), - vec![Event::::VerificationFailed(0, FeasibilityError::ScoreTooLow)] + vec![Event::::VerificationFailed { + page: 0, + error: FeasibilityError::SnapshotUnavailable + }] ); - assert_eq!(VerificationStatus::::get(), Status::Nothing); }); } #[test] - fn on_initialize_feasible_solution_but_score_invalid() { + fn on_initialize_ongoing_invalid_score() { ExtBuilder::default().pages(1).desired_targets(2).build_and_execute(|| { // solution insertion let round = current_round(); @@ -596,8 +600,11 @@ mod hooks { assert_eq!( verifier_events(), vec![ - Event::::Verified(0, 0), - Event::::VerificationFailed(0, FeasibilityError::InvalidScore) + Event::::Verified { page: 0, backers: 0 }, + Event::::VerificationFailed { + page: 0, + error: FeasibilityError::InvalidScore + } ] ); assert!(QueuedSolution::::queued_score().is_none()); @@ -606,7 +613,7 @@ mod hooks { } #[test] - fn on_initialize_feasible_solution_and_valid_score() { + fn on_initialize_ongoing_works() { ExtBuilder::default().pages(1).desired_targets(2).build_and_execute(|| { roll_to_phase(Phase::Signed); let solution = mine_full(0).unwrap(); @@ -624,12 +631,15 @@ mod hooks { assert_eq!(VerifierPallet::on_initialize(0), Default::default()); - let events = verifier_events(); - assert!(events.len() > 0); assert_eq!( - events.get(0), - Some(&Event::::Queued(solution.score, Some(solution.score))) + verifier_events(), + vec![ + Event::::Queued { score: solution.score, old_score: Some(solution.score) }, + Event::Verified { page: 0, backers: 0 }, + Event::VerificationFailed { page: 0, error: FeasibilityError::InvalidScore }, + ] ); + assert_eq!(VerificationStatus::::get(), Status::Nothing); }); }