From 9e885ddc8255d14e074f53933d65846dc07fbd99 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Wed, 15 Jan 2020 12:38:27 +0100 Subject: [PATCH 01/11] Initial work --- frame/session/src/historical.rs | 110 +++++++++++++++----------------- frame/session/src/lib.rs | 47 +++++++------- 2 files changed, 73 insertions(+), 84 deletions(-) diff --git a/frame/session/src/historical.rs b/frame/session/src/historical.rs index 939e6133e8520..575cc37557a01 100644 --- a/frame/session/src/historical.rs +++ b/frame/session/src/historical.rs @@ -48,8 +48,7 @@ pub trait Trait: super::Trait { /// validator, since they may be outdated by the time this is queried from a /// historical trie. /// - /// This mapping is expected to remain stable in between calls to - /// `Self::OnSessionEnding::on_session_ending` which return new validators. + /// It must return the identification for the current session index. type FullIdentificationOf: Convert>; } @@ -57,9 +56,11 @@ decl_storage! { trait Store for Module as Session { /// Mapping from historical session indices to session-data root hash and validator count. HistoricalSessions get(fn historical_root): map SessionIndex => Option<(T::Hash, ValidatorCount)>; - /// Queued full identifications for queued sessions whose validators have become obsolete. - CachedObsolete get(fn cached_obsolete): map SessionIndex - => Option>; + // /// Queued full identifications for queued sessions whose validators have become obsolete. + // CachedObsolete get(fn cached_obsolete): map SessionIndex + // => Option>; + // TODO TODO: clean this, no longer need to store some fullidentification before the + // session get started /// The range of historical sessions we store. [first, last) StoredRange: Option<(SessionIndex, SessionIndex)>; } @@ -97,52 +98,52 @@ impl Module { } } -/// Specialization of the crate-level `OnSessionEnding` which returns the old -/// set of full identification when changing the validator set. -pub trait OnSessionEnding: crate::OnSessionEnding { - /// If there was a validator set change, its returns the set of new validators along with the - /// old validators and their full identifications. - fn on_session_ending(ending: SessionIndex, will_apply_at: SessionIndex) - -> Option<(Vec, Vec<(ValidatorId, FullIdentification)>)>; +/// Specialization of the crate-level `SessionManager` which returns the set of full identification +/// when creating a new session. +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 end_session(end_index: SessionIndex); } -/// An `OnSessionEnding` implementation that wraps an inner `I` and also +/// An `SessionManager` implementation that wraps an inner `I` and also /// sets the historical trie root of the ending session. pub struct NoteHistoricalRoot(sp_std::marker::PhantomData<(T, I)>); -impl crate::OnSessionEnding for NoteHistoricalRoot - where I: OnSessionEnding +impl crate::SessionManager for NoteHistoricalRoot + where I: SessionManager { - fn on_session_ending(ending: SessionIndex, applied_at: SessionIndex) -> Option> { + fn new_session(new_index: SessionIndex) -> Option> { StoredRange::mutate(|range| { - range.get_or_insert_with(|| (ending, ending)).1 = ending + 1; + range.get_or_insert_with(|| (new_index, new_index)).1 = new_index; }); - // do all of this _before_ calling the other `on_session_ending` impl - // so that we have e.g. correct exposures from the _current_. + 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 count = >::validators().len() as u32; - match ProvingTrie::::generate_for(ending) { - Ok(trie) => >::insert(ending, &(trie.root, count)), - Err(reason) => { - print("Failed to generate historical ancestry-inclusion proof."); - print(reason); + if let Some(new_validators) = new_validators_and_id { + let count = new_validators.len() as u32; + match ProvingTrie::::generate_for(new_validators) { + Ok(trie) => >::insert(new_index, &(trie.root, count)), + Err(reason) => { + print("Failed to generate historical ancestry-inclusion proof."); + print(reason); + } + }; + } else { + let previous_index = new_index.saturating_sub(1); + if let Some(previous_session) = >::get(previous_index) { + >::insert(new_index, previous_session); } - }; - - // trie has been generated for this session, so it's no longer queued. - >::remove(&ending); - - let (new_validators, old_exposures) = >::on_session_ending(ending, applied_at)?; - - // every session from `ending+1 .. applied_at` now has obsolete `FullIdentification` - // now that a new validator election has occurred. - // we cache these in the trie until those sessions themselves end. - for obsolete in (ending + 1) .. applied_at { - >::insert(obsolete, &old_exposures); } - Some(new_validators) + new_validators + } + fn end_session(end_index: SessionIndex) { + >::end_session(end_index) } } @@ -158,15 +159,14 @@ pub struct ProvingTrie { } impl ProvingTrie { - fn generate_for(now: SessionIndex) -> Result { + fn generate_for(validators: I) -> Result + where I: IntoIterator + { let mut db = MemoryDB::default(); let mut root = Default::default(); - fn build(root: &mut T::Hash, db: &mut MemoryDB>, validators: I) - -> Result<(), &'static str> - where I: IntoIterator)> { - let mut trie = TrieDBMut::new(db, root); + let mut trie = TrieDBMut::new(&mut db, &mut root); for (i, (validator, full_id)) in validators.into_iter().enumerate() { let i = i as u32; let keys = match >::load_keys(&validator) { @@ -174,11 +174,7 @@ impl ProvingTrie { Some(k) => k, }; - let full_id = full_id.or_else(|| T::FullIdentificationOf::convert(validator.clone())); - let full_id = match full_id { - None => return Err("no full identification for a current validator"), - Some(full) => (validator, full), - }; + let full_id = (validator, full_id); // map each key to the owner index. for key_id in T::Keys::key_ids() { @@ -194,17 +190,6 @@ impl ProvingTrie { let _ = i.using_encoded(|k| full_id.using_encoded(|v| trie.insert(k, v))) .map_err(|_| "failed to insert into trie")?; } - - Ok(()) - } - - // if the current session's full identifications are obsolete but cached, - // use those. - if let Some(obsolete) = >::get(&now) { - build::(&mut root, &mut db, obsolete.into_iter().map(|(v, f)| (v, Some(f))))? - } else { - let validators = >::validators(); - build::(&mut root, &mut db, validators.into_iter().map(|v| (v, None)))? } Ok(ProvingTrie { @@ -281,7 +266,12 @@ impl> frame_support::traits::KeyOwnerProofSystem<(KeyTy fn prove(key: (KeyTypeId, D)) -> Option { let session = >::current_index(); - let trie = ProvingTrie::::generate_for(session).ok()?; + let validators = >::validators().into_iter() + .filter_map(|validator| { + T::FullIdentificationOf::convert(validator.clone()) + .map(|full_id| (validator, full_id)) + }); + let trie = ProvingTrie::::generate_for(validators).ok()?; let (id, data) = key; diff --git a/frame/session/src/lib.rs b/frame/session/src/lib.rs index 2c5668f6fa9f2..17a9bcdeda760 100644 --- a/frame/session/src/lib.rs +++ b/frame/session/src/lib.rs @@ -162,29 +162,28 @@ impl< } } -/// An event handler for when the session is ending. -/// TODO [slashing] consider renaming to OnSessionStarting -pub trait OnSessionEnding { - /// Handle the fact that the session is ending, and optionally provide the new validator set. +/// A trait for managing creation of new validator set. +pub trait SessionManager { + /// Plan a new session, and optionally provide the new validator set. /// /// Even if the validator-set is the same as before, if any underlying economic /// conditions have changed (i.e. stake-weights), the new validator set must be returned. /// This is necessary for consensus engines making use of the session module to /// issue a validator-set change so misbehavior can be provably associated with the new /// economic conditions as opposed to the old. + /// The returned validator set, if any, will not be applied until `new_index`. + /// `new_index` is strictly greater than from previous call. + fn new_session(new_index: SessionIndex) -> Option>; + /// End the session. /// - /// `ending_index` is the index of the currently ending session. - /// The returned validator set, if any, will not be applied until `will_apply_at`. - /// `will_apply_at` is guaranteed to be at least `ending_index + 1`, since session indices don't - /// repeat, but it could be some time after in case we are staging authority set changes. - fn on_session_ending( - ending_index: SessionIndex, - will_apply_at: SessionIndex - ) -> Option>; + /// 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); } -impl OnSessionEnding for () { - fn on_session_ending(_: SessionIndex, _: SessionIndex) -> Option> { None } +impl SessionManager for () { + fn new_session(_: SessionIndex) -> Option> { None } + fn end_session(_: SessionIndex) {} } /// Handler for session lifecycle events. @@ -214,7 +213,7 @@ pub trait SessionHandler { /// A notification for end of the session. /// - /// Note it is triggered before any `OnSessionEnding` handlers, + /// Note it is triggered before any `SessionManager::end_session` handlers, /// so we can still affect the validator set. fn on_before_session_ending() {} @@ -248,7 +247,7 @@ pub trait OneSessionHandler: BoundToRuntimeAppPublic { /// A notification for end of the session. /// - /// Note it is triggered before any `OnSessionEnding` handlers, + /// Note it is triggered before any `SessionManager::end_session` handlers, /// so we can still affect the validator set. fn on_before_session_ending() {} @@ -352,8 +351,8 @@ pub trait Trait: frame_system::Trait { /// Indicator for when to end the session. type ShouldEndSession: ShouldEndSession; - /// Handler for when a session is about to end. - type OnSessionEnding: OnSessionEnding; + /// Handler for managing new session. + type SessionManager: SessionManager; /// Handler when a session has changed. type SessionHandler: SessionHandler; @@ -543,10 +542,14 @@ impl Module { DisabledValidators::take(); } - let applied_at = session_index + 2; + T::SessionManager::end_session(session_index); + + // Increment session index. + let session_index = session_index + 1; + CurrentIndex::put(session_index); // Get next validator set. - let maybe_next_validators = T::OnSessionEnding::on_session_ending(session_index, applied_at); + let maybe_next_validators = T::SessionManager::new_session(session_index + 2); let (next_validators, next_identities_changed) = if let Some(validators) = maybe_next_validators { @@ -558,10 +561,6 @@ impl Module { (>::get(), false) }; - // Increment session index. - let session_index = session_index + 1; - CurrentIndex::put(session_index); - // Queue next session keys. let (queued_amalgamated, next_changed) = { // until we are certain there has been a change, iterate the prior From 3b7db305011035ced1ba8fdacc04b12647b5f1d5 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Wed, 15 Jan 2020 16:28:40 +0100 Subject: [PATCH 02/11] Fix most things --- bin/node/runtime/src/lib.rs | 2 +- frame/authority-discovery/src/lib.rs | 12 +++---- frame/babe/src/mock.rs | 3 +- frame/im-online/src/mock.rs | 22 ++++++------- frame/session/src/historical.rs | 21 ++++++------ frame/session/src/lib.rs | 37 ++++++++------------- frame/session/src/mock.rs | 25 +++++++-------- frame/staking/src/lib.rs | 48 +++++++++++++--------------- frame/staking/src/mock.rs | 3 +- 9 files changed, 74 insertions(+), 99 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index e5e453fcaff04..47b29f08849d6 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -236,7 +236,7 @@ parameter_types! { } impl pallet_session::Trait for Runtime { - type OnSessionEnding = Staking; + type SessionManager = Staking; type SessionHandler = ::KeyTypeIdProviders; type ShouldEndSession = Babe; type Event = Event; diff --git a/frame/authority-discovery/src/lib.rs b/frame/authority-discovery/src/lib.rs index b3911859f4787..e40f174d8bb6a 100644 --- a/frame/authority-discovery/src/lib.rs +++ b/frame/authority-discovery/src/lib.rs @@ -109,11 +109,10 @@ mod tests { pub struct Test; impl Trait for Test {} - pub struct TestOnSessionEnding; - impl pallet_session::OnSessionEnding for TestOnSessionEnding { - fn on_session_ending(_: SessionIndex, _: SessionIndex) -> Option> { - None - } + pub struct TestSessionManager; + impl pallet_session::SessionManager for TestSessionManager { + fn new_session(_: SessionIndex) -> Option> { None } + fn end_session(_: SessionIndex) {} } parameter_types! { @@ -121,14 +120,13 @@ mod tests { } impl pallet_session::Trait for Test { - type OnSessionEnding = TestOnSessionEnding; + type SessionManager = TestSessionManager; type Keys = UintAuthorityId; type ShouldEndSession = pallet_session::PeriodicSessions; type SessionHandler = TestSessionHandler; type Event = (); type ValidatorId = AuthorityId; type ValidatorIdOf = ConvertInto; - type SelectInitialValidators = (); type DisabledValidatorsThreshold = DisabledValidatorsThreshold; } diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index 3f0c42a6cb9c6..e65f305dc4de2 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -79,9 +79,8 @@ impl pallet_session::Trait for Test { type ValidatorId = ::AccountId; type ShouldEndSession = Babe; type SessionHandler = (Babe,Babe,); - type OnSessionEnding = (); + type SessionManager = (); type ValidatorIdOf = (); - type SelectInitialValidators = (); type Keys = MockSessionKeys; type DisabledValidatorsThreshold = DisabledValidatorsThreshold; } diff --git a/frame/im-online/src/mock.rs b/frame/im-online/src/mock.rs index 387ff47c09fa5..5c428c38582ff 100644 --- a/frame/im-online/src/mock.rs +++ b/frame/im-online/src/mock.rs @@ -43,28 +43,25 @@ thread_local! { pub static VALIDATORS: RefCell>> = RefCell::new(Some(vec![1, 2, 3])); } -pub struct TestOnSessionEnding; -impl pallet_session::OnSessionEnding for TestOnSessionEnding { - fn on_session_ending(_ending_index: SessionIndex, _will_apply_at: SessionIndex) - -> Option> - { +pub struct TestSessionManager; +impl pallet_session::SessionManager for TestSessionManager { + fn new_session(_new_index: SessionIndex) -> Option> { VALIDATORS.with(|l| l.borrow_mut().take()) } + fn end_session(_: SessionIndex) {} } -impl pallet_session::historical::OnSessionEnding for TestOnSessionEnding { - fn on_session_ending(_ending_index: SessionIndex, _will_apply_at: SessionIndex) - -> Option<(Vec, Vec<(u64, u64)>)> - { +impl pallet_session::historical::SessionManager for TestSessionManager { + fn new_session(_new_index: SessionIndex) -> Option> { VALIDATORS.with(|l| l .borrow_mut() .take() .map(|validators| { - let full_identification = validators.iter().map(|v| (*v, *v)).collect(); - (validators, full_identification) + validators.iter().map(|v| (*v, *v)).collect() }) ) } + fn end_session(_: SessionIndex) {} } /// An extrinsic type used for tests. @@ -131,13 +128,12 @@ parameter_types! { impl pallet_session::Trait for Runtime { type ShouldEndSession = pallet_session::PeriodicSessions; - type OnSessionEnding = pallet_session::historical::NoteHistoricalRoot; + type SessionManager = pallet_session::historical::NoteHistoricalRoot; type SessionHandler = (ImOnline, ); type ValidatorId = u64; type ValidatorIdOf = ConvertInto; type Keys = UintAuthorityId; type Event = (); - type SelectInitialValidators = (); type DisabledValidatorsThreshold = DisabledValidatorsThreshold; } diff --git a/frame/session/src/historical.rs b/frame/session/src/historical.rs index 575cc37557a01..33d99bb2aeea9 100644 --- a/frame/session/src/historical.rs +++ b/frame/session/src/historical.rs @@ -14,6 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . +// TODO TODO: make at genesis the current session using fullidentificationof //! An opt-in utility for tracking historical sessions in SRML-session. //! //! This is generally useful when implementing blockchains that require accountable @@ -116,7 +117,7 @@ impl crate::SessionManager for NoteHistoricalRoot Option> { StoredRange::mutate(|range| { - range.get_or_insert_with(|| (new_index, new_index)).1 = new_index; + range.get_or_insert_with(|| (new_index, new_index)).1 = new_index + 1; }); let new_validators_and_id = >::new_session(new_index); @@ -125,6 +126,7 @@ impl crate::SessionManager for NoteHistoricalRoot::generate_for(new_validators) { Ok(trie) => >::insert(new_index, &(trie.root, count)), @@ -338,13 +340,10 @@ mod tests { set_next_validators(vec![1, 2, 4]); force_new_session(); - assert!(Historical::cached_obsolete(&(proof.session + 1)).is_none()); - System::set_block_number(2); Session::on_initialize(2); - assert!(Historical::cached_obsolete(&(proof.session + 1)).is_some()); - + println!("proof session: {}", proof.session); assert!(Historical::historical_root(proof.session).is_some()); assert!(Session::current_index() > proof.session); @@ -356,15 +355,13 @@ mod tests { force_new_session(); System::set_block_number(3); Session::on_initialize(3); - - assert!(Historical::cached_obsolete(&(proof.session + 1)).is_none()); }); } #[test] fn prune_up_to_works() { new_test_ext().execute_with(|| { - for i in 1..101u64 { + for i in 1..99u64 { set_next_validators(vec![i]); force_new_session(); @@ -375,7 +372,7 @@ mod tests { assert_eq!(StoredRange::get(), Some((0, 100))); - for i in 1..100 { + for i in 0..100 { assert!(Historical::historical_root(i).is_some()) } @@ -395,7 +392,7 @@ mod tests { Historical::prune_up_to(100); assert_eq!(StoredRange::get(), None); - for i in 101..201u64 { + for i in 99..199u64 { set_next_validators(vec![i]); force_new_session(); @@ -406,14 +403,14 @@ mod tests { assert_eq!(StoredRange::get(), Some((100, 200))); - for i in 101..200 { + for i in 100..200 { assert!(Historical::historical_root(i).is_some()) } Historical::prune_up_to(9999); assert_eq!(StoredRange::get(), None); - for i in 101..200 { + for i in 100..200 { assert!(Historical::historical_root(i).is_none()) } }); diff --git a/frame/session/src/lib.rs b/frame/session/src/lib.rs index 7581ee0b80b57..a56b2c00dff27 100644 --- a/frame/session/src/lib.rs +++ b/frame/session/src/lib.rs @@ -317,21 +317,6 @@ impl SessionHandler for TestSessionHandler { fn on_disabled(_: usize) {} } -/// Handler for selecting the genesis validator set. -pub trait SelectInitialValidators { - /// Returns the initial validator set. If `None` is returned - /// all accounts that have session keys set in the genesis block - /// will be validators. - fn select_initial_validators() -> Option>; -} - -/// Implementation of `SelectInitialValidators` that does nothing. -impl SelectInitialValidators for () { - fn select_initial_validators() -> Option> { - None - } -} - impl ValidatorRegistration for Module { fn is_registered(id: &T::ValidatorId) -> bool { Self::load_keys(id).is_some() @@ -365,9 +350,6 @@ pub trait Trait: frame_system::Trait { /// After the threshold is reached `disabled` method starts to return true, /// which in combination with `pallet_staking` forces a new era. type DisabledValidatorsThreshold: Get; - - /// Select initial validators. - type SelectInitialValidators: SelectInitialValidators; } const DEDUP_KEY_PREFIX: &[u8] = b":session:keys"; @@ -434,12 +416,19 @@ decl_storage! { .expect("genesis config must not contain duplicates; qed"); } - let initial_validators = T::SelectInitialValidators::select_initial_validators() - .unwrap_or_else(|| config.keys.iter().map(|(ref v, _)| v.clone()).collect()); + let initial_validators_0 = T::SessionManager::new_session(0) + // TODO TODO: should we always expect one can we fall back with config keys ? + // TODO TODO: it might be relevant for test, but if so we should just change + // implementation of SessionManager for type `()` + .expect("TODO TODO: cannot have empty validator set for session 0"); + // .unwrap_or_else(|| config.keys.iter().map(|(ref v, _)| v.clone()).collect()); + assert!(!initial_validators_0.is_empty(), "Empty validator set in genesis block!"); - assert!(!initial_validators.is_empty(), "Empty validator set in genesis block!"); + let initial_validators_1 = T::SessionManager::new_session(1) + .unwrap_or_else(|| initial_validators_0.clone()); + assert!(!initial_validators_1.is_empty(), "Empty validator set in genesis block!"); - let queued_keys: Vec<_> = initial_validators + let queued_keys: Vec<_> = initial_validators_1 .iter() .cloned() .map(|v| ( @@ -451,7 +440,7 @@ decl_storage! { // Tell everyone about the genesis session keys T::SessionHandler::on_genesis_session::(&queued_keys); - >::put(initial_validators); + >::put(initial_validators_0); >::put(queued_keys); }); } @@ -551,7 +540,7 @@ impl Module { CurrentIndex::put(session_index); // Get next validator set. - let maybe_next_validators = T::SessionManager::new_session(session_index + 2); + let maybe_next_validators = T::SessionManager::new_session(session_index + 1); let (next_validators, next_identities_changed) = if let Some(validators) = maybe_next_validators { diff --git a/frame/session/src/mock.rs b/frame/session/src/mock.rs index 7fe9cd01f43bf..ff84743a61596 100644 --- a/frame/session/src/mock.rs +++ b/frame/session/src/mock.rs @@ -88,9 +88,10 @@ impl SessionHandler for TestSessionHandler { } } -pub struct TestOnSessionEnding; -impl OnSessionEnding for TestOnSessionEnding { - fn on_session_ending(_: SessionIndex, _: SessionIndex) -> Option> { +pub struct TestSessionManager; +impl SessionManager for TestSessionManager { + fn end_session(_: SessionIndex) {} + fn new_session(_: SessionIndex) -> Option> { if !TEST_SESSION_CHANGED.with(|l| *l.borrow()) { VALIDATORS.with(|v| { let mut v = v.borrow_mut(); @@ -108,14 +109,13 @@ impl OnSessionEnding for TestOnSessionEnding { } #[cfg(feature = "historical")] -impl crate::historical::OnSessionEnding for TestOnSessionEnding { - fn on_session_ending(ending_index: SessionIndex, will_apply_at: SessionIndex) - -> Option<(Vec, Vec<(u64, u64)>)> +impl crate::historical::SessionManager for TestSessionManager { + fn end_session(_: SessionIndex) {} + fn new_session(new_index: SessionIndex) + -> Option> { - let pair_with_ids = |vals: &[u64]| vals.iter().map(|&v| (v, v)).collect::>(); - >::on_session_ending(ending_index, will_apply_at) - .map(|vals| (pair_with_ids(&vals), vals)) - .map(|(ids, vals)| (vals, ids)) + >::new_session(new_index) + .map(|vals| vals.into_iter().map(|val| (val, val)).collect()) } } @@ -190,15 +190,14 @@ parameter_types! { impl Trait for Test { type ShouldEndSession = TestShouldEndSession; #[cfg(feature = "historical")] - type OnSessionEnding = crate::historical::NoteHistoricalRoot; + type SessionManager = crate::historical::NoteHistoricalRoot; #[cfg(not(feature = "historical"))] - type OnSessionEnding = TestOnSessionEnding; + type SessionManager = TestSessionManager; type SessionHandler = TestSessionHandler; type ValidatorId = u64; type ValidatorIdOf = ConvertInto; type Keys = MockSessionKeys; type Event = (); - type SelectInitialValidators = (); type DisabledValidatorsThreshold = DisabledValidatorsThreshold; } diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 326a01599034c..e0570499aa9ef 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -265,7 +265,7 @@ use frame_support::{ WithdrawReasons, OnUnbalanced, Imbalance, Get, Time } }; -use pallet_session::{historical::OnSessionEnding, SelectInitialValidators}; +use pallet_session::historical::SessionManager; use sp_runtime::{ Perbill, RuntimeDebug, @@ -575,8 +575,7 @@ impl SessionInterface<::AccountId> for T whe FullIdentificationOf = ExposureOf, >, T::SessionHandler: pallet_session::SessionHandler<::AccountId>, - T::OnSessionEnding: pallet_session::OnSessionEnding<::AccountId>, - T::SelectInitialValidators: pallet_session::SelectInitialValidators<::AccountId>, + T::SessionManager: pallet_session::SessionManager<::AccountId>, T::ValidatorIdOf: Convert<::AccountId, Option<::AccountId>> { fn disable_validator(validator: &::AccountId) -> Result { @@ -1349,7 +1348,7 @@ impl Module { /// Session has just ended. Provide the validator set for the next session if it's an era-end, along /// with the exposure of the prior validator set. fn new_session(session_index: SessionIndex) - -> Option<(Vec, Vec<(T::AccountId, Exposure>)>)> + -> Option>)>> { let era_length = session_index.checked_sub(Self::current_era_start_session_index()).unwrap_or(0); match ForceEra::get() { @@ -1358,12 +1357,13 @@ impl Module { Forcing::NotForcing if era_length >= T::SessionsPerEra::get() => (), _ => return None, } - let validators = T::SessionInterface::validators(); - let prior = validators.into_iter() - .map(|v| { let e = Self::stakers(&v); (v, e) }) - .collect(); - Self::new_era(session_index).map(move |new| (new, prior)) + Self::new_era(session_index).map(|validators| { + validators.into_iter().map(|v| { + let exposure = >::get(&v); + (v, exposure) + }).collect() + }) } /// The era has changed - enact new staking set. @@ -1646,19 +1646,24 @@ impl Module { } } -impl pallet_session::OnSessionEnding for Module { - fn on_session_ending(_ending: SessionIndex, start_session: SessionIndex) -> Option> { - Self::ensure_storage_upgraded(); - Self::new_session(start_session - 1).map(|(new, _old)| new) +impl pallet_session::SessionManager for Module { + fn new_session(new_index: SessionIndex) -> Option> { + >::new_session(new_index) + .map(|v| v.into_iter().map(|(v, _expo)| v).collect()) + } + fn end_session(end_index: SessionIndex) { + >::end_session(end_index) } } -impl OnSessionEnding>> for Module { - fn on_session_ending(_ending: SessionIndex, start_session: SessionIndex) - -> Option<(Vec, Vec<(T::AccountId, Exposure>)>)> +impl SessionManager>> for Module { + fn new_session(new_index: SessionIndex) + -> Option>)>> { Self::ensure_storage_upgraded(); - Self::new_session(start_session - 1) + Self::new_session(new_index - 1) + } + fn end_session(_end_index: SessionIndex) { } } @@ -1707,12 +1712,6 @@ impl Convert> } } -impl SelectInitialValidators for Module { - fn select_initial_validators() -> Option> { - >::select_validators().1 - } -} - /// This is intended to be used with `FilterHistoricalOffences`. impl OnOffenceHandler> for Module where T: pallet_session::Trait::AccountId>, @@ -1721,8 +1720,7 @@ impl OnOffenceHandler, >, T::SessionHandler: pallet_session::SessionHandler<::AccountId>, - T::OnSessionEnding: pallet_session::OnSessionEnding<::AccountId>, - T::SelectInitialValidators: pallet_session::SelectInitialValidators<::AccountId>, + T::SessionManager: pallet_session::SessionManager<::AccountId>, T::ValidatorIdOf: Convert<::AccountId, Option<::AccountId>> { fn on_offence( diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 3c238b56ed317..8aa20c19c6e29 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -162,14 +162,13 @@ parameter_types! { pub const DisabledValidatorsThreshold: Perbill = Perbill::from_percent(25); } impl pallet_session::Trait for Test { - type OnSessionEnding = pallet_session::historical::NoteHistoricalRoot; + type SessionManager = pallet_session::historical::NoteHistoricalRoot; type Keys = UintAuthorityId; type ShouldEndSession = pallet_session::PeriodicSessions; type SessionHandler = TestSessionHandler; type Event = (); type ValidatorId = AccountId; type ValidatorIdOf = crate::StashOf; - type SelectInitialValidators = Staking; type DisabledValidatorsThreshold = DisabledValidatorsThreshold; } From 6853dd80b2e5b2c6e2269370c06bc1b42c5168d7 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Wed, 15 Jan 2020 17:01:43 +0100 Subject: [PATCH 03/11] fix test --- frame/session/src/historical.rs | 1 - frame/staking/src/lib.rs | 26 +++++++++++++------------- frame/staking/src/tests.rs | 12 ++++++------ 3 files changed, 19 insertions(+), 20 deletions(-) diff --git a/frame/session/src/historical.rs b/frame/session/src/historical.rs index 33d99bb2aeea9..692f8773abd8f 100644 --- a/frame/session/src/historical.rs +++ b/frame/session/src/historical.rs @@ -14,7 +14,6 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -// TODO TODO: make at genesis the current session using fullidentificationof //! An opt-in utility for tracking historical sessions in SRML-session. //! //! This is generally useful when implementing blockchains that require accountable diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index e0570499aa9ef..0f2ee4430a6f2 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -1347,9 +1347,7 @@ impl Module { /// Session has just ended. Provide the validator set for the next session if it's an era-end, along /// with the exposure of the prior validator set. - fn new_session(session_index: SessionIndex) - -> Option>)>> - { + fn new_session(session_index: SessionIndex) -> Option> { let era_length = session_index.checked_sub(Self::current_era_start_session_index()).unwrap_or(0); match ForceEra::get() { Forcing::ForceNew => ForceEra::kill(), @@ -1358,12 +1356,7 @@ impl Module { _ => return None, } - Self::new_era(session_index).map(|validators| { - validators.into_iter().map(|v| { - let exposure = >::get(&v); - (v, exposure) - }).collect() - }) + Self::new_era(session_index) } /// The era has changed - enact new staking set. @@ -1648,8 +1641,11 @@ impl Module { impl pallet_session::SessionManager for Module { fn new_session(new_index: SessionIndex) -> Option> { - >::new_session(new_index) - .map(|v| v.into_iter().map(|(v, _expo)| v).collect()) + Self::ensure_storage_upgraded(); + if new_index < 2 { + return >::select_validators().1 + } + Self::new_session(new_index - 1) } fn end_session(end_index: SessionIndex) { >::end_session(end_index) @@ -1660,8 +1656,12 @@ impl SessionManager> fn new_session(new_index: SessionIndex) -> Option>)>> { - Self::ensure_storage_upgraded(); - Self::new_session(new_index - 1) + >::new_session(new_index).map(|validators| { + validators.into_iter().map(|v| { + let exposure = >::get(&v); + (v, exposure) + }).collect() + }) } fn end_session(_end_index: SessionIndex) { } diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 555edac979d87..1d25654e2bcd2 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -377,7 +377,7 @@ fn less_than_needed_candidates_works() { #[test] fn no_candidate_emergency_condition() { ExtBuilder::default() - .minimum_validator_count(10) + .minimum_validator_count(1) .validator_count(15) .num_validators(4) .validator_pool(true) @@ -386,21 +386,21 @@ fn no_candidate_emergency_condition() { .execute_with(|| { // initial validators assert_eq_uvec!(validator_controllers(), vec![10, 20, 30, 40]); + let prefs = ValidatorPrefs { commission: Perbill::one() }; + ::Validators::insert(11, prefs.clone()); // set the minimum validator count. ::MinimumValidatorCount::put(10); - ::ValidatorCount::put(15); - assert_eq!(Staking::validator_count(), 15); let _ = Staking::chill(Origin::signed(10)); // trigger era - System::set_block_number(1); - Session::on_initialize(System::block_number()); + start_era(1); // Previous ones are elected. chill is invalidates. TODO: #2494 assert_eq_uvec!(validator_controllers(), vec![10, 20, 30, 40]); - assert_eq!(Staking::current_elected().len(), 0); + // Though the validator preferences has been removed. + assert!(Staking::validators(11) != prefs); }); } From fe1ef849f1bd137cf1e9f5e1c1cba42fd0fe991a Mon Sep 17 00:00:00 2001 From: thiolliere Date: Wed, 15 Jan 2020 17:14:12 +0100 Subject: [PATCH 04/11] fix old comment --- bin/node/runtime/src/lib.rs | 1 - frame/session/src/historical.rs | 1 - frame/session/src/lib.rs | 10 +++------- 3 files changed, 3 insertions(+), 9 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 47b29f08849d6..e516ee56b0ccb 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -243,7 +243,6 @@ impl pallet_session::Trait for Runtime { type Keys = SessionKeys; type ValidatorId = ::AccountId; type ValidatorIdOf = pallet_staking::StashOf; - type SelectInitialValidators = Staking; type DisabledValidatorsThreshold = DisabledValidatorsThreshold; } diff --git a/frame/session/src/historical.rs b/frame/session/src/historical.rs index 692f8773abd8f..50a81867a8631 100644 --- a/frame/session/src/historical.rs +++ b/frame/session/src/historical.rs @@ -125,7 +125,6 @@ impl crate::SessionManager for NoteHistoricalRoot::generate_for(new_validators) { Ok(trie) => >::insert(new_index, &(trie.root, count)), diff --git a/frame/session/src/lib.rs b/frame/session/src/lib.rs index a56b2c00dff27..683d0a4f48584 100644 --- a/frame/session/src/lib.rs +++ b/frame/session/src/lib.rs @@ -417,16 +417,12 @@ decl_storage! { } let initial_validators_0 = T::SessionManager::new_session(0) - // TODO TODO: should we always expect one can we fall back with config keys ? - // TODO TODO: it might be relevant for test, but if so we should just change - // implementation of SessionManager for type `()` - .expect("TODO TODO: cannot have empty validator set for session 0"); - // .unwrap_or_else(|| config.keys.iter().map(|(ref v, _)| v.clone()).collect()); - assert!(!initial_validators_0.is_empty(), "Empty validator set in genesis block!"); + .expect("No initial validator set by `SessionManager`"); + assert!(!initial_validators_0.is_empty(), "Empty validator set for session 0 in genesis block!"); let initial_validators_1 = T::SessionManager::new_session(1) .unwrap_or_else(|| initial_validators_0.clone()); - assert!(!initial_validators_1.is_empty(), "Empty validator set in genesis block!"); + assert!(!initial_validators_1.is_empty(), "Empty validator set for session 1 in genesis block!"); let queued_keys: Vec<_> = initial_validators_1 .iter() From 214c7fd50efc207b8cad3b44b527311064f04704 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Wed, 15 Jan 2020 17:17:41 +0100 Subject: [PATCH 05/11] migration --- frame/session/src/historical.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/frame/session/src/historical.rs b/frame/session/src/historical.rs index 50a81867a8631..8f2976cc74f13 100644 --- a/frame/session/src/historical.rs +++ b/frame/session/src/historical.rs @@ -56,18 +56,19 @@ decl_storage! { trait Store for Module as Session { /// Mapping from historical session indices to session-data root hash and validator count. HistoricalSessions get(fn historical_root): map SessionIndex => Option<(T::Hash, ValidatorCount)>; - // /// Queued full identifications for queued sessions whose validators have become obsolete. - // CachedObsolete get(fn cached_obsolete): map SessionIndex - // => Option>; - // TODO TODO: clean this, no longer need to store some fullidentification before the - // session get started /// The range of historical sessions we store. [first, last) StoredRange: Option<(SessionIndex, SessionIndex)>; + /// Deprecated. + CachedObsolete: map SessionIndex => Option>; } } decl_module! { - pub struct Module for enum Call where origin: T::Origin { } + pub struct Module for enum Call where origin: T::Origin { + fn on_initialize(_n: T::BlockNumber) { + CachedObsolete::remove_all(); + } + } } impl Module { From f1fe86d94bbc6bbff1ccf5f2277016bb5cf9bd7c Mon Sep 17 00:00:00 2001 From: thiolliere Date: Wed, 15 Jan 2020 17:32:29 +0100 Subject: [PATCH 06/11] fix --- frame/session/src/historical.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/session/src/historical.rs b/frame/session/src/historical.rs index 8f2976cc74f13..e7b172d57d078 100644 --- a/frame/session/src/historical.rs +++ b/frame/session/src/historical.rs @@ -66,7 +66,7 @@ decl_storage! { decl_module! { pub struct Module for enum Call where origin: T::Origin { fn on_initialize(_n: T::BlockNumber) { - CachedObsolete::remove_all(); + CachedObsolete::::remove_all(); } } } From 83588c4334a7e9f5eee488dc160db3232e59d9a0 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Wed, 15 Jan 2020 17:33:41 +0100 Subject: [PATCH 07/11] remove useless stuff --- frame/session/src/historical.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/frame/session/src/historical.rs b/frame/session/src/historical.rs index e7b172d57d078..e67a2ae1b0eab 100644 --- a/frame/session/src/historical.rs +++ b/frame/session/src/historical.rs @@ -342,7 +342,6 @@ mod tests { System::set_block_number(2); Session::on_initialize(2); - println!("proof session: {}", proof.session); assert!(Historical::historical_root(proof.session).is_some()); assert!(Session::current_index() > proof.session); From b6b5be25a13b42116bb8d590f47b90253815e525 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Wed, 15 Jan 2020 23:40:05 +0100 Subject: [PATCH 08/11] fix --- frame/authority-discovery/src/lib.rs | 8 +------- frame/session/src/lib.rs | 6 +++++- frame/staking/src/lib.rs | 5 ++--- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/frame/authority-discovery/src/lib.rs b/frame/authority-discovery/src/lib.rs index e40f174d8bb6a..c427043397075 100644 --- a/frame/authority-discovery/src/lib.rs +++ b/frame/authority-discovery/src/lib.rs @@ -109,18 +109,12 @@ mod tests { pub struct Test; impl Trait for Test {} - pub struct TestSessionManager; - impl pallet_session::SessionManager for TestSessionManager { - fn new_session(_: SessionIndex) -> Option> { None } - fn end_session(_: SessionIndex) {} - } - parameter_types! { pub const DisabledValidatorsThreshold: Perbill = Perbill::from_percent(33); } impl pallet_session::Trait for Test { - type SessionManager = TestSessionManager; + type SessionManager = (); type Keys = UintAuthorityId; type ShouldEndSession = pallet_session::PeriodicSessions; type SessionHandler = TestSessionHandler; diff --git a/frame/session/src/lib.rs b/frame/session/src/lib.rs index 683d0a4f48584..d2fb3b7130470 100644 --- a/frame/session/src/lib.rs +++ b/frame/session/src/lib.rs @@ -417,7 +417,11 @@ decl_storage! { } let initial_validators_0 = T::SessionManager::new_session(0) - .expect("No initial validator set by `SessionManager`"); + .unwrap_or_else(|| { + frame_support::print("No initial validator provided by `SessionManager`, use \ + session config keys to generate initial validator set."); + config.keys.iter().map(|(ref v, _)| v.clone()).collect() + }); assert!(!initial_validators_0.is_empty(), "Empty validator set for session 0 in genesis block!"); let initial_validators_1 = T::SessionManager::new_session(1) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 0f2ee4430a6f2..41d01008b3a46 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -1345,8 +1345,7 @@ impl Module { imbalance } - /// Session has just ended. Provide the validator set for the next session if it's an era-end, along - /// with the exposure of the prior validator set. + /// Session has just ended. Provide the validator set for the next session if it's an era-end. fn new_session(session_index: SessionIndex) -> Option> { let era_length = session_index.checked_sub(Self::current_era_start_session_index()).unwrap_or(0); match ForceEra::get() { @@ -1642,7 +1641,7 @@ impl Module { impl pallet_session::SessionManager for Module { fn new_session(new_index: SessionIndex) -> Option> { Self::ensure_storage_upgraded(); - if new_index < 2 { + if new_index == 0 { return >::select_validators().1 } Self::new_session(new_index - 1) From 8e486142ad241ef92142e701e2e02697f122f57c Mon Sep 17 00:00:00 2001 From: thiolliere Date: Thu, 16 Jan 2020 13:28:21 +0100 Subject: [PATCH 09/11] less spaghetti implementation --- frame/staking/src/lib.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 41d01008b3a46..61f6a68e4f134 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -1646,9 +1646,7 @@ impl pallet_session::SessionManager for Module { } Self::new_session(new_index - 1) } - fn end_session(end_index: SessionIndex) { - >::end_session(end_index) - } + fn end_session(_end_index: SessionIndex) {} } impl SessionManager>> for Module { @@ -1662,7 +1660,8 @@ impl SessionManager> }).collect() }) } - fn end_session(_end_index: SessionIndex) { + fn end_session(end_index: SessionIndex) { + >::end_session(end_index) } } From d964a63b4cc3048c1803daa9d612cea4109188ae Mon Sep 17 00:00:00 2001 From: thiolliere Date: Fri, 17 Jan 2020 14:05:10 +0100 Subject: [PATCH 10/11] fix initial session --- frame/session/src/lib.rs | 2 ++ frame/staking/src/lib.rs | 11 ++++++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/frame/session/src/lib.rs b/frame/session/src/lib.rs index d2fb3b7130470..098b5330779ef 100644 --- a/frame/session/src/lib.rs +++ b/frame/session/src/lib.rs @@ -173,6 +173,8 @@ pub trait SessionManager { /// economic conditions as opposed to the old. /// The returned validator set, if any, will not be applied until `new_index`. /// `new_index` is strictly greater than from previous call. + /// + /// The first session start at index 0. fn new_session(new_index: SessionIndex) -> Option>; /// End the session. /// diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 61f6a68e4f134..04356d3dd3d3e 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -1358,6 +1358,15 @@ impl Module { Self::new_era(session_index) } + /// Initialise the first session (and consequently the first era) + fn initial_session(session_index) -> Option> { + // note: `CurrentEraStart` is set in `on_finalize` of the first block because now is not + // available yet. + CurrentEraStartSessionIndex::put(session_index); + BondedEras::mutate(|bonded| bonded.push((current_era, 0))); + Self::select_validators().1 + } + /// The era has changed - enact new staking set. /// /// NOTE: This always happens immediately before a session change to ensure that new validators @@ -1642,7 +1651,7 @@ impl pallet_session::SessionManager for Module { fn new_session(new_index: SessionIndex) -> Option> { Self::ensure_storage_upgraded(); if new_index == 0 { - return >::select_validators().1 + return Self::initial_session(); } Self::new_session(new_index - 1) } From 2fee00f4b1a57a6ada5e607f4b9a10e7cf7e929d Mon Sep 17 00:00:00 2001 From: thiolliere Date: Fri, 17 Jan 2020 18:26:20 +0100 Subject: [PATCH 11/11] fix --- frame/staking/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 04356d3dd3d3e..9bfe2fbf9cd45 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -1359,11 +1359,11 @@ impl Module { } /// Initialise the first session (and consequently the first era) - fn initial_session(session_index) -> Option> { + fn initial_session() -> Option> { // note: `CurrentEraStart` is set in `on_finalize` of the first block because now is not // available yet. - CurrentEraStartSessionIndex::put(session_index); - BondedEras::mutate(|bonded| bonded.push((current_era, 0))); + CurrentEraStartSessionIndex::put(0); + BondedEras::mutate(|bonded| bonded.push((0, 0))); Self::select_validators().1 }