From 0139b15abbc02561dbce0ffea760f4b9e9724ca2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Mon, 27 May 2024 16:27:06 +0200 Subject: [PATCH] refactoring and improvements (still failing) --- polkadot/runtime/westend/src/lib.rs | 2 + .../src/migrations/v13_stake_tracker/mod.rs | 148 +++++++++++------- substrate/frame/staking/src/pallet/impls.rs | 91 +++++++---- 3 files changed, 155 insertions(+), 86 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index c2d2d68ec485..e8712047f27d 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -2714,6 +2714,8 @@ mod remote_tests { .unwrap(); ext.execute_with(|| { + //let mut meter = WeightMeter::with_limit(Weight::from_parts(100_779_206_000, + // 10477090)); let mut meter = WeightMeter::new(); let mut cursor = None; loop { diff --git a/substrate/frame/staking/src/migrations/v13_stake_tracker/mod.rs b/substrate/frame/staking/src/migrations/v13_stake_tracker/mod.rs index 29226cd5f805..db96eac60c84 100644 --- a/substrate/frame/staking/src/migrations/v13_stake_tracker/mod.rs +++ b/substrate/frame/staking/src/migrations/v13_stake_tracker/mod.rs @@ -20,9 +20,9 @@ //! sorted list of targets with a bags-list. use super::PALLET_MIGRATIONS_ID; -use crate::{log, weights, Config, Nominators, Pallet, StakerStatus}; +use crate::{log, weights, Config, Nominators, Pallet, StakerStatus, Validators}; use core::marker::PhantomData; -use frame_election_provider_support::SortedListProvider; +use frame_election_provider_support::{SortedListProvider, VoteWeight}; use frame_support::{ ensure, migrations::{MigrationId, SteppedMigration, SteppedMigrationError}, @@ -34,9 +34,6 @@ use sp_std::prelude::*; #[cfg(test)] mod tests; -#[cfg(test)] -const TRY_STATE_INTERVAL: usize = 10_000_000; - /// V13 Multi-block migration to introduce the stake-tracker pallet. /// /// A step of the migration consists of processing one nominator in the [`Nominators`] list. All @@ -50,8 +47,6 @@ const TRY_STATE_INTERVAL: usize = 10_000_000; /// - Ensure the new targets in the list are sorted per total stake (as per the underlying /// [`SortedListProvider`]). pub struct MigrationV13(PhantomData<(T, W)>); - -// TODO: check bonds again. impl, W: weights::WeightInfo> SteppedMigration for MigrationV13 { @@ -74,11 +69,12 @@ impl, W: weights::WeightInfo> SteppedMigration return Err(SteppedMigrationError::InsufficientWeight { required }); } - #[cfg(test)] - let mut counter = 0; + let mut chilled = 0; // do as much progress as possible per step. while meter.try_consume(required).is_ok() { + //TODO: only bench a sub-step function, e.g. Self::do_migrate_nominatior(who); + // fetch the next nominator to migrate. let mut iter = if let Some(ref last_nom) = cursor { Nominators::::iter_from(Nominators::::hashed_key_for(last_nom)) @@ -87,58 +83,28 @@ impl, W: weights::WeightInfo> SteppedMigration }; if let Some((nominator, _)) = iter.next() { - // try chill nominator. If chilled, skip migration of this nominator. + // try chill nominator. If chilled, skip migration of this nominator. The nominator + // is force-chilled because it may need to re-nominate with a different set of + // nominations (see below). Thus it is better to just chill the nominator and move + // on. if Self::try_chill_nominator(&nominator) { - log!(info, "nominator {:?} chilled, skip it.", nominator); + //log!(info, "nominator {:?} chilled, skip it.", nominator); + chilled += 1; + cursor = Some(nominator); continue; } // clean the nominations before migrating. This will ensure that the voter is not // nominating duplicate and/or dangling targets. let nominations = Self::clean_nominations(&nominator)?; - let nominator_stake = Pallet::::weight_of(&nominator); - log!( - info, - "mmb: processing nominator {:?} with {} nominations. remaining {} nominators to migrate.", - nominator, - nominations.len(), - iter.count() - ); - - // iter over up to `MaxNominationsOf` targets of `nominator`. + // iter over up to `MaxNominationsOf` targets of `nominator` and insert or + // update the target's approval's score. for target in nominations.into_iter() { - if let Ok(current_stake) = ::TargetList::get_score(&target) { - // target is in the target list. update with nominator's stake. - if let Some(total_stake) = current_stake.checked_add(nominator_stake.into()) - { - let _ = ::TargetList::on_update(&target, total_stake) - .defensive(); - } else { - log!(error, "target stake overflow. exit."); - return Err(SteppedMigrationError::Failed) - } + if ::TargetList::contains(&target) { + Self::update_target(&target, nominator_stake)?; } else { - // target is not in the target list, insert new node and consider self - // stake. - let self_stake = Pallet::::weight_of(&target); - if let Some(total_stake) = self_stake.checked_add(nominator_stake) { - let _ = - ::TargetList::on_insert(target, total_stake.into()) - .defensive(); - } else { - log!(error, "target stake overflow. exit."); - return Err(SteppedMigrationError::Failed) - } - } - } - - // enable partial checks for testing purposes only. - #[cfg(test)] - { - counter += 1; - if counter % TRY_STATE_INTERVAL == 0 { - Pallet::::do_try_state_approvals(Some(nominator.clone())).unwrap(); + Self::insert_target(&target, nominator_stake)?; } } @@ -146,6 +112,25 @@ impl, W: weights::WeightInfo> SteppedMigration cursor = Some(nominator) } else { // done, return earlier. + + // TODO: do this as the migration is performed -- not a large step at the end. + + let mut a = 0; + // but before, add active validators without any nominations. + for (validator, _) in Validators::::iter() { + if !::TargetList::contains(&validator) && + as StakingInterface>::status(&validator) == + Ok(StakerStatus::Validator) + { + a += 1; + let self_stake = Pallet::::weight_of(&validator); + ::TargetList::on_insert(validator, self_stake.into()) + .expect("node does not exist, checked above; qed."); + } + } + + log!(info, "Added validators with self stake: {:?}", a); + log!(info, "Chilled nominators: {:?}", chilled); return Ok(None) } } @@ -154,7 +139,59 @@ impl, W: weights::WeightInfo> SteppedMigration } } -impl MigrationV13 { +impl, W: weights::WeightInfo> MigrationV13 { + /// Inserts a new target in the list. + /// + /// Note: the caller must ensure that the target node does not exist in the list yet. + /// Oterhwise, use [`Self::update_target`]. + fn insert_target( + who: &T::AccountId, + nomination_stake: VoteWeight, + ) -> Result<(), SteppedMigrationError> { + let init_stake = match as StakingInterface>::status(&who) { + Ok(StakerStatus::Validator) => { + let self_stake = Pallet::::weight_of(&who); + if let Some(total_stake) = self_stake.checked_add(nomination_stake) { + total_stake + } else { + log!(error, "target stake overflow. exit."); + return Err(SteppedMigrationError::Failed) + } + }, + _ => nomination_stake, + }; + + match ::TargetList::on_insert(who.clone(), init_stake.into()) { + Err(e) => { + log!(error, "inserting {:?} in TL: {:?}", who, e); + Err(SteppedMigrationError::Failed) + }, + Ok(_) => Ok(()), + } + } + + /// Updates the target score in the target list. + /// + /// Note: the caller must ensure that the target node already exists in the list. Otherwise, + /// use [`Self::insert_target`]. + fn update_target( + who: &T::AccountId, + nomination_stake: VoteWeight, + ) -> Result<(), SteppedMigrationError> { + let current_stake = + ::TargetList::get_score(&who).expect("node is in the list"); + + if let Some(total_stake) = current_stake.checked_add(nomination_stake.into()) { + let _ = ::TargetList::on_update(&who, total_stake) + .map_err(|e| log!(error, "updating TL score of {:?}: {:?}", who, e)) + .defensive(); + Ok(()) + } else { + log!(error, "target stake overflow. exit."); + Err(SteppedMigrationError::Failed) + } + } + /// Chills a nominator if their active stake is below the minimum bond. pub(crate) fn try_chill_nominator(who: &T::AccountId) -> bool { if Pallet::::active_stake(&who).unwrap_or_default() < @@ -206,7 +243,12 @@ impl MigrationV13 { // update `who`'s nominations in staking or chill voter, if necessary. if nominations.len() == 0 { - let _ = as StakingInterface>::chill(&who).defensive(); + let _ = as StakingInterface>::chill(&who) + .map_err(|e| { + log!(error, "ERROR when chilling {:?}", who); + e + }) + .defensive(); } else if count_before > nominations.len() { as StakingInterface>::nominate(who, nominations.clone()).map_err(|e| { log!(error, "failed to migrate nominations {:?}.", e); diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 67b41bf1a6b8..282ed1c4c921 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -2110,6 +2110,7 @@ impl Pallet { "VoterList contains non-staker" ); + /* Self::check_ledgers()?; Self::check_bonded_consistency()?; Self::check_payees()?; @@ -2118,7 +2119,8 @@ impl Pallet { Self::check_paged_exposures()?; Self::check_count()?; Self::ensure_disabled_validators_sorted()?; - Self::do_try_state_approvals(None)?; + */ + Self::do_try_state_approvals()?; Self::do_try_state_target_sorting() } @@ -2471,10 +2473,8 @@ impl Pallet { /// score. /// * The number of target nodes in the target list matches the number of /// (active_validators + idle_validators + dangling_targets_score_with_score). - pub fn do_try_state_approvals( - maybe_last: Option, - ) -> Result<(), sp_runtime::TryRuntimeError> { - use sp_std::collections::btree_map::BTreeMap; + pub fn do_try_state_approvals() -> Result<(), sp_runtime::TryRuntimeError> { + use sp_std::collections::{btree_map::BTreeMap, btree_set::BTreeSet}; let mut approvals_map: BTreeMap = BTreeMap::new(); // build map of approvals stakes from the `Nominators` storage map POV. @@ -2484,7 +2484,6 @@ impl Pallet { // fail if nominator has duplicate nominations, unexpected in the context of // keeping track of the stake approvals. - use sp_std::collections::btree_set::BTreeSet; let count_before = nominations.len(); let dedup_noms: Vec = nominations .clone() @@ -2499,9 +2498,28 @@ impl Pallet { ); for target in nominations.into_iter() { + // skip if for some reason there is a nominator being nominated. + match Self::status(&target) { + Ok(StakerStatus::Nominator(_)) => { + log!( + warn, + "nominated stakers {:?} is a nominator, not a validator.", + target + ); + continue; + }, + _ => (), + } + if let Some(approvals) = approvals_map.get_mut(&target) { *approvals += vote.into(); } else { + // TODO: simplify + let self_stake = Pallet::::weight_of(&target); + let init_stake = self_stake.saturating_add(vote); + approvals_map.insert(target, init_stake.into()); + + /* // new addition to the map. add self-stake if validator is active. let _ = match Self::status(&target) { Ok(StakerStatus::Validator) => { @@ -2510,63 +2528,70 @@ impl Pallet { }, _ => approvals_map.insert(target, vote.into()), }; + */ } } - - // stop builing the approvals map at the cursor, if it is set. - match maybe_last { - Some(last) if voter == last => break, - _ => (), - } } + let mut a = 0; // add active validators without any nominations. for (validator, _) in Validators::::iter() { + // do not add validator if it is not in a good state. + match Self::inspect_bond_state(&validator) { + Ok(LedgerIntegrityState::Ok) | Err(_) => (), + _ => continue, + } + if !approvals_map.contains_key(&validator) { - let self_stake = Pallet::::weight_of(&validator); - approvals_map.insert(validator, self_stake.into()); + // skip if for some reason there is a nominator being nominated. + match Self::status(&validator) { + Ok(StakerStatus::Nominator(_)) => { + log!( + warn, + "nominated staker {:?} is a nominator, not a validator.", + validator + ); + }, + _ => { + a += 1; + let self_stake = Pallet::::weight_of(&validator); + approvals_map.insert(validator, self_stake.into()); + }, + } } } - let mut bad_count = 0; + let mut mismatch_approvals = 0; // compare calculated approvals per target with target list state. for (target, calculated_stake) in approvals_map.iter() { - let stake_in_list = if let Ok(stake) = T::TargetList::get_score(target) { - stake - } else { - continue; - }; + let stake_in_list = T::TargetList::get_score(target).unwrap(); if *calculated_stake != stake_in_list { - log!( - error, - "try-runtime: score of {:?} in `TargetList` list: {:?}, calculated sum of all stake: {:?}", - target, - stake_in_list, - calculated_stake, - ); + mismatch_approvals += 1; log!( error, - "{:?} score in the target list {:?} is different than the expected {:?}", + "try-runtime: score of {:?} in `TargetList` list: {:?}, calculated sum of all stake: {:?} -- status: {:?}", target, stake_in_list, calculated_stake, + Self::status(&target), ); - bad_count += 1; - //return Err("target score in the target list is different than the - // expected".into()) } } - log!(error, "BAD COUNT: {:?}", bad_count); - frame_support::ensure!( approvals_map.keys().count() == T::TargetList::iter().count(), "calculated approvals count is different from total of target list.", ); + if !mismatch_approvals.is_zero() { + log!(error, "{} targets with unexpected score in list", mismatch_approvals); + + return Err("final calculated approvals != target list scores".into()); + } + Ok(()) }