Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TryDecodeEntireState check for storage types and pallets #1805

Merged
merged 31 commits into from
Nov 6, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
292b747
patch outcome
PieWol Oct 5, 2023
5ebb6db
remove patchfile
PieWol Oct 6, 2023
5d20d23
Merge branch 'paritytech:master' into decode-all
PieWol Oct 6, 2023
c6128fa
Merge branch 'master' into decode-all
ggwpez Oct 21, 2023
ee5b1df
Make stuff compile
ggwpez Oct 21, 2023
720ebb2
Introduce custom error type
ggwpez Oct 21, 2023
993c2c7
Move stuff into own files
ggwpez Oct 21, 2023
61883ba
Cleanup
ggwpez Oct 21, 2023
1cf9ae2
Add prdoc
ggwpez Oct 21, 2023
7e4394d
Update UI tests
ggwpez Oct 21, 2023
30569ac
Add to Rococo and Westend runtimes
ggwpez Oct 21, 2023
30bb3b1
Add log
ggwpez Oct 21, 2023
e2e22e3
fmt
ggwpez Oct 21, 2023
25741db
Remove migration struct and call directly from executive
ggwpez Oct 24, 2023
4890794
Fix
ggwpez Oct 24, 2023
1c34c2d
Nicer error print
ggwpez Oct 24, 2023
b4851d4
Merge remote-tracking branch 'origin/master' into decode-all
ggwpez Oct 24, 2023
b003d2e
Fix debug print
ggwpez Oct 24, 2023
3c9907a
Fix docs
ggwpez Oct 24, 2023
e00cab5
Merge branch 'master' into decode-all
ggwpez Oct 24, 2023
ba1a5bb
Merge branch 'master' into decode-all
ggwpez Oct 24, 2023
022e1e7
Apply suggestions from code review
ggwpez Oct 26, 2023
4623145
Return all errors
ggwpez Oct 26, 2023
d7688b2
Also try to decode the state in try_execute_block
ggwpez Oct 26, 2023
fbb1e25
Tweak error print
ggwpez Oct 26, 2023
81885ff
Fix CI
ggwpez Oct 26, 2023
2d8b214
log extended info to debug
ggwpez Oct 27, 2023
5472c3e
Prefix runtime log target
ggwpez Nov 6, 2023
41056ab
Merge remote-tracking branch 'origin/master' into decode-all
ggwpez Nov 6, 2023
3119539
Resolve unused import
ggwpez Nov 6, 2023
ac80d5d
Merge branch 'master' into decode-all
ggwpez Nov 6, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion polkadot/runtime/rococo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1407,7 +1407,7 @@ pub type UncheckedExtrinsic =
///
/// This contains the combined migrations of the last 10 releases. It allows to skip runtime
/// upgrades in case governance decides to do so. THE ORDER IS IMPORTANT.
pub type Migrations = migrations::Unreleased;
pub type Migrations = (migrations::Unreleased, migrations::Perpetual);

/// The runtime migrations per release.
#[allow(deprecated, missing_docs)]
Expand Down Expand Up @@ -1488,6 +1488,12 @@ pub mod migrations {
frame_support::migrations::RemovePallet<TechnicalMembershipPalletName, <Runtime as frame_system::Config>::DbWeight>,
frame_support::migrations::RemovePallet<TipsPalletName, <Runtime as frame_system::Config>::DbWeight>,
);

/// Migrations that should run in *every* upgrade. Do not change on release!
pub type Perpetual = (
// This must be last:
frame_support::storage::migration::EnsureStateDecodes<AllPalletsWithSystem>,
);
}

/// Executive: handles dispatch to the various modules.
Expand Down
8 changes: 7 additions & 1 deletion polkadot/runtime/westend/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1527,7 +1527,7 @@ impl Get<Perbill> for NominationPoolsMigrationV4OldPallet {
///
/// This contains the combined migrations of the last 10 releases. It allows to skip runtime
/// upgrades in case governance decides to do so. THE ORDER IS IMPORTANT.
pub type Migrations = migrations::Unreleased;
pub type Migrations = (migrations::Unreleased, migrations::Perpetual);

/// The runtime migrations per release.
#[allow(deprecated, missing_docs)]
Expand Down Expand Up @@ -1558,6 +1558,12 @@ pub mod migrations {
pallet_referenda::migration::v1::MigrateV0ToV1<Runtime, ()>,
pallet_nomination_pools::migration::versioned_migrations::V6ToV7<Runtime>,
);

/// Migrations that should run in *every* upgrade. Do not change on release!
pub type Perpetual = (
// This must be last:
frame_support::storage::migration::EnsureStateDecodes<AllPalletsWithSystem>,
);
}

/// Unchecked extrinsic type as expected by this runtime.
Expand Down
23 changes: 23 additions & 0 deletions prdoc/pr_1805.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
title: Introduce state decoding check after runtime upgrades.

doc:
- audience: Core Dev
description: |
Adds a `TryDecodeEntireState` migration that can be optionally included into the list of migrations of a runtime. It will try to decode the state of all pallets to spot any decoding errors that might have been introduced by missing a migration.

migrations:
db: []

runtime: []

crates:
- name: frame-support
semver: minor
- name: frame-support-procedural
semver: minor
- name: kitchensink-runtime
semver: minor
- name: node-template-runtime
semver: minor

host_functions: []
9 changes: 9 additions & 0 deletions substrate/bin/node-template/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,14 @@ pub type SignedExtra = (
pallet_transaction_payment::ChargeTransactionPayment<Runtime>,
);

/// All migrations of the runtime, aside from the ones declared in the pallets.
///
/// This can be a tuple of types, each implementing `OnRuntimeUpgrade`. Add other migration types
/// before `EnsureStateDecodes` as needed -- this is only for testing, and
// should come last.
#[allow(unused_parens)]
type Migrations = (frame_support::migration::EnsureStateDecodes<AllPalletsWithSystem>);

/// Unchecked extrinsic type as expected by this runtime.
pub type UncheckedExtrinsic =
generic::UncheckedExtrinsic<Address, RuntimeCall, Signature, SignedExtra>;
Expand All @@ -324,6 +332,7 @@ pub type Executive = frame_executive::Executive<
frame_system::ChainContext<Runtime>,
Runtime,
AllPalletsWithSystem,
Migrations,
>;

#[cfg(feature = "runtime-benchmarks")]
Expand Down
2 changes: 2 additions & 0 deletions substrate/bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2181,6 +2181,8 @@ type Migrations = (
pallet_nomination_pools::migration::v2::MigrateToV2<Runtime>,
pallet_alliance::migration::Migration<Runtime>,
pallet_contracts::Migration<Runtime>,
// This should always be the last migration item.
frame_support::storage::migration::EnsureStateDecodes<AllPalletsWithSystem>,
);

type EventRecord = frame_system::EventRecord<
Expand Down
5 changes: 2 additions & 3 deletions substrate/frame/glutton/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,8 @@
//!
//! # Glutton Pallet
//!
//! Pallet that consumes `ref_time` and `proof_size` of a block. Based on the
//! `Compute` and `Storage` parameters the pallet consumes the adequate amount
//! of weight.
//! Pallet that consumes `ref_time` and `proof_size` of a block. Based on the `Compute` and
//! `Storage` parameters the pallet consumes the adequate amount of weight.

#![deny(missing_docs)]
#![cfg_attr(not(feature = "std"), no_std)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,12 @@ pub fn expand_event(def: &mut Def) -> proc_macro2::TokenStream {
let trait_use_gen = &def.trait_use_generics(event.attr_span);
let type_impl_gen = &def.type_impl_generics(event.attr_span);
let type_use_gen = &def.type_use_generics(event.attr_span);
let pallet_ident = &def.pallet_struct.pallet;

let PalletEventDepositAttr { fn_vis, fn_span, .. } = deposit_event;

quote::quote_spanned!(*fn_span =>
impl<#type_impl_gen> Pallet<#type_use_gen> #completed_where_clause {
impl<#type_impl_gen> #pallet_ident<#type_use_gen> #completed_where_clause {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was on purpose, you think its wrong?

#fn_vis fn deposit_event(event: Event<#event_use_gen>) {
let event = <
<T as Config #trait_use_gen>::RuntimeEvent as
Expand Down
47 changes: 47 additions & 0 deletions substrate/frame/support/procedural/src/pallet/expand/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -822,12 +822,57 @@ pub fn expand_storages(def: &mut Def) -> proc_macro2::TokenStream {
)
});

// aggregated where clause of all storage types and the whole pallet.
let mut where_clauses = vec![&def.config.where_clause];
where_clauses.extend(def.storages.iter().map(|storage| &storage.where_clause));
let completed_where_clause = super::merge_where_clauses(&where_clauses);
let type_impl_gen = &def.type_impl_generics(proc_macro2::Span::call_site());
let type_use_gen = &def.type_use_generics(proc_macro2::Span::call_site());

let try_decode_entire_state = {
let mut storage_names = def
.storages
.iter()
.filter_map(|storage| {
if storage.cfg_attrs.is_empty() {
let ident = &storage.ident;
let gen = &def.type_use_generics(storage.attr_span);
Some(quote::quote_spanned!(storage.attr_span => #ident<#gen> ))
} else {
None
}
})
.collect::<Vec<_>>();
storage_names.sort_by_cached_key(|ident| ident.to_string());

quote::quote!(
#[cfg(feature = "try-runtime")]
impl<#type_impl_gen> #frame_support::traits::TryDecodeEntireStorage
for #pallet_ident<#type_use_gen> #completed_where_clause
{
fn try_decode_entire_state() -> Result<usize, #frame_support::traits::TryDecodeEntireStorageError> {
let pallet_name = <<T as #frame_system::Config>::PalletInfo as frame_support::traits::PalletInfo>
::name::<#pallet_ident<#type_use_gen>>()
.expect("Every active pallet has a name in the runtime; qed");

#frame_support::__private::log::debug!(target: "try-decode-state", "trying to decode pallet: {pallet_name}");

// NOTE: for now, we have to exclude storage items that are feature gated.
let mut decoded = 0usize;
#(
#frame_support::__private::log::debug!(target: "try-decode-state", "trying to decode storage: \
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
{pallet_name}::{}", stringify!(#storage_names));

decoded +=
<#storage_names as #frame_support::traits::TryDecodeEntireStorage>::try_decode_entire_state()?;
)*
ggwpez marked this conversation as resolved.
Show resolved Hide resolved

Ok(decoded)
}
}
)
};

quote::quote!(
impl<#type_impl_gen> #pallet_ident<#type_use_gen>
#completed_where_clause
Expand All @@ -853,5 +898,7 @@ pub fn expand_storages(def: &mut Def) -> proc_macro2::TokenStream {
#( #getters )*
#( #prefix_structs )*
#( #on_empty_structs )*

#try_decode_entire_state
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ pub enum QueryKind {
/// `type MyStorage = StorageValue<MyStorageP, u32>`
/// The keys and values types are parsed in order to get metadata
pub struct StorageDef {
/// The index of error item in pallet module.
/// The index of storage item in pallet module.
pub index: usize,
/// Visibility of the storage type.
pub vis: syn::Visibility,
Expand Down
6 changes: 3 additions & 3 deletions substrate/frame/support/src/storage/generator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@
//!
//! This is internal api and is subject to change.

mod double_map;
pub(crate) mod double_map;
pub(crate) mod map;
mod nmap;
mod value;
pub(crate) mod nmap;
pub(crate) mod value;

pub use double_map::StorageDoubleMap;
pub use map::StorageMap;
Expand Down
45 changes: 45 additions & 0 deletions substrate/frame/support/src/storage/migration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,51 @@ pub fn move_prefix(from_prefix: &[u8], to_prefix: &[u8]) {
}
}

/// A phony migration that does nothing, except executing `TryDecodeEntireStorage` on
/// `post_upgrade`, which implies it is only available if `try-state` feature is used.
///
/// This can be used typically in the top level runtime, where `AllPallets` typically comes from
/// `construct_runtime!`.
pub struct EnsureStateDecodes<AllPallets>(sp_std::marker::PhantomData<AllPallets>);
ggwpez marked this conversation as resolved.
Show resolved Hide resolved

#[cfg(not(feature = "try-runtime"))]
impl<AllPallets> crate::traits::OnRuntimeUpgrade for EnsureStateDecodes<AllPallets> {
fn on_runtime_upgrade() -> crate::weights::Weight {
Default::default()
}
}

#[cfg(feature = "try-runtime")]
impl<AllPallets: crate::traits::TryDecodeEntireStorage> crate::traits::OnRuntimeUpgrade
for EnsureStateDecodes<AllPallets>
{
fn on_runtime_upgrade() -> sp_weights::Weight {
Default::default()
}

fn post_upgrade(_: Vec<u8>) -> Result<(), sp_runtime::TryRuntimeError> {
let decoded = match AllPallets::try_decode_entire_state() {
Ok(bytes) => bytes,
Err(err) => {
log::info!(
target: crate::LOG_TARGET,
"failed to decode the entire state: {}", err
);
// NOTE: This only supports static strings, so we cannot bubble up the exact error.
return Err("failed to decode a value from the storage".into())
},
};

log::info!(
target: crate::LOG_TARGET,
"decoded the entire state, total size = {} bytes",
decoded
);

Ok(())
}
}

#[cfg(test)]
mod tests {
use super::{
Expand Down
14 changes: 9 additions & 5 deletions substrate/frame/support/src/storage/types/counted_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,11 @@ impl<P: CountedStorageMapInstance, H, K, V, Q, O, M> MapWrapper
type Map = StorageMap<P, H, K, V, Q, O, M>;
}

type CounterFor<P> = StorageValue<<P as CountedStorageMapInstance>::CounterPrefix, u32, ValueQuery>;
/// The numeric counter type.
pub type Counter = u32;

type CounterFor<P> =
StorageValue<<P as CountedStorageMapInstance>::CounterPrefix, Counter, ValueQuery>;

/// On removal logic for updating counter while draining upon some prefix with
/// [`crate::storage::PrefixIterator`].
Expand Down Expand Up @@ -378,14 +382,14 @@ where
/// can be very heavy, so use with caution.
///
/// Returns the number of items in the map which is used to set the counter.
pub fn initialize_counter() -> u32 {
let count = Self::iter_values().count() as u32;
pub fn initialize_counter() -> Counter {
let count = Self::iter_values().count() as Counter;
CounterFor::<Prefix>::set(count);
count
}

/// Return the count.
pub fn count() -> u32 {
pub fn count() -> Counter {
CounterFor::<Prefix>::get()
}
}
Expand Down Expand Up @@ -1162,7 +1166,7 @@ mod test {
StorageEntryMetadataIR {
name: "counter_for_foo",
modifier: StorageEntryModifierIR::Default,
ty: StorageEntryTypeIR::Plain(scale_info::meta_type::<u32>()),
ty: StorageEntryTypeIR::Plain(scale_info::meta_type::<Counter>()),
default: vec![0, 0, 0, 0],
docs: if cfg!(feature = "no-metadata-docs") {
vec![]
Expand Down
6 changes: 4 additions & 2 deletions substrate/frame/support/src/storage/types/counted_nmap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,10 @@ impl<P: CountedStorageNMapInstance, K, V, Q, O, M> MapWrapper
type Map = StorageNMap<P, K, V, Q, O, M>;
}

type Counter = super::counted_map::Counter;

type CounterFor<P> =
StorageValue<<P as CountedStorageNMapInstance>::CounterPrefix, u32, ValueQuery>;
StorageValue<<P as CountedStorageNMapInstance>::CounterPrefix, Counter, ValueQuery>;

/// On removal logic for updating counter while draining upon some prefix with
/// [`crate::storage::PrefixIterator`].
Expand Down Expand Up @@ -429,7 +431,7 @@ where
}

/// Return the count.
pub fn count() -> u32 {
pub fn count() -> Counter {
CounterFor::<Prefix>::get()
}
}
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/support/src/storage/types/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ mod map;
mod nmap;
mod value;

pub use counted_map::{CountedStorageMap, CountedStorageMapInstance};
pub use counted_map::{CountedStorageMap, CountedStorageMapInstance, Counter};
pub use counted_nmap::{CountedStorageNMap, CountedStorageNMapInstance};
pub use double_map::StorageDoubleMap;
pub use key::{
Expand Down
4 changes: 2 additions & 2 deletions substrate/frame/support/src/storage/types/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use crate::{
types::{OptionQuery, QueryKindTrait, StorageEntryMetadataBuilder},
StorageAppend, StorageDecodeLength, StorageTryAppend,
},
traits::{GetDefault, StorageInfo, StorageInstance},
traits::{Get, GetDefault, StorageInfo, StorageInstance},
};
use codec::{Decode, Encode, EncodeLike, FullCodec, MaxEncodedLen};
use sp_arithmetic::traits::SaturatedConversion;
Expand All @@ -46,7 +46,7 @@ where
Prefix: StorageInstance,
Value: FullCodec,
QueryKind: QueryKindTrait<Value, OnEmpty>,
OnEmpty: crate::traits::Get<QueryKind::Query> + 'static,
OnEmpty: Get<QueryKind::Query> + 'static,
{
type Query = QueryKind::Query;
fn pallet_prefix() -> &'static [u8] {
Expand Down
5 changes: 4 additions & 1 deletion substrate/frame/support/src/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,4 +125,7 @@ pub use tx_pause::{TransactionPause, TransactionPauseError};
#[cfg(feature = "try-runtime")]
mod try_runtime;
#[cfg(feature = "try-runtime")]
pub use try_runtime::{Select as TryStateSelect, TryState, UpgradeCheckSelect};
pub use try_runtime::{
Select as TryStateSelect, TryDecodeEntireStorage, TryDecodeEntireStorageError, TryState,
UpgradeCheckSelect,
};
Loading
Loading