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

migrations: VersionedRuntimeUpgrade #14311

Merged
merged 46 commits into from
Jul 2, 2023
Merged
Changes from 6 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
0885a4f
VersionedRuntimeUpgrade
liamaharon Jun 6, 2023
9c0072f
only require one version and add a pre-upgrade check
liamaharon Jun 6, 2023
2f30d00
add docs
liamaharon Jun 6, 2023
451de26
improve warning log
liamaharon Jun 6, 2023
d8bc4d7
improve comments
liamaharon Jun 6, 2023
53f6170
fix log
liamaharon Jun 6, 2023
617c864
use associated constants
liamaharon Jun 9, 2023
1b9c68d
allow passing from and to versions
liamaharon Jun 11, 2023
acc9b0c
test versioned_runtime_upgrade
liamaharon Jun 11, 2023
c3638c5
fix typo
liamaharon Jun 11, 2023
f5cab56
improve docs
liamaharon Jun 11, 2023
bd42fbe
docs
liamaharon Jun 11, 2023
0aae7e8
docs
liamaharon Jun 11, 2023
71f7c6c
remove event from dummy pallet
liamaharon Jun 12, 2023
7919778
remove pre_upgrade current storage version check
liamaharon Jun 12, 2023
44d65b0
derive_impl
liamaharon Jun 13, 2023
87ecff5
skip pre/post checks if the actual migration will not run
liamaharon Jun 20, 2023
284a403
improve variable naming
liamaharon Jun 20, 2023
53bf2f1
docs
liamaharon Jun 20, 2023
d34405a
fix post_upgrade 'should run' logic
liamaharon Jun 20, 2023
46f8bfb
fix comments
liamaharon Jun 20, 2023
3c31046
pre and post hook tests
liamaharon Jun 20, 2023
5bdb5b8
feature gate try-runtime stuff
liamaharon Jun 20, 2023
65fd376
Merge branch 'master' of github.com:paritytech/substrate into liam-ve…
liamaharon Jun 20, 2023
9941c7c
remove deprecated macro
liamaharon Jun 20, 2023
6fd6c10
Update frame/support/src/migrations.rs
liamaharon Jun 21, 2023
446b19e
decode_all
liamaharon Jun 21, 2023
2b23890
Merge branch 'liam-versioned-runtime-upgrade' of github.com:paritytec…
liamaharon Jun 21, 2023
53473ee
make experimental
liamaharon Jun 21, 2023
e78ff5a
use rust generics
liamaharon Jun 21, 2023
27f35d1
add info log when running
liamaharon Jun 23, 2023
7f8add6
simplify tests
liamaharon Jun 23, 2023
52b920e
improve log
liamaharon Jun 23, 2023
a16eba7
improve log
liamaharon Jun 23, 2023
c8e0538
cleaner pre_upgrade encoding
liamaharon Jun 23, 2023
2417754
Update frame/support/src/migrations.rs
liamaharon Jun 29, 2023
a72c9b7
Update frame/support/src/migrations.rs
liamaharon Jun 29, 2023
63542cf
Update frame/support/src/migrations.rs
liamaharon Jun 29, 2023
547feec
Update frame/support/src/migrations.rs
liamaharon Jun 29, 2023
8c2050c
Update frame/support/src/migrations.rs
liamaharon Jun 29, 2023
ae484c7
Merge branch 'master' of github.com:paritytech/substrate into liam-ve…
liamaharon Jun 29, 2023
ab880cb
VersionedPostUpgradeData enum
liamaharon Jun 29, 2023
605f63d
move versioned runtime upgrade tests to test/tests
liamaharon Jun 29, 2023
8a9e31c
fix rust doc
liamaharon Jun 29, 2023
e5d4207
Merge branch 'master' of github.com:paritytech/substrate into liam-ve…
liamaharon Jun 30, 2023
888f5eb
clarify comment
liamaharon Jul 2, 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
117 changes: 114 additions & 3 deletions frame/support/src/migrations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,126 @@
// See the License for the specific language governing permissions and
// limitations under the License.

#[cfg(feature = "try-runtime")]
use crate::storage::unhashed::contains_prefixed_key;
use crate::{
traits::{GetStorageVersion, NoStorageVersionSet, PalletInfoAccess, StorageVersion},
traits::{
GetStorageVersion, NoStorageVersionSet, OnRuntimeUpgrade, PalletInfoAccess, StorageVersion,
},
weights::{RuntimeDbWeight, Weight},
};
use impl_trait_for_tuples::impl_for_tuples;
use sp_core::Get;
use sp_io::{hashing::twox_128, storage::clear_prefix, KillStorageResult};
use sp_std::marker::PhantomData;

#[cfg(feature = "try-runtime")]
use sp_std::vec::Vec;

/// Make it easier to write versioned runtime upgrades.
///
/// VersionedRuntimeUpgrade allows developers to write migrations without worrying about checking
liamaharon marked this conversation as resolved.
Show resolved Hide resolved
/// and setting storage versions. Instead, the developer wraps their migration in this struct which
/// takes care of version handling using best practices.
///
/// It takes 4 type parameters:
/// - `Version`: The version being upgraded *from*.
liamaharon marked this conversation as resolved.
Show resolved Hide resolved
/// - `Inner`: An implementation of `OnRuntimeUpgrade`.
/// - `Pallet`: The Pallet being upgraded.
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
/// - `Weight`: The runtime's RuntimeDbWeight implementation.
///
/// `Version` is used to check if the migration should be run. If it is run, the pallets on-chain
/// version is incremented.
///
/// Example:
/// ```ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

possibly not to difficult to make this not be ignore and actually compile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you thinking I'd use the dummy pallet and upgrades from my test file? I'd rather not use a migration from a real pallet since it creates a dependency that could break. I'm also curious what the benefit/s are of making the comment compile

/// parameter_types! {
/// pub const V4ToV5: StorageVersion = StorageVersion::new(4);
/// pub const V5ToV6: StorageVersion = StorageVersion::new(5);
/// }
///
/// // Migrations to pass to the Executive pallet.
/// pub type Migrations = (
/// // ...other migrations
/// VersionedRuntimeUpgrade<
/// V4ToV5,
/// parachains_configuration::migration::v5::MigrateToV5<Runtime>,
/// Configuration,
/// RocksDbWeight,
/// >,
/// VersionedRuntimeUpgrade<
/// V5ToV6,
/// parachains_configuration::migration::v6::MigrateToV6<Runtime>,
liamaharon marked this conversation as resolved.
Show resolved Hide resolved
/// Configuration,
/// RocksDbWeight,
/// >,
/// // ...other migrations
/// );
/// ```
pub struct VersionedRuntimeUpgrade<Version, Inner, Pallet, Weight> {
_marker: PhantomData<(Version, Inner, Pallet, Weight)>,
}

/// Implementation of the `OnRuntimeUpgrade` trait for `VersionedRuntimeUpgrade`.
///
/// Primarily, it
/// 1. Logs a warning in `pre_upgrade` if the storage version specified in the pallet appears to be
/// misconfigured before returning `Inner::pre_upgrade`.
///
/// 2. Performs the actual runtime upgrade in `on_runtime_upgrade` only if the on-chain version of
/// the pallets storage matches `Version`. If the versions do not match, it writes a log notifying
/// the developer that the migration is a noop.
impl<
Version: Get<StorageVersion>,
liamaharon marked this conversation as resolved.
Show resolved Hide resolved
Inner: OnRuntimeUpgrade,
Pallet: GetStorageVersion<CurrentStorageVersion = StorageVersion> + PalletInfoAccess,
DbWeight: Get<RuntimeDbWeight>,
Comment on lines +97 to +98
Copy link
Contributor

Choose a reason for hiding this comment

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

For readability purposes, I would consider moving Pallet and DbWeight to be associated types rather than generics and hardcoded. This might require you to add another trait. When a pallet is implementing this new trait, it will fix its Pallet and DbWeight (both of which can be obtained from system).

Then, in the runtime, when you are wrapping things in VersionedRuntimeUpgrade, you only see (VersionedRuntimeUpgrade<1, 2, Foo>, VersionedRuntimeUpgrade<5, 6, Bar>).

Possibly not worth it, especially as I am still critical of the fundamental approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure how to obtain Pallet from system do you have an example you could point me towards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm able to remove the DbWeight from the runtime file like this, without needing to create a new trait:

before (inside migrations file):

pub type VersionCheckedMigrateV5ToV6<Runtime, Pallet, DbWeight> = VersionedRuntimeUpgrade<
	ConstU16<5>,
	ConstU16<6>,
	VersionUncheckedMigrateV5ToV6<Runtime>,
	Pallet,
	DbWeight,
>;

after (inside migrations file):

pub type VersionCheckedMigrateV5ToV6<Runtime, Pallet> = VersionedRuntimeUpgrade<
	ConstU16<5>,
	ConstU16<6>,
	VersionUncheckedMigrateV5ToV6<Runtime>,
	Pallet,
	<Runtime as frame_system::Config>::DbWeight,
>;

Then in the runtime file, you no longer need to pass the DbWeight:

parachains_configuration::migration::v6::VersionCheckedMigrateV5ToV6<
	Runtime,
	Configuration,
>,

Let me know if this approach is also OK, it seems to keep things clean in the runtime file without needing to create an extra trait, but I'm a noob with associated types so definitely may be missing something, please let me know if I am.

> OnRuntimeUpgrade for VersionedRuntimeUpgrade<Version, Inner, Pallet, DbWeight>
{
/// VersionedRuntimeUpgrade pre-upgrade checks.
///
/// Logs a warning if the storage version specified in the pallet appears to be misconfigured,
/// before returning `Inner::pre_upgrade`.
#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::TryRuntimeError> {
let current_version = Pallet::current_storage_version();
if current_version < Version::get() + 1 {
log::warn!(
"Pallet '{}' appears to be misconfigured: its version is less than the on-chain version that will be set by this VersionedRuntimeUpgrade. Please ensure the pallet version is set correctly.",
Pallet::name()
);
}

Inner::pre_upgrade()
}

/// Actually executes the versioned runtime upgrade.
///
/// First checks if the pallet's on-chain storage version matches the version of this upgrade.
/// If it matches, it calls `Inner::on_runtime_upgrade`, increments the on-chain version, and
/// returns the weight. If it does not match, it writes a log notifying the developer that the
/// migration is a noop.
fn on_runtime_upgrade() -> Weight {
let on_chain_version = Pallet::on_chain_storage_version();
if on_chain_version == Version::get() {
// Execute the migration
let weight = Inner::on_runtime_upgrade();

// Update the on-chain version
let next = Version::get() + 1;
next.put::<Pallet>();

weight.saturating_add(DbWeight::get().reads_writes(1, 1))
} else {
log::warn!(
"{} VersionedOnRuntimeUpgrade {:?} skipped because current on-chain version is {:?}.",
Pallet::name(),
Version::get(),
on_chain_version
);
DbWeight::get().reads(1)
}
}
}

/// Can store the current pallet version in storage.
pub trait StoreCurrentStorageVersion<T: GetStorageVersion + PalletInfoAccess> {
/// Write the current storage version to the storage.
Expand Down Expand Up @@ -185,6 +292,8 @@ impl<P: Get<&'static str>, DbWeight: Get<RuntimeDbWeight>> frame_support::traits

#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::TryRuntimeError> {
use crate::storage::unhashed::contains_prefixed_key;

let hashed_prefix = twox_128(P::get().as_bytes());
match contains_prefixed_key(&hashed_prefix) {
true => log::info!("Found {} keys pre-removal 👀", P::get()),
Expand All @@ -198,6 +307,8 @@ impl<P: Get<&'static str>, DbWeight: Get<RuntimeDbWeight>> frame_support::traits

#[cfg(feature = "try-runtime")]
fn post_upgrade(_state: Vec<u8>) -> Result<(), sp_runtime::TryRuntimeError> {
use crate::storage::unhashed::contains_prefixed_key;

let hashed_prefix = twox_128(P::get().as_bytes());
match contains_prefixed_key(&hashed_prefix) {
true => {
Expand Down