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

Remove xcm on_runtime_upgrade pallet hook #7235

Merged
merged 15 commits into from
Aug 5, 2023
Merged

Remove xcm on_runtime_upgrade pallet hook #7235

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
518f7df
move migration stuffs
pgherveou May 15, 2023
35c0800
Apply suggestions from code review
pgherveou May 17, 2023
4e3f15f
Merge branch 'master' into pg/move-xcm-upgrade
pgherveou May 30, 2023
c4fb47b
Merge branch 'master' into pg/move-xcm-upgrade
pgherveou Jun 5, 2023
bc41403
Fix test
pgherveou Jun 5, 2023
011f017
fix
pgherveou Jun 5, 2023
a8772b8
lint
pgherveou Jun 5, 2023
f07d01a
fix lint
pgherveou Jun 5, 2023
fa9ad9a
rm extra space
pgherveou Jun 5, 2023
6fad87b
fix lint
pgherveou Jun 5, 2023
92a2d9f
Merge branch 'master' into pg/move-xcm-upgrade
pgherveou Aug 1, 2023
f279726
PR review
pgherveou Aug 4, 2023
bd6615b
fixes
pgherveou Aug 4, 2023
ed26d11
use saturating_accrue in fn
pgherveou Aug 4, 2023
6f58baf
fix test
pgherveou Aug 4, 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
6 changes: 0 additions & 6 deletions xcm/pallet-xcm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,12 +697,6 @@ pub mod pallet {
}
weight_used
}
fn on_runtime_upgrade() -> Weight {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure if the migration has already happened in all of the runtimes, but removing this here without adding the migration back to the runtimes would prevent any migration from happening. Have you checked what the onchain storage version is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's hard to check for all of them. Every migration should be tested with try-runtime which will yell at you if there is a storage mismatch for any pallet

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a list of runtimes we can check off before merging this?

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 initially did that in this PR, but after reviews decided to only apply it to the chain I was upgrading (contracts-rococo)
paritytech/cumulus#2570, (see also other earlier commit where I apply it to all runtimes paritytech/cumulus@d890570)

The reasoning is that adding a migration that have already been applied, adds some unnecessary bloat to the wasm runtime, so the best practice should be to test with try-runtime and see what needs to be included.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the basic ones that we care about the most would do, e.g. polkadot, kusama, westend, rococo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

following up, here since @vstam1 is trying to close all pending issues before the repo migration.
This logic is moved to the Migration Struct that should be included in the frame_executive::Executive when a migration needs to be passed.

My understanding is that on_runtime_upgrade hooks are deprecated and the migration logic should move out of the pallet into these Migration structs so that they can be controlled and configured by runtime authors.

This is why I opened this PR in the first place, so that pallet_xcm adheres to this new conventions and integrate better with tools like try-runtime

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but what we're saying here in this comment is whether or not the existing runtimes that we care about is already on storage version 1.

Copy link
Contributor Author

@pgherveou pgherveou Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this should impact whether or not we merge this PR?
The code here was moved to MigrateToV1.
If one runtime is not on v1 then it should run the upgrade by passing MigrateToV1 to frame_executive
if it's already on V1, then it should not use MigrateToV1.
In both scenarios dry-running the migration with try-runtime will tell you if something is missing or superfluous.

Feel free to close to unblock the repo migration if I got that wrong

// Start a migration (this happens before on_initialize so it'll happen later in this
// block, which should be good enough)...
CurrentMigration::<T>::put(VersionMigrationStage::default());
T::DbWeight::get().writes(1)
}
}

pub mod migrations {
Expand Down
14 changes: 10 additions & 4 deletions xcm/pallet-xcm/src/migration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ pub const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
const DEFAULT_PROOF_SIZE: u64 = 64 * 1024;

pub mod v1 {
use crate::{CurrentMigration, VersionMigrationStage};

use super::*;

pub struct MigrateToV1<T>(sp_std::marker::PhantomData<T>);
Expand All @@ -38,8 +40,11 @@ pub mod v1 {
}

fn on_runtime_upgrade() -> Weight {
CurrentMigration::<T>::put(VersionMigrationStage::default());
liamaharon marked this conversation as resolved.
Show resolved Hide resolved
let mut weight = T::DbWeight::get().writes(1);

if StorageVersion::get::<Pallet<T>>() == 0 {
let mut weight = T::DbWeight::get().reads(1);
weight.saturating_accrue(T::DbWeight::get().reads(1));

let translate = |pre: (u64, u64, u32)| -> Option<(u64, Weight, u32)> {
weight = weight.saturating_add(T::DbWeight::get().reads_writes(1, 1));
Expand All @@ -52,12 +57,13 @@ pub mod v1 {

log::info!("v1 applied successfully");
STORAGE_VERSION.put::<Pallet<T>>();
pgherveou marked this conversation as resolved.
Show resolved Hide resolved

weight.saturating_add(T::DbWeight::get().writes(1))
weight.saturating_add(T::DbWeight::get().writes(1));
} else {
log::warn!("skipping v1, should be removed");
T::DbWeight::get().reads(1)
weight.saturating_accrue(T::DbWeight::get().reads(1));
}

weight
}
}
}