Delay session changes' effects on parachains by 1 block

roadmap/implementers-guide/src/runtime/README.md

@@ -23,17 +23,13 @@ We will split the logic of the runtime up into these modules:
 
 The [Initializer module](initializer.md) is special - it's responsible for handling the initialization logic of the other modules to ensure that the correct initialization order and related invariants are maintained. The other modules won't specify a on-initialize logic, but will instead expose a special semi-private routine that the initialization module will call. The other modules are relatively straightforward and perform the roles described above.
 
-The Parachain Host operates under a changing set of validators. Time is split up into periodic sessions, where each session brings a potentially new set of validators. Sessions are buffered by one, meaning that the validators of the upcoming session are fixed and always known. Parachain Host runtime modules need to react to changes in the validator set, as it will affect the runtime logic for processing candidate backing, availability bitfields, and misbehavior reports. The Parachain Host modules can't determine ahead-of-time exactly when session change notifications are going to happen within the block (note: this depends on module initialization order again - better to put session before parachains modules). Ideally, session changes are always handled before initialization. It is clearly a problem if we compute validator assignments to parachains during initialization and then the set of validators changes. In the best case, we can recognize that re-initialization needs to be done. In the worst case, bugs would occur.
+The Parachain Host operates under a changing set of validators. Time is split up into periodic sessions, where each session brings a potentially new set of validators. Sessions are buffered by one, meaning that the validators of the upcoming session `n+1` are determined at the end of session `n-1`, right before session `n` starts. Parachain Host runtime modules need to react to changes in the validator set, as it will affect the runtime logic for processing candidate backing, availability bitfields, and misbehavior reports. The Parachain Host modules can't determine ahead-of-time exactly when session change notifications are going to happen within the block (note: this depends on module initialization order again - better to put session before parachains modules).
 
-There are 3 main ways that we can handle this issue:
+The relay chain is intended to use BABE or SASSAFRAS, which both have the property that a session changing at a block is determined not by the number of the block but instead by the time the block is authored. In some sense, sessions change in-between blocks, not at blocks. This has the side effect that the session of a child block cannot be determined solely by the parent block's identifier. Being able to unilaterally determine the validator-set at a specific block based on its parent hash would make a lot of Node-side logic much simpler.
 
-1. Establish an invariant that session change notifications always happen after initialization. This means that when we receive a session change notification before initialization, we call the initialization routines before handling the session change.
-1. Require that session change notifications always occur before initialization. Brick the chain if session change notifications ever happen after initialization.
-1. Handle both the before and after cases.
+In order to regain the property that the validator set of a block is predictable by its parent block, we delay session changes' application to Parachains by 1 block. This means that if there is a session change at block X, that session change will be stored and applied during initialization of direct descendents of X. This principal side effect of this change is that the Parachains runtime can disagree with session or consensus modules about which session it currently is. Misbehavior reporting routines in particular will be affected by this, although not severely. The parachains runtime might believe it is the last block of the session while the system is really in the first block of the next session. In such cases, a historical validator-set membership proof will need to accompany any misbehavior report, although they typically do not need to during current-session misbehavior reports.
 
-Although option 3 is the most comprehensive, it runs counter to our goal of simplicity. Option 1 means requiring the runtime to do redundant work at all sessions and will also mean, like option 3, that designing things in such a way that initialization can be rolled back and reapplied under the new environment. That leaves option 2, although it is a "nuclear" option in a way and requires us to constrain the parachain host to only run in full runtimes with a certain order of operations.
-
-So the other role of the initializer module is to forward session change notifications to modules in the initialization order, throwing an unrecoverable error if the notification is received after initialization. Session change is the point at which the [Configuration Module](configuration.md) updates the configuration. Most of the other modules will handle changes in the configuration during their session change operation, so the initializer should provide both the old and new configuration to all the other
+So the other role of the initializer module is to forward session change notifications to modules in the initialization order. Session change is also the point at which the [Configuration Module](configuration.md) updates the configuration. Most of the other modules will handle changes in the configuration during their session change operation, so the initializer should provide both the old and new configuration to all the other
 modules alongside the session change notification. This means that a session change notification should consist of the following data:
 
 ```rust
@@ -53,5 +49,4 @@ struct SessionChangeNotification {
 }
 ```
 
-> REVIEW: other options? arguments in favor of going for options 1 or 3 instead of 2. we could do a "soft" version of 2 where we note that the chain is potentially broken due to bad initialization order
 > TODO Diagram: order of runtime operations (initialization, session change)

roadmap/implementers-guide/src/runtime/initializer.md

@@ -1,16 +1,23 @@
 # Initializer Module
 
-This module is responsible for initializing the other modules in a deterministic order. It also has one other purpose as described above: accepting and forwarding session change notifications.
+This module is responsible for initializing the other modules in a deterministic order. It also has one other purpose as described in the overview of the runtime: accepting and forwarding session change notifications.
 
 ## Storage
 
 ```rust
-HasInitialized: bool
+HasInitialized: bool;
+// buffered session changes along with the block number at which they should be applied.
+//
+// typically this will be empty or one element long. ordered ascending by BlockNumber and insertion
+// order.
+BufferedSessionChanges: Vec<(BlockNumber, ValidatorSet, ValidatorSet)>;
 ```
 
 ## Initialization
 
-The other modules are initialized in this order:
+Before initializing modules, remove all changes from the `BufferedSessionChanges` with number less than or equal to the current block number, and apply the last one. The session change is applied to all modules in the same order as initialization.
+
+The other parachains modules are initialized in this order:
 
 1. Configuration
 1. Paras
@@ -25,8 +32,7 @@ Set `HasInitialized` to true.
 
 ## Session Change
 
-If `HasInitialized` is true, throw an unrecoverable error (panic).
-Otherwise, forward the session change notification to other modules in initialization order.
+Store the session change information in `BufferedSessionChange` along with the block number at which it was submitted, plus one. Although the expected operational parameters of the block authorship system should prevent more than one change from being buffered at any time, it may occur. Regardless, we always need to track the block number at which the session change can be applied so as to remain flexible over session change notifications being issued before or after initialization of the current block.
 
 ## Finalization
 

runtime/parachains/src/initializer.rs

@@ -27,6 +27,8 @@ use primitives::{
 use frame_support::{
 	decl_storage, decl_module, decl_error, traits::Randomness,
 };
+use sp_runtime::traits::One;
+use codec::{Encode, Decode};
 use crate::{configuration::{self, HostConfiguration}, paras, scheduler, inclusion};
 
 /// Information about a session change that has just occurred.
@@ -46,6 +48,14 @@ pub struct SessionChangeNotification<BlockNumber> {
 	pub session_index: sp_staking::SessionIndex,
 }
 
+#[derive(Encode, Decode)]
+struct BufferedSessionChange<N> {
+	apply_at: N,
+	validators: Vec<ValidatorId>,
+	queued: Vec<ValidatorId>,
+	session_index: sp_staking::SessionIndex,
+}
+
 pub trait Trait:
 	system::Trait + configuration::Trait + paras::Trait + scheduler::Trait + inclusion::Trait
 {
@@ -64,6 +74,14 @@ decl_storage! {
 		/// them writes to the trie and one does not. This confusion makes `Option<()>` more suitable for
 		/// the semantics of this variable.
 		HasInitialized: Option<()>;
+		/// Buffered session changes along with the block number at which they should be applied.
+		///
+		/// Typically this will be empty or one element long, with the single element having a block
+		/// number of the next block.
+		///
+		/// However this is a `Vec` regardless to handle various edge cases that may occur at runtime
+		/// upgrade boundaries or if governance intervenes.
+		BufferedSessionChanges: Vec<BufferedSessionChange<T::BlockNumber>>;
 	}
 }
 
@@ -77,6 +95,21 @@ decl_module! {
 		type Error = Error<T>;
 
 		fn on_initialize(now: T::BlockNumber) -> Weight {
+			// Apply buffered session changes before initializing modules, so they
+			// can be initialized with respect to the current validator set.
+			<BufferedSessionChanges<T>>::mutate(|v| {
+				let drain_up_to = v.iter().take_while(|b| b.apply_at <= now).count();
+
+				// apply only the last session as all others lasted less than a block (weirdly).
+				if let Some(buffered) = v.drain(..drain_up_to).last() {
+					Self::apply_new_session(
+						buffered.session_index,
+						buffered.validators,
+						buffered.queued,
+					);
+				}
+			});
+
 			// The other modules are initialized in this order:
 			// - Configuration
 			// - Paras
@@ -106,27 +139,11 @@ decl_module! {
 }
 
 impl<T: Trait> Module<T> {
-	/// Should be called when a new session occurs. Forwards the session notification to all
-	/// wrapped modules. If `queued` is `None`, the `validators` are considered queued.
-	///
-	/// Panics if the modules have already been initialized.
-	fn on_new_session<'a, I: 'a>(
-		_changed: bool,
+	fn apply_new_session(
 		session_index: sp_staking::SessionIndex,
-		validators: I,
-		queued: Option<I>,
-	)
-		where I: Iterator<Item=(&'a T::AccountId, ValidatorId)>
-	{
-		assert!(HasInitialized::get().is_none());
-
-		let validators: Vec<_> = validators.map(|(_, v)| v).collect();
-		let queued: Vec<_> = if let Some(queued) = queued {
-			queued.map(|(_, v)| v).collect()
-		} else {
-			validators.clone()
-		};
-
+		validators: Vec<ValidatorId>,
+		queued: Vec<ValidatorId>,
+	) {
 		let prev_config = <configuration::Module<T>>::config();
 
 		let random_seed = {
@@ -156,6 +173,31 @@ impl<T: Trait> Module<T> {
 		scheduler::Module::<T>::initializer_on_new_session(&notification);
 		inclusion::Module::<T>::initializer_on_new_session(&notification);
 	}
+
+	/// Should be called when a new session occurs. Buffers the session notification to be applied
+	/// at the next block. If `queued` is `None`, the `validators` are considered queued.
+	fn on_new_session<'a, I: 'a>(
+		_changed: bool,
+		session_index: sp_staking::SessionIndex,
+		validators: I,
+		queued: Option<I>,
+	)
+		where I: Iterator<Item=(&'a T::AccountId, ValidatorId)>
+	{
+		let validators: Vec<_> = validators.map(|(_, v)| v).collect();
+		let queued: Vec<_> = if let Some(queued) = queued {
+			queued.map(|(_, v)| v).collect()
+		} else {
+			validators.clone()
+		};
+
+		<BufferedSessionChanges<T>>::mutate(|v| v.push(BufferedSessionChange {
+			apply_at: <system::Module<T>>::block_number() + One::one(),
+			validators,
+			queued,
+			session_index,
+		}));
+	}
 }
 
 impl<T: Trait> sp_runtime::BoundToRuntimeAppPublic for Module<T> {
@@ -184,21 +226,47 @@ impl<T: session::Trait + Trait> session::OneSessionHandler<T::AccountId> for Mod
 #[cfg(test)]
 mod tests {
 	use super::*;
-	use crate::mock::{new_test_ext, Initializer};
+	use crate::mock::{new_test_ext, Initializer, Test, System};
 
 	use frame_support::traits::{OnFinalize, OnInitialize};
 
 	#[test]
-	#[should_panic]
-	fn panics_if_session_changes_after_on_initialize() {
+	fn session_change_before_initialize_is_still_buffered_after() {
+		new_test_ext(Default::default()).execute_with(|| {
+			Initializer::on_new_session(
+				false,
+				1,
+				Vec::new().into_iter(),
+				Some(Vec::new().into_iter()),
+			);
+
+			let now = System::block_number();
+			Initializer::on_initialize(now);
+
+			let v = <BufferedSessionChanges<Test>>::get();
+			assert_eq!(v.len(), 1);
+
+			let apply_at = now + 1;
+			assert_eq!(v[0].apply_at, apply_at);
+		});
+	}
+
+	#[test]
+	fn session_change_applied_on_initialize() {
 		new_test_ext(Default::default()).execute_with(|| {
 			Initializer::on_initialize(1);
+
+			let now = System::block_number();
 			Initializer::on_new_session(
 				false,
 				1,
 				Vec::new().into_iter(),
 				Some(Vec::new().into_iter()),
 			);
+
+			Initializer::on_initialize(now + 1);
+
+			assert!(<BufferedSessionChanges<Test>>::get().is_empty());
 		});
 	}