From c023fa22219a5044270cef86800ea650ab5658d3 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Tue, 25 May 2021 19:43:54 +0200 Subject: [PATCH 01/15] Implementation but weird initial era in tests --- frame/staking/src/benchmarking.rs | 7 +- frame/staking/src/lib.rs | 191 +++++++++++++++++++----------- frame/staking/src/tests.rs | 44 ++++++- 3 files changed, 166 insertions(+), 76 deletions(-) diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 1d8a5c1fd6451..89c90d05e331e 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -91,7 +91,7 @@ pub fn create_validator_with_nominators( ValidatorCount::put(1); // Start a new Era - let new_validators = Staking::::new_era(SessionIndex::one()).unwrap(); + let new_validators = Staking::::try_trigger_new_era(SessionIndex::one()).unwrap(); assert_eq!(new_validators.len(), 1); assert_eq!(new_validators[0], v_stash, "Our validator was not selected!"); @@ -484,7 +484,8 @@ benchmarks! { )?; let session_index = SessionIndex::one(); }: { - let validators = Staking::::new_era(session_index).ok_or("`new_era` failed")?; + let validators = Staking::::try_trigger_new_era(session_index) + .ok_or("`new_era` failed")?; assert!(validators.len() == v as usize); } @@ -500,7 +501,7 @@ benchmarks! { None, )?; // Start a new Era - let new_validators = Staking::::new_era(SessionIndex::one()).unwrap(); + let new_validators = Staking::::try_trigger_new_era(SessionIndex::one()).unwrap(); assert!(new_validators.len() == v as usize); let current_era = CurrentEra::get().unwrap(); diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 67726f69228f7..4ef80372d83be 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -306,7 +306,7 @@ use sp_runtime::{ curve::PiecewiseLinear, traits::{ Convert, Zero, StaticLookup, CheckedSub, Saturating, SaturatedConversion, - AtLeast32BitUnsigned, + AtLeast32BitUnsigned, Bounded, }, }; use sp_staking::{ @@ -796,6 +796,8 @@ pub enum Forcing { /// Not forcing anything - just let whatever happen. NotForcing, /// Force a new era, then reset to `NotForcing` as soon as it is done. + /// Note that this will force to trigger an election until a new era is triggered, if the + /// election failed, the next session end will trigger a new election again, until success. ForceNew, /// Avoid a new era indefinitely. ForceNone, @@ -1103,6 +1105,8 @@ decl_event!( Withdrawn(AccountId, Balance), /// A nominator has been kicked from a validator. \[nominator, stash\] Kicked(AccountId, AccountId), + /// The election failed. No new era is planned. + StakingElectionFailed, } ); @@ -1654,6 +1658,12 @@ decl_module! { /// /// The dispatch origin must be Root. /// + /// # Warning + /// + /// The election process start multiple block before the end of the era. + /// Thus the election process may have started when this is called. In this case the + /// election will be ongoing until the next era is triggered. + /// /// # /// - No arguments. /// - Weight: O(1) @@ -1670,6 +1680,12 @@ decl_module! { /// /// The dispatch origin must be Root. /// + /// # Warning + /// + /// The election process start multiple block before the end of the era. + /// If this is called just before a new era is triggered, the election process may not + /// have enough block to get a result. + /// /// # /// - No arguments. /// - Weight: O(1) @@ -1720,6 +1736,12 @@ decl_module! { /// /// The dispatch origin must be Root. /// + /// # Warning + /// + /// The election process start multiple block before the end of the era. + /// If this is called just before a new era is triggered, the election process may not + /// have enough block to get a result. + /// /// # /// - Weight: O(1) /// - Write: ForceEra @@ -2131,11 +2153,11 @@ impl Module { .unwrap_or(0); // Must never happen. match ForceEra::get() { - // Will set to default again, which is `NotForcing`. - Forcing::ForceNew => ForceEra::kill(), - // Short circuit to `new_era`. + // Will be set to `NotForcing` again if a new era has been triggered. + Forcing::ForceNew => (), + // Short circuit to `try_trigger_new_era`. Forcing::ForceAlways => (), - // Only go to `new_era` if deadline reached. + // Only go to `try_trigger_new_era` if deadline reached. Forcing::NotForcing if era_length >= T::SessionsPerEra::get() => (), _ => { // either `Forcing::ForceNone`, @@ -2145,11 +2167,17 @@ impl Module { } // new era. - Self::new_era(session_index) + let new_era_validators = Self::try_trigger_new_era(session_index); + + if new_era_validators.is_some() && matches!(ForceEra::get(), Forcing::ForceNew) { + ForceEra::kill(); + } + + new_era_validators } else { // Set initial era log!(debug, "Starting the first era."); - Self::new_era(session_index) + Self::try_trigger_new_era(session_index) } } @@ -2246,77 +2274,102 @@ impl Module { } } - /// Plan a new era. Return the potential new staking set. - fn new_era(start_session_index: SessionIndex) -> Option> { + /// Plan a new era. + /// + /// * Bump the current era storage (which holds the latest planned era). + /// * Store start session index for the new planned era. + /// * Clean old era information. + /// * Store staking information for the new planned era + /// + /// Returns the new validator set. + pub fn new_era( + start_session_index: SessionIndex, + exposures: Vec<(T::AccountId, Exposure>)>, + ) -> Vec { // Increment or set current era. - let current_era = CurrentEra::mutate(|s| { + let new_planned_era = CurrentEra::mutate(|s| { *s = Some(s.map(|s| s + 1).unwrap_or(0)); s.unwrap() }); - ErasStartSessionIndex::insert(¤t_era, &start_session_index); + ErasStartSessionIndex::insert(&new_planned_era, &start_session_index); // Clean old era information. - if let Some(old_era) = current_era.checked_sub(Self::history_depth() + 1) { + if let Some(old_era) = new_planned_era.checked_sub(Self::history_depth() + 1) { Self::clear_era_information(old_era); } - // Set staking information for new era. - let maybe_new_validators = Self::enact_election(current_era); - - maybe_new_validators + // Set staking information for the new era. + Self::store_stakers_info(exposures, new_planned_era) } - /// Enact and process the election using the `ElectionProvider` type. + /// Potentially plan a new era. + /// + /// Get election result from `T::ElectionProvider`. + /// In case election result has more than [`MinimumValidatorCount`] validator trigger a new era. /// - /// This will also process the election, as noted in [`process_election`]. - fn enact_election(current_era: EraIndex) -> Option> { - T::ElectionProvider::elect() + /// In case a new era is planned, the new validator set is returned. + fn try_trigger_new_era(start_session_index: SessionIndex) -> Option> { + let (election_result, weight) = T::ElectionProvider::elect() .map_err(|e| { - log!(warn, "election provider failed due to {:?}", e) - }) - .and_then(|(res, weight)| { - >::register_extra_weight_unchecked( - weight, - frame_support::weights::DispatchClass::Mandatory, - ); - Self::process_election(res, current_era) + log!(warn, "election provider failed due to {:?}", e); + Self::deposit_event(RawEvent::StakingElectionFailed); }) - .ok() - } + .ok()?; - /// Process the output of the election. - /// - /// This ensures enough validators have been elected, converts all supports to exposures and - /// writes them to the associated storage. - /// - /// Returns `Err(())` if less than [`MinimumValidatorCount`] validators have been elected, `Ok` - /// otherwise. - pub fn process_election( - flat_supports: frame_election_provider_support::Supports, - current_era: EraIndex, - ) -> Result, ()> { - let exposures = Self::collect_exposures(flat_supports); - let elected_stashes = exposures.iter().cloned().map(|(x, _)| x).collect::>(); + >::register_extra_weight_unchecked( + weight, + frame_support::weights::DispatchClass::Mandatory, + ); - if (elected_stashes.len() as u32) < Self::minimum_validator_count().max(1) { + let exposures = Self::collect_exposures(election_result); + + if (exposures.len() as u32) < Self::minimum_validator_count().max(1) { // Session will panic if we ever return an empty validator set, thus max(1) ^^. - if current_era > 0 { - log!( + match CurrentEra::get() { + Some(current_era) if current_era > 0 => log!( warn, - "chain does not have enough staking candidates to operate for era {:?} ({} elected, minimum is {})", - current_era, - elected_stashes.len(), + "chain does not have enough staking candidates to operate for era {:?} ({} \ + elected, minimum is {})", + CurrentEra::get().unwrap_or(0), + exposures.len(), Self::minimum_validator_count(), - ); + ), + None => { + // The initial era is allowed to have no exposures. + // In this case the SessionManager is expected to choose a sensible validator + // set. + // TODO: this should be simplified #8911 + CurrentEra::put(0); + ErasStartSessionIndex::insert(&0, &start_session_index); + }, + _ => () } - return Err(()); + + Self::deposit_event(RawEvent::StakingElectionFailed); + + return None } + Self::deposit_event(RawEvent::StakingElection); + + + Some(Self::new_era(start_session_index, exposures)) + } + + /// Process the output of the election. + /// + /// Store staking information for the new planned era + pub fn store_stakers_info( + exposures: Vec<(T::AccountId, Exposure>)>, + new_planned_era: EraIndex, + ) -> Vec { + let elected_stashes = exposures.iter().cloned().map(|(x, _)| x).collect::>(); + // Populate stakers, exposures, and the snapshot of validator prefs. let mut total_stake: BalanceOf = Zero::zero(); exposures.into_iter().for_each(|(stash, exposure)| { total_stake = total_stake.saturating_add(exposure.total); - >::insert(current_era, &stash, &exposure); + >::insert(new_planned_era, &stash, &exposure); let mut exposure_clipped = exposure; let clipped_max_len = T::MaxNominatorRewardedPerValidator::get() as usize; @@ -2324,31 +2377,28 @@ impl Module { exposure_clipped.others.sort_by(|a, b| a.value.cmp(&b.value).reverse()); exposure_clipped.others.truncate(clipped_max_len); } - >::insert(¤t_era, &stash, exposure_clipped); + >::insert(&new_planned_era, &stash, exposure_clipped); }); // Insert current era staking information - >::insert(¤t_era, total_stake); + >::insert(&new_planned_era, total_stake); // collect the pref of all winners for stash in &elected_stashes { let pref = Self::validators(stash); - >::insert(¤t_era, stash, pref); + >::insert(&new_planned_era, stash, pref); } - // emit event - Self::deposit_event(RawEvent::StakingElection); - - if current_era > 0 { + if new_planned_era > 0 { log!( info, "new validator set of size {:?} has been processed for era {:?}", elected_stashes.len(), - current_era, + new_planned_era, ); } - Ok(elected_stashes) + elected_stashes } /// Consume a set of [`Supports`] from [`sp_npos_elections`] and collect them into a @@ -2581,18 +2631,23 @@ impl frame_election_provider_support::ElectionDataProvider Bounded::max_value(), + Forcing::ForceNew | Forcing::ForceAlways => Zero::zero(), + Forcing::NotForcing if era_length >= T::SessionsPerEra::get() => Zero::zero(), + Forcing::NotForcing => T::SessionsPerEra::get() + .saturating_sub(era_length) + // one session is computed in this_session_end. + .saturating_sub(1) + .into(), + }; now.saturating_add( until_this_session_end.saturating_add(sessions_left.saturating_mul(session_length)), diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index ec5a61d46885b..0e40f93fbc84e 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -440,13 +440,24 @@ fn no_candidate_emergency_condition() { let res = Staking::chill(Origin::signed(10)); assert_ok!(res); - // trigger era - mock::start_active_era(1); + let current_era = CurrentEra::get(); - // Previous ones are elected. chill is invalidates. TODO: #2494 + // try trigger new era + mock::run_to_block(20); + assert_eq!( + *staking_events().last().unwrap(), + RawEvent::StakingElectionFailed, + ); + // No new era is created + assert_eq!(current_era, CurrentEra::get()); + + // Go to far further session to see if validator have changed + mock::run_to_block(100); + + // Previous ones are elected. chill is not effective in active era (as era hasn't changed) assert_eq_uvec!(validator_controllers(), vec![10, 20, 30, 40]); - // Though the validator preferences has been removed. - assert!(Staking::validators(11) != prefs); + // The chill is still pending. + assert!(!::Validators::contains_key(11)); }); } @@ -3970,6 +3981,29 @@ mod election_data_provider { *staking_events().last().unwrap(), RawEvent::StakingElection ); + + Staking::force_no_eras(Origin::root()).unwrap(); + assert_eq!(Staking::next_election_prediction(System::block_number()), u64::max_value()); + + Staking::force_new_era_always(Origin::root()).unwrap(); + assert_eq!(Staking::next_election_prediction(System::block_number()), 45 + 5); + + Staking::force_new_era(Origin::root()).unwrap(); + assert_eq!(Staking::next_election_prediction(System::block_number()), 45 + 5); + + // Do a fail election + MinimumValidatorCount::put(1000); + run_to_block(50); + // Election: failed, next session is a new election + assert_eq!(Staking::next_election_prediction(System::block_number()), 50 + 5); + MinimumValidatorCount::put(2); + run_to_block(55); + assert_eq!(Staking::next_election_prediction(System::block_number()), 55 + 25); + assert_eq!(staking_events().len(), 6); + assert_eq!( + *staking_events().last().unwrap(), + RawEvent::StakingElection + ); }) } } From 0bb7d20333292e07e11c330355da28b912313ecf Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Fri, 28 May 2021 11:16:11 +0200 Subject: [PATCH 02/15] Emergency mode for elections. (#8918) --- Cargo.lock | 4 ++ .../election-provider-multi-phase/src/lib.rs | 64 +++++++++++++++---- 2 files changed, 57 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 39ec3e8c26b18..dbe983e79914b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6669,6 +6669,10 @@ dependencies = [ "jsonrpsee-ws-client", "log", "parity-scale-codec", +<<<<<<< HEAD +======= + "serde_json", +>>>>>>> 59b2fac1b... emergency mode: allow governance origin to provide solution to election in case of failure "sp-core", "sp-io", "sp-runtime", diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 9bec5cc4bd310..dfc7df8c2497f 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -311,9 +311,13 @@ pub enum Phase { /// advising validators not to bother running the unsigned offchain worker. /// /// As validator nodes are free to edit their OCW code, they could simply ignore this advisory - /// and always compute their own solution. However, by default, when the unsigned phase is passive, - /// the offchain workers will not bother running. + /// and always compute their own solution. However, by default, when the unsigned phase is + /// passive, the offchain workers will not bother running. Unsigned((bool, Bn)), + /// The emergency phase. This is enabled upon a failing call to `T::ElectionProvider::elect`. + /// After that, the only way to leave this phase is through a successful + /// `T::ElectionProvider::elect`. + Emergency, } impl Default for Phase { @@ -323,6 +327,11 @@ impl Default for Phase { } impl Phase { + /// Whether the phase is emergency or not. + pub fn is_emergency(&self) -> bool { + matches!(self, Phase::Emergency) + } + /// Whether the phase is signed or not. pub fn is_signed(&self) -> bool { matches!(self, Phase::Signed) @@ -581,7 +590,7 @@ pub mod pallet { /// Configuration for the fallback type Fallback: Get; - /// Origin that can set the minimum score. + /// Origin that can control this pallet. type ForceOrigin: EnsureOrigin; /// The configuration of benchmarking. @@ -793,6 +802,29 @@ pub mod pallet { >::set(maybe_next_score); Ok(()) } + + /// Set a solution in the queue, to be handed out to the client of this pallet in the next + /// call to `ElectionProvider::elect`. + /// + /// This can only be set by `T::ForceOrigin`, and only when the phase is `Emergency`. + /// + /// The solution is not checked for any feasibility and is assumed to be trustworthy, as any + /// feasibility check itself can in principle cause the election process to fail (due to + /// memory/weight constrains). + #[pallet::weight(T::DbWeight::get().reads_writes(1, 1))] + fn set_emergency_election_result( + origin: OriginFor, + solution: ReadySolution, + ) -> DispatchResult { + T::ForceOrigin::ensure_origin(origin)?; + ensure!(Self::current_phase().is_emergency(), >::CallNotAllowed); + + // Note: we don't `rotate_round` at this point; the next call to + // `ElectionProvider::elect` will not succeed and take care of that. + + >::put(solution); + Ok(()) + } } #[pallet::event] @@ -828,6 +860,8 @@ pub mod pallet { PreDispatchWeakSubmission, /// OCW submitted solution for wrong round OcwCallWrongEra, + /// The call is now allowed at this point. + CallNotAllowed, } #[pallet::origin] @@ -1162,14 +1196,14 @@ impl Pallet { /// 1. Increment round. /// 2. Change phase to [`Phase::Off`] /// 3. Clear all snapshot data. - fn post_elect() { - // inc round + fn rotate_round() { + // inc round. >::mutate(|r| *r = *r + 1); - // change phase + // phase is off now. >::put(Phase::Off); - // kill snapshots + // kill snapshots. Self::kill_snapshot(); } @@ -1219,10 +1253,18 @@ impl ElectionProvider for Pallet { type DataProvider = T::DataProvider; fn elect() -> Result<(Supports, Weight), Self::Error> { - let outcome_and_weight = Self::do_elect(); - // IMPORTANT: regardless of if election was `Ok` or `Err`, we shall do some cleanup. - Self::post_elect(); - outcome_and_weight + match Self::do_elect() { + Ok((supports, weight)) => { + // all went okay, put sign to be Off, clean snapshot, etc. + Self::rotate_round(); + Ok((supports, weight)) + }, + Err(why) => { + log!(error, "Entering emergency mode."); + >::put(Phase::Emergency); + Err(why) + } + } } } From 904048ea414fdfbf82476679be7140479dc99344 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Sat, 29 May 2021 09:32:50 +0200 Subject: [PATCH 03/15] do some testing, some logging. --- Cargo.lock | 4 ---- bin/node/cli/src/chain_spec.rs | 2 +- bin/node/runtime/src/constants.rs | 2 +- bin/node/runtime/src/lib.rs | 2 +- frame/election-provider-multi-phase/src/lib.rs | 1 + frame/staking/src/lib.rs | 15 +++++---------- 6 files changed, 9 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dbe983e79914b..39ec3e8c26b18 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6669,10 +6669,6 @@ dependencies = [ "jsonrpsee-ws-client", "log", "parity-scale-codec", -<<<<<<< HEAD -======= - "serde_json", ->>>>>>> 59b2fac1b... emergency mode: allow governance origin to provide solution to election in case of failure "sp-core", "sp-io", "sp-runtime", diff --git a/bin/node/cli/src/chain_spec.rs b/bin/node/cli/src/chain_spec.rs index eb3ee5124ac0a..5d3d37f650073 100644 --- a/bin/node/cli/src/chain_spec.rs +++ b/bin/node/cli/src/chain_spec.rs @@ -284,7 +284,7 @@ pub fn testnet_genesis( }).collect::>(), }, pallet_staking: StakingConfig { - validator_count: initial_authorities.len() as u32, + validator_count: (initial_authorities.len() * 2) as u32, // TODO: just for testing, revert at last. minimum_validator_count: initial_authorities.len() as u32, invulnerables: initial_authorities.iter().map(|x| x.0.clone()).collect(), slash_reward_fraction: Perbill::from_percent(10), diff --git a/bin/node/runtime/src/constants.rs b/bin/node/runtime/src/constants.rs index 2f6ad002a9283..1d780ab8c010d 100644 --- a/bin/node/runtime/src/constants.rs +++ b/bin/node/runtime/src/constants.rs @@ -63,7 +63,7 @@ pub mod time { // NOTE: Currently it is not possible to change the epoch duration after the chain has started. // Attempting to do so will brick block production. - pub const EPOCH_DURATION_IN_BLOCKS: BlockNumber = 10 * MINUTES; + pub const EPOCH_DURATION_IN_BLOCKS: BlockNumber = 1 * MINUTES; // TODO: just for testing, revert at last. pub const EPOCH_DURATION_IN_SLOTS: u64 = { const SLOT_FILL_RATE: f64 = MILLISECS_PER_BLOCK as f64 / SLOT_DURATION as f64; diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 92f3d43901a97..ad495ee8fb667 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -467,7 +467,7 @@ pallet_staking_reward_curve::build! { } parameter_types! { - pub const SessionsPerEra: sp_staking::SessionIndex = 6; + pub const SessionsPerEra: sp_staking::SessionIndex = 3; // TODO: just for testing, revert at last. pub const BondingDuration: pallet_staking::EraIndex = 24 * 28; pub const SlashDeferDuration: pallet_staking::EraIndex = 24 * 7; // 1/4 the bonding duration. pub const RewardCurve: &'static PiecewiseLinear<'static> = &REWARD_CURVE; diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index dfc7df8c2497f..72a38ce127448 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -611,6 +611,7 @@ pub mod pallet { let remaining = next_election - now; let current_phase = Self::current_phase(); + log!(trace, "current phase {:?}, next election {:?}", current_phase, next_election); match current_phase { Phase::Off if remaining <= signed_deadline && remaining > unsigned_deadline => { // NOTE: if signed-phase length is zero, second part of the if-condition fails. diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 4ef80372d83be..00d6d3ddac1a3 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -2142,7 +2142,6 @@ impl Module { fn new_session(session_index: SessionIndex) -> Option> { if let Some(current_era) = Self::current_era() { // Initial era has been set. - let current_era_start_session_index = Self::eras_start_session_index(current_era) .unwrap_or_else(|| { frame_support::print("Error: start_session_index must be set for current_era"); @@ -2166,16 +2165,15 @@ impl Module { }, } - // new era. - let new_era_validators = Self::try_trigger_new_era(session_index); - - if new_era_validators.is_some() && matches!(ForceEra::get(), Forcing::ForceNew) { + // New era. + let maybe_new_era_validators = Self::try_trigger_new_era(session_index); + if maybe_new_era_validators.is_some() && matches!(ForceEra::get(), Forcing::ForceNew) { ForceEra::kill(); } - new_era_validators + maybe_new_era_validators } else { - // Set initial era + // Set initial era. log!(debug, "Starting the first era."); Self::try_trigger_new_era(session_index) } @@ -2346,13 +2344,10 @@ impl Module { } Self::deposit_event(RawEvent::StakingElectionFailed); - return None } Self::deposit_event(RawEvent::StakingElection); - - Some(Self::new_era(start_session_index, exposures)) } From cb82d40e6b8eb83a95be79ffe3ea58c2ef95bec6 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Sat, 29 May 2021 10:03:37 +0200 Subject: [PATCH 04/15] some testing apparatus --- bin/node/runtime/src/lib.rs | 2 +- .../election-provider-multi-phase/src/lib.rs | 57 +++++++++++++++---- 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index ad495ee8fb667..50b361082d3da 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -508,7 +508,7 @@ parameter_types! { // fallback: no need to do on-chain phragmen initially. pub const Fallback: pallet_election_provider_multi_phase::FallbackStrategy = - pallet_election_provider_multi_phase::FallbackStrategy::OnChain; + pallet_election_provider_multi_phase::FallbackStrategy::Nothing; pub SolutionImprovementThreshold: Perbill = Perbill::from_rational(1u32, 10_000); diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 72a38ce127448..9cd46dc96c641 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -114,7 +114,24 @@ //! good solution is queued, then the fallback strategy [`pallet::Config::Fallback`] is used to //! determine what needs to be done. The on-chain election is slow, and contains no balancing or //! reduction post-processing. See [`onchain::OnChainSequentialPhragmen`]. The -//! [`FallbackStrategy::Nothing`] should probably only be used for testing, and returns an error. +//! [`FallbackStrategy::Nothing`] just returns an error, and enables the [`Phase::Emergency`]. +//! +//! ### Emergency Phase +//! +//! If, for any of the below reasons: +//! +//! 1. No signed and unsigned solution submitted +//! 2. Internal error +//! 3. Fallback being `None` +//! +//! A call to `T::ElectionProvider::elect` is made, and `Ok(_)` cannot be returned, then the pallet +//! proceeds to the [`Phase::Emergency`]. During this phase, any solution can be submitted from +//! [`T::ForceOrigin`], without any checking. Once submitted, the forced solution is kept in +//! [`QueuedSolution`] until the next call to `T::ElectionProvider::elect`, where it is returned and +//! [`Phase`] goes back to `Off`. +//! +//! This implies that the user of this pallet (i.e. a staking pallet) should re-try calling +//! `T::ElectionProvider::elect` in case of error until `OK(_)` is returned. //! //! ## Feasible Solution (correct solution) //! @@ -611,7 +628,13 @@ pub mod pallet { let remaining = next_election - now; let current_phase = Self::current_phase(); - log!(trace, "current phase {:?}, next election {:?}", current_phase, next_election); + log!( + trace, + "current phase {:?}, next election {:?}, metadata: {:?}", + current_phase, + next_election, + Self::snapshot_metadata() + ); match current_phase { Phase::Off if remaining <= signed_deadline && remaining > unsigned_deadline => { // NOTE: if signed-phase length is zero, second part of the if-condition fails. @@ -1254,17 +1277,27 @@ impl ElectionProvider for Pallet { type DataProvider = T::DataProvider; fn elect() -> Result<(Supports, Weight), Self::Error> { - match Self::do_elect() { - Ok((supports, weight)) => { - // all went okay, put sign to be Off, clean snapshot, etc. - Self::rotate_round(); - Ok((supports, weight)) - }, - Err(why) => { - log!(error, "Entering emergency mode."); - >::put(Phase::Emergency); - Err(why) + if Self::round() > 1 { + match Self::do_elect() { + Ok((supports, weight)) => { + // all went okay, put sign to be Off, clean snapshot, etc. + Self::rotate_round(); + Ok((supports, weight)) + } + Err(why) => { + log!(error, "Entering emergency mode."); + >::put(Phase::Emergency); + Err(why) + } } + } else { + // first round, always run on-chain. + // TODO: move all of this into a new trait like `GenesisElectionProvide`, or a new + // function in the same trait. + let result = Self::onchain_fallback(); + log!(info, "Finalized initial election round with onchain compute."); + Self::rotate_round(); + result } } } From 3bf17f30695314d1965696db56cafaa4e744d73c Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Thu, 3 Jun 2021 21:16:07 +0200 Subject: [PATCH 05/15] genesis election provider (#8970) * genesis election provider * fix historical stuff * Fix test * remove dbg --- Cargo.lock | 2 + bin/node/runtime/Cargo.toml | 1 + bin/node/runtime/src/lib.rs | 5 +- frame/babe/src/mock.rs | 1 + .../election-provider-multi-phase/src/lib.rs | 32 ++++------- frame/grandpa/src/mock.rs | 1 + frame/offences/benchmarking/src/mock.rs | 1 + frame/session/Cargo.toml | 2 + frame/session/benchmarking/src/mock.rs | 1 + frame/session/src/historical/mod.rs | 41 ++++++++++---- frame/session/src/lib.rs | 27 +++++----- frame/staking/src/benchmarking.rs | 6 +-- frame/staking/src/lib.rs | 54 +++++++++++++++---- frame/staking/src/mock.rs | 1 + 14 files changed, 117 insertions(+), 58 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5f1c123357ee6..aaa086ee28264 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4316,6 +4316,7 @@ name = "node-runtime" version = "2.0.1" dependencies = [ "frame-benchmarking", + "frame-election-provider-support", "frame-executive", "frame-support", "frame-system", @@ -5373,6 +5374,7 @@ dependencies = [ "frame-system", "impl-trait-for-tuples", "lazy_static", + "log", "pallet-timestamp", "parity-scale-codec", "sp-application-crypto", diff --git a/bin/node/runtime/Cargo.toml b/bin/node/runtime/Cargo.toml index 335d9a1aa2a98..f1eb30213e8e2 100644 --- a/bin/node/runtime/Cargo.toml +++ b/bin/node/runtime/Cargo.toml @@ -43,6 +43,7 @@ frame-benchmarking = { version = "3.1.0", default-features = false, path = "../. frame-support = { version = "3.0.0", default-features = false, path = "../../../frame/support" } frame-system = { version = "3.0.0", default-features = false, path = "../../../frame/system" } frame-system-benchmarking = { version = "3.0.0", default-features = false, path = "../../../frame/system/benchmarking", optional = true } +frame-election-provider-support = { version = "3.0.0", default-features = false, path = "../../../frame/election-provider-support" } frame-system-rpc-runtime-api = { version = "3.0.0", default-features = false, path = "../../../frame/system/rpc/runtime-api/" } frame-try-runtime = { version = "0.9.0", default-features = false, path = "../../../frame/try-runtime", optional = true } pallet-assets = { version = "3.0.0", default-features = false, path = "../../../frame/assets" } diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 11a50aef18b69..d09ee510065dd 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -467,7 +467,7 @@ pallet_staking_reward_curve::build! { } parameter_types! { - pub const SessionsPerEra: sp_staking::SessionIndex = 3; // TODO: just for testing, revert at last. + pub const SessionsPerEra: sp_staking::SessionIndex = 1; // TODO: just for testing, revert at last. pub const BondingDuration: pallet_staking::EraIndex = 24 * 28; pub const SlashDeferDuration: pallet_staking::EraIndex = 24 * 7; // 1/4 the bonding duration. pub const RewardCurve: &'static PiecewiseLinear<'static> = &REWARD_CURVE; @@ -475,6 +475,7 @@ parameter_types! { pub OffchainRepeat: BlockNumber = 5; } +use frame_election_provider_support::onchain; impl pallet_staking::Config for Runtime { const MAX_NOMINATIONS: u32 = MAX_NOMINATIONS; type Currency = Balances; @@ -498,6 +499,8 @@ impl pallet_staking::Config for Runtime { type NextNewSession = Session; type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; type ElectionProvider = ElectionProviderMultiPhase; + type GenesisElectionProvider = + onchain::OnChainSequentialPhragmen>; type WeightInfo = pallet_staking::weights::SubstrateWeight; } diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index 40ee782e721d6..7e7b36a456744 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -211,6 +211,7 @@ impl pallet_staking::Config for Test { type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; type NextNewSession = Session; type ElectionProvider = onchain::OnChainSequentialPhragmen; + type GenesisElectionProvider = Self::ElectionProvider; type WeightInfo = (); } diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 9cd46dc96c641..b32c9fdbf4df6 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -285,7 +285,7 @@ pub type CompactAccuracyOf = as CompactSolution>::Accuracy; pub type OnChainAccuracyOf = ::OnChainAccuracy; /// Wrapper type that implements the configurations needed for the on-chain backup. -struct OnChainConfig(sp_std::marker::PhantomData); +pub struct OnChainConfig(sp_std::marker::PhantomData); impl onchain::Config for OnChainConfig { type AccountId = T::AccountId; type BlockNumber = T::BlockNumber; @@ -1277,27 +1277,17 @@ impl ElectionProvider for Pallet { type DataProvider = T::DataProvider; fn elect() -> Result<(Supports, Weight), Self::Error> { - if Self::round() > 1 { - match Self::do_elect() { - Ok((supports, weight)) => { - // all went okay, put sign to be Off, clean snapshot, etc. - Self::rotate_round(); - Ok((supports, weight)) - } - Err(why) => { - log!(error, "Entering emergency mode."); - >::put(Phase::Emergency); - Err(why) - } + match Self::do_elect() { + Ok((supports, weight)) => { + // all went okay, put sign to be Off, clean snapshot, etc. + Self::rotate_round(); + Ok((supports, weight)) + } + Err(why) => { + log!(error, "Entering emergency mode."); + >::put(Phase::Emergency); + Err(why) } - } else { - // first round, always run on-chain. - // TODO: move all of this into a new trait like `GenesisElectionProvide`, or a new - // function in the same trait. - let result = Self::onchain_fallback(); - log!(info, "Finalized initial election round with onchain compute."); - Self::rotate_round(); - result } } } diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index 1ab28f7752ef0..0bbd0cc966749 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -217,6 +217,7 @@ impl pallet_staking::Config for Test { type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; type NextNewSession = Session; type ElectionProvider = onchain::OnChainSequentialPhragmen; + type GenesisElectionProvider = Self::ElectionProvider; type WeightInfo = (); } diff --git a/frame/offences/benchmarking/src/mock.rs b/frame/offences/benchmarking/src/mock.rs index 9047120923ad6..c62521b945c8d 100644 --- a/frame/offences/benchmarking/src/mock.rs +++ b/frame/offences/benchmarking/src/mock.rs @@ -176,6 +176,7 @@ impl pallet_staking::Config for Test { type NextNewSession = Session; type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; type ElectionProvider = onchain::OnChainSequentialPhragmen; + type GenesisElectionProvider = Self::ElectionProvider; type WeightInfo = (); } diff --git a/frame/session/Cargo.toml b/frame/session/Cargo.toml index 44e1f2f67858b..efe7bc133fb4d 100644 --- a/frame/session/Cargo.toml +++ b/frame/session/Cargo.toml @@ -24,6 +24,7 @@ frame-support = { version = "3.0.0", default-features = false, path = "../suppor frame-system = { version = "3.0.0", default-features = false, path = "../system" } pallet-timestamp = { version = "3.0.0", default-features = false, path = "../timestamp" } sp-trie = { version = "3.0.0", optional = true, default-features = false, path = "../../primitives/trie" } +log = { version = "0.4.0", default-features = false } impl-trait-for-tuples = "0.2.1" [dev-dependencies] @@ -44,5 +45,6 @@ std = [ "sp-staking/std", "pallet-timestamp/std", "sp-trie/std", + "log/std", ] try-runtime = ["frame-support/try-runtime"] diff --git a/frame/session/benchmarking/src/mock.rs b/frame/session/benchmarking/src/mock.rs index cf2fa8a07cfe0..f2d3a493ea9ae 100644 --- a/frame/session/benchmarking/src/mock.rs +++ b/frame/session/benchmarking/src/mock.rs @@ -181,6 +181,7 @@ impl pallet_staking::Config for Test { type NextNewSession = Session; type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; type ElectionProvider = onchain::OnChainSequentialPhragmen; + type GenesisElectionProvider = Self::ElectionProvider; type WeightInfo = (); } diff --git a/frame/session/src/historical/mod.rs b/frame/session/src/historical/mod.rs index 8902ebe551f6c..a0c38ae6a182d 100644 --- a/frame/session/src/historical/mod.rs +++ b/frame/session/src/historical/mod.rs @@ -124,10 +124,17 @@ impl ValidatorSetWithIdentification for Module { /// Specialization of the crate-level `SessionManager` which returns the set of full identification /// when creating a new session. -pub trait SessionManager: crate::SessionManager { +pub trait SessionManager: + crate::SessionManager +{ /// If there was a validator set change, its returns the set of new validators along with their /// full identifications. fn new_session(new_index: SessionIndex) -> Option>; + fn new_session_genesis( + new_index: SessionIndex, + ) -> Option> { + >::new_session(new_index) + } fn start_session(start_index: SessionIndex); fn end_session(end_index: SessionIndex); } @@ -136,19 +143,20 @@ pub trait SessionManager: crate::SessionManager /// sets the historical trie root of the ending session. pub struct NoteHistoricalRoot(sp_std::marker::PhantomData<(T, I)>); -impl crate::SessionManager for NoteHistoricalRoot - where I: SessionManager -{ - fn new_session(new_index: SessionIndex) -> Option> { - +impl> NoteHistoricalRoot { + fn do_new_session(new_index: SessionIndex, is_genesis: bool) -> Option> { StoredRange::mutate(|range| { range.get_or_insert_with(|| (new_index, new_index)).1 = new_index + 1; }); - let new_validators_and_id = >::new_session(new_index); - let new_validators = new_validators_and_id.as_ref().map(|new_validators| { - new_validators.iter().map(|(v, _id)| v.clone()).collect() - }); + let new_validators_and_id = if is_genesis { + >::new_session_genesis(new_index) + } else { + >::new_session(new_index) + }; + let new_validators = new_validators_and_id + .as_ref() + .map(|new_validators| new_validators.iter().map(|(v, _id)| v.clone()).collect()); if let Some(new_validators) = new_validators_and_id { let count = new_validators.len() as ValidatorCount; @@ -168,6 +176,19 @@ impl crate::SessionManager for NoteHistoricalRoot< new_validators } +} + +impl crate::SessionManager for NoteHistoricalRoot +where + I: SessionManager, +{ + fn new_session(new_index: SessionIndex) -> Option> { + Self::do_new_session(new_index, false) + } + + fn new_session_genesis(new_index: SessionIndex) -> Option> { + Self::do_new_session(new_index, true) + } fn start_session(start_index: SessionIndex) { >::start_session(start_index) diff --git a/frame/session/src/lib.rs b/frame/session/src/lib.rs index 8574979ef2fea..c9bfb0bcf7c61 100644 --- a/frame/session/src/lib.rs +++ b/frame/session/src/lib.rs @@ -238,12 +238,19 @@ pub trait SessionManager { /// `new_session(session)` is guaranteed to be called before `end_session(session-1)`. In other /// words, a new session must always be planned before an ongoing one can be finished. fn new_session(new_index: SessionIndex) -> Option>; + /// Same as `new_session`, but it this should only be called at genesis. + /// + /// The session manager might decide to treat this in a different way. Default impl is simply + /// using [`new_session`]. + fn new_session_genesis(new_index: SessionIndex) -> Option> { + Self::new_session(new_index) + } /// End the session. /// /// Because the session pallet can queue validator set the ending session can be lower than the /// last new session index. fn end_session(end_index: SessionIndex); - /// Start the session. + /// Start an already planned the session. /// /// The session start to be used for validation. fn start_session(start_index: SessionIndex); @@ -340,13 +347,9 @@ impl SessionHandler for Tuple { pub struct TestSessionHandler; impl SessionHandler for TestSessionHandler { const KEY_TYPE_IDS: &'static [KeyTypeId] = &[sp_runtime::key_types::DUMMY]; - fn on_genesis_session(_: &[(AId, Ks)]) {} - fn on_new_session(_: bool, _: &[(AId, Ks)], _: &[(AId, Ks)]) {} - fn on_before_session_ending() {} - fn on_disabled(_: usize) {} } @@ -451,7 +454,7 @@ decl_storage! { } } - let initial_validators_0 = T::SessionManager::new_session(0) + let initial_validators_0 = T::SessionManager::new_session_genesis(0) .unwrap_or_else(|| { frame_support::print("No initial validator provided by `SessionManager`, use \ session config keys to generate initial validator set."); @@ -459,7 +462,7 @@ decl_storage! { }); assert!(!initial_validators_0.is_empty(), "Empty validator set for session 0 in genesis block!"); - let initial_validators_1 = T::SessionManager::new_session(1) + let initial_validators_1 = T::SessionManager::new_session_genesis(1) .unwrap_or_else(|| initial_validators_0.clone()); assert!(!initial_validators_1.is_empty(), "Empty validator set for session 1 in genesis block!"); @@ -548,7 +551,7 @@ decl_module! { /// Actual cost depends on the number of length of `T::Keys::key_ids()` which is fixed. /// - DbReads: `T::ValidatorIdOf`, `NextKeys`, `origin account` /// - DbWrites: `NextKeys`, `origin account` - /// - DbWrites per key id: `KeyOwnder` + /// - DbWrites per key id: `KeyOwner` /// # #[weight = T::WeightInfo::purge_keys()] pub fn purge_keys(origin) { @@ -573,17 +576,17 @@ decl_module! { } impl Module { - /// Move on to next session. Register new validator set and session keys. Changes - /// to the validator set have a session of delay to take effect. This allows for - /// equivocation punishment after a fork. + /// Move on to next session. Register new validator set and session keys. Changes to the + /// validator set have a session of delay to take effect. This allows for equivocation + /// punishment after a fork. pub fn rotate_session() { let session_index = CurrentIndex::get(); + log::trace!(target: "runtime::session", "rotating session {:?}", session_index); let changed = QueuedChanged::get(); // Inform the session handlers that a session is going to end. T::SessionHandler::on_before_session_ending(); - T::SessionManager::end_session(session_index); // Get queued session keys and validators. diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 89c90d05e331e..973f34ce5236d 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -91,7 +91,7 @@ pub fn create_validator_with_nominators( ValidatorCount::put(1); // Start a new Era - let new_validators = Staking::::try_trigger_new_era(SessionIndex::one()).unwrap(); + let new_validators = Staking::::try_trigger_new_era(SessionIndex::one(), true).unwrap(); assert_eq!(new_validators.len(), 1); assert_eq!(new_validators[0], v_stash, "Our validator was not selected!"); @@ -484,7 +484,7 @@ benchmarks! { )?; let session_index = SessionIndex::one(); }: { - let validators = Staking::::try_trigger_new_era(session_index) + let validators = Staking::::try_trigger_new_era(session_index, true) .ok_or("`new_era` failed")?; assert!(validators.len() == v as usize); } @@ -501,7 +501,7 @@ benchmarks! { None, )?; // Start a new Era - let new_validators = Staking::::try_trigger_new_era(SessionIndex::one()).unwrap(); + let new_validators = Staking::::try_trigger_new_era(SessionIndex::one(), true).unwrap(); assert!(new_validators.len() == v as usize); let current_era = CurrentEra::get().unwrap(); diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 00d6d3ddac1a3..612bc77ab89a8 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -738,6 +738,13 @@ pub trait Config: frame_system::Config + SendTransactionTypes> { DataProvider = Module, >; + /// Something that provides the election functionality at genesis. + type GenesisElectionProvider: frame_election_provider_support::ElectionProvider< + Self::AccountId, + Self::BlockNumber, + DataProvider = Module, + >; + /// Maximum number of nominations per nominator. const MAX_NOMINATIONS: u32; @@ -2139,7 +2146,7 @@ impl Module { } /// Plan a new session potentially trigger a new era. - fn new_session(session_index: SessionIndex) -> Option> { + fn new_session(session_index: SessionIndex, is_genesis: bool) -> Option> { if let Some(current_era) = Self::current_era() { // Initial era has been set. let current_era_start_session_index = Self::eras_start_session_index(current_era) @@ -2166,7 +2173,7 @@ impl Module { } // New era. - let maybe_new_era_validators = Self::try_trigger_new_era(session_index); + let maybe_new_era_validators = Self::try_trigger_new_era(session_index, is_genesis); if maybe_new_era_validators.is_some() && matches!(ForceEra::get(), Forcing::ForceNew) { ForceEra::kill(); } @@ -2175,7 +2182,7 @@ impl Module { } else { // Set initial era. log!(debug, "Starting the first era."); - Self::try_trigger_new_era(session_index) + Self::try_trigger_new_era(session_index, is_genesis) } } @@ -2306,13 +2313,19 @@ impl Module { /// In case election result has more than [`MinimumValidatorCount`] validator trigger a new era. /// /// In case a new era is planned, the new validator set is returned. - fn try_trigger_new_era(start_session_index: SessionIndex) -> Option> { - let (election_result, weight) = T::ElectionProvider::elect() - .map_err(|e| { + fn try_trigger_new_era(start_session_index: SessionIndex, is_genesis: bool) -> Option> { + let (election_result, weight) = if is_genesis { + T::GenesisElectionProvider::elect().map_err(|e| { + log!(warn, "genesis election provider failed due to {:?}", e); + Self::deposit_event(RawEvent::StakingElectionFailed); + }) + } else { + T::ElectionProvider::elect().map_err(|e| { log!(warn, "election provider failed due to {:?}", e); Self::deposit_event(RawEvent::StakingElectionFailed); }) - .ok()?; + } + .ok()?; >::register_extra_weight_unchecked( weight, @@ -2707,16 +2720,21 @@ impl frame_election_provider_support::ElectionDataProvider pallet_session::SessionManager for Module { fn new_session(new_index: SessionIndex) -> Option> { - log!(trace, "planning new_session({})", new_index); + log!(trace, "planning new session {}", new_index); CurrentPlannedSession::put(new_index); - Self::new_session(new_index) + Self::new_session(new_index, false) + } + fn new_session_genesis(new_index: SessionIndex) -> Option> { + log!(trace, "planning new session {} at genesis", new_index); + CurrentPlannedSession::put(new_index); + Self::new_session(new_index, true) } fn start_session(start_index: SessionIndex) { - log!(trace, "starting start_session({})", start_index); + log!(trace, "starting session {}", start_index); Self::start_session(start_index) } fn end_session(end_index: SessionIndex) { - log!(trace, "ending end_session({})", end_index); + log!(trace, "ending session {}", end_index); Self::end_session(end_index) } } @@ -2738,6 +2756,20 @@ impl historical::SessionManager Option>)>> { + >::new_session_genesis(new_index).map(|validators| { + let current_era = Self::current_era() + // Must be some as a new era has been created. + .unwrap_or(0); + + validators.into_iter().map(|v| { + let exposure = Self::eras_stakers(current_era, &v); + (v, exposure) + }).collect() + }) + } fn start_session(start_index: SessionIndex) { >::start_session(start_index) } diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 4027ac1f670bc..464b91e0cc507 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -259,6 +259,7 @@ impl Config for Test { type NextNewSession = Session; type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator; type ElectionProvider = onchain::OnChainSequentialPhragmen; + type GenesisElectionProvider = Self::ElectionProvider; type WeightInfo = (); } From 786c7e778109a2e0beadc632e9da960c4e090f01 Mon Sep 17 00:00:00 2001 From: Guillaume Thiolliere Date: Wed, 9 Jun 2021 17:56:51 +0200 Subject: [PATCH 06/15] Apply suggestions from code review Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com> Co-authored-by: Peter Goodspeed-Niklaus --- frame/election-provider-multi-phase/src/lib.rs | 7 +++---- frame/staking/src/lib.rs | 14 +++++++------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index b32c9fdbf4df6..9847313fc36be 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -120,9 +120,8 @@ //! //! If, for any of the below reasons: //! -//! 1. No signed and unsigned solution submitted +//! 1. No signed or unsigned solution submitted & Fallback is `None` //! 2. Internal error -//! 3. Fallback being `None` //! //! A call to `T::ElectionProvider::elect` is made, and `Ok(_)` cannot be returned, then the pallet //! proceeds to the [`Phase::Emergency`]. During this phase, any solution can be submitted from @@ -884,7 +883,7 @@ pub mod pallet { PreDispatchWeakSubmission, /// OCW submitted solution for wrong round OcwCallWrongEra, - /// The call is now allowed at this point. + /// The call is not allowed at this point. CallNotAllowed, } @@ -1284,7 +1283,7 @@ impl ElectionProvider for Pallet { Ok((supports, weight)) } Err(why) => { - log!(error, "Entering emergency mode."); + log!(error, "Entering emergency mode: {}", why); >::put(Phase::Emergency); Err(why) } diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 612bc77ab89a8..3f41822447d98 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -1667,9 +1667,9 @@ decl_module! { /// /// # Warning /// - /// The election process start multiple block before the end of the era. - /// Thus the election process may have started when this is called. In this case the - /// election will be ongoing until the next era is triggered. + /// The election process starts multiple blocks before the end of the era. + /// Thus the election process may be ongoing when this is called. In this case the + /// election will continue until the next era is triggered. /// /// # /// - No arguments. @@ -1689,9 +1689,9 @@ decl_module! { /// /// # Warning /// - /// The election process start multiple block before the end of the era. + /// The election process starts multiple blocks before the end of the era. /// If this is called just before a new era is triggered, the election process may not - /// have enough block to get a result. + /// have enough blocks to get a result. /// /// # /// - No arguments. @@ -1745,9 +1745,9 @@ decl_module! { /// /// # Warning /// - /// The election process start multiple block before the end of the era. + /// The election process starts multiple blocks before the end of the era. /// If this is called just before a new era is triggered, the election process may not - /// have enough block to get a result. + /// have enough blocks to get a result. /// /// # /// - Weight: O(1) From a71ad8dd9261674cec4ad94acc8980f874805301 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Wed, 9 Jun 2021 18:09:23 +0200 Subject: [PATCH 07/15] capitalize comment and name without conflict --- .../election-provider-multi-phase/src/lib.rs | 100 +++++++++--------- frame/session/src/historical/mod.rs | 4 +- frame/session/src/lib.rs | 2 +- frame/staking/src/lib.rs | 58 +++++----- 4 files changed, 82 insertions(+), 82 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 9847313fc36be..5cad09ecc51be 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -643,7 +643,7 @@ pub mod pallet { T::WeightInfo::on_initialize_open_signed().saturating_add(snap_weight) } Err(why) => { - // not much we can do about this at this point. + // Not much we can do about this at this point. log!(warn, "failed to open signed phase due to {:?}", why); T::WeightInfo::on_initialize_nothing() // NOTE: ^^ The trait specifies that this is a noop in terms of weight @@ -654,13 +654,13 @@ pub mod pallet { Phase::Signed | Phase::Off if remaining <= unsigned_deadline && remaining > Zero::zero() => { - // determine if followed by signed or not. + // Determine if followed by signed or not. let (need_snapshot, enabled, signed_weight) = if current_phase == Phase::Signed { - // followed by a signed phase: close the signed phase, no need for snapshot. + // Followed by a signed phase: close the signed phase, no need for snapshot. // TODO: proper weight https://github.com/paritytech/substrate/pull/7910. (false, true, Weight::zero()) } else { - // no signed phase: create a new snapshot, definitely `enable` the unsigned + // No signed phase: create a new snapshot, definitely `enable` the unsigned // phase. (true, true, Weight::zero()) }; @@ -677,7 +677,7 @@ pub mod pallet { base_weight.saturating_add(snap_weight).saturating_add(signed_weight) } Err(why) => { - // not much we can do about this at this point. + // Not much we can do about this at this point. log!(warn, "failed to open unsigned phase due to {:?}", why); T::WeightInfo::on_initialize_nothing() // NOTE: ^^ The trait specifies that this is a noop in terms of weight @@ -692,7 +692,7 @@ pub mod pallet { fn offchain_worker(now: T::BlockNumber) { use sp_runtime::offchain::storage_lock::{StorageLock, BlockAndTime}; - // create a lock with the maximum deadline of number of blocks in the unsigned phase. + // Create a lock with the maximum deadline of number of blocks in the unsigned phase. // This should only come useful in an **abrupt** termination of execution, otherwise the // guard will be dropped upon successful execution. let mut lock = StorageLock::>>::with_block_deadline( @@ -718,7 +718,7 @@ pub mod pallet { assert!(size_of::>() <= size_of::()); // ---------------------------- - // based on the requirements of [`sp_npos_elections::Assignment::try_normalize`]. + // Based on the requirements of [`sp_npos_elections::Assignment::try_normalize`]. let max_vote: usize = as CompactSolution>::LIMIT; // 1. Maximum sum of [ChainAccuracy; 16] must fit into `UpperOf`.. @@ -792,7 +792,7 @@ pub mod pallet { // Check score being an improvement, phase, and desired targets. Self::unsigned_pre_dispatch_checks(&solution).expect(error_message); - // ensure witness was correct. + // Ensure witness was correct. let SolutionOrSnapshotSize { voters, targets } = Self::snapshot_metadata().expect(error_message); @@ -803,7 +803,7 @@ pub mod pallet { let ready = Self::feasibility_check(solution, ElectionCompute::Unsigned).expect(error_message); - // store the newly received solution. + // Store the newly received solution. log!(info, "queued unsigned solution with score {:?}", ready.score); >::put(ready); Self::deposit_event(Event::SolutionStored(ElectionCompute::Unsigned)); @@ -894,7 +894,7 @@ pub mod pallet { type Call = Call; fn validate_unsigned(source: TransactionSource, call: &Self::Call) -> TransactionValidity { if let Call::submit_unsigned(solution, _) = call { - // discard solution not coming from the local OCW. + // Discard solution not coming from the local OCW. match source { TransactionSource::Local | TransactionSource::InBlock => { /* allowed */ } _ => { @@ -916,10 +916,10 @@ pub mod pallet { solution.score[0].saturated_into() ), ) - // used to deduplicate unsigned solutions: each validator should produce one + // Used to deduplicate unsigned solutions: each validator should produce one // solution per round at most, and solutions are not propagate. .and_provides(solution.round) - // transaction should stay in the pool for the duration of the unsigned phase. + // Transaction should stay in the pool for the duration of the unsigned phase. .longevity(T::UnsignedPhase::get().saturated_into::()) // We don't propagate this. This can never be validated at a remote node. .propagate(false) @@ -1006,14 +1006,14 @@ impl Pallet { log!(trace, "lock for offchain worker acquired."); match Self::current_phase() { Phase::Unsigned((true, opened)) if opened == now => { - // mine a new solution, cache it, and attempt to submit it + // Mine a new solution, cache it, and attempt to submit it let initial_output = Self::ensure_offchain_repeat_frequency(now).and_then(|_| { Self::mine_check_save_submit() }); log!(debug, "initial offchain thread output: {:?}", initial_output); } Phase::Unsigned((true, opened)) if opened < now => { - // try and resubmit the cached solution, and recompute ONLY if it is not + // Try and resubmit the cached solution, and recompute ONLY if it is not // feasible. let resubmit_output = Self::ensure_offchain_repeat_frequency(now).and_then(|_| { Self::restore_or_compute_then_maybe_submit() @@ -1023,7 +1023,7 @@ impl Pallet { _ => {} } - // after election finalization, clear OCW solution storage. + // After election finalization, clear OCW solution storage. if >::events() .into_iter() .filter_map(|event_record| { @@ -1063,7 +1063,7 @@ impl Pallet { now: T::BlockNumber, ) -> Result { let weight = if need_snapshot { - // if not being followed by a signed phase, then create the snapshots. + // If not being followed by a signed phase, then create the snapshots. debug_assert!(Self::snapshot().is_none()); Self::create_snapshot()? } else { @@ -1093,13 +1093,13 @@ impl Pallet { let (desired_targets, w3) = T::DataProvider::desired_targets().map_err(ElectionError::DataProvider)?; - // defensive-only + // Defensive-only if targets.len() > target_limit || voters.len() > voter_limit { debug_assert!(false, "Snapshot limit has not been respected."); return Err(ElectionError::DataProvider("Snapshot too big for submission.")); } - // only write snapshot if all existed. + // Only write snapshot if all existed. >::put(SolutionOrSnapshotSize { voters: voters.len() as u32, targets: targets.len() as u32, @@ -1123,10 +1123,10 @@ impl Pallet { ) -> Result, FeasibilityError> { let RawSolution { compact, score, round } = solution; - // first, check round. + // First, check round. ensure!(Self::round() == round, FeasibilityError::InvalidRound); - // winners are not directly encoded in the solution. + // Winners are not directly encoded in the solution. let winners = compact.unique_targets(); let desired_targets = @@ -1137,7 +1137,7 @@ impl Pallet { // upon arrival, thus we would then remove it here. Given overlay it is cheap anyhow ensure!(winners.len() as u32 == desired_targets, FeasibilityError::WrongWinnerCount); - // ensure that the solution's score can pass absolute min-score. + // Ensure that the solution's score can pass absolute min-score. let submitted_score = solution.score.clone(); ensure!( Self::minimum_untrusted_score().map_or(true, |min_score| @@ -1146,7 +1146,7 @@ impl Pallet { FeasibilityError::UntrustedScoreTooLow ); - // read the entire snapshot. + // Read the entire snapshot. let RoundSnapshot { voters: snapshot_voters, targets: snapshot_targets } = Self::snapshot().ok_or(FeasibilityError::SnapshotUnavailable)?; @@ -1156,7 +1156,7 @@ impl Pallet { let target_at = helpers::target_at_fn::(&snapshot_targets); let voter_index = helpers::voter_index_fn_usize::(&cache); - // first, make sure that all the winners are sane. + // First, make sure that all the winners are sane. // OPTIMIZATION: we could first build the assignments, and then extract the winners directly // from that, as that would eliminate a little bit of duplicate work. For now, we keep them // separate: First extract winners separately from compact, and then assignments. This is @@ -1175,19 +1175,19 @@ impl Pallet { let _ = assignments .iter() .map(|ref assignment| { - // check that assignment.who is actually a voter (defensive-only). + // Check that assignment.who is actually a voter (defensive-only). // NOTE: while using the index map from `voter_index` is better than a blind linear // search, this *still* has room for optimization. Note that we had the index when // we did `compact -> assignment` and we lost it. Ideal is to keep the index around. - // defensive-only: must exist in the snapshot. + // Defensive-only: must exist in the snapshot. let snapshot_index = voter_index(&assignment.who).ok_or(FeasibilityError::InvalidVoter)?; - // defensive-only: index comes from the snapshot, must exist. + // Defensive-only: index comes from the snapshot, must exist. let (_voter, _stake, targets) = snapshot_voters.get(snapshot_index).ok_or(FeasibilityError::InvalidVoter)?; - // check that all of the targets are valid based on the snapshot. + // Check that all of the targets are valid based on the snapshot. if assignment.distribution.iter().any(|(d, _)| !targets.contains(d)) { return Err(FeasibilityError::InvalidVote); } @@ -1220,13 +1220,13 @@ impl Pallet { /// 2. Change phase to [`Phase::Off`] /// 3. Clear all snapshot data. fn rotate_round() { - // inc round. + // Inc round. >::mutate(|r| *r = *r + 1); - // phase is off now. + // Phase is off now. >::put(Phase::Off); - // kill snapshots. + // Kill snapshots. Self::kill_snapshot(); } @@ -1278,7 +1278,7 @@ impl ElectionProvider for Pallet { fn elect() -> Result<(Supports, Weight), Self::Error> { match Self::do_elect() { Ok((supports, weight)) => { - // all went okay, put sign to be Off, clean snapshot, etc. + // All went okay, put sign to be Off, clean snapshot, etc. Self::rotate_round(); Ok((supports, weight)) } @@ -1318,7 +1318,7 @@ mod feasibility_check { assert!(MultiPhase::current_phase().is_signed()); let solution = raw_solution(); - // for whatever reason it might be: + // For whatever reason it might be: >::kill(); assert_noop!( @@ -1371,7 +1371,7 @@ mod feasibility_check { assert_eq!(MultiPhase::snapshot().unwrap().targets.len(), 4); // ----------------------------------------------------^^ valid range is [0..3]. - // swap all votes from 3 to 4. This will ensure that the number of unique winners + // Swap all votes from 3 to 4. This will ensure that the number of unique winners // will still be 4, but one of the indices will be gibberish. Requirement is to make // sure 3 a winner, which we don't do here. solution @@ -1397,7 +1397,7 @@ mod feasibility_check { #[test] fn voter_indices() { - // should be caught in `compact.into_assignment`. + // Should be caught in `compact.into_assignment`. ExtBuilder::default().desired_targets(2).build_and_execute(|| { roll_to(::get() - ::get() - ::get()); assert!(MultiPhase::current_phase().is_signed()); @@ -1406,7 +1406,7 @@ mod feasibility_check { assert_eq!(MultiPhase::snapshot().unwrap().voters.len(), 8); // ----------------------------------------------------^^ valid range is [0..7]. - // check that there is a index 7 in votes1, and flip to 8. + // Check that there is a index 7 in votes1, and flip to 8. assert!( solution .compact @@ -1433,7 +1433,7 @@ mod feasibility_check { assert_eq!(MultiPhase::snapshot().unwrap().voters.len(), 8); // ----------------------------------------------------^^ valid range is [0..7]. - // first, check that voter at index 7 (40) actually voted for 3 (40) -- this is self + // First, check that voter at index 7 (40) actually voted for 3 (40) -- this is self // vote. Then, change the vote to 2 (30). assert_eq!( solution @@ -1461,7 +1461,7 @@ mod feasibility_check { let mut solution = raw_solution(); assert_eq!(MultiPhase::snapshot().unwrap().voters.len(), 8); - // simply faff with the score. + // Simply faff with the score. solution.score[0] += 1; assert_noop!( @@ -1521,7 +1521,7 @@ mod tests { assert_eq!(MultiPhase::current_phase(), Phase::Unsigned((true, 25))); assert!(MultiPhase::snapshot().is_some()); - // we close when upstream tells us to elect. + // We close when upstream tells us to elect. roll_to(32); assert_eq!(MultiPhase::current_phase(), Phase::Unsigned((true, 25))); assert!(MultiPhase::snapshot().is_some()); @@ -1604,7 +1604,7 @@ mod tests { roll_to(30); assert!(MultiPhase::current_phase().is_off()); - // this module is now only capable of doing on-chain backup. + // This module is now only capable of doing on-chain backup. assert_ok!(MultiPhase::elect()); assert!(MultiPhase::current_phase().is_off()); @@ -1613,9 +1613,9 @@ mod tests { #[test] fn early_termination() { - // an early termination in the signed phase, with no queued solution. + // An early termination in the signed phase, with no queued solution. ExtBuilder::default().build_and_execute(|| { - // signed phase started at block 15 and will end at 25. + // Signed phase started at block 15 and will end at 25. roll_to(14); assert_eq!(MultiPhase::current_phase(), Phase::Off); @@ -1624,11 +1624,11 @@ mod tests { assert_eq!(MultiPhase::current_phase(), Phase::Signed); assert_eq!(MultiPhase::round(), 1); - // an unexpected call to elect. + // An unexpected call to elect. roll_to(20); MultiPhase::elect().unwrap(); - // we surely can't have any feasible solutions. This will cause an on-chain election. + // We surely can't have any feasible solutions. This will cause an on-chain election. assert_eq!( multi_phase_events(), vec![ @@ -1636,7 +1636,7 @@ mod tests { Event::ElectionFinalized(Some(ElectionCompute::OnChain)) ], ); - // all storage items must be cleared. + // All storage items must be cleared. assert_eq!(MultiPhase::round(), 2); assert!(MultiPhase::snapshot().is_none()); assert!(MultiPhase::snapshot_metadata().is_none()); @@ -1654,7 +1654,7 @@ mod tests { roll_to(25); assert_eq!(MultiPhase::current_phase(), Phase::Unsigned((true, 25))); - // zilch solutions thus far. + // Zilch solutions thus far. let (supports, _) = MultiPhase::elect().unwrap(); assert_eq!( @@ -1673,7 +1673,7 @@ mod tests { roll_to(25); assert_eq!(MultiPhase::current_phase(), Phase::Unsigned((true, 25))); - // zilch solutions thus far. + // Zilch solutions thus far. assert_eq!(MultiPhase::elect().unwrap_err(), ElectionError::NoFallbackConfigured); }) } @@ -1683,15 +1683,15 @@ mod tests { ExtBuilder::default().build_and_execute(|| { Targets::set((0..(TargetIndex::max_value() as AccountId) + 1).collect::>()); - // signed phase failed to open. + // Signed phase failed to open. roll_to(15); assert_eq!(MultiPhase::current_phase(), Phase::Off); - // unsigned phase failed to open. + // Unsigned phase failed to open. roll_to(25); assert_eq!(MultiPhase::current_phase(), Phase::Off); - // on-chain backup works though. + // On-chain backup works though. roll_to(29); let (supports, _) = MultiPhase::elect().unwrap(); assert!(supports.len() > 0); @@ -1706,7 +1706,7 @@ mod tests { let (solution, _) = MultiPhase::mine_solution(2).unwrap(); - // default solution has a score of [50, 100, 5000]. + // Default solution has a score of [50, 100, 5000]. assert_eq!(solution.score, [50, 100, 5000]); >::put([49, 0, 0]); diff --git a/frame/session/src/historical/mod.rs b/frame/session/src/historical/mod.rs index a0c38ae6a182d..3cfcbf98bf38c 100644 --- a/frame/session/src/historical/mod.rs +++ b/frame/session/src/historical/mod.rs @@ -154,7 +154,7 @@ impl> NoteHi } else { >::new_session(new_index) }; - let new_validators = new_validators_and_id + let new_validators_opt = new_validators_and_id .as_ref() .map(|new_validators| new_validators.iter().map(|(v, _id)| v.clone()).collect()); @@ -174,7 +174,7 @@ impl> NoteHi } } - new_validators + new_validators_opt } } diff --git a/frame/session/src/lib.rs b/frame/session/src/lib.rs index c9bfb0bcf7c61..a14c0241981ba 100644 --- a/frame/session/src/lib.rs +++ b/frame/session/src/lib.rs @@ -250,7 +250,7 @@ pub trait SessionManager { /// Because the session pallet can queue validator set the ending session can be lower than the /// last new session index. fn end_session(end_index: SessionIndex); - /// Start an already planned the session. + /// Start an already planned session. /// /// The session start to be used for validation. fn start_session(start_index: SessionIndex); diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 3f41822447d98..e866cdebccc55 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -543,7 +543,7 @@ impl StakingLedger where if !slash_from_target.is_zero() { *target -= slash_from_target; - // don't leave a dust balance in the staking system. + // Don't leave a dust balance in the staking system. if *target <= minimum_balance { slash_from_target += *target; *value += sp_std::mem::replace(target, Zero::zero()); @@ -561,10 +561,10 @@ impl StakingLedger where slash_out_of(total, &mut chunk.value, &mut value); chunk.value }) - .take_while(|value| value.is_zero()) // take all fully-consumed chunks out. + .take_while(|value| value.is_zero()) // Take all fully-consumed chunks out. .count(); - // kill all drained chunks. + // Kill all drained chunks. let _ = self.unlocking.drain(..i); pre_total.saturating_sub(*total) @@ -734,7 +734,7 @@ pub trait Config: frame_system::Config + SendTransactionTypes> { type ElectionProvider: frame_election_provider_support::ElectionProvider< Self::AccountId, Self::BlockNumber, - // we only accept an election provider that has staking as data provider. + // We only accept an election provider that has staking as data provider. DataProvider = Module, >; @@ -1055,12 +1055,12 @@ pub mod migrations { /// check to execute prior to migration. pub fn pre_migrate() -> Result<(), &'static str> { - // these may or may not exist. + // These may or may not exist. log!(info, "SnapshotValidators.exits()? {:?}", SnapshotValidators::exists()); log!(info, "SnapshotNominators.exits()? {:?}", SnapshotNominators::exists()); log!(info, "QueuedElected.exits()? {:?}", QueuedElected::exists()); log!(info, "QueuedScore.exits()? {:?}", QueuedScore::exists()); - // these must exist. + // These must exist. assert!(IsCurrentSessionFinal::exists(), "IsCurrentSessionFinal storage item not found!"); assert!(EraElectionStatus::exists(), "EraElectionStatus storage item not found!"); Ok(()) @@ -1200,7 +1200,7 @@ decl_module! { } fn on_initialize(_now: T::BlockNumber) -> Weight { - // just return the weight of the on_finalize. + // Just return the weight of the on_finalize. T::DbWeight::get().reads(1) } @@ -1268,7 +1268,7 @@ decl_module! { Err(Error::::AlreadyPaired)? } - // reject a bond which is considered to be _dust_. + // Reject a bond which is considered to be _dust_. if value < T::Currency::minimum_balance() { Err(Error::::InsufficientValue)? } @@ -1330,7 +1330,7 @@ decl_module! { let extra = extra.min(max_additional); ledger.total += extra; ledger.active += extra; - // last check: the new active amount of ledger must be more than ED. + // Last check: the new active amount of ledger must be more than ED. ensure!(ledger.active >= T::Currency::minimum_balance(), Error::::InsufficientValue); Self::deposit_event(RawEvent::Bonded(stash, extra)); @@ -1443,7 +1443,7 @@ decl_module! { // portion to fall below existential deposit + will have no more unlocking chunks // left. We can now safely remove all staking-related information. Self::kill_stash(&stash, num_slashing_spans)?; - // remove the lock. + // Remove the lock. T::Currency::remove_lock(STAKING_ID, &stash); // This is worst case scenario, so we use the full weight and return None None @@ -1532,7 +1532,7 @@ decl_module! { let nominations = Nominations { targets, - // initial nominations are considered submitted at era 0. See `Nominations` doc + // Initial nominations are considered submitted at era 0. See `Nominations` doc submitted_in: Self::current_era().unwrap_or(0), suppressed: false, }; @@ -1732,10 +1732,10 @@ decl_module! { fn force_unstake(origin, stash: T::AccountId, num_slashing_spans: u32) { ensure_root(origin)?; - // remove all staking-related information. + // Remove all staking-related information. Self::kill_stash(&stash, num_slashing_spans)?; - // remove the lock. + // Remove the lock. T::Currency::remove_lock(STAKING_ID, &stash); } @@ -1846,7 +1846,7 @@ decl_module! { ensure!(!ledger.unlocking.is_empty(), Error::::NoUnlockChunk); let ledger = ledger.rebond(value); - // last check: the new active amount of ledger must be more than ED. + // Last check: the new active amount of ledger must be more than ED. ensure!(ledger.active >= T::Currency::minimum_balance(), Error::::InsufficientValue); Self::update_ledger(&controller, &ledger); @@ -2166,7 +2166,7 @@ impl Module { // Only go to `try_trigger_new_era` if deadline reached. Forcing::NotForcing if era_length >= T::SessionsPerEra::get() => (), _ => { - // either `Forcing::ForceNone`, + // Either `Forcing::ForceNone`, // or `Forcing::NotForcing if era_length >= T::SessionsPerEra::get()`. return None }, @@ -2241,12 +2241,12 @@ impl Module { if active_era > bonding_duration { let first_kept = active_era - bonding_duration; - // prune out everything that's from before the first-kept index. + // Prune out everything that's from before the first-kept index. let n_to_prune = bonded.iter() .take_while(|&&(era_idx, _)| era_idx < first_kept) .count(); - // kill slashing metadata. + // Kill slashing metadata. for (pruned_era, _) in bonded.drain(..n_to_prune) { slashing::clear_era_metadata::(pruned_era); } @@ -2391,7 +2391,7 @@ impl Module { // Insert current era staking information >::insert(&new_planned_era, total_stake); - // collect the pref of all winners + // Collect the pref of all winners for stash in &elected_stashes { let pref = Self::validators(stash); >::insert(&new_planned_era, stash, pref); @@ -2422,7 +2422,7 @@ impl Module { supports .into_iter() .map(|(validator, support)| { - // build `struct exposure` from `support` + // Build `struct exposure` from `support` let mut others = Vec::with_capacity(support.voters.len()); let mut own: BalanceOf = Zero::zero(); let mut total: BalanceOf = Zero::zero(); @@ -2557,12 +2557,12 @@ impl Module { let mut all_voters = Vec::new(); for (validator, _) in >::iter() { - // append self vote + // Append self vote let self_vote = (validator.clone(), weight_of(&validator), vec![validator.clone()]); all_voters.push(self_vote); } - // collect all slashing spans into a BTreeMap for further queries. + // Collect all slashing spans into a BTreeMap for further queries. let slashing_spans = >::iter().collect::>(); for (nominator, nominations) in >::iter() { @@ -2652,7 +2652,7 @@ impl frame_election_provider_support::ElectionDataProvider= T::SessionsPerEra::get() => Zero::zero(), Forcing::NotForcing => T::SessionsPerEra::get() .saturating_sub(era_length) - // one session is computed in this_session_end. + // One session is computed in this_session_end. .saturating_sub(1) .into(), }; @@ -2858,7 +2858,7 @@ where let active_era = Self::active_era(); add_db_reads_writes(1, 0); if active_era.is_none() { - // this offence need not be re-submitted. + // This offence need not be re-submitted. return consumed_weight } active_era.expect("value checked not to be `None`; qed").index @@ -2872,7 +2872,7 @@ where let window_start = active_era.saturating_sub(T::BondingDuration::get()); - // fast path for active-era report - most likely. + // Fast path for active-era report - most likely. // `slash_session` cannot be in a future active era. It must be in `active_era` or before. let slash_era = if slash_session >= active_era_start_session_index { active_era @@ -2880,10 +2880,10 @@ where let eras = BondedEras::get(); add_db_reads_writes(1, 0); - // reverse because it's more likely to find reports from recent eras. + // Reverse because it's more likely to find reports from recent eras. match eras.iter().rev().filter(|&&(_, ref sesh)| sesh <= &slash_session).next() { Some(&(ref slash_era, _)) => *slash_era, - // before bonding period. defensive - should be filtered out. + // Before bonding period. defensive - should be filtered out. None => return consumed_weight, } }; @@ -2929,7 +2929,7 @@ where } unapplied.reporters = details.reporters.clone(); if slash_defer_duration == 0 { - // apply right away. + // Apply right away. slashing::apply_slash::(unapplied); { let slash_cost = (6, 5); @@ -2940,7 +2940,7 @@ where ); } } else { - // defer to end of some `slash_defer_duration` from now. + // Defer to end of some `slash_defer_duration` from now. ::UnappliedSlashes::mutate( active_era, move |for_later| for_later.push(unapplied), @@ -2969,7 +2969,7 @@ where O: Offence, { fn report_offence(reporters: Vec, offence: O) -> Result<(), OffenceError> { - // disallow any slashing from before the current bonding period. + // Disallow any slashing from before the current bonding period. let offence_session = offence.session_index(); let bonded_eras = BondedEras::get(); From 883b0cc3998b078948ceb3e8419cd78b2617a35b Mon Sep 17 00:00:00 2001 From: thiolliere Date: Wed, 9 Jun 2021 18:13:27 +0200 Subject: [PATCH 08/15] fix log --- frame/election-provider-multi-phase/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 5cad09ecc51be..e296a7dd77120 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -1283,7 +1283,7 @@ impl ElectionProvider for Pallet { Ok((supports, weight)) } Err(why) => { - log!(error, "Entering emergency mode: {}", why); + log!(error, "Entering emergency mode: {:?}", why); >::put(Phase::Emergency); Err(why) } From 911bb03dc0bcba01c643ab47c4427683ced9ef35 Mon Sep 17 00:00:00 2001 From: Guillaume Thiolliere Date: Wed, 9 Jun 2021 18:21:41 +0200 Subject: [PATCH 09/15] Update frame/election-provider-multi-phase/src/lib.rs --- frame/election-provider-multi-phase/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index e296a7dd77120..7b4e879172825 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -120,7 +120,7 @@ //! //! If, for any of the below reasons: //! -//! 1. No signed or unsigned solution submitted & Fallback is `None` +//! 1. No signed or unsigned solution submitted & Fallback is `None` or failed //! 2. Internal error //! //! A call to `T::ElectionProvider::elect` is made, and `Ok(_)` cannot be returned, then the pallet From ce85c950848d1d97b0a1b34968bdb1a1f4a3c14e Mon Sep 17 00:00:00 2001 From: Guillaume Thiolliere Date: Wed, 9 Jun 2021 18:44:29 +0200 Subject: [PATCH 10/15] Update frame/election-provider-multi-phase/src/lib.rs Co-authored-by: Peter Goodspeed-Niklaus --- frame/election-provider-multi-phase/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 7b4e879172825..dc701207a6dae 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -843,7 +843,7 @@ pub mod pallet { ensure!(Self::current_phase().is_emergency(), >::CallNotAllowed); // Note: we don't `rotate_round` at this point; the next call to - // `ElectionProvider::elect` will not succeed and take care of that. + // `ElectionProvider::elect` will succeed and take care of that. >::put(solution); Ok(()) From 86dab55c65fc90d2ddd040a6a6a3dbd93807c7e6 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Wed, 9 Jun 2021 19:00:13 +0200 Subject: [PATCH 11/15] apply suggestion on tests --- frame/staking/src/tests.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 0e40f93fbc84e..f302c01c23b7a 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -458,6 +458,8 @@ fn no_candidate_emergency_condition() { assert_eq_uvec!(validator_controllers(), vec![10, 20, 30, 40]); // The chill is still pending. assert!(!::Validators::contains_key(11)); + // No new era is created + assert_eq!(current_era, CurrentEra::get()); }); } @@ -3996,6 +3998,9 @@ mod election_data_provider { run_to_block(50); // Election: failed, next session is a new election assert_eq!(Staking::next_election_prediction(System::block_number()), 50 + 5); + // The new era is still forced until a new era is planned + assert_eq!(ForceEra::get(), Forcing::ForceNew); + MinimumValidatorCount::put(2); run_to_block(55); assert_eq!(Staking::next_election_prediction(System::block_number()), 55 + 25); @@ -4004,6 +4009,8 @@ mod election_data_provider { *staking_events().last().unwrap(), RawEvent::StakingElection ); + // The new era has been planned, forcing is changed from `ForceNew` to `NotForcing` + assert_eq!(ForceEra::get(), Forcing::NotForcing); }) } } From 2f0b0a712f22b13c78c77bfa1a63341df09f6571 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Wed, 9 Jun 2021 19:06:09 +0200 Subject: [PATCH 12/15] remove testing modifications --- bin/node/cli/src/chain_spec.rs | 2 +- bin/node/runtime/src/constants.rs | 2 +- bin/node/runtime/src/lib.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/bin/node/cli/src/chain_spec.rs b/bin/node/cli/src/chain_spec.rs index 5d3d37f650073..eb3ee5124ac0a 100644 --- a/bin/node/cli/src/chain_spec.rs +++ b/bin/node/cli/src/chain_spec.rs @@ -284,7 +284,7 @@ pub fn testnet_genesis( }).collect::>(), }, pallet_staking: StakingConfig { - validator_count: (initial_authorities.len() * 2) as u32, // TODO: just for testing, revert at last. + validator_count: initial_authorities.len() as u32, minimum_validator_count: initial_authorities.len() as u32, invulnerables: initial_authorities.iter().map(|x| x.0.clone()).collect(), slash_reward_fraction: Perbill::from_percent(10), diff --git a/bin/node/runtime/src/constants.rs b/bin/node/runtime/src/constants.rs index 1d780ab8c010d..2f6ad002a9283 100644 --- a/bin/node/runtime/src/constants.rs +++ b/bin/node/runtime/src/constants.rs @@ -63,7 +63,7 @@ pub mod time { // NOTE: Currently it is not possible to change the epoch duration after the chain has started. // Attempting to do so will brick block production. - pub const EPOCH_DURATION_IN_BLOCKS: BlockNumber = 1 * MINUTES; // TODO: just for testing, revert at last. + pub const EPOCH_DURATION_IN_BLOCKS: BlockNumber = 10 * MINUTES; pub const EPOCH_DURATION_IN_SLOTS: u64 = { const SLOT_FILL_RATE: f64 = MILLISECS_PER_BLOCK as f64 / SLOT_DURATION as f64; diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index d09ee510065dd..d1c765cf1b7c9 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -467,7 +467,7 @@ pallet_staking_reward_curve::build! { } parameter_types! { - pub const SessionsPerEra: sp_staking::SessionIndex = 1; // TODO: just for testing, revert at last. + pub const SessionsPerEra: sp_staking::SessionIndex = 6; pub const BondingDuration: pallet_staking::EraIndex = 24 * 28; pub const SlashDeferDuration: pallet_staking::EraIndex = 24 * 7; // 1/4 the bonding duration. pub const RewardCurve: &'static PiecewiseLinear<'static> = &REWARD_CURVE; From 87b3701a64125efa07928c871f5ebcd54bc6e571 Mon Sep 17 00:00:00 2001 From: Guillaume Thiolliere Date: Thu, 10 Jun 2021 10:25:02 +0200 Subject: [PATCH 13/15] Apply suggestions from code review Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: Dmitry Kashitsyn --- frame/election-provider-multi-phase/src/lib.rs | 7 ++++--- frame/staking/src/lib.rs | 8 ++++---- frame/staking/src/tests.rs | 6 +++--- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index a5bffec701378..7801ac36e3a20 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -607,7 +607,8 @@ pub mod pallet { /// Configuration for the fallback type Fallback: Get; - /// Origin that can control this pallet. + /// Origin that can control this pallet. Note that any action taken by this origin (such) + /// as providing an emergency solution is not checked. Thus, it must be a trusted origin. type ForceOrigin: EnsureOrigin; /// The configuration of benchmarking. @@ -1094,7 +1095,7 @@ impl Pallet { let (desired_targets, w3) = T::DataProvider::desired_targets().map_err(ElectionError::DataProvider)?; - // Defensive-only + // Defensive-only. if targets.len() > target_limit || voters.len() > voter_limit { debug_assert!(false, "Snapshot limit has not been respected."); return Err(ElectionError::DataProvider("Snapshot too big for submission.")); @@ -1407,7 +1408,7 @@ mod feasibility_check { assert_eq!(MultiPhase::snapshot().unwrap().voters.len(), 8); // ----------------------------------------------------^^ valid range is [0..7]. - // Check that there is a index 7 in votes1, and flip to 8. + // Check that there is an index 7 in votes1, and flip to 8. assert!( solution .compact diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 0306f9617fbb7..18a47e3c9a2fd 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -2176,7 +2176,7 @@ impl Module { // New era. let maybe_new_era_validators = Self::try_trigger_new_era(session_index, is_genesis); if maybe_new_era_validators.is_some() && matches!(ForceEra::get(), Forcing::ForceNew) { - ForceEra::kill(); + ForceEra::put(Forcing::NotForcing); } maybe_new_era_validators @@ -2392,7 +2392,7 @@ impl Module { // Insert current era staking information >::insert(&new_planned_era, total_stake); - // Collect the pref of all winners + // Collect the pref of all winners. for stash in &elected_stashes { let pref = Self::validators(stash); >::insert(&new_planned_era, stash, pref); @@ -2423,7 +2423,7 @@ impl Module { supports .into_iter() .map(|(validator, support)| { - // Build `struct exposure` from `support` + // Build `struct exposure` from `support`. let mut others = Vec::with_capacity(support.voters.len()); let mut own: BalanceOf = Zero::zero(); let mut total: BalanceOf = Zero::zero(); @@ -2558,7 +2558,7 @@ impl Module { let mut all_voters = Vec::new(); for (validator, _) in >::iter() { - // Append self vote + // Append self vote. let self_vote = (validator.clone(), weight_of(&validator), vec![validator.clone()]); all_voters.push(self_vote); } diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index f302c01c23b7a..3dba9b59587ab 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -458,7 +458,7 @@ fn no_candidate_emergency_condition() { assert_eq_uvec!(validator_controllers(), vec![10, 20, 30, 40]); // The chill is still pending. assert!(!::Validators::contains_key(11)); - // No new era is created + // No new era is created. assert_eq!(current_era, CurrentEra::get()); }); } @@ -3998,7 +3998,7 @@ mod election_data_provider { run_to_block(50); // Election: failed, next session is a new election assert_eq!(Staking::next_election_prediction(System::block_number()), 50 + 5); - // The new era is still forced until a new era is planned + // The new era is still forced until a new era is planned. assert_eq!(ForceEra::get(), Forcing::ForceNew); MinimumValidatorCount::put(2); @@ -4009,7 +4009,7 @@ mod election_data_provider { *staking_events().last().unwrap(), RawEvent::StakingElection ); - // The new era has been planned, forcing is changed from `ForceNew` to `NotForcing` + // The new era has been planned, forcing is changed from `ForceNew` to `NotForcing`. assert_eq!(ForceEra::get(), Forcing::NotForcing); }) } From 641d394b4ea8e035154f914263fa2690f5cbc42e Mon Sep 17 00:00:00 2001 From: thiolliere Date: Thu, 10 Jun 2021 10:31:06 +0200 Subject: [PATCH 14/15] apply suggestion --- frame/staking/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 18a47e3c9a2fd..aa7cda25c8b58 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -2288,7 +2288,7 @@ impl Module { /// * Store staking information for the new planned era /// /// Returns the new validator set. - pub fn new_era( + pub fn trigger_new_era( start_session_index: SessionIndex, exposures: Vec<(T::AccountId, Exposure>)>, ) -> Vec { @@ -2362,7 +2362,7 @@ impl Module { } Self::deposit_event(RawEvent::StakingElection); - Some(Self::new_era(start_session_index, exposures)) + Some(Self::trigger_new_era(start_session_index, exposures)) } /// Process the output of the election. From cb0cb33fd688ce8db85607dddbce0f1f906bcdfa Mon Sep 17 00:00:00 2001 From: thiolliere Date: Mon, 14 Jun 2021 12:32:52 +0200 Subject: [PATCH 15/15] fix master merge --- frame/election-provider-multi-phase/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index e954f8c33d1ce..2bb47a8778074 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -837,7 +837,7 @@ pub mod pallet { /// feasibility check itself can in principle cause the election process to fail (due to /// memory/weight constrains). #[pallet::weight(T::DbWeight::get().reads_writes(1, 1))] - fn set_emergency_election_result( + pub fn set_emergency_election_result( origin: OriginFor, solution: ReadySolution, ) -> DispatchResult {