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
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions frame/support/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ runtime-benchmarks = [
try-runtime = [
"sp-debug-derive/force-debug"
]
experimental = []
# By default some types have documentation, `no-metadata-docs` allows to reduce the documentation
# in the metadata.
no-metadata-docs = ["frame-support-procedural/no-metadata-docs", "sp-api/no-metadata-docs"]
Expand Down
145 changes: 143 additions & 2 deletions frame/support/src/migrations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
// 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},
weights::{RuntimeDbWeight, Weight},
Expand All @@ -25,9 +23,148 @@ 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;

#[cfg(feature = "experimental")]
use crate::traits::OnRuntimeUpgrade;

/// Make it easier to write versioned runtime upgrades.
///
/// [`VersionedRuntimeUpgrade`] allows developers to write migrations without worrying about
/// checking and setting storage versions. Instead, the developer wraps their migration in this
/// struct which takes care of version handling using best practices.
///
/// It takes 5 type parameters:
/// - `From`: The version being upgraded from.
/// - `To`: The version being upgraded to.
/// - `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.
///
/// When a [`VersionedRuntimeUpgrade`] `on_runtime_upgrade`, `pre_upgrade`, or `post_upgrade`
/// method is called, the on-chain version of the pallet is compared to `From`. If they match, the
/// `Inner` equivalent is called and the pallets on-chain version is set to `To` after the
/// migration. Otherwise, a warning is logged notifying the developer that the upgrade was a noop
/// and should probably be removed.
///
/// ### Examples
/// ```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

/// // In file defining migrations
/// pub struct VersionUncheckedMigrateV5ToV6<T>(sp_std::marker::PhantomData<T>);
/// impl<T: Config> OnRuntimeUpgrade for VersionUncheckedMigrateV5ToV6<T> {
/// // OnRuntimeUpgrade implementation...
/// }
///
/// pub type VersionCheckedMigrateV5ToV6<Runtime, Pallet, DbWeight> =
/// VersionedRuntimeUpgrade<5, 6, VersionUncheckedMigrateV5ToV6<Runtime>, Pallet, DbWeight>;
///
/// // Migrations tuple to pass to the Executive pallet:
/// pub type Migrations = (
/// // other migrations...
/// VersionCheckedMigrateV5ToV6<Runtime, Balances, RuntimeDbWeight>,
/// // other migrations...
/// );
/// ```
#[cfg(feature = "experimental")]
pub struct VersionedRuntimeUpgrade<const FROM: u16, const TO: u16, Inner, Pallet, Weight> {
_marker: PhantomData<(Inner, Pallet, Weight)>,
}

/// A helper enum to wrap the pre_upgrade bytes like an Option before passing them to post_upgrade.
/// This enum is used rather than an Option to make the API clearer to the developer.
#[cfg(feature = "experimental")]
#[derive(codec::Encode, codec::Decode)]
pub enum VersionedPostUpgradeData {
/// The migration ran, inner vec contains pre_upgrade data.
MigrationExecuted(Vec<u8>),
/// This migration is a noop, do not run post_upgrade checks.
Noop,
}

/// Implementation of the `OnRuntimeUpgrade` trait for `VersionedRuntimeUpgrade`.
///
/// Its main function is to perform the runtime upgrade in `on_runtime_upgrade` only if the on-chain
/// version of the pallets storage matches `From`, and after the upgrade set the on-chain storage to
/// `To`. If the versions do not match, it writes a log notifying the developer that the migration
/// is a noop.
#[cfg(feature = "experimental")]
impl<
const FROM: u16,
const TO: u16,
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<FROM, TO, Inner, Pallet, DbWeight>
{
/// Executes pre_upgrade if the migration will run, and wraps the pre_upgrade bytes in
/// [`VersionedPostUpgradeData`] before passing them to post_upgrade, so it knows whether the
/// migration ran or not.
#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::TryRuntimeError> {
use codec::Encode;
let on_chain_version = Pallet::on_chain_storage_version();
if on_chain_version == FROM {
Ok(VersionedPostUpgradeData::MigrationExecuted(Inner::pre_upgrade()?).encode())
} else {
Ok(VersionedPostUpgradeData::Noop.encode())
}
}

/// Executes the versioned runtime upgrade.
///
/// First checks if the pallets on-chain storage version matches the version of this upgrade. If
/// it matches, it calls `Inner::on_runtime_upgrade`, updates 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 == FROM {
log::info!(
"Running {} VersionedOnRuntimeUpgrade: version {:?} to {:?}.",
Pallet::name(),
FROM,
TO
);

// Execute the migration
let weight = Inner::on_runtime_upgrade();

// Update the on-chain version
StorageVersion::new(TO).put::<Pallet>();

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

/// Executes `Inner::post_upgrade` if the migration just ran.
///
/// pre_upgrade passes [`VersionedPostUpgradeData::MigrationExecuted`] to post_upgrade if
/// the migration ran, and [`VersionedPostUpgradeData::Noop`] otherwise.
#[cfg(feature = "try-runtime")]
fn post_upgrade(
versioned_post_upgrade_data_bytes: Vec<u8>,
) -> Result<(), sp_runtime::TryRuntimeError> {
use codec::DecodeAll;
match <VersionedPostUpgradeData>::decode_all(&mut &versioned_post_upgrade_data_bytes[..])
.map_err(|_| "VersionedRuntimeUpgrade post_upgrade failed to decode PreUpgradeData")?
{
VersionedPostUpgradeData::MigrationExecuted(inner_bytes) =>
Inner::post_upgrade(inner_bytes),
VersionedPostUpgradeData::Noop => Ok(()),
}
}
}

/// 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 +322,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 +337,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
1 change: 1 addition & 0 deletions frame/support/test/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ std = [
"sp-version/std",
"test-pallet/std",
]
experimental = ["frame-support/experimental"]
try-runtime = [
"frame-support/try-runtime",
"frame-system/try-runtime",
Expand Down
Loading