Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Improve handling of unset StorageVersion #13417

Merged
merged 16 commits into from
May 4, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

32 changes: 32 additions & 0 deletions frame/support/procedural/src/pallet/expand/hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,36 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream {
proc_macro2::TokenStream::new()
};

// If a storage version is set, we should ensure that the storage version on chain matches the
// current storage version. This assumes that `Executive` is running custom migrations before
// the pallets are called.
let post_storage_version_check = if def.pallet_struct.storage_version.is_some() {
quote::quote! {
let on_chain_version = <Self as #frame_support::traits::GetStorageVersion>::on_chain_storage_version();
let current_version = <Self as #frame_support::traits::GetStorageVersion>::current_storage_version();

if on_chain_version != current_version {
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
let pallet_name = <
<T as #frame_system::Config>::PalletInfo
as
#frame_support::traits::PalletInfo
>::name::<Self>().unwrap_or("<unknown pallet name>");

#frame_support::log::error!(
target: #frame_support::LOG_TARGET,
"{}: On chain storage version {:?} doesn't match current storage version {:?}.",
pallet_name,
on_chain_version,
current_version,
);

return Err("On chain and current storage version do not match. Missing runtime upgrade?");
bkchr marked this conversation as resolved.
Show resolved Hide resolved
}
}
} else {
quote::quote! {}
};

quote::quote_spanned!(span =>
#hooks_impl

Expand Down Expand Up @@ -170,6 +200,8 @@ pub fn expand_hooks(def: &mut Def) -> proc_macro2::TokenStream {

#[cfg(feature = "try-runtime")]
fn post_upgrade(state: #frame_support::sp_std::vec::Vec<u8>) -> Result<(), &'static str> {
#post_storage_version_check

<
Self
as
Expand Down
20 changes: 13 additions & 7 deletions frame/support/procedural/src/pallet/expand/pallet_struct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,15 @@ pub fn expand_pallet_struct(def: &mut Def) -> proc_macro2::TokenStream {
}
);

let storage_version = if let Some(v) = def.pallet_struct.storage_version.as_ref() {
quote::quote! { #v }
} else {
quote::quote! { #frame_support::traits::StorageVersion::default() }
};
let (storage_version, current_storage_version_ty) =
bkchr marked this conversation as resolved.
Show resolved Hide resolved
if let Some(v) = def.pallet_struct.storage_version.as_ref() {
(quote::quote! { #v }, quote::quote! { #frame_support::traits::StorageVersion })
} else {
(
quote::quote! { core::default::Default::default() },
quote::quote! { #frame_support::traits::NoStorageVersionSet },
)
};

let whitelisted_storage_idents: Vec<syn::Ident> = def
.storages
Expand Down Expand Up @@ -199,7 +203,9 @@ pub fn expand_pallet_struct(def: &mut Def) -> proc_macro2::TokenStream {
for #pallet_ident<#type_use_gen>
#config_where_clause
{
fn current_storage_version() -> #frame_support::traits::StorageVersion {
type CurrentStorageVersion = #current_storage_version_ty;

fn current_storage_version() -> Self::CurrentStorageVersion {
#storage_version
}

Expand All @@ -214,7 +220,7 @@ pub fn expand_pallet_struct(def: &mut Def) -> proc_macro2::TokenStream {
#config_where_clause
{
fn on_genesis() {
let storage_version = #storage_version;
let storage_version: #frame_support::traits::StorageVersion = #storage_version;
storage_version.put::<Self>();
}
}
Expand Down
38 changes: 26 additions & 12 deletions frame/support/src/dispatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2520,14 +2520,26 @@ macro_rules! decl_module {
impl<$trait_instance: $trait_name $(<I>, $instance: $instantiable)?> $crate::traits::GetStorageVersion
for $module<$trait_instance $(, $instance)?> where $( $other_where_bounds )*
{
fn current_storage_version() -> $crate::traits::StorageVersion {
type CurrentStorageVersion = $crate::traits::StorageVersion;

fn current_storage_version() -> Self::CurrentStorageVersion {
$( $storage_version )*
}

fn on_chain_storage_version() -> $crate::traits::StorageVersion {
$crate::traits::StorageVersion::get::<Self>()
}
}

// Implement `OnGenesis` for `Module`
impl<$trait_instance: $trait_name $(<I>, $instance: $instantiable)?> $crate::traits::OnGenesis
for $module<$trait_instance $(, $instance)?> where $( $other_where_bounds )*
{
fn on_genesis() {
let storage_version = <Self as $crate::traits::GetStorageVersion>::current_storage_version();
storage_version.put::<Self>();
}
}
};

// Implementation for `GetStorageVersion` when no storage version is passed.
Expand All @@ -2539,14 +2551,26 @@ macro_rules! decl_module {
impl<$trait_instance: $trait_name $(<I>, $instance: $instantiable)?> $crate::traits::GetStorageVersion
for $module<$trait_instance $(, $instance)?> where $( $other_where_bounds )*
{
fn current_storage_version() -> $crate::traits::StorageVersion {
type CurrentStorageVersion = $crate::traits::NoStorageVersionSet;

fn current_storage_version() -> Self::CurrentStorageVersion {
Default::default()
}

fn on_chain_storage_version() -> $crate::traits::StorageVersion {
$crate::traits::StorageVersion::get::<Self>()
}
}

// Implement `OnGenesis` for `Module`
impl<$trait_instance: $trait_name $(<I>, $instance: $instantiable)?> $crate::traits::OnGenesis
for $module<$trait_instance $(, $instance)?> where $( $other_where_bounds )*
{
fn on_genesis() {
let storage_version = $crate::traits::StorageVersion::default();
storage_version.put::<Self>();
}
}
};

// The main macro expansion that actually renders the module code.
Expand Down Expand Up @@ -2834,16 +2858,6 @@ macro_rules! decl_module {
}
}

// Implement `OnGenesis` for `Module`
impl<$trait_instance: $trait_name $(<I>, $instance: $instantiable)?> $crate::traits::OnGenesis
for $mod_type<$trait_instance $(, $instance)?> where $( $other_where_bounds )*
{
fn on_genesis() {
let storage_version = <Self as $crate::traits::GetStorageVersion>::current_storage_version();
storage_version.put::<Self>();
}
}

// manual implementation of clone/eq/partialeq because using derive erroneously requires
// clone/eq/partialeq from T.
impl<$trait_instance: $trait_name $(<I>, $instance: $instantiable)?> $crate::dispatch::Clone
Expand Down
32 changes: 28 additions & 4 deletions frame/support/src/migrations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,41 @@
// limitations under the License.

use crate::{
traits::{GetStorageVersion, PalletInfoAccess},
traits::{GetStorageVersion, NoStorageVersionSet, PalletInfoAccess, StorageVersion},
weights::{RuntimeDbWeight, Weight},
};
use impl_trait_for_tuples::impl_for_tuples;

pub trait StoreCurrentStorageVersion<T: GetStorageVersion + PalletInfoAccess> {
bkchr marked this conversation as resolved.
Show resolved Hide resolved
fn store_current_storage_version();
bkchr marked this conversation as resolved.
Show resolved Hide resolved
}

impl<T: GetStorageVersion<CurrentStorageVersion = StorageVersion> + PalletInfoAccess>
StoreCurrentStorageVersion<T> for StorageVersion
{
fn store_current_storage_version() {
let version = <T as GetStorageVersion>::current_storage_version();
version.put::<T>();
}
}

impl<T: GetStorageVersion<CurrentStorageVersion = NoStorageVersionSet> + PalletInfoAccess>
StoreCurrentStorageVersion<T> for NoStorageVersionSet
{
fn store_current_storage_version() {
StorageVersion::default().put::<T>();
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
}
}

/// Trait used by [`migrate_from_pallet_version_to_storage_version`] to do the actual migration.
pub trait PalletVersionToStorageVersionHelper {
fn migrate(db_weight: &RuntimeDbWeight) -> Weight;
}

impl<T: GetStorageVersion + PalletInfoAccess> PalletVersionToStorageVersionHelper for T {
impl<T: GetStorageVersion + PalletInfoAccess> PalletVersionToStorageVersionHelper for T
where
T::CurrentStorageVersion: StoreCurrentStorageVersion<T>,
{
fn migrate(db_weight: &RuntimeDbWeight) -> Weight {
const PALLET_VERSION_STORAGE_KEY_POSTFIX: &[u8] = b":__PALLET_VERSION__:";

Expand All @@ -36,8 +60,8 @@ impl<T: GetStorageVersion + PalletInfoAccess> PalletVersionToStorageVersionHelpe

sp_io::storage::clear(&pallet_version_key(<T as PalletInfoAccess>::name()));

let version = <T as GetStorageVersion>::current_storage_version();
version.put::<T>();
<T::CurrentStorageVersion as StoreCurrentStorageVersion<T>>::store_current_storage_version(
);

db_weight.writes(2)
}
Expand Down
6 changes: 3 additions & 3 deletions frame/support/src/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,9 @@ pub use randomness::Randomness;

mod metadata;
pub use metadata::{
CallMetadata, CrateVersion, GetCallMetadata, GetCallName, GetStorageVersion, PalletInfo,
PalletInfoAccess, PalletInfoData, PalletsInfoAccess, StorageVersion,
STORAGE_VERSION_STORAGE_KEY_POSTFIX,
CallMetadata, CrateVersion, GetCallMetadata, GetCallName, GetStorageVersion,
NoStorageVersionSet, PalletInfo, PalletInfoAccess, PalletInfoData, PalletsInfoAccess,
StorageVersion, STORAGE_VERSION_STORAGE_KEY_POSTFIX,
};

mod hooks;
Expand Down
22 changes: 21 additions & 1 deletion frame/support/src/traits/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,16 @@ impl PartialOrd<u16> for StorageVersion {
}
}

/// Special marker struct if no storage version is set for a pallet.
///
/// If you (the reader) end up here, it probably means that you tried to compare
/// [`GetStorageVersion::on_chain_storage_version`] against
/// [`GetStorageVersion::current_storage_version`]. This basically means that the
/// [`storage_version`](crate::pallet_macros::storage_version) is missing in the pallet where the
/// mentioned functions are being called.
#[derive(Debug, Default)]
pub struct NoStorageVersionSet;
ggwpez marked this conversation as resolved.
Show resolved Hide resolved

/// Provides information about the storage version of a pallet.
///
/// It differentiates between current and on-chain storage version. Both should be only out of sync
Expand All @@ -236,8 +246,18 @@ impl PartialOrd<u16> for StorageVersion {
///
/// It is required to update the on-chain storage version manually when a migration was applied.
pub trait GetStorageVersion {
/// This will be filled out by the [`pallet`](crate::pallet) macro.
///
/// If the [`storage_version`](crate::pallet_macros::storage_version) attribute isn't given
/// this is set to [`NoStorageVersionSet`] to inform the user that the attribute is missing.
/// This should prevent that the user forgets to set a storage version when required. However,
/// this will only work when the user actually tries to call [`Self::current_storage_version`]
/// to compare it against the [`Self::on_chain_storage_version`]. If the attribute is given,
/// this will be set to [`StorageVersion`].
type CurrentStorageVersion;

/// Returns the current storage version as supported by the pallet.
fn current_storage_version() -> StorageVersion;
fn current_storage_version() -> Self::CurrentStorageVersion;
/// Returns the on-chain storage version of the pallet as stored in the storage.
fn on_chain_storage_version() -> StorageVersion;
}
Expand Down
8 changes: 7 additions & 1 deletion frame/support/test/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ trybuild = { version = "1.0.74", features = [ "diff" ] }
pretty_assertions = "1.2.1"
rustversion = "1.0.6"
frame-system = { version = "4.0.0-dev", default-features = false, path = "../../system" }
frame-executive = { version = "4.0.0-dev", default-features = false, path = "../../executive" }
# The "std" feature for this pallet is never activated on purpose, in order to test construct_runtime error message
test-pallet = { package = "frame-support-test-pallet", default-features = false, path = "pallet" }

Expand All @@ -38,6 +39,7 @@ std = [
"codec/std",
"scale-info/std",
"frame-benchmarking/std",
"frame-executive/std",
"frame-support/std",
"frame-system/std",
"sp-core/std",
Expand All @@ -48,7 +50,11 @@ std = [
"sp-arithmetic/std",
"sp-version/std",
]
try-runtime = ["frame-support/try-runtime"]
try-runtime = [
"frame-support/try-runtime",
"frame-system/try-runtime",
"frame-executive/try-runtime",
]
# WARNING:
# Only CI runs with this feature enabled. This feature is for testing stuff related to the FRAME macros
# in conjunction with rust features.
Expand Down
Loading