From 3f4b89b8269971cac53eeb35ec31a169e11eca9d Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 3 Jul 2020 19:29:49 -0400 Subject: [PATCH 1/7] note that the initializer is responsible for buffering session changes --- roadmap/implementors-guide/src/runtime/README.md | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/roadmap/implementors-guide/src/runtime/README.md b/roadmap/implementors-guide/src/runtime/README.md index 2a806d4c2dbc..247f3aca0485 100644 --- a/roadmap/implementors-guide/src/runtime/README.md +++ b/roadmap/implementors-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 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). -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. -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) From a2b7f171261c10a4de2117f4bd9a46eb3fbbcdd2 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 3 Jul 2020 19:36:05 -0400 Subject: [PATCH 2/7] amend initializer definition to include session change buffering --- .../implementors-guide/src/runtime/initializer.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/roadmap/implementors-guide/src/runtime/initializer.md b/roadmap/implementors-guide/src/runtime/initializer.md index aba4d5f352ab..30cd7c9b968f 100644 --- a/roadmap/implementors-guide/src/runtime/initializer.md +++ b/roadmap/implementors-guide/src/runtime/initializer.md @@ -1,16 +1,19 @@ # 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; +BufferedSessionChange: Option<(ValidatorSet, ValidatorSet)>; // (new, queued) ``` ## Initialization -The other modules are initialized in this order: +Before initializing modules, we apply the `BufferedSessionChange`, if any, and remove it from storage. 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 +28,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`. If there is already a value present, it should be overwritten. The only way there can already be a value present is if either 2 session changes occur within one block, or one session-change happens after initialization and then another happens before initialization in the next block. In either case, although both are far outside of the expected operational parameters of the chain, there is no time occupied where the clobbered validator set can reasonably be in charge of any validation work, so we do not lose anything by clobbering it. ## Finalization From 61eb5c7c29899c665d4018ae37f7d74df611f051 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 3 Jul 2020 19:44:47 -0400 Subject: [PATCH 3/7] support buffered changes before `on_initialize` --- roadmap/implementors-guide/src/runtime/initializer.md | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/roadmap/implementors-guide/src/runtime/initializer.md b/roadmap/implementors-guide/src/runtime/initializer.md index 30cd7c9b968f..5fd2bc3bd60f 100644 --- a/roadmap/implementors-guide/src/runtime/initializer.md +++ b/roadmap/implementors-guide/src/runtime/initializer.md @@ -6,12 +6,16 @@ This module is responsible for initializing the other modules in a deterministic ```rust HasInitialized: bool; -BufferedSessionChange: Option<(ValidatorSet, ValidatorSet)>; // (new, queued) +// 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 -Before initializing modules, we apply the `BufferedSessionChange`, if any, and remove it from storage. The session change is applied to all modules in the same order as initialization. +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: @@ -28,7 +32,7 @@ Set `HasInitialized` to true. ## Session Change -Store the session change information in `BufferedSessionChange`. If there is already a value present, it should be overwritten. The only way there can already be a value present is if either 2 session changes occur within one block, or one session-change happens after initialization and then another happens before initialization in the next block. In either case, although both are far outside of the expected operational parameters of the chain, there is no time occupied where the clobbered validator set can reasonably be in charge of any validation work, so we do not lose anything by clobbering it. +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 From 1e558e308851f51e8bf1d8e8158c44cb9209ffdd Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 3 Jul 2020 20:06:15 -0400 Subject: [PATCH 4/7] implement and test session buffering --- runtime/parachains/src/initializer.rs | 114 ++++++++++++++++++++------ 1 file changed, 91 insertions(+), 23 deletions(-) diff --git a/runtime/parachains/src/initializer.rs b/runtime/parachains/src/initializer.rs index 5668a216f061..8cde52cca162 100644 --- a/runtime/parachains/src/initializer.rs +++ b/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 { pub session_index: sp_staking::SessionIndex, } +#[derive(Encode, Decode)] +struct BufferedSessionChange { + apply_at: N, + validators: Vec, + queued: Vec, + 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>; } } @@ -77,6 +95,21 @@ decl_module! { type Error = Error; 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. + >::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 Module { - /// 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, - ) - where I: Iterator - { - 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, + queued: Vec, + ) { let prev_config = >::config(); let random_seed = { @@ -156,6 +173,31 @@ impl Module { scheduler::Module::::initializer_on_new_session(¬ification); inclusion::Module::::initializer_on_new_session(¬ification); } + + /// 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, + ) + where I: Iterator + { + let validators: Vec<_> = validators.map(|(_, v)| v).collect(); + let queued: Vec<_> = if let Some(queued) = queued { + queued.map(|(_, v)| v).collect() + } else { + validators.clone() + }; + + >::mutate(|v| v.push(BufferedSessionChange { + apply_at: >::block_number() + One::one(), + validators, + queued, + session_index, + })); + } } impl sp_runtime::BoundToRuntimeAppPublic for Module { @@ -184,21 +226,47 @@ impl session::OneSessionHandler 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 = >::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!(>::get().is_empty()); }); } From 85bf8d3d3fd579c28cfc95fbefdf01ac057ca067 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Mon, 6 Jul 2020 11:17:29 -0400 Subject: [PATCH 5/7] Update roadmap/implementors-guide/src/runtime/README.md Co-authored-by: Bernhard Schuster --- roadmap/implementors-guide/src/runtime/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roadmap/implementors-guide/src/runtime/README.md b/roadmap/implementors-guide/src/runtime/README.md index 247f3aca0485..af1a68d42d5e 100644 --- a/roadmap/implementors-guide/src/runtime/README.md +++ b/roadmap/implementors-guide/src/runtime/README.md @@ -23,7 +23,7 @@ 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). +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). 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. From a7a406e52fbd05c7cfd37eb4d04da153ac7f9956 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Mon, 6 Jul 2020 14:24:46 -0400 Subject: [PATCH 6/7] expand on how this affects misbehavior reports --- roadmap/implementors-guide/src/runtime/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roadmap/implementors-guide/src/runtime/README.md b/roadmap/implementors-guide/src/runtime/README.md index af1a68d42d5e..1bf5064325c7 100644 --- a/roadmap/implementors-guide/src/runtime/README.md +++ b/roadmap/implementors-guide/src/runtime/README.md @@ -27,7 +27,7 @@ The Parachain Host operates under a changing set of validators. Time is split up 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. -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. +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 in 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. 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: From 93960d13af1a5fcb39810f8a25413c4263282ccb Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Mon, 6 Jul 2020 14:25:50 -0400 Subject: [PATCH 7/7] fix typo --- roadmap/implementors-guide/src/runtime/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roadmap/implementors-guide/src/runtime/README.md b/roadmap/implementors-guide/src/runtime/README.md index 1bf5064325c7..5279752e911c 100644 --- a/roadmap/implementors-guide/src/runtime/README.md +++ b/roadmap/implementors-guide/src/runtime/README.md @@ -27,7 +27,7 @@ The Parachain Host operates under a changing set of validators. Time is split up 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. -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 in 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. +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. 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: