Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unit tests for pallet EPM-MB #6332

Merged
merged 16 commits into from
Nov 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions substrate/frame/election-provider-multi-block/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,11 @@
//! - pallet-unsigned
//! ```
//!
//! Each sub-pallet has a specific set of tasks and implement one or more interfaces to expose their
//! functionality to the core pallet:
//! Each sub-pallet has a specific set of tasks and implements one or more interfaces to expose
//! their functionality to the core pallet:
//! - The [`verifier`] pallet provides an implementation of the [`verifier::Verifier`] trait, which
//! exposes the functionality to verify NPoS solutions in a multi-block context. In addition, it
//! implements [`verifier::AsyncVerifier`] which verifies multi-paged solution asynchronously.
//! implements [`verifier::AsyncVerifier`] which verifies multi-paged solutions asynchronously.
//! - The [`signed`] pallet implements the signed phase, where off-chain entities commit to and
//! submit their election solutions. This pallet implements the
//! [`verifier::SolutionDataProvider`], which is used by the [`verifier`] pallet to fetch solution
Expand All @@ -62,7 +62,7 @@
//! ### Pallet ordering
//!
//! Due to the assumptions of when the `on_initialize` hook must be called by the executor for the
//! core pallet and each sub-pallets, it is crucial the the pallets are ordered correctly in the
//! core pallet and each sub-pallets, it is crucial that the pallets are ordered correctly in the
//! construct runtime. The ordering must be the following:
//!
//! ```text
Expand Down Expand Up @@ -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";

Expand Down
47 changes: 43 additions & 4 deletions substrate/frame/election-provider-multi-block/src/mock/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ use crate::{
};
use frame_support::{
derive_impl, pallet_prelude::*, parameter_types, traits::fungible::InspectHold,
weights::RuntimeDbWeight,
};
use parking_lot::RwLock;
use sp_runtime::{
Expand Down Expand Up @@ -66,6 +67,14 @@ pub type T = Runtime;
pub type Block = frame_system::mocking::MockBlock<Runtime>;
pub(crate) type Solver = SequentialPhragmen<AccountId, sp_runtime::PerU16, MaxBackersPerWinner, ()>;

pub struct Weighter;

impl Get<RuntimeDbWeight> for Weighter {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this? The type DbWeight can be set to unit in the configs in test mocks, ie.

 type DbWeight = ();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at test called on_initialize_returns_specific_weight_in_off_phase.
If DbWeight is set to () it fails.
So I defined it as such for the reason explained in the test.

fn get() -> RuntimeDbWeight {
return RuntimeDbWeight { read: 12, write: 12 }
}
}

frame_election_provider_support::generate_solution_type!(
#[compact]
pub struct TestNposSolution::<
Expand All @@ -80,6 +89,7 @@ frame_election_provider_support::generate_solution_type!(
impl frame_system::Config for Runtime {
type Block = Block;
type AccountData = pallet_balances::AccountData<Balance>;
type DbWeight = Weighter;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type DbWeight = Weighter;
type DbWeight = ();

}

parameter_types! {
Expand Down Expand Up @@ -274,13 +284,14 @@ impl ElectionProvider for MockFallback {

#[derive(Copy, Clone)]
pub struct ExtBuilder {
minimum_score: Option<ElectionScore>,
core_try_state: bool,
signed_try_state: bool,
}

impl Default for ExtBuilder {
fn default() -> Self {
Self { core_try_state: true, signed_try_state: true }
Self { core_try_state: true, signed_try_state: true, minimum_score: None }
}
}

Expand Down Expand Up @@ -337,7 +348,12 @@ impl ExtBuilder {
}

pub(crate) fn desired_targets(self, desired: u32) -> Self {
DesiredTargets::set(desired);
DesiredTargets::set(Ok(desired));
self
}

pub(crate) fn no_desired_targets(self) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this? I think we should remove this, also setting desired targets to Err("none") doesn't seem correct to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In short to make some actions fail due to desired targets not being configured.
Needed for:

  1. Test worker_fails_to_mine_solution
  2. Test desired_targets_not_in_snapshot- note: this one I will simplify a bit.

Are these not valid scenarios to test?
It seemed like valuable checks to me.

DesiredTargets::set(Err("none"));
self
}

Expand All @@ -351,6 +367,11 @@ impl ExtBuilder {
self
}

pub(crate) fn minimum_score(mut self, score: ElectionScore) -> Self {
self.minimum_score = Some(score);
self
}

pub(crate) fn core_try_state(mut self, enable: bool) -> Self {
self.core_try_state = enable;
self
Expand All @@ -362,8 +383,6 @@ impl ExtBuilder {
}

pub(crate) fn build(self) -> sp_io::TestExternalities {
sp_tracing::try_init_simple();

let mut storage = frame_system::GenesisConfig::<T>::default().build_storage().unwrap();
let _ = pallet_balances::GenesisConfig::<T> {
balances: vec![
Expand All @@ -390,6 +409,11 @@ impl ExtBuilder {
}
.assimilate_storage(&mut storage);

let _ = verifier_pallet::GenesisConfig::<T> {
minimum_score: self.minimum_score,
..Default::default()
}
.assimilate_storage(&mut storage);
sp_io::TestExternalities::from(storage)
}

Expand All @@ -414,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(|| {
Expand Down Expand Up @@ -520,6 +545,10 @@ pub fn roll_to_phase(phase: Phase<BlockNumber>) {
}
}

pub fn set_phase_to(phase: Phase<BlockNumber>) {
CurrentPhase::<Runtime>::set(phase);
}

pub fn roll_to_export() {
while !MultiPhase::current_phase().is_export() {
roll_to(System::block_number() + 1);
Expand Down Expand Up @@ -674,6 +703,16 @@ pub(crate) fn signed_events() -> Vec<crate::signed::Event<T>> {
.collect()
}

pub(crate) fn verifier_events() -> Vec<crate::verifier::Event<T>> {
System::events()
.into_iter()
.map(|r| r.event)
.filter_map(
|e| if let RuntimeEvent::VerifierPallet(inner) = e { Some(inner) } else { None },
)
.collect()
}

// TODO fix or use macro.
pub(crate) fn filter_events(
types: Vec<RuntimeEvent>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ parameter_types! {
(80, 80, bounded_vec![80]),
];
pub static EpochLength: u64 = 30;
pub static DesiredTargets: u32 = 5;
pub static DesiredTargets: data_provider::Result<u32> = Ok(5);

pub static LastIteratedTargetIndex: Option<usize> = None;
pub static LastIteratedVoterIndex: Option<usize> = None;
Expand Down Expand Up @@ -146,7 +146,7 @@ impl ElectionDataProvider for MockStaking {
fn desired_targets() -> data_provider::Result<u32> {
match DataProviderErrors::get() {
true => Err("MockDataProviderError"),
false => Ok(DesiredTargets::get()),
false => Ok(DesiredTargets::get()?),
}
}

Expand Down
45 changes: 38 additions & 7 deletions substrate/frame/election-provider-multi-block/src/signed/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ type BalanceOf<T> = <<T as Config>::Currency as FnInspect<AccountIdOf<T>>>::Bala
pub type CreditOf<T> = Credit<AccountIdOf<T>, <T as Config>::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,
Expand 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))]
Expand Down Expand Up @@ -321,7 +321,7 @@ pub mod pallet {

/// Wrapper for signed submissions.
///
/// It handle 3 storage items:
/// It handles 3 storage items:
///
/// 1. [`SortedScores`]: A flat, striclty sorted, vector with all the submission's scores. The
/// vector contains a tuple of `submitter_id` and `claimed_score`.
Expand Down Expand Up @@ -495,7 +495,9 @@ pub mod pallet {
who: &T::AccountId,
metadata: SubmissionMetadata<T>,
) {
debug_assert!(SortedScores::<T>::get(round).iter().any(|(account, _)| who == account));
// TODO: remove comment
//debug_assert!(SortedScores::<T>::get(round).iter().any(|(account, _)| who ==
// account));

Self::mutate_checked(round, || {
SubmissionMetadataStorage::<T>::insert(round, who, metadata);
Expand Down Expand Up @@ -580,7 +582,7 @@ pub mod pallet {
Submissions::<T>::scores_for(round).last().cloned()
}

/// Returns the metadata of a submitter for a given account.
/// Returns the metadata of a submitter for a given round.
pub(crate) fn metadata_for(
round: u32,
who: &T::AccountId,
Expand Down Expand Up @@ -723,6 +725,35 @@ pub mod pallet {
}
}

#[cfg(any(feature = "runtime-benchmarks", test))]
impl<T: Config> Submissions<T> {
pub(crate) fn submission_metadata_from(
claimed_score: ElectionScore,
pages: BoundedVec<bool, PagesOf<T>>,
deposit: BalanceOf<T>,
release_strategy: ReleaseStrategy,
) -> SubmissionMetadata<T> {
SubmissionMetadata { claimed_score, pages, deposit, release_strategy }
}

pub(crate) fn insert_score_and_metadata(
round: u32,
who: T::AccountId,
maybe_score: Option<ElectionScore>,
maybe_metadata: Option<SubmissionMetadata<T>>,
) {
if let Some(score) = maybe_score {
let mut scores = Submissions::<T>::scores_for(round);
scores.try_push((who.clone(), score)).unwrap();
SortedScores::<T>::insert(round, scores);
}

if let Some(metadata) = maybe_metadata {
SubmissionMetadataStorage::<T>::insert(round, who.clone(), metadata);
}
}
}

impl<T: Config> Pallet<T> {
pub(crate) fn do_register(
who: &T::AccountId,
Expand Down Expand Up @@ -839,7 +870,7 @@ pub mod pallet {
Ok(())
}

/// Force clean submissions storage for a given (`sumitter`, `round`) tuple.
/// Force clean submissions storage for a given (`submitter`, `round`) tuple.
///
/// This pallet expects that submitted pages for `round` may exist IFF a corresponding
/// metadata exists.
Expand Down Expand Up @@ -937,7 +968,7 @@ impl<T: Config> SolutionDataProvider for Pallet<T> {
})
}

/// Returns the score of the *best* solution in the queueu.
/// Returns the score of the *best* solution in the queue.
fn get_score() -> Option<ElectionScore> {
let round = CorePallet::<T>::current_round();
Submissions::<T>::leader(round).map(|(_who, score)| score)
Expand Down
Loading
Loading