Unverified Commit e21f5cec authored by Sergey Pepyakin's avatar Sergey Pepyakin Committed by GitHub
Browse files

Do not use rely on the block initialization when calling runtime APIs (#2123)



* Don't initialize block when calling runtime APIs

* Adapt check_validation_outputs

We split the code path for the inclusion and for the commitments checking.

* Slap #[skip_initialize_block] on safe runtime APIs

That is, those that should not be affected by this attribute

* Make `Scheduled` not ephemeral

So that it is persisted in the storage and ready to be inspected
by the runtime APIs. This is in contrast to what was before, where we
would remove the storage entry and then rely on the scheduling performed
by `on_initialize` again.

* Add a big fat comment

* Typos
Co-authored-by: asynchronous rob's avatarRobert Habermeier <rphmeier@gmail.com>

* Move session change to the end of the current block

Previously, it was the beginning of the next block. This allows us to
put #[skip_initialize_block]

* Update tests

* Fix a test in paras registrar

Also refactor it a bit so the next time there are more chances this kind
of issue is diagnosed quicker.

* Add for_runtime_api to inclusion's check_validation_outputs
Co-authored-by: asynchronous rob's avatarRobert Habermeier <rphmeier@gmail.com>
parent 4685d8f4
Pipeline #117863 passed with stages
in 28 minutes and 53 seconds
......@@ -719,16 +719,37 @@ pub struct SessionInfo {
sp_api::decl_runtime_apis! {
/// The API for querying the state of parachains on-chain.
pub trait ParachainHost<H: Decode = Hash, N: Encode + Decode = BlockNumber> {
// NOTE: Many runtime API are declared with `#[skip_initialize_block]`. This is because without
// this attribute before each runtime call, the `initialize_block` runtime API will be called.
// That in turns will lead to two things:
//
// (a) The frame_system module will be initialized to the next block.
// (b) Initialization sequences for each runtime module (pallet) will be run.
//
// (a) is undesirable because the runtime APIs are querying the state against a specific
// block state. However, due to that initialization the observed block number would be as if
// it was the next block.
//
// We dont want (b) mainly because block initialization can be very heavy. Upgrade enactment,
// storage migration, and whatever other logic exists in `on_initialize` will be executed
// if not explicitly opted out with the `#[skip_initialize_block]` attribute.
//
// Additionally, some runtime APIs may depend on state that is pruned on the `on_initialize`.
// At the moment of writing, this is `candidate_events`.
/// Get the current validators.
#[skip_initialize_block]
fn validators() -> Vec<ValidatorId>;
/// Returns the validator groups and rotation info localized based on the block whose state
/// this is invoked on. Note that `now` in the `GroupRotationInfo` should be the successor of
/// the number of the block.
#[skip_initialize_block]
fn validator_groups() -> (Vec<Vec<ValidatorIndex>>, GroupRotationInfo<N>);
/// Yields information on all availability cores. Cores are either free or occupied. Free
/// cores can have paras assigned to them.
#[skip_initialize_block]
fn availability_cores() -> Vec<CoreState<H, N>>;
/// Yields the full validation data for the given ParaId along with an assumption that
......@@ -736,6 +757,7 @@ sp_api::decl_runtime_apis! {
///
/// Returns `None` if either the para is not registered or the assumption is `Freed`
/// and the para already occupies a core.
#[skip_initialize_block]
fn full_validation_data(para_id: Id, assumption: OccupiedCoreAssumption)
-> Option<ValidationData<N>>;
......@@ -744,24 +766,29 @@ sp_api::decl_runtime_apis! {
///
/// Returns `None` if either the para is not registered or the assumption is `Freed`
/// and the para already occupies a core.
#[skip_initialize_block]
fn persisted_validation_data(para_id: Id, assumption: OccupiedCoreAssumption)
-> Option<PersistedValidationData<N>>;
/// Checks if the given validation outputs pass the acceptance criteria.
#[skip_initialize_block]
fn check_validation_outputs(para_id: Id, outputs: CandidateCommitments) -> bool;
/// Returns the session index expected at a child of the block.
///
/// This can be used to instantiate a `SigningContext`.
#[skip_initialize_block]
fn session_index_for_child() -> SessionIndex;
/// Get the session info for the given session, if stored.
#[skip_initialize_block]
fn session_info(index: SessionIndex) -> Option<SessionInfo>;
/// Fetch the validation code used by a para, making the given `OccupiedCoreAssumption`.
///
/// Returns `None` if either the para is not registered or the assumption is `Freed`
/// and the para already occupies a core.
#[skip_initialize_block]
fn validation_code(para_id: Id, assumption: OccupiedCoreAssumption)
-> Option<ValidationCode>;
......@@ -770,26 +797,28 @@ sp_api::decl_runtime_apis! {
///
/// `context_height` may be no greater than the height of the block in whose
/// state the runtime API is executed.
#[skip_initialize_block]
fn historical_validation_code(para_id: Id, context_height: N)
-> Option<ValidationCode>;
/// Get the receipt of a candidate pending availability. This returns `Some` for any paras
/// assigned to occupied cores in `availability_cores` and `None` otherwise.
#[skip_initialize_block]
fn candidate_pending_availability(para_id: Id) -> Option<CommittedCandidateReceipt<H>>;
/// Get a vector of events concerning candidates that occurred within a block.
// NOTE: this needs to skip block initialization as events are wiped within block
// initialization.
#[skip_initialize_block]
fn candidate_events() -> Vec<CandidateEvent<H>>;
/// Get all the pending inbound messages in the downward message queue for a para.
#[skip_initialize_block]
fn dmq_contents(
recipient: Id,
) -> Vec<InboundDownwardMessage<N>>;
/// Get the contents of all channels addressed to the given recipient. Channels that have no
/// messages in them are also included.
#[skip_initialize_block]
fn inbound_hrmp_channels_contents(recipient: Id) -> BTreeMap<Id, Vec<InboundHrmpMessage<N>>>;
}
}
......
......@@ -162,7 +162,7 @@ AvailabilityCores: Vec<Option<CoreOccupied>>;
ParathreadClaimIndex: Vec<ParaId>;
/// The block number where the session start occurred. Used to track how many group rotations have occurred.
SessionStartBlock: BlockNumber;
/// Currently scheduled cores - free but up to be occupied. Ephemeral storage item that's wiped on finalization.
/// Currently scheduled cores - free but up to be occupied.
Scheduled: Vec<CoreAssignment>, // sorted ascending by CoreIndex.
```
......@@ -190,13 +190,12 @@ Actions:
## Initialization
1. Free all scheduled cores and return parathread claims to queue, with retries incremented.
1. Schedule free cores using the `schedule(Vec::new())`.
## Finalization
Actions:
1. Free all scheduled cores and return parathread claims to queue, with retries incremented.
No finalization routine runs for this module.
## Routines
......
......@@ -579,13 +579,9 @@ mod tests {
t.into()
}
fn init_block() {
println!("Initializing {}", System::block_number());
System::on_initialize(System::block_number());
Initializer::on_initialize(System::block_number());
}
fn run_to_block(n: BlockNumber) {
// NOTE that this function only simulates modules of interest. Depending on new module may
// require adding it here.
println!("Running until block {}", n);
while System::block_number() < n {
let b = System::block_number();
......@@ -593,9 +589,11 @@ mod tests {
if System::block_number() > 1 {
println!("Finalizing {}", System::block_number());
System::on_finalize(System::block_number());
Initializer::on_finalize(System::block_number());
}
// Session change every 3 blocks.
if (b + 1) % 3 == 0 {
println!("New session at {}", System::block_number());
Initializer::on_new_session(
false,
Vec::new().into_iter(),
......@@ -603,7 +601,9 @@ mod tests {
);
}
System::set_block_number(b + 1);
init_block();
println!("Initializing {}", System::block_number());
System::on_initialize(System::block_number());
Initializer::on_initialize(System::block_number());
}
}
......@@ -725,8 +725,9 @@ mod tests {
WASM_MAGIC.to_vec().into(),
).is_err());
run_to_block(6);
// The session will be changed on the 6th block, as part of finalization. The change
// will be observed on the 7th.
run_to_block(7);
assert_ok!(Registrar::register_parachain(
1u32.into(),
vec![1; 3].into(),
......
......@@ -399,7 +399,12 @@ impl<T: Config> Module<T> {
let validators = Validators::get();
let parent_hash = <frame_system::Module<T>>::parent_hash();
let check_cx = CandidateCheckContext::<T>::new();
// At the moment we assume (and in fact enforce, below) that the relay-parent is always one
// before of the block where we include a candidate (i.e. this code path).
let now = <frame_system::Module<T>>::block_number();
let relay_parent_number = now - One::one();
let check_cx = CandidateCheckContext::<T>::new(now, relay_parent_number);
// do all checks before writing storage.
let core_indices_and_backers = {
......@@ -483,7 +488,10 @@ impl<T: Config> Module<T> {
{
// this should never fail because the para is registered
let persisted_validation_data =
match crate::util::make_persisted_validation_data::<T>(para_id) {
match crate::util::make_persisted_validation_data::<T>(
para_id,
relay_parent_number,
) {
Some(l) => l,
None => {
// We don't want to error out here because it will
......@@ -592,7 +600,7 @@ impl<T: Config> Module<T> {
hash: candidate_hash,
descriptor,
availability_votes,
relay_parent_number: check_cx.relay_parent_number,
relay_parent_number,
backers,
backed_in_number: check_cx.now,
});
......@@ -603,11 +611,17 @@ impl<T: Config> Module<T> {
}
/// Run the acceptance criteria checks on the given candidate commitments.
pub(crate) fn check_validation_outputs(
pub(crate) fn check_validation_outputs_for_runtime_api(
para_id: ParaId,
validation_outputs: primitives::v1::CandidateCommitments,
) -> bool {
if let Err(err) = CandidateCheckContext::<T>::new().check_validation_outputs(
// This function is meant to be called from the runtime APIs against the relay-parent, hence
// `relay_parent_number` is equal to `now`.
let now = <frame_system::Module<T>>::block_number();
let relay_parent_number = now;
let check_cx = CandidateCheckContext::<T>::new(now, relay_parent_number);
if let Err(err) = check_cx.check_validation_outputs(
para_id,
&validation_outputs.head_data,
&validation_outputs.new_validation_code,
......@@ -812,12 +826,11 @@ struct CandidateCheckContext<T: Config> {
}
impl<T: Config> CandidateCheckContext<T> {
fn new() -> Self {
let now = <frame_system::Module<T>>::block_number();
fn new(now: T::BlockNumber, relay_parent_number: T::BlockNumber) -> Self {
Self {
config: <configuration::Module<T>>::config(),
now,
relay_parent_number: now - One::one(),
relay_parent_number,
}
}
......@@ -1111,8 +1124,12 @@ mod tests {
}
fn make_vdata_hash(para_id: ParaId) -> Option<Hash> {
let relay_parent_number = <frame_system::Module<Test>>::block_number() - 1;
let persisted_validation_data
= crate::util::make_persisted_validation_data::<Test>(para_id)?;
= crate::util::make_persisted_validation_data::<Test>(
para_id,
relay_parent_number,
)?;
Some(persisted_validation_data.hash())
}
......
......@@ -25,7 +25,6 @@ use primitives::v1::ValidatorId;
use frame_support::{
decl_storage, decl_module, decl_error, traits::Randomness,
};
use sp_runtime::traits::One;
use parity_scale_codec::{Encode, Decode};
use crate::{
configuration::{self, HostConfiguration},
......@@ -63,8 +62,7 @@ impl<BlockNumber: Default + From<u32>> Default for SessionChangeNotification<Blo
}
#[derive(Encode, Decode)]
struct BufferedSessionChange<N> {
apply_at: N,
struct BufferedSessionChange {
validators: Vec<ValidatorId>,
queued: Vec<ValidatorId>,
session_index: sp_staking::SessionIndex,
......@@ -98,12 +96,12 @@ decl_storage! {
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.
/// Typically this will be empty or one element long. Apart from that this item never hits
/// the storage.
///
/// 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>>;
BufferedSessionChanges: Vec<BufferedSessionChange>;
}
}
......@@ -117,21 +115,6 @@ 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
......@@ -158,7 +141,6 @@ decl_module! {
fn on_finalize() {
// reverse initialization order.
hrmp::Module::<T>::initializer_finalize();
ump::Module::<T>::initializer_finalize();
dmp::Module::<T>::initializer_finalize();
......@@ -167,6 +149,20 @@ decl_module! {
scheduler::Module::<T>::initializer_finalize();
paras::Module::<T>::initializer_finalize();
configuration::Module::<T>::initializer_finalize();
// Apply buffered session changes as the last thing. This way the runtime APIs and the
// next block will observe the next session.
//
// Note that we only apply the last session as all others lasted less than a block (weirdly).
if let Some(BufferedSessionChange {
session_index,
validators,
queued,
}) = BufferedSessionChanges::take().pop()
{
Self::apply_new_session(session_index, validators, queued);
}
HasInitialized::take();
}
}
......@@ -213,7 +209,7 @@ impl<T: Config> Module<T> {
}
/// 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.
/// at the end of the block. If `queued` is `None`, the `validators` are considered queued.
fn on_new_session<'a, I: 'a>(
_changed: bool,
session_index: sp_staking::SessionIndex,
......@@ -229,8 +225,7 @@ impl<T: Config> Module<T> {
validators.clone()
};
<BufferedSessionChanges<T>>::mutate(|v| v.push(BufferedSessionChange {
apply_at: <frame_system::Module<T>>::block_number() + One::one(),
BufferedSessionChanges::mutate(|v| v.push(BufferedSessionChange {
validators,
queued,
session_index,
......@@ -264,7 +259,7 @@ impl<T: pallet_session::Config + Config> pallet_session::OneSessionHandler<T::Ac
#[cfg(test)]
mod tests {
use super::*;
use crate::mock::{new_test_ext, Initializer, Test, System};
use crate::mock::{new_test_ext, Initializer, System};
use frame_support::traits::{OnFinalize, OnInitialize};
......@@ -281,20 +276,15 @@ mod tests {
let now = System::block_number();
Initializer::on_initialize(now);
let v = <BufferedSessionChanges<Test>>::get();
let v = <BufferedSessionChanges>::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() {
fn session_change_applied_on_finalize() {
new_test_ext(Default::default()).execute_with(|| {
Initializer::on_initialize(1);
let now = System::block_number();
Initializer::on_new_session(
false,
1,
......@@ -302,9 +292,9 @@ mod tests {
Some(Vec::new().into_iter()),
);
Initializer::on_initialize(now + 1);
Initializer::on_finalize(1);
assert!(<BufferedSessionChanges<Test>>::get().is_empty());
assert!(<BufferedSessionChanges>::get().is_empty());
});
}
......
......@@ -193,12 +193,13 @@ pub fn full_validation_data<T: initializer::Config>(
)
-> Option<ValidationData<T::BlockNumber>>
{
let relay_parent_number = <frame_system::Module<T>>::block_number();
with_assumption::<T, _, _>(
para_id,
assumption,
|| Some(ValidationData {
persisted: crate::util::make_persisted_validation_data::<T>(para_id)?,
transient: crate::util::make_transient_validation_data::<T>(para_id)?,
persisted: crate::util::make_persisted_validation_data::<T>(para_id, relay_parent_number)?,
transient: crate::util::make_transient_validation_data::<T>(para_id, relay_parent_number)?,
}),
)
}
......@@ -208,10 +209,11 @@ pub fn persisted_validation_data<T: initializer::Config>(
para_id: ParaId,
assumption: OccupiedCoreAssumption,
) -> Option<PersistedValidationData<T::BlockNumber>> {
let relay_parent_number = <frame_system::Module<T>>::block_number();
with_assumption::<T, _, _>(
para_id,
assumption,
|| crate::util::make_persisted_validation_data::<T>(para_id),
|| crate::util::make_persisted_validation_data::<T>(para_id, relay_parent_number),
)
}
......@@ -220,7 +222,7 @@ pub fn check_validation_outputs<T: initializer::Config>(
para_id: ParaId,
outputs: primitives::v1::CandidateCommitments,
) -> bool {
<inclusion::Module<T>>::check_validation_outputs(para_id, outputs)
<inclusion::Module<T>>::check_validation_outputs_for_runtime_api(para_id, outputs)
}
/// Implementation for the `session_index_for_child` function of the runtime API.
......
......@@ -182,7 +182,7 @@ decl_storage! {
ParathreadClaimIndex: Vec<ParaId>;
/// The block number where the session start occurred. Used to track how many group rotations have occurred.
SessionStartBlock get(fn session_start_block): T::BlockNumber;
/// Currently scheduled cores - free but up to be occupied. Ephemeral storage item that's wiped on finalization.
/// Currently scheduled cores - free but up to be occupied.
///
/// Bounded by the number of cores: one for each parachain and parathread multiplexer.
Scheduled get(fn scheduled): Vec<CoreAssignment>; // sorted ascending by CoreIndex.
......@@ -203,13 +203,6 @@ decl_module! {
impl<T: Config> Module<T> {
/// Called by the initializer to initialize the scheduler module.
pub(crate) fn initializer_initialize(_now: T::BlockNumber) -> Weight {
Self::schedule(Vec::new());
0
}
/// Called by the initializer to finalize the scheduler module.
pub(crate) fn initializer_finalize() {
// Free all scheduled cores and return parathread claims to queue, with retries incremented.
let config = <configuration::Module<T>>::config();
ParathreadQueue::mutate(|queue| {
......@@ -225,10 +218,16 @@ impl<T: Config> Module<T> {
}
}
}
})
});
Self::schedule(Vec::new());
0
}
/// Called by the initializer to finalize the scheduler module.
pub(crate) fn initializer_finalize() {}
/// Called by the initializer to note that a new session has started.
pub(crate) fn initializer_on_new_session(notification: &SessionChangeNotification<T::BlockNumber>) {
let &SessionChangeNotification {
......
......@@ -17,19 +17,19 @@
//! Utilities that don't belong to any particular module but may draw
//! on all modules.
use sp_runtime::traits::{One, Saturating};
use sp_runtime::traits::Saturating;
use primitives::v1::{Id as ParaId, PersistedValidationData, TransientValidationData};
use crate::{configuration, paras, dmp, hrmp};
/// Make the persisted validation data for a particular parachain.
/// Make the persisted validation data for a particular parachain and a specified relay-parent.
///
/// This ties together the storage of several modules.
pub fn make_persisted_validation_data<T: paras::Config + hrmp::Config>(
para_id: ParaId,
relay_parent_number: T::BlockNumber,
) -> Option<PersistedValidationData<T::BlockNumber>> {
let config = <configuration::Module<T>>::config();
let relay_parent_number = <frame_system::Module<T>>::block_number() - One::one();
Some(PersistedValidationData {
parent_head: <paras::Module<T>>::para_head(&para_id)?,
......@@ -40,14 +40,14 @@ pub fn make_persisted_validation_data<T: paras::Config + hrmp::Config>(
})
}
/// Make the transient validation data for a particular parachain.
/// Make the transient validation data for a particular parachain and a specified relay-parent.
///
/// This ties together the storage of several modules.
pub fn make_transient_validation_data<T: paras::Config + dmp::Config>(
para_id: ParaId,
relay_parent_number: T::BlockNumber,
) -> Option<TransientValidationData<T::BlockNumber>> {
let config = <configuration::Module<T>>::config();
let relay_parent_number = <frame_system::Module<T>>::block_number() - One::one();
let freq = config.validation_upgrade_frequency;
let delay = config.validation_upgrade_delay;
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment